Bug 217851 – REGRESSION(r268615): some accelerated transform tests are failing

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

The test transitions/interrupted-transition-hardware.html has regressed after the fix for bug 217842 on WK1.

[Radar WebKit Bug Importer]

<rdar://problem/70394402>

[Antoine Quint]

*** Bug 217852 has been marked as a duplicate of this bug. ***

[Antoine Quint]

Actually, it's a few tests and it's not specific to WK1:

transitions/interrupted-transition-hardware.html
webanimations/accelerated-transform-related-animation-property-order.html
webanimations/accelerated-translate-animation-additional-animation-added-in-flight.html
webanimations/accelerated-translate-animation-underlying-transform-changed-in-flight.html
webanimations/accelerated-translate-animation-with-transform.html
webanimations/accelerated-translate-animation.html

[Antoine Quint]

These failures are specific to Mojave, where CA behaves differently than on Catalina and up.

[Antoine Quint]

Interestingly, the failures stop reproducing if I use slightly smaller values. For these tests, we use a large value for animation duration : 1 day. Using a smaller but still long duration, such as 18 hours, stops reproducing the issue.

[Antoine Quint]

In the failing cases we set a negative time value for the begin time.

[Antoine Quint]

Looking at the bot results, those tests were consistent failures and are now passing fine. I think this has something to do with the value returned by CACurrentMediaTime().

[Antoine Quint]

Right, rebooting a Catalina machine reproduces the issue as well.

[Antoine Quint]

I think this issue is fortuitous and not actually related to the change in r268615. I'm building with a previous revision to determine this.

[Antoine Quint]

Removing the identity transform animation fixes the issue. As soon as we have two animations, having one animation with a start time that is in the past causes this problem.

[Antoine Quint]

Created attachment 411855 [details]
Patch

[Antoine Quint]

Created attachment 411856 [details]
Patch

[Antoine Quint]

Created attachment 411874 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 411874 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2865
> +    auto currentTime = Seconds(CACurrentMediaTime());

Shouldn't this be related to the time value we snapshotted for the current rendering update?

[Dean Jackson]

Comment on attachment 411874 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2865
>> +    auto currentTime = Seconds(CACurrentMediaTime());
> 
> Shouldn't this be related to the time value we snapshotted for the current rendering update?

same question

[Antoine Quint]

This wasn't a Mojave-specific regression.

[Antoine Quint]

Committed r268746: <https://trac.webkit.org/changeset/268746>

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 411874 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411874&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2865
> > +    auto currentTime = Seconds(CACurrentMediaTime());
> 
> Shouldn't this be related to the time value we snapshotted for the current
> rendering update?

It probably should! Note that we only use this for animations that have a timeOffset specified.

I've filed bug 218016 to look into potential syncing issues and the notion of "ready time".