Bug 231019

Summary: [MSE][GStreamer] Honor MP4 edit lists
Product: WebKit Reporter: Alicia Boya García <aboya>
Component: WebKitGTKAssignee: Enrique Ocaña <eocanha>
Status: REOPENED    
Severity: Normal CC: bugs-noreply, calvaris, cgarcia, commit-queue, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, philipj, philn, pnormand, sergio, vjaquez
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=233861
Bug Depends on: 231708, 244428    
Bug Blocks: 270622    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch none

Alicia Boya García
Reported 2021-09-30 04:31:22 PDT
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.
Attachments
Patch (14.43 KB, patch)
2021-09-30 04:34 PDT, Alicia Boya García 		no flags
DetailsFormatted DiffDiff
Patch (14.17 KB, patch)
2021-10-05 05:45 PDT, Alicia Boya García 		no flags
DetailsFormatted DiffDiff
Patch for landing (12.73 KB, patch)
2021-10-13 02:28 PDT, Alicia Boya García 		no flags
DetailsFormatted DiffDiff
Patch (13.26 KB, patch)
2021-10-15 13:12 PDT, Alicia Boya García 		no flags
DetailsFormatted DiffDiff
Patch (13.28 KB, patch)
2021-10-19 00:59 PDT, Alicia Boya García 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alicia Boya García
Comment 1 2021-09-30 04:34:30 PDT
Created attachment 439726 [details] Patch
Xabier Rodríguez Calvar
Comment 2 2021-09-30 07:41:38 PDT
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 3 2021-10-01 06:27:33 PDT
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 4 2021-10-01 07:05:31 PDT
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
Comment 5 2021-10-05 05:45:01 PDT
Created attachment 440203 [details] Patch
EWS
Comment 6 2021-10-06 02:48:46 PDT
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
Comment 7 2021-10-13 01:54:20 PDT
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
Comment 8 2021-10-13 02:28:42 PDT
Created attachment 441049 [details] Patch for landing
EWS
Comment 9 2021-10-13 03:21:57 PDT
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
Comment 10 2021-10-13 06:24:48 PDT
(reopening)
Alicia Boya García
Comment 11 2021-10-15 13:12:33 PDT
Created attachment 441417 [details] Patch
Alicia Boya García
Comment 12 2021-10-19 00:59:49 PDT
Created attachment 441696 [details] Patch
EWS
Comment 13 2021-10-22 13:22:59 PDT
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
Comment 14 2022-03-08 18:08:30 PST
I think this broke YouTube, see bug #233861.
Philippe Normand
Comment 15 2022-03-12 01:39:48 PST
Reopening because the patch was reverted, see https://bugs.webkit.org/show_bug.cgi?id=233861
Enrique Ocaña
Comment 16 2022-05-25 11:15:34 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1020
EWS
Comment 17 2022-06-06 07:35:36 PDT
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
Comment 18 2022-08-27 05:10:31 PDT
Re-opened since this is blocked by bug 244428
Philippe Normand
Comment 19 2022-08-27 05:16:02 PDT
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).
Enrique Ocaña
Comment 20 2025-05-16 06:10:45 PDT
Hmmm... but... the improvement of edit list support was mostly needed for mp4/h264. Maybe we can make have the support in this patch depending on the container and codec, so it fixes the problematic cases it indended to fix (mp4/h264) and leaves the other ones (webm/vp9, webm/opus) intact. I'm commenting this because this patch would be needed for https://bugs.webkit.org/show_bug.cgi?id=270622.
Enrique Ocaña
Comment 21 2025-06-10 11:47:28 PDT
Pull request: https://github.com/WebKit/WebKit/pull/46557
Note You need to log in before you can comment on or make changes to this bug.