Closed Bug 1023677 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Graphics: Layers, defect)

31 Branch
x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED WORKSFORME
mozilla33
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- wontfix
firefox-esr24 --- unaffected
firefox-esr31 --- wontfix

People

(Reporter: geo.demoulin, Assigned: mattwoodrow)

References

Details

(Keywords: regression)

Attachments

(6 files, 3 obsolete files)

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
Attached file bug test case
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
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)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → matt.woodrow
Target Milestone: --- → mozilla32
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
Confirmed, the problem exists in Beta31.0b4.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
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)
It doesn't happen in ff32-Aurora. But it is still there on the latest beta (31).
Benoit?
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.
Adding regressionwindow-wanted to find where this was fixed between 31 and 32. Hopefully it's something we can uplift.
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)
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)
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)
(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.
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.
We should also enable flattening of component alpha layers for the software compositor, to match the behaviour of BasicLayers.
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)
Attachment #8455878 - Flags: review?(roc)
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-
(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+
(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.
Too late for 31 but I will be happy to take a fix for the first 31 esr.
We're wasting time copying backgrounds up when there's no component alpha.
Attachment #8455878 - Attachment is obsolete: true
Attachment #8456613 - Flags: review?(roc)
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+
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)
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 :)
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
Keywords: verifyme
Resolution: FIXED → ---
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
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.
Duplicate of this bug: 1160431
Florin, could you please check if this is still an issue?
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
Closed: 5 years ago4 years ago
Flags: needinfo?(florin.mezei)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.