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

• Component: Media
• Status: RESOLVED FIXED

[Diego Pino]

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

[Philippe Normand]

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]

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]

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

[Enrique Ocaña]

Created attachment 432608 [details]
Patch

[Enrique Ocaña]

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 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]

Created attachment 434627 [details]
Patch

[Enrique Ocaña]

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

[Enrique Ocaña]

Created attachment 434632 [details]
Patch

[Alicia Boya García]

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]

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

[Philippe Normand]

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]

(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]

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]

Created attachment 436307 [details]
Patch

[Philippe Normand]

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 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 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]

Created attachment 436400 [details]
Patch

[EWS]

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]

<rdar://problem/82380870>