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

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Zan Dobersek <zan at falconsigh.net> ---
Comment on attachment 304937
  --> https://bugs.webkit.org/attachment.cgi?id=304937
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170321/5bb54c9d/attachment-0001.html>