Bug 190143 – [MSE][GStreamer] Add h264parse to accept MP4 without stss

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

The MP4 file used in this URL does not contain a stss (Sync Sample
Box). In consequence, in acordance with the ISO BMFF spec, all samples
are assumed to be sync frames... But in this case that is not true,
it's just that the file is wrong (e.g. created with a buggy muxer).

http://orange-opensource.github.io/hasplayer.js/1.2.0/player.html?url=http://playready.directtaps.net/smoothstreaming/SSWSS720H264/SuperSpeedway_720.ism/Manifest

The way it works in other browsers is because instead of trusting the
MP4 stss table, they rely on parsing the h264 frames. We can do that
too, using h264parse.

[Alicia Boya García]

Created attachment 351262 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 351262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351262&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:872
> +        RELEASE_ASSERT(h264parse);

Please, add a GST_ERROR, ASSERT_NOT_REACHED and return nullptr in case there is no parser. Better a not playing element than a web process crass.

And we should do the same for vorbisparse.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #2)
> And we should do the same for vorbisparse.

I don't mean we need to do it in this bug, it can be another. If you decide to do it in this bug, which I won't oppose, make it explicit in the changelog.

[Philippe Normand]

A long-term and generic solution could be to use parsebin.

[Alicia Boya García]

Created attachment 351361 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 351361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351361&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:872
> +        GstElement* h264parse = gst_element_factory_make("h264parse", parserName.get());
> +        g_return_val_if_fail(h264parse, nullptr);

ASSERT on the element as well to get a crash in debug.

[Alicia Boya García]

(In reply to Xabier Rodríguez Calvar from comment #6)
> Comment on attachment 351361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351361&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:872
> > +        GstElement* h264parse = gst_element_factory_make("h264parse", parserName.get());
> > +        g_return_val_if_fail(h264parse, nullptr);
> 
> ASSERT on the element as well to get a crash in debug.

You can use G_DEBUG=fatal-criticals for that.

[Alicia Boya García]

Created attachment 351367 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351367 [details]
Patch

Clearing flags on attachment: 351367

Committed r236735: <https://trac.webkit.org/changeset/236735>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alicia Boya García]

Minor correction: it's not actually about a stss box because fragmented files use the `sample_is_difference_sample` flag of `trun` instead; but otherwise the problem is the same.