[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 #7 from Martin Robinson <mrobinson at webkit.org>  2012-07-07 19:04:55 PST ---
(From update of attachment 151150)
View in context: https://bugs.webkit.org/attachment.cgi?id=151150&action=review

Great! I'm not quite sure I understand all these changes yet, so I'm not going to flip the flags. I have a few comments though.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1444
> +    // We need to know the total amount of bytes occupied by the
> +    // media, especially if on-disk buffering is requested.

You can let this comment flow.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1448
> +    if (attemptTotalBytesCache && cacheTotalBytes() && totalBytes() && !isLiveStream() && m_initialPreload == MediaPlayer::Auto) {
> +        setPreload(m_initialPreload);
> +        gst_element_set_state(m_playBin, GST_STATE_NULL);
> +        gst_element_set_state(m_playBin, GST_STATE_PAUSED);

I'm not exactly sure why you want to only cache the total bytes sometimes...

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:203
> +            unsigned m_totalBytes;
> +            bool m_totalBytesCached;

This should probably be called m_cachedTotalBytes, because at first I read it like "The total bytes we have cached" and the name I suggested is less ambiguous.

Even better is if you could find a way to eliminate m_totalBytesCached entirely, perhaps by initializing  m_totalBytes to -1 (perhaps make it a long).

Assuming you don't do that...an unsigned seems an odd type to use for some number of bytes. Perhaps size_t would be better?

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