Bug 217851

Summary: REGRESSION(r268615): some accelerated transform tests are failing
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, simon.fraser, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=217976
https://bugs.webkit.org/show_bug.cgi?id=218016
https://bugs.webkit.org/show_bug.cgi?id=220018
Attachments:
Description Flags
Patch
none
Patch
none
Patch dino: review+

Description Antoine Quint 2020-10-16 15:30:05 PDT
The test transitions/interrupted-transition-hardware.html has regressed after the fix for bug 217842 on WK1.
Comment 1 Radar WebKit Bug Importer 2020-10-16 15:30:29 PDT
<rdar://problem/70394402>
Comment 2 Antoine Quint 2020-10-16 15:34:47 PDT
*** Bug 217852 has been marked as a duplicate of this bug. ***
Comment 3 Antoine Quint 2020-10-16 15:44:18 PDT
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
Comment 4 Antoine Quint 2020-10-19 08:14:36 PDT
These failures are specific to Mojave, where CA behaves differently than on Catalina and up.
Comment 5 Antoine Quint 2020-10-20 02:32:58 PDT
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.
Comment 6 Antoine Quint 2020-10-20 02:45:53 PDT
In the failing cases we set a negative time value for the begin time.
Comment 7 Antoine Quint 2020-10-20 02:56:41 PDT
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().
Comment 8 Antoine Quint 2020-10-20 03:10:57 PDT
Right, rebooting a Catalina machine reproduces the issue as well.
Comment 9 Antoine Quint 2020-10-20 03:19:54 PDT
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.
Comment 10 Antoine Quint 2020-10-20 04:06:02 PDT
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.
Comment 11 Antoine Quint 2020-10-20 05:14:10 PDT
Created attachment 411855 [details]
Patch
Comment 12 Antoine Quint 2020-10-20 05:18:44 PDT
Created attachment 411856 [details]
Patch
Comment 13 Antoine Quint 2020-10-20 09:37:24 PDT
Created attachment 411874 [details]
Patch
Comment 14 Simon Fraser (smfr) 2020-10-20 10:22:17 PDT
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?
Comment 15 Dean Jackson 2020-10-20 10:26:44 PDT
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
Comment 16 Antoine Quint 2020-10-20 10:30:46 PDT
This wasn't a Mojave-specific regression.
Comment 17 Antoine Quint 2020-10-20 10:34:46 PDT
Committed r268746: <https://trac.webkit.org/changeset/268746>
Comment 18 Antoine Quint 2020-10-21 05:45:26 PDT
(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".