Bug 201024 – [Picture-in-Picture Web API] Implement HTMLVideoElement.requestPictureInPicture() / Document.exitPictureInPicture()

• Component: Media
• Status: RESOLVED FIXED

[Carlos Bentzen]

This should be the first part of the API: the ability to enter and exit Picture-in-Picture.

Implementation can reuse code of webkitSetPresentationMode().

[Carlos Bentzen]

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?

[Carlos Bentzen]

(In reply to Carlos Eduardo Ramalho from comment #1)
> Created attachment 376973 [details]
> WIP

Looks like it needs some rebasing. Will do.

[Carlos Bentzen]

Created attachment 376975 [details]
WIP

Rebased WIP patch.

[Carlos Bentzen]

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.

[youenn fablet]

(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.

[Carlos Bentzen]

(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 :)

[Carlos Bentzen]

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.

[Carlos Bentzen]

(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

[Peng Liu]

Hi Carlos, any update on the progress? Thanks!

[Carlos Bentzen]

(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.

[Peng Liu]

Created attachment 380292 [details]
Patch

[Peng Liu]

Comment on attachment 380292 [details]
Patch

Majority work of this patch was done by Carlos Eduardo Ramalho <[e-mail]>.

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.

[Peng Liu]

Created attachment 380301 [details]
Update the patch to fix several layout test case failures

[Peng Liu]

Created attachment 380303 [details]
An updated patch to fix iOS build

[Peng Liu]

Created attachment 380304 [details]
An updated patch to fix gtk build failure

[Peng Liu]

Created attachment 380464 [details]
Patch

[Eric Carlson]

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)"

[Eric Carlson]

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).

[Peng Liu]

Created attachment 380584 [details]
Patch

[Peng Liu]

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.

[EWS Watchlist]

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

[Eric Carlson]

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?

[Peng Liu]

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.

[Peng Liu]

Created attachment 380792 [details]
Patch

[Darin Adler]

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?

[Peng Liu]

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.

[Peng Liu]

Created attachment 380903 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56268316>

[Peng Liu]

Created attachment 380945 [details]
Patch

Rebased

[Eric Carlson]

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.

[Chris Dumez]

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.

[Peng Liu]

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.

[Peng Liu]

Created attachment 381018 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 381018 [details]
Patch for landing

Clearing flags on attachment: 381018

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