Closed Bug 706179 Opened 13 years ago Closed 12 years ago

Async CSS animation

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: BenWa, Assigned: dzbarsky)

References

(Depends on 1 open bug)

Details

(Whiteboard: [Snappy:P1][soft])

Attachments

(4 files, 17 obsolete files)

53.80 KB, patch
dbaron
: review+
roc
: review+
Details | Diff | Splinter Review
18.06 KB, patch
roc
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
4.10 KB, patch
cjones
: review+
dzbarsky
: checkin+
Details | Diff | Splinter Review
27.41 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
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.
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.
Depends on: 702739
Readback for old-style transparent plugins would make async animation fantastically complicated.  We should probably just disable async animation when there are readback consumers.
Whiteboard: [Snappy:P1]
We can derive the Layers.h API and compositor impl from this.
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.
Yep.  As long as it's not too COMtaminated I don't expect a lot of issues there.
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.
(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?
I think we can avoid having layout depend on the getters for animated properties. We should probably remove the getters from the public API.
Blocks: 749062
Attached patch starter patch (obsolete) — Splinter Review
Attachment #613055 - Attachment is obsolete: true
No longer depends on: omtc
Depends on: omtc
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"
So apparently the Bezier function can change between keyframes, so we should probably store it on the KeyFrame rather than on the Animation.
I'm going to take cjones' patch and run with it.
Assignee: nobody → mrbkap
Attached patch Layers+layout wip patch (obsolete) — Splinter Review
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.
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.
Depends on: 758984
blocking-basecamp: --- → +
Whiteboard: [Snappy:P1] → [Snappy:P1][soft]
blocking-kilimanjaro: --- → +
Attached patch Layers patch (obsolete) — Splinter Review
Assignee: mrbkap → dzbarsky
Attachment #622642 - Attachment is obsolete: true
Attachment #626268 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #630384 - Flags: review?(roc)
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?
Attached patch Patch (obsolete) — Splinter Review
Fixed transforms and a bunch of other bugs.
Attachment #630384 - Attachment is obsolete: true
Attachment #630384 - Flags: review?(roc)
Attachment #632974 - Flags: review?(roc)
Comment on attachment 632974 [details] [diff] [review]
Patch

dbaron, could you look at the interpolation code in gfx/layers/ipc/CompositorParent.cpp
Attachment #632974 - Flags: review?(dbaron)
Hmm, this patch is a little messed up from the backout of OMTC Basic Layers.  I'll fix it tomorrow.
(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.
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.
Attached patch Patch, cleaned up a little (obsolete) — Splinter Review
Attachment #632974 - Attachment is obsolete: true
Attachment #632974 - Flags: review?(roc)
Attachment #632974 - Flags: review?(dbaron)
Attachment #633287 - Flags: review?(roc)
Attachment #633287 - Flags: review?(dbaron)
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?
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.
Attached patch Patch (obsolete) — Splinter Review
Passes try-server preffed off.
Attachment #633287 - Attachment is obsolete: true
Attachment #633287 - Flags: review?(roc)
Attachment #633287 - Flags: review?(dbaron)
Attachment #634549 - Flags: review?(roc)
Attachment #634549 - Flags: review?(dbaron)
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?
OS: Mac OS X → All
Hardware: x86 → All
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?
Roc, I just copied the same thing that we did for nsRect.  It would be a little weird to have them be different.
Attached patch Patch (obsolete) — Splinter Review
Attachment #636668 - Flags: review?(roc)
Attachment #636668 - Flags: review?(dbaron)
Attachment #634549 - Attachment is obsolete: true
Attachment #634549 - Flags: review?(roc)
Attachment #634549 - Flags: review?(dbaron)
Blocks: 768440
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?
(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
(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.
(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<>> ?
Depends on: 769266
(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?
No, InfallibleTArray is better --- didn't mean for you to change that.
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?
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.
Attached patch Layers patch (obsolete) — Splinter Review
Attachment #636668 - Attachment is obsolete: true
Attachment #636668 - Flags: review?(roc)
Attachment #636668 - Flags: review?(dbaron)
Attachment #640384 - Flags: review?(roc)
Attachment #640384 - Flags: review?(dbaron)
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();

{}
(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.
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.
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.
Attached patch Scalingmatrix changes (obsolete) — Splinter Review
These are just the changes you requested in your last comment.
Attachment #641231 - Flags: review?(roc)
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.
(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 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.
Attached patch Scalingmatrix changes v2 (obsolete) — Splinter Review
Attachment #641231 - Attachment is obsolete: true
Attachment #641231 - Flags: review?(roc)
Attachment #641339 - Flags: review?(roc)
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
Attachment #641339 - Flags: review?(roc) → review+
Attached patch PatchSplinter Review
Attachment #640384 - Attachment is obsolete: true
Attachment #641339 - Attachment is obsolete: true
Attachment #640384 - Flags: review?(roc)
Attachment #640384 - Flags: review?(dbaron)
Attachment #642822 - Flags: review?(dbaron)
Attachment #642822 - Flags: review?(roc)
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?
Attachment #642822 - Flags: review?(roc) → review+
Attached patch Address comment 55 (obsolete) — Splinter Review
Attachment #643042 - Flags: review?(roc)
Attached patch Rollup patch of everything (obsolete) — Splinter Review
Attached patch Rollup rebased on apz (obsolete) — Splinter Review
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.
(Although I'm about 95% sure they are.)
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
Attachment #642822 - Flags: review?(dbaron) → review+
(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.
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 (==)
Target Milestone: mozilla17 → ---
Attachment #645584 - Flags: review?(roc)
I'm not sure about which GetTransform calls really want GetBaseTransform, running on tryserver to see if I missed any.
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
Attachment #645584 - Flags: review?(roc) → review+
(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.
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/169ff207ed19
https://hg.mozilla.org/mozilla-central/rev/a76e0a267f26
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
This majorly broke Firefox for Android on Mozilla-Central; see bug 778580
Backed out:
http://hg.mozilla.org/mozilla-central/rev/9d2a7a8ca1c7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ignore the PLayers changes, those didn't actually change here. I'm going to land this patch along with the "Add BaseTransform to layers"
Attachment #647308 - Flags: review?(jones.chris.g)
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.
Attachment #647308 - Flags: review?(jones.chris.g)
Attachment #643042 - Attachment is obsolete: true
Attachment #644037 - Attachment is obsolete: true
Attachment #644205 - Attachment is obsolete: true
Attachment #647308 - Attachment is obsolete: true
Attachment #647373 - Flags: review?(jones.chris.g)
Comment on attachment 647373 [details] [diff] [review]
Translate the layer after we apply scaling

r=me with the slight comment change
Attachment #647373 - Flags: review?(jones.chris.g) → review+
Landed the Basetransform changes to isolate breakage
https://hg.mozilla.org/integration/mozilla-inbound/rev/e64fcdf0c985
Whiteboard: [Snappy:P1][soft] → [Snappy:P1][soft][leave open]
Attachment #645584 - Flags: checkin+
Attachment #647373 - Flags: checkin+
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
Depends on: 779344
David, what still needs to land here?
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.
Attachment #648623 - Flags: review?(matt.woodrow)
OK, this one actually work on fennec. Yay!
Attachment #648623 - Attachment is obsolete: true
Attachment #648623 - Flags: review?(matt.woodrow)
Attachment #648635 - Flags: review?(matt.woodrow)
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.
Attachment #648635 - Flags: review?(matt.woodrow) → review+
Landed the last part:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e76632e148d6
Whiteboard: [Snappy:P1][soft][leave open] → [Snappy:P1][soft]
https://hg.mozilla.org/mozilla-central/rev/e76632e148d6
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 781097
Blocks: 958480
No longer blocks: 958480
You need to log in before you can comment on or make changes to this bug.