Bug 30000 – [GStreamer] Check return values of gst_element_set_state()

• Component: Media
• Status: RESOLVED FIXED

[Sebastian Dröge (slomo)]

Hi,
currently the media player doesn't check the return values of gst_element_set_state(). It should really do that.

If it's SUCCESS, NO_PREROLL or ASYNC everything is fine (errors will be posted as bus message), if it's FAILURE things should fail immediately (there will be no error messages on the bus in the worst case).

This is only for state changes from NULL or READY to PAUSED or PLAYING. Downwards state changes can fail but you're usually not interested in that.

[Philippe Normand]

Created attachment 44398 [details]
Check return values of gst_element_set_state()

when going from READY/NULL to PAUSED or PLAYING.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44398 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44398 [details]
Check return values of gst_element_set_state()

It seems play and pause could share all of their code here in a single method which took either GST_STATE_PAUSED or GST_STATE_PLAYING.

I think that would be cleaner long-term.

[Philippe Normand]

Created attachment 44519 [details]
Check for state change failure when going from READY/NULL to PAUSED or PLAYING.

[Eric Seidel (no email)]

Comment on attachment 44519 [details]
Check for state change failure when going from READY/NULL to PAUSED or PLAYING.

OK.  There is one slight change in logging behavior here, it won't log Play if the switch to play fails.  I don't think that matters though.

Style violation:
 121             bool changePipelineState(GstState state);
we don't name arguments when the name doesn't add anything.

Shouldn't changePipelineState be private?

And why is it called "pipeline state"?

It would appear that this function only works for either PLAY or PAUSE, but I don't see you asserting that anywhere.  You might want to ASSERT the inputs.

[Philippe Normand]

Created attachment 44879 [details]
Check for state change failure when going from READY/NULL to PAUSED or PLAYING.

Check for state change failure

When going from READY/NULL to PAUSED or PLAYING.

[Philippe Normand]

Comment on attachment 44879 [details]
Check for state change failure when going from READY/NULL to PAUSED or PLAYING.

Sorry that one is broken

[Philippe Normand]

(In reply to comment #5)
> (From update of attachment 44519 [details])
> OK.  There is one slight change in logging behavior here, it won't log Play if
> the switch to play fails.  I don't think that matters though.
> 
> Style violation:
>  121             bool changePipelineState(GstState state);
> we don't name arguments when the name doesn't add anything.

Well the variable is used in the code, I will name it newState, which adds "new" :)

> 
> Shouldn't changePipelineState be private?
> 

It is already a private method. Did I do something wrong?

> And why is it called "pipeline state"?
> 

This is the GStreamer vocabulary, the pipeline is composed of elements, linked etween themselves. To start playback you set the pipeline state to PLAYING, the state change propagates through the elements of the pipeline.

> It would appear that this function only works for either PLAY or PAUSE, but I
> don't see you asserting that anywhere.  You might want to ASSERT the inputs.

Will do ;)

Thanks for  the review, I will send a new patch.

[Philippe Normand]

Created attachment 44880 [details]
Check for state change failure when going from READY/NULL to PAUSED or PLAYING.

When going from READY/NULL to PAUSED or PLAYING.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44880 [details] without any errors.

[Xan Lopez]

Comment on attachment 44880 [details]
Check for state change failure when going from READY/NULL to PAUSED or PLAYING.

Makes sense, although I guess it would be best to somehow inform the user instead of silently doing nothing? It can be done in a follow-up though.

[Philippe Normand]

Landed as r53255. Thanks!