Bug 50382 - [GStreamer] hasVideo/Audio return false until the pipeline reaches PAUSED
Summary: [GStreamer] hasVideo/Audio return false until the pipeline reaches PAUSED
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 50228
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-02 04:09 PST by Philippe Normand
Modified: 2010-12-03 03:07 PST (History)
2 users (show)

See Also:


Attachments
proposed patch (15.01 KB, patch)
2010-12-02 07:40 PST, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
proposed patch (15.09 KB, patch)
2010-12-02 08:56 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-12-02 04:09:16 PST
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().
Comment 1 Philippe Normand 2010-12-02 07:40:51 PST
Created attachment 75377 [details]
proposed patch
Comment 2 Martin Robinson 2010-12-02 07:54:51 PST
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?
Comment 3 Philippe Normand 2010-12-02 08:56:55 PST
Created attachment 75381 [details]
proposed patch
Comment 4 Martin Robinson 2010-12-02 09:54:36 PST
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.
Comment 5 Philippe Normand 2010-12-03 02:17:12 PST
(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? :)
Comment 6 Martin Robinson 2010-12-03 02:57:36 PST
(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.
Comment 7 Philippe Normand 2010-12-03 03:07:52 PST
Committed r73257: <http://trac.webkit.org/changeset/73257>