Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 9 years ago

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

	Maksim Lebedev 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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 2 • 9 years ago

Thanks for looking into this. Let me know when you find the root cause of the problem.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) 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.

	Maksim Lebedev 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).

	Maksim Lebedev 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;

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 6 • 9 years ago

That seems like it will defeat the purpose of the decode-on-draw code. Seth, any thoughts?

	Maksim Lebedev 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).

	Maksim Lebedev Assignee
	Comment 8 • 9 years ago

Just for clarification: Tests at comment 1 and comment 4 happened on Windows platform.

	Seth Fowler [:seth] [:s2h]
	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.

	Seth Fowler [:seth] [:s2h]
	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.

	Seth Fowler [:seth] [:s2h]
	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.)

	Maksim Lebedev 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.

	Maksim Lebedev Assignee
	Comment 13 • 9 years ago

+ Disabling decode-only-on-draw preference

Suggestions and comments and objections are very welcome.

	Maksim Lebedev 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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) 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

	Maksim Lebedev Assignee
	Comment 16 • 9 years ago

APZ(true): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7333c96e8afb
APZ(true) + PATCH: https://treeherder.mozilla.org/#/jobs?repo=try&revision=971a88c2481e
PATCH: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1120a7f5430d

	Maksim Lebedev Assignee
	Comment 17 • 9 years ago

What should I write as comment at ImageFactory.cpp ?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) 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).

	Maksim Lebedev 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.

	Maksim Lebedev Assignee
	Comment 20 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a777fa95fe9

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) 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."

	Maksim Lebedev Assignee
	Comment 22 • 9 years ago

+ Changed comment at ComputeImageFlags

Suggestions and comments and objections are very welcome.

	Maksim Lebedev Assignee
	Comment 23 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5c44fd7b1ad

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) 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.

	Seth Fowler [:seth] [:s2h]
	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.

	Seth Fowler [:seth] [:s2h]
	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."

	Maksim Lebedev 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.

	Maksim Lebedev Assignee
	Comment 28 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab24a10d400

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) 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.

	Seth Fowler [:seth] [:s2h]
	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.

	Maksim Lebedev Assignee
	Comment 31 • 9 years ago

+ Changed comment at ComputeImageFlags

Suggestions and comments and objections are very welcome.

	Maksim Lebedev 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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) 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.

	Maksim Lebedev Assignee
	Comment 34 • 9 years ago

If there are no objections, I put checkin-needed flag.

	Pulsebot
	Comment 35 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2261cdab5b3

	Carsten Book [:Tomcat]
	Comment 36 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/a2261cdab5b3