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.
Created attachment 439726 [details] Patch
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.
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.
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.
Created attachment 440203 [details] Patch
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].
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.
Created attachment 441049 [details] Patch for landing
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].
(reopening)
Created attachment 441417 [details] Patch
Created attachment 441696 [details] Patch
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].
I think this broke YouTube, see bug #233861.
Reopening because the patch was reverted, see https://bugs.webkit.org/show_bug.cgi?id=233861
Pull request: https://github.com/WebKit/WebKit/pull/1020
Committed r295286 (251332@main): <https://commits.webkit.org/251332@main> Reviewed commits have been landed. Closing PR #1020 and removing active labels.
Re-opened since this is blocked by bug 244428
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).