Last Comment Bug 706179 - Async CSS animation
: Async CSS animation
Status: RESOLVED FIXED
[Snappy:P1][soft]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
: 711991 730387 796359 (view as bug list)
Depends on: omtc 711997 715433 769266 702739 711991 758984 779344 781097
Blocks: 713532 749062 759252 b2g-layers-work 755084 768440 enable-omt-animations
  Show dependency treegraph
 
Reported: 2011-11-29 11:00 PST by Benoit Girard (:BenWa)
Modified: 2015-12-12 02:59 PST (History)
49 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
strawman PLayers spec for animation (1.74 KB, patch)
2012-04-06 18:56 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
starter patch (33.81 KB, patch)
2012-05-09 12:57 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
skeleton of layers/compositor side (35.88 KB, patch)
2012-05-10 00:15 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Layers+layout wip patch (66.06 KB, patch)
2012-05-22 17:47 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Layers patch (24.60 KB, patch)
2012-06-05 17:33 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch (47.29 KB, patch)
2012-06-13 17:36 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch, cleaned up a little (40.29 KB, patch)
2012-06-14 14:46 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch (41.65 KB, patch)
2012-06-19 12:37 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch (42.17 KB, patch)
2012-06-26 05:22 PDT, David Zbarsky (:dzbarsky)
cjones.bugs: review+
Details | Diff | Review
Layers patch (46.69 KB, patch)
2012-07-09 15:19 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Scalingmatrix changes (8.82 KB, patch)
2012-07-11 15:43 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Scalingmatrix changes v2 (7.43 KB, patch)
2012-07-11 20:52 PDT, David Zbarsky (:dzbarsky)
roc: review+
Details | Diff | Review
Patch (53.80 KB, patch)
2012-07-16 18:32 PDT, David Zbarsky (:dzbarsky)
dbaron: review+
roc: review+
Details | Diff | Review
Address comment 55 (949 bytes, patch)
2012-07-17 11:10 PDT, David Zbarsky (:dzbarsky)
roc: review+
Details | Diff | Review
Rollup patch of everything (227.35 KB, patch)
2012-07-19 15:28 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Rollup rebased on apz (226.16 KB, patch)
2012-07-20 00:28 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Add BaseTransform to layers (18.06 KB, patch)
2012-07-24 16:48 PDT, David Zbarsky (:dzbarsky)
roc: review+
dzbarsky: checkin+
Details | Diff | Review
Translate the layer after we apply scaling (5.70 KB, patch)
2012-07-30 14:56 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Translate the layer after we apply scaling (4.10 KB, patch)
2012-07-30 17:41 PDT, David Zbarsky (:dzbarsky)
cjones.bugs: review+
dzbarsky: checkin+
Details | Diff | Review
Part 3: Turn on the use of scaling for layers in FrameLayerBuilder (27.66 KB, text/plain)
2012-08-02 22:55 PDT, David Zbarsky (:dzbarsky)
no flags Details
Part 3: Turn on the use of scaling for layers in FrameLayerBuilder (27.41 KB, patch)
2012-08-03 00:06 PDT, David Zbarsky (:dzbarsky)
matt.woodrow: review+
Details | Diff | Review

Description Benoit Girard (:BenWa) 2011-11-29 11:00:40 PST
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 2011-11-29 16:51:58 PST
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 2012-01-05 01:16:19 PST
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 3 Joe Drew (not getting mail) 2012-02-24 12:15:09 PST
*** Bug 730387 has been marked as a duplicate of this bug. ***
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-06 18:56:30 PDT
Created attachment 613055 [details] [diff] [review]
strawman PLayers spec for animation

We can derive the Layers.h API and compositor impl from this.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-07 04:12:08 PDT
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 2012-04-07 23:19:23 PDT
Yep.  As long as it's not too COMtaminated I don't expect a lot of issues there.
Comment 7 Benoit Girard (:BenWa) 2012-04-11 07:49:47 PDT
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 2012-04-11 18:23:15 PDT
(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) (Exited; email my personal email if necessary) 2012-04-11 22:14:08 PDT
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 2012-05-09 12:57:35 PDT
Created attachment 622473 [details] [diff] [review]
starter patch
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-10 00:15:35 PDT
Created attachment 622642 [details] [diff] [review]
skeleton of layers/compositor side
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-21 12:44:09 PDT
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) 2012-05-21 20:42:35 PDT
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) (please use needinfo!) 2012-05-22 17:39:04 PDT
I'm going to take cjones' patch and run with it.
Comment 15 David Zbarsky (:dzbarsky) 2012-05-22 17:47:48 PDT
Created attachment 626268 [details] [diff] [review]
Layers+layout wip patch
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 00:56:10 PDT
\o/
Comment 17 David Zbarsky (:dzbarsky) 2012-05-25 00:47:31 PDT
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 2012-05-25 01:02:28 PDT
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) 2012-06-05 17:33:08 PDT
Created attachment 630384 [details] [diff] [review]
Layers patch
Comment 20 David Zbarsky (:dzbarsky) 2012-06-10 13:31:37 PDT
*** Bug 711991 has been marked as a duplicate of this bug. ***
Comment 21 Oleg Romashin (:romaxa) 2012-06-11 19:15:03 PDT
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) 2012-06-13 17:36:48 PDT
Created attachment 632974 [details] [diff] [review]
Patch

Fixed transforms and a bunch of other bugs.
Comment 23 David Zbarsky (:dzbarsky) 2012-06-13 17:37:39 PDT
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) 2012-06-14 01:36:36 PDT
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] 2012-06-14 03:53:10 PDT
(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) 2012-06-14 11:31:35 PDT
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) 2012-06-14 14:46:15 PDT
Created attachment 633287 [details] [diff] [review]
Patch, cleaned up a little
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-18 15:09:15 PDT
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) 2012-06-18 15:34:05 PDT
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) 2012-06-19 12:37:35 PDT
Created attachment 634549 [details] [diff] [review]
Patch

Passes try-server preffed off.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-19 15:53:36 PDT
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) (Exited; email my personal email if necessary) 2012-06-20 20:00:24 PDT
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) 2012-06-25 05:35:55 PDT
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) (Exited; email my personal email if necessary) 2012-06-25 20:08:43 PDT
So you did. OK.
Comment 35 David Zbarsky (:dzbarsky) 2012-06-26 05:22:45 PDT
Created attachment 636668 [details] [diff] [review]
Patch
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-26 20:50:41 PDT
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 2012-06-27 02:59:47 PDT
Comment on attachment 636668 [details] [diff] [review]
Patch

r+ on GetLocalOpacity().
Comment 38 David Zbarsky (:dzbarsky) 2012-06-27 05:48:29 PDT
(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) 2012-06-27 08:02:19 PDT
(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) (Exited; email my personal email if necessary) 2012-06-27 14:58:49 PDT
(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) 2012-06-30 20:23:17 PDT
(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) (Exited; email my personal email if necessary) 2012-07-01 04:32:35 PDT
No, InfallibleTArray is better --- didn't mean for you to change that.
Comment 43 David Zbarsky (:dzbarsky) 2012-07-01 12:13:44 PDT
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-7 (review requests must explain patch) 2012-07-02 15:43:44 PDT
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) 2012-07-09 15:19:53 PDT
Created attachment 640384 [details] [diff] [review]
Layers patch
Comment 46 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-09 21:31:24 PDT
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) 2012-07-09 22:30:43 PDT
(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) (Exited; email my personal email if necessary) 2012-07-09 23:08:47 PDT
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) 2012-07-11 14:28:02 PDT
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) 2012-07-11 15:43:37 PDT
Created attachment 641231 [details] [diff] [review]
Scalingmatrix changes

These are just the changes you requested in your last comment.
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-11 19:05:31 PDT
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) (Exited; email my personal email if necessary) 2012-07-11 19:06:22 PDT
(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) (Exited; email my personal email if necessary) 2012-07-11 19:21:36 PDT
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) 2012-07-11 20:52:28 PDT
Created attachment 641339 [details] [diff] [review]
Scalingmatrix changes v2
Comment 55 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-11 23:00:10 PDT
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) 2012-07-16 18:32:38 PDT
Created attachment 642822 [details] [diff] [review]
Patch
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-16 20:34:08 PDT
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) 2012-07-17 11:10:23 PDT
Created attachment 643042 [details] [diff] [review]
Address comment 55
Comment 59 David Zbarsky (:dzbarsky) 2012-07-19 15:28:34 PDT
Created attachment 644037 [details] [diff] [review]
Rollup patch of everything
Comment 60 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 00:28:05 PDT
Created attachment 644205 [details] [diff] [review]
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 2012-07-20 00:36:50 PDT
(Although I'm about 95% sure they are.)
Comment 62 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-21 15:18:00 PDT
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) 2012-07-21 15:39:37 PDT
(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) 2012-07-23 17:35:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f5189b1d0c
Comment 65 Matt Brubeck (:mbrubeck) 2012-07-23 20:42:50 PDT
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) 2012-07-24 16:48:19 PDT
Created attachment 645584 [details] [diff] [review]
Add BaseTransform to layers
Comment 67 David Zbarsky (:dzbarsky) 2012-07-24 16:49:18 PDT
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) (Exited; email my personal email if necessary) 2012-07-24 17:14:19 PDT
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) (Exited; email my personal email if necessary) 2012-07-24 17:16:32 PDT
(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 74 Aaron Train [:aaronmt] 2012-07-30 08:53:40 PDT
This majorly broke Firefox for Android on Mozilla-Central; see bug 778580
Comment 75 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-30 11:39:33 PDT
Backed out:
http://hg.mozilla.org/mozilla-central/rev/9d2a7a8ca1c7
Comment 76 David Zbarsky (:dzbarsky) 2012-07-30 14:56:01 PDT
Created attachment 647308 [details] [diff] [review]
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 2012-07-30 17:24:47 PDT
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) 2012-07-30 17:41:31 PDT
Created attachment 647373 [details] [diff] [review]
Translate the layer after we apply scaling
Comment 79 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-30 18:19:53 PDT
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) 2012-07-30 19:22:59 PDT
Landed the Basetransform changes to isolate breakage
https://hg.mozilla.org/integration/mozilla-inbound/rev/e64fcdf0c985
Comment 81 Ed Morley [:emorley] 2012-07-31 06:11:08 PDT
https://hg.mozilla.org/mozilla-central/rev/e64fcdf0c985
Comment 82 David Zbarsky (:dzbarsky) 2012-07-31 10:30:13 PDT
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] 2012-07-31 19:18:15 PDT
https://hg.mozilla.org/mozilla-central/rev/9538c86590a4
Comment 84 :Ms2ger 2012-08-02 02:33:39 PDT
David, what still needs to land here?
Comment 85 David Zbarsky (:dzbarsky) 2012-08-02 09:56:54 PDT
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) 2012-08-02 22:55:11 PDT
Created attachment 648623 [details]
Part 3: Turn on the use of scaling for layers in FrameLayerBuilder
Comment 87 David Zbarsky (:dzbarsky) 2012-08-03 00:06:40 PDT
Created attachment 648635 [details] [diff] [review]
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) (PTO until 27 June) 2012-08-03 13:27:34 PDT
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) 2012-08-03 14:30:03 PDT
Landed the last part:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e76632e148d6
Comment 90 Ed Morley [:emorley] 2012-08-04 11:18:24 PDT
https://hg.mozilla.org/mozilla-central/rev/e76632e148d6
Comment 91 David Zbarsky (:dzbarsky) 2012-10-01 21:07:46 PDT
*** Bug 796359 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.