Over-invalidation with animated transforms on window-sized divs

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cwiiis, Assigned: roc)

Tracking

({perf, regression})

unspecified
mozilla27
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [c= p= s=2013.10.25 u=])

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Created attachment 816862 [details]
Reduced test-case

I'm getting whole-layer invalidation on the attached test-case for every rendered frame of a css transition. I've not easily been able to omit any part of it to fix the issue.

This is unfortunate, as this is a common method for switching between pages in mobile web-apps (less unfortunate for desktop due to raw power...) - the same bug doesn't seem to manifest on b2g18, so would appear to be a regression (though so much has changed in that space that I'm loathe to mark it as such. I will, though.)

I've not debugged this further, what with being on sabbatical and all :)

Updated

5 years ago
Keywords: perf

Updated

5 years ago
Whiteboard: [c= p= s= u=]

Comment 1

5 years ago
Chris, what's the end-user impact and do you have numbers for the performance impact of this problem?
Flags: needinfo?(chrislord.net)
(Reporter)

Comment 2

5 years ago
(In reply to Mike Lee [:mlee] from comment #1)
> Chris, what's the end-user impact and do you have numbers for the
> performance impact of this problem?

An extra note, I've used paint flashing and confirmed that this is a problem on Nightly on Android and *not* a problem on b2g18 on FirefoxOS 1.1.

Numbers for this will be entirely dependent on the page and the specs of the device rendering it, but I can say that having to redraw the entire screen (and possibly some off-screen content too?) for every frame of an animation makes the difference from fluid 60fps animations to terribly, visibly janky, sub-30fps animations.

To put it in perspective, I'm writing an app that uses this technique to transition between screens. On b2g18, these transitions are 60fps on a Geeksphone Keon (ignoring the first few skipped frames, which is a different bug we need to tackle). Using Firefox Nightly on a Motorola RAZRi (a reasonably powerful, x86 Android device), the same transitions are noticeably jerky. Certainly sub-30fps. My app's pages consist of text with text-shadow on a transparent background and aren't really any more complicated than the attached example.

I could attach an even more representative example if it helps, but the attached test-case is simpler and should be enough to diagnose and fix whatever's causing this.

I'd be surprised if this bug wasn't affecting b2g >=1.2 in some way.
Flags: needinfo?(chrislord.net)
(In reply to Chris Lord [:cwiiis] from comment #2)
> An extra note, I've used paint flashing and confirmed that this is a problem
> on Nightly on Android and *not* a problem on b2g18 on FirefoxOS 1.1.

Other than at the start and end of the animation (when the layer for the div becomes active/deactive), I'm not seeing full layer invalidation in Nightly or FF24 on OS X. Only the text invalidates on every frame of the animation (still undesirable).
(Reporter)

Comment 4

5 years ago
(In reply to Jonathan Watt [:jwatt] from comment #3)
> (In reply to Chris Lord [:cwiiis] from comment #2)
> > An extra note, I've used paint flashing and confirmed that this is a problem
> > on Nightly on Android and *not* a problem on b2g18 on FirefoxOS 1.1.
> 
> Other than at the start and end of the animation (when the layer for the div
> becomes active/deactive), I'm not seeing full layer invalidation in Nightly
> or FF24 on OS X. Only the text invalidates on every frame of the animation
> (still undesirable).

This is correct - but I assume that only the text counts as the valid region of the layer - if the layer had an opaque background, perhaps you'd see the entire bounding rectangle invalidate? In my app, I have other non-text items that also invalidate during the movement.
(Reporter)

Comment 5

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #4)
> (In reply to Jonathan Watt [:jwatt] from comment #3)
> > (In reply to Chris Lord [:cwiiis] from comment #2)
> > > An extra note, I've used paint flashing and confirmed that this is a problem
> > > on Nightly on Android and *not* a problem on b2g18 on FirefoxOS 1.1.
> > 
> > Other than at the start and end of the animation (when the layer for the div
> > becomes active/deactive), I'm not seeing full layer invalidation in Nightly
> > or FF24 on OS X. Only the text invalidates on every frame of the animation
> > (still undesirable).
> 
> This is correct - but I assume that only the text counts as the valid region
> of the layer - if the layer had an opaque background, perhaps you'd see the
> entire bounding rectangle invalidate? In my app, I have other non-text items
> that also invalidate during the movement.

Just to add, I assume that the beginning and end showing a larger rectangle is because the layer gets flattened into the background when the animation isn't active (which is kind of odd that it's instant, don't we have a time-out for this?)
(In reply to Chris Lord [:cwiiis] from comment #4)
> if the layer had an opaque background, perhaps you'd see the
> entire bounding rectangle invalidate?

Setting "background-color" on the page div doesn't seem to cause the whole layer to invalidate either. It seems to be something specific to the text content.

(In reply to Chris Lord [:cwiiis] from comment #5)
> Just to add, I assume that the beginning and end showing a larger rectangle
> is because the layer gets flattened into the background when the animation
> isn't active

Right.

> (which is kind of odd that it's instant, don't we have a
> time-out for this?)

We do - GENERATION_MS is the constant that we use. Note that it's now set to 100ms instead of the 500ms that it used to be set to, and that may be a short enough of an interval that it seems instant to you.
(Reporter)

Comment 7

5 years ago
huh, so that's unexpected - this actually works fine in Firefox 23, so it must be a (relatively) recent regression. Adding regressionwindow-wanted.
Keywords: regressionwindow-wanted
(Reporter)

Comment 8

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #7)
> huh, so that's unexpected - this actually works fine in Firefox 23, so it
> must be a (relatively) recent regression. Adding regressionwindow-wanted.

I take it back... The attached example is still broken in 23, but my personal app isn't - which is kind of interesting. I expect something is going wrong in layer building and the association between frame and layer is getting lost(?)
Created attachment 817778 [details]
testcase simplified further

This is ultra bad.
Assignee: nobody → roc
Here's what's happening:
-- This testcase has the viewport defaulting to overflow:auto, so a scrollbar appears for the offscreen content.
-- As the transform changes, the overflow area of the viewport changes and nsGfxScrollFrameInner::UpdateOverflow decides we need to update scrollbars, so schedules a reflow of the scrollframe.
-- The scrollframe reflow uses NS_FRAME_IS_DIRTY, which basically means we have to reflow the scrollframe and everything under it! This is dumb.
-- We reflow the text, and reflowing a text frame always invalidates it, so we have to repaint the text all the time.
The immediate workaround is to use overflow:hidden on the viewport in any application like this.

We should definitely fix item 3 in my list above so that we don't force reflows of the descendants of the scrollframe.

Another thing we can do, and probably should do but don't have any compelling testcase for yet, is to throttle the rate at which we update scrollbars due to animated transformed descendants. Updating the scrollbars once every 250ms would be quite good enough.
Created attachment 817790 [details] [diff] [review]
fix

This is egregious. This bug has been present since we introduced UpdateOverflow.
Attachment #817790 - Flags: review?(tnikkel)
Note that with this fix, on some configurations you still see unnecessary invalidation on the testcases in this bug. That's because the transformed element has text on a transparent background, so on platforms without component alpha support but with subpixel AA for text, we can't give the element its own layer.
Duplicate of this bug: 922918
Attachment #817790 - Flags: review?(tnikkel) → review+

Comment 15

5 years ago
Created attachment 818178 [details]
Screenshot of Attachment #816862 [details] on FxOS 1.2 on Geeksphone Keon

Chris, does this attached screenshot show the problem you expect or another problem?
Flags: needinfo?(chrislord.net)

Updated

5 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 16

5 years ago
(In reply to Mike Lee [:mlee] from comment #15)
> Created attachment 818178 [details]
> Screenshot of Attachment #816862 [details] on FxOS 1.2 on Geeksphone Keon
> 
> Chris, does this attached screenshot show the problem you expect or another
> problem?

This is another problem. The bug described here isn't visual (you'd have to enable paint flashing to determine whether it's happening).
Flags: needinfo?(chrislord.net)
(Reporter)

Comment 17

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> The immediate workaround is to use overflow:hidden on the viewport in any
> application like this.
> 
> We should definitely fix item 3 in my list above so that we don't force
> reflows of the descendants of the scrollframe.
> 
> Another thing we can do, and probably should do but don't have any
> compelling testcase for yet, is to throttle the rate at which we update
> scrollbars due to animated transformed descendants. Updating the scrollbars
> once every 250ms would be quite good enough.

Ah, adding overflow:hidden to the body fixes my issue and I'm ashamed I didn't notice this. On the other hand, there does appear to be another bug here - in my app, I had all my content in a container on the body (because fennec doesn't respect overflow:hidden on the body) and the body had no scrollable area, yet it was still invalidating. I'll attach another example showing this.

Also, may I suggest that the updating delay be 500ms? That would cover most UI transitions, which tend to be in the 150-400ms range in my experience (and those are the numbers I tend to pick when writing my own apps too).
(Reporter)

Comment 18

5 years ago
Created attachment 818344 [details]
Another manifestation of this bug (perhaps already fixed?)

I'll test this when this patch lands in Nightly, but this attached test-case is more similar to how the bug manifested in my app.
https://hg.mozilla.org/mozilla-central/rev/45d9e6cd3473
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

5 years ago
Depends on: 928607
(Reporter)

Comment 21

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #18)
> Created attachment 818344 [details]
> Another manifestation of this bug (perhaps already fixed?)
> 
> I'll test this when this patch lands in Nightly, but this attached test-case
> is more similar to how the bug manifested in my app.

This is also fixed - good stuff :)

Updated

5 years ago
Blocks: 917060

Updated

5 years ago
Whiteboard: [c= p= s= u=] → [c= p= s=2013.10.25 u=]
Comment on attachment 817790 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: column flicker - bug 917060
Testing completed (on m-c, etc.): on m-c since 2013-10-18, currently in Aurora(27)
Risk to taking this patch (and alternatives if risky): low risk, but we need bug 928607 too
String or IDL/UUID changes made by this patch: none

Landing bug 926706 and bug 928607 on Beta(26) to fix/wallpaper bug 917060
should be straightforward and lower risk than backing out what caused that bug.
Attachment #817790 - Flags: approval-mozilla-beta?
Attachment #817790 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/a9fe3f739b3f
status-firefox26: --- → fixed
status-firefox27: --- → fixed
status-b2g-v1.2: --- → fixed

Updated

4 years ago
Depends on: 985303
Issue is resolved - clearing old keywords - qa-wanted clean-up
Keywords: regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.