Bug 214252 – [MSE][GStreamer] Discard PTS-less samples

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

In some cases GStreamer demuxers emit PTS-less samples with metadata
at the beginning of the container. These are fortunately not necessary
for playback, and in fact incompatible with the way MSE works, where
you should be able to start playing a stream from the middle.

This patch skips these frames in the AppendPipeline so they don't
pollute other parts of the MSE codebase.

Since these frames were not necessary and were later overwritten,
this patch is just a cleanup introducing no notable behavior changes.

[Alicia Boya García]

Created attachment 404138 [details]
Patch

[Alicia Boya García]

This a cherry picked change from the WebKitMediaSrc rework patch.

[Philippe Normand]

Comment on attachment 404138 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464
> +        // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those.

If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination?

[Alicia Boya García]

Comment on attachment 404138 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464
>> +        // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those.
> 
> If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination?

Not really. Imagine there was another container+format case where this happens, skipping these frames (which are MSE-incompatible) would still be the best outcome. Adding a further check would add more complexity for no gain: regardless of the format where they are emitted, the root problem is they are "metadata" samples without a presentation time.

[Philippe Normand]

Comment on attachment 404138 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464
>>> +        // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those.
>> 
>> If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination?
> 
> Not really. Imagine there was another container+format case where this happens, skipping these frames (which are MSE-incompatible) would still be the best outcome. Adding a further check would add more complexity for no gain: regardless of the format where they are emitted, the root problem is they are "metadata" samples without a presentation time.

I don't really agree, I prefer to be explicit in this case. But I don't have the energy to discuss about this.
Anyway, this doesn't impact live streams broadcasted on youtube.com/live?

[Alicia Boya García]

Comment on attachment 404138 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464
>>>> +        // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those.
>>> 
>>> If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination?
>> 
>> Not really. Imagine there was another container+format case where this happens, skipping these frames (which are MSE-incompatible) would still be the best outcome. Adding a further check would add more complexity for no gain: regardless of the format where they are emitted, the root problem is they are "metadata" samples without a presentation time.
> 
> I don't really agree, I prefer to be explicit in this case. But I don't have the energy to discuss about this.
> Anyway, this doesn't impact live streams broadcasted on youtube.com/live?

I see no benefit in adding more conditions, for the reasons explained above. The only case in which it could make sense to me would be if I asserted on the caps inside the `if` block (e.g. this only happens for Vorbis streams), but it feels a bit overkill.

It doesn't impact live streams. Live streams frames contain PTS just like regular videos do. All frames in MSE need a PTS. You can indeed open a developer console and inspect `$("video").buffered` in a YouTube live to see how this is the case. The only difference with live streams is that there is not necessarily a frame with PTS=0, implementations may make the live start at whatever PTS they want.

[EWS]

Committed r264299: <https://trac.webkit.org/changeset/264299>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404138 [details].