Bug 231019 – [MSE][GStreamer] Honor MP4 edit lists

• Status: REOPENED
• Component: WebKitGTK

[Alicia Boya García]

This patch takes into consideration the GstSegment attached to a
sample to offset the PTS and DTS. This ensures accurate timestamps are
obtained for MP4 files containing edit lists (commonly necessary for
files containing video with B frames to have PTS starting at zero).

Before this was implemented, a workaround was in place based on a
heuristic (DTS = 0 && PTS > 0 && PTS < 0.1). The workaround is
preserved for the sake of content without proper edit lists, but
any edit list takes preference.

The time fudge factor has been modified from 0.083 seconds up to
0.100 seconds to accomodate the size of the empty edit in test.mp4
used by Web Platform Tests.

This test fixes improves expectation results and fixes two subtests in
imported/w3c/web-platform-tests/media-source/mediasource-remove.html.

[Alicia Boya García]

Created attachment 439726 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 439726 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:395
> +    {

Why do you open a scope here? I think you can remove it.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:436
> +            gst_sample_set_buffer(sample.get(), nullptr);

Do you need to set the buffer to null here if you're going to set it below again?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:441
> +            gst_sample_set_buffer(sample.get(), newBuffer.leakRef());

You're leaking here. gst_sample_set_buffer takes a transfer none buffer.

[Alicia Boya García]

Comment on attachment 439726 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:395
>> +    {
> 
> Why do you open a scope here? I think you can remove it.

To prevent accidental use of variables such as `buffer` after the new buffer is potentially created.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:436
>> +            gst_sample_set_buffer(sample.get(), nullptr);
> 
> Do you need to set the buffer to null here if you're going to set it below again?

The goal of doing this is ensuring that the buffer has 1 as ref count when gst_buffer_make_writable() is called if possible, enabling copy ellision.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:441
>> +            gst_sample_set_buffer(sample.get(), newBuffer.leakRef());
> 
> You're leaking here. gst_sample_set_buffer takes a transfer none buffer.

Good catch.

[Xabier Rodríguez Calvar]

Comment on attachment 439726 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:395
>>> +    {
>> 
>> Why do you open a scope here? I think you can remove it.
> 
> To prevent accidental use of variables such as `buffer` after the new buffer is potentially created.

I don't think this is necessary, at least for this reason. Anyway, I don't have a strong feeling against it. If you still think it is necessary, maybe adding a comment about it would be a good idea but if you add a comment for it, it might be a good idea to just add the comment without the scope (: . Anyway, your call.

>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:436
>>> +            gst_sample_set_buffer(sample.get(), nullptr);
>> 
>> Do you need to set the buffer to null here if you're going to set it below again?
> 
> The goal of doing this is ensuring that the buffer has 1 as ref count when gst_buffer_make_writable() is called if possible, enabling copy ellision.

Nice.

[Alicia Boya García]

Created attachment 440203 [details]
Patch

[EWS]

Committed r283609 (242561@main): <https://commits.webkit.org/242561@main>

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

[Alicia Boya García]

There are two issues with the current patch:

* gst_sample_set_buffer() is not available in gst 1.14, breaking builds for old distros.
* In at least one use case, frame corruption occurs, probably from a key frame being skipped. Needs further investigation.

Given these, I will revert the patch and work on a modified version that takes care of both issues.

[Alicia Boya García]

Created attachment 441049 [details]
Patch for landing

[EWS]

Committed r284087 (242915@main): <https://commits.webkit.org/242915@main>

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

[Alicia Boya García]

(reopening)

[Alicia Boya García]

Created attachment 441417 [details]
Patch

[Alicia Boya García]

Created attachment 441696 [details]
Patch

[EWS]

Committed r284711 (243426@main): <https://commits.webkit.org/243426@main>

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

[Michael Catanzaro]

I think this broke YouTube, see bug #233861.

[Philippe Normand]

Reopening because the patch was reverted, see https://bugs.webkit.org/show_bug.cgi?id=233861

[Enrique Ocaña]

Pull request: https://github.com/WebKit/WebKit/pull/1020

[EWS]

Committed r295286 (251332@main): <https://commits.webkit.org/251332@main>

Reviewed commits have been landed. Closing PR #1020 and removing active labels.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 244428

[Philippe Normand]

This patch unfortunately made seeking in YT highly unreliable :( Sometimes it works, sometimes it doesn't and an infinite spinner is showing up... In the gst logs a high volume of gap events is logged on the append pipeline sink... Very easy to reproduce on any YT video (vp9/opus/webm).