The GStreamer MSE implementation didn't implement flush() because GStreamer didn't offer a natural way to flush just one (audio/video) stream instead of flushing the whole pipeline. As this use case was only relevant during seeks, we just did nothing and waited the seek to happen and perform the flush by itself (a seek implies a flush in GStreamer). However, we have recently detected that flush() also happens in YouTube TV when a quality change happens, so the web page reenqueues higher quality samples and overwrites some buffered ranges having lower quality and having already been enqueued for playback. In that case, flush() should clean the already enqueued data only on that stream and a new reenqueue would provide more accurate (higher resolution) data. As flush() does nothing in our case, the outcome was that the user could see some parts of the video "played twice", together with some audio/video desynchronization. The right fix would be to properly implement per-stream flushing.
Created attachment 303511 [details] Patch
Comment on attachment 303511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303511&action=review > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:387 > + PlaybackPipeline* playbackPipeline = static_cast<PlaybackPipeline*>(userData); Where is this used? > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:389 > + GstSegment* segment = nullptr; > + gst_event_parse_segment(event, const_cast<const GstSegment**>(&segment)); Dunno if I'm reading documentation for gst_event_parse_segment() wrong, but to me it seems as if the segment pointer should point to a valid GstSegment object. > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:453 > + std::unique_ptr<GstSegment, decltype(&gst_segment_free)> segment(gst_segment_new(), &gst_segment_free); Please add the necessary deleter to GUniquePtrGStreamer.h
Comment on attachment 303511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303511&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:387 >> + PlaybackPipeline* playbackPipeline = static_cast<PlaybackPipeline*>(userData); > > Where is this used? It was a leftover of my debugging branch. Thanks for pointing it out. >> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:389 >> + gst_event_parse_segment(event, const_cast<const GstSegment**>(&segment)); > > Dunno if I'm reading documentation for gst_event_parse_segment() wrong, but to me it seems as if the segment pointer should point to a valid GstSegment object. It's a GstSegment** (ie: pointer pointer). This kind of typing is typically used to return pointers, not pointer contents like it would be if it was a GstSegment*. The pattern is similar to the one used here for gst_message_parse_tag(): https://github.com/WebKit/webkit/blob/8091486f/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp#L1078
Created attachment 303547 [details] Patch
(In reply to comment #3) > >> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:389 > >> + gst_event_parse_segment(event, const_cast<const GstSegment**>(&segment)); > > > > Dunno if I'm reading documentation for gst_event_parse_segment() wrong, but to me it seems as if the segment pointer should point to a valid GstSegment object. > > It's a GstSegment** (ie: pointer pointer). This kind of typing is typically > used to return pointers, not pointer contents like it would be if it was a > GstSegment*. The pattern is similar to the one used here for > gst_message_parse_tag(): > https://github.com/WebKit/webkit/blob/8091486f/Source/WebCore/platform/ > graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp#L1078 So it retrieves a pointer to the Segment object we create in flush()?
(In reply to comment #5) > (In reply to comment #3) > > >> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:389 > > >> + gst_event_parse_segment(event, const_cast<const GstSegment**>(&segment)); > > > > > > Dunno if I'm reading documentation for gst_event_parse_segment() wrong, but to me it seems as if the segment pointer should point to a valid GstSegment object. > > > > It's a GstSegment** (ie: pointer pointer). This kind of typing is typically > > used to return pointers, not pointer contents like it would be if it was a > > GstSegment*. The pattern is similar to the one used here for > > gst_message_parse_tag(): > > https://github.com/WebKit/webkit/blob/8091486f/Source/WebCore/platform/ > > graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp#L1078 > > So it retrieves a pointer to the Segment object we create in flush()? OK, re-reading gst_event_parse_segment(), the GstSegment object is stored in the GstEvent's custom structure. That's clear now.
Comment on attachment 303547 [details] Patch It seems that this patch is breaking 5 tests, so I prefer to have a look at them before committing: imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-av-audio-bitrate.html imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-av-video-bitrate.html imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-bitrate.html imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framerate.html
Created attachment 303672 [details] Patch
Comment on attachment 303672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303672&action=review > Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:428 > + if (position == GST_CLOCK_TIME_NONE) { I just added this extra condition. This solves the test regressions.
Comment on attachment 303672 [details] Patch Clearing flags on attachment: 303672 Committed r213572: <http://trac.webkit.org/changeset/213572>
All reviewed patches have been landed. Closing bug.