Bug 90084 – [GStreamer] Live stream support is weak

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

For instance Youtube live streams don't play at all unless on-disk bufferring is disabled (see ::setPreload()).

If that's disabled, one can see the live stream and pause it. But not resume it (this needs investigation, looks like we attempt a seek and that fails).
Additionally we should also implement ::movieLoadType() (needs investigation).

[Philippe Normand]

*** Bug 90086 has been marked as a duplicate of this bug. ***

[Simon Pena]

(In reply to comment #0)
[...] 
> If that's disabled, one can see the live stream and pause it. But not resume it (this needs investigation, looks like we attempt a seek and that fails).
> Additionally we should also implement ::movieLoadType() (needs investigation).

This failing seek feels somehow related with bug #85994, where the seek operations were implemented based on Ranged Requests, so when the server didn't reply with a 206 status code, our player would fail.

[Philippe Normand]

Sounds related indeed.

[Philippe Normand]

In webkitwebsrc we should also configure appsrc for GST_APP_STREAM_TYPE_STREAM instead of _SEEKABLE for live sources, I think.

[Philippe Normand]

Created attachment 151150 [details]
Patch

[Philippe Normand]

(In reply to comment #4)
> In webkitwebsrc we should also configure appsrc for GST_APP_STREAM_TYPE_STREAM instead of _SEEKABLE for live sources, I think.

Oh well, not sure this is needed.
I got the Youtube live streams to play/pause/resume with the attached patch, without breaking on-disk buffering for non-live media, hopefully.

[Martin Robinson]

Comment on attachment 151150 [details]
Patch

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?

[Philippe Normand]

(In reply to comment #7)

> > 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...
> 

There's no need to cache it if durationChanged() is called during on-disk buffering, which can be done only if totalBytes() is non-null anyway.

> > 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.
> 

Oh yes, I didn't realize that ambiguation indeed.

> 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?

Hum, I'll give this some thoughts and testing. Thanks a lot for the initial feedback!

[Philippe Normand]

Created attachment 151155 [details]
Patch

[Simon Hausmann]

Comment on attachment 151155 [details]
Patch

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

I very much agree with the general direction of this patch. I had to do the same thing for the N9 :)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1434
> +    if (m_preload == MediaPlayer::None && m_initialPreload == MediaPlayer::Auto) {

I think it may be theoretically possible that you would access m_initialPreload uninitialized. Perhaps you should check for m_preloadSet here also?

[Philippe Normand]

(In reply to comment #10)
> (From update of attachment 151155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151155&action=review
> 
> I very much agree with the general direction of this patch. I had to do the same thing for the N9 :)
> 

Too bad I didn't see any patch in bugzilla! :/

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1434
> > +    if (m_preload == MediaPlayer::None && m_initialPreload == MediaPlayer::Auto) {
> 
> I think it may be theoretically possible that you would access m_initialPreload uninitialized. Perhaps you should check for m_preloadSet here also?

Oh yes good point. I'll check this issue and update the patch. Thanks for the feedback!

[Philippe Normand]

Created attachment 153237 [details]
Patch

Only change is the one suggested by Simon.

[Martin Robinson]

Comment on attachment 153237 [details]
Patch

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.

[Philippe Normand]

Created attachment 155549 [details]
Patch

[Philippe Normand]

Created attachment 155559 [details]
Patch

[Martin Robinson]

Comment on attachment 155559 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:156
> +            bool isLiveStream() const { return m_isStreaming; }

I think I would prefer isStreaming() here.

[Philippe Normand]

Committed r124217: <http://trac.webkit.org/changeset/124217>