706179 - Async CSS animation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benoit Girard (:BenWa)]

My understanding is that once we have Off-main-thread-composition we can perform simple css animation asynchronously. Roc should know the details once we're ready to begin work on this.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We won't get it for free after async compositing, but async compositing is a prerequisite.  We need some work in the layout engine like bug 702739, and we need Layers API support for animating properties.  Neither should be particularly difficult.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Readback for old-style transparent plugins would make async animation fantastically complicated.  We should probably just disable async animation when there are readback consumers.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: strawman PLayers spec for animation

We can derive the Layers.h API and compositor impl from this.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's reasonable, but it does mean we have to make all the interpolation logic from the style system available in the layer system, especially onerous for transforms.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yep.  As long as it's not too COMtaminated I don't expect a lot of issues there.

Comment 7

[Benoit Girard (:BenWa)]

I don't really like having the implementation details of the interpolation logic exposed in the ipdl like this. How about an abstract 'AnimatedProperty' class used in the ipdl and we can let subclasses provide a simple serializer method. 'AnimatedProperty' would have an abstract 'void AnimateLayer(Layer*, TimeStamp targetTime) = 0' method.

We should consider how animated properties will affect the getters. What value does getOpacity() returns if it's being animated? We need some way to request the animated value for time 'X'. One option is to have a method that will transform the layer tree to time 'X' and then any getters will return the expected value for that timestamp.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Benoit Girard (:BenWa) from comment #7)
> I don't really like having the implementation details of the interpolation
> logic exposed in the ipdl like this. How about an abstract
> 'AnimatedProperty' class used in the ipdl and we can let subclasses provide
> a simple serializer method. 'AnimatedProperty' would have an abstract 'void
> AnimateLayer(Layer*, TimeStamp targetTime) = 0' method.
> 

That smells like overengineering to me.  It's abstraction and additional complication for what benefit?

> We should consider how animated properties will affect the getters. What
> value does getOpacity() returns if it's being animated? We need some way to
> request the animated value for time 'X'. One option is to have a method that
> will transform the layer tree to time 'X' and then any getters will return
> the expected value for that timestamp.

What content-side (LayerManager2.0, as opposed to compositor) code do you now of that requires the interpolated values of layer properties?  AFAIK layout code answers queries from web content.  But I'm also not sure if that code will continue to work with async animation.

roc?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we can avoid having layout depend on the getters for animated properties. We should probably remove the getters from the public API.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: starter patch

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: skeleton of layers/compositor side

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

To try out this patch, you need to
 - run on an omtc-enabled platform, OS X, x11, android, or gonk right now
 - flip the "layers.offmainthreadcomposition.enabled" pref
 - for x11 and sometimes OS X, force on GL layers "layers.acceleration.force-enabled"

Comment 13

[David Zbarsky (:dzbarsky)]

So apparently the Bezier function can change between keyframes, so we should probably store it on the KeyFrame rather than on the Animation.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I'm going to take cjones' patch and run with it.

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: Layers+layout wip patch

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

\o/

Comment 17

[David Zbarsky (:dzbarsky)]

Blake, I just had an idea for the Bezier.  We could create a class in layout that stores different ComputedTimingFunctions hashed on the Bezier coefficients (which may also be a memory win, i'm guessing the majority of animations are linear or ease/ease-in/ease-out), and just call into it from layers with our coefficients to have it compute the point for us.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

After the lookup, we still have to sample the timing function right?  And the coefficients occupy 16 bytes?  The overhead of hashing and cross-thread table synchronization leaves me doubtful that this would win any perf, but would definitely add complexity.  But I may be misunderstanding the problem you're trying to solve.

Comment 19

[David Zbarsky (:dzbarsky)]

Attachment: Layers patch

Comment 21

[Oleg Romashin (:romaxa)]

Comment on attachment 630384 [details] [diff] [review]
Layers patch


>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h
...
> class ThebesLayer;
>+typedef InfallibleTArray<Animation> AnimationArray;
> 
with this change cannot anymore use nsIWidget.h outside of MOZILLA_INTERNAL_API (external extensions code), 
should it be ifdefed with MOZILLA_INTERNAL_API?

Comment 22

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Fixed transforms and a bunch of other bugs.

Comment 23

[David Zbarsky (:dzbarsky)]

Comment on attachment 632974 [details] [diff] [review]
Patch

dbaron, could you look at the interpolation code in gfx/layers/ipc/CompositorParent.cpp

Comment 24

[David Zbarsky (:dzbarsky)]

Hmm, this patch is a little messed up from the backout of OMTC Basic Layers.  I'll fix it tomorrow.

Comment 25

[Marco Castelluccio [:marco]]

(In reply to David Zbarsky from comment #24)
> Hmm, this patch is a little messed up from the backout of OMTC Basic Layers.
> I'll fix it tomorrow.

It's relanded now.

Comment 26

[David Zbarsky (:dzbarsky)]

Marco, I know.  Blake backed it out in our repo and then I merged to m-c after it was relanded so there's some pieces of it in the diff against m-c.

Comment 27

[David Zbarsky (:dzbarsky)]

Attachment: Patch, cleaned up a little

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 633287 [details] [diff] [review]
Patch, cleaned up a little

Could you describe:
 * what this patch does, and
 * how it fits in to the larger picture?

Comment 29

[David Zbarsky (:dzbarsky)]

This patch enables adding animations to a layer, and when there are animations on a layer, the compositor will animate them, using code in layout to interpolate between the start and end states.

The patch in bug 755084 has the changes in layout to decide when an animation can be done on the compositor and the code to convert ElementAnimations* to a representation that can be shipped over to the compositor thread.  It addition, it prevents the gecko thread from interpolating the animation if the compositor is going to do it and makes sure that we will create separate layers for elements that will be animated by the compositor.

Comment 30

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Passes try-server preffed off.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 634549 [details] [diff] [review]
Patch

Some thoughts so far:

>+  // special, private, don't touch

This could use a clearer description.  Obviously somebody touches it;
 otherwise there wouldn't be a point to having it.


>+struct Animation {
>+  TimeStamp startTime;
>+  TimeDuration duration;
>+  // For each frame, the interpolation point is computed based on the
>+  // elapsed time between [start, end] and the sample function.  Then
>+  // the property's value is interpolated at that point.  There must
>+  // be keyframes for points 0.0 and 1.0.  The keyframes must be
>+  // sorted by |point|.
>+  AnimationSegment[] segments;
>+  // How many times to repeat the animation.  < 0 means "forever".
>+  int32_t numIterations;
>+  // 0 = forward
>+  // 1 = alternate
>+  int32_t direction;
>+  AnimationData data;

I'm really not happy about the idea of duplicating the logic to
implement all of these things.  (I have two local patches to them right
now, for example, and this patch appears to duplicate the logic for
handling all of this into the |SampleAnimations| function.)

(Also, don't say 0 = ..., say you're using the
NS_STYLE_ANIMATION_DIRECTION_* constants.  One of my local patches adds
two more of them.)

I'm also not crazy about CreateCSSValueList, and not quite sure how to
verify that it's going to produce the same results as the existing code.

Is it intentional that there are features in animations (like, say, step
timing functions) that you don't support here?  Is the plan to avoid
doing those animations on the compositor thread?  Or was that an oversight?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 634549 [details] [diff] [review]
Patch

Review of attachment 634549 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsRect.h
@@ +203,5 @@
> +  // This is here only to keep IPDL-generated code happy. DO NOT USE.
> +  bool operator==(const nsRect& aRect) const
> +  {
> +    return IsEqualEdges(aRect);
> +  }

This is nasty. Can't we avoid this somehow? E.g. can we make this private and declare a particular class as a friend?

Comment 33

[David Zbarsky (:dzbarsky)]

Roc, I just copied the same thing that we did for nsRect.  It would be a little weird to have them be different.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So you did. OK.

Comment 35

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 636668 [details] [diff] [review]
Patch

Review of attachment 636668 [details] [diff] [review]:
-----------------------------------------------------------------

You're using nsCSSValue and some nsDisplayList stuff off the main thread here for the first time. That should be OK, at least for the nsDisplayList code, but please do add comments to the nsDisplayList methods that now have to work when called by multiple threads. There shouldn't be any shared data involved but we wouldn't want someone to add a global cache or something like that.

::: gfx/layers/Layers.h
@@ +739,5 @@
> +  bool AddAnimation(const Animation& aAnimation);
> +  void ClearAnimations();
> +  // This is only called when the layer tree is updated. To add an animation to
> +  // this layer, use AddAnimation.
> +  void SetAnimations(const AnimationArray& aAnimations);

This comment isn't very clear. Are these "CONSTRUCTION PHASE ONLY"? Which ones is layout allowed to call? Who calls the others?

You need to document what the return value of AddAnimation means, and what callers should do about it.

@@ +991,5 @@
>    nsIntRegion mVisibleRegion;
>    gfx3DMatrix mTransform;
>    gfx3DMatrix mEffectiveTransform;
> +  AnimationArray mAnimations;
> +  InfallibleTArray<InfallibleTArray<css::ComputedTimingFunction*>*> mFunctions;

Should these be nsAutoPtrs?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +380,5 @@
> +static nsCSSValueList*
> +CreateCSSValueList(InfallibleTArray<TransformFunction>& aFunctions)
> +{
> +  nsAutoPtr<nsCSSValueList> result;
> +  nsCSSValueList** resultTail = getter_Transfers(result);

This reads oddly. Try to avoid using getter_Transfers here.

@@ +457,5 @@
> +        arr->Item(12).SetFloatValue(matrix._34, eCSSUnit_Number);
> +        arr->Item(13).SetFloatValue(matrix._41, eCSSUnit_Number);
> +        arr->Item(14).SetFloatValue(matrix._42, eCSSUnit_Number);
> +        arr->Item(15).SetFloatValue(matrix._43, eCSSUnit_Number);
> +        arr->Item(16).SetFloatValue(matrix._44, eCSSUnit_Number);

Crikey this is ugly! I don't actually have any better ideas that don't require rewriting gfx3DMatrix...

@@ +554,5 @@
> +    switch (interpolatedValue.type()) {
> +    case Animatable::TColor:
> +      // TODO
> +      NS_NOTREACHED("Don't animate color yet");
> +      break;

Take this out?

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 636668 [details] [diff] [review]
Patch

r+ on GetLocalOpacity().

Comment 38

[David Zbarsky (:dzbarsky)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> 
> @@ +991,5 @@
> >    nsIntRegion mVisibleRegion;
> >    gfx3DMatrix mTransform;
> >    gfx3DMatrix mEffectiveTransform;
> > +  AnimationArray mAnimations;
> > +  InfallibleTArray<InfallibleTArray<css::ComputedTimingFunction*>*> mFunctions;
> 
> Should these be nsAutoPtrs?

I can't do that because then I have to include AnimationCommon.h in Layer.h, which leads to all kinds of include/forward-declare hell.
 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +380,5 @@
> > +static nsCSSValueList*
> > +CreateCSSValueList(InfallibleTArray<TransformFunction>& aFunctions)
> > +{
> > +  nsAutoPtr<nsCSSValueList> result;
> > +  nsCSSValueList** resultTail = getter_Transfers(result);
> 
> This reads oddly. Try to avoid using getter_Transfers here.

I agree, but I copied this from
http://dxr.lanedo.com/mozilla-central/layout/style/nsStyleAnimation.cpp.html#l1329

Comment 39

[David Zbarsky (:dzbarsky)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> 
> You need to document what the return value of AddAnimation means, and what
> callers should do about it.
> 

AddAnimation should never return false.  That check should actually be an assertion, but cjones says we may fail it during shutdown.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Zbarsky from comment #39)
> AddAnimation should never return false.  That check should actually be an
> assertion, but cjones says we may fail it during shutdown.

At least remove the return value.

(In reply to David Zbarsky from comment #38)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> > Should these be nsAutoPtrs?
> 
> I can't do that because then I have to include AnimationCommon.h in Layer.h,
> which leads to all kinds of include/forward-declare hell.

Deep sigh. At least the outer one can be an nsAutoPtr (or maybe it can just be nsTArray<nsTArray<>> ?

Comment 41

[David Zbarsky (:dzbarsky)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> (In reply to David Zbarsky from comment #39)
> > AddAnimation should never return false.  That check should actually be an
> > assertion, but cjones says we may fail it during shutdown.
> 
> At least remove the return value.

Yep, did this.

> (In reply to David Zbarsky from comment #38)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> > > Should these be nsAutoPtrs?
> > 
> > I can't do that because then I have to include AnimationCommon.h in Layer.h,
> > which leads to all kinds of include/forward-declare hell.
> 
> Deep sigh. At least the outer one can be an nsAutoPtr (or maybe it can just
> be nsTArray<nsTArray<>> ?

Is nsTArray better than InfallibleTArray in this case?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, InfallibleTArray is better --- didn't mean for you to change that.

Comment 43

[David Zbarsky (:dzbarsky)]

Roc, ok, I will leave it as is.

dbaron, I've been thinking about this code, and there is an issue with the way I represent rotations and skew.  Since I convert all rotations to rotate3d and then back to the proper rotation, this will introduce edge case issues since we should not allow linear interpolation between rotate(x) and roate3d(0, 0, 1, y) (but this patch will convert both to rotate).  After our discussion I submitted a post about this issue to the mailing list, but I'm not sure if it ever got posted.  Could you check that?

The same thing happens with skew - we will interpolate between skewX(x) and skewY(y) - easy enough to change, but perhaps there should be a skew(x, y) primitive?

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The primitives you use here need to represent the information that you need to convey.  If the spec requires that animation behavior distinguish between rotate3d() and some other expressions that mean the same thing, then either you need that information or you need to get the spec changed to not require that information.  (That said, right now it looks like the spec doesn't say anything about rotate3d(), which is a bug if I'm reading it correctly.)

And I don't think there should be a skew(x,y) primitive; we used to have one but it was very unintuitive.  I think that means that you'd need more primitives here; otherwise you couldn't distinguish between skewX(0deg) -> skewX(45deg) which is animatable and skewY(0deg) -> skewX(45deg) which is not.

Comment 45

[David Zbarsky (:dzbarsky)]

Attachment: Layers patch

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 640384 [details] [diff] [review]
Layers patch

Review of attachment 640384 [details] [diff] [review]:
-----------------------------------------------------------------

Shouldn't something be multiplying by the scalingMatrix when we composite without going through CompositorParent?

::: gfx/layers/Layers.h
@@ +1015,5 @@
>    nsRefPtr<Layer> mMaskLayer;
>    LayerUserDataSet mUserData;
>    nsIntRegion mVisibleRegion;
>    gfx3DMatrix mTransform;
> +  gfx3DMatrix mScalingMatrix;

Urgh. How about we just store two floats here instead of 16? Change Get/SetScalingMatrix to Get/SetScaleTransform and just use two floats.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +373,5 @@
>  SetShadowProperties(Layer* aLayer)
>  {
>    // FIXME: Bug 717688 -- Do these updates in ShadowLayersParent::RecvUpdate.
>    ShadowLayer* shadow = aLayer->AsShadowLayer();
>    shadow->SetShadowTransform(aLayer->GetTransform());

Shouldn't we be multiplying by GetScalingMatrix here to be consistent with the animation sampling code?

@@ +575,5 @@
> +
> +  bool activeAnimation = false;
> +  SampleAnimations(layer, mLastCompose, &activeAnimation);
> +  if (activeAnimation)
> +    ScheduleComposition();

{}

Comment 47

[David Zbarsky (:dzbarsky)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46)
> Comment on attachment 640384 [details] [diff] [review]
> Layers patch
> 
> Review of attachment 640384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't something be multiplying by the scalingMatrix when we composite
> without going through CompositorParent?
> 

No.  So the way this works with the patch is as follows:

No OMTA: We set the transform to GetTransform() * scalingMatrix * transform.
OMTA: We set the shadow transform to scalingMatrix * interpolatedTransform.

So the reason that we need to set the scaling matrix in the OMTA case is because we just set the interpolated transform without multiplying by GetTransform.

It may be clearer to just always multiply by the scalingMatrix in SetShadowProperties instead of composing the scalingMatrix with GetTransform in ChooseScaleAndSetTransform.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not good to add layer features that only work in certain configurations such as OMTC, and then selectively use them under certain conditions. We should reduce the divergence between code paths by using them consistently and making them work consistently.

Comment 49

[David Zbarsky (:dzbarsky)]

Roc, I tried your suggestion in comment 46.  The animation looks correct, but when it finishes, there is a 1 second pause during which the element disappears, after which it reappears in the correct place.  Going to investigate this some more, but the current patch doesn't have this problem.

Comment 50

[David Zbarsky (:dzbarsky)]

Attachment: Scalingmatrix changes

These are just the changes you requested in your last comment.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 641231 [details] [diff] [review]
Scalingmatrix changes

Review of attachment 641231 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +741,5 @@
> +    mXScale = aXScale;
> +    Mutated();
> +  }
> +
> +  void SetYScale(float aYScale)

Make it SetScale(float aXScale, float aYScale) since they always get called together anyway.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Zbarsky from comment #49)
> Roc, I tried your suggestion in comment 46.  The animation looks correct,
> but when it finishes, there is a 1 second pause during which the element
> disappears, after which it reappears in the correct place.  Going to
> investigate this some more, but the current patch doesn't have this problem.

I hope you can figure it out. I still think it's important to have the Layers API work consistently whether OMTC is on or off.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 641231 [details] [diff] [review]
Scalingmatrix changes

Review of attachment 641231 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +220,5 @@
>    mPrevSibling(nsnull),
>    mImplData(aImplData),
>    mMaskLayer(nsnull),
> +  mXScale(1.0),
> +  mYScale(1.0),

1.0f

::: layout/base/FrameLayerBuilder.cpp
@@ +2036,4 @@
>    aLayer->SetTransform(transform);
> +  // Store the inverse of our resolution-scale on the layer
> +  aLayer->SetXScale(1.0/scale.width);
> +  aLayer->SetYScale(1.0/scale.height);

An explicit float() cast might be needed to avoid warnings on Windows. Better change to 1.0f/float(scale.width) etc.

Comment 54

[David Zbarsky (:dzbarsky)]

Attachment: Scalingmatrix changes v2

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 641339 [details] [diff] [review]
Scalingmatrix changes v2

Review of attachment 641339 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +591,3 @@
>    if (ShadowLayer* shadow = AsShadowLayer())
> +    return scalingMatrix * shadow->GetShadowTransform();
> +  return scalingMatrix * mTransform;

I think you can call Scale() instead, more efficient than a matrix multiply. Or maybe you can't, because Scale() looks wrong to me, but it should be fixed and then you can use it.

::: layout/base/FrameLayerBuilder.cpp
@@ +2034,5 @@
>    }
>  
>    aLayer->SetTransform(transform);
>    // Store the inverse of our resolution-scale on the layer
> +  aLayer->SetScale(1.0f/float(scale.width), 1.0/float(scale.height));

1.0f

Comment 56

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 642822 [details] [diff] [review]
Patch

Review of attachment 642822 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +599,3 @@
>    if (ShadowLayer* shadow = AsShadowLayer())
> +    return scalingMatrix * shadow->GetShadowTransform();
> +  return scalingMatrix * mTransform;

What about my comment #55?

Comment 58

[David Zbarsky (:dzbarsky)]

Attachment: Address comment 55

Comment 59

[David Zbarsky (:dzbarsky)]

Attachment: Rollup patch of everything

Comment 60

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rollup rebased on apz

Mostly trivial except
 - code to sampling CSS animations before pan/zoom, obviously
 - slightly redid the interface to SampleAnimations() to better match SampleContentTransform()
 - ViewTransform is now used by AsyncPanZoomController, so back --> CompositorParent.h
 - use new GetLayerBuilderForManager() API in nsDisplayList

I'm not sure if async animations are actually being used though.

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Although I'm about 95% sure they are.)

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 642822 [details] [diff] [review]
Patch

>+void
>+Layer::AddAnimation(const Animation& aAnimation)
>+{
>+  if (!AsShadowableLayer() || !AsShadowableLayer()->HasShadow())
>+    return;

Is there a way we could avoid spending the time to build up the list
of animations if we're just going to throw it away here?  Could
definitely be a follow-up bug.

>+    switch(aFunctions[i].type()) {

space between switch and (

In Layer::SetAnimations:

>+    nsTArray<AnimationSegment> segments = mAnimations.ElementAt(i).segments();

Much better to use a const nsTArray<AnimationSegment> segments& so
you don't copy the array.  (The reference is the important part, but
const is good too.)

>+      AnimationSegment segment = mAnimations[i].segments()[j];

same here

And same for startFunctions and endFunctions.

In the else in which you handle opacity, you should have an NS_ASSERTION
about segment.endState().type() (i.e., the same type that you handled
in the if).

Layer::ComputeEffectiveTransformForMaskLayer shouldn't get incorrectly
reindented; it needs the space back.  (Especially since there are no
other changes to it.)

I'm not crazy about having #include "nsStyleAnimation.h" in Layers.h.
Is there a way around that?  Probably not.

SampleValue:

Please rename aPoint -> aPortion.  (Point normally has an x and y and
maybe a z.)  Same for SampleAnimations.

Could you add a comment that eventually SampleValue should take the
CSS property as an argument.  (This will be needed if layers ever,
in the future, become capable of animating two properties that have
the same type but different interpolation rules.)

SampleAnimations:

>+  AnimationArray& animations =
>+    const_cast<AnimationArray&>(aLayer->GetAnimations());
>+  InfallibleTArray<AnimData>& animationData =
>+    const_cast<InfallibleTArray<AnimData>&>(aLayer->GetAnimationData());

Maybe these getters shouldn't return const?  It could confuse somebody
looking at the code behind them and not realizing that callers of the
getters can mutate the data returned.

>+  for (PRInt32 i = animations.Length() - 1; i >= 0; --i) {

so that you're not switching an unsigned to signed, instead do:
  for (PRUint32 i = animations.Length(); i-- != 0; ) {

PLayers.ipdl:

>+  // Animated colors are only honored for ColorLayers.

are they?  maybe "will be"??

>+  int type; //this is really nsTimingFunction::Type

based on comments in the other bug, I think you should say 1=...,2=...
(with ... filled in properly)

BezierFunction probably should have been called CubicBezierFunction
(since Bézier functions can have any order, and quadratic Bézier
functions are also common).  If it's not too hard I think it would be
good to rename.  Likewise, the values should probably be called x1, y1,
x2, and y2.  (A cubic bezier function normally has four control points,
but in the case of transition/animation timing functions, x0=y0=0 and
x3=y3=1.)  If not, at the very least you should have a comment
explaining that it's a cubic Bézier and that these 4 values are
x1, y1, x2, and y2.


>+struct TransformData {
>+  nsPoint origin;
>+  gfxPoint3D mozOrigin;
>+  gfxPoint3D perspectiveOrigin;
>+  nsRect bounds;
>+  nscoord perspective;
>+};

Some comments here wouldn't be a bad idea.  Maybe for some of the
other structures too.

>+  float startPoint;
>+  float endPoint;

Maybe Point -> Portion?

>+  // For each frame, the interpolation point is computed based on the
>+  // elapsed time between [start, end] and the sample function.  Then
>+  // the property's value is interpolated at that point.  There must
>+  // be keyframes for points 0.0 and 1.0.  The keyframes must be
>+  // sorted by |point|.

This seems rather unclear.  Isn't it really a function of start,
duration, and direction, rather than start and end?  And the
relationship between keyframes and segments is unclear:  really, since
you're talking about segments, you should say that the segments must
cover (uniquely) the portions from 0.0 to 1.0.

IPCMessageUtils.h:

I'm not a fan of the extra () around the argument to return, but I
guess it's local style, so should probably stay.  (If it were
layout/style I'd ask you to change it.)


r=dbaron with that

Comment 63

[David Zbarsky (:dzbarsky)]

(In reply to David Baron [:dbaron] from comment #62)

> Is there a way we could avoid spending the time to build up the list
> of animations if we're just going to throw it away here?  Could
> definitely be a follow-up bug.

That check is basically for sanity.  The only time a layer does not have a shadow is when we do not have OMTC, in which case GetAnimationsForCompositor will return null.

> 
> I'm not crazy about having #include "nsStyleAnimation.h" in Layers.h.
> Is there a way around that?  Probably not.

Not without allocating the nsStyleAnimation::Value's on the heap.


> Could you add a comment that eventually SampleValue should take the
> CSS property as an argument.  (This will be needed if layers ever,
> in the future, become capable of animating two properties that have
> the same type but different interpolation rules.)

Sure.  I've actually run into this issue already when animating perspective and perspective-origin.  Fortunately they are interpolated the same way, so I can hack around it for now, but it's not that great.

> SampleAnimations:
> 
> >+  AnimationArray& animations =
> >+    const_cast<AnimationArray&>(aLayer->GetAnimations());
> >+  InfallibleTArray<AnimData>& animationData =
> >+    const_cast<InfallibleTArray<AnimData>&>(aLayer->GetAnimationData());
> 
> Maybe these getters shouldn't return const?  It could confuse somebody
> looking at the code behind them and not realizing that callers of the
> getters can mutate the data returned.

I think I do this in a few places.  I will change the getters.

> 
> >+  // Animated colors are only honored for ColorLayers.
> 
> are they?  maybe "will be"??

Will be.  I'll remove Color from Animatable before I land and file a followup to animate color.

> 
> >+  // For each frame, the interpolation point is computed based on the
> >+  // elapsed time between [start, end] and the sample function.  Then
> >+  // the property's value is interpolated at that point.  There must
> >+  // be keyframes for points 0.0 and 1.0.  The keyframes must be
> >+  // sorted by |point|.
> 
> This seems rather unclear.  Isn't it really a function of start,
> duration, and direction, rather than start and end?  And the
> relationship between keyframes and segments is unclear:  really, since
> you're talking about segments, you should say that the segments must
> cover (uniquely) the portions from 0.0 to 1.0.

I didn't write this comment, but I can change it.

Comment 64

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f5189b1d0c

Comment 65

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out (along with two other bugs from the same push) because of reftest failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/442e36401b38

https://tbpl.mozilla.org/php/getParsedLog.php?id=13791557&tree=Mozilla-Inbound
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/transform-3d/perspective-origin-4a.html | image comparison (==)

Comment 66

[David Zbarsky (:dzbarsky)]

Attachment: Add BaseTransform to layers

Comment 67

[David Zbarsky (:dzbarsky)]

I'm not sure about which GetTransform calls really want GetBaseTransform, running on tryserver to see if I missed any.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 645584 [details] [diff] [review]
Add BaseTransform to layers

Review of attachment 645584 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/base/FrameLayerBuilder.cpp
@@ +1268,5 @@
>        data->mImage->ConfigureLayer(imageLayer);
>        // The layer's current transform is applied first, then the result is scaled.
>        gfx3DMatrix transform = imageLayer->GetTransform()*
>          gfx3DMatrix::ScalingMatrix(mParameters.mXScale, mParameters.mYScale, 1.0f);
> +      imageLayer->SetBaseTransform(transform);

Seems like it would make sense to not call SetBaseTransform here and call SetScale instead? It probably doesn't really matter, but this would be more logical.

@@ +1280,5 @@
>        colorLayer->SetIsFixedPosition(data->mLayer->GetIsFixedPosition());
>        colorLayer->SetColor(data->mSolidColor);
>  
>        // Copy transform
> +      colorLayer->SetBaseTransform(data->mLayer->GetTransform());

Probably should copy any scale here as well

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Zbarsky (:dzbarsky) from comment #67)
> I'm not sure about which GetTransform calls really want GetBaseTransform,
> running on tryserver to see if I missed any.

That isn't necessarily foolproof so please audit them manually as well.

I had a look at the ones in layout and I didn't see any additional ones that need to be converted.

Comment 70

[David Zbarsky (:dzbarsky)]

Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/876726b632c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/32d16d0f87c9

Comment 71

[Ed Morley [:emorley]]

Push backed out for xul android R3 failures (tried asking on IRC which specific csets should be backed out, but presume you were afk):
https://tbpl.mozilla.org/php/getParsedLog.php?id=13839408&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13834332&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13837514&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad3878167c1

Comment 72

[David Zbarsky (:dzbarsky)]

Third time's the charm:
https://hg.mozilla.org/integration/mozilla-inbound/rev/169ff207ed19
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76e0a267f26

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/169ff207ed19
https://hg.mozilla.org/mozilla-central/rev/a76e0a267f26

Comment 74

[Aaron Train [:aaronmt]]

This majorly broke Firefox for Android on Mozilla-Central; see bug 778580

Comment 75

[Mark Finkle (:mfinkle) (use needinfo?)]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/9d2a7a8ca1c7

Comment 76

[David Zbarsky (:dzbarsky)]

Attachment: Translate the layer after we apply scaling

Ignore the PLayers changes, those didn't actually change here. I'm going to land this patch along with the "Add BaseTransform to layers"

Comment 77

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 647308 [details] [diff] [review]
Translate the layer after we apply scaling

Need comments on why each interface is being used at each location.  My eyes are crossing a bit, will reread with that.

Comment 78

[David Zbarsky (:dzbarsky)]

Attachment: Translate the layer after we apply scaling

Comment 79

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 647373 [details] [diff] [review]
Translate the layer after we apply scaling

r=me with the slight comment change

Comment 80

[David Zbarsky (:dzbarsky)]

Landed the Basetransform changes to isolate breakage
https://hg.mozilla.org/integration/mozilla-inbound/rev/e64fcdf0c985

Comment 81

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e64fcdf0c985

Comment 82

[David Zbarsky (:dzbarsky)]

Landed the rest of it since the basetransform changes didn't seem to break things this time, and we want these patches on inbound to be able to profile B2G.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9538c86590a4

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9538c86590a4

Comment 84

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

David, what still needs to land here?

Comment 85

[David Zbarsky (:dzbarsky)]

There is one more patch that actually enables us to use the scaling on layers, which needs to land in order to get scaling animations working correctly.  It currently causes problems on Fennec, I'm working on sorting it out.

Comment 86

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Turn on the use of scaling for layers in FrameLayerBuilder

Comment 87

[David Zbarsky (:dzbarsky)]

Attachment: Part 3: Turn on the use of scaling for layers in FrameLayerBuilder

OK, this one actually work on fennec. Yay!

Comment 88

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 648635 [details] [diff] [review]
Part 3: Turn on the use of scaling for layers in FrameLayerBuilder

Review of attachment 648635 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +708,5 @@
> +    aTo.AppendPrintf(" [xPostScale=%g]", mPostXScale);
> +  }
> +  if (1.0 != mPostYScale) {
> +    aTo.AppendPrintf(" [yPostScale=%g]", mPostYScale);
> +  }

Could probably save some space here and collapse these into [preScale=1,2] etc.

::: gfx/layers/Layers.h
@@ +639,5 @@
>      mTransform = aMatrix;
>      Mutated();
>    }
>  
> +  void SetPreScale(float aXScale, float aYScale)

Move this onto ContainerLayer since it's only used there. This should prevent confusion and potential bugs around trying to set/read the pre scale on leaf layers.

::: gfx/thebes/gfx3DMatrix.h
@@ +147,5 @@
>     * | 0  0  0  1 |
>     */
>    void Scale(float aX, float aY, float aZ);
>  
> +  void PostScale(float aX, float aY, float aZ);

Call this ScalePost (to match TranslatePost) and move it under the 'Post-multiplication' section in the header.

Comment 89

[David Zbarsky (:dzbarsky)]

Landed the last part:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e76632e148d6

Comment 90

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e76632e148d6