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

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jrmuizel, Assigned: roc)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

unspecified
mozilla14
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

(Whiteboard: [layout])

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
It seems like when we are zoomed in we use 565, when we are zoomed out we use 8888
(Reporter)

Comment 1

6 years ago
For the site listed we should always be drawing opaquely.
Summary: maple: The texture format depends on the zoom → maple: The texture format we use for http://people.mozilla.org/~jmuizelaar changes

Comment 2

6 years ago
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.
Summary: maple: The texture format we use for http://people.mozilla.org/~jmuizelaar changes → Maple: Rounding of visible/opaque regions when zooming prevents the use of opaque layers (and 16-bit textures)

Updated

6 years ago
OS: Linux → Android
Hardware: x86_64 → ARM

Updated

6 years ago
Blocks: 729391
Whiteboard: [layout]

Comment 3

6 years ago
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.
Assignee: nobody → matspal
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.
Assignee: matspal → jet
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

6 years ago
Created attachment 605918 [details] [diff] [review]
Workaround: Round the opaque region outward

This is the workaround described in Comment 2.
Given comment 4, we have a winner here!
Assignee: jet → roc

Comment 8

6 years ago
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.

Updated

6 years ago
Attachment #605934 - Attachment is patch: true

Updated

6 years ago
Whiteboard: [layout] → [layout][autoland-try:605918:-p all -u all][autoland-try:605934:-p all -u all]

Updated

6 years ago
Whiteboard: [layout][autoland-try:605918:-p all -u all][autoland-try:605934:-p all -u all] → [layout][autoland-in-queue][autoland-try:605934:-p all -u all]

Comment 9

6 years ago
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

6 years ago
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

Updated

6 years ago
Whiteboard: [layout][autoland-in-queue][autoland-try:605934:-p all -u all] → [layout][autoland-try:605934:-p all -u all]

Updated

6 years ago
Whiteboard: [layout][autoland-try:605934:-p all -u all] → [layout][autoland-in-queue]

Comment 11

6 years ago
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

6 years ago
(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

6 years ago
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.

Updated

6 years ago
Whiteboard: [layout][autoland-in-queue] → [layout]

Comment 14

6 years ago
(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.
Yeah, both approaches do something invalid, so I'd expect to get reftest fails with both of them.
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.
roc, will you try that out?
I don't have time to work on that this week.

Updated

6 years ago
blocking-fennec1.0: --- → ?
(Reporter)

Updated

5 years ago
Blocks: 717774
blocking-fennec1.0: ? → beta+
Created attachment 612159 [details] [diff] [review]
WIP

This is my approach. Running it through tryserver now.
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.
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 on attachment 612159 [details] [diff] [review]
WIP

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

Let's do this.
Attachment #612159 - Flags: review?(tnikkel)
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.
Ah, that test is already marked as failing if Android&&layersOpenGL, so I'll just remove the &&layersOpenGL.
Blocks: 735898
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.
Attachment #612159 - Flags: review?(tnikkel) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/25a13d26509d
https://hg.mozilla.org/mozilla-central/rev/25a13d26509d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14

Updated

5 years ago
Depends on: 744607

Updated

5 years ago
Depends on: 744666

Updated

5 years ago
Depends on: 758862

Updated

5 years ago
Depends on: 791048
Depends on: 830406
Blocks: 729781
Depends on: 887691
You need to log in before you can comment on or make changes to this bug.