CSS animation makes containing text jump/shake with hardware acceleration disabled

RESOLVED WORKSFORME

Status

()

Core
Graphics: Layers
RESOLVED WORKSFORME
4 years ago
2 years ago

People

(Reporter: Geoffrey D., Assigned: mattwoodrow)

Tracking

({regression})

31 Branch
mozilla33
x86_64
Windows 8
regression
Points:
---

Firefox Tracking Flags

(firefox30 wontfix, firefox31 wontfix, firefox33 wontfix, firefox34 wontfix, firefox35 wontfix, firefox-esr24 unaffected, firefox-esr31 wontfix)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Steps to reproduce:
1/ Open the attached html file or go to http://fortawesome.github.io/Font-Awesome/examples/#spinning and notice that the rotation animation is smooth.
2/ Disable hardware acceleration (options -> advanced -> browsing section)
3/ Restart Firefox and go back to one of the pages in step 1.

Actual results:
The content of the animation is shaking/reflowing constantly.

Expected results:
The animation should be smooth (as when the hardware acceleration is enabled).

Additional informations:
This could be linked to bug 492360
(Reporter)

Comment 1

4 years ago
Created attachment 8438097 [details]
bug test case

Comment 2

4 years ago
And this seemed fixed by Bug 1013769 in 32.

I think it is worth to fix in 31 because it will be ESR31.


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5a62ab693929
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20131211131243
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d8fb025ca7d2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20131211132759
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5a62ab693929&tochange=d8fb025ca7d2
Regressed by:
d8fb025ca7d2	Benoit Girard — Bug 948531 - Layerize elements with transition or animation immediately. r=mattwoodrow
Blocks: 948531
Status: UNCONFIRMED → NEW
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
status-firefox-esr24: --- → unaffected
tracking-firefox31: --- → ?
Component: General → Graphics: Layers
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Benoit, does it ring a bell? Thanks
Flags: needinfo?(bgirard)
Request uplift approval to beta.
To clarify, I requested uplift here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1013769#c8
Flags: needinfo?(bgirard)
I've uplifted this in bug 1013769:
https://hg.mozilla.org/releases/mozilla-beta/rev/55fb3669faea

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Assignee: nobody → matt.woodrow
status-firefox30: affected → wontfix
status-firefox31: affected → fixed
Target Milestone: --- → mozilla32
Keywords: verifyme
tracking-firefox31: ? → +
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

With HWA off the issue still reproduces on Firefox 31 beta 4 under Win 7 64-bit.
Keywords: verifyme

Comment 8

4 years ago
Confirmed, the problem exists in Beta31.0b4.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

3 years ago
status-firefox31: fixed → ?
(Reporter)

Comment 9

3 years ago
Can confirm this in 31.0b5 too. I removed the "fixed" flag from ff 31.

Has the fix been abandoned for the version 31?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 10

3 years ago
The fix was uplifted to Beta already so this should be fixed. Does it happen in aurora (firefox 32)?

It sounds like bug 1013769 wasn't sufficient to fix this bug.
Flags: needinfo?(matt.woodrow)
(Reporter)

Comment 11

3 years ago
It doesn't happen in ff32-Aurora. But it is still there on the latest beta (31).
Benoit?
status-firefox31: ? → affected
Flags: needinfo?(bgirard)
Let's track this for 32; that release is when a lot of OMTC Windows work landed (see bug 899785 dependents), so the uplift to 31 may not be trivial.
Flags: needinfo?(bgirard) → needinfo?(milan)
Bas, we're already tracking some "this doesn't work with HWA off", so adding you here on the off chance your work there helps.  It also adds to the "it is bad we don't test this configuration" argument.
(Assignee)

Comment 15

3 years ago
Adding regressionwindow-wanted to find where this was fixed between 31 and 32. Hopefully it's something we can uplift.
Keywords: regressionwindow-wanted
On 32 it worked when bug 1013769 landed, this bug was uplifted to 31. 
I can see that the beta changeset also contains modifications in gfx/2d/Matrix.h.

Last good revision: 1e712b724d17 (2014-05-29)
First bad revision: e017c15325ae (2014-05-28)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e017c15325ae&tochange=1e712b724d17

Matt, should I continue investigating anything else? Thanks!
Flags: needinfo?(matt.woodrow)

Comment 17

3 years ago
I think that the patch of Bug 759252 needs to uplift to Firefox31.
(Because Bug 1013769 is depend on  Bug 759252, see Bug 1016643)
Alice, it might bit late in the cycle now. Beta 9 is going to build today.
Maybe Jared, the author of the patch in bug 759252, have an opinion on this.
Flags: needinfo?(jaws)
(Assignee)

Comment 19

3 years ago
I suspect bug 1013769 fixed the bug with OMTC+h/w acceleration disabled since we had OMTC enabled on 32 at that time.

31 doesn't have OMTC, so the bug there will be different code.

Does this still reproduce in the latest 32 builds (with OMTC disabled - check graphics section of about:support, GPU Accelerated Windows for "(OMTC)")?

Does it reproduce on nightly without OMTC (layers.offmainthreadcomposition.enabled=false)?

If it's fixed in either of those places, then a regression range of where that happened would be awesome :)
Flags: needinfo?(matt.woodrow) → needinfo?(petruta.rasa)
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> Does this still reproduce in the latest 32 builds (with OMTC disabled - check graphics section of about:support, GPU Accelerated Windows for "(OMTC)")?
> Does it reproduce on nightly without OMTC (layers.offmainthreadcomposition.enabled=false)?

It reproduces on latest Aurora 32.0a2 20140709004001 and latest Nightly 33.0a1 20140713030204 with OMTC disabled if HWA is off (GPU Accelerated Windows	0/1 Basic ).
Flags: needinfo?(petruta.rasa)
I'm not sure how bug 759252 is related here, as that is only focused on the tab-loading animation in the tabstrip.
Flags: needinfo?(jaws)
(Assignee)

Comment 22

3 years ago
(In reply to Petruta Rasa [QA] [:petruta] from comment #20)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> > Does this still reproduce in the latest 32 builds (with OMTC disabled - check graphics section of about:support, GPU Accelerated Windows for "(OMTC)")?
> > Does it reproduce on nightly without OMTC (layers.offmainthreadcomposition.enabled=false)?
> 
> It reproduces on latest Aurora 32.0a2 20140709004001 and latest Nightly
> 33.0a1 20140713030204 with OMTC disabled if HWA is off (GPU Accelerated
> Windows	0/1 Basic ).

Great, thanks. So we've got nothing to uplift since the bug still exists for the configuration 31 will ship with.

 (In reply to [Away for most of July] (please needinfo? me) Jared Wein [:jaws] from comment #21)
> I'm not sure how bug 759252 is related here, as that is only focused on the
> tab-loading animation in the tabstrip.

Agreed, that bug was just what exposed an existing graphics issue.
(Assignee)

Comment 23

3 years ago
This happens because we determine that some of the pixels drawn by the text might extend beyond the bounds of the yellow square, and we mark the layer as needing component alpha. BasicLayers doesn't support component alpha, so we flatten it into the background and have to repaint the text during the animation.

I think we should avoid flattening component alpha across an animated transform, since it's likely to look worse than just disabling subpixel-AA for the text.
(Assignee)

Comment 24

3 years ago
We should also enable flattening of component alpha layers for the software compositor, to match the behaviour of BasicLayers.
(Assignee)

Comment 25

3 years ago
Created attachment 8455872 [details] [diff] [review]
Mark BasicCompositor as not supporting component alpha layers

If we're going to start using BasicCompositor instead of BasicLayers then we probably need this to match text quality expectations.
Attachment #8455872 - Flags: review?(roc)
(Assignee)

Comment 26

3 years ago
Created attachment 8455874 [details] [diff] [review]
Don't repeat layer building if there was only a single layer
Attachment #8455874 - Flags: review?(roc)
(Assignee)

Comment 27

3 years ago
Created attachment 8455876 [details] [diff] [review]
Don't use PushGroupAndCopyBackground for retained layers
Attachment #8455876 - Flags: review?(roc)
(Assignee)

Comment 28

3 years ago
Created attachment 8455878 [details] [diff] [review]
Don't flatten active transform layers
Attachment #8455878 - Flags: review?(roc)
Attachment #8455872 - Flags: review?(roc) → review+
Comment on attachment 8455874 [details] [diff] [review]
Don't repeat layer building if there was only a single layer

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3428,5 @@
>  
>      if ((flags & Layer::CONTENT_COMPONENT_ALPHA) &&
>          mRetainingManager &&
>          !mRetainingManager->AreComponentAlphaLayersEnabled() &&
> +        containerLayer->GetFirstChild() && containerLayer->GetNextSibling() &&

I don't understand this. Why are we testing containerLayer->GetNextSibling()? Shouldn't we test containerLayer->GetFirstChild()->GetNextSibling()?
Attachment #8455874 - Flags: review?(roc) → review-
(Assignee)

Comment 30

3 years ago
Created attachment 8455879 [details] [diff] [review]
Don't repeat layer building if there was only a single layer v2

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> I don't understand this. Why are we testing
> containerLayer->GetNextSibling()? Shouldn't we test
> containerLayer->GetFirstChild()->GetNextSibling()?

Yes indeed, simplified to HasMultipleChildren()
Attachment #8455874 - Attachment is obsolete: true
Attachment #8455879 - Flags: review?(roc)
Comment on attachment 8455878 [details] [diff] [review]
Don't flatten active transform layers

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3081,5 @@
>      layer = mNewChildLayers[i];
>  
> +    // Don't propogate CONTENT_COMPONENT_ALPHA flags for active (retained)
> +    // transform layers since it's only used for component alpha flattening
> +    // (see BuildContainerLayerFor) and repainting looks worse than losing

Really? The layers system uses CONTENT_COMPONENT_ALPHA.
Attachment #8455878 - Flags: review?(roc) → review-
Comment on attachment 8455876 [details] [diff] [review]
Don't use PushGroupAndCopyBackground for retained layers

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +102,5 @@
>      *aNeedsClipToVisibleRegion = false;
>      result = aContext;
> +    // Retained layer managers have already drawn any text, so copying
> +    // the background can't help.
> +    if (aLayer->GetContentFlags() & Layer::CONTENT_COMPONENT_ALPHA &&

Parens around & suexpression
Attachment #8455876 - Flags: review?(roc) → review+
Attachment #8455879 - Flags: review?(roc) → review+
(Assignee)

Comment 33

3 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> Comment on attachment 8455878 [details] [diff] [review]
> Don't flatten active transform layers
> 
> Review of attachment 8455878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +3081,5 @@
> >      layer = mNewChildLayers[i];
> >  
> > +    // Don't propogate CONTENT_COMPONENT_ALPHA flags for active (retained)
> > +    // transform layers since it's only used for component alpha flattening
> > +    // (see BuildContainerLayerFor) and repainting looks worse than losing
> 
> Really? The layers system uses CONTENT_COMPONENT_ALPHA.

Only on ThebesLayers afaict. BasicLayers was the only one checking it on a general Layer*, and the earlier patch ensured that this only happens with inactive layer managers.
Keywords: regressionwindow-wanted
Too late for 31 but I will be happy to take a fix for the first 31 esr.
status-firefox31: affected → wontfix
status-firefox-esr31: --- → affected
tracking-firefox-esr31: --- → +
(Assignee)

Comment 35

3 years ago
Created attachment 8456613 [details] [diff] [review]
Don't set mSupportComponentAlphaChildren unless we actually have them

We're wasting time copying backgrounds up when there's no component alpha.
Attachment #8455878 - Attachment is obsolete: true
Attachment #8456613 - Flags: review?(roc)
(Assignee)

Comment 36

3 years ago
Created attachment 8456614 [details] [diff] [review]
Don't flatten active transform layers v2
Attachment #8456614 - Flags: review?(roc)
Flags: needinfo?(milan)
Comment on attachment 8456613 [details] [diff] [review]
Don't set mSupportComponentAlphaChildren unless we actually have them

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

::: gfx/layers/Layers.cpp
@@ +1022,5 @@
>  
>  void
>  ContainerLayer::DefaultComputeSupportsComponentAlphaChildren(bool* aNeedsSurfaceCopy)
>  {
> +  if (!GetContentFlags() & Layer::CONTENT_COMPONENT_ALPHA) {

Parens around & expression!!!
Attachment #8456613 - Flags: review?(roc) → review+
Comment on attachment 8456614 [details] [diff] [review]
Don't flatten active transform layers v2

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3086,5 @@
> +      // Notify the parent of component alpha children unless it's coming from
> +      // within a transformed child since we don't want flattening of component
> +      // alpha layers to happen across transforms. Re-rendering the text during
> +      // transform animations looks worse than losing subpixel-AA.
> +      if (layer->GetContentFlags() & Layer::CONTENT_COMPONENT_ALPHA &&

parens around & expression!
Attachment #8456614 - Flags: review?(roc) → review+
(Assignee)

Comment 39

3 years ago
Created attachment 8458368 [details] [diff] [review]
Don't set mSupportComponentAlphaChildren unless we actually have them v2

Unfortunately the previously version of this patch failed some tests where we weren't propagating CONTENT_COMPONENT_ALPHA up and so we disabled background copying when we needed it.
Attachment #8456613 - Attachment is obsolete: true
Attachment #8458368 - Flags: review?(roc)
Attachment #8458368 - Flags: review?(roc) → review+
(Assignee)

Comment 40

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5dbd56332e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5f96882e6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/84df2aaf5e2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d03cb12a166e
sorry had to backout this changeset in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e97c96651f93 since one of this csets seems to have caused https://tbpl.mozilla.org/php/getParsedLog.php?id=44087909&tree=Mozilla-Inbound (ignore the hg error there :)
(Assignee)

Comment 42

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6dd8895df48
https://hg.mozilla.org/integration/mozilla-inbound/rev/167bb4ee1c8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/151b2045009e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6209437d8f
https://hg.mozilla.org/mozilla-central/rev/b6dd8895df48
https://hg.mozilla.org/mozilla-central/rev/167bb4ee1c8a
https://hg.mozilla.org/mozilla-central/rev/151b2045009e
https://hg.mozilla.org/mozilla-central/rev/7b6209437d8f
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: mozilla32 → mozilla33
status-firefox33: unaffected → fixed
Keywords: verifyme
The issue still reproduces on page http://fortawesome.github.io/Font-Awesome/examples/#spinning under Win 7 64-bit using latest Aurora 33.0a2 20140901004002 and latest Nightly 34.0a1 20140901091008 when:
- HWA is off
- pref layers.offmainthreadcomposition.enabled is either true or false

This doesn't seem to happen with the attached test case.

With Firefox 32.0 RC it reproduces only when OMTC pref is false for both testcases.
Status: RESOLVED → REOPENED
status-firefox33: fixed → affected
status-firefox34: --- → affected
Keywords: verifyme
Resolution: FIXED → ---
(Reporter)

Updated

3 years ago
status-firefox32: unaffected → ?
(Reporter)

Updated

3 years ago
status-firefox35: --- → ?
The issue reproduces with Firefox 35 Beta 1 (BuildID=20141201162954) same as stated in comment 44:
- reproduces with http://fortawesome.github.io/Font-Awesome/examples/#spinning but not with the attached test case
- reproduces when HWA is off, regardless of the setting for layers.offmainthreadcomposition.enabled
status-firefox35: ? → affected
So, to be clear, this did work for a while after it landed on 2014/07/18 (and I assume in the 2014/07/19 nightly), but it stopped since?  Or it never worked with this fix at all?
(In reply to Milan Sreckovic [:milan] from comment #46)
> So, to be clear, this did work for a while after it landed on 2014/07/18
> (and I assume in the 2014/07/19 nightly), but it stopped since?  Or it never
> worked with this fix at all?

I did some additional investigation on Windows 7 x64, using Nightly builds from July 18th and 19th. Here's what I've found out (HWA=OFF for testing profile):
- July 19th (BuildID=20140719030203)
   - OMTC = ON/OFF ===> spinners = NOT OK (shaking), testcase = OK
- July 18th (BuildID=20140718030202)
   - OMTC = ON ===> spinners & testcase = OK
   - OMTC = OFF ===> spinners & testcase = NOT OK (shaking)

It seems that the patch fixed the attached test case for OMTC = OFF, but broke the spinners for OMTC = ON (which used to work on July 18th). Basically it fully fixed the test case, while fully breaking the spinners.

Updated

3 years ago
Duplicate of this bug: 1160431
Florin, could you please check if this is still an issue?
status-firefox32: ? → ---
status-firefox33: affected → wontfix
status-firefox34: affected → wontfix
status-firefox35: affected → wontfix
status-firefox-esr31: affected → wontfix
tracking-firefox31: + → ---
tracking-firefox-esr31: + → ---
Flags: needinfo?(florin.mezei)
I reproduced the initial issue using Firefox 36 RC but I can't reproduce on latest Nightly 45.0a1 using http://fortawesome.github.io/Font-Awesome/examples/#spinning and the testcase attached with or without HWA and with OMTC on or off.
I think its safe to call this WFM.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Flags: needinfo?(florin.mezei)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.