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

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.

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

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?

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

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

Don't forget about this bug, I nearly did :-)

-- Attachment #8670586 [details] [diff] - 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.

(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 [details] [diff] - Attachment is obsolete: true

-- Attachment #8678454 [details] [diff] - Attachment is obsolete: true

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

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)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=349ae9203659 attachment 8681555 [details] has confirmed.

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

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.

Christian, could you please confirm that the jittery animation issues have been fixed as expected on the latest Nightly build? Thanks in advance.

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

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

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