Closed Bug 1425837 Opened 2 years ago Closed 8 months ago

Support animation on the compositor for individual transform, including animation test cases

Categories

(Core :: DOM: Animation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: u459114, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 4 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
No description provided.
Priority: -- → P3
Summary: supporting animation on the compositor for individual transform → Support animation on the compositor for individual transform, including animation test cases
Duplicate of this bug: 1424985
Attached patch Update LayerAnimationInfo (obsolete) — Splinter Review
Assignee: nobody → cku
Assignee: cku → nobody
Moving this to Core::Animation.  This should depend on bug 1479173.
Component: Layout → DOM: Animation
Depends on: 1479173
Assignee: nobody → boris.chiou
I did innocently use nsCSSPropertyIDSet::CompositorAnimatableCount() for LayerAnimationInfo::sDisplayItemTypes, but it should be the number of |nsCSSPropertyIDSet::CompositorAnimatableCount() - the number of individual transform properties|.
Status: NEW → ASSIGNED
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> I did innocently use nsCSSPropertyIDSet::CompositorAnimatableCount() for
> LayerAnimationInfo::sDisplayItemTypes, but it should be the number of
> |nsCSSPropertyIDSet::CompositorAnimatableCount() - the number of individual
> transform properties|.

Oh, yes. Because all of them use DisplayItemType::Transform. Thanks for this reminder. :)
(In reply to Boris Chiou [:boris]  (UTC-7) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > I did innocently use nsCSSPropertyIDSet::CompositorAnimatableCount() for
> > LayerAnimationInfo::sDisplayItemTypes, but it should be the number of
> > |nsCSSPropertyIDSet::CompositorAnimatableCount() - the number of individual
> > transform properties|.
> 
> Oh, yes. Because all of them use DisplayItemType::Transform. Thanks for this
> reminder. :)

And this also applies to motion-path.
Attachment #8937916 - Attachment is obsolete: true
It seems we "hardcode" `eCSSProperty_transform` in a lot of functions, so even though I add new properties in the compositor animation list, we cannot set AnimationProperty::mRunningOnCompositor=true for the properties which also use nsDisplayTransfrom. :(

We have to make nsDisplayTransform more general, for all the transformation properties (i.e. transform function list, individual transforms, and motion-path).
Depends on: 1526847
Depends on: 1526850
Blocks: 1526847
No longer depends on: 1526847
The major implementation. FIXME: Add comments.

LayerAnimationInfo knows the transform-like properties we support on
the compositor thread, so we should use it in HasTransformAnimation().

Depends on D19634

These functions set frame bits or update change hint for layer
animations on transform-like properties, so we should also take
other transform-like properties into account.

Attachment #9046790 - Attachment description: Bug 1425837 - Part 5: Check all transform-like properties in CalculateCumulativeChangeHint and MaybeUpdateFrameForCompositor. → Bug 1425837 - Part 5: Check all transform-like properties in CalculateCumulativeChangeHint.

Sometimes, we want to set the performance warning by a property set, so
add a new function for it.

Attachment #9046061 - Attachment is obsolete: true

It seems this function uses some FFIs to generate the AnimationValue,
We could move it into AnimationValue class, although what we really need is
RefPtr<ServoAnimationValue>.

Maybe we should use AnimationValue everywhere, instead of the Servo type.
This could be done by other patches.

Both layers and web-render use this function, so we factor it out.

Depends on D22562

I separate two parts while implementing compositor animations for
individual transforms. The first part is on the sender side of transactions.
Basically, we have to convert the individual transforms to the proper
types in layers::Animations. So this patch is focus on SetAnimatable and
the definition in LayersMessages.

Depends on D22563

This is the 2nd part of the implementation. We focus on the compositor side
(i.e. received side of transactions). Basically, we convert the list of
layers::Animation into a list of SampleAnimationData, which is an
intermediate value. And then use this list to do interpolation for each
property in SampleAnimationForEachNode, which will return a list of
RefPtr<RawServoAnimationValue>.

Depends on D22564

Attachment #9046789 - Attachment description: Bug 1425837 - Part 4: Check all transform-like properties in AnimationInfo::HasTransformAnimation. → Bug 1425837 - Part 4: AnimationInfo::HasTransformAnimation should check other OMTA transform-like properties

Now, its time to let individual transform run on the compositor thread.

Depends on D19636

Drop the hack which prevents individual transform running on the
compositor thread.

Depends on D22566

This also adds the missing preference in test_transition_per_property.html.

Depends on D22567

Attachment #9046790 - Attachment description: Bug 1425837 - Part 5: Check all transform-like properties in CalculateCumulativeChangeHint. → Bug 1425837 - Part 8: Throttle transform-like properties animations without 0% or 100% keyframe.
Attachment #9046822 - Attachment description: Bug 1425837 - Add one more function to set performance warning by a property set. → Bug 1425837 - Part 9: Set performance warning by a property set.
Blocks: 1533594

Comment on attachment 9046822 [details]
Bug 1425837 - Part 9: Set performance warning by a property set.

Revision D19633 was moved to bug 1533594. Setting attachment 9046822 [details] to obsolete.

Attachment #9046822 - Attachment is obsolete: true

This is the pre-patch for the next patch (i.e. Part 3-3).
The original implementation about "setting animations" is a little bit hard
to read. In SetAnimations(), we create a new intermediate data,
AnimData, and we mutate the original animations. And then iterate this
mutated animations & intermediate data for sampling. In this bug, we are
planning to group the AnimData as a useful data structure for supporting
multiple properties transform-like animations, so it seems the structure
of original animations may be hard to use after that. Therefore,
we decide to do some reworks on this:

First, we do renames,

  1. InfalliableTArray to nsTArray. (They are the same.)
  2. AnimData to PropertyAnimation.
  3. SetAnimations() to ExtractAnimations(), which returns
    nsTArray<PropertyAnimationGroup>. Each entry in the array is for one
    property. In this patch, there is only one entry. We will extend this
    to multiple entries in the next patch.

And then rework ExtractAnimations(), which stores all the necessary data
in PropertyAnimationGroup. For WR, we store this in
CompositorAnimationStorage. For non-WR, we store it in AnimationInfo.
So we can just use this organized data structure for supporting multiple
properties animations. (See the next patch.)

Depends on D22564

Attachment #9049287 - Attachment description: Bug 1425837 - Part 3-2: Implement compositor animations on translate/rotate/scale on the compositor side. → Bug 1425837 - Part 3-3: Implement compositor animations on translate/rotate/scale on the compositor side.
Attachment #9050161 - Attachment description: Bug 1425837 - Part 3-2: Don't store AnimationArray on the compositor side. → Bug 1425837 - Part 3: Don't store AnimationArray (a.k.a. AnimationInfo::mAnimations) on the compositor thread.
Attachment #9049287 - Attachment description: Bug 1425837 - Part 3-3: Implement compositor animations on translate/rotate/scale on the compositor side. → Bug 1425837 - Part 4: Implement compositor animations on translate/rotate/scale.
Attachment #9046789 - Attachment description: Bug 1425837 - Part 4: AnimationInfo::HasTransformAnimation should check other OMTA transform-like properties → Bug 1425837 - Part 5: AnimationInfo::HasTransformAnimation should check other OMTA transform-like properties
Attachment #9049286 - Attachment is obsolete: true
Attachment #9049288 - Attachment description: Bug 1425837 - Part 5: Add individual transform properties into compositor animation list. → Bug 1425837 - Part 6: Add individual transform properties into compositor animation list.
Attachment #9049289 - Attachment description: Bug 1425837 - Part 6: Drop the hard-code disabling compositor animations on individual transforms. → Bug 1425837 - Part 7: Drop the hard-code disabling compositor animations on individual transforms.
Attachment #9049290 - Attachment description: Bug 1425837 - Part 7: Test that individual transforms run on the compositor thread. → Bug 1425837 - Part 8: Test that individual transforms run on the compositor thread.
Attachment #9046790 - Attachment description: Bug 1425837 - Part 8: Throttle transform-like properties animations without 0% or 100% keyframe. → Bug 1425837 - Part 9: Throttle transform-like properties animations without 0% or 100% keyframe.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3a636faee85571c0c96599b96b297bfb7f8b35a

Will land these patches after the soft freeze.

Thanks for your reviews, Brian and Hiro. :)

Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07a1ab95598e
Part 1: Move ToAnimationValue into AnimationValue. r=hiro
https://hg.mozilla.org/integration/autoland/rev/5d416b07c9b9
Part 2: Factor out the conversion from ServoAnimationValue into Matrix4x4. r=hiro
https://hg.mozilla.org/integration/autoland/rev/6c55b82deb9b
Part 3: Don't store AnimationArray (a.k.a. AnimationInfo::mAnimations) on the compositor thread. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b27d18cccc62
Part 4: Implement compositor animations on translate/rotate/scale. r=hiro,birtles
https://hg.mozilla.org/integration/autoland/rev/067a9dac440a
Part 5: AnimationInfo::HasTransformAnimation should check other OMTA transform-like properties r=hiro
https://hg.mozilla.org/integration/autoland/rev/066e7dffe044
Part 6: Add individual transform properties into compositor animation list. r=hiro
https://hg.mozilla.org/integration/autoland/rev/af39dc228d19
Part 7: Drop the hard-code disabling compositor animations on individual transforms. r=hiro,birtles
https://hg.mozilla.org/integration/autoland/rev/9fe743aa6c94
Part 8: Test that individual transforms run on the compositor thread. r=hiro,birtles
https://hg.mozilla.org/integration/autoland/rev/08126c1f53fe
Part 9: Throttle transform-like properties animations without 0% or 100% keyframe. r=hiro
See Also: → 1536392
Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/487ede41ade2
Drop the redundant animation debug log. r=me
Regressions: 1555548
You need to log in before you can comment on or make changes to this bug.