Bug 226216

Summary: REGRESSION (r272201): iPad render flashing on load
Product: WebKit Reporter: Roland Soos <roland>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, dino, graouts, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: iPhone / iPad   
OS: iOS 14   
Attachments:
Description Flags
Screen recording on iPad
none
Flashing text on load
none
Patch for EWS
none
Patch simon.fraser: review+

Description Roland Soos 2021-05-25 04:08:24 PDT
Created attachment 429641 [details]
Screen recording on iPad

Steps to reproduce:
1. Open https://smartslider3.com/bugs/webkit/flashing2/

What's went wrong:
The see through black area and it's content is flashing for a few frame.

I was able to reproduce it only on iPad. IOS version: 14.5.1
Comment 1 Simon Fraser (smfr) 2021-05-25 10:07:17 PDT
Roland, do you know if this occurred in earlier iOS versions?
Comment 2 Roland Soos 2021-05-25 10:21:16 PDT
I don't know, sorry.
Comment 3 Roland Soos 2021-05-25 10:25:51 PDT
Also on this page: https://smartslider3.com/full-page-hotel/
A very similar flashing occurs on load only on iPad. Do you want me to create a static page from it?
Comment 4 Simon Fraser (smfr) 2021-05-25 10:43:37 PDT
The https://smartslider3.com/full-page-hotel/ is a flash caused by async image decoding. In cases like this where you animated a large image on-screen, you need to be using the image.decode() API (https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/decode) to know when the image is ready to display.
Comment 5 Roland Soos 2021-05-25 10:45:27 PDT
These texts are flashing for a moment, not the image in the background: https://i.imgur.com/d9z1ZsC.png
Comment 6 Radar WebKit Bug Importer 2021-05-25 11:06:58 PDT
<rdar://problem/78466306>
Comment 7 Roland Soos 2021-05-26 05:18:30 PDT
Created attachment 429750 [details]
Flashing text on load
Comment 8 Simon Fraser (smfr) 2021-05-26 12:10:29 PDT
I think we're changing from a tiled layer to a non-tiled layer, and fail to move the animations to the new layer.
Comment 9 Simon Fraser (smfr) 2021-05-26 16:37:30 PDT
Antoine, I think GraphicsLayerCA::moveOrCopyLayerAnimation() is broken with animation groups.

PlatformCALayers have animation groups on them:

    (layer 29
      (animation group-c0316d87-a66f-4fd0-9c2a-b1d002514449_5_0_0 type=group keyPath=
        (beginTime 1.00)

but GraphicsLayerCA::moveOrCopyLayerAnimation() only deals with animationForKey(animationIdentifier)
Comment 10 Antoine Quint 2021-05-27 00:30:09 PDT
Cool, thanks for the analysis, hopefully that's an easy fix.
Comment 11 Antoine Quint 2021-06-04 09:40:28 PDT
On iPhone this reproduces as well.
Comment 12 Antoine Quint 2021-06-04 10:57:53 PDT
While the issue appears differently between iPhone and iPad, the regression point is the same.
Comment 13 Antoine Quint 2021-06-04 11:01:15 PDT
I think I have done the required changes to GraphcisLayerCA::moveOrCopyAnimations() but the bug reproduces still. I need to check whether Core Animation allows setting a group animation on different layers while holding the children animations.
Comment 14 Antoine Quint 2021-06-04 13:58:36 PDT
Found the issue: Core Animation seems to compute the begin time when calling animationForKey(), so we need to re-set it to the original value when setting the animation anew on the new layer.

The tricky bit now is to write a test.
Comment 15 Antoine Quint 2021-06-04 14:08:22 PDT
Created attachment 430610 [details]
Patch for EWS
Comment 16 Antoine Quint 2021-06-07 10:12:05 PDT
Created attachment 430759 [details]
Patch
Comment 17 Simon Fraser (smfr) 2021-06-07 11:59:11 PDT
Comment on attachment 430759 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:714
> +        if ((animatedPropertyIsTransformOrRelated(animationGroup.m_property)
> +            || animationGroup.m_property == AnimatedPropertyOpacity
> +            || animationGroup.m_property == AnimatedPropertyBackgroundColor
> +            || animationGroup.m_property == AnimatedPropertyFilter))

I wonder why we check the types here. Can't we just move all animations?
Comment 18 Antoine Quint 2021-06-07 12:26:50 PDT
(In reply to Simon Fraser (smfr) from comment #17)
> Comment on attachment 430759 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430759&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:714
> > +        if ((animatedPropertyIsTransformOrRelated(animationGroup.m_property)
> > +            || animationGroup.m_property == AnimatedPropertyOpacity
> > +            || animationGroup.m_property == AnimatedPropertyBackgroundColor
> > +            || animationGroup.m_property == AnimatedPropertyFilter))
> 
> I wonder why we check the types here. Can't we just move all animations?

I think it was to isolate the AnimatedPropertyBackgroundColor and AnimatedPropertyWebkitBackdropFilter animations which apply to something other than the primary layer.
Comment 19 Antoine Quint 2021-06-07 12:44:20 PDT
Committed r278566 (238564@main): <https://commits.webkit.org/238564@main>