Bug 116642 – [Gstreamer] cleanup buffering and state updates

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Balazs Kelemen]

Plans:
 - always do download buffering
 - some logic cleanup

[Balazs Kelemen]

Created attachment 202630 [details]
Patch

[Balazs Kelemen]

Created attachment 202633 [details]
better to init members

[Balazs Kelemen]

Comment on attachment 202633 [details]
better to init members

This needs some more work.

[Philippe Normand]

Please obsolete and clear the r flag instead of marking r- if you're not a reviewer yet.

[Balazs Kelemen]

Created attachment 202831 [details]
Patch

[Philippe Normand]

Comment on attachment 202831 [details]
Patch

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

It's odd that patch fixes http media tests for EFL, those seem to pass already in GTK at least.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1182
> +        // Live pipelines go in PAUSED without prerolling.
> +        m_isStreaming = true;
> +        setDownloadBuffering();

Why moving this out of the test below?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1521
> +    bool shouldDownload = !isLiveStream();
> +    if (shouldDownload) {

No need for a local variable here.

[Balazs Kelemen]

(In reply to comment #6)
> (From update of attachment 202831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202831&action=review
> 
> It's odd that patch fixes http media tests for EFL, those seem to pass already in GTK at least.

Indeed, that's strange. Event loop is different on EFL (that integrates the glib loop with it's own). I guess the difference is timing related.

> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1182
> > +        // Live pipelines go in PAUSED without prerolling.
> > +        m_isStreaming = true;
> > +        setDownloadBuffering();
> 
> Why moving this out of the test below?

To execute it ASAP. Btw GST_STATE_CHANGE_NO_PREROLL should only be returned in GST_STATE_PAUSED state, no? Actually I need something to test live streams, could you give me a link or smg?

> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1521
> > +    bool shouldDownload = !isLiveStream();
> > +    if (shouldDownload) {
> 
> No need for a local variable here.

[Balazs Kelemen]

Created attachment 202978 [details]
Patch

[Balazs Kelemen]

Created attachment 202979 [details]
Patch

[Balazs Kelemen]

Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1182
> > > +        // Live pipelines go in PAUSED without prerolling.
> > > +        m_isStreaming = true;
> > > +        setDownloadBuffering();
> > 
> > Why moving this out of the test below?
> 
> To execute it ASAP. Btw GST_STATE_CHANGE_NO_PREROLL should only be returned in GST_STATE_PAUSED state, no? Actually I need something to test live streams, could you give me a link or smg?
> 

I removed this change. We can handle that separately.

[Balazs Kelemen]

> 
> I removed this change. We can handle that separately.

Err, I mean all the change I introduced for live streams.

[Philippe Normand]

Comment on attachment 202979 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:419
> +    GstState currentState, pendingState;
> +    gst_element_get_state(m_playBin.get(), &currentState, &pendingState, 0);
> +    if (currentState < GST_STATE_PAUSED && pendingState <= GST_STATE_PAUSED)

Move this to ::changePipelineState() perhaps?

[Balazs Kelemen]

(In reply to comment #12)
> (From update of attachment 202979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202979&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:419
> > +    GstState currentState, pendingState;
> > +    gst_element_get_state(m_playBin.get(), &currentState, &pendingState, 0);
> > +    if (currentState < GST_STATE_PAUSED && pendingState <= GST_STATE_PAUSED)
> 
> Move this to ::changePipelineState() perhaps?

It won't work, in seek we use changePipelineState for reseting the pipeline. Also I was thinking of using it more consistently at every place where we change pipeline state (thus not using gst_element_set_state anywhere outside of this function) and update state variables in it accordingly to make sure they are valid all the time.

[Balazs Kelemen]

Can we go on with this?

[Philippe Normand]

Comment on attachment 202979 [details]
Patch

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

Sorry I don't have time now for a full review but I'll try to do it tomorrow

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-860
> -        gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);

Where is this done now?

[Balazs Kelemen]

Comment on attachment 202979 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-860
>> -        gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);
> 
> Where is this done now?

updateStates also takes care of this, I think it is redundant (independently from the patch)

[Philippe Normand]

Comment on attachment 202979 [details]
Patch

Ok, sorry for the delay :/

[Balazs Kelemen]

Landed in http://trac.webkit.org/changeset/151797