Bug 167120 – [GTK][GStreamer][MSE] Crash on youtube when MSE is enabled but gstreamer cant find the decoder element.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Alberto Lopez Perez]

With WebKitGTK+ trunk (r210800):


1. Move gstreamer h264 decoders outside of your gst plugin path
2. Tools/jhbuild/jhbuild-wrapper --gtk run  gst-inspect-1.0 |grep 264|grep -i decoder
3. In my case i moved WebKitBuild/DependenciesGTK/Root/lib/gstreamer-1.0/libgstopenh264.so and WebKitBuild/DependenciesGTK/Root/lib/gstreamer-1.0/libgstlibav.so to /tmp
4. Repeat step 2, ensure no 264 decoders are available.

5. Open youtube with MSE support

$ Tools/Scripts/run-minibrowser --gtk --release  --enable-mediasource=true https://www.youtube.com/watch?v=iNJdPyoqt8U
Starting MiniBrowser.

(MiniBrowser:20425): GLib-GObject-WARNING **: The property GtkToolButton:stock-id is deprecated and shouldn't be used anymore. It will be removed in a future version.
GLib-GIO-Message: Using the 'memory' GSettings backend.  Your settings will not be saved or shared with other applications.
1   0x7ff682156c06 /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(+0x11c3c06) [0x7ff682156c06]
2   0x7ff682156bf6 /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(+0x11c3bf6) [0x7ff682156bf6]
3   0x7ff68308eb3b /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(+0x20fbb3b) [0x7ff68308eb3b]
4   0x7ff6829a3335 /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(WebCore::HTMLMediaElement::removeAudioTrack(WebCore::AudioTrack&)+0x15) [0x7ff6829a3335]
5   0x7ff68254721e /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(WebCore::MediaSource::removeSourceBuffer(WebCore::SourceBuffer&)+0xde) [0x7ff68254721e]
6   0x7ff6825475d5 /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(WebCore::MediaSource::detachFromElement(WebCore::HTMLMediaElement&)+0x125) [0x7ff6825475d5]
7   0x7ff68299f09c /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(WebCore::HTMLMediaElement::noneSupported()+0x28c) [0x7ff68299f09c]
8   0x7ff68299ba84 /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(WebCore::HTMLMediaElement::mediaLoadingFailed(WebCore::MediaPlayerEnums::NetworkState)+0xc4) [0x7ff68299ba84]
9   0x7ff68299f4ea /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(non-virtual thunk to WebCore::HTMLMediaElement::mediaPlayerNetworkStateChanged(WebCore::MediaPlayer*)+0x2a) [0x7ff68299f4ea]
10  0x7ff6830bc2be /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(WebCore::MediaPlayerPrivateGStreamer::handleMessage(_GstMessage*)+0x9ce) [0x7ff6830bc2be]
11  0x7ff67b0e5ad3 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libgobject-2.0.so.0(g_cclosure_marshal_VOID__BOXEDv+0x73) [0x7ff67b0e5ad3]
12  0x7ff67b0e2f34 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libgobject-2.0.so.0(+0xff34) [0x7ff67b0e2f34]
13  0x7ff67b0fc048 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libgobject-2.0.so.0(g_signal_emit_valist+0x898) [0x7ff67b0fc048]
14  0x7ff67b0fc942 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libgobject-2.0.so.0(g_signal_emit+0x82) [0x7ff67b0fc942]
15  0x7ff67c62aa12 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libgstreamer-1.0.so.0(gst_bus_async_signal_func+0x82) [0x7ff67c62aa12]
16  0x7ff67c62b836 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libgstreamer-1.0.so.0(+0x40836) [0x7ff67c62b836]
17  0x7ff67afe1cad /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_context_dispatch+0x13d) [0x7ff67afe1cad]
18  0x7ff67afe2048 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x49048) [0x7ff67afe2048]
19  0x7ff67afe2362 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_loop_run+0xc2) [0x7ff67afe2362]
20  0x7ff6801ac70c /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(WTF::RunLoop::run()+0x9c) [0x7ff6801ac70c]
21  0x7ff68245dee2 /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**)+0xa2) [0x7ff68245dee2]
22  0x7ff678820b45 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7ff678820b45]
23  0x400cb9 /home/clopez/webkit/webkit/WebKitBuild/Release/bin/WebKitWebProcess() [0x400cb9]

[Carlos Alberto Lopez Perez]

Also opening https://www.youtube.com/html5?gl=GB without those codecs shows H264 as not available, but it shows MSE&H264 as available.

I think it should also show MSE&H264 as not available when the H264 decoder is not there.

[Vanessa Chipirrás Navalón]

Created attachment 304936 [details]
Patch

[Vanessa Chipirrás Navalón]

Created attachment 304937 [details]
Patch

[Enrique Ocaña]

Comment on attachment 304937 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:113
> +bool doInitializeGStreamerAndRegisterWebKitElements()

I'd thank feedback from some reviewer about if this is the best way to expose the original initializeGStreamerAndRegisterWebKitElements() internal function to be called from another internal function in MediaPlayerPrivateGStreamer.cpp.

If resorting to declare a friend function is too cumbersome, maybe a more direct approach would be to just declare the original initializeGStreamerAndRegisterWebKitElements() publicly in the header file. I'm still not sure.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:776
> +        Vector<GstCapsWebKitMapping> mapping = {

It's great that you have removed the old elements not mapping to any webkitCodecs.

Now the AudioDecoder elementType category has no elements. Still, I think keeping this AudioDecoder/VideoDecoder distinction is useful in case we add support for more mime types in MSE in the future.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:837
> +        for (String pattern : codecSet()) {

If you use "const auto&" here instead of String, you're avoiding String copies and increasing the performance.

[Zan Dobersek]

Comment on attachment 304937 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:113
>> +bool doInitializeGStreamerAndRegisterWebKitElements()
> 
> I'd thank feedback from some reviewer about if this is the best way to expose the original initializeGStreamerAndRegisterWebKitElements() internal function to be called from another internal function in MediaPlayerPrivateGStreamer.cpp.
> 
> If resorting to declare a friend function is too cumbersome, maybe a more direct approach would be to just declare the original initializeGStreamerAndRegisterWebKitElements() publicly in the header file. I'm still not sure.

IMO this should be a public static function in the MediaPlayerPrivateGStreamerBase class. That way there's no need to declare friendship in the MediaPlayerPrivateGStreamerMSE class.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:755
> +static HashSet<String, ASCIICaseInsensitiveHash>& codecSet()

Should return a const reference.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:759
> +        callInitializeGStreamerAndRegisterWebKitElements();

This would be a direct call to MediaPlayerPrivateGStreamerBase::initializeGStreamerAndRegisterWebKitElements().

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:780
> +        Vector<GstCapsWebKitMapping> mapping = {
> +            {VideoDecoder, "video/x-h264,  profile=(string){ constrained-baseline, baseline }", {"x-h264"}},
> +            {VideoDecoder, "video/x-h264, stream-format=avc", {"avc*"}},
> +            {VideoDecoder, "video/mpeg, mpegversion=(int){1,2}, systemstream=(boolean)false", {"mpeg"}},
> +        };

This should be a std::array<>.

The brace-initialization should have spaces at after the opening and before the closing brace: { VideoDecoder, ..., { "x-h264" } },

Why AtomicStrings? Regardless whether you use AtomicStrings or ordinary Strings, you should intialize them with AtomicString("...", AtomicString::ConstructFromLiteral) or ASCIILiteral("...").

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:787
> +            GList* factories = nullptr;
> +            if (current.elementType == AudioDecoder)
> +                factories = audioDecoderFactories;
> +            else if (current.elementType == VideoDecoder)
> +                factories = videoDecoderFactories;

This doesn't handle the Demuxer value, which would leave the factories GList null and cause problems in gstRegistryHasElementForMediaType(). But as it stands the Demuxer value isn't used in the mapping array.

If it's not needed, it should be removed from the enum. If it's needed, it should be handled here. Either way, this if-elseif block should be a switch statement that handles all the values in the enum.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:811
> +            set.add(AtomicString("audio/mpeg"));
> +            set.add(AtomicString("audio/x-mpeg"));

Again regarding using AtomicStrings -- you should use Strings all the way if the target HashSet has String values. But don't forget to use ASCIILiteral directly.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:814
> +
> +

Nit: one extraneous newline.

[Michael Catanzaro]

Comment on attachment 304937 [details]
Patch

Please address the review feedback from Zan.

[Michael Catanzaro]

(And Enrique.)

[Vanessa Chipirrás Navalón]

(In reply to Zan Dobersek from comment #5)
> Comment on attachment 304937 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304937&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:113
> >> +bool doInitializeGStreamerAndRegisterWebKitElements()
> > 
> > I'd thank feedback from some reviewer about if this is the best way to expose the original initializeGStreamerAndRegisterWebKitElements() internal function to be called from another internal function in MediaPlayerPrivateGStreamer.cpp.
> > 
> > If resorting to declare a friend function is too cumbersome, maybe a more direct approach would be to just declare the original initializeGStreamerAndRegisterWebKitElements() publicly in the header file. I'm still not sure.
> 
> IMO this should be a public static function in the
> MediaPlayerPrivateGStreamerBase class. That way there's no need to declare
> friendship in the MediaPlayerPrivateGStreamerMSE class.
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:755
> > +static HashSet<String, ASCIICaseInsensitiveHash>& codecSet()
> 
> Should return a const reference.
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:759
> > +        callInitializeGStreamerAndRegisterWebKitElements();
> 
> This would be a direct call to
> MediaPlayerPrivateGStreamerBase::
> initializeGStreamerAndRegisterWebKitElements().
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:780
> > +        Vector<GstCapsWebKitMapping> mapping = {
> > +            {VideoDecoder, "video/x-h264,  profile=(string){ constrained-baseline, baseline }", {"x-h264"}},
> > +            {VideoDecoder, "video/x-h264, stream-format=avc", {"avc*"}},
> > +            {VideoDecoder, "video/mpeg, mpegversion=(int){1,2}, systemstream=(boolean)false", {"mpeg"}},
> > +        };
> 
> This should be a std::array<>.
> 
> The brace-initialization should have spaces at after the opening and before
> the closing brace: { VideoDecoder, ..., { "x-h264" } },
> 
> Why AtomicStrings? Regardless whether you use AtomicStrings or ordinary
> Strings, you should intialize them with AtomicString("...",
> AtomicString::ConstructFromLiteral) or ASCIILiteral("...").
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:787
> > +            GList* factories = nullptr;
> > +            if (current.elementType == AudioDecoder)
> > +                factories = audioDecoderFactories;
> > +            else if (current.elementType == VideoDecoder)
> > +                factories = videoDecoderFactories;
> 
> This doesn't handle the Demuxer value, which would leave the factories GList
> null and cause problems in gstRegistryHasElementForMediaType(). But as it
> stands the Demuxer value isn't used in the mapping array.
> 
> If it's not needed, it should be removed from the enum. If it's needed, it
> should be handled here. Either way, this if-elseif block should be a switch
> statement that handles all the values in the enum.
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:811
> > +            set.add(AtomicString("audio/mpeg"));
> > +            set.add(AtomicString("audio/x-mpeg"));
> 
> Again regarding using AtomicStrings -- you should use Strings all the way if
> the target HashSet has String values. But don't forget to use ASCIILiteral
> directly.
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:814
> > +
> > +
> 
> Nit: one extraneous newline.

OK Zan, thanks for the review. I am going to apply those changes in my patch.

[Vanessa Chipirrás Navalón]

Created attachment 309076 [details]
Patch

[Vanessa Chipirrás Navalón]

Created attachment 309524 [details]
Patch

[Zan Dobersek]

Comment on attachment 309524 [details]
Patch

LGTM.

[Carlos Alberto Lopez Perez]

Comment on attachment 309524 [details]
Patch

Lets land this!

[WebKit Commit Bot]

Comment on attachment 309524 [details]
Patch

Clearing flags on attachment: 309524

Committed r217045: <http://trac.webkit.org/changeset/217045>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.