Last Comment Bug 733607 - Maple: Rounding of visible/opaque regions when zooming prevents the use of opaque layers (and 16-bit textures)
: Maple: Rounding of visible/opaque regions when zooming prevents the use of op...
Status: RESOLVED FIXED
[layout]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla14
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 758862 791048 830406 744607 744666 887691
Blocks: checkerboarding 729391 729781 735898
  Show dependency treegraph
 
Reported: 2012-03-06 15:57 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2013-07-03 10:35 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta+


Attachments
Workaround: Round the opaque region outward (1.45 KB, patch)
2012-03-14 13:18 PDT, Ali Juma [:ajuma]
no flags Details | Diff | Review
Workaround: Round the visible region inward (2.28 KB, patch)
2012-03-14 13:49 PDT, Ali Juma [:ajuma]
no flags Details | Diff | Review
WIP (98.99 KB, patch)
2012-04-04 05:39 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
tnikkel: review+
Details | Diff | Review
transform testcase (491 bytes, text/html)
2012-04-04 05:53 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details

Description Jeff Muizelaar [:jrmuizel] 2012-03-06 15:57:54 PST
It seems like when we are zoomed in we use 565, when we are zoomed out we use 8888
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-03-06 15:59:56 PST
For the site listed we should always be drawing opaquely.
Comment 2 Ali Juma [:ajuma] 2012-03-08 11:39:10 PST
In ContainerState::ThebesLayerData::Accumulate, mVisibleRegion is rounded up using SimplifyOutward, while mOpaque region is rounded down using ScaleToInsidePixels. As a result, when we zoom, we end up with a tiny (e.g. height 1) portion of mVisibleRegion that's not in mOpaqueRegion. Then, in ContainerState::PopThebesLayerData, transparentRegion is non-empty, and hence we don't set the CONTENT_OPAQUE flag on the layer. This means we end up using an 8888 texture rather than a 565 texture for the layer. 

This happens on other sites too, like cnn.com (in fact, this probably happens everywhere when we zoom). 

Rounding the opaque region using ScaleToOutside rather than ScaleToInside does work around the problem (albeit at the expense of correctness when we really do have a tiny visible non-opaque region). Given that on some mobile GPUs (e.g. Adreno) upload for 565 textures is almost twice as fast as for 8888 textures, it may be worth making the correctness-for-speed tradeoff on Android.

CC-ing people who'd know more about what we should do here.
Comment 3 Jet Villegas (:jet) 2012-03-13 15:24:59 PDT
Mats: can you have a look at this one too? It seems weird to ever change the RGBA format based on zoom factor. Layout may need to unify how we round coords zooming in and out.
Comment 4 Mats Palmgren (:mats) 2012-03-14 10:52:51 PDT
I have nothing further to add to Ali's analysis in comment 2.  Roc's comments
in ContainerState::ThebesLayerData::Accumulate indicates that the current
rounding is intentional, so I think he needs to look at this when he comes back.

Meanwhile, if Ali's workaround using ScaleToOutside does pass regression tests
on all platforms, assuming it improves the situation for mobile, then I can r+
that as a temporary workaround if you want...  but I really don't know if that's
the right fix.
Comment 5 Timothy Nikkel (:tnikkel) 2012-03-14 13:16:31 PDT
The other obvious simple fix other than ScaleToOutside on the opaque region is ScaleToInside on the visible region. Not sure if that works, but at least it doesn't lie about opaque-ness of some pixels.
Comment 6 Ali Juma [:ajuma] 2012-03-14 13:18:05 PDT
Created attachment 605918 [details] [diff] [review]
Workaround: Round the opaque region outward

This is the workaround described in Comment 2.
Comment 7 Joe Drew (not getting mail) 2012-03-14 13:27:12 PDT
Given comment 4, we have a winner here!
Comment 8 Ali Juma [:ajuma] 2012-03-14 13:49:30 PDT
Created attachment 605934 [details] [diff] [review]
Workaround: Round the visible region inward

Scaling the visible region inward as suggested in Comment 5 also seems to work.
Comment 9 Mozilla RelEng Bot 2012-03-14 14:03:24 PDT
Autoland Patchset:
	Patches: 605918
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=c7a2cef23e1a
Try run started, revision c7a2cef23e1a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=c7a2cef23e1a
Comment 10 Mozilla RelEng Bot 2012-03-15 00:46:40 PDT
Try run for c7a2cef23e1a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c7a2cef23e1a
Results (out of 216 total builds):
    success: 160
    warnings: 56
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-c7a2cef23e1a
Comment 11 Mozilla RelEng Bot 2012-03-15 00:51:19 PDT
Autoland Patchset:
	Patches: 605934
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=8ec35dadce16
Try run started, revision 8ec35dadce16. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=8ec35dadce16
Comment 12 Ali Juma [:ajuma] 2012-03-15 06:40:56 PDT
(In reply to Mozilla RelEng Bot from comment #10)
> Try run for c7a2cef23e1a is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=c7a2cef23e1a
> Results (out of 216 total builds):
>     success: 160
>     warnings: 56
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/
> autolanduser@mozilla.com-c7a2cef23e1a

Using ScaleToOutside on the opaque region is causing two transform-3d reftest failures on most platforms (but these tests are already hidden on native Android).
Comment 13 Mozilla RelEng Bot 2012-03-15 13:17:32 PDT
Try run for 8ec35dadce16 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8ec35dadce16
Results (out of 163 total builds):
    exception: 2
    success: 114
    warnings: 39
    failure: 2
    other: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-8ec35dadce16
 Timed out after 12 hours without completing.
Comment 14 Ali Juma [:ajuma] 2012-03-15 13:30:48 PDT
(In reply to Mozilla RelEng Bot from comment #13)
> Try run for 8ec35dadce16 is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=8ec35dadce16
> Results (out of 163 total builds):
>     exception: 2
>     success: 114
>     warnings: 39
>     failure: 2
>     other: 6
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/
> autolanduser@mozilla.com-8ec35dadce16
>  Timed out after 12 hours without completing.

Using ScaleToInside on the visible region is causing at least eight reftest failures on most platforms.
Comment 15 Timothy Nikkel (:tnikkel) 2012-03-15 23:47:42 PDT
Yeah, both approaches do something invalid, so I'd expect to get reftest fails with both of them.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-19 01:40:16 PDT
The current rounding is correct because in general, when an opaque object is scaled so that edges are not at pixel boundaries, those edges might end up partially transparent.

When we actually render, we might not get partial transparent edges because of pixel snapping --- depending on what's in the layer. (Is that what actually happens?) Or we might get a complete row or column of fully transparent pixels.

We could take account of snapping and the FrameLayerBuilder scale factors to compute more precise bounds for the display items of the layer. Maybe that's the way to go here.
Comment 17 Joe Drew (not getting mail) 2012-03-20 10:35:58 PDT
roc, will you try that out?
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 11:49:15 PDT
I don't have time to work on that this week.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-04 05:39:03 PDT
Created attachment 612159 [details] [diff] [review]
WIP

This is my approach. Running it through tryserver now.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-04 05:53:22 PDT
Created attachment 612160 [details]
transform testcase

Clicking on the yellow DIV starts it moving. On trunk, its layer is not opaque because the background display item bounds are not pixel-aligned. With this patch, the layer is opaque.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-04 13:50:47 PDT
Try push is pretty good:
https://tbpl.mozilla.org/?tree=Try&rev=2f598ab19ed7

The only possible issue due to this patch, I think, is a small fuzz error in layout/reftests/text-overflow/block-padding.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10635962&tree=Try&full=1#error0

I don't understand the failure. It may be random.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-04 13:52:22 PDT
Comment on attachment 612159 [details] [diff] [review]
WIP

Review of attachment 612159 [details] [diff] [review]:
-----------------------------------------------------------------

Let's do this.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-04 14:52:28 PDT
The failure's not random. Given it's only on Android, and it's not a visible issue, I think it's not worth investigating and we should just fuzz it away.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-04 15:04:37 PDT
Ah, that test is already marked as failing if Android&&layersOpenGL, so I'll just remove the &&layersOpenGL.
Comment 25 Timothy Nikkel (:tnikkel) 2012-04-10 00:35:37 PDT
Comment on attachment 612159 [details] [diff] [review]
WIP

+  nsIntRect ScaleToInsidePixels(const nsRect& aRect, bool aSnap)
+  {
+    // When AllowResidualTranslation is false, display items will be drawn
+    // scaled with a translation by integer pixels, so we know how the snapping
+    // will work.
+    if (aSnap && mSnappingEnabled) {
+      return ScaleToNearestPixels(aRect);
+    }

The comment seems out of place, and you already have it elsewhere. You can probably drop it.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-10 04:50:58 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/25a13d26509d
Comment 27 :Ehsan Akhgari (out sick) 2012-04-10 13:08:16 PDT
https://hg.mozilla.org/mozilla-central/rev/25a13d26509d

Note You need to log in before you can comment on or make changes to this bug.