Bug 210341

Summary:

[GStreamer][MSE] Implement the "sequence" mode in SourceBuffer for the GStreamer ports

Product:

WebKit

Reporter:

Diego Pino <dpino>

Component:

Media

Assignee:

Enrique Ocaña <eocanha>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboya, calvaris, cgarcia, clopez, crzwdjk, eocanha, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, mrobinson, philipj, pnormand, sergio, vjaquez, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=210498
https://bugs.webkit.org/show_bug.cgi?id=167108

Bug Depends on:

Bug Blocks:

228976

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Diego Pino

Reported 2020-04-10 09:36:45 PDT

See: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r259858%20(13277)/imported/w3c/web-platform-tests/media-source/mediasource-sourcebuffer-mode-timestamps-crash-log.txt Thread 1 (Thread 0x7f006ffff700 (LWP 23222)): #0 0x00007f019f6ca243 in gst_gl_upload_transform_caps () at ../../Source/gst-plugins-base-1.16.1/gst-libs/gst/gl/gstglupload.c:1810 #1 gst_gl_upload_transform_caps () at ../../Source/gst-plugins-base-1.16.1/gst-libs/gst/gl/gstglupload.c:1783 #2 0x00007f019fa22751 in gst_base_transform_transform_caps () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:474 #3 0x00007f019fa25f4b in gst_base_transform_query_caps () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:650 #4 gst_base_transform_default_query () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:1550 #5 0x00007f019f925c40 in gst_pad_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4072 #6 0x00007f019f92639b in gst_pad_peer_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4204 #7 0x00007f019f95c8b8 in query_caps_func () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:2765 #8 0x00007f019f92480e in gst_pad_forward () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3008 #9 0x00007f019f95f37a in gst_pad_proxy_query_caps () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:2815 #10 0x00007f019f9249f4 in gst_pad_query_caps_default () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3187 #11 gst_pad_query_default () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3415 #12 0x00007f019f925c40 in gst_pad_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4072 #13 0x00007f019f92639b in gst_pad_peer_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4204 #14 0x00007f019f95c8b8 in query_caps_func () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:2765 #15 0x00007f019f92480e in gst_pad_forward () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3008 #16 0x00007f019f95f37a in gst_pad_proxy_query_caps () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:2815 #17 0x00007f019f9249f4 in gst_pad_query_caps_default () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3187 #18 gst_pad_query_default () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3415 #19 0x00007f019f925c40 in gst_pad_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4072 #20 0x00007f019f92639b in gst_pad_peer_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4204 #21 0x00007f019f95c8b8 in query_caps_func () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:2765 #22 0x00007f019f92480e in gst_pad_forward () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3008 #23 0x00007f019f95f37a in gst_pad_proxy_query_caps () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:2815 #24 0x00007f019f9249f4 in gst_pad_query_caps_default () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3187 #25 gst_pad_query_default () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3415 #26 0x00007f019f925c40 in gst_pad_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4072 #27 0x00007f019f92639b in gst_pad_peer_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4204 #28 0x00007f019f962114 in gst_pad_peer_query_caps () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:3107 #29 0x00007f019fa260ae in gst_base_transform_query_caps () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:678 #30 gst_base_transform_default_query () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:1550 #31 0x00007f019f925c40 in gst_pad_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4072 #32 0x00007f019f92639b in gst_pad_peer_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4204 #33 0x00007f019f962114 in gst_pad_peer_query_caps () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:3107 #34 0x00007f019fa260ae in gst_base_transform_query_caps () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:678 #35 gst_base_transform_default_query () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:1550 #36 0x00007f019f925c40 in gst_pad_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4072 #37 0x00007f019f92639b in gst_pad_peer_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4204 #38 0x00007f019f962114 in gst_pad_peer_query_caps () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:3107 #39 0x00007f019fa260ae in gst_base_transform_query_caps () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:678 #40 gst_base_transform_default_query () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:1550 #41 0x00007f019f925c40 in gst_pad_query () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:4072 #42 0x00007f019f95fb34 in gst_pad_query_caps () at ../../Source/gstreamer-1.16.1/gst/gstutils.c:3061 #43 0x00007f019fa2468f in gst_base_transform_find_transform () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:1123 #44 gst_base_transform_setcaps () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:1309 #45 0x00007f019fa2683d in gst_base_transform_sink_eventfunc () at ../../Source/gstreamer-1.16.1/libs/gst/base/gstbasetransform.c:1898 #46 0x00007f019f91e7c7 in gst_pad_send_event_unchecked () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:5766 #47 0x00007f019f91ece4 in gst_pad_push_event_unchecked () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:5411 #48 0x00007f019f91f154 in push_sticky () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3927 #49 0x00007f019f91cb48 in events_foreach () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:608 #50 0x00007f019f927e21 in check_sticky () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3986 #51 gst_pad_push_event () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:5542 #52 0x00007f019f9283b4 in event_forward_func () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3054 #53 0x00007f019f92480e in gst_pad_forward () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3008 #54 0x00007f019f92491d in gst_pad_event_default () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3105 #55 0x00007f014c1d4416 in gst_play_sink_convert_bin_sink_event () at ../../Source/gst-plugins-base-1.16.1/gst/playback/gstplaysinkconvertbin.c:260 #56 0x00007f019f91e7c7 in gst_pad_send_event_unchecked () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:5766 #57 0x00007f019f91ece4 in gst_pad_push_event_unchecked () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:5411 #58 0x00007f019f91f154 in push_sticky () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3927 #59 0x00007f019f91cb48 in events_foreach () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:608 #60 0x00007f019f927e21 in check_sticky () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:3986 #61 gst_pad_push_event () at ../../Source/gstreamer-1.16.1/gst/gstpad.c:5542 #62 0x00007f014c34011b in gst_queue_push_one () at ../../Source/gstreamer-1.16.1/plugins/elements/gstqueue.c:1455 #63 gst_queue_loop () at ../../Source/gstreamer-1.16.1/plugins/elements/gstqueue.c:1537 #64 0x00007f019f953dd1 in gst_task_func () at ../../Source/gstreamer-1.16.1/gst/gsttask.c:328 #65 0x00007f019d80d6a3 in g_thread_pool_thread_proxy () at ../../Source/glib-2.58.1/glib/gthreadpool.c:307 #66 0x00007f019d80cd45 in g_thread_proxy () at ../../Source/glib-2.58.1/glib/gthread.c:784 #67 0x00007f01a0b20fa3 in start_thread (arg=<optimized out>) at pthread_create.c:486 #68 0x00007f019d36d4cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Attachments
Patch (7.80 KB, patch) 2021-06-30 10:06 PDT, Enrique Ocaña	no flags	Details Formatted Diff Diff
Patch (14.70 KB, patch) 2021-07-30 08:22 PDT, Enrique Ocaña	no flags	Details Formatted Diff Diff
Patch (14.60 KB, patch) 2021-07-30 08:39 PDT, Enrique Ocaña	no flags	Details Formatted Diff Diff
Patch (21.57 KB, patch) 2021-08-24 10:48 PDT, Enrique Ocaña	no flags	Details Formatted Diff Diff
Patch (21.02 KB, patch) 2021-08-25 10:07 PDT, Enrique Ocaña	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Philippe Normand

Comment 1 2020-04-10 09:55:02 PDT

Consistent crash? Program terminated with signal SIGSEGV, Segmentation fault. #0 0x00007f019f6ca243 in gst_gl_upload_transform_caps () at ../../Source/gst-plugins-base-1.16.1/gst-libs/gst/gl/gstglupload.c:1810 1810 upload_methods[i]->transform_caps (upload->priv->upload_impl[i],

Diego Pino

Comment 2 2020-04-13 17:24:57 PDT

Crashes don't happen regularly, but some times. Other recent crashes happened in r258474 and r256096. Stacktrace for r258474 is the same as the one posted, r256096 seems unrelated. r258474: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r258474%20(12975)/imported/w3c/web-platform-tests/media-source/mediasource-sourcebuffer-mode-timestamps-crash-log.txt r256096: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r256096%20(12608)/imported/w3c/web-platform-tests/media-source/mediasource-sourcebuffer-mode-timestamps-crash-log.txt

Xabier Rodríguez Calvar

Comment 3 2021-04-07 07:17:38 PDT

I can't repro the crash though I do see some random crashes at Wayland wk2 Release with x86 bot.

Enrique Ocaña

Comment 4 2021-06-30 10:06:38 PDT

Created attachment 432608 [details] Patch

Enrique Ocaña

Comment 5 2021-06-30 10:10:18 PDT

I couldn't reproduce the crashes either, but this test was failing, so I submitted a patch to fix it.

Alicia Boya García

Comment 6 2021-06-30 11:36:17 PDT

Comment on attachment 432608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432608&action=review LGTM, except for what I commented. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:147 > + if (type.endsWith("mp4") || type.endsWith("aac") || type.endsWith("mpeg")) > m_demux = makeGStreamerElement("qtdemux", nullptr); There is a difference between container formats and audio formats. AAC audio can be packaged in a container such as MP4 -- in which case the caps will be audio/x-m4a or video/quicktime and would be handled by qtdemux), or it can be in a more barebones format, in which case the caps are audio/mpeg, mpegversion=4, stream-format=<one of "raw", "adts", "adif" or "loas">, in which case it can be fed directly to a decoder that understands that stream-format. Using qtdemux in the latter case would fail to negotiate caps. Conversions between stream-formats can be done with aacparse. Regardless of the stream-format used, they all have in common that audio frames are *NOT* timestamped, and therefore only usable in "sequence" mode -- where the JS user sets the timestamp of a segment before appending it. This is also the case for MP3, whose caps are audio/mpeg, mpegversion=1, layer=3. Currently, we don't support "sequence" mode, only "segments". I am not aware of how much "sequence" mode is used in web apps. In any case, creating a qtdemux for this context is wrong, but we should handle attempts to use the unsupported mode gracefully. Maybe by throwing an error instead of an assert, or by ensuring that an error is reported earlier than that when the "sequence" mode is engaged.

Enrique Ocaña

Comment 7 2021-07-30 08:22:23 PDT

Created attachment 434627 [details] Patch

Enrique Ocaña

Comment 8 2021-07-30 08:35:57 PDT

This patch implemented the "sequence" mode in SourceBuffer for the GStreamer port. I'm renaming the bug to reflect this.

Enrique Ocaña

Comment 9 2021-07-30 08:39:22 PDT

Created attachment 434632 [details] Patch

Alicia Boya García

Comment 10 2021-08-02 09:15:23 PDT

Comment on attachment 434632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434632&action=review It might make sense to rename m_demux to something more generic, like m_parser, which could be a demuxer or a filter parser (although that name may be already in use, which complicates things a bit). I would expect imported/w3c/web-platform-tests/media-source/mediasource-sequencemode-append-buffer.html to pass after implementing this feature. Can you check where it's failing? > Source/WebCore/ChangeLog:3 > + [GStreamer][MSE] Implemente the "sequence" mode in SourceBuffer for the GStreamer ports typo: Implement > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:147 > + bool hasIdentityDemuxer = false; mpegaudioparse is not an identity demuxer, it's a filter element, with static srcpads. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:154 > + m_demux = makeGStreamerElement("mpegaudioparse", nullptr); m_demux becomes a bit of a misleading case since in this case we have a parser that is not a demuxer.

Arcady Goldmints-Orlov

Comment 11 2021-08-11 12:52:40 PDT

*** Bug 228976 has been marked as a duplicate of this bug. ***

Philippe Normand

Comment 12 2021-08-16 04:46:48 PDT

Comment on attachment 434632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434632&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:153 > + else if (type == "audio/mpeg") { Depending on the mpegversion caps field I think you should either add mpegaudioparse or aacparse? mpegaudioparse supports only MPEG-1, AFAICT.

Philippe Normand

Comment 13 2021-08-17 01:46:23 PDT

(In reply to Arcady Goldmints-Orlov from comment #11) > *** Bug 228976 has been marked as a duplicate of this bug. *** The patch should un-flag the test gardened in that bug ^^

Enrique Ocaña

Comment 14 2021-08-24 10:13:39 PDT

I'm about to attach a new version of the patch, but before doing it I checked imported/w3c/web-platform-tests/media-source/mediasource-sequencemode-append-buffer.html (commented by Alicia) and media/media-source/media-mp4-h264-sequence-mode.html (referred by #228976, commented by Phil). In the first case, the test fails because of a 0.095 difference in the PTS. In the second case, the test fails because of a 0.041666 difference (even in appends done in "segments" mode!). I think both failures might be related to how the demuxer generates the PTS and if it implements edit lists properly. IMHO, this kind of problem is out of the scope of the basic sequence mode implemetnation addressed in this bug and should be analyzed independently in their respective bugs.

Enrique Ocaña

Comment 15 2021-08-24 10:48:22 PDT

Created attachment 436307 [details] Patch

Philippe Normand

Comment 16 2021-08-25 07:36:57 PDT

Comment on attachment 436307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436307&action=review > Source/WebCore/ChangeLog:13 > + aacparse for mpeg layer 3 and 4). The existing createOptionalParserForFormat() s/3/2

Alicia Boya García

Comment 17 2021-08-25 09:22:32 PDT

Comment on attachment 436307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436307&action=review > Source/WebCore/ChangeLog:12 > + A parser element is needed to process the data (mpegaudioparse for mpeg layer 1, MPEG Audio Version 1. MPEG version 1 includes layer 1, layer 2 and layer 3, the last one most commonly known as MP3. > Source/WebCore/ChangeLog:13 > + aacparse for mpeg layer 3 and 4). The existing createOptionalParserForFormat() MPEG Audio Versions 2 and 4, both commonly referred to as AAC. There is no concept of layers in MPEG Audio Versions 2 and 4, instead profiles are used. Yes, MPEG nomenclature is quite confusing. > Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp:199 > +void MediaSampleGStreamer::setTimestamps(const MediaTime& presentationTime, const MediaTime& decodeTime) > +{ > + m_pts = presentationTime; > + m_dts = decodeTime; > +} You're updating the timestamps in WebKit but not in GStreamer, which means they will played at the wrong time in the playback pipeline. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:182 > + GRefPtr<GstCaps> caps = gst_pad_get_current_caps(pad); > + if (!caps) > + return GST_PAD_PROBE_DROP; In the hypothetical case that a segment is sent before caps, it would be lost, which could confuse downstream. Maybe it makes more sense to drop any buffers before caps are received, but not anything else.

Enrique Ocaña

Comment 18 2021-08-25 09:54:44 PDT

Comment on attachment 436307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436307&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaSampleGStreamer.cpp:199 >> +} > > You're updating the timestamps in WebKit but not in GStreamer, which means they will played at the wrong time in the playback pipeline. This slip was indeed important. When I corrected it (locally), it fixed the use case of appending the same audio multiple times in reverse order (from higer to lower timestampOffsets) and having it playing back properly. Thanks for having noticed it! >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:182 >> + return GST_PAD_PROBE_DROP; > > In the hypothetical case that a segment is sent before caps, it would be lost, which could confuse downstream. Maybe it makes more sense to drop any buffers before caps are received, but not anything else. Have into account that it's a GST_PAD_PROBE_TYPE_BUFFER probe, and therefore shouldn't be processing (and dropping) events or anything that's not a buffer. The only possible buffers without caps that might be traversing the pipeline are the dummy ones that have EndOfAppendMeta and are used to detect the end of appends. Those buffers are captured and dropped in the appsrc src pad, and shouldn't ever reach the identity element that is in place of the demuxer in the case of uncontainerized audio.

Enrique Ocaña

Comment 19 2021-08-25 10:07:28 PDT

Created attachment 436400 [details] Patch

EWS

Comment 20 2021-08-26 02:30:09 PDT

Committed r281617 (240973@main): <https://commits.webkit.org/240973@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436400 [details].

Radar WebKit Bug Importer

Comment 21 2021-08-26 02:31:15 PDT

<rdar://problem/82380870>

Note You need to log in before you can comment on or make changes to this bug.