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

Status: RESOLVED WORKSFORME

(Core :: Graphics: Layers, defect)

Description

[Geoffrey D.]

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

Comment 1

[Geoffrey D.]

Attachment: bug test case

Comment 2

[Alice0775 White]

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

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Benoit, does it ring a bell? Thanks

Comment 4

[Benoit Girard (:BenWa)]

Request uplift approval to beta.

Comment 5

[Benoit Girard (:BenWa)]

To clarify, I requested uplift here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1013769#c8

Comment 6

[Benoit Girard (:BenWa)]

I've uplifted this in bug 1013769:
https://hg.mozilla.org/releases/mozilla-beta/rev/55fb3669faea

Comment 7

[Petruta Horea [:phorea], Desktop Test Engineering]

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.

Comment 8

[Alice0775 White]

Confirmed, the problem exists in Beta31.0b4.

Comment 9

[Geoffrey D.]

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

Has the fix been abandoned for the version 31?

Comment 10

[Matt Woodrow (:mattwoodrow)]

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.

Comment 11

[Geoffrey D.]

It doesn't happen in ff32-Aurora. But it is still there on the latest beta (31).

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Benoit?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Adding regressionwindow-wanted to find where this was fixed between 31 and 32. Hopefully it's something we can uplift.

Comment 16

[Petruta Horea [:phorea], Desktop Test Engineering]

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!

Comment 17

[Alice0775 White]

I think that the patch of Bug 759252 needs to uplift to Firefox31.
(Because Bug 1013769 is depend on  Bug 759252, see Bug 1016643)

Comment 18

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 19

[Matt Woodrow (:mattwoodrow)]

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

Comment 20

[Petruta Horea [:phorea], Desktop Test Engineering]

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

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

I'm not sure how bug 759252 is related here, as that is only focused on the tab-loading animation in the tabstrip.

Comment 22

[Matt Woodrow (:mattwoodrow)]

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

Comment 23

[Matt Woodrow (:mattwoodrow)]

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.

Comment 24

[Matt Woodrow (:mattwoodrow)]

We should also enable flattening of component alpha layers for the software compositor, to match the behaviour of BasicLayers.

Comment 25

[Matt Woodrow (:mattwoodrow)]

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

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't repeat layer building if there was only a single layer

Comment 27

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't use PushGroupAndCopyBackground for retained layers

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't flatten active transform layers

Comment 29

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

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

Comment 30

[Matt Woodrow (:mattwoodrow)]

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

Comment 31

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

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.

Comment 32

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

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

Comment 33

[Matt Woodrow (:mattwoodrow)]

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

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Too late for 31 but I will be happy to take a fix for the first 31 esr.

Comment 35

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't set mSupportComponentAlphaChildren unless we actually have them

We're wasting time copying backgrounds up when there's no component alpha.

Comment 36

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't flatten active transform layers v2

Comment 37

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

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!!!

Comment 38

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

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!

Comment 39

[Matt Woodrow (:mattwoodrow)]

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

Comment 40

[Matt Woodrow (:mattwoodrow)]

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

Comment 41

[Carsten Book [:Tomcat]]

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

Comment 42

[Matt Woodrow (:mattwoodrow)]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[Petruta Horea [:phorea], Desktop Test Engineering]

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.

Comment 45

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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

Comment 46

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 47

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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

Comment 49

[u279076]

Florin, could you please check if this is still an issue?

Comment 50

[Bogdan Maris, Desktop Test Engineering]

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.