Bug 201024 - [Picture-in-Picture Web API] Implement HTMLVideoElement.requestPictureInPicture() / Document.exitPictureInPicture()
Summary: [Picture-in-Picture Web API] Implement HTMLVideoElement.requestPictureInPictu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks: 182688 202774
  Show dependency treegraph
 
Reported: 2019-08-21 20:28 PDT by Carlos Eduardo Ramalho
Modified: 2019-10-15 16:38 PDT (History)
30 users (show)

See Also:


Attachments
WIP (180.98 KB, patch)
2019-08-21 20:35 PDT, Carlos Eduardo Ramalho
no flags Details | Formatted Diff | Diff
WIP (139.00 KB, patch)
2019-08-21 20:53 PDT, Carlos Eduardo Ramalho
no flags Details | Formatted Diff | Diff
WIP (143.11 KB, patch)
2019-08-22 14:41 PDT, Carlos Eduardo Ramalho
no flags Details | Formatted Diff | Diff
Patch (162.95 KB, patch)
2019-10-05 22:04 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Update the patch to fix several layout test case failures (163.02 KB, patch)
2019-10-06 11:51 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
An updated patch to fix iOS build (159.22 KB, patch)
2019-10-06 13:57 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
An updated patch to fix gtk build failure (159.15 KB, patch)
2019-10-06 15:31 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (171.21 KB, patch)
2019-10-08 14:23 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (169.76 KB, patch)
2019-10-09 16:30 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (167.39 KB, patch)
2019-10-11 15:20 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (168.42 KB, patch)
2019-10-14 11:31 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (168.93 KB, patch)
2019-10-14 18:54 PDT, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (169.34 KB, patch)
2019-10-15 13:54 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Eduardo Ramalho 2019-08-21 20:28:58 PDT
This should be the first part of the API: the ability to enter and exit Picture-in-Picture.

Implementation can reuse code of webkitSetPresentationMode().
Comment 1 Carlos Eduardo Ramalho 2019-08-21 20:35:39 PDT
Created attachment 376973 [details]
WIP

WIP patch:
- Implements HTMLVideoElement.requestPictureInPicture()
- Imports WPT tests
- Adds compile flag to XCode (TODO CMake) and runtime flag

Need to implement document.exitPictureInPicture() too for the patch to be self-contained, I think. Should be the minimal for the demo at https://googlechrome.github.io/samples/picture-in-picture/ to work minimally.

Will split this patch into smaller ones for the flags first and need to skip PIP tests too.

However, I accept comments on the requestPictureInPicture() bits.
- Is it OK to use Supplement/Supplementable or it preferable add to HTMLVideoElement directly?
Comment 2 Carlos Eduardo Ramalho 2019-08-21 20:44:32 PDT
(In reply to Carlos Eduardo Ramalho from comment #1)
> Created attachment 376973 [details]
> WIP

Looks like it needs some rebasing. Will do.
Comment 3 Carlos Eduardo Ramalho 2019-08-21 20:53:58 PDT
Created attachment 376975 [details]
WIP

Rebased WIP patch.
Comment 4 Carlos Eduardo Ramalho 2019-08-22 09:24:43 PDT
The UserGestureIndicator::processingUserGesture() check returns false if the call was done via the Web Inspector. Is there any way to enable accepting calls to requestPictureInPicture() in the web inspector too, other than just removing this check?

I was thinking of not enabling web-sites to call this by themselves without user activation, but let developers call it on the web inspector. I do it all the time when I can't reach the context menu for Picture-in-Picture easily.
Comment 5 youenn fablet 2019-08-22 09:45:30 PDT
(In reply to Carlos Eduardo Ramalho from comment #4)
> The UserGestureIndicator::processingUserGesture() check returns false if the
> call was done via the Web Inspector. Is there any way to enable accepting
> calls to requestPictureInPicture() in the web inspector too, other than just
> removing this check?

In Web Inspector console tab, you can select "Emulate User Gesture" for that purpose.
Comment 6 Carlos Eduardo Ramalho 2019-08-22 09:52:53 PDT
(In reply to youenn fablet from comment #5)
> (In reply to Carlos Eduardo Ramalho from comment #4)
> > The UserGestureIndicator::processingUserGesture() check returns false if the
> > call was done via the Web Inspector. Is there any way to enable accepting
> > calls to requestPictureInPicture() in the web inspector too, other than just
> > removing this check?
> 
> In Web Inspector console tab, you can select "Emulate User Gesture" for that
> purpose.

Great! Hadn't noticed that. Thanks :)
Comment 7 Carlos Eduardo Ramalho 2019-08-22 14:41:20 PDT
Created attachment 377047 [details]
WIP

- Implements Document.exitPictureInPicture()
- Adds Logging utilities

As a next step I should probably refactor this messy code into a central "controller" or "manager" class, owned by Document, which gathers the spec algorithms.

The current code is breaking inheritance and has no clearly defined lifetimes.

Nevertheless, it should be able to enter/exit PIP using the API now.

Running the WPT tests with https://w3c-test.org/tools/runner/index.html gives:

Passed:   8
Failed:   7
Timeouts: 8
Errors:   0
Not Run:  2

So, some progress.
Comment 8 Carlos Eduardo Ramalho 2019-08-22 14:42:45 PDT
(In reply to Carlos Eduardo Ramalho from comment #7)
> Running the WPT tests with https://w3c-test.org/tools/runner/index.html
> gives:

The query: /picture-in-picture
Comment 9 Peng Liu 2019-09-16 15:48:19 PDT
Hi Carlos, any update on the progress? Thanks!
Comment 10 Carlos Eduardo Ramalho 2019-09-17 07:15:11 PDT
(In reply to Peng Liu from comment #9)
> Hi Carlos, any update on the progress? Thanks!

Hi Peng Liu! Progress was halted last weeks due to other obligations.

I shall get back to it this week and send a reviewable patch.
Comment 11 Peng Liu 2019-10-05 22:04:10 PDT
Created attachment 380292 [details]
Patch
Comment 12 Peng Liu 2019-10-05 22:14:03 PDT
Comment on attachment 380292 [details]
Patch

Majority work of this patch was done by Carlos Eduardo Ramalho <cadubentzen@gmail.com>.

Currently, enter-picture-in-picture.html and exit-picture-in-picture.html pass if we open them in the browser or with https://w3c-test.org/tools/runner/index.html.
Comment 13 Peng Liu 2019-10-06 11:51:08 PDT
Created attachment 380301 [details]
Update the patch to fix several layout test case failures
Comment 14 Peng Liu 2019-10-06 13:57:57 PDT
Created attachment 380303 [details]
An updated patch to fix iOS build
Comment 15 Peng Liu 2019-10-06 15:31:47 PDT
Created attachment 380304 [details]
An updated patch to fix gtk build failure
Comment 16 Peng Liu 2019-10-08 14:23:14 PDT
Created attachment 380464 [details]
Patch
Comment 17 Eric Carlson 2019-10-09 09:42:33 PDT
Comment on attachment 380464 [details]
Patch

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

r=me with the suggested fixes.

I think this new API *needs* extensive runtime logging. I'm OK with adding that in a followup patch, but file a bug and add it quickly.

> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:47
> +    auto element = document.pictureInPictureElement();
> +    if (!element) {

You should also ensure that document.pictureInPictureElement() returns a video element:

if (!element || !is<HTMLVideoElement>(*element) ) {
    ASSERT(is<HTMLVideoElement>(element))
    ...

> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.h:44
> +    static bool pictureInPictureEnabled(Document&) { return true; }

Ports definitely need a way to disable PiP at runtime. Why not check the PictureInPictureAPI runtime flag here instead of always returning true?

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:68
> +    if (!videoElement.player() || !videoElement.player()->supportsPictureInPicture()) {
> +        promise->reject(NotSupportedError);
> +        return;

Nit: all of these rejections should log so it is possible to figure exactly what caused the failure.

> Source/WebCore/html/HTMLVideoElement.cpp:495
> +            if (toPresentationMode(mode) == HTMLVideoElement::VideoPresentationMode::PictureInPicture && toPresentationMode(fullscreenMode()) != HTMLVideoElement::VideoPresentationMode::PictureInPicture)
> +                m_pictureInPictureObserver->didEnterPictureInPicture();
> +            else if (toPresentationMode(mode) != HTMLVideoElement::VideoPresentationMode::PictureInPicture && toPresentationMode(fullscreenMode()) == HTMLVideoElement::VideoPresentationMode::PictureInPicture)
> +                m_pictureInPictureObserver->didExitPictureInPicture();

Nit: you might as well cache toPresentationMode() in local variables instead of calling it multiple times.

> Source/WebCore/platform/Logging.h:85
> +    M(PictureInPictureAPI) \

This is unused, and I think it would be better to use the existing Media logging channel in any case.

> Source/WebCore/platform/PictureInPictureObserver.h:28
> +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE))

Is this the right set of build flags - don't we want "#if ENABLE(PICTURE_IN_PICTURE_API)"
Comment 18 Eric Carlson 2019-10-09 09:51:51 PDT
Comment on attachment 380464 [details]
Patch

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

>> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.h:44
>> +    static bool pictureInPictureEnabled(Document&) { return true; }
> 
> Ports definitely need a way to disable PiP at runtime. Why not check the PictureInPictureAPI runtime flag here instead of always returning true?

The runtime flag clearly won't work because it disables the feature completely, but I think do. need a way to have the API available but disabled (which I assume is the purpose of this attribute).
Comment 19 Peng Liu 2019-10-09 16:30:30 PDT
Created attachment 380584 [details]
Patch
Comment 20 Peng Liu 2019-10-09 16:57:39 PDT
Comment on attachment 380464 [details]
Patch

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

>> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:47
>> +    if (!element) {
> 
> You should also ensure that document.pictureInPictureElement() returns a video element:
> 
> if (!element || !is<HTMLVideoElement>(*element) ) {
>     ASSERT(is<HTMLVideoElement>(element))
>     ...

Right. I will fix it.

>>> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.h:44
>>> +    static bool pictureInPictureEnabled(Document&) { return true; }
>> 
>> Ports definitely need a way to disable PiP at runtime. Why not check the PictureInPictureAPI runtime flag here instead of always returning true?
> 
> The runtime flag clearly won't work because it disables the feature completely, but I think do. need a way to have the API available but disabled (which I assume is the purpose of this attribute).

The API that will be available but disabled is disablePictureInPicture in the Extensions to HTMLVideoElement.
In the new patch, EnabledBySetting is used instead of the EnabledAtRuntime in the IDL, then we can always return true in this function.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:68
>> +        return;
> 
> Nit: all of these rejections should log so it is possible to figure exactly what caused the failure.

I will file a bug to add log messages.

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:83
> +    if (HTMLVideoElementPictureInPicture::disablePictureInPicture(videoElement)) {

This block needs to be removed.

>> Source/WebCore/html/HTMLVideoElement.cpp:495
>> +                m_pictureInPictureObserver->didExitPictureInPicture();
> 
> Nit: you might as well cache toPresentationMode() in local variables instead of calling it multiple times.

Good idea.

>> Source/WebCore/platform/Logging.h:85
>> +    M(PictureInPictureAPI) \
> 
> This is unused, and I think it would be better to use the existing Media logging channel in any case.

Right.

>> Source/WebCore/platform/PictureInPictureObserver.h:28
>> +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE))
> 
> Is this the right set of build flags - don't we want "#if ENABLE(PICTURE_IN_PICTURE_API)"

I chose the current flags because the class can be used in other scenarios even though it was added to support Picture-in-Picture API. I will change the flags to "#if ENABLE(PICTURE_IN_PICTURE_API)" anyway.
Comment 21 Build Bot 2019-10-10 21:24:39 PDT
Comment on attachment 380584 [details]
Patch

Attachment 380584 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13117759

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
Comment 22 Eric Carlson 2019-10-11 08:17:22 PDT
Comment on attachment 380584 [details]
Patch

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

> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:50
> +    auto element = document.pictureInPictureElement();
> +    if (!element || !is<HTMLVideoElement>(*element)) {
> +        promise->reject(InvalidStateError);
> +        return;
> +    }

The PiP element should *always* be an HTMLVideoElement, so this ASSERT so we can catch it in debug builds.

> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:53
> +    HTMLVideoElement& videoElement = downcast<HTMLVideoElement>(*element);
> +    HTMLVideoElementPictureInPicture::from(videoElement)->exitPictureInPicture(WTFMove(promise));

Nit: this can be done in one line.

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:102
> +        videoElementPictureInPicture->m_enteringPictureInPicture = true;
> +        videoElementPictureInPicture->m_enterPictureInPicturePromise = WTFMove(promise);

Do you need a both a bool and a Promise? Couldn't you use just the promises to signal that enter or exit is in progress?

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:191
> +// Defaults to false
> +WK_EXPORT void WKPreferencesSetPictureInPictureAPIEnabled(WKPreferencesRef preferencesRef, bool enabled);
> +WK_EXPORT bool WKPreferencesGetPictureInPictureAPIEnabled(WKPreferencesRef preferencesRef);
> +

Do we need to add new C API?
Comment 23 Peng Liu 2019-10-11 15:12:02 PDT
Comment on attachment 380584 [details]
Patch

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

>> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:50
>> +    }
> 
> The PiP element should *always* be an HTMLVideoElement, so this ASSERT so we can catch it in debug builds.

Right. I think the code can be changed to:
 
    if (!element) {
        promise->reject(InvalidStateError);
        return;
    }

    ASSERT(is<HTMLVideoElement>(element));

>> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:53
>> +    HTMLVideoElementPictureInPicture::from(videoElement)->exitPictureInPicture(WTFMove(promise));
> 
> Nit: this can be done in one line.

Correct.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:102
>> +        videoElementPictureInPicture->m_enterPictureInPicturePromise = WTFMove(promise);
> 
> Do you need a both a bool and a Promise? Couldn't you use just the promises to signal that enter or exit is in progress?

Yes, we can use the promises for that purpose. Will update the patch.

>> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:191
>> +
> 
> Do we need to add new C API?

No. I will remove related changes.
Comment 24 Peng Liu 2019-10-11 15:20:48 PDT
Created attachment 380792 [details]
Patch
Comment 25 Darin Adler 2019-10-11 15:30:41 PDT
Comment on attachment 380792 [details]
Patch

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

> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:53
> +    ASSERT(is<HTMLVideoElement>(element));

Three comments:

1) The whole point of downcast<> is that it contains this assertion built in. So we don’t need this.

2) Maybe we do want a RELEASE_ASSERT here. Some day maybe we’ll change downcast to include a RELEASE_ASSERT instead of an ASSERT, but until then there may be security hardening benefit in doing this type check.

3) If pictureInPictureElement is guaranteed to be HTMLVideoElement, then why isn’t it defined that way? Or maybe it’s only guaranteed to be that in some contexts?
Comment 26 Peng Liu 2019-10-11 16:59:20 PDT
Comment on attachment 380792 [details]
Patch

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

>> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:53
>> +    ASSERT(is<HTMLVideoElement>(element));
> 
> Three comments:
> 
> 1) The whole point of downcast<> is that it contains this assertion built in. So we don’t need this.
> 
> 2) Maybe we do want a RELEASE_ASSERT here. Some day maybe we’ll change downcast to include a RELEASE_ASSERT instead of an ASSERT, but until then there may be security hardening benefit in doing this type check.
> 
> 3) If pictureInPictureElement is guaranteed to be HTMLVideoElement, then why isn’t it defined that way? Or maybe it’s only guaranteed to be that in some contexts?

The pictureInPictureElement is guaranteed to be HTMLVideoElement in current implementation. We chose to use Element as the type to follow the picture-in-picture API spec and we may need to support picture-in-picture for other elements in the future. But for now, we can define it as HTMLVideoElement and I will update the patch accordingly.
Since we will use HTMLVideoElement as the type, the downcast and assert will not be necessary.
Comment 27 Peng Liu 2019-10-14 11:31:50 PDT
Created attachment 380903 [details]
Patch
Comment 28 Radar WebKit Bug Importer 2019-10-14 16:03:50 PDT
<rdar://problem/56268316>
Comment 29 Peng Liu 2019-10-14 18:54:04 PDT
Created attachment 380945 [details]
Patch

Rebased
Comment 30 Eric Carlson 2019-10-15 06:25:56 PDT
Comment on attachment 380945 [details]
Patch

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

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:59
> +    return false;

This will prevent any page with a PictureInPictureWindow from entering the PageCache. Can we do better?

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.h:47
> +    int width() const { return m_width; }
> +    int height() const { return m_height; }

You aren't maintaining PiP window 'state', which is important here as spec says:

    The [width/height] attribute MUST return the [width/height] in CSS pixels of 
    the Picture-in-Picture window associated with pictureInPictureElement if
    the state is opened. Otherwise, it MUST return 0.

> Source/WebCore/dom/Document.cpp:109
> +#if ENABLE(PICTURE_IN_PICTURE_API)
> +#include "HTMLVideoElement.h"
> +#endif

Nit: conditionally included headers are typically listed after the unconditional ones.
Comment 31 Chris Dumez 2019-10-15 10:26:16 PDT
Comment on attachment 380945 [details]
Patch

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

>> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:59
>> +    return false;
> 
> This will prevent any page with a PictureInPictureWindow from entering the PageCache. Can we do better?

This should return true. Please do not add new ActiveDOMObjects that prevent page caching.

This definitely does not need to prevent page cache right now. Later on, when you actually do fire the resize event, I suggest you enqueue it to the DocumentEventQueue instead of dispatching it yourself synchronously. The DocumentEventQueue is page-cache aware.

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:62
> +void PictureInPictureWindow::stop()

Probably not needed.
Comment 32 Peng Liu 2019-10-15 10:39:56 PDT
Comment on attachment 380945 [details]
Patch

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

>>> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:59
>>> +    return false;
>> 
>> This will prevent any page with a PictureInPictureWindow from entering the PageCache. Can we do better?
> 
> This should return true. Please do not add new ActiveDOMObjects that prevent page caching.
> 
> This definitely does not need to prevent page cache right now. Later on, when you actually do fire the resize event, I suggest you enqueue it to the DocumentEventQueue instead of dispatching it yourself synchronously. The DocumentEventQueue is page-cache aware.

Got it, thanks!

>> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:62
>> +void PictureInPictureWindow::stop()
> 
> Probably not needed.

OK, will remove it.

>> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.h:47
>> +    int height() const { return m_height; }
> 
> You aren't maintaining PiP window 'state', which is important here as spec says:
> 
>     The [width/height] attribute MUST return the [width/height] in CSS pixels of 
>     the Picture-in-Picture window associated with pictureInPictureElement if
>     the state is opened. Otherwise, it MUST return 0.

This is planned to be done for https://bugs.webkit.org/show_bug.cgi?id=202615.

>> Source/WebCore/dom/Document.cpp:109
>> +#endif
> 
> Nit: conditionally included headers are typically listed after the unconditional ones.

I see. Will fix it.
Comment 33 Peng Liu 2019-10-15 13:54:56 PDT
Created attachment 381018 [details]
Patch for landing
Comment 34 WebKit Commit Bot 2019-10-15 15:06:31 PDT
Comment on attachment 381018 [details]
Patch for landing

Clearing flags on attachment: 381018

Committed r251160: <https://trac.webkit.org/changeset/251160>