733607 - Maple: Rounding of visible/opaque regions when zooming prevents the use of opaque layers (and 16-bit textures)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

It seems like when we are zoomed in we use 565, when we are zoomed out we use 8888

Comment 1

[Jeff Muizelaar [:jrmuizel]]

For the site listed we should always be drawing opaquely.

Comment 2

[Ali Juma [:ajuma]]

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 (inactive)]

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 (inactive)]

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)]

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]]

Attachment: Workaround: Round the opaque region outward

This is the workaround described in Comment 2.

Comment 7

[Joe Drew (not getting mail)]

Given comment 4, we have a winner here!

Comment 8

[Ali Juma [:ajuma]]

Attachment: 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]

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]

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/[e-mail]-c7a2cef23e1a

Comment 11

[Mozilla RelEng Bot]

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]]

(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/
> [e-mail]-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]

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/[e-mail]-8ec35dadce16
 Timed out after 12 hours without completing.

Comment 14

[Ali Juma [:ajuma]]

(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/
> [e-mail]-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)]

Yeah, both approaches do something invalid, so I'd expect to get reftest fails with both of them.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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)]

roc, will you try that out?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't have time to work on that this week.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: WIP

This is my approach. Running it through tryserver now.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

Ah, that test is already marked as failing if Android&&layersOpenGL, so I'll just remove the &&layersOpenGL.

Comment 25

[Timothy Nikkel (:tnikkel)]

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) (email my personal email if necessary)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/25a13d26509d

Comment 27

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/25a13d26509d