The default bug view has changed. See this FAQ.

Async CSS animation

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: BenWa, Assigned: dzbarsky)

Tracking

(Depends on: 4 bugs, Blocks: 2 bugs)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

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

Attachments

(4 attachments, 17 obsolete attachments)

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

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Depends on: 702739
Depends on: 711991
Depends on: 711997
Depends on: 715433
Readback for old-style transparent plugins would make async animation fantastically complicated.  We should probably just disable async animation when there are readback consumers.
Blocks: 713532
Duplicate of this bug: 730387

Updated

5 years ago
Whiteboard: [Snappy:P1]
Created attachment 613055 [details] [diff] [review]
strawman PLayers spec for animation

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

Comment 7

5 years ago
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: 745135
(Reporter)

Updated

5 years ago
Blocks: 749062
Created attachment 622473 [details] [diff] [review]
starter patch
Attachment #613055 - Attachment is obsolete: true
Created attachment 622642 [details] [diff] [review]
skeleton of layers/compositor side
Attachment #622473 - Attachment is obsolete: true
Blocks: 755084
No longer depends on: 598873
Depends on: 598873
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"
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Comment 15

5 years ago
Created attachment 626268 [details] [diff] [review]
Layers+layout wip patch
\o/
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 758984
Blocks: 759252
blocking-basecamp: --- → +
Whiteboard: [Snappy:P1] → [Snappy:P1][soft]
blocking-kilimanjaro: --- → +
(Assignee)

Comment 19

5 years ago
Created attachment 630384 [details] [diff] [review]
Layers patch
Assignee: mrbkap → dzbarsky
Attachment #622642 - Attachment is obsolete: true
Attachment #626268 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #630384 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 711991
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?
(Assignee)

Comment 22

5 years ago
Created attachment 632974 [details] [diff] [review]
Patch

Fixed transforms and a bunch of other bugs.
Attachment #630384 - Attachment is obsolete: true
Attachment #630384 - Flags: review?(roc)
Attachment #632974 - Flags: review?(roc)
(Assignee)

Comment 23

5 years ago
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)
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 26

5 years ago
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.
(Assignee)

Comment 27

5 years ago
Created attachment 633287 [details] [diff] [review]
Patch, cleaned up a little
Attachment #632974 - Attachment is obsolete: true
Attachment #632974 - Flags: review?(roc)
Attachment #632974 - Flags: review?(dbaron)
Attachment #633287 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 29

5 years ago
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.
(Assignee)

Comment 30

5 years ago
Created attachment 634549 [details] [diff] [review]
Patch

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

Updated

5 years ago
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?
(Assignee)

Comment 33

5 years ago
Roc, I just copied the same thing that we did for nsRect.  It would be a little weird to have them be different.
So you did. OK.
(Assignee)

Comment 35

5 years ago
Created attachment 636668 [details] [diff] [review]
Patch
Attachment #636668 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #636668 - Flags: review?(dbaron)
(Assignee)

Updated

5 years ago
Attachment #634549 - Attachment is obsolete: true
Attachment #634549 - Flags: review?(roc)
Attachment #634549 - Flags: review?(dbaron)
(Assignee)

Updated

5 years ago
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?
Comment on attachment 636668 [details] [diff] [review]
Patch

r+ on GetLocalOpacity().
Attachment #636668 - Flags: review+
(Assignee)

Comment 38

5 years ago
(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
(Assignee)

Comment 39

5 years ago
(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<>> ?
(Assignee)

Updated

5 years ago
Depends on: 769266
(Assignee)

Comment 41

5 years ago
(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.
(Assignee)

Comment 43

5 years ago
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.
(Assignee)

Comment 45

5 years ago
Created attachment 640384 [details] [diff] [review]
Layers patch
Attachment #636668 - Attachment is obsolete: true
Attachment #636668 - Flags: review?(roc)
Attachment #636668 - Flags: review?(dbaron)
Attachment #640384 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
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();

{}
(Assignee)

Comment 47

5 years ago
(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.
(Assignee)

Comment 49

5 years ago
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.
(Assignee)

Comment 50

5 years ago
Created attachment 641231 [details] [diff] [review]
Scalingmatrix changes

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

Comment 54

5 years ago
Created attachment 641339 [details] [diff] [review]
Scalingmatrix changes v2
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+
(Assignee)

Comment 56

5 years ago
Created attachment 642822 [details] [diff] [review]
Patch
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)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 58

5 years ago
Created attachment 643042 [details] [diff] [review]
Address comment 55
Attachment #643042 - Flags: review?(roc)
Attachment #643042 - Flags: review?(roc) → review+
(Assignee)

Comment 59

5 years ago
Created attachment 644037 [details] [diff] [review]
Rollup patch of everything
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.
(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+
(Assignee)

Comment 63

5 years ago
(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.
(Assignee)

Comment 64

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f5189b1d0c
Target Milestone: --- → mozilla17
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 → ---
(Assignee)

Comment 66

5 years ago
Created attachment 645584 [details] [diff] [review]
Add BaseTransform to layers
Attachment #645584 - Flags: review?(roc)
(Assignee)

Comment 67

5 years ago
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.
(Assignee)

Comment 70

5 years ago
Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/876726b632c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/32d16d0f87c9
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla17
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
Target Milestone: mozilla17 → ---
(Assignee)

Comment 72

5 years ago
Third time's the charm:
https://hg.mozilla.org/integration/mozilla-inbound/rev/169ff207ed19
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76e0a267f26
https://hg.mozilla.org/mozilla-central/rev/169ff207ed19
https://hg.mozilla.org/mozilla-central/rev/a76e0a267f26
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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 → ---
(Assignee)

Comment 76

5 years ago
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"
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)
(Assignee)

Comment 78

5 years ago
Created attachment 647373 [details] [diff] [review]
Translate the layer after we apply scaling
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+
(Assignee)

Comment 80

5 years ago
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]
(Assignee)

Updated

5 years ago
Attachment #645584 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #647373 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/e64fcdf0c985
(Assignee)

Comment 82

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/9538c86590a4
David, what still needs to land here?
(Assignee)

Comment 85

5 years ago
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.
(Assignee)

Comment 86

5 years ago
Created attachment 648623 [details]
Part 3: Turn on the use of scaling for layers in FrameLayerBuilder
Attachment #648623 - Flags: review?(matt.woodrow)
(Assignee)

Comment 87

5 years ago
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!
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+
(Assignee)

Comment 89

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Depends on: 781097
(Assignee)

Updated

5 years ago
Duplicate of this bug: 796359

Updated

3 years ago
Blocks: 958480

Updated

3 years ago
No longer blocks: 958480
Blocks: 980770
You need to log in before you can comment on or make changes to this bug.