[webkit-reviews] review granted: [Bug 210341] [GStreamer] imported/w3c/web-platform-tests/media-source/mediasource-sourcebuffer-mode-timestamps.html sometimes crashes : [Attachment 432608] Patch

[bugzilla-daemon at webkit.org]

Alicia Boya García <aboya at igalia.com> has granted Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 210341: [GStreamer]
imported/w3c/web-platform-tests/media-source/mediasource-sourcebuffer-mode-time
stamps.html sometimes crashes
https://bugs.webkit.org/show_bug.cgi?id=210341

Attachment 432608: Patch

https://bugs.webkit.org/attachment.cgi?id=432608&action=review




--- Comment #6 from Alicia Boya García <aboya at igalia.com> ---
Comment on attachment 432608
  --> https://bugs.webkit.org/attachment.cgi?id=432608
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.