Bug 90084

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

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

Sounds related indeed.

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

Created attachment 151150 [details] Patch

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

(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!

Created attachment 151155 [details] Patch

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?

(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!

Created attachment 153237 [details] Patch Only change is the one suggested by Simon.

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.

Created attachment 155549 [details] Patch

Created attachment 155559 [details] Patch

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.

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