Bug 90084 - [GStreamer] Live stream support is weak
Summary: [GStreamer] Live stream support is weak
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://www.youtube.com/live
Keywords:
: 90086 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-06-27 10:48 PDT by Philippe Normand
Modified: 2012-07-31 10:19 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.80 KB, patch)
2012-07-07 17:37 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2012-07-07 21:16 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (12.52 KB, patch)
2012-07-19 05:44 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (12.09 KB, patch)
2012-07-31 08:37 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (12.12 KB, patch)
2012-07-31 09:08 PDT, 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 2012-06-27 10:48:34 PDT
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).
Comment 1 Philippe Normand 2012-06-27 11:35:56 PDT
*** Bug 90086 has been marked as a duplicate of this bug. ***
Comment 2 Simon Pena 2012-06-28 01:53:27 PDT
(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.
Comment 3 Philippe Normand 2012-06-28 09:30:18 PDT
Sounds related indeed.
Comment 4 Philippe Normand 2012-06-28 10:30:40 PDT
In webkitwebsrc we should also configure appsrc for GST_APP_STREAM_TYPE_STREAM instead of _SEEKABLE for live sources, I think.
Comment 5 Philippe Normand 2012-07-07 17:37:30 PDT
Created attachment 151150 [details]
Patch
Comment 6 Philippe Normand 2012-07-07 17:39:33 PDT
(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.
Comment 7 Martin Robinson 2012-07-07 19:04:55 PDT
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?
Comment 8 Philippe Normand 2012-07-07 20:02:13 PDT
(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!
Comment 9 Philippe Normand 2012-07-07 21:16:39 PDT
Created attachment 151155 [details]
Patch
Comment 10 Simon Hausmann 2012-07-13 05:05:49 PDT
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?
Comment 11 Philippe Normand 2012-07-18 06:43:26 PDT
(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!
Comment 12 Philippe Normand 2012-07-19 05:44:27 PDT
Created attachment 153237 [details]
Patch

Only change is the one suggested by Simon.
Comment 13 Martin Robinson 2012-07-29 03:18:44 PDT
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.
Comment 14 Philippe Normand 2012-07-31 08:37:25 PDT
Created attachment 155549 [details]
Patch
Comment 15 Philippe Normand 2012-07-31 09:08:44 PDT
Created attachment 155559 [details]
Patch
Comment 16 Martin Robinson 2012-07-31 09:15:35 PDT
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.
Comment 17 Philippe Normand 2012-07-31 10:19:28 PDT
Committed r124217: <http://trac.webkit.org/changeset/124217>