| Summary: | [GTK][GStreamer][MSE] Crash on youtube when MSE is enabled but gstreamer cant find the decoder element. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bugs-noreply, clopez, commit-queue, eocanha, mcatanzaro, vchipirras, zan | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 167107 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Carlos Alberto Lopez Perez
2017-01-17 09:36:21 PST
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. Created attachment 304936 [details]
Patch
Created attachment 304937 [details]
Patch
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. 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. Comment on attachment 304937 [details]
Patch
Please address the review feedback from Zan.
(And Enrique.) (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. Created attachment 309076 [details]
Patch
Created attachment 309524 [details]
Patch
Comment on attachment 309524 [details]
Patch
LGTM.
Comment on attachment 309524 [details]
Patch
Lets land this!
Comment on attachment 309524 [details] Patch Clearing flags on attachment: 309524 Committed r217045: <http://trac.webkit.org/changeset/217045> All reviewed patches have been landed. Closing bug. |