Skip to content

Conversation

@eocanha
Copy link
Contributor

@eocanha eocanha commented Nov 14, 2023

e062421

[GStreamer] Video goes blank on some platforms after playback ends
https://bugs.webkit.org/show_bug.cgi?id=264739

Reviewed by NOBODY (OOPS!).

On some downstream platforms, the video gets black after playback finishes.

This happens because https://bugs.webkit.org/show_bug.cgi?id=89122 used to set
the pipeline to NULL, although later https://bugs.webkit.org/show_bug.cgi?id=117354
set it to READY. This change deinitializes a big part of the pipeline, which
should otherwise stay in a working state if the state was PAUSED as it should
be without those changes. While on desktop platforms the last video frame is
still kept and shown, on some downstream platforms using a custom multimedia
subsystem the deinitialization destroys internal resources and makes that last
video frame not to be visible anymore.

The original purpose of the mentioned bugs was to avoid having the audio device
opened after the video finished. This was relevant in 2012, when many
platforms still used ALSA as a sound system, but it's not so relevant nowadays,
when PulseAudio is the usual choice on desktop.

This patch reworks the readyTimerHandler and readyTimerFired into
pausedTimerHandler and pausedTimerFired, because now the pipeline stays in
PAUSED and having a timer to eventually releasing resources by transitioning
to NULL still makes sense. The timeout has been increased to 5 minutes and
maybe it would eventually need to be even larger.

Original author: suresh-khurdiya-epam <skhurdiya.contractor@libertyglobal.com>

See: WebPlatformForEmbedded/WPEWebKit#1205

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer): Renamed m_readyTimerHandler to m_pausedTimerHandler.
(WebCore::MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto, and added extra conditions to only start the timer on PAUSE at end.
(WebCore::MediaPlayerPrivateGStreamer::loadingFailed): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::didEnd): Set the pipeline to PAUSED on end, instead of setting it to READY.
(WebCore::MediaPlayerPrivateGStreamer::pausedTimerFired): Renamed from readyTimerFired.
(WebCore::MediaPlayerPrivateGStreamer::readyTimerFired): Deleted.
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Renamed m_readyTimerHandler to m_pausedTimerHandler and readyTimerFired() to pausedTimerFired().

e062421

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ❌ 🛠 wpe ❌ 🛠 wincairo
❌ 🧪 bindings ❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2
❌ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ❌ 🛠 gtk
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
❌ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🧪 api-gtk
❌ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
❌ 🛠 tv-sim ❌ 🧪 mac-wk2-stress
❌ 🛠 watch
❌ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=264739

Reviewed by NOBODY (OOPS!).

On some downstream platforms, the video gets black after playback finishes.

This happens because https://bugs.webkit.org/show_bug.cgi?id=89122 used to set
the pipeline to NULL, although later https://bugs.webkit.org/show_bug.cgi?id=117354
set it to READY. This change deinitializes a big part of the pipeline, which
should otherwise stay in a working state if the state was PAUSED as it should
be without those changes. While on desktop platforms the last video frame is
still kept and shown, on some downstream platforms using a custom multimedia
subsystem the deinitialization destroys internal resources and makes that last
video frame not to be visible anymore.

The original purpose of the mentioned bugs was to avoid having the audio device
opened after the video finished. This was relevant in 2012, when many
platforms still used ALSA as a sound system, but it's not so relevant nowadays,
when PulseAudio is the usual choice on desktop.

This patch reworks the readyTimerHandler and readyTimerFired into
pausedTimerHandler and pausedTimerFired, because now the pipeline stays in
PAUSED and having a timer to eventually releasing resources by transitioning
to NULL still makes sense. The timeout has been increased to 5 minutes and
maybe it would eventually need to be even larger.

Original author: suresh-khurdiya-epam <skhurdiya.contractor@libertyglobal.com>

See: WebPlatformForEmbedded/WPEWebKit#1205

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer): Renamed m_readyTimerHandler to m_pausedTimerHandler.
(WebCore::MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto, and added extra conditions to only start the timer on PAUSE at end.
(WebCore::MediaPlayerPrivateGStreamer::loadingFailed): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::didEnd): Set the pipeline to PAUSED on end, instead of setting it to READY.
(WebCore::MediaPlayerPrivateGStreamer::pausedTimerFired): Renamed from readyTimerFired.
(WebCore::MediaPlayerPrivateGStreamer::readyTimerFired): Deleted.
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Renamed m_readyTimerHandler to m_pausedTimerHandler and readyTimerFired() to pausedTimerFired().
@eocanha eocanha requested a review from philn as a code owner November 14, 2023 14:32
@eocanha eocanha self-assigned this Nov 14, 2023
@eocanha eocanha added the Media Bugs related to the HTML 5 Media elements. label Nov 14, 2023
@philn
Copy link
Member

philn commented Nov 14, 2023

EWS doesn't seem to be able to access your WebKit repo? Also why open a new PR? There was one already.

@eocanha
Copy link
Contributor Author

eocanha commented Nov 14, 2023

Ah... it might be because I used a different branch locally. I'm cancelling this PR, updating WebKit, rebasing and resubmitting the original one.

@eocanha eocanha closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Media Bugs related to the HTML 5 Media elements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants