[Webkit-unassigned] [Bug 90084] [GStreamer] Live stream support is weak

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90084





--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2012-07-29 03:18:44 PST ---
(From update of attachment 153237)
View in context: https://bugs.webkit.org/attachment.cgi?id=153237&action=review

This looks good, but based on our in-person discussion it seems like this can be simplified a bit.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:227
> -    , m_preload(MediaPlayer::Auto)
> +    , m_preload(MediaPlayer::None)

It seems that this may be unnecessary based on our conversations yesterday...

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1431
>  {
> +

This seems like it was added accidentally.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1449
> +    if (m_preload == MediaPlayer::None && m_preloadSet && m_initialPreload == MediaPlayer::Auto) {
> +        m_totalBytes = -1;
> +        if (totalBytes() && !isLiveStream()) {
> +            setPreload(m_initialPreload);
> +            gst_element_set_state(m_playBin, GST_STATE_NULL);
> +            gst_element_set_state(m_playBin, GST_STATE_PAUSED);
> +        }
> +    }
>  }

Perhaps roll these members into one like m_originalPreloadWasOverridden (or something with an even better name).

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1680
> +    if (m_readyState < MediaPlayer::HaveMetadata)

See my comment below.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1702
> +    if (m_preload < MediaPlayer::Auto) {

I think it would be better to be explicit here and to enumerate all states that aren't valid. It would be pretty easy to break this check by reordering the enum.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.