Bug 220660 – PiP video subtitles stop updating when Safari is backgrounded

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

PiP video subtitles stop updating when Safari is backgrounded

[Peng Liu]

<rdar://problem/70864713>

[Peng Liu]

Created attachment 417714 [details]
Patch

[Darin Adler]

Comment on attachment 417714 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:219
> +    return m_isRenderingSuspended && !m_webPage.videoFullscreenManager().hasVideo();

This code is a little bit mysterious, not obvious. Hard to understand this is about PIP and why it’s correct. There’s not even a why comment!

And worse, we have no regression test for this, so someone could break it and commit the change without ever knowing it.

That’s bad combination: seems like we may be unable to keep this working and might make the same kind of mistake again in the future.

[Peng Liu]

Created attachment 417746 [details]
Only prevent suspending rendering when a video with visible text tracks is in PiP

[Peng Liu]

Comment on attachment 417714 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:219
>> +    return m_isRenderingSuspended && !m_webPage.videoFullscreenManager().hasVideo();
> 
> This code is a little bit mysterious, not obvious. Hard to understand this is about PIP and why it’s correct. There’s not even a why comment!
> 
> And worse, we have no regression test for this, so someone could break it and commit the change without ever knowing it.
> 
> That’s bad combination: seems like we may be unable to keep this working and might make the same kind of mistake again in the future.

Sorry, I forgot to remove the "r?" flag when we are discussing the patch on slack. :-(

Agree! A regression test is needed. But the issue happens when the browser is in the background and a video with text tracks is playing in picture-in-picture. We are not able to test it on iOS before fixing bug 203724.

[Peng Liu]

Created attachment 417933 [details]
Only unfreeze the layer tree when we need to update text track representation

[Peng Liu]

Created attachment 418017 [details]
Patch

[Peng Liu]

Created attachment 418018 [details]
Patch

[Eric Carlson]

Comment on attachment 418018 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2650
> +    if (m_layerTreeFreezeReasons.hasExactlyOneBitSet() && m_layerTreeFreezeReasons.contains(LayerTreeFreezeReason::BackgroundApplication) && m_videoFullscreenManager && m_videoFullscreenManager->videoInPictureInPicture()) {

Nit: `m_videoFullscreenManager` will be NULL most of the time, so you might want to check it first to short circuit the rest of the tests.

[Peng Liu]

Comment on attachment 418018 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2650
>> +    if (m_layerTreeFreezeReasons.hasExactlyOneBitSet() && m_layerTreeFreezeReasons.contains(LayerTreeFreezeReason::BackgroundApplication) && m_videoFullscreenManager && m_videoFullscreenManager->videoInPictureInPicture()) {
> 
> Nit: `m_videoFullscreenManager` will be NULL most of the time, so you might want to check it first to short circuit the rest of the tests.

Good point! Thanks!

[Darin Adler]

Comment on attachment 418018 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        Subtitles in the picture-in-picture window will stop updating when the browser is
> +        in the background because we freeze the layer tree when a browser is in the background.
> +        This patch fixes this issue by avoiding freezing the layer tree if a video is playing
> +        in picture-in-picture when the browser is in the background.

Can we construct a regression test?

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h:121
> +    bool videoInPictureInPicture() const;

This sounds like a function that would return a "video", not a function that would return a boolean answer to a question.

Given the comment you wrote above, it seems that this function’s name should be hasVideoPlayingInPictureInPicture().

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:274
> +    if (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture)
> +        m_videoElementInPictureInPicture = makeWeakPtr(videoElement);

Seems like an omission that if the mode is something else, we don’t null out m_videoElementInPictureInPicture. Do we have an iron clad guarantee it will be null in that case? Could we maybe just set to null? Or is there some chance that we can have a PiP lingering while we start some other full-screen, perhaps on a different element?

[Peng Liu]

Comment on attachment 418018 [details]
Patch

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

>> Source/WebKit/ChangeLog:11
>> +        in picture-in-picture when the browser is in the background.
> 
> Can we construct a regression test?

I have to manually test this patch because we cannot automatically test picture-in-picture features on iOS now.
We plan to add regression tests for video fullscreen and picture-in-picture with the XCTest framework in WebKit.

>>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2650
>>> +    if (m_layerTreeFreezeReasons.hasExactlyOneBitSet() && m_layerTreeFreezeReasons.contains(LayerTreeFreezeReason::BackgroundApplication) && m_videoFullscreenManager && m_videoFullscreenManager->videoInPictureInPicture()) {
>> 
>> Nit: `m_videoFullscreenManager` will be NULL most of the time, so you might want to check it first to short circuit the rest of the tests.
> 
> Good point! Thanks!

Fixed.

>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h:121
>> +    bool videoInPictureInPicture() const;
> 
> This sounds like a function that would return a "video", not a function that would return a boolean answer to a question.
> 
> Given the comment you wrote above, it seems that this function’s name should be hasVideoPlayingInPictureInPicture().

Agree! Fixed.

>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:274
>> +        m_videoElementInPictureInPicture = makeWeakPtr(videoElement);
> 
> Seems like an omission that if the mode is something else, we don’t null out m_videoElementInPictureInPicture. Do we have an iron clad guarantee it will be null in that case? Could we maybe just set to null? Or is there some chance that we can have a PiP lingering while we start some other full-screen, perhaps on a different element?

The behavior is intended.

More than one video elements can be in fullscreen (sounds strange though), but only one video can be in picture-in-picture. And it is possible that one video is in picture-in-picture while another video is in fullscreen.

When a video is in picture-in-picture and another video tries to enter picture-in-picture, the current video in picture-in-picture will exit picture-in-picture while the new video enters picture-in-picture.

This patch works in those cases.

[Peng Liu]

Created attachment 418105 [details]
Patch for landing

[EWS]

Committed r271737: <https://trac.webkit.org/changeset/271737>

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