Bug 226216 - REGRESSION (r272201): iPad render flashing on load
Summary: REGRESSION (r272201): iPad render flashing on load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari 14
Hardware: iPhone / iPad iOS 14
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-25 04:08 PDT by Roland Soos
Modified: 2021-06-07 12:44 PDT (History)
6 users (show)

See Also:


Attachments
Screen recording on iPad (600.42 KB, video/quicktime)
2021-05-25 04:08 PDT, Roland Soos
no flags Details
Flashing text on load (13.63 MB, video/mp4)
2021-05-26 05:18 PDT, Roland Soos
no flags Details
Patch for EWS (4.87 KB, patch)
2021-06-04 14:08 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (8.17 KB, patch)
2021-06-07 10:12 PDT, Antoine Quint
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>