Closed
Bug 1177323
Opened 9 years ago
Closed 9 years ago
reftest layout/reftests/image/image-orientation-border-image.html?90&flip fails on Linux-e10s with APZ enabled
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: kats, Assigned: alessarik)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
When APZ is enabled (e.g. [1]), the reftest at layout/reftests/image/image-orientation-border-image.html?90&flip fails on many platforms including OS X and Windows. This is particularly bizarre because the reftests run without e10s and so APZ should have no effect but for some reason it does.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8739d63c549e
Assignee | ||
Comment 1•9 years ago
|
||
Some investigation:
As I correctly understand failed reftests, they check equality of two pictures.
Looks like APZ has bad influence on rendering pictures, unfortunately.
1. At normal case we can see square with four borders.
In case APZ(enabled) we can see squere with only three borders (in most of times)
2. If we refresh page with mouse (with or without [shift] pressed button) we see three borders.
But if we put cursor into adressline and press [enter] (refresh page) we can see four borders again.
4. Test has value "27". And we cannot see left border of square.
If change value to "20", we will not see to top border of square.
For all another values "1-19,21-30" we cannot see left border of square.
Flags: needinfo?(bugmail.mozilla)
Reporter | ||
Comment 2•9 years ago
|
||
Thanks for looking into this. Let me know when you find the root cause of the problem.
Assignee: nobody → alessarik
Flags: needinfo?(bugmail.mozilla)
Reporter | ||
Comment 3•9 years ago
|
||
FWIW see bug 1178274; that should fix half of this bug (the failures on Mac and Windows). I'll leave this bug for the Linux e10s failure - I'm pretty sure the problem here is that the decode-only-on-draw flag gets set to true at http://mxr.mozilla.org/mozilla-central/source/image/ImageFactory.cpp?rev=cf2d7e67dab0#57 and the reftest harness is taking a snapshot before the image has been decoded and drawn.
Summary: reftest layout/reftests/image/image-orientation-border-image.html?90&flip fails on various platforms with APZ enabled → reftest layout/reftests/image/image-orientation-border-image.html?90&flip fails on Linux-e10s with APZ enabled
Reporter | ||
Updated•9 years ago
|
No longer blocks: apz-windows, 1157746
Updated•9 years ago
|
tracking-e10s:
--- → -
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> ... and the reftest harness is taking a snapshot before the image has been decoded and drawn.
The reason is not related with snapshot during reftest harness.
Such behavior reproduced at real time on browser (I mean without reftest harness).
Assignee | ||
Comment 5•9 years ago
|
||
Some investigation:
The reason really can be related with time, when image will be fully decoded.
Tests showed that if "decode-immediately" preference set to TRUE (in that case decode-only-on-draw will be FALSE) we will get correct behavior (at least at real time on browser).
So I can propose the simple idea for patch:
> if(AsyncPanZoomEnabled)
> decode-immediately = true;
Flags: needinfo?(bugmail.mozilla)
Reporter | ||
Comment 6•9 years ago
|
||
That seems like it will defeat the purpose of the decode-on-draw code. Seth, any thoughts?
Flags: needinfo?(bugmail.mozilla) → needinfo?(seth)
Assignee | ||
Comment 7•9 years ago
|
||
Some investigation:
Tests on LINUX showed that after SHIFT+RELOAD we can see black border instead of picture. And after second picture was showed. (slow connection was used, page from server). Such behavior happen despite of APZ(true/false) or "decode-immediately" (true/false) or "decode-only-on-draw" (true/false).
Assignee | ||
Comment 8•9 years ago
|
||
Comment 9•9 years ago
|
||
(In reply to Maksim Lebedev from comment #5)
> Some investigation:
> The reason really can be related with time, when image will be fully decoded.
> Tests showed that if "decode-immediately" preference set to TRUE (in that
> case decode-only-on-draw will be FALSE) we will get correct behavior (at
> least at real time on browser).
>
> So I can propose the simple idea for patch:
> > if(AsyncPanZoomEnabled)
> > decode-immediately = true;
This won't work. We *never* want decode-immediately enabled by default.
Flags: needinfo?(seth)
Comment 10•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> FWIW see bug 1178274; that should fix half of this bug (the failures on Mac
> and Windows). I'll leave this bug for the Linux e10s failure - I'm pretty
> sure the problem here is that the decode-only-on-draw flag gets set to true
> at
> http://mxr.mozilla.org/mozilla-central/source/image/ImageFactory.
> cpp?rev=cf2d7e67dab0#57 and the reftest harness is taking a snapshot before
> the image has been decoded and drawn.
If decode-only-on-draw is the problem, then feel free to just flip the pref to false in all.js. I'll r+ that. We're going to remove that code soon anyway.
Comment 11•9 years ago
|
||
(FWIW, though, I am skeptical that decode-only-on-draw is the real problem. And it sounds like comment 7 backs that up.)
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #9)
> This won't work. We *never* want decode-immediately enabled by default.
I can assume that decode-only-on-draw works like lazy initialization, so You do not want to use decode-immediately by default. But It works. I tested such approach on Windows.
Assignee | ||
Comment 13•9 years ago
|
||
+ Disabling decode-only-on-draw preference
Suggestions and comments and objections are very welcome.
Attachment #8628798 -
Flags: review?(seth)
Attachment #8628798 -
Flags: feedback?(bugmail.mozilla)
Attachment #8628798 -
Flags: feedback?(botond)
Assignee | ||
Comment 14•9 years ago
|
||
According with fails in tests and information from related bug 1159065, I can assume that picture rendering happens later than reftest makes snapshot. And I can assume that it happens because some of flags in image rendering code were set earlier than it needed. Unfortunately, I am not familiar with that code, and I think disabling preference is very simple solution. But according with Seth words about removing that code soon, we can make such decision.
Flags: needinfo?(bugmail.mozilla)
OS: Unspecified → Linux
Reporter | ||
Comment 15•9 years ago
|
||
Comment on attachment 8628798 [details] [diff] [review]
reftest_image_issue_ver1.diff
Review of attachment 8628798 [details] [diff] [review]:
-----------------------------------------------------------------
Seth's call on this one. But if he's ok with this change please also update http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h?rev=ff2afd92f8ab#262 to default to false, and also update the comment at http://mxr.mozilla.org/mozilla-central/source/image/ImageFactory.cpp?rev=e51ef562f1c5#54
Attachment #8628798 -
Flags: feedback?(bugmail.mozilla)
Attachment #8628798 -
Flags: feedback?(botond)
Attachment #8628798 -
Flags: feedback+
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Comment 17•9 years ago
|
||
What should I write as comment at ImageFactory.cpp ?
Reporter | ||
Comment 18•9 years ago
|
||
// It's safe since this is an optimization, and the only platform
// ImageDecodeOnlyOnDraw is enabled on is Fennec (where APZ is disabled in all
// widgets anyway).
Assignee | ||
Comment 19•9 years ago
|
||
+ Added disabling decode-only-on-draw at gfx::Prefs
+ Changed comment at ComputeImageFlags
Suggestions and comments and objections are very welcome.
Attachment #8628798 -
Attachment is obsolete: true
Attachment #8628798 -
Flags: review?(seth)
Flags: needinfo?(seth)
Attachment #8629296 -
Flags: review?(seth)
Attachment #8629296 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 20•9 years ago
|
||
Reporter | ||
Comment 21•9 years ago
|
||
Comment on attachment 8629296 [details] [diff] [review]
reftest_image_issue_ver2.diff
Review of attachment 8629296 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/ImageFactory.cpp
@@ +51,5 @@
> bool doDownscaleDuringDecode = gfxPrefs::ImageDownscaleDuringDecodeEnabled();
>
> + // It's safe since this is an optimization,
> + // and ImageDecodeOnlyOnDraw is enabled only on Fennec
> + // (where APZ is disabled in all widgets anyway).
Sorry, I meant that you should keep the first line of the comment and only change the last three lines. (i.e. keep the line that says "// We use the platform APZ value here since we don't have a widget to test."
Attachment #8629296 -
Flags: review?(bugmail.mozilla) → feedback+
Assignee | ||
Comment 22•9 years ago
|
||
+ Changed comment at ComputeImageFlags
Suggestions and comments and objections are very welcome.
Attachment #8629296 -
Attachment is obsolete: true
Attachment #8629296 -
Flags: review?(seth)
Attachment #8629408 -
Flags: review?(seth)
Attachment #8629408 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Comment 23•9 years ago
|
||
Reporter | ||
Comment 24•9 years ago
|
||
(In reply to Maksim Lebedev from comment #23)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5c44fd7b1ad
Please don't do extra try pushes just for comment changes. It's a waste of machine time since the code is exactly the same as it was before.
Reporter | ||
Updated•9 years ago
|
Attachment #8629408 -
Flags: feedback?(bugmail.mozilla) → feedback+
Comment 25•9 years ago
|
||
(In reply to Maksim Lebedev from comment #12)
> (In reply to Seth Fowler [:seth] from comment #9)
> > This won't work. We *never* want decode-immediately enabled by default.
> I can assume that decode-only-on-draw works like lazy initialization, so You
> do not want to use decode-immediately by default. But It works. I tested
> such approach on Windows.
I should have explained myself more clearly above. decode-immediately basically throws out all the hard work we've done over the past few years on image memory management, lazy image decoding, and decoding more important images before less important ones. Enabling it would result in major performance and memory usage regressions. That's why we will never enable it by default on any platform.
Flags: needinfo?(seth)
Comment 26•9 years ago
|
||
Comment on attachment 8629408 [details] [diff] [review]
reftest_image_issue_ver3.diff
Review of attachment 8629408 [details] [diff] [review]:
-----------------------------------------------------------------
r+ if you disable on Fennec too (mobile/android/app/mobile.js) and update the comment.
::: image/ImageFactory.cpp
@@ +52,5 @@
>
> // We use the platform APZ value here since we don't have a widget to test.
> + // It's safe since this is an optimization,
> + // and ImageDecodeOnlyOnDraw is enabled only on Fennec
> + // (where APZ is disabled in all widgets anyway).
Just disable decode-on-draw-only on all platforms. You can update this to read "This feature is disable everywhere anyway, and will be removed soon."
Attachment #8629408 -
Flags: review?(seth) → review+
Assignee | ||
Comment 27•9 years ago
|
||
+ Disabled decode-only-on-draw at Fennec (Android)
+ Changed comment at ComputeImageFlags
Suggestions and comments and objections are very welcome.
Attachment #8629408 -
Attachment is obsolete: true
Attachment #8630343 -
Flags: review?(seth)
Attachment #8630343 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Comment 28•9 years ago
|
||
Reporter | ||
Comment 29•9 years ago
|
||
Comment on attachment 8630343 [details] [diff] [review]
reftest_image_issue_ver4.diff
Review of attachment 8630343 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/ImageFactory.cpp
@@ +52,5 @@
>
> // We use the platform APZ value here since we don't have a widget to test.
> + // It's safe since this is an optimization,
> + // and ImageDecodeOnlyOnDraw is disabled everywhere and will be removed soon
> + // (where APZ is disabled in all widgets anyway).
Remove the last line of the comment since that's not necessary anymore. And please do *not* do another try push for that change.
Attachment #8630343 -
Flags: feedback?(bugmail.mozilla) → feedback+
Comment 30•9 years ago
|
||
Comment on attachment 8630343 [details] [diff] [review]
reftest_image_issue_ver4.diff
Review of attachment 8630343 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. Just fix the comment and you're good to go; you don't need to request review again.
::: image/ImageFactory.cpp
@@ +52,5 @@
>
> // We use the platform APZ value here since we don't have a widget to test.
> + // It's safe since this is an optimization,
> + // and ImageDecodeOnlyOnDraw is disabled everywhere and will be removed soon
> + // (where APZ is disabled in all widgets anyway).
Agreed with kats re: removing the last line of the comment. Also, please rewrap the words in the comment to fit 80 columns correctly.
Attachment #8630343 -
Flags: review?(seth) → review+
Assignee | ||
Comment 31•9 years ago
|
||
+ Changed comment at ComputeImageFlags
Suggestions and comments and objections are very welcome.
Attachment #8630343 -
Attachment is obsolete: true
Attachment #8630895 -
Flags: review?(seth)
Attachment #8630895 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Comment 32•9 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #30)
> you don't need to request review again.
I prefere to re-request, because I cannot write comments very correctly, so...
And for simplier re-review I try to summarize my changes at comments with patch.
Reporter | ||
Comment 33•9 years ago
|
||
Comment on attachment 8630895 [details] [diff] [review]
reftest_image_issue_ver5.diff
Review of attachment 8630895 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM, carrying r+ for seth.
Attachment #8630895 -
Flags: review?(seth)
Attachment #8630895 -
Flags: review+
Attachment #8630895 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Comment 34•9 years ago
|
||
If there are no objections, I put checkin-needed flag.
Keywords: checkin-needed
Comment 35•9 years ago
|
||
Keywords: checkin-needed
Comment 36•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•