I plan to modify test_transitions_per_property.html to call a function in DOMWindowUtils which will sample the animations on the compositor at the specified time and makes sure that the interpolated nsCSSValueList is the same as the one computed on the main thread.

But the main thread isn't going to be interpolating all async animations after your other patch, and anyway this would be a racy strategy, right? Or maybe I misunderstand what you propose.

Even with throttling animations, we still sample the animations. We just don't flush style. This shouldn't be racy because we can manually update the time on the main thread, send a sync messages to the compositor to update time, and have the compositor send back a sync message with the interpolated value.

I thought we weren't going to sample animations at all in common cases, like elements within overflow:hidden (except when input events etc. force that). Throttling isn't much of a solution to the problems we discussed. It sounds pretty tricky to me to try to manually force both the refresh driver time and compositor time. I'm afraid this will hit subtleties like clocks going backwards, or time being forced after animations complete. What if we took the existing tests, but simply changed the way they checked the computed style to be this new magical compositor request? We also need to deal with animations that we don't async-ify yet, so there may be value in having two separate test runs, and moving tests back and forth between the async/not-async buckets as we extend the platform support.

Yes, that's what I mean by throttling. We still sample in any case. Sampling is not the slow part - flushing is. We already manually set refresh driver times on the main thread for animation tests. On the compositor, we pass in a time to sample animations, so we can just pass this time from the main thread, and this time will be set in the test script.

Really need an owner here, especially as my recommendation is to leave async animation goodness disabled for fennec until we test it better. b2g has been flying by the seat of its pants and dealing with fallout and regressions as they occur, because the feature is so critical for b2g. Time to be mature engineers! ;)

We've got the testing requirements from nrc, roc, and dbaron brainstorming. Pasting this here while I have another think at who to assign this to... nrc>>> We need to be able to query properties of layers from JS in tests and get the composited values. To do that we need to read a bunch of properties from layers on the compositor (opacity, transform and/or absolute position, others in the future). We then need to pass that back to the main thread and propagate it (probably via the layer manager) to the DOM and then expose it as special powers only stuff. I'm not sure exactly how to talk about layers from js/dom land, but I guess we could query on an element and return 0 if it did not get layerised. I think getting the values on the compositor thread and passing back to main thread is not too hard, either via the layer and then up to the manager, or up to the shadow manager/compositor and then back to main thread. I'm not sure how to get that stuff back to DOM, but I don't suppose it is too hard. I think this infrastructure would be useful for doing other tests for OMTC stuff. Secondly, we need to be able to spoof the time on the compositor. We can do this for the refresh driver, but CompositorParent does it's own thing using Timestamp::Now, we would need to abstract that a bit and add in the ability to supply our own 'time'. I guess we would need a fair bit of wiring to pass that into layers and across IPC. Both things would need new API on layers and web-facing API, but the latter only for tests with special powers. roc>>> Then I guess we would need a single nsDOMWindowUtils method for that, which takes an element and a property name, finds the primary frame for the element, then calls FrameLayerBuilder::GetDedicatedLayer(frame, itemtype) where the itemtype is deduced from the property name. Then we can query some new API on the layer to fetch the data from the compositor.

There's also one simpler piece, which is that we should have a way of forcing values to run through the value conversion code and interpolation code. In other words, we should be able to test that the codepath that sends the values to the compositor (converting from nsStyleAnimation::Value to the ipdl structures in AddAnimationsForProperty and AddTransformFunctions in nsDisplayList.cpp, and then back in Layer::SetAnimations and CreateCSSValueList) and invokes nsStyleAnimation::Interpolate from SampleValue (in CompositorParent) produces the same results as the non-OMTA codepath that just invokes nsStyleAnimation::Interpolate directly on the results of nsStyleAnimation::ExtractComputedValue. With that, we can have layout/style/test/test_transitions_per_property.html test (for properties that we animate on the compositor) both codepaths rather than just the latter.

It's incorrect to update the style rule refresh time without updating the style rule. Because of this, we can run into issues if we update the refresh time and then try to flush on the same tick and don't update the style rule.

Pretty useless with infallible new

Comment on attachment 753104 [details] [diff] [review] Part 1: Fix animation throttling This seems right, though also a bit scary. Might want to check with :nrc as well. And did you test this in a debug build and make sure you didn't hit any assertions?

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12) > And did you test this in a debug build and make sure you didn't hit any > assertions? Yup, things look good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/68cc545b9106 https://hg.mozilla.org/integration/mozilla-inbound/rev/be938f823e77

Comment on attachment 753104 [details] [diff] [review] Part 1: Fix animation throttling Review of attachment 753104 [details] [diff] [review]: ----------------------------------------------------------------- This is the change we discussed the other day, right? If so r=me, otherwise I should think about it some more.

(In reply to Nick Cameron [:nrc] from comment #15) > Comment on attachment 753104 [details] [diff] [review] > Part 1: Fix animation throttling > > Review of attachment 753104 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is the change we discussed the other day, right? Yes. https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1c87602308 Also pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbe372ab8e0 to make MatrixToCSSValue public.

https://hg.mozilla.org/mozilla-central/rev/68cc545b9106 https://hg.mozilla.org/mozilla-central/rev/be938f823e77 https://hg.mozilla.org/mozilla-central/rev/9bbe372ab8e0 https://hg.mozilla.org/mozilla-central/rev/0f1c87602308

Comment on attachment 753700 [details] [diff] [review] Part 4: Add the ability to pause and sample animations on compositors at a specified time Review of attachment 753700 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorParent.h @@ +162,5 @@ > * the compositor thread. > */ > static const LayerTreeState* GetIndirectShadowTree(uint64_t aId); > > + static void SetTimeAndSampleAnimations(TimeStamp aTime, bool aIsTesting); Document this method and its parameters.

Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1cf4694a13

Backed out for not building on windows and android. https://hg.mozilla.org/integration/mozilla-inbound/rev/aae356806829

Comment on attachment 754631 [details] [diff] [review] Part 5: Add a function to get opacity and transform property values from layers The GetAttributeValue thing on nsIDOMWindowUtils seems weird -- why not just add two methods? That's basically what methods are for. And I'm not crazy about using nsIDOMCSSValue -- we're really trying to avoid adding new uses of those interfaces. You also need to rev the IID of nsIDOMWindowUtils. Otherwise this seems fine to me, though I'm not sure what sort of feedback you were looking for.

Although if it's specifically about getting CSS properties, maybe a GetOMTCCSSPropertyValue would make sense? (I'd rather it return a string than nsIDOMCSSValue, if that makes sense.)

Comment on attachment 754631 [details] [diff] [review] Part 5: Add a function to get opacity and transform property values from layers Review of attachment 754631 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/base/nsIDOMWindowUtils.idl @@ +1398,5 @@ > void runBeforeNextEvent(in nsIRunnable runnable); > + > + /* > + * Returns the value of a given attribute. If the attribute is animated off the > + * main thread, this function will fetch the correct value from the compositor. Document the valid attribute names!

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #24) > Comment on attachment 754631 [details] [diff] [review] > Part 5: Add a function to get opacity and transform property values from > layers > > The GetAttributeValue thing on nsIDOMWindowUtils seems weird -- why not just > add two methods? That's basically what methods are for. The idea is that test code can call this method without worrying about which properties are OMTA'ed, and it will always return the right thing because it falls back to getComputedStyle. > And I'm not crazy > about using nsIDOMCSSValue -- we're really trying to avoid adding new uses > of those interfaces. OK, I can just return the value's cssText property. > Otherwise this seems fine to me, though I'm not sure what sort of feedback > you were looking for. I mostly figured you would be interested in taking a look.

(In reply to David Zbarsky (:dzbarsky) from comment #27) > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from > comment #24) > > Comment on attachment 754631 [details] [diff] [review] > > Part 5: Add a function to get opacity and transform property values from > > layers > > > > The GetAttributeValue thing on nsIDOMWindowUtils seems weird -- why not just > > add two methods? That's basically what methods are for. OK, then I guess a single method is fine, but it needs a better name. CSS properties are properties, not attributes. How about calling the method nsIDOMWindowUtils.getOMTAOrComputedStyle() ? That describes what it does much more clearly. And just returning the .cssText sounds great. (And don't forget to rev the IID.)

And please don't use the term "attribute" for CSS properties in the comments, either.

Comment on attachment 755685 [details] [diff] [review] Part 6: Remove a redundant if r=dbaron (Though in general I'd like to see commit messages as part of the review request. "remove a redundant if" is fine as a commit message here; using the summary of this bug is not.)

Turns out I wasn't testing the right thing. This works better, modulo some rounding error in the 10^-6 spot for numbers that should be 0. Not sure if there's much we can do about that.

(In reply to David Zbarsky (:dzbarsky) from comment #35) > Created attachment 756141 [details] [diff] [review] > Part 7: Fix RecvGetTransform > > Turns out I wasn't testing the right thing. This works better, modulo some > rounding error in the 10^-6 spot for numbers that should be 0. Not sure if > there's much we can do about that. The other option is to save the untranslated transform on the layer, but I don't think we want to do that since this is just testing code.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf84683f9e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6f8502d1af

Comment on attachment 756342 [details] [diff] [review] Part 9: Fix matrix() functions Yeah, the old code here is clearly wrong. But if we assume the matrix3d code is right (i.e., assume that we don't need to flip row-major and column-major here), then I think this is also wrong. I think what you should end up doing is assigning: Item(1) -> matrix._11 (as you do) Item(2) -> matrix._12 (as you do) Item(3) -> matrix._21 (as you do) Item(4) -> matrix._22 (as you do) Item(5) -> matrix._41 Item(6) -> matrix._42 I tested that the behavior of certain components of matrix() and matrix3d() matches each other by fiddling with this testcase in Firefox: http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Abody%3Ediv%20%7B%20border%3A%20thin%20solid%20blue%3B%20width%3A%20100px%3B%20margin%3A%205px%200%3B%20%7D%0Abody%3Ediv%3Ediv%20%7B%20border%3A%20thin%20solid%20green%3B%20height%3A%2020px%3B%20transform-origin%3A%200%200%3B%20%7D%0Abody%3Ediv%3Anth-child(1)%3Ediv%20%7B%20transform%3A%20matrix(1%2C%200%2C%200.5%2C%201%2C%2020%2C%203)%3B%20%7D%0Abody%3Ediv%3Anth-child(2)%3Ediv%20%7B%20transform%3A%20matrix3d(1%2C%200%2C%200%2C%200%2C%200.5%2C%201%2C%200%2C%200%2C%200%2C%200%2C%201%2C%200%2C%2020%2C%203%2C%200%2C%201)%20%7D%0A%3C%2Fstyle%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E and this one in Chrome: http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Abody%3Ediv%20%7B%20border%3A%20thin%20solid%20blue%3B%20width%3A%20100px%3B%20margin%3A%205px%200%3B%20%7D%0Abody%3Ediv%3Ediv%20%7B%20border%3A%20thin%20solid%20green%3B%20height%3A%2020px%3B%20-webkit-transform-origin%3A%200%200%3B%20%7D%0Abody%3Ediv%3Anth-child(1)%3Ediv%20%7B%20-webkit-transform%3A%20matrix(1%2C%200%2C%200%2C%201%2C%2020%2C%203)%3B%20%7D%0Abody%3Ediv%3Anth-child(2)%3Ediv%20%7B%20-webkit-transform%3A%20matrix3d(1%2C%200%2C%200%2C%200%2C%200%2C%201%2C%200%2C%200%2C%200%2C%200%2C%201%2C%200%2C%2020%2C%203%2C%200%2C%201)%20%7D%0A%3C%2Fstyle%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E So, not sure what tests you're currently debugging... but they're not good enough. Could you add a test that catches the translation components of matrix() being wrong too? r=dbaron if you (a) agree that what I'm suggesting is correct and (b) fix the code to do it and (c) have a test that catches it, though.

(And matrix._33 and matrix._44 should be 1, as they are now, and all the others not mentioned in the previous comment or this one should be 0.)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #41) > Comment on attachment 756342 [details] [diff] [review] > Part 9: Fix matrix() functions > > Yeah, the old code here is clearly wrong. > > But if we assume the matrix3d code is right (i.e., assume that we don't need > to flip row-major and column-major here), then I think this is also wrong. > > I think what you should end up doing is assigning: > Item(1) -> matrix._11 (as you do) > Item(2) -> matrix._12 (as you do) > Item(3) -> matrix._21 (as you do) > Item(4) -> matrix._22 (as you do) > Item(5) -> matrix._41 > Item(6) -> matrix._42 I don't understand why the matrix is flipped here, but assuming that's the case, this makes sense. And it fixes the rest of the test failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b31ac7a1bb4 https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e5cb861643 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e151e7eb99

https://hg.mozilla.org/mozilla-central/rev/0bf84683f9e1 https://hg.mozilla.org/mozilla-central/rev/3e6f8502d1af

https://hg.mozilla.org/mozilla-central/rev/1b31ac7a1bb4 https://hg.mozilla.org/mozilla-central/rev/c0e5cb861643 https://hg.mozilla.org/mozilla-central/rev/d9e151e7eb99

Lost this hunk earlier: https://hg.mozilla.org/integration/mozilla-inbound/rev/5103d37775ce

Comment on attachment 756770 [details] [diff] [review] Part 11: Refactor test_transitions_per_property Ignore this, it's failing intermittently on try.

Is part 11 in preparation for tests that aren't attached yet (rather than the tests in part 12)? Also, preparation is misspelled in the commit message in part 11. And is Reuben the correct patch author of part 12? (I don't see him involved in this bug at all.) So should I wait to review part 12 as well given comment 51? (I was just getting to the review requests here...)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #52) > Is part 11 in preparation for tests that aren't attached yet (rather than > the tests in part 12)? > Part 11 prepares for part 12. > Also, preparation is misspelled in the commit message in part 11. And is > Reuben the correct patch author of part 12? (I don't see him involved in > this bug at all.) No, he was testing something for me and I guess the commit message got changed when he imported the patch. > So should I wait to review part 12 as well given comment 51? (I was just > getting to the review requests here...) I obsoleted the wrong patch. 11 should be fine, 12 needs more work.

Comment on attachment 756770 [details] [diff] [review] Part 11: Refactor test_transitions_per_property >+SimpleTest.waitForExplicitFinish(); >+ >+ SimpleTest.finish(); >+ I don't see any reason for these two additions; the entire test still runs synchronously. >+var transformTestIndex = 0; Don't add this unused variable. r=dbaron on the rest

My test was failing intermittently on try because if we push a shadow layers update with a throttled animation, the compositor will think the animation is at the first frame until there is another Composite. If we query the layer's transform during that time, we get an identity transform.

Comment on attachment 757587 [details] [diff] [review] Part 12: Add tests for async transforms >+ var test = transformTests[transformTestIndex]; >+ >+ if (!test) { >+ transformDiv.style.removeProperty("transition"); >+ SimpleTest.finish(); >+ return; >+ } Probably cleaner (is there a strict warning?) to do: + if (transformTestIndex >= transformTests.length) { + transformDiv.style.removeProperty("transition"); + SimpleTest.finish(); + return; + } + + var test = transformTests[transformTestIndex]; So the combination of: * the fact that you don't set the transition-timing-function to linear anywhere (which means you get 'ease', the default) * the fact that you're looking at 1/3 of the way through the transition (100s of 300s) rather than 1/4 of the way through like the tests are meant to be used is a little confusing; it means the OMTA tests are testing a point 57.59% of the way through the animation whereas the other tests are testing a point 25% of the way through. While I suppose there's value testing something different, some comments to that effect would at least be useful, and it might be worth testing something a little further from 50%. In nextAsyncTransformTest: >+ var computed_transform = transformCs.getPropertyValue("transform"); You don't need the "var computed_transform = " here. (You do in the later function, of course.) Are you planning to investigate big_omta_round_error as a followup? r=dbaron

(In reply to David Baron [:dbaron] (in W3C meetings through June 11) (don't cc:, use needinfo? instead) from comment #58) > Comment on attachment 757587 [details] [diff] [review] > Part 12: Add tests for async transforms > > > is a little confusing; it means the OMTA tests are testing a point > 57.59% of the way through the animation whereas the other tests are > testing a point 25% of the way through. While I suppose there's value > testing something different, some comments to that effect would at > least be useful, and it might be worth testing something a little further > from 50%. > I changed it to linear 200s and commented. > > Are you planning to investigate big_omta_round_error as a followup? > Yes, but as far as I can tell we're just getting errors from lots of floating point math. I have an idea that may work, but it's better as a followup.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8b4e6a986a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/411b2e15a286 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7cf4aeef44d Still need to write opacity tests.

https://hg.mozilla.org/mozilla-central/rev/1b8b4e6a986a https://hg.mozilla.org/mozilla-central/rev/411b2e15a286 https://hg.mozilla.org/mozilla-central/rev/e7cf4aeef44d

Might be worth renaming the transformDiv.

Comment on attachment 765665 [details] [diff] [review] Part 14: Add test for opacity >+ is(async_opacity, computed_opacity, >+ "Async animated opacity does not much computed opacity"); The test message should make sense when both passing or failing, so you should s/does not much/should match/. And renaming transformDiv, transformCs, to something like OMTADiv, OMTACs, might make sense as well. r=dbaron with that

This landed on mozilla-central whilst it was still mozilla24, however mcMerge references the bugzilla target milestone field ordering, which was updated pre-emptively before the merge had occurred.

Comment on attachment 820413 [details] [diff] [review] Part 15: Add tests for async animations Review of attachment 820413 [details] [diff] [review]: ----------------------------------------------------------------- You don't need the -moz prefix. ::: layout/style/test/test_animations.html @@ +247,5 @@ > + return undefined; > + } > + return undefined; > +} > + You can just call getOMTAOrComputedStyle here. It automatically falls back to cs.getPropertyValue if omta is off. Also, I'm surprised all these tests pass. Are you actually getting OMTA? You can try changing http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#400 to pass in the wrong values and see if this starts failing.

These tests are in rough shape but I finally got OMTA code paths to get triggered. Unfortunately, it's turned flakey and crashy. Over to dzbarsky for a look at why and how to modify the tests/code as needed.

Jet, what did you do to trigger the crashes? I added test_OMTA.html as a mochitest and switched on the async animations pref, but the test runs fine.

Just enabling layers.offmainthreadcomposition.async-animations to true got asserts and a crash. I'll see if I can get a stack up.

I've split off a separate bug (bug 964646) to track testing CSS Animations and making this bug specific to CSS Transitions.