WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
117354
[GStreamer] Don't set state to NULL until element is destroyed
https://bugs.webkit.org/show_bug.cgi?id=117354
Summary
[GStreamer] Don't set state to NULL until element is destroyed
Sebastian Dröge (slomo)
Reported
2013-06-07 10:44:30 PDT
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
Attachments
Don't set GStreamer element state to NULL until it is to be destroyed
(3.65 KB, patch)
2013-06-07 10:57 PDT
,
Sebastian Dröge (slomo)
no flags
Details
Formatted Diff
Diff
Patch
(13.25 KB, patch)
2013-08-30 11:22 PDT
,
Andre Moreira Magalhaes
no flags
Details
Formatted Diff
Diff
Patch
(13.26 KB, patch)
2013-08-30 11:32 PDT
,
Andre Moreira Magalhaes
no flags
Details
Formatted Diff
Diff
Patch
(13.41 KB, patch)
2013-09-02 09:23 PDT
,
Andre Moreira Magalhaes
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Sebastian Dröge (slomo)
Comment 1
2013-06-07 10:57:30 PDT
Created
attachment 204055
[details]
Don't set GStreamer element state to NULL until it is to be destroyed
Philippe Normand
Comment 2
2013-06-10 01:15:01 PDT
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)
Comment 3
2013-06-10 04:42:04 PDT
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
Comment 4
2013-06-10 05:44:37 PDT
(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
Comment 5
2013-08-30 11:22:35 PDT
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
Comment 6
2013-08-30 11:32:03 PDT
Created
attachment 210140
[details]
Patch Same patch as the one from
comment #5
but with a comment fixed/updated.
Build Bot
Comment 7
2013-08-30 13:15:22 PDT
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 8
2013-08-30 13:31:23 PDT
Comment on
attachment 210140
[details]
Patch Looks like the win EWS is still b0rked.
Philippe Normand
Comment 9
2013-09-02 03:24:39 PDT
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
Comment 10
2013-09-02 09:23:12 PDT
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 11
2013-09-03 09:27:15 PDT
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 12
2013-09-03 09:50:26 PDT
Comment on
attachment 210293
[details]
Patch Clearing flags on attachment: 210293 Committed
r154988
: <
http://trac.webkit.org/changeset/154988
>
WebKit Commit Bot
Comment 13
2013-09-03 09:50:30 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug