Bug 202615 – [Picture-in-Picture Web API] Implement PictureInPictureWindow

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

Implement the PictureInPictureWindow related interface and events.

[Radar WebKit Bug Importer]

<rdar://problem/56267934>

[Peng Liu]

Created attachment 381878 [details]
Patch

WIP

[Peng Liu]

Created attachment 381910 [details]
Patch

[Eric Carlson]

Comment on attachment 381910 [details]
Patch

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

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:104
> +        videoElement.scheduleEvent(EnterPictureInPictureEvent::create(eventNames().enterpictureinpictureEvent, WTFMove(initializer)));

Why fire an event? The spec says to abort the Picture-in-Picture algorithm at step 7 if requestPictureInPicture is called on the current PiP element, and the event is fired at step 11:

When the request Picture-in-Picture algorithm with video, userActivationRequired and playingRequired is invoked, the user agent MUST run the following steps:
...
7. If video is pictureInPictureElement, abort these steps.
...
11. Queue a task to fire an event with the name enterpictureinpicture using EnterPictureInPictureEvent at the video with its bubbles attribute initialized to true and its pictureInPictureWindow attribute initialized to Picture-in-Picture window.

https://w3c.github.io/picture-in-picture/#request-picture-in-picture-algorithm

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:173
>      m_videoElement.document().setPictureInPictureElement(nullptr);
> +    m_pictureInPictureWindow->setSize({0, 0});

The spec says to run the close window algorithm before clearing pictureInPictureElement. The close window algorithm says to fire a resize event at pictureInPictureElement if the size changes, so you should call pictureInPictureWindowResized instead of setting the size directly.

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:187
> +    m_pictureInPictureWindow->setSize(windowSize);
> +    auto resizeEvent = Event::create(eventNames().resizeEvent, Event::CanBubble::Yes, Event::IsCancelable::No);
> +    resizeEvent->setTarget(m_pictureInPictureWindow);
> +    m_videoElement.scheduleEvent(WTFMove(resizeEvent));

The event should only fire when the window size changes.

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:46
> +    , m_width(0)
> +    , m_height(0)

Nit: why not use an IntSize for the member variable as well?

> Source/WebCore/html/HTMLVideoElement.cpp:532
> +    // fullscreenMode() does not always provide the correct fullscreen mode
> +    // when mode changing is happening

It seems that it would be better to fix that problem instead of working around it here so we don't trip over this again.

[Sam Weinig]

Comment on attachment 381910 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        With manual test, the implementation passes
> +        - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html

What is holding up writing an automated test for this?

[Peng Liu]

Comment on attachment 381910 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html
> 
> What is holding up writing an automated test for this?

https://bugs.webkit.org/show_bug.cgi?id=202617
Current WebkitTestRunner does not support wpt/resources/testdriver.js:test_driver.bless() properly, so the operations requiring user gesture will not work properly in the test cases.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:104
>> +        videoElement.scheduleEvent(EnterPictureInPictureEvent::create(eventNames().enterpictureinpictureEvent, WTFMove(initializer)));
> 
> Why fire an event? The spec says to abort the Picture-in-Picture algorithm at step 7 if requestPictureInPicture is called on the current PiP element, and the event is fired at step 11:
> 
> When the request Picture-in-Picture algorithm with video, userActivationRequired and playingRequired is invoked, the user agent MUST run the following steps:
> ...
> 7. If video is pictureInPictureElement, abort these steps.
> ...
> 11. Queue a task to fire an event with the name enterpictureinpicture using EnterPictureInPictureEvent at the video with its bubbles attribute initialized to true and its pictureInPictureWindow attribute initialized to Picture-in-Picture window.
> 
> https://w3c.github.io/picture-in-picture/#request-picture-in-picture-algorithm

Good catch. Yes, we should not fire an event here. But we do need to resolve the promise with the current picture-in-picture window here.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:173
>> +    m_pictureInPictureWindow->setSize({0, 0});
> 
> The spec says to run the close window algorithm before clearing pictureInPictureElement. The close window algorithm says to fire a resize event at pictureInPictureElement if the size changes, so you should call pictureInPictureWindowResized instead of setting the size directly.

I use the setSize() function to set the width and height to 0, which indicates the picture-in-picture window is in the closed state. Defining a close() function to do that seems to be a better idea.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:187
>> +    m_videoElement.scheduleEvent(WTFMove(resizeEvent));
> 
> The event should only fire when the window size changes.

Right, this implementation does the same as you describe.

>> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:46
>> +    , m_height(0)
> 
> Nit: why not use an IntSize for the member variable as well?

Yes, we can do that. Will change the implementation.

>> Source/WebCore/html/HTMLVideoElement.cpp:532
>> +    // when mode changing is happening
> 
> It seems that it would be better to fix that problem instead of working around it here so we don't trip over this again.

Agree. But that will be a big patch. Changing the presentation mode is a multiple-step operation and more than one process is involved. The current implementation does not handle the state synchronization very well. I think reporting a new bug to track the fix will be a better idea.

[Peng Liu]

Comment on attachment 381910 [details]
Patch

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

>>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:187
>>> +    m_videoElement.scheduleEvent(WTFMove(resizeEvent));
>> 
>> The event should only fire when the window size changes.
> 
> Right, this implementation does the same as you describe.

Oh, got your point. You mean we need to check the original window size and only fire event if the window size is changed. Right? Actually function pictureInPictureWindowResized() will be called only if the window size IS changed, but we definitely can check the size in this function.

[Peng Liu]

Created attachment 381986 [details]
Patch

Update the patch based on comments from Eric

[Eric Carlson]

Comment on attachment 381986 [details]
Patch

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

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:101
> +        auto pictureInPictureWindow = videoElementPictureInPicture->m_pictureInPictureWindow;
> +        promise->resolve<IDLInterface<PictureInPictureWindow>>(*pictureInPictureWindow);

Nit: I'm not sure the local variable adds anything.

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:169
>      INFO_LOG(LOGIDENTIFIER);
> +    m_pictureInPictureWindow->close();
>      m_videoElement.document().setPictureInPictureElement(nullptr);

Step 2 of the "exit Picture-in-Picture algorithm" is "run the close window algorithm", and the "close window algorithm" says you must:

    ... set the window state is set to closed.
    ... set the width attribute to 0.
    ... set the height attribute to 0.
    ... if the size of the Picture-in-Picture window changes, queue a task to fire a 'resize' event

So if this is called when the window has not already been set to { 0, 0 }, you need to fire a 'resize' event.

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:58
> +    m_size.setWidth(0);
> +    m_size.setHeight(0);

Nit: I think you can simplify this to something like "m_size = { 0, 0 }"

> Source/WebCore/html/HTMLVideoElement.cpp:532
> +    // fullscreenMode() does not always provide the correct fullscreen mode
> +    // when mode changing is happening

Nit: it would be good to have a bug filed for the real fix for this problem so we don't lose track of it. Please include the bug number here.

[Peng Liu]

Comment on attachment 381986 [details]
Patch

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

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:101
>> +        promise->resolve<IDLInterface<PictureInPictureWindow>>(*pictureInPictureWindow);
> 
> Nit: I'm not sure the local variable adds anything.

Right, will fix it.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:169
>>      m_videoElement.document().setPictureInPictureElement(nullptr);
> 
> Step 2 of the "exit Picture-in-Picture algorithm" is "run the close window algorithm", and the "close window algorithm" says you must:
> 
>     ... set the window state is set to closed.
>     ... set the width attribute to 0.
>     ... set the height attribute to 0.
>     ... if the size of the Picture-in-Picture window changes, queue a task to fire a 'resize' event
> 
> So if this is called when the window has not already been set to { 0, 0 }, you need to fire a 'resize' event.

Hmm, you mean we need to fire a resize event with the window size to be {0, 0}? That sounds a little weird because a "leavepictureinpicture" event will be fired as well. I am not convinced that the spec means that behavior. If it does, we may need to report a bug to it. What do you think?

>> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:58
>> +    m_size.setHeight(0);
> 
> Nit: I think you can simplify this to something like "m_size = { 0, 0 }"

Right, will change the patch.

>> Source/WebCore/html/HTMLVideoElement.cpp:532
>> +    // when mode changing is happening
> 
> Nit: it would be good to have a bug filed for the real fix for this problem so we don't lose track of it. Please include the bug number here.

https://bugs.webkit.org/show_bug.cgi?id=203443

[Sam Weinig]

(In reply to Peng Liu from comment #6)
> Comment on attachment 381910 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381910&action=review
> 
> >> Source/WebCore/ChangeLog:9
> >> +        - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html
> > 
> > What is holding up writing an automated test for this?
> 
> https://bugs.webkit.org/show_bug.cgi?id=202617
> Current WebkitTestRunner does not support
> wpt/resources/testdriver.js:test_driver.bless() properly, so the operations
> requiring user gesture will not work properly in the test cases.

That seems to be about support WPT test. Can you write a regression test, even an API test, for this in the mean time?

[Peng Liu]

Comment on attachment 381910 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:9
>>>> +        - LayoutTests/imported/w3c/web-platform-tests/picture-in-picture/picture-in-picture-window.html
>>> 
>>> What is holding up writing an automated test for this?
>> 
>> https://bugs.webkit.org/show_bug.cgi?id=202617
>> Current WebkitTestRunner does not support wpt/resources/testdriver.js:test_driver.bless() properly, so the operations requiring user gesture will not work properly in the test cases.
> 
> That seems to be about support WPT test. Can you write a regression test, even an API test, for this in the mean time?

Sure, I will add some layout test cases for it.

[Peng Liu]

Created attachment 382101 [details]
Patch

[Peng Liu]

Created attachment 382125 [details]
Patch

Update the patch to fix layout test failure

[Peng Liu]

Created attachment 382134 [details]
Patch

The added layout test cases are only relevant on mac and iPad, they are enabled only for mac in this patch.

[Peng Liu]

Created attachment 382146 [details]
Patch

Rebasing the patch

[EWS Watchlist]

Comment on attachment 382146 [details]
Patch

Attachment 382146 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13187024

New failing tests:
http/tests/misc/repeat-open-cancel.html

[EWS Watchlist]

Created attachment 382152 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Peng Liu]

Created attachment 382156 [details]
Patch

The layout test cases are enabled for mac-wk2 only

[Eric Carlson]

Comment on attachment 382156 [details]
Patch

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

This is much better and really close, but I think the test should be filled out a bit.

r=me with a more complete test.

> Source/WebCore/html/HTMLVideoElement.cpp:532
> +    // fullscreenMode() does not always provide the correct fullscreen mode
> +    // when mode changing is happening

Nit: this should reference the bug you filed about fixing this.

> LayoutTests/media/picture-in-picture-api-pip-events.html:19
> +            run('video.src = findMediaFile("video", "content/test")');
> +            await waitFor(video, 'canplaythrough');
> +
> +            run('video.play()');
> +            await waitFor(video, 'playing');
> +
> +            runWithKeyDown(function() { video.requestPictureInPicture(); });
> +            await waitFor(video, 'enterpictureinpicture');

This is fine as far as it goes, but you should test the other steps in the request Picture-in-Picture algorithm: throw if the state is HAVE_NOTHING, throw if there is no video track, throw if not triggered by a user gesture, check that document.pictureInPictureElement is set correctly, check that the 'enterpictureinpicture' event's pictureInPictureWindow attribute is set.

> LayoutTests/media/picture-in-picture-api-pip-events.html:22
> +            runWithKeyDown(function() { document.exitPictureInPicture(); });
> +            await waitFor(video, 'leavepictureinpicture');

This should check everything mandated by the exit Picture-in-Picture algorithm.

[Peng Liu]

Comment on attachment 382156 [details]
Patch

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

>> Source/WebCore/html/HTMLVideoElement.cpp:532
>> +    // when mode changing is happening
> 
> Nit: this should reference the bug you filed about fixing this.

OK, will do that.

>> LayoutTests/media/picture-in-picture-api-pip-events.html:19
>> +            await waitFor(video, 'enterpictureinpicture');
> 
> This is fine as far as it goes, but you should test the other steps in the request Picture-in-Picture algorithm: throw if the state is HAVE_NOTHING, throw if there is no video track, throw if not triggered by a user gesture, check that document.pictureInPictureElement is set correctly, check that the 'enterpictureinpicture' event's pictureInPictureWindow attribute is set.

Will create a new test page for all those cases.

>> LayoutTests/media/picture-in-picture-api-pip-events.html:22
>> +            await waitFor(video, 'leavepictureinpicture');
> 
> This should check everything mandated by the exit Picture-in-Picture algorithm.

OK, ditto.

[Peng Liu]

Created attachment 382201 [details]
Patch

Upate the patch based on Eric's comments and add a bunch of layout test cases.

[Peng Liu]

Created attachment 382221 [details]
Updated patch for landing

Update LayoutTests/media/picture-in-picture-api-enter-pip-2.html to fix a potential race condition (Caught by Eric).

[WebKit Commit Bot]

Comment on attachment 382221 [details]
Updated patch for landing

Rejecting attachment 382221 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 382221, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13189372

[WebKit Commit Bot]

Comment on attachment 382221 [details]
Updated patch for landing

Clearing flags on attachment: 382221

Committed r251745: <https://trac.webkit.org/changeset/251745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

4 of the new tests added in https://trac.webkit.org/changeset/251745/webkit

are flakey timeouts:
media/picture-in-picture-api-enter-pip-4.html
media/picture-in-picture-api-exit-pip-1.html
media/picture-in-picture-api-pip-events.html
media/picture-in-picture-api-pip-window.html

The other tests may also be flakey timeouts but have not done that yet.

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=media%2Fpicture-in-picture-api-enter-pip-2.html&test=media%2Fpicture-in-picture-api-enter-pip-3.html&test=media%2Fpicture-in-picture-api-enter-pip-4.html&test=media%2Fpicture-in-picture-api-exit-pip-1.html&test=media%2Fpicture-in-picture-api-exit-pip-2.html&test=media%2Fpicture-in-picture-api-pip-events.html&test=media%2Fpicture-in-picture-api-pip-window.html

Can this be looked at soon?

[Peng Liu]

Sure, looking into them now. Thanks for the reminder.

[Peng Liu]

(In reply to Truitt Savell from comment #27)
> 4 of the new tests added in https://trac.webkit.org/changeset/251745/webkit
> 
> are flakey timeouts:
> media/picture-in-picture-api-enter-pip-4.html
> media/picture-in-picture-api-exit-pip-1.html
> media/picture-in-picture-api-pip-events.html
> media/picture-in-picture-api-pip-window.html
> 
> The other tests may also be flakey timeouts but have not done that yet.
> 
> History:
> https://results.webkit.org/?suite=layout-tests&suite=layout-
> tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-
> tests&suite=layout-tests&test=media%2Fpicture-in-picture-api-enter-pip-2.
> html&test=media%2Fpicture-in-picture-api-enter-pip-3.
> html&test=media%2Fpicture-in-picture-api-enter-pip-4.
> html&test=media%2Fpicture-in-picture-api-exit-pip-1.
> html&test=media%2Fpicture-in-picture-api-exit-pip-2.
> html&test=media%2Fpicture-in-picture-api-pip-events.
> html&test=media%2Fpicture-in-picture-api-pip-window.html
> 
> Can this be looked at soon?

I can reproduce the timeout locally by running multiple cases simultaneously. A bug is reported for it.
https://bugs.webkit.org/show_bug.cgi?id=203614

[Peng Liu]

(In reply to Truitt Savell from comment #27)

> 
> The other tests may also be flakey timeouts but have not done that yet.
> 

The other test cases will be OK because they cover the corner cases in which PiP mode will not be activated.

[Peng Liu]

Created attachment 382329 [details]
Mark four layout test cases as flakey timeout

[Peng Liu]

Reopening to fix layout tests flakey timeout.

[Jer Noble]

Comment on attachment 382329 [details]
Mark four layout test cases as flakey timeout

Clearing flags on attachment: 382329

Committed r251797: <https://trac.webkit.org/changeset/251797>

[Jer Noble]

All reviewed patches have been landed.  Closing bug.