[GPUProcess] Video does not resume if the GPU Process crashes while in fullscreen

[Chris Dumez]

Video does not resume if the GPU Process crashes while in fullscreen.

[Chris Dumez]

Created attachment 415685 [details]
Patch

[Peng Liu]

Comment on attachment 415685 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        again. However, the full screen logic lives in the UIProcess and I could no figure out how to

s/no/not/

> Source/WebCore/ChangeLog:13
> +        I have verified that the video renders properly if the user re-enter full screen after a crash.

Nice! I assume this patch works for the picture-in-picture case as well, right? Did you see any issue in the animation (exit fullscreen)?

[Chris Dumez]

(In reply to Peng Liu from comment #2)
> Comment on attachment 415685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415685&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        again. However, the full screen logic lives in the UIProcess and I could no figure out how to
> 
> s/no/not/
> 
> > Source/WebCore/ChangeLog:13
> > +        I have verified that the video renders properly if the user re-enter full screen after a crash.
> 
> Nice! I assume this patch works for the picture-in-picture case as well,
> right? Did you see any issue in the animation (exit fullscreen)?

I see no reason why it would no work in PIP but I have not tested. I personally don't see an animation when full screen exits due to a crash. I am not sure why since we are not calling the exitFullScreenWithoutAnination. That said, it does not look bad, especially considering something crashed.

[Chris Dumez]

Created attachment 415687 [details]
Patch

[Darin Adler]

Comment on attachment 415687 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:535
> +    if (client().mediaPlayerFullscreenMode() != MediaPlayer::VideoFullscreenModeNone)
> +        client().mediaPlayerExitFullscreen();

Not sure it’s self explanatory that "we would play in full screen if we could, but we can’t make that work, so exit full screen to work around that" is the intention here. Maybe add a brief comment?

[Darin Adler]

Comment on attachment 415687 [details]
Patch

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

>> Source/WebCore/platform/graphics/MediaPlayer.cpp:535
>> +        client().mediaPlayerExitFullscreen();
> 
> Not sure it’s self explanatory that "we would play in full screen if we could, but we can’t make that work, so exit full screen to work around that" is the intention here. Maybe add a brief comment?

Maybe something like: "It would be even better if we could resume in full screen mode, but, for now, exiting full screen makes the video rendering work."

[Chris Dumez]

Committed r270586: <https://trac.webkit.org/changeset/270586>

[Radar WebKit Bug Importer]

<rdar://problem/72141981>