Containerless scrolling means that the pan zoom controller applies it's transforms (to compensate for differences between the state of layout the last time we painted and the current state as composited to the screen) to the layers that are scrolled instead of the container layer that contains the layers that scroll. When running test_animations_omta.html there is a zoom of 1.306122 applied, and the page is scrolled down to 67 screen pixels (before the test starts, not sure why exactly). Gecko scrolls as close to 67 screen pixels as it can: 67/1.306122 = 51.29689 css pixels, which is 3077.813 appunits. Gecko scrolls to 3078 app units. When AsyncCompositionManager::TransformScrollableLayer runs we calculate the scroll position of gecko and the current scroll position that the pan zoom controller is using. Since there are no async pan or zoom operations taking place these should match. However when the gecko scroll position is calculated we get 3078/60*1.306122 = 67.0040586. So it applies a transform of 0.0040586 to the container layer for the transform that test_animations_omta.html is animating off main thread. When test_animations_omta.html reads the transform of this layer it fails because it's expecting 0 and 0.0040586 is outside of it's epsilon for considering it to be close enough. kats suggested that when getting the transfrom vis nsDOMWindowUtils::GetOMTAStyle we should just not be included any transforms that arise from async panning or zooming.

Does this seem like a reasonable plan to you Brian?

Is something we could address in LayerTransactionParent::RecvGetAnimationTransform: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayerTransactionParent.cpp#672 We already try to undo all sorts of scaling to get back to CSS pixels there.

(In reply to Brian Birtles (:birtles) from comment #2) > Is something we could address in > LayerTransactionParent::RecvGetAnimationTransform: > > > https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ > LayerTransactionParent.cpp#672 > > We already try to undo all sorts of scaling to get back to CSS pixels there. We'd need to know basically the transform that the pan zoom controller applies, or have the information that it computes that transform from and then undo the transform. Seems like just not including the transform would be cleaner.

(In reply to Timothy Nikkel (:tn) from comment #3) > We'd need to know basically the transform that the pan zoom controller > applies, or have the information that it computes that transform from and > then undo the transform. Seems like just not including the transform would > be cleaner. I wondered about that. For animations we can get the animation data back from the layer in order to undo the scaling applying to the scale origin and transform origin but I guess for APZ we don't have that data around. (In reply to Timothy Nikkel (:tn) from comment #0) > ... However when the gecko > scroll position is calculated we get 3078/60*1.306122 = 67.0040586. So it > applies a transform of 0.0040586 to the container layer for the transform > that test_animations_omta.html is animating off main thread. Transform here being a translation? > When > test_animations_omta.html reads the transform of this layer it fails because > it's expecting 0 and 0.0040586 is outside of it's epsilon for considering it > to be close enough. What exactly in this test is failing? Is there a try run for this patch? The call to mShadowLayersManager->ApplyAsyncProperties(this) inside LayerTransactionParent::RecvGetAnimationTransform doesn't do anything if there isn't already a composite scheduled. I'm pretty sure we could sometimes have composited with aIgnoreAPZTransforms = false, have no composite task scheduled, and then fail to re-apply the async properties *without* APZ transforms when fetching the OMTA transform. It's tempting to make RecvGetAnimationTransform *always* just re-apply the async properties minus APZ transforms but the point of only updating them when there was a composite scheduled is to better mimic regular operation. Is there some state we can query to determine if APZ transforms are rolled up in the animation transform and just re-apply the async properties in that case?

(In reply to Brian Birtles (:birtles) from comment #4) > (In reply to Timothy Nikkel (:tn) from comment #3) > > We'd need to know basically the transform that the pan zoom controller > > applies, or have the information that it computes that transform from and > > then undo the transform. Seems like just not including the transform would > > be cleaner. > > I wondered about that. For animations we can get the animation data back > from the layer in order to undo the scaling applying to the scale origin and > transform origin but I guess for APZ we don't have that data around. > > (In reply to Timothy Nikkel (:tn) from comment #0) > > ... However when the gecko > > scroll position is calculated we get 3078/60*1.306122 = 67.0040586. So it > > applies a transform of 0.0040586 to the container layer for the transform > > that test_animations_omta.html is animating off main thread. > > Transform here being a translation? > > > When > > test_animations_omta.html reads the transform of this layer it fails because > > it's expecting 0 and 0.0040586 is outside of it's epsilon for considering it > > to be close enough. > > What exactly in this test is failing? Many parts of the test are failing. I picked on the very first failure. This one: http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_animations_omta.html?force=1#235 It's expecting to have an x translate of 66.66666, and 0 y translate. We end up with a y translate of 0.0040586 so it fails. Here is a try run with containerless scrolling enabled https://treeherder.mozilla.org/#/jobs?repo=try&revision=764254718be3 > Is there a try run for this patch? The call to > mShadowLayersManager->ApplyAsyncProperties(this) inside > LayerTransactionParent::RecvGetAnimationTransform doesn't do anything if > there isn't already a composite scheduled. I'm pretty sure we could > sometimes have composited with aIgnoreAPZTransforms = false, have no > composite task scheduled, and then fail to re-apply the async properties > *without* APZ transforms when fetching the OMTA transform. Here are try runs with this patch, with containerless enabled and disabled (default) https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ddfae9886a1 https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7598b4dae47 I'll let kats chime in on this and the next paragraph, he's better positioned to answer than me. > It's tempting to make RecvGetAnimationTransform *always* just re-apply the > async properties minus APZ transforms but the point of only updating them > when there was a composite scheduled is to better mimic regular operation. > Is there some state we can query to determine if APZ transforms are rolled > up in the animation transform and just re-apply the async properties in that > case?

Ok, I'll wait to see what kats has to say. I'm pretty sure, however, that this is going to lead to some intermittent failures because we'll sometimes update with APZ transforms and sometimes not. I think we should probably just make RecvGetAnimationTransform *always* update the async properties without APZ transforms whether or not there is a composite scheduled and then drop the call to ApplyAsyncProperties() from ShadowLayersUpdated().

I think unapplying the APZ transforms after having applied them is going to be a lot trickier to get right (and also involve more code), so I'd rather focus more on the current approach where we don't apply the APZ transform at all. If I understand what Brian says about the intermittent failure problem, then we should be able to fix that by doing something like the below in combination with tn's patch. That way ApplyAsyncProperties will always recompute the properties in the manner the caller specified (i.e. with or without APZ transforms), and since it's called unconditionally from RecvGetAnimationTransform it should always return a consistent result. Brian, would that resolve your concern? diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp index 83bb642..466e89e 100644 --- a/gfx/layers/ipc/CompositorParent.cpp +++ b/gfx/layers/ipc/CompositorParent.cpp @@ -1205,23 +1205,21 @@ CompositorParent::LeaveTestMode(LayerTransactionParent* aLayerTree) void CompositorParent::ApplyAsyncProperties(LayerTransactionParent* aLayerTree) { // NOTE: This should only be used for testing. For example, when mIsTesting is // true or when called from test-only methods like // LayerTransactionParent::RecvGetAnimationTransform. - // Synchronously update the layer tree, but only if a composite was already - // scehduled. - if (aLayerTree->GetRoot() && - (mCurrentCompositeTask || - (mCompositorVsyncObserver && - mCompositorVsyncObserver->NeedsComposite()))) { + // Synchronously update the layer tree + if (aLayerTree->GetRoot()) { AutoResolveRefLayers resolve(mCompositionManager); + SetShadowProperties(mLayerManager->GetRoot()); + TimeStamp time = mIsTesting ? mTestTime : mLastCompose; bool requestNextFrame = mCompositionManager->TransformShadowTree(time); if (!requestNextFrame) { CancelCurrentCompositeTask(); // Pretend we composited in case someone is waiting for this event. DidComposite(); }

(Note also that the only other call site of ApplyAsyncProperties, in CompositorParent::ShadowLayersUpdated does a call to ScheduleComposition() just before calling ApplyAsyncProperties, so in that path we would always go inside the if condition in ApplyAsyncProperties anyway).

Comment on attachment 8595676 [details] [diff] [review] patch Review of attachment 8595676 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for now until but this looks like what I had in mind. I don't think you need to put all the specific numeric values into the commit message though, specially since it's already documented in the bug.

try job with attached patch plus patch from comment 7 https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f4242c8656b

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > I think unapplying the APZ transforms after having applied them is going to > be a lot trickier to get right (and also involve more code), so I'd rather > focus more on the current approach where we don't apply the APZ transform at > all. > > If I understand what Brian says about the intermittent failure problem, then > we should be able to fix that by doing something like the below in > combination with tn's patch. That way ApplyAsyncProperties will always > recompute the properties in the manner the caller specified (i.e. with or > without APZ transforms), and since it's called unconditionally from > RecvGetAnimationTransform it should always return a consistent result. > Brian, would that resolve your concern? That's pretty much what I had in mind. The only thing I might add to that is that if we do that, we can probably drop the call to ApplyAsyncProperties() from ShadowLayersUpdated()--we'd need to test though (it might still be needed for some tests of opacity in which case we could probably fix that by adding a call to ApplyAsyncProperties in RecvGetOpacity as well). (Apart from that, as a minor nit, could we replace the bool param with a scoped enum or something that's a bit more readable at call sites?)

Comment on attachment 8595676 [details] [diff] [review] patch Cancelling the review request for now since I think we agree on what changes we need to make.

Moving ApplyAsyncProperties into RecvGetOpacity and turning the bool into an enum both sound like good changes to me.

Addressed all the comments.

Comment on attachment 8596982 [details] [diff] [review] patch v2 All the callers of ApplyAsyncProperties now passed the ignore apz flag, so only TransformShadowTree needs the flag.

Comment on attachment 8596982 [details] [diff] [review] patch v2 Looks good to me assuming all tests pass. As a minor nit, what do you think about using an enum class instead? e.g. enum class TransformsToSkip { None = 0, APZ = 1 }; It means we get strong-typing and name scoping. Then we can use MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS if we need to combine flags. Or if you wanted to make it really neat: enum class TransformsToApply { None = 0, Animations = 1 << 0, APZ = 1 << 1, All = (1 << 2) - 1 };

(In reply to Brian Birtles (:birtles) from comment #16) > enum class TransformsToSkip { > None = 0, > APZ = 1 > }; > > It means we get strong-typing and name scoping. Then we can use > MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS if we need to combine flags. Thanks, I changed it to this. Be able to pick and choose exactly which transforms you wanted seemed overkill unless (until?) we need that.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f53de79da7f2 for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=9288809&repo=mozilla-inbound

(In reply to Timothy Nikkel (:tn) from comment #19) > (In reply to Brian Birtles (:birtles) from comment #16) > > enum class TransformsToSkip { > > None = 0, > > APZ = 1 > > }; Looks like X11/X1.h defines 'None' as a macro -_-