Closed Bug 1211360 Opened 6 years ago Closed 6 years ago

Jittery CSS animation

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- unaffected
firefox43 --- unaffected
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 --- fixed
b2g-v2.5 --- unaffected
b2g-master --- fixed

People

(Reporter: cers, Assigned: sinker)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(3 files, 5 obsolete files)

Animations like the one I have attached have become very jittery recently.

mozregression yields the following ranges:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=11dc79e232110ba6de5179e46dfbda77b52a88c3&tochange=4313752f69956ae248bd4e7ff3913c8dd4252698
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7641104770a80015e63597b58cb312fefcbd9ab4&tochange=621ab19e86db28c38bbbf9119fbf6831ea344c54

Which implicates bug 1097464. As of most recent nightly (2015-10-04) the problem persists.

STR:
1) load attached test in nightly from 2015-09-18
2) load attached test in nightly from 2015-09-19

Result:
The circles appear jittery in nightly from 2015-09-19

Expected result:
Animation should look the same (or at least stable) as in nightly from 2015-09-18
Attachment #8669551 - Flags: feedback?(tlee)
I have found the root cause is
  https://hg.mozilla.org/mozilla-central/rev/dc43e93fb4c2
The result of snapping during doing animation and drawing are different.
Attachment #8669551 - Flags: feedback?(tlee)
Christian, could you check this patch.  Thank you!
(In reply to Thinker Li [:sinker] from comment #4)
> Christian, could you check this patch.  Thank you!

Looks like it fixes the bug for me! Thanks for the quick response :-)
Attachment #8669574 - Flags: review?(roc)
Comment on attachment 8669574 [details] [diff] [review]
Don't snapping for non-translation 3D transform

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

::: gfx/layers/Layers.cpp
@@ +624,5 @@
>      return result;
>    }
>  
>    if(aTransform.IsSingular() ||
> +     (aTransform._14 != 0 || aTransform._24 != 0 || aTransform._34 != 0) ||

Can you replace this line with a method on aTransform?

@@ +626,5 @@
>  
>    if(aTransform.IsSingular() ||
> +     (aTransform._14 != 0 || aTransform._24 != 0 || aTransform._34 != 0) ||
> +     aTransform.HasNonTranslation() ||
> +     !aTransform.HasNonIntegerTranslation()) {

We're calling HasNonTranslation() twice here. How about creating TranslationIsInteger that tests just 41/42/43 and calling that here?
Attachment #8669574 - Flags: review?(roc)
Comment on attachment 8669574 [details] [diff] [review]
Don't snapping for non-translation 3D transform

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

::: gfx/layers/Layers.cpp
@@ +626,5 @@
>  
>    if(aTransform.IsSingular() ||
> +     (aTransform._14 != 0 || aTransform._24 != 0 || aTransform._34 != 0) ||
> +     aTransform.HasNonTranslation() ||
> +     !aTransform.HasNonIntegerTranslation()) {

I believe and trust modern compilers to optimize inline code like this very well.  And, it is for gcc with -O2 according a field test.  Nowadays, functions, like fabs(), in standard libraries are inline and optimized by compilers, and compilers try to make anything small enough inline.  Even linkers are involved for optimization.
--
Attachment #8669574 [details] [diff] - Attachment is obsolete: true
Attachment #8670586 - Flags: review?(roc)
Attachment #8669574 - Attachment is obsolete: true
Attachment #8670586 - Flags: review?(roc)
(In reply to Thinker Li [:sinker] from comment #7)
> I believe and trust modern compilers to optimize inline code like this very
> well.  And, it is for gcc with -O2 according a field test.

OK, but it's also simpler code since TranslationIsInteger is simpler than HasNonIntegerTranslation.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> (In reply to Thinker Li [:sinker] from comment #7)
> > I believe and trust modern compilers to optimize inline code like this very
> > well.  And, it is for gcc with -O2 according a field test.
> 
> OK, but it's also simpler code since TranslationIsInteger is simpler than
> HasNonIntegerTranslation.

I agree with you, but it would make two different sets of functions for Matrix and Matrix4x4, my major concern.  And, for the 2D case, 

  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#623

I think it should be change too for consistent.
If you think it is OK, I would do that.
Comment on attachment 8670586 [details] [diff] [review]
Don't snapping for non-translation 3D transform, v2

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

::: gfx/2d/Matrix.h
@@ +1032,5 @@
> +  /**
> +   * Return true if the matrix has any translation.
> +   */
> +  bool HasTranslation() const {
> +    return _14 != 0 || _24 != 0 || _34 != 0;

This name isn't correct. These components aren't "translation", they're coefficients for the w component.
Attachment #8670586 - Flags: review-
Don't forget about this bug, I nearly did :-)
Flags: needinfo?(tlee)
--
Attachment #8670586 [details] [diff] - Attachment is obsolete: true
Attachment #8676714 - Flags: review?(roc)
Attachment #8670586 - Attachment is obsolete: true
> This name isn't correct. These components aren't "translation", they're
> coefficients for the w component.

I must lost my mind to name this function as this.
Flags: needinfo?(tlee)
(In reply to Thinker Li [:sinker] from comment #14)
> > This name isn't correct. These components aren't "translation", they're
> > coefficients for the w component.
> 
> I must lost my mind to name this function as this.

:-)
Comment on attachment 8676714 [details] [diff] [review]
Don't snapping for non-translation 3D transform, v3

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

::: gfx/layers/Layers.cpp
@@ +637,5 @@
>      return result;
>    }
>  
>    if(aTransform.IsSingular() ||
> +     (aTransform._14 != 0 || aTransform._24 != 0 || aTransform._34 != 0) ||

I still think this should be a function. How about "HasPerspectiveComponent"? Probably could check _44 as well just for clarity.
Attachment #8676714 - Flags: review?(roc)
--
Attachment #8676714 [details] [diff] - Attachment is obsolete: true
Attachment #8678454 - Flags: review?(roc)
Attachment #8676714 - Attachment is obsolete: true
--
Attachment #8678454 [details] [diff] - Attachment is obsolete: true
Attachment #8678544 - Flags: review?(roc)
Attachment #8678454 - Attachment is obsolete: true
Attachment #8678454 - Flags: review?(roc)
Comment on attachment 8678544 [details] [diff] [review]
Don't snapping for non-translation 3D transform, v4

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

::: layout/reftests/transform-3d/reftest.list
@@ +19,5 @@
>  == preserve3d-2c.html preserve3d-2-ref.html
>  == preserve3d-2d.html preserve3d-2-ref.html
>  == preserve3d-3a.html preserve3d-3-ref.html
>  skip-if(B2G||Mulet) == preserve3d-4a.html green-rect.html # Initial mulet triage: parity with B2G/B2G Desktop
> +fuzzy-if(gtkWidget,4,200) fuzzy-if(Android&&AndroidVersion>=15,4,300) fuzzy-if(winWidget,2,100) == preserve3d-5a.html preserve3d-5-ref.html

This was only hitting reftest-noaccel, so you could make the check winWidget&&!layersGPUAccelerated to be a bit more strict.
It is a useful information.  Thank you!
--
Attachment #8678544 [details] [diff] - Attachment is obsolete: true
Attachment #8678670 - Flags: review?(roc)
Attachment #8678544 - Attachment is obsolete: true
Attachment #8678544 - Flags: review?(roc)
No longer blocks: 1220317
Duplicate of this bug: 1220317
ni=Thinker to get this landed & backported to Aurora (which just branched off of trunk in the last day or so)

Also: when verifying that this bug is fixed, please also double-check the testcase on duplicate bug 1220317 -- attachment 8681555 [details]. (and feel free to un-dupe if the testcase here becomes fixed but bug 1220317's testcase remains jittery)
Assignee: nobody → tlee
Flags: needinfo?(tlee)
https://hg.mozilla.org/mozilla-central/rev/08819a7485b7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8678670 [details] [diff] [review]
Don't snapping for non-translation 3D transform, v5

Approval Request Comment
[Feature/regressing bug #]: regression of bug 1097464
[User impact if declined]: Some animations would be jittery.
[Describe test coverage new/current, TreeHerder]: Have passed all reftests, and other testcases.
[Risks and why]: see https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=451a18579143
[String/UUID change made/needed]:
Attachment #8678670 - Flags: approval-mozilla-aurora?
Comment on attachment 8678670 [details] [diff] [review]
Don't snapping for non-translation 3D transform, v5

Given that the automated tests pass, seems safe to uplift to Aurora44.
Attachment #8678670 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Christian, could you please confirm that the jittery animation issues have been fixed as expected on the latest Nightly build? Thanks in advance.
Flags: needinfo?(cers)
(In reply to Ritu Kothari (:ritu) from comment #32)
> Christian, could you please confirm that the jittery animation issues have
> been fixed as expected on the latest Nightly build? Thanks in advance.

I can confirm that the attached test case looks as it should in Nightly 45.0a1 (2015-11-04)
Flags: needinfo?(cers)
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Backed out from Aurora44 so that bug 1097464 could be backed out cleanly. It remains fixed on Fx45+ where bug 1097464 is still landed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/636ac1e9d3e7
Blocks: 1240783
This along with the change that caused this regression were backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d
You need to log in before you can comment on or make changes to this bug.