Bug 50382 – [GStreamer] hasVideo/Audio return false until the pipeline reaches PAUSED

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

For a video with preload=none and controls shown, no autoplay, the controls won't display the fullscreen/mute buttons until playback started. This is because in that case the playback is delayed and pipeline state remains NULL. So in this case hasVideo and hasAudio return false because the queries on playbin are failing.

The proposed solution is to set pipeline state to PAUSED even if preload=none and in this case ignore the state-change messages and call ::updateStates() once in commitLoad().

[Philippe Normand]

Created attachment 75377 [details]
proposed patch

[Martin Robinson]

Comment on attachment 75377 [details]
proposed patch

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

Seems quite reasonable. I think it needs just a couple fixes.

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:208
> +void mediaPlayerPrivateVideoTagsChangedCallback(GObject* element, gint streamId, gpointer data)

You can just make this void mediaPlayerPrivateVideoTagsChangedCallback(GObject* element, gint streamId, MediaPlayerPrivateGStreamer* player)

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:214
> +void mediaPlayerPrivateAudioTagsChangedCallback(GObject* element, gint streamId, gpointer data)

Same here.

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:346
> +    , m_hasAudio(false)

Shouldn't m_videoTagsTimerHandler and m_audioTagstimreHandler be initialized to 0 here. To guard touching junk data in the destructor?

[Philippe Normand]

Created attachment 75381 [details]
proposed patch

[Martin Robinson]

Comment on attachment 75381 [details]
proposed patch

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

Seems reasonable, but please remove the unecessary lines from the destructor.

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:396
> +    m_videoTagsTimerHandler = 0;

You shouldn't need to zero this field out in the destructor.

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:400
> +    m_audioTagsTimerHandler = 0;

Same here.

[Philippe Normand]

(In reply to comment #4)
> (From update of attachment 75381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75381&action=review
> 
> Seems reasonable, but please remove the unecessary lines from the destructor.
> 
> > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:396
> > +    m_videoTagsTimerHandler = 0;
> 
> You shouldn't need to zero this field out in the destructor.
> 
> > WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:400
> > +    m_audioTagsTimerHandler = 0;
> 
> Same here.

Hum ok but why did you approve the similar changes for the mute/volume timer handlers? :)

[Martin Robinson]

(In reply to comment #5)

> Hum ok but why did you approve the similar changes for the mute/volume timer handlers? :)

Sorry. I missed this in the previous patch. Since this code is in the destructor, it would be an error if any other method ran after this one. Thus, you don't have to worry about nulling out members that aren't accessed again.

[Philippe Normand]

Committed r73257: <http://trac.webkit.org/changeset/73257>