PiP video subtitles stop updating when Safari is backgrounded
<rdar://problem/70864713>
Created attachment 417714 [details] Patch
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.
Created attachment 417746 [details] Only prevent suspending rendering when a video with visible text tracks is in PiP
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.
Created attachment 417933 [details] Only unfreeze the layer tree when we need to update text track representation
Created attachment 418017 [details] Patch
Created attachment 418018 [details] Patch
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.
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!
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?
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.
Created attachment 418105 [details] Patch for landing
Committed r271737: <https://trac.webkit.org/changeset/271737> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418105 [details].