Bug 169202 – [GStreamer][MSE] Actually implement flush() on playback pipeline

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Enrique Ocaña]

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.

[Enrique Ocaña]

Created attachment 303511 [details]
Patch

[Zan Dobersek]

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

[Enrique Ocaña]

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

[Enrique Ocaña]

Created attachment 303547 [details]
Patch

[Zan Dobersek]

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

[Zan Dobersek]

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

[Enrique Ocaña]

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

[Enrique Ocaña]

Created attachment 303672 [details]
Patch

[Enrique Ocaña]

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.

[WebKit Commit Bot]

Comment on attachment 303672 [details]
Patch

Clearing flags on attachment: 303672

Committed r213572: <http://trac.webkit.org/changeset/213572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.