1157066 - test_animations_omta.html fails on Android with containerless scrolling

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Timothy Nikkel (:tnikkel)]

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.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Does this seem like a reasonable plan to you Brian?

Comment 2

[Brian Birtles (:birtles)]

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.

Comment 3

[Timothy Nikkel (:tnikkel)]

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

Comment 4

[Brian Birtles (:birtles)]

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

Comment 5

[Timothy Nikkel (:tnikkel)]

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

Comment 6

[Brian Birtles (:birtles)]

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Timothy Nikkel (:tnikkel)]

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

Comment 11

[Brian Birtles (:birtles)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 12

[Brian Birtles (:birtles)]

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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Addressed all the comments.

Comment 15

[Timothy Nikkel (:tnikkel)]

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 16

[Brian Birtles (:birtles)]

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
};

Comment 17

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd0f91d9ad5

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/517b9b831685

Comment 19

[Timothy Nikkel (:tnikkel)]

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

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 21

[Botond Ballo [:botond]]

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

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/058e2755777a

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/058e2755777a