926706 - Over-invalidation with animated transforms on window-sized divs

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Lord [:cwiiis]]

Attachment: 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 :)

Comment 1

[Mike Lee [:mlee]]

Chris, what's the end-user impact and do you have numbers for the performance impact of this problem?

Comment 2

[Chris Lord [:cwiiis]]

(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.

Comment 3

[Jonathan Watt [:jwatt]]

(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).

Comment 4

[Chris Lord [:cwiiis]]

(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.

Comment 5

[Chris Lord [:cwiiis]]

(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?)

Comment 6

[Jonathan Watt [:jwatt]]

(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.

Comment 7

[Chris Lord [:cwiiis]]

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

Comment 8

[Chris Lord [:cwiiis]]

(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(?)

Comment 9

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

Attachment: testcase simplified further

This is ultra bad.

Comment 10

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

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.

Comment 11

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

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.

Comment 12

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

Attachment: fix

This is egregious. This bug has been present since we introduced UpdateOverflow.

Comment 13

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

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.

Comment 15

[Mike Lee [:mlee]]

Attachment: Screenshot of Attachment #816862 on FxOS 1.2 on Geeksphone Keon

Chris, does this attached screenshot show the problem you expect or another problem?

Comment 16

[Chris Lord [:cwiiis]]

(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).

Comment 17

[Chris Lord [:cwiiis]]

(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).

Comment 18

[Chris Lord [:cwiiis]]

Attachment: 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.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/45d9e6cd3473

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45d9e6cd3473

Comment 21

[Chris Lord [:cwiiis]]

(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 :)

Comment 22

[Mats Palmgren (inactive)]

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.

Comment 23

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a9fe3f739b3f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a9fe3f739b3f

Comment 25

[Joshua Mitchell (Inactive)]

Issue is resolved - clearing old keywords - qa-wanted clean-up