1337889 - Do not use LayerHandle in OMTA testing methods

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Peter Chang[:pchang]]

There are two OMTA testing methods[1] that use LayerHandle in PLayerTransaction protocol. Because there is no 'Layer' or 'PLayerTransaction' in Quantum Render, I would like to replace the 'LayerHandle' with the u64 'AnimatedPropKey'. So we can use this new key to get the opacity/transform value from compositor.

And then I will move these methods to somewhere and then we can share these methods for current gecko and Quantum Render.

sync GetAnimationOpacity(LayerHandle layer) returns (float opacity,
                                                  bool hasAnimationOpacity);
sync GetAnimationTransform(LayerHandle layer) returns (MaybeTransform transform);

http://searchfox.org/mozilla-central/source/gfx/layers/ipc/PLayerTransaction.ipdl#83

Comment 1

[Peter Chang[:pchang]]

Attachment: Bug 1337889 - Add AnimatedPropKey for layer animations,

Review commit: https://reviewboard.mozilla.org/r/110734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110734/

Comment 2

[Peter Chang[:pchang]]

Comment on attachment 8835042 [details]
Bug 1337889 - Add AnimatedPropKey for layer animations,

https://reviewboard.mozilla.org/r/110734/#review112120

::: gfx/layers/ipc/LayersMessages.ipdlh:235
(Diff revision 1)
>    // stack). In all other cases the value is null_t.
>    Animatable baseStyle;
>  };
>  
> +struct AnimatedProp {
> +  Animation[] animations;

I will add the following comment in next patch.

// This key is used to link the sampled animation 
// value in compositor based on the animation data

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Peter Chang[:pchang] from comment #0)
> There are two OMTA testing methods[1] that use LayerHandle in
> PLayerTransaction protocol. Because there is no 'Layer' or
> 'PLayerTransaction' in Quantum Render, I would like to replace the
> 'LayerHandle' with the u64 'AnimatedPropKey'. So we can use this new key to
> get the opacity/transform value from compositor.
> 
> And then I will move these methods to somewhere and then we can share these
> methods for current gecko and Quantum Render.
> 
> sync GetAnimationOpacity(LayerHandle layer) returns (float opacity,
>                                                   bool hasAnimationOpacity);
> sync GetAnimationTransform(LayerHandle layer) returns (MaybeTransform
> transform);
> 
> http://searchfox.org/mozilla-central/source/gfx/layers/ipc/PLayerTransaction.
> ipdl#83

Thank you Peter!  Now I understand what the AnimatedPropKey is.  Then it should be named 'AnimationEffectStackId' or something similar.  "Effect stack" is the word defined in Web Animations spec [1].

One question:  IIUC, to use this AnimationEffectStackId for GetAnimation[Transform|Opacity] we need a getter for the id, will the getter be implemented in a later bug?  It's not still clear to me how the id is treated between the main thread and the compositor.

[1] https://w3c.github.io/web-animations/#combining-effects

Comment 4

[Peter Chang[:pchang]]

Attachment: Bug 1337889 - use animatedPropKey to query Opacity/Transform animation

Review commit: https://reviewboard.mozilla.org/r/111360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111360/

Comment 5

[Peter Chang[:pchang]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> (In reply to Peter Chang[:pchang] from comment #0)
> > There are two OMTA testing methods[1] that use LayerHandle in
> > PLayerTransaction protocol. Because there is no 'Layer' or
> > 'PLayerTransaction' in Quantum Render, I would like to replace the
> > 'LayerHandle' with the u64 'AnimatedPropKey'. So we can use this new key to
> > get the opacity/transform value from compositor.
> > 
> > And then I will move these methods to somewhere and then we can share these
> > methods for current gecko and Quantum Render.
> > 
> > sync GetAnimationOpacity(LayerHandle layer) returns (float opacity,
> >                                                   bool hasAnimationOpacity);
> > sync GetAnimationTransform(LayerHandle layer) returns (MaybeTransform
> > transform);
> > 
> > http://searchfox.org/mozilla-central/source/gfx/layers/ipc/PLayerTransaction.
> > ipdl#83
> 
> Thank you Peter!  Now I understand what the AnimatedPropKey is.  Then it
> should be named 'AnimationEffectStackId' or something similar.  "Effect
> stack" is the word defined in Web Animations spec [1].
> 
I copied the 'Effect Stack' wording from the spec. My intention is using 'AnimatedPropKey'to represent the dom element which contains animation effect. It doesn't look like link to the Effect Stack. Am I right?

Effect Stack
  Associated with each property targeted by one or more keyframe effects is an effect stack that establishes the relative composite order of the keyframe effects.

> One question:  IIUC, to use this AnimationEffectStackId for
> GetAnimation[Transform|Opacity] we need a getter for the id, will the getter
> be implemented in a later bug?  It's not still clear to me how the id is
> treated between the main thread and the compositor.
> 
I just uploaded attachment 8835743 [details] to get rid of 'LayerHandle' for GetAnimationOpacity.
Now I'm checking how to read transform matrix from StyleAnimationValue. It looks like related to  nsStyleTransformMatrix::ReadTransforms.

> [1] https://w3c.github.io/web-animations/#combining-effects

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thanks for doing!  I think we should replace LayerHandle with the id and confirm it works fine on current Gecko in a bug (i.e. this bug).

Comment 7

[Peter Chang[:pchang]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Thanks for doing!  I think we should replace LayerHandle with the id and
> confirm it works fine on current Gecko in a bug (i.e. this bug).

With attachment 8835743 [details], the testing of OMTA related to opacity are passed. Today I will work on the transform part.

Comment 8

[Peter Chang[:pchang]]

(In reply to Peter Chang[:pchang] from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > Thanks for doing!  I think we should replace LayerHandle with the id and
> > confirm it works fine on current Gecko in a bug (i.e. this bug).
> 
> With attachment 8835743 [details], the testing of OMTA related to opacity
> are passed. Today I will work on the transform part.

Here I mean the following two tests.
 [style/test_composite.html]
 [style/test_missing-keyframe-on-compositor.html]

Comment 9

[Peter Chang[:pchang]]

Now I'm able to call GetAnimatedPropValue to get correct Opacity/Transform animation and tests in comment 8 get passed. But I also need to cache the animations because it contains the transformdata that is needed to calculate transform matrix.

Hiro, do we need to worry about SVG transform in [1] for OMTA? Because I just call the 
nsStyleTransformMatrix::ReadTransforms directly to get transform from StyleAnimationValue(transform type).

[1]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#6379

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Peter Chang[:pchang] from comment #8)
> (In reply to Peter Chang[:pchang] from comment #7)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > > Thanks for doing!  I think we should replace LayerHandle with the id and
> > > confirm it works fine on current Gecko in a bug (i.e. this bug).
> > 
> > With attachment 8835743 [details], the testing of OMTA related to opacity
> > are passed. Today I will work on the transform part.
> 
> Here I mean the following two tests.
>  [style/test_composite.html]
>  [style/test_missing-keyframe-on-compositor.html]

Thanks.  Would you mind trying layout/style/test_animations_omta.html too?  This is the most important one for OMTA tests.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Peter Chang[:pchang] from comment #9)
> Now I'm able to call GetAnimatedPropValue to get correct Opacity/Transform
> animation and tests in comment 8 get passed. But I also need to cache the
> animations because it contains the transformdata that is needed to calculate
> transform matrix.
> 
> Hiro, do we need to worry about SVG transform in [1] for OMTA? Because I
> just call the 
> nsStyleTransformMatrix::ReadTransforms directly to get transform from
> StyleAnimationValue(transform type).
> 
> [1]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.
> cpp#6379

Actually SVG transform is out of my knowledge.  I think :mattwoodrow or :jwatt know well.

Comment 12

[Peter Chang[:pchang]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Thanks.  Would you mind trying layout/style/test_animations_omta.html too? 
> This is the most important one for OMTA tests.

Yes, this test was passed with my patches.(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

About SVG in comment 11, I just checked with mattwoodrow and we don't need to worry about it.

Comment 13

[Peter Chang[:pchang]]

It looks like I need more works before closing this bug, I will create smaller bugs to land first.

Comment 14

[Peter Chang[:pchang]]

Based on the discussion on IRC, we agree to use 'CompositorAnimationsId' instead of 'AnimatedPropKey' and the new naming is more clear for me as well.

Comment 15

[Peter Chang[:pchang]]

Attachment: Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

Review commit: https://reviewboard.mozilla.org/r/112292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112292/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837026 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/112292/#review113596

Thanks!  I am hoping subsequent patches will use the id for GetOMTA methods in this bug.

::: gfx/layers/AnimationHelper.cpp:421
(Diff revision 1)
>  
> +uint64_t
> +AnimationHelper::GetNextCompositorAnimationsId()
> +{
> +  static uint32_t sNextId = 1;
> +  ++sNextId;

I think we should skip the case of sNextId == 0.

::: gfx/layers/Layers.cpp:209
(Diff revision 1)
> -  MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) AddAnimation", this));
> +  if (!mCompositorAnimationsId) {
> +    mCompositorAnimationsId = AnimationHelper::GetNextCompositorAnimationsId();
> +  }
> +

I think we should add some comments here that we do increment the id only for the first animation on the layer since we use the same id for multiple animations on a layer.

Comment 17

[Peter Chang[:pchang]]

Comment on attachment 8837026 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112292/diff/1-2/

Comment 18

[Peter Chang[:pchang]]

Comment on attachment 8837026 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/112292/#review113596

> I think we should skip the case of sNextId == 0.

yes, you are right because procId should be non-zero at this time.

> I think we should add some comments here that we do increment the id only for the first animation on the layer since we use the same id for multiple animations on a layer.

Done

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837026 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/112292/#review113632

::: gfx/layers/AnimationHelper.cpp:420
(Diff revisions 1 - 2)
>  }
>  
>  uint64_t
>  AnimationHelper::GetNextCompositorAnimationsId()
>  {
> -  static uint32_t sNextId = 1;
> +  static uint32_t sNextId = 0;

I think the initiali value was correct. I meant it's integer overflow case. in ++sNextId.

Comment 20

[Peter Chang[:pchang]]

Comment on attachment 8837026 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112292/diff/2-3/

Comment 21

[Peter Chang[:pchang]]

Attachment: Bug 1337889 - Add CompositorAnimationsId for layer animations,

Review commit: https://reviewboard.mozilla.org/r/113002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113002/

Comment 22

[Peter Chang[:pchang]]

Hiro, there are some problem when I upload the second patch and it looks like it just replaced the first part. But I think you can just compare with the revision 0.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> Comment on attachment 8837026 [details]
> Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform
> animation,
> 
> https://reviewboard.mozilla.org/r/112292/#review113632
> 
> ::: gfx/layers/AnimationHelper.cpp:420
> (Diff revisions 1 - 2)
> >  }
> >  
> >  uint64_t
> >  AnimationHelper::GetNextCompositorAnimationsId()
> >  {
> > -  static uint32_t sNextId = 1;
> > +  static uint32_t sNextId = 0;
> 
> I think the initiali value was correct. I meant it's integer overflow case.
> in ++sNextId.

Thanks, I found there was something wrong when calculate the ID. And I just fixed it.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Though I don't know much about Quantum Render, do we really need to store opacity and transform matrix somewhere?  Even so, I don't think StyleAnimationValue is a good one, it will be eliminated in Quantum Style.

Comment 24

[Peter Chang[:pchang]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> Though I don't know much about Quantum Render, do we really need to store
> opacity and transform matrix somewhere?  Even so, I don't think
In [1], we won't use layer related data in WebRenderBridgeParent. Therefore, we couldn't get animation and animationData from layer properties. In this bug, I create the storage for animation data, I think we can create a follow up to remove animation data that are stored in layer properties. And then we can share the code between gecko and Quantum Render project. Hiro, does it make sense to you?

> StyleAnimationValue is a good one, it will be eliminated in Quantum Style.

Boris just told me to repalce StyleAnimationValue with ServoANimationValue.
This should be easy to fix, do you want me to fix in this bug or create another bug to follow up?

[1]http://searchfox.org/mozilla-central/source/gfx/layers/wr/WebRenderBridgeParent.cpp

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Peter Chang[:pchang] from comment #24)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > Though I don't know much about Quantum Render, do we really need to store
> > opacity and transform matrix somewhere?  Even so, I don't think
> In [1], we won't use layer related data in WebRenderBridgeParent. Therefore,
> we couldn't get animation and animationData from layer properties. In this
> bug, I create the storage for animation data, I think we can create a follow
> up to remove animation data that are stored in layer properties. And then we
> can share the code between gecko and Quantum Render project. Hiro, does it
> make sense to you?

OK. I wonder, in case of Gecko, we can leave current code as it is even if the values are queried by the id. Will we switch to Web Render soon?

> > StyleAnimationValue is a good one, it will be eliminated in Quantum Style.
> 
> Boris just told me to repalce StyleAnimationValue with ServoANimationValue.
> This should be easy to fix, do you want me to fix in this bug or create
> another bug to follow up?

I don't think ServoAnimationValue is a good one either. Both of them are too big to store a float value (opacity) or a matrix (transform).

> [1]http://searchfox.org/mozilla-central/source/gfx/layers/wr/
> WebRenderBridgeParent.cpp

I am wondering there is no use case to query some values from the compositor result?  Is it only for animation tests?

Comment 26

[Peter Chang[:pchang]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> (In reply to Peter Chang[:pchang] from comment #24)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > > Though I don't know much about Quantum Render, do we really need to store
> > > opacity and transform matrix somewhere?  Even so, I don't think
> > In [1], we won't use layer related data in WebRenderBridgeParent. Therefore,
> > we couldn't get animation and animationData from layer properties. In this
> > bug, I create the storage for animation data, I think we can create a follow
> > up to remove animation data that are stored in layer properties. And then we
> > can share the code between gecko and Quantum Render project. Hiro, does it
> > make sense to you?
> 
> OK. I wonder, in case of Gecko, we can leave current code as it is even if
> the values are queried by the id. Will we switch to Web Render soon?
> 
It would take a while to enable OMTA in quantum render(Add sampling animation in WebRenderBridgeParent without using layer information). But for now, I would like to share the same code for OMTA testing.
> > > StyleAnimationValue is a good one, it will be eliminated in Quantum Style.
> > 
> > Boris just told me to repalce StyleAnimationValue with ServoANimationValue.
> > This should be easy to fix, do you want me to fix in this bug or create
> > another bug to follow up?
> 
> I don't think ServoAnimationValue is a good one either. Both of them are too
> big to store a float value (opacity) or a matrix (transform).
> 
Do we have the similar one(StyleAnimationValue) from servo?
> > [1]http://searchfox.org/mozilla-central/source/gfx/layers/wr/
> > WebRenderBridgeParent.cpp
> 
> I am wondering there is no use case to query some values from the compositor
> result?  Is it only for animation tests?
Do you mean Quantum Render? If yes, I will add sampling animation logic after this bug because I almost remove the layer dependency for sampling or OMTA testing.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

(In reply to Peter Chang[:pchang] from comment #26)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> > (In reply to Peter Chang[:pchang] from comment #24)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > > > Though I don't know much about Quantum Render, do we really need to store
> > > > opacity and transform matrix somewhere?  Even so, I don't think
> > > In [1], we won't use layer related data in WebRenderBridgeParent. Therefore,
> > > we couldn't get animation and animationData from layer properties. In this
> > > bug, I create the storage for animation data, I think we can create a follow
> > > up to remove animation data that are stored in layer properties. And then we
> > > can share the code between gecko and Quantum Render project. Hiro, does it
> > > make sense to you?
> > 
> > OK. I wonder, in case of Gecko, we can leave current code as it is even if
> > the values are queried by the id. Will we switch to Web Render soon?
> > 
> It would take a while to enable OMTA in quantum render(Add sampling
> animation in WebRenderBridgeParent without using layer information). But for
> now, I would like to share the same code for OMTA testing.
> > > > StyleAnimationValue is a good one, it will be eliminated in Quantum Style.
> > > 
> > > Boris just told me to repalce StyleAnimationValue with ServoANimationValue.
> > > This should be easy to fix, do you want me to fix in this bug or create
> > > another bug to follow up?
> > 
> > I don't think ServoAnimationValue is a good one either. Both of them are too
> > big to store a float value (opacity) or a matrix (transform).
> > 
> Do we have the similar one(StyleAnimationValue) from servo?

The ServoAnimationValue is an equivalent of StyleAnimationValue, but both of them have unnecessary information (e.g. transform functions). For getOMTAStyle() we just needs a float value or a matrix.  I don't quite understand why we need to re-calculate transform functions every time we call getOMTAStyle().

Comment 28

[Peter Chang[:pchang]]

Attachment: animation transfrom

Comment 29

[Peter Chang[:pchang]]

Comment on attachment 8839945 [details]
animation transfrom

>ApplyAnimatedValue(Layer* aLayer,
>                   nsCSSPropertyID aProperty,
>                   const AnimationData& aAnimationData,
>                   const StyleAnimationValue& aValue)
>{
>  ...
>  switch (aProperty) {
>    case eCSSProperty_opacity: {
>    ...
>    }
>    case eCSSProperty_transform: {
>      ...
>
>      // If our parent layer is a perspective layer, then the offset into reference
>      // frame coordinates is already on that layer. If not, then we need to ask
>      // for it to be added here.
>      uint32_t flags = 0;
>      if (!aLayer->GetParent() ||
>          !aLayer->GetParent()->GetTransformIsPerspective()) {
>        flags = nsDisplayTransform::OFFSET_BY_ORIGIN;
Here is a transform postTransalte operation that happenes in GetResultingTransformMatrix.
>      }
>
>      Matrix4x4 transform =
>        nsDisplayTransform::GetResultingTransformMatrix(props, origin,
>                                                        transformData.appUnitsPerDevPixel(),
>                                                        flags, &transformData.bounds());
>
>      if (ContainerLayer* c = aLayer->AsContainerLayer()) {
>        transform.PostScale(c->GetInheritedXScale(), c->GetInheritedYScale(), 1);
Here is another transfrom postScale operation.
>      }
>      layerCompositor->SetShadowBaseTransform(transform);

Before I thought it was easy to get transform from StyleAnimationValue because it didn't apply additional layer transform, like above postTranslate and postScale operations. But I agree that we should only save float or a matrix for getOMTAStyle() because it is more efficient.

But how do we undo the postTranslate and postScale without querying layer's attributes when call getOMTAStyle().

The layer transform(ShadowBaseTransform) is calculated via 'layerTransfrom in DeviceSpace = frame transform(GetResultingTransformMatrix) * postTranslate * postScale'. If we can cache the transform in device space (called TransformInDevSpace) and the transform 'postTranslate * postScale'(called appliedTransform), we can get the correct frame transform via 'TransformInDevSpace * appliedTransform.Inverse()' in getOMTAStyle().

Comment 30

[Peter Chang[:pchang]]

(In reply to Peter Chang[:pchang] from comment #29)
> Comment on attachment 8839945 [details]
...
> The layer transform(ShadowBaseTransform) is calculated via 'layerTransfrom
> in DeviceSpace = frame transform(GetResultingTransformMatrix) *
> postTranslate * postScale'. If we can cache the transform in device space
> (called TransformInDevSpace) and the transform 'postTranslate *
> postScale'(called appliedTransform), we can get the correct frame transform
> via 'TransformInDevSpace * appliedTransform.Inverse()' in getOMTAStyle().

I re-write the above transform calculation for better understanding.

LayerTransfromInDeviceSpace = frameTransform * postTranslate * postScale.

Then frameTransform = LayerTransformInDeviceSpace * Inverse of (postTranslate*postScale).

Comment 31

[Peter Chang[:pchang]]

Need info hiro for comment 29 and comment 30.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

I am not sure what we don't have in web render, so I might be wrong, but can we use such missing information on the main-thread? I.e. in nsDOMWindowUtils::GetOMTAStyle.

Comment 33

[Peter Chang[:pchang]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> I am not sure what we don't have in web render, so I might be wrong, but can
> we use such missing information on the main-thread? I.e. in
> nsDOMWindowUtils::GetOMTAStyle.

We don't have layer in parent side(compositor), but we applied some transforms[1]&[2] in compositor world to get the layer transform in device space. 
And we can't get these information from nsDOMWindowUtils::GetOMTAStyle in content process.

[1]http://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#616
[2]http://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#625

Comment 34

[Hiroyuki Ikezoe (:hiro)]

I mean nsIFrame not layer.

Comment 35

[Peter Chang[:pchang]]

Comment on attachment 8839945 [details]
animation transfrom

>ApplyAnimatedValue(Layer* aLayer,
>                   nsCSSPropertyID aProperty,
>                   const AnimationData& aAnimationData,
>                   const StyleAnimationValue& aValue)
>{
>  ...
>  switch (aProperty) {
>    case eCSSProperty_opacity: {
>    ...
>    }
>    case eCSSProperty_transform: {
>      ...
>
>      // If our parent layer is a perspective layer, then the offset into reference
>      // frame coordinates is already on that layer. If not, then we need to ask
>      // for it to be added here.
>      uint32_t flags = 0;
>      if (!aLayer->GetParent() ||
>          !aLayer->GetParent()->GetTransformIsPerspective()) {
>        flags = nsDisplayTransform::OFFSET_BY_ORIGIN;
>      }
>
>      Matrix4x4 transform =
>        nsDisplayTransform::GetResultingTransformMatrix(props, origin,
>                                                        transformData.appUnitsPerDevPixel(),
>                                                        flags, &transformData.bounds());
There is another straightforward approach that we cache two transforms, the transfrom from frame and final layer transform.

The transform we got from GetResultingTransformMatrix is the frame transform.

So we can return transformInDevSpace(layer transform) for sampleanimation, return frameTransform for GetOMTAStyle().
>
>      if (ContainerLayer* c = aLayer->AsContainerLayer()) {
>        transform.PostScale(c->GetInheritedXScale(), c->GetInheritedYScale(), 1);
>      }
>      layerCompositor->SetShadowBaseTransform(transform);

Comment 36

[Peter Chang[:pchang]]

Attachment: Bug 1337889 - Add CompositorAnimationsId for layer animations,

Review commit: https://reviewboard.mozilla.org/r/114850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114850/

Comment 37

[Peter Chang[:pchang]]

Attachment: Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

Review commit: https://reviewboard.mozilla.org/r/114852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114852/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8840348 [details]
Bug 1337889 - Add CompositorAnimationsId for layer animations,

https://reviewboard.mozilla.org/r/114850/#review116322

I thought I stamped r+ for this.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116318

Thanks! This looks much better.  But I think this needs still more work.
I will re-review once this patch is updated.  But before doing it, please ask someone who is familiar with web render architecture that using the singleton of CompositorAnimationStorage is a right approach here.

Thanks!

::: gfx/layers/AnimationHelper.cpp:52
(Diff revision 1)
> +  for ( ;iter != mAnimatedValues.end(); iter++) {
> +    if (iter->first() == aId) {
> +      aValue = iter->second();
> +      found = true;
> +      break;

This looks quite slow.  We should use hash table instead.
Also I am not entirely sure using this singleton (CompositorAnimationStorage) is a right approach here.  Could you please request a person who is familiar with web render architecture?

::: gfx/layers/AnimationHelper.cpp:65
(Diff revision 1)
> +                                             const gfx::Matrix4x4& aTransformInDevSpace,
> +                                             const gfx::Matrix4x4& aFrameTransform,
> +                                             const TransformData& aData)

Can't we move these values instead of copying?

::: gfx/layers/composite/AsyncCompositionManager.cpp:674
(Diff revision 1)
> +    // Clean up the CompositorAnimationStorage because
> +    // there are no active animations running
> +    CompositorAnimationStorage::GetInstance()->Clear();

I don't quite understand this.  I think this Clear() remove all of animations on other layers?

Comment 40

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116318

Kats, could you help me to review my patch as well? The purpose of CompositorAnimationStorage is to save animation data based on CompositorAnimationId. Then we can query the right animation data in WebRenerBridgeParent with id. Right now I store the data inside nsTArray, I will investigate to use hash table as hiro suggested.

> This looks quite slow.  We should use hash table instead.
> Also I am not entirely sure using this singleton (CompositorAnimationStorage) is a right approach here.  Could you please request a person who is familiar with web render architecture?

Yes, I will consider to use hash table.

> Can't we move these values instead of copying?

Yes, we can save the copys for matrixs but TransformData is still from layer.

> I don't quite understand this.  I think this Clear() remove all of animations on other layers?

Yes, this code will clear the CompositorAnimationStorage include samplenation value and animationdata where there is no active animation. If there are new animations happen, there will be new animations data avaialbe throught LayerTransaction. Therefore, we don't need to maintain the old animation data. Hiro, is it clear to you?

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116634

::: gfx/layers/composite/AsyncCompositionManager.cpp:674
(Diff revision 1)
> +    // Clean up the CompositorAnimationStorage because
> +    // there are no active animations running
> +    CompositorAnimationStorage::GetInstance()->Clear();

I am confused.  This SampleAnimations is called against each Layer. If the activeAnimations is false on a layer, there might be other layers that have animations.  No?
I am seeing a different place?

Comment 42

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> Comment on attachment 8840349 [details]
> Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform
> animation,
> 
> https://reviewboard.mozilla.org/r/114852/#review116634
> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:674
> (Diff revision 1)
> > +    // Clean up the CompositorAnimationStorage because
> > +    // there are no active animations running
> > +    CompositorAnimationStorage::GetInstance()->Clear();
> 
> I am confused.  This SampleAnimations is called against each Layer. If the
> activeAnimations is false on a layer, there might be other layers that have
> animations.  No?
> I am seeing a different place?

Oh, sorry. SampleAnimations has been changed for a while. Sorry for the confusion.

Comment 43

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

Comment on attachment 8840348 [details]
Bug 1337889 - Add CompositorAnimationsId for layer animations,

https://reviewboard.mozilla.org/r/114850/#review116730

::: gfx/layers/Layers.cpp:1921
(Diff revision 1)
>        aStream << nsPrintfCString(" [metrics%d=", i).get();
>        AppendToString(aStream, mScrollMetadata[i], "", "]");
>      }
>    }
>    if (!mAnimations.IsEmpty()) {
> -    aStream << nsPrintfCString(" [%d animations]", (int) mAnimations.Length()).get();
> +    aStream << nsPrintfCString(" [%d animations with id=%llu ]",

Please do:
 ... id=" PRIu64 " ]",
instead of using "%llu", because this doesn't work on all platforms.

Same change needs to be made in Layer::AddAnimation and Layer::SetCompositionAnimations

Comment 44

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

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116634

> I am confused.  This SampleAnimations is called against each Layer. If the activeAnimations is false on a layer, there might be other layers that have animations.  No?
> I am seeing a different place?

This is still wrong, because there is one AsyncCompositionManager instance per compositor, and the CompositorAnimationStorage is a singleton. So if you have two browser windows open, and one has no animations, you will clear out the animations for the other window as well.

Comment 45

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

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116742

Minusing for now to clear review flag. Overall the idea of the patch looks ok to me, but there are changes (such as using a hashtable) that will change the patch quite a bit so I want to re-review after those changes are made. I have some other comments below.

::: gfx/layers/AnimationHelper.h:49
(Diff revision 1)
> +  union {
> +    AnimationTransform mTransform;
> +    float mOpacity;
> +  };
> +
> +  AnimatedValue() : mType(AnimatedValue::NONE)
> +  {
> +    memset(&mTransform, 0, sizeof(AnimationTransform));

I would prefer that you named the union, and then did memset on the union. The way it's written now, if we add another type inside the union that's larger than mTransform it will not zero it out fully.

union {
  AnimationTransform mTransform;
  float mOpacity;
  ReallyBigClass mSomeOtherProperty;
}

memset(&mTransform, 0, sizeof(AnimationTransform)); // this will only partially zero out mSomeOtherProperty.

Instead:

union {
  AnimationTransform mTransform;
  float mOpacity;
  ReallyBigClass mSomeOtherProperty;
} prop;

memset(&prop, 0, sizeof(prop)); // this will always work.

::: gfx/layers/AnimationHelper.h:59
(Diff revision 1)
> +  AnimatedValue() : mType(AnimatedValue::NONE)
> +  {
> +    memset(&mTransform, 0, sizeof(AnimationTransform));
> +  }
> +
> +  AnimatedValue& operator=(const AnimatedValue& aValue)

Why do you need this? It doesn't seem to handle being assigned a NONE value, and the default assignment operator should work just fine because it's a POD struct, right?

::: gfx/layers/AnimationHelper.h:72
(Diff revision 1)
> +    }
> +
> +    return *this;
> +  }
> +
> +  AnimatedValue(const AnimatedValue& aValue)

Ditto, this seems unnecessary

::: gfx/layers/AnimationHelper.h:93
(Diff revision 1)
> +    mTransform.mTransformInDevSpace = aTransformInDevSpace;
> +    mTransform.mFrameTransform = aFrameTransform;
> +    mTransform.mData = aData;
> +  }
> +
> +  AnimatedValue(const float& aValue)

this will probably need |explicit| to avoid static analysis build failure.

::: gfx/layers/AnimationHelper.h:100
(Diff revision 1)
> +    mType = AnimatedValue::OPACITY;
> +    mOpacity = aValue;
> +  }
> +
> +  ~AnimatedValue(){}
> + };

nit: indentation

::: gfx/layers/AnimationHelper.h:106
(Diff revision 1)
> +typedef Pair<uint64_t, AnimatedValue> AnimatedValuePair;
> +typedef nsTArray<AnimatedValuePair> AnimatedValues;
> +
> +typedef Pair<uint64_t, AnimationArray> AnimationsPair;
> +typedef nsTArray<AnimationsPair> Animations;

nit: indentation. I suspect this class will look very different after you switch to use a hashtable, so I'll skip reviewing it for now.

::: gfx/layers/ipc/LayerTransactionParent.cpp:701
(Diff revision 1)
> +  CompositorAnimationStorage::GetInstance()->
> +    GetAnimatedValue(aCompositorAnimationsId, value);

Don't you want to check the return value here?

::: gfx/layers/ipc/LayerTransactionParent.cpp:731
(Diff revision 1)
> -  // transforms that are not set by animation we don't have this information
> +  if (!found) {
> -  // available.
> -  if (!layer->AsHostLayer()->GetShadowTransformSetByAnimation()) {
>      *aTransform = mozilla::void_t();
>      return IPC_OK();

Don't you want to also check value.mType here?

Comment 46

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116634

> This is still wrong, because there is one AsyncCompositionManager instance per compositor, and the CompositorAnimationStorage is a singleton. So if you have two browser windows open, and one has no animations, you will clear out the animations for the other window as well.

Kats, how about maintain another hashtable to record the RootLayerTreeID when OMTA is running on that tree?
For multiple windows, we will add RootLayerTreeId (per windows) into hash table if there is active animation.
We will only clean up the CompositorAnimationStorage when there is no entry in this 'ActiveAnimations' hash table.

Based this approach, we can use the same logic for Quantum Render. The only disadvantage of this approach is we keep redundant data(from inactive animations) longer but the advantage is that we don't need to worry about the animation data synchronization between content and compositor.

+nsTHashtable<nsUint64HashKey> sActiveAnimations;

 void
 CompositorBridgeParent::CompositeToTarget(DrawTarget* aTarget, const gfx::IntRect* aRect)
 {
   ...
   bool requestNextFrame = mCompositionManager->TransformShadowTree(time, mVsyncRate);
   if (requestNextFrame) {
     ScheduleComposition();
+    sActiveAnimations.PutEntry(RootLayerTreeId());
+  } else {
+    sActiveAnimations.RemoveEntry(RootLayerTreeId());
+  }
+
+  if (!sActiveAnimations.Count()) {
+    printf_stderr("there is no active animation, we can clean up animation data lm=%lld\n", RootLayerTreeId());
+  } else {
+    printf_stderr("there are %d active animations running lm=%lld\n", sActiveAnimations.Count(), RootLayerTreeId());
   }

 void
 CompositorBridgeParent::ActorDestroy(ActorDestroyReason why)
 {
   ... 
   { // scope lock
     MonitorAutoLock lock(*sIndirectLayerTreesLock);
     sIndirectLayerTrees.erase(mRootLayerTreeID);
   }
+  sActiveAnimations.RemoveEntry(RootLayerTreeId());

Comment 47

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

(In reply to Peter Chang[:pchang] from comment #46)
> Kats, how about maintain another hashtable to record the RootLayerTreeID
> when OMTA is running on that tree?
> For multiple windows, we will add RootLayerTreeId (per windows) into hash
> table if there is active animation.
> We will only clean up the CompositorAnimationStorage when there is no entry
> in this 'ActiveAnimations' hash table.

This seems a little complex to me. I didn't think it through fully but it might work. However another approach that I think might be cleaner is to not have the CompositorAnimationStorage be a singleton. Instead make it a per-compositor storage object. You can put it on the LayerTreeState struct or the CompositorBridgeParent or something.

Comment 48

[Peter Chang[:pchang]]

Comment on attachment 8840348 [details]
Bug 1337889 - Add CompositorAnimationsId for layer animations,

https://reviewboard.mozilla.org/r/114850/#review116730

> Please do:
>  ... id=" PRIu64 " ]",
> instead of using "%llu", because this doesn't work on all platforms.
> 
> Same change needs to be made in Layer::AddAnimation and Layer::SetCompositionAnimations

Done

Comment 49

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116634

> Kats, how about maintain another hashtable to record the RootLayerTreeID when OMTA is running on that tree?
> For multiple windows, we will add RootLayerTreeId (per windows) into hash table if there is active animation.
> We will only clean up the CompositorAnimationStorage when there is no entry in this 'ActiveAnimations' hash table.
> 
> Based this approach, we can use the same logic for Quantum Render. The only disadvantage of this approach is we keep redundant data(from inactive animations) longer but the advantage is that we don't need to worry about the animation data synchronization between content and compositor.
> 
> +nsTHashtable<nsUint64HashKey> sActiveAnimations;
> 
>  void
>  CompositorBridgeParent::CompositeToTarget(DrawTarget* aTarget, const gfx::IntRect* aRect)
>  {
>    ...
>    bool requestNextFrame = mCompositionManager->TransformShadowTree(time, mVsyncRate);
>    if (requestNextFrame) {
>      ScheduleComposition();
> +    sActiveAnimations.PutEntry(RootLayerTreeId());
> +  } else {
> +    sActiveAnimations.RemoveEntry(RootLayerTreeId());
> +  }
> +
> +  if (!sActiveAnimations.Count()) {
> +    printf_stderr("there is no active animation, we can clean up animation data lm=%lld\n", RootLayerTreeId());
> +  } else {
> +    printf_stderr("there are %d active animations running lm=%lld\n", sActiveAnimations.Count(), RootLayerTreeId());
>    }
> 
>  void
>  CompositorBridgeParent::ActorDestroy(ActorDestroyReason why)
>  {
>    ... 
>    { // scope lock
>      MonitorAutoLock lock(*sIndirectLayerTreesLock);
>      sIndirectLayerTrees.erase(mRootLayerTreeID);
>    }
> +  sActiveAnimations.RemoveEntry(RootLayerTreeId());

Now I added CompositorAnimationStorage into CompositorBridgeParent, so each windows(mapped to one compositor) has its own animation storage. Each tab in one window shares one animation storage.

Comment 50

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review116742

> I would prefer that you named the union, and then did memset on the union. The way it's written now, if we add another type inside the union that's larger than mTransform it will not zero it out fully.
> 
> union {
>   AnimationTransform mTransform;
>   float mOpacity;
>   ReallyBigClass mSomeOtherProperty;
> }
> 
> memset(&mTransform, 0, sizeof(AnimationTransform)); // this will only partially zero out mSomeOtherProperty.
> 
> Instead:
> 
> union {
>   AnimationTransform mTransform;
>   float mOpacity;
>   ReallyBigClass mSomeOtherProperty;
> } prop;
> 
> memset(&prop, 0, sizeof(prop)); // this will always work.

After changing to hash-table, I don't need the default constructor and I just marked it as delete.
But here requires different constructors for different enum types.

> Why do you need this? It doesn't seem to handle being assigned a NONE value, and the default assignment operator should work just fine because it's a POD struct, right?

Removed.

> Ditto, this seems unnecessary

Removed.

> this will probably need |explicit| to avoid static analysis build failure.

Done

> nit: indentation

Done

> nit: indentation. I suspect this class will look very different after you switch to use a hashtable, so I'll skip reviewing it for now.

Changed.

> Don't you want to check the return value here?

I modified the getter API to return AnimatedValue* dirlectly, so we could validate the value and its type.

> Don't you want to also check value.mType here?

Fixed.

Comment 51

[Peter Chang[:pchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #47)
> (In reply to Peter Chang[:pchang] from comment #46)
> > Kats, how about maintain another hashtable to record the RootLayerTreeID
> > when OMTA is running on that tree?
> > For multiple windows, we will add RootLayerTreeId (per windows) into hash
> > table if there is active animation.
> > We will only clean up the CompositorAnimationStorage when there is no entry
> > in this 'ActiveAnimations' hash table.
> 
> This seems a little complex to me. I didn't think it through fully but it
> might work. However another approach that I think might be cleaner is to not
> have the CompositorAnimationStorage be a singleton. Instead make it a
> per-compositor storage object. You can put it on the LayerTreeState struct
> or the CompositorBridgeParent or something.

Agree your concern, now I put the CompositorAnimationStroage into CompositorBridgeParent to make it a per-compositor storage object.

Comment 52

[Peter Chang[:pchang]]

Comment on attachment 8840348 [details]
Bug 1337889 - Add CompositorAnimationsId for layer animations,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114850/diff/1-2/

Comment 53

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114852/diff/1-2/

Comment 54

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

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review119396

Looks much better, thanks. Some review comments below.

::: gfx/layers/AnimationHelper.cpp:28
(Diff revision 2)
> +  if (mAnimatedValues.Count()) {
> +    mAnimatedValues.Clear();
> +  }
> +  if (mAnimations.Count()) {
> +    mAnimations.Clear();

I think the .Count() checks are unnecessary, you can just call Clear() unconditionally. It should early-exit if there's nothing in there.

::: gfx/layers/AnimationHelper.cpp:39
(Diff revision 2)
> +}
> +
> +AnimatedValue*
> +CompositorAnimationStorage::GetAnimatedValue(uint64_t aId)
> +{
> +  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());

Thanks for putting these thread assertions here.

::: gfx/layers/composite/AsyncCompositionManager.cpp:1341
(Diff revision 2)
> -    !mPreviousFrameTimeStamp.IsNull() ?
> +                     !mPreviousFrameTimeStamp.IsNull() ?
> -      mPreviousFrameTimeStamp : aCurrentFrame,
> +                     mPreviousFrameTimeStamp : aCurrentFrame,

Please keep the second line here indented a little more, as it is a continuation of the expression

::: gfx/layers/ipc/LayerTransactionParent.cpp:721
(Diff revision 2)
> +  if (!storage) {
> +    return IPC_OK();

Do we want to be more aggressive here and return IPC_FAIL_NO_REASON instead of IPC_OK? The old code did effectively that, and I *think* this is only exercised during tests, so it might be a good idea, to more easily catch errors. GetAnimationStorage should only ever return null if the layers id passed to it is bogus, so I think it would be good to fail on that.

::: gfx/layers/ipc/LayerTransactionParent.cpp:753
(Diff revision 2)
> -  // recover the untranslated transform from the shadow transform. For
> +  if (!storage) {
> -  // transforms that are not set by animation we don't have this information
> -  // available.
> -  if (!layer->AsHostLayer()->GetShadowTransformSetByAnimation()) {
>      *aTransform = mozilla::void_t();
>      return IPC_OK();

Again, I think instead of setting *aTransform to void_t() it might make more sense to return IPC_FAIL_NO_REASON here.

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review119402

This looks really nice, but I'd want to know there is no way to avoid storing AnimationArray and TransformData.

::: gfx/layers/AnimationHelper.h:58
(Diff revision 2)
> +
> +  AnimatedValue(gfx::Matrix4x4&& aTransformInDevSpace,
> +                gfx::Matrix4x4&& aFrameTransform,
> +                const TransformData& aData)
> +  {
> +    mType = AnimatedValue::TRANSFORM;

I prefer to use ': mType(AnimatedValue::TRANSFORM)'.

::: gfx/layers/AnimationHelper.h:65
(Diff revision 2)
> +    mTransform.mFrameTransform = Move(aFrameTransform);
> +    mTransform.mData = aData;
> +  }
> +
> +  explicit AnimatedValue(const float& aValue)
> +  {

Likewise here.

::: gfx/layers/AnimationHelper.h:80
(Diff revision 2)
> +
> +// CompositorAnimationStorage stores the layer animations and animated value
> +// after sampling based on an unique id (CompositorAnimationsId)
> +class CompositorAnimationStorage final
>  {
> +  typedef nsClassHashtable<nsUint64HashKey, AnimatedValue> AnimatedValueTable;

We really need to store pointers?  I thought we can use nsDataHashtable instead.

::: gfx/layers/AnimationHelper.h:102
(Diff revision 2)
> +  void SetAnimatedValue(uint64_t aId, const float& aOpacity);
> +
> +  /**
> +   * Return the animated value if a given id can map to its animated value
> +   */
> +  AnimatedValue* GetAnimatedValue(uint64_t aId);

This should be const function.

::: gfx/layers/AnimationHelper.h:112
(Diff revision 2)
> +  void SetAnimations(uint64_t aId, const AnimationArray& aAnimations);
> +
> +  /**
> +   * Return the animations if a given id can map to its animations
> +   */
> +  AnimationArray* GetAnimations(uint64_t aId);

This also should be const function.

::: gfx/layers/AnimationHelper.cpp:28
(Diff revision 2)
>  
> +void
> +CompositorAnimationStorage::Clear()
> +{
> +  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> +  if (mAnimatedValues.Count()) {

I don't think Count() check is necessary.

::: gfx/layers/AnimationHelper.cpp:31
(Diff revision 2)
> +{
> +  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> +  if (mAnimatedValues.Count()) {
> +    mAnimatedValues.Clear();
> +  }
> +  if (mAnimations.Count()) {

Likewise here.

::: gfx/layers/AnimationHelper.cpp:50
(Diff revision 2)
> +                                             gfx::Matrix4x4&& aTransformInDevSpace,
> +                                             gfx::Matrix4x4&& aFrameTransform,
> +                                             const TransformData& aData)
> +{
> +  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> +  AnimatedValue* value = new AnimatedValue(Move(aTransformInDevSpace), Move(aFrameTransform), aData);

If we use nsDataHashtable, we don't need to do 'new' I think.

::: gfx/layers/AnimationHelper.cpp:74
(Diff revision 2)
> +  AnimationArray* value = new AnimationArray(aValue);
> +  mAnimations.Put(aId, value);
> +}

I'd really like to avoid this copy.  This is related to my question below about AnimationData, if we store this animation array somewhere in the case of web render, can we reuse it somehow?

::: gfx/layers/composite/AsyncCompositionManager.cpp:640
(Diff revision 2)
> +
>        layerCompositor->SetShadowBaseTransform(transform);
>        layerCompositor->SetShadowTransformSetByAnimation(true);
> +      aStorage->SetAnimatedValue(aLayer->GetCompositorAnimationsId(),
> +                                 Move(transform), Move(frameTransform),
> +                                 aAnimationData);

I think we should pass transformData instead. Though I still don't understand web render architecture, if we have no layers in web render, I am wondering where this AnimationData is stored.  If there is a place for AnimationData, we don't need to store it CompositorAnimationStorage?

::: gfx/layers/composite/AsyncCompositionManager.cpp:1327
(Diff revision 2)
> +  // GetAnimationStorage in CompositorBridgeParent expects id as 0
> +  CompositorAnimationStorage* storage =
> +    mCompositorBridge->GetAnimationStorage(0);

Actually I don't understand this, but I am OK if kats is OK.

Comment 56

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review119396

> I think the .Count() checks are unnecessary, you can just call Clear() unconditionally. It should early-exit if there's nothing in there.

Done

> Please keep the second line here indented a little more, as it is a continuation of the expression

Done

> Do we want to be more aggressive here and return IPC_FAIL_NO_REASON instead of IPC_OK? The old code did effectively that, and I *think* this is only exercised during tests, so it might be a good idea, to more easily catch errors. GetAnimationStorage should only ever return null if the layers id passed to it is bogus, so I think it would be good to fail on that.

Good suggestion. Fixed.

> Again, I think instead of setting *aTransform to void_t() it might make more sense to return IPC_FAIL_NO_REASON here.

Fixed.

Comment 57

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review119402

In bug 1345017, I will add sample animation support for Quantum Render. 
In the end, I would like to keep only one copy of animation data( AnimationArray and AnimData Array) and which saves in CompositorAnimationStorage in compositor side. Therefore, current gecko or Quantum render could get these animation data for sampleanimation. The final animated values will be saved in CompositorAnimationStorage and shadowLayerTree(for gecko, this is extra copy cost). For Quantum Render and OMTA testing, we can provide the result directly from CompositorAnimationStorage.

TransformData is part of AnimationArray. Once we keep one copy of above data, I can remove the copy of TransformData here. How do you think?

> We really need to store pointers?  I thought we can use nsDataHashtable instead.

I read MDN that suggests to use nsDataHashtable for simple type, like int or bool.
Therefore, I used nsClassHashtable to store AnimatedValue.


nsDataHashtable<KeyClass, DataType> - DataType is a simple type such as PRUint32 or PRBool.
https://developer.mozilla.org/en-US/docs/Detailed_XPCOM_hashtable_guide

> If we use nsDataHashtable, we don't need to do 'new' I think.

I explained why I use nsClassHashTable above.

> I'd really like to avoid this copy.  This is related to my question below about AnimationData, if we store this animation array somewhere in the case of web render, can we reuse it somehow?

I will remove the SetAnimations call(LayerTransactionParent.cpp) in this patch but still keep Get/SetAnimations interfaces. Therefore, we don't need to worry the copy right now.

> I think we should pass transformData instead. Though I still don't understand web render architecture, if we have no layers in web render, I am wondering where this AnimationData is stored.  If there is a place for AnimationData, we don't need to store it CompositorAnimationStorage?

Good catch. Yes, we need to pass transformData instead. Actually animationdata[1] here is the same as transform data.

I will create a follow up bug to rename [1] to MaybeTransformData which is more clear.


[1]http://searchfox.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh#187

> Actually I don't understand this, but I am OK if kats is OK.

This is regarding the relation between parent(windows) and child(tabs) in one windown.
We assign parent's id as zero, and assign child's id based on a layer tree map.

Comment 58

[Peter Chang[:pchang]]

Comment on attachment 8840348 [details]
Bug 1337889 - Add CompositorAnimationsId for layer animations,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114850/diff/2-3/

Comment 59

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114852/diff/2-3/

Comment 60

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

https://reviewboard.mozilla.org/r/114852/#review119544

r=me with the below changes.

I hope the follow-up bug will be fixed as soon as possible.

::: gfx/layers/AnimationHelper.h:54
(Diff revisions 2 - 3)
>    union {
>      AnimationTransform mTransform;
>      float mOpacity;
>    };
>  
> -  AnimatedValue(gfx::Matrix4x4&& aTransformInDevSpace,
> +  explicit AnimatedValue(gfx::Matrix4x4&& aTransformInDevSpace,

I think we don't need explicit here.

::: gfx/layers/AnimationHelper.h:102
(Diff revisions 2 - 3)
>    void SetAnimatedValue(uint64_t aId, const float& aOpacity);
>  
>    /**
>     * Return the animated value if a given id can map to its animated value
>     */
> -  AnimatedValue* GetAnimatedValue(uint64_t aId);
> +  AnimatedValue* GetAnimatedValue(const uint64_t& aId);

I meant:
const AnimatedValue* GetAnimatedValue() const;
or
AnimatedValue* GetAnimatedValue() const;

::: gfx/layers/AnimationHelper.h:112
(Diff revisions 2 - 3)
>    void SetAnimations(uint64_t aId, const AnimationArray& aAnimations);
>  
>    /**
>     * Return the animations if a given id can map to its animations
>     */
> -  AnimationArray* GetAnimations(uint64_t aId);
> +  AnimationArray* GetAnimations(const uint64_t& aId);

Likewise.

Comment 61

[Hiroyuki Ikezoe (:hiro)]

(In reply to Peter Chang[:pchang] from comment #57)
> > We really need to store pointers?  I thought we can use nsDataHashtable instead.
> 
> I read MDN that suggests to use nsDataHashtable for simple type, like int or
> bool.
> Therefore, I used nsClassHashtable to store AnimatedValue.

I think we can use nsDataHashtable here (see 1304922 comment 53), but it's OK if you prefer it.

Comment 62

[Peter Chang[:pchang]]

Comment on attachment 8840349 [details]
Bug 1337889 - use CompositorAnimationsId to query Opacity/Transform animation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114852/diff/3-4/

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/395c8cab2c26
Add CompositorAnimationsId for layer animations, r=hiro
https://hg.mozilla.org/integration/autoland/rev/415811f3804f
use CompositorAnimationsId to query Opacity/Transform animation, r=hiro,kats

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/395c8cab2c26
https://hg.mozilla.org/mozilla-central/rev/415811f3804f