WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
213175
Scrunching a video to PiP can result in broken animation and leave Safari in a bad state
https://bugs.webkit.org/show_bug.cgi?id=213175
Summary
Scrunching a video to PiP can result in broken animation and leave Safari in ...
Peng Liu
Reported
2020-06-13 20:19:59 PDT
Scrunching a video to PiP can result in broken animation and leave Safari in a bad state
Attachments
WIP patch
(8.44 KB, patch)
2020-06-13 21:08 PDT
,
Peng Liu
no flags
Details
Formatted Diff
Diff
Remove the one second delay in the previous patch
(5.38 KB, patch)
2020-06-18 14:22 PDT
,
Peng Liu
no flags
Details
Formatted Diff
Diff
Don't proceed to enter picture-in-picture if the video is exiting fullscreen
(7.41 KB, patch)
2020-06-25 18:13 PDT
,
Peng Liu
no flags
Details
Formatted Diff
Diff
Fix issues found in stress tests
(8.28 KB, patch)
2020-06-28 21:34 PDT
,
Peng Liu
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Peng Liu
Comment 1
2020-06-13 20:20:44 PDT
<
rdar://problem/61948663
>
Peng Liu
Comment 2
2020-06-13 21:08:06 PDT
Created
attachment 401851
[details]
WIP patch
Darin Adler
Comment 3
2020-06-14 15:17:26 PDT
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
Comment 4
2020-06-18 14:22:28 PDT
Created
attachment 402235
[details]
Remove the one second delay in the previous patch
Peng Liu
Comment 5
2020-06-18 14:42:48 PDT
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
Comment 6
2020-06-18 14:58:50 PDT
(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
Comment 7
2020-06-18 21:46:23 PDT
(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
Comment 8
2020-06-25 18:13:37 PDT
Created
attachment 402837
[details]
Don't proceed to enter picture-in-picture if the video is exiting fullscreen
Peng Liu
Comment 9
2020-06-28 21:34:57 PDT
Created
attachment 403028
[details]
Fix issues found in stress tests
Jer Noble
Comment 10
2020-06-30 10:43:40 PDT
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
Comment 11
2020-06-30 10:53:50 PDT
Committed
r263760
: <
https://trac.webkit.org/changeset/263760
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 403028
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug