724786 - Layers are invalidated after every pan due to slightly different scrolled root origin

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Patrick Walton (:pcwalton)]

After every pan, all layers in Native Fennec get completely invalidated, defeating any buffer rotation optimizations. This is due to hitting the following condition:

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#880

That is, activeScrolledRootTopLeft != data->mActiveScrolledRootPosition. The data is off by a few subpixels, causing layout to invalidate the entire layer we wanted to scroll. Turns out that the position of the top left corner of the scrolled root is changing slightly, so layout is bailing out and repainting everything. It's *very* slight -- the first five or so decimal places are always the same. (FuzzyEquals is not a solution here -- it changes more than FuzzyEquals' tolerance.)

As it stands now (in the kiwifox branch), paints take 74.33 ms (13 fps). After commenting out that check, paints drop to 43.08 ms (23 fps) when panning quickly and 32.74 ms (30 fps) when panning slowly. That's over a 2x improvement in panning performance and will lead to correspondingly less checkerboarding.

I think we definitely want to fix this—it will help the Java compositor as well.

Comment 1

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

Does the amount of error depend on the scale factor?

Bug 681192 might be relevant.

Comment 2

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

Sorry, I'm already completely swamped.

Comment 3

[Patrick Walton (:pcwalton)]

Yes, this looks exactly like what Oleg was seeing in bug 681192. I guess this is a Fennec bug that's been around forever. I'll try to apply his patches and see if they still apply.

Comment 4

[Patrick Walton (:pcwalton)]

Doug, shall I take this or should Chris?

Comment 5

[Oleg Romashin (:romaxa)]

Problem with patches from bug 681192, is broken reftest, which seems related to some other bug, and that other bug need to be fixed in order to push 681192

Comment 6

[Chris Peterson [:cpeterson]]

romaxa, is there a good test to verify performance improvements from bug 681192's patches? 

I enabled pcwalton's frame rate HUD. I don't see much frame rate improvements when panning and zooming timecube.com on my Galaxy Nexus. Without the patches, frame time is about 1-3 ms. With the patches, frame time is about 2-5 ms. However, timecube.com is checkerboarding *badly*, which may invalidate this test.

Comment 7

[Patrick Walton (:pcwalton)]

(In reply to Oleg Romashin (:romaxa) from comment #5)
> Problem with patches from bug 681192, is broken reftest, which seems related
> to some other bug, and that other bug need to be fixed in order to push
> 681192

Can we just disable the reftest? This is very important to land.

(In reply to Chris Peterson (:cpeterson) from comment #6)
> romaxa, is there a good test to verify performance improvements from bug
> 681192's patches? 
> 
> I enabled pcwalton's frame rate HUD. I don't see much frame rate
> improvements when panning and zooming timecube.com on my Galaxy Nexus.
> Without the patches, frame time is about 1-3 ms. With the patches, frame
> time is about 2-5 ms. However, timecube.com is checkerboarding *badly*,
> which may invalidate this test.

The patch won't affect framerate. It's purely a checkerboarding improvement.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

cpeterson, if you want to measure checkerboarding you can use the talos test we have in robocop. ping me on IRC if you need help with that.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: bug-724786-part-1-tweak-viewport-zoom-factor.patch

Bug 724786 part 1 - Tweak browser.js' viewport zoom factor to avoid layer invalidation when panning. Android only.

FrameLayerBuilder.cpp floors offset to an nscoord (int32_t) before scaling. It then compares scaledOffset to NSToIntRoundUp(scaledOffset). To minimize the delta, we want to find the nearest integral scaled value and compute a new zoom between the integral offsets for each axis.

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: bug-724786-part-2-thebes-fuzzy-equal.patch

Bug 724786 part 2 - Use fuzzy coordinate comparisons when invalidating recycled Thebes layers.

This patch changes core layout code! This change is a rebased subset from bug 681192's patch 557768, which was r=roc.

roc, this patch adds a fuzzy tolerance to CreateOrRecycleThebesLayer() of 1e-4 in a code path I believe is shared by desktop code. What is the smallest tolerance you would accept?

This patch is a "nice to have". My bug-724786-part-1.patch actually does a pretty good job of tweaking the zoom values to induce near-zero deltas in CreateOrRecycleThebesLayer(). Adding the fuzzy tolerance just gives browser.js a "bigger target" to hit. :)

Comment 11

[Patrick Walton (:pcwalton)]

Comment on attachment 596238 [details] [diff] [review]
bug-724786-part-1-tweak-viewport-zoom-factor.patch

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

What are the differences you see between scaleX and scaleY here? Hopefully they're very small, otherwise you could get distortion.

Also, I'm somewhat surprised that changing scaleX and scaleY doesn't cause layer invalidation in some other part of layout. But if you didn't observe layer invalidation, this is fine by me!

Comment 12

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

Comment on attachment 596238 [details] [diff] [review]
bug-724786-part-1-tweak-viewport-zoom-factor.patch

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

::: mobile/android/chrome/content/browser.js
@@ +1577,5 @@
> +    // To minimize the delta, we want to find the nearest integral scaled value
> +    // and compute a new zoom between the integral offsets for each axis.
> +    let thebesOffsetX = Math.floor(aViewport.x);
> +    aViewport.x = Math.floor(thebesOffsetX / aViewport.zoom);
> +    let zoomFromIntToIntX = aViewport.x ? (thebesOffsetX / aViewport.x) : aViewport.zoom;

I don't really understand what you're doing here. You're leaving aViewport.x divided by its zoom, which seems beyond the scope of what I thought you were going to do here.

However, I don't actually need to review this code.

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: bug-724786-part-1-zoom-to-integer-viewport-offsets-v2.patch

Cwiiis, can you please review these browser.js changes? I'm trying to tweak the viewport zoom to minimize the offset rounding error causing layer invalidations in FrameLayerBuilder.cpp's CreateOrRecycleThebesLayer() in patch part-2:

https://bugzilla.mozilla.org/attachment.cgi?id=596239&action=diff

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 597632 [details] [diff] [review]
bug-724786-part-1-zoom-to-integer-viewport-offsets-v2.patch

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

I don't want to r- this, but I can't r+ this until the questions I have below are answered.

::: mobile/android/chrome/content/browser.js
@@ +1591,5 @@
> +    // Y axis is longer and thus more senstive to zoom rounding errors, so use
> +    // its new zoom for both axes.
> +    aViewport.y = Math.floor(aViewport.y / aViewport.zoom);
> +    if (aViewport.y > 0)
> +        aViewport.zoom = thebesOffsetY / aViewport.y;

Changing the zoom is going to cause problems when we send viewport updates back to Gecko - updateViewport in GeckoLayerClient/GeckoSoftwareLayerClient uses the zoom to determine whether to synchronise the page size when drawing is finished.

Do we not want to do this on the Java side, so that the Java layers have the correct zoom level? I may not have the entire picture here, so sorry if this question is invalid.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 597632 [details] [diff] [review]
bug-724786-part-1-zoom-to-integer-viewport-offsets-v2.patch

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

Yeah, this doesn't seem like the right place to be doing this. This almost seems like we're changing our front-end stuff to be bug-compatible with the backend. Why does FrameLayerBuilder.cpp floor offset to nscoord (int32_t) before scaling?

Comment 17

[Patrick Walton (:pcwalton)]

(In reply to Kartikaya Gupta (:kats) from comment #16)
> Comment on attachment 597632 [details] [diff] [review]
> bug-724786-part-1-zoom-to-integer-viewport-offsets-v2.patch
> 
> Review of attachment 597632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, this doesn't seem like the right place to be doing this. This almost
> seems like we're changing our front-end stuff to be bug-compatible with the
> backend. Why does FrameLayerBuilder.cpp floor offset to nscoord (int32_t)
> before scaling?

See https://bugzilla.mozilla.org/show_bug.cgi?id=681192#c8. roc says the invalidation code is correct here.

CreateOrRecycleThebesLayer() doesn't offset to nscoord before scaling -- it uses the nscoord to convert `offset` from app units (1/60 of a pixel, used for most of layout) to doubles.

The right fix is pretty complex -- it basically involves adding a new ScrollTo() API. You either have to add compensation for this issue in the frontend or add it to the implementation of ScrollTo(). Based on what Chris has been telling me, we may end up having to do the latter anyway. But this is an attempt at making a simpler fix.

Note that Oleg actually implemented the latter, and it's probably bitrotted, but as I understand things the only problem was a failing reftest. We could just disable that reftest on mobile.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

I don't know why I moved this in the first place, putting back to beta

Comment 20

[JP Rosevear [:jpr]]

Who is actually the right assignee here?

Comment 21

[JP Rosevear [:jpr]]

Matt, its hard for us to tell how bad this is, can you take a look?

Comment 22

[Patrick Walton (:pcwalton)]

Note that I landed a hack early on in Maple to work around this (it basically just comments out the offending code). I don't know whether my hack still exists. We should fix this for real, as there can be (minor) graphical corruption with my hack.

Comment 23

[Erin Lancaster [:elan]]

Just talked to Patrick; the behavior described in the bug isn't happening but he would like to figure out why. Assigning to him. This can also be a release blocker.

Comment 24

[Kevin Brosnan [Ex-Mozilla]]

Was the minusing intentional? Just want to make sure this does not fall off the radar if it was not.

Comment 25

[Jay Sullivan]

I didn't minus this myself. Damon, did you minus this when you were reviewing the bug list on my laptop Friday?

Comment 26

[Patrick Walton (:pcwalton)]

I think I'm seeing the graphical corruption that this bug entails -- it's annoying -- so this should be fixed the right way.

Comment 27

[Kevin Brosnan [Ex-Mozilla]]

Putting it back on the beta radar then.

Comment 28

[JP Rosevear [:jpr]]

Jeff, are we seeing the invalidation in the profiler?

Comment 29

[Patrick Walton (:pcwalton)]

You won't see the invalidation, because the code is ifdef'd out:

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#912

This is my hack, but it causes minor graphical corruption (one pixel). It should be fixed the right way. I'm not sure if it needs to block beta though, because there *is* that hack there.

Comment 30

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

Are we expecting the "fix this the right way" to happen for Fennec beta? That would require finishing 681192, landing it, and then making further Fennec front-end changes.

This bug is confusing. The bug as filed was fixed by pcwalton's workaround. That introduced a new bug --- the 1px corruption --- but that bug is less important and it's not clear to me that we need to block on it.

Comment 31

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

If we do want to block on the 1px corruption, please file a new bug, make it block, and make it depend on bug 681192 (and make that block too).

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

re-noming based on comment 30

Comment 33

[Joe Drew (not getting mail)]

This bug is worked around, which causes the single-pixel corruption. Let's close this out, and I'll open a new bug on the single-pixel corruption.

Comment 34

[Joe Drew (not getting mail)]

The new bug is bug 749731.