Scrunching a video to PiP can result in broken animation and leave Safari in a bad state

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

Scrunching a video to PiP can result in broken animation and leave Safari in a bad state

[Peng Liu]

<rdar://problem/61948663>

[Peng Liu]

Created attachment 401851 [details]
WIP patch

[Darin Adler]

Comment on attachment 401851 [details]
WIP patch

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

This calls for a regression test; it’s the kind of thing that’s really hard to get right without a test. I’m not going to say review- because I’m not really an expert on this area, but this doesn’t look quite right to me.

> Source/WebCore/html/HTMLMediaElement.cpp:201
> +static const Seconds setVideoFullscreenStandbyDelay { 1_s };

What’s the rationale for this being one second?

Also, eventually we should change these to be constexpr instead of const.

> Source/WebCore/html/HTMLMediaElement.cpp:6061
> +    // A page may request a video element to exit fullscreen and standby state while the
> +    // underlying media framework is trying to enter Picture-in-Picture from the standby state.
> +    // Therefore, we had better wait a while before exiting standby. If the media framework
> +    // does decide to enter Picture-in-Picture, the video element will be notified and
> +    // m_videoFullscreenMode will be updated.

This 1 second delay sounds like a recipe for race conditions. Like literally fixing a bug my intentionally adding a race. It’s almost always preferable to have a solution that does not rely on a particular duration.

[Peng Liu]

Created attachment 402235 [details]
Remove the one second delay in the previous patch

[Peng Liu]

Comment on attachment 401851 [details]
WIP patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:201
>> +static const Seconds setVideoFullscreenStandbyDelay { 1_s };
> 
> What’s the rationale for this being one second?
> 
> Also, eventually we should change these to be constexpr instead of const.

The UI process side (AVKit) will recognize the scrunching gesture and ask the video to enter picture-in-picture, while WebKit will also recognize the gesture and request the video element to exit fullscreen at the same time. In the current implementation of the element fullscreen API regarding the "standby" state, the web process will also send a request to the UI process to exit "video fullscreen", which might be entering picture-in-picture.

By adding the one-second delay, the web process will be notified by the UI process that the video element is entering picture-in-picture, so the web process won't send a request to confuse the UI process. There is no rationale to decide the length of the delay, but it works in my test.

>> Source/WebCore/html/HTMLMediaElement.cpp:6061
>> +    // m_videoFullscreenMode will be updated.
> 
> This 1 second delay sounds like a recipe for race conditions. Like literally fixing a bug my intentionally adding a race. It’s almost always preferable to have a solution that does not rely on a particular duration.

Agree. Unfortunately, we don't have an easy solution without a timer to completely fix the problem.

I have revised the patch to remove the timer. The new patch will prevent webkit from entering a bad state. However, in response to a scrunching gesture, a video element can enter picture-in-picture first, then close the PiP window automatically in seconds. I will file another bug to track that issue.

[Peng Liu]

(In reply to Darin Adler from comment #3)
> Comment on attachment 401851 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401851&action=review
> 
> This calls for a regression test; it’s the kind of thing that’s really hard
> to get right without a test. I’m not going to say review- because I’m not
> really an expert on this area, but this doesn’t look quite right to me.

Agree. But we are not able to add a test for it before fixing bug 212654.

[Peng Liu]

(In reply to Peng Liu from comment #4)
> Created attachment 402235 [details]
> Remove the one second delay in the previous patch

With this patch, sometimes the video in the PiP window is zoomed in. Looks like the state regarding the video layer is corrupted.

[Peng Liu]

Created attachment 402837 [details]
Don't proceed to enter picture-in-picture if the video is exiting fullscreen

[Peng Liu]

Created attachment 403028 [details]
Fix issues found in stress tests

[Jer Noble]

Comment on attachment 403028 [details]
Fix issues found in stress tests

r=me; I discussed this with Peng, and it's impossible to write a test case for this fix at the moment, because TestWebKitAPI.app isn't a "real" iOS application, and will never successfully enter fullscreen mode. Peng has a TestWebKitAPI patch which _would_ verify the fix, but it would land broken. For now, we'll have to rely on manual testing.

[EWS]

Committed r263760: <https://trac.webkit.org/changeset/263760>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403028 [details].