Bug 117354

Created attachment 204055 [details] Don't set GStreamer element state to NULL until it is to be destroyed

We used to do that (setting to READY) but then people started to complain that resources/devices were not freed properly. It turns out this is Bug 109350. You can also see Bug 89122... So, until Bug 109350 is fixed I'd like to hold on this patch, if you don't object.

Fine with me, I don't really understand the problem in that other bug though. That's more related to webkit (and not even the webkit gstreamer backend) not cleaning up its stuff, no? For this patch here, something important is indeed missing. After going to READY, e.g. audio devices are still opened. So there should be a timeout, after which the element is set to NULL.

(In reply to comment #3) > Fine with me, I don't really understand the problem in that other bug though. That's more related to webkit (and not even the webkit gstreamer backend) not cleaning up its stuff, no? > It is related, I think. In some cases the MediaPlayer and its Private backend actually are not freed and leak :( > > For this patch here, something important is indeed missing. After going to READY, e.g. audio devices are still opened. So there should be a timeout, after which the element is set to NULL. Yes, that sounds like a good idea to me.

Created attachment 210138 [details] Patch (In reply to comment #4) > > For this patch here, something important is indeed missing. After going to READY, e.g. audio devices are still opened. So there should be a timeout, after which the element is set to NULL. > > Yes, that sounds like a good idea to me. Updated patch should handle this. I used a 1 minute timeout for now. I found an issue when setting the pipeline to READY on didEnd() where the updateStates() method was being invoked and resetting the network/ready states thus making the HTMLMediaElement recreate the player when playing the video again after EOS. When setting to NULL on EOS the updateStates() method is not called (pipeline flushing), hence why we didn't have this issue before. To fix it the patch added a check for EOS on updateStates before updating the network/ready states. So what happens now is that when finishing playing a video (EOS) we set the pipeline to READY state and if the video is played again (without reloading page of course) before the 1 minute timeout we go from READY->PAUSED->PLAYING instead of starting from NULL, thus saving some resources loading (e.g. metadata). Apart from the possible (if played before timeout) gain when replaying videos on EOS I don't see any other advantage in not setting the pipeline to NULL as it is done today. Please let me know what you think.

Created attachment 210140 [details] Patch Same patch as the one from comment #5 but with a comment fixed/updated.

Comment on attachment 210140 [details] Patch Attachment 210140 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1641061

Comment on attachment 210140 [details] Patch Looks like the win EWS is still b0rked.

Comment on attachment 210140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210140&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:291 > + if (m_readyTimerHandler) > + g_source_remove(m_readyTimerHandler); Shouldn't this be done before m_playBin is cleared? Otherwise the timer might fire with an invalid player pointer? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:413 > + // See also https://bugzilla.gnome.org/show_bug.cgi?id=117354 That bug seems quite unrelated :)

Created attachment 210293 [details] Patch (In reply to comment #9) > (From update of attachment 210140 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210140&action=review > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:291 > > + if (m_readyTimerHandler) > > + g_source_remove(m_readyTimerHandler); > > Shouldn't this be done before m_playBin is cleared? Otherwise the timer might fire with an invalid player pointer? > Changed to remove the source before m_playBin is cleared. The change is not actually required as the timer would not be fired in the destructor (same thread and no one calling main_loop_iterate) but doesn't hurt. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:413 > > + // See also https://bugzilla.gnome.org/show_bug.cgi?id=117354 > > That bug seems quite unrelated :) hehe, oops, fixed :) Also rebased against upstream/master and fixed conflicts.

Comment on attachment 210293 [details] Patch Ok, I'm not sure about the 60 seconds timer though... We can tweak it later if needed.

Comment on attachment 210293 [details] Patch Clearing flags on attachment: 210293 Committed r154988: <http://trac.webkit.org/changeset/154988>

All reviewed patches have been landed. Closing bug.