Bug 179973 - [Web Animations] Perform accelerated animations when possible
Summary: [Web Animations] Perform accelerated animations when possible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-23 05:56 PST by Antoine Quint
Modified: 2017-11-24 01:45 PST (History)
2 users (show)

See Also:


Attachments
Patch (22.82 KB, patch)
2017-11-23 07:16 PST, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2017-11-23 05:56:22 PST
Certain layer properties (opacity, transform, filter, etc.) can be animated using hardware compositing when using CSS Animations, but not currently with Web Animations. It should be possible to use the same approach used for CSS Animations with Web Animations and have the same performance gains.
Comment 1 Antoine Quint 2017-11-23 05:56:41 PST
<rdar://problem/34953922>
Comment 2 Antoine Quint 2017-11-23 07:16:02 PST
Created attachment 327498 [details]
Patch
Comment 3 Dean Jackson 2017-11-23 11:20:54 PST
Comment on attachment 327498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327498&action=review

I know you're unsure about the term "hardware compositing" vs "accelerated". I think more people are familiar with the term accelerated, or maybe just "composited".

> Source/WebCore/animation/DocumentTimeline.cpp:170
> +        animation->toggleHardwareCompositing();

I don't like the name "toggle", because it's not clear what is actually happening. Is this turning it on or off? Or does it depend on the animation?

> Source/WebCore/animation/DocumentTimeline.cpp:182
> +bool DocumentTimeline::elementRunningAnimationsAreAllHardwareComposited(Element& element)

This is also a weird name. It's asking if the Element has only hardware composited animations. Maybe elementUsingHardwareCompositingForAnimations?

> Source/WebCore/animation/KeyframeEffect.cpp:150
> +    if (m_startedHardwareCompositedAnimation && localTime >= timing()->duration()) {

I wonder if relativeTime is a better name than localTime. I had to think a bit to work out what localTime >= duration meant (because it makes no sense to compare a general time to a duration).

> Source/WebCore/animation/KeyframeEffect.cpp:184
> +    if (needsToStartWithHardwareCompositing)
> +        animation()->hardwareCompositingActiveStateDidChange();

I assume you have to call this after you flip the Z index? Otherwise it makes more sense to go up where you set needsToStartWithHardwareCompositing.
Comment 4 Antoine Quint 2017-11-23 11:46:19 PST
(In reply to Dean Jackson from comment #3)
> Comment on attachment 327498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327498&action=review
> 
> I know you're unsure about the term "hardware compositing" vs "accelerated".
> I think more people are familiar with the term accelerated, or maybe just
> "composited".

I definitely don't want to use "composited" because of the notion of "effect composition" in Web Animations (https://w3c.github.io/web-animations/#effect-composition). I think accelerated is the way to go. I'll rename everything to use that term instead.

> > Source/WebCore/animation/DocumentTimeline.cpp:170
> > +        animation->toggleHardwareCompositing();
> 
> I don't like the name "toggle", because it's not clear what is actually
> happening. Is this turning it on or off? Or does it depend on the animation?

It turns it on if it's off, and off if it's on, hence the use of "toggle". Specifically, it starts the accelerated animation if it's not running yet, and it ends it if it's already running.

> > Source/WebCore/animation/DocumentTimeline.cpp:182
> > +bool DocumentTimeline::elementRunningAnimationsAreAllHardwareComposited(Element& element)
> 
> This is also a weird name. It's asking if the Element has only hardware
> composited animations. Maybe elementUsingHardwareCompositingForAnimations?

I think areAllRunningAnimationsForElementAccelerated()?

> > Source/WebCore/animation/KeyframeEffect.cpp:150
> > +    if (m_startedHardwareCompositedAnimation && localTime >= timing()->duration()) {
> 
> I wonder if relativeTime is a better name than localTime. I had to think a
> bit to work out what localTime >= duration meant (because it makes no sense
> to compare a general time to a duration).

The term "local time" comes from the spec (https://w3c.github.io/web-animations/#local-time-section).

> > Source/WebCore/animation/KeyframeEffect.cpp:184
> > +    if (needsToStartWithHardwareCompositing)
> > +        animation()->hardwareCompositingActiveStateDidChange();
> 
> I assume you have to call this after you flip the Z index? Otherwise it
> makes more sense to go up where you set needsToStartWithHardwareCompositing.

Actually, we don't. This code was there because the needsToStartWithHardwareCompositing was the return value for a previous iteration of this patch. Now we no longer use a return value, I can set it earlier as suggested.
Comment 5 Antoine Quint 2017-11-24 01:45:37 PST
Committed r225128: <https://trac.webkit.org/changeset/225128>