[GStreamer] Don't set state to NULL until element is destroyed

[Sebastian Dröge (slomo)]

It would be good if the MediaPlayer does not set playbin to NULL until it is really done with it and it is to be destroyed. Instead it should only set the state to READY as this allows much faster to go to PAUSED/PLAYING again (playbin caches some things internally, nothing related to the URI though so you could even change URIs).

Patch following

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

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.

[Sebastian Dröge (slomo)]

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.

[Philippe Normand]

(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.

[Andre Moreira Magalhaes]

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.

[Andre Moreira Magalhaes]

Created attachment 210140 [details]
Patch

Same patch as the one from comment #5 but with a comment fixed/updated.

[Build Bot]

Comment on attachment 210140 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 210140 [details]
Patch

Looks like the win EWS is still b0rked.

[Philippe Normand]

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 :)

[Andre Moreira Magalhaes]

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.

[Philippe Normand]

Comment on attachment 210293 [details]
Patch

Ok, I'm not sure about the 60 seconds timer though... We can tweak it later if needed.

[WebKit Commit Bot]

Comment on attachment 210293 [details]
Patch

Clearing flags on attachment: 210293

Committed r154988: <http://trac.webkit.org/changeset/154988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.