<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK][GStreamer][MSE] Crash on youtube when MSE is enabled but gstreamer cant find the decoder element."
href="https://bugs.webkit.org/show_bug.cgi?id=167120#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK][GStreamer][MSE] Crash on youtube when MSE is enabled but gstreamer cant find the decoder element."
href="https://bugs.webkit.org/show_bug.cgi?id=167120">bug 167120</a>
from <span class="vcard"><a class="email" href="mailto:zan@falconsigh.net" title="Zan Dobersek <zan@falconsigh.net>"> <span class="fn">Zan Dobersek</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=304937&action=diff" name="attach_304937" title="Patch">attachment 304937</a> <a href="attachment.cgi?id=304937&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=304937&action=review">https://bugs.webkit.org/attachment.cgi?id=304937&action=review</a>
<span class="quote">>> 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.</span >
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.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:755
> +static HashSet<String, ASCIICaseInsensitiveHash>& codecSet()</span >
Should return a const reference.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:759
> + callInitializeGStreamerAndRegisterWebKitElements();</span >
This would be a direct call to MediaPlayerPrivateGStreamerBase::initializeGStreamerAndRegisterWebKitElements().
<span class="quote">> 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"}},
> + };</span >
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("...").
<span class="quote">> 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;</span >
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.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:811
> + set.add(AtomicString("audio/mpeg"));
> + set.add(AtomicString("audio/x-mpeg"));</span >
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.
<span class="quote">> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:814
> +
> +</span >
Nit: one extraneous newline.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>