Closed Bug 1100140 Opened 5 years ago Closed 5 years ago

Performance problems when scrolling in a fullscreen element

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.1 --- affected
b2g-v2.2 --- affected

People

(Reporter: kgrandon, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(2 files)

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?
Kats or Botond - have either of you seen this before?
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(botond)
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).
Flags: needinfo?(kgrandon)
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(botond)
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?
Flags: needinfo?(kgrandon) → needinfo?(bugmail.mozilla)
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?
Flags: needinfo?(bgirard)
Flags: needinfo?(bugmail.mozilla)
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?
Flags: needinfo?(bgirard) → needinfo?(bugmail.mozilla)
Hm ok I'll investigate this a bit more then. Leaving ni on me for now.
Attached file 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.
Flags: needinfo?(bugmail.mozilla)
Attached patch PatchSplinter Review
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.
Assignee: nobody → bugmail.mozilla
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.
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.
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 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".
Attachment #8524643 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/613266d6f6ec
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.