1100140 - Performance problems when scrolling in a fullscreen element

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kevin Grandon :kgrandon]

Scrolling takes a huge hit after calling document.body.mozRequestFullScreen();. 

STR:
- You can apply the patch from bug 1100138, and install the gaia home screen.
- Enter edit mode by long pressing on any icon and letting go.
- Scroll the screen.

Expected result:
Smooth scrolling.

Actual result:
Poor scrolling performance, possibly falling back to non-async scrolling?

Comment 1

[Kevin Grandon :kgrandon]

Kats or Botond - have either of you seen this before?

Comment 2

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

Can you turn on the FPS counter from the developer settings and see if you get a red square in the top-right corner when you do this? If so that means we're falling back to sync scrolling. (For reference, this indicator was added in bug 1053992).

Comment 3

[Kevin Grandon :kgrandon]

Hey Kats - it looks like fullscreen mode does something weird and hides the overlays, so it's impossible to tell just from those. I do see some HWComposer logging, which looks like: I/Gecko   ( 1856): HWComposer: FPS is XX. Unfortunately I don't see anything in the logs that say whether or not I'm hitting sync scrolling.

Is there some additional logging I could add to check for sync scrolling?

Comment 4

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

Ah it sounds like then full-screen mode uses the HWComposer rather than our C++ compositor code. I think it makes sense then that it falls back to sync scrolling. Technically all the APZ machinery is still there and working fine, but since its our C++ compositor that applies the APZ transform, and the C++ compositor isn't being used, the APZ transform is effectively ignored and it only updates when it receives the newly painted content from gecko.

I don't know if there's any way to make to do async transforms in HWComposer and if we can support this. BenWa, do you know? Do we support things like OMTA in HWComposer?

Comment 5

[Benoit Girard (:BenWa)]

AFAIK HWC just replaces the Render code of the compositor and nothing else. It will look at the current shadow transforms and such. As long as APZ code doesn't run code in the Render phase and doesn't require any fancy drawing feature then it should just work. Perhaps I'm missing something?

Comment 6

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

Hm ok I'll investigate this a bit more then. Leaving ni on me for now.

Comment 7

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

Attachment: Layer dumps

So I grabbed layer dumps with and without the fullscreening patch. Once the edit-mode is fullscreen a lot more things are "fixed position" which kind of makes sense, because the way fullscreen is implemented is by applying fixed-position styles and sizing things to the viewport. What's happening is the scrollable container layer also ends up being fixed-position, so the async transform is cancelled out.

However I think this might be a bug in our fixed-position handling code, because here the "fixed position" is really relative to the ancestor scrollable layer. That is, in the with-patch layer dump, the layer 0xae5fb800 is fixed relative to 0xa9aab400 but we actually treat 0xae5fb800 as fixed relative to both 0xa9aab400 and 0xae5fb800 itself.

Comment 8

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

Attachment: Patch

This patch fixes it for me but I'm worried it might break other things. Kevin, do you mind trying this patch and verifying it fixes the problem you were seeing? Also if you see any breakage due to this please let me know.

Comment 9

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

This was probably broken by the change in bug 1063494. The subtlety is that a sticky layer can have its sticky annotation on the scrollable layer it's relative to, but fixed-position annotations are (I hope) never on the scrollable layer they're relative to. I verified that this patch doesn't appear to regress bug 1063494.

Comment 10

[Kevin Grandon :kgrandon]

Thanks for the quick turnaround on this Kats. Not sure if I'm going to be able to spin a build and test the patch this week because I'm at a conference, but I will try to test it asap. If I don't get to it this week, I will definitely get to it early next week. Don't let me hold you up from getting this reviewed/landed if you need to though.

Comment 11

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

Try is clean: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3ad0d3d283c3

Comment 12

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

Btw Kevin, if when you try this patch you see the screen flicker black as you cross over the icon divider thingy let me know. I was seeing that and wasn't sure what it was, but I now suspect it is another manifestation of bug 844911 (although that's just a guess).

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8524643 [details] [diff] [review]
Patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +267,5 @@
>                                                     const Matrix4x4& aCurrentTransformForRoot,
>                                                     const LayerMargin& aFixedLayerMargins)
>  {
>    bool isRootFixed = aLayer->GetIsFixedPosition() &&
> +    aLayer != aTransformedSubtreeRoot &&

Please add a comment that explains this condition. Something like "if aTransformedSubtreeRoot itself is fixed, interpret it as being fixed relative to the ancestor scrollable layer, not itself".

Comment 14

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

Added a comment and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/613266d6f6ec

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/613266d6f6ec