Bug #172787 added a new mediaContentTypesRequiringHardwareSupport setting to disable some multimedia codecs in case there's no accelerating hardware decoding available for them. Bug #191191 layed the ground for discriminating hardware supported codecs, but no GstRegistryScanner::areAllCodecsSupported() or GstRegistryScanner::isCodecSupported() call was actually passing the hardware support flag set to true (it was optional and set to false by default). Using only hardware supported video codecs is critical on embedded devices and undesired on desktop devices, so a proper implementation honoring the mediaContentTypesRequiringHardwareSupport setting both for MSE and the regular player in WebKitGTK/WPE is needed.
(In reply to Enrique Ocaña from comment #0) > Bug #172787 added a new mediaContentTypesRequiringHardwareSupport setting to > disable some multimedia codecs in case there's no accelerating hardware > decoding available for them. Bug #191191 layed the ground for discriminating > hardware supported codecs, but no > GstRegistryScanner::areAllCodecsSupported() or > GstRegistryScanner::isCodecSupported() call was actually passing the > hardware support flag set to true (it was optional and set to false by > default). Well there's one call-site where the flag is true: https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp#L397
Created attachment 399478 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
(In reply to Philippe Normand from comment #1) > (In reply to Enrique Ocaña from comment #0) > > Bug #172787 added a new mediaContentTypesRequiringHardwareSupport setting to > > disable some multimedia codecs in case there's no accelerating hardware > > decoding available for them. Bug #191191 layed the ground for discriminating > > hardware supported codecs, but no > > GstRegistryScanner::areAllCodecsSupported() or > > GstRegistryScanner::isCodecSupported() call was actually passing the > > hardware support flag set to true (it was optional and set to false by > > default). > > Well there's one call-site where the flag is true: > https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/ > graphics/gstreamer/GStreamerRegistryScanner.cpp#L397 Oh, I see, thanks. Somehow I had missing that place. Anyway, while that's useful for querying according to the media capabilities spec, it's of no use for the MediaSource::isTypeSupported(), MediaSource::addSourceBuffer() and the regular MediaPlayer selection when providing multiple video tracks. Those use cases are addressed in the patch submitted in this bug.
Created attachment 399480 [details] Patch
Created attachment 399481 [details] Patch
Comment on attachment 399481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399481&action=review The new API looks good to me. Thanks! > Source/WebCore/Modules/mediasource/MediaSource.cpp:685 > + if (!isTypeSupported(type, mediaContentTypesRequiringHardwareSupport)) I think you could move the Vector instead passing the ownership to isTypeSupported(() > Source/WebCore/Modules/mediasource/MediaSource.cpp:881 > + Document* document = downcast<Document>(&context); auto& document = downcast<Document>(context); > Source/WebCore/Modules/mediasource/MediaSource.cpp:885 > + return isTypeSupported(type, mediaContentTypesRequiringHardwareSupport); Move the vector here too. > Source/WebCore/Modules/mediasource/MediaSource.cpp:888 > +bool MediaSource::isTypeSupported(const String& type, const Vector<ContentType>& contentTypesRequiringHardwareSupport) And receive a Vector<ContentType>&& instead > Source/WebCore/Modules/mediasource/MediaSource.cpp:911 > + if (!contentTypesRequiringHardwareSupport.isEmpty()) > + parameters.contentTypesRequiringHardwareSupport.appendVector(contentTypesRequiringHardwareSupport); So here you can just move without copying. I don't think you need to check if it's empty. > Source/WebCore/Modules/mediasource/MediaSource.h:134 > + static bool isTypeSupported(const String& type, const Vector<ContentType>& contentTypesRequiringHardwareSupport); I think you can omit the Vector parameter name. > Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:303 > + for (auto contentTypeRequiringHardwareSupport : contentTypesRequiringHardwareSupport) { Vector::findMatching? > Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:330 > + for (auto codec : codecs) { const auto& codec : codecs > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1561 > + "", We normally use nullptr as default value for strings. > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3834 > + * Returns: Media content types requiring hardware support. , or %NULL > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3842 > + return settings->priv->preferences->mediaContentTypesRequiringHardwareSupport().utf8().data(); This is returning a temporary, we need to cache the CString in the WebKitSettings priv struct to be able to return a const char*. See how we handle the other string settings. > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3848 > + * @media_content_types_requiring_hardware_support: list of media content types requiring hardware support, split by semicolons (:). I think we can use a shorter parameter name, since we have enough context in the already long function name. So media_content_types should be enough, or event content_types > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3861 > + String mediaContentTypesRequiringHardwareSupportString = String::fromUTF8(mediaContentTypesRequiringHardwareSupport); > + settings->priv->preferences->setMediaContentTypesRequiringHardwareSupport(mediaContentTypesRequiringHardwareSupportString); > + g_object_notify(G_OBJECT(settings), "media-content-types-requiring-hardware-support"); You should check it has actually changed before setting it and emit the notify signal. > Source/WebKit/UIProcess/API/gtk/WebKitSettings.h:509 > +WEBKIT_API const gchar* const gchar* -> const gchar * > Source/WebKit/UIProcess/API/gtk/WebKitSettings.h:514 > + const gchar* media_content_types_requiring_hardware_support); Ditto, and properly lined up. > Source/WebKit/UIProcess/API/wpe/WebKitSettings.h:466 > +WEBKIT_API const gchar* Ditto. > Source/WebKit/UIProcess/API/wpe/WebKitSettings.h:471 > + const gchar* media_content_types_requiring_hardware_support); Ditto. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:344 > + // Default media content types requiring hardware support is "". Better use nullptr.
Comment on attachment 399481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399481&action=review >> Source/WebCore/Modules/mediasource/MediaSource.h:134 >> + static bool isTypeSupported(const String& type, const Vector<ContentType>& contentTypesRequiringHardwareSupport); > > I think you can omit the Vector parameter name. In this specific case, I don't think a developer can infer the meaning of the parameter without a meaningful name. The "requiring hardware support" name is key to know what you're supplying there. Otherwise you could understand that you can pass a Vector of ContentType to check if they're supported or something like that. If you don't mind, I'll keep the parameter name in this case. >> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:303 >> + for (auto contentTypeRequiringHardwareSupport : contentTypesRequiringHardwareSupport) { > > Vector::findMatching? Ok, but since refactoring for findMatching() makes the early returns useless and they were the main reason to have an independent function to compute the need for hardware support I'm now moving this code inside isContentTypeSupported(). >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:344 >> + // Default media content types requiring hardware support is "". > > Better use nullptr. I'm using nullptr as default in the property definition in the corrected patch, but the code actually gets the value from the underlying WebCore setting, which has "" as default. I've checked that now nullptr is going to be supported and is going to do what it should (revert to default when set), but the retrieved value will still be "" (the underlying default). Or should I add extra code to return nullptr when the inner value is ""?
Created attachment 399829 [details] Patch
(In reply to Enrique Ocaña from comment #8) > Comment on attachment 399481 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399481&action=review > > >> Source/WebCore/Modules/mediasource/MediaSource.h:134 > >> + static bool isTypeSupported(const String& type, const Vector<ContentType>& contentTypesRequiringHardwareSupport); > > > > I think you can omit the Vector parameter name. > > In this specific case, I don't think a developer can infer the meaning of > the parameter without a meaningful name. The "requiring hardware support" > name is key to know what you're supplying there. Otherwise you could > understand that you can pass a Vector of ContentType to check if they're > supported or something like that. If you don't mind, I'll keep the parameter > name in this case. > > >> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:303 > >> + for (auto contentTypeRequiringHardwareSupport : contentTypesRequiringHardwareSupport) { > > > > Vector::findMatching? > > Ok, but since refactoring for findMatching() makes the early returns useless > and they were the main reason to have an independent function to compute the > need for hardware support I'm now moving this code inside > isContentTypeSupported(). > > >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:344 > >> + // Default media content types requiring hardware support is "". > > > > Better use nullptr. > > I'm using nullptr as default in the property definition in the corrected > patch, but the code actually gets the value from the underlying WebCore > setting, which has "" as default. I've checked that now nullptr is going to > be supported and is going to do what it should (revert to default when set), > but the retrieved value will still be "" (the underlying default). Or should > I add extra code to return nullptr when the inner value is ""? Yes, if the CString is empty return nullptr
Created attachment 399833 [details] Patch
Comment on attachment 399833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399833&action=review Patch r=me. We need another reviewer to approve as this adds new API, but I am sure Carlos García will stop by and this can be landed momentarily :) > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1559 > + g_param_spec_string("media-content-types-requiring-hardware-support", Wow, this property name is a mouthful… OTOH, I do not have any ideas for a shorter name which would still carry all the meaning, so let's just go with this 🙃️
Comment on attachment 399833 [details] Patch lgtm
Comment on attachment 399833 [details] Patch Setting r+ as we have now two positive reviews :]
(In reply to Adrian Perez from comment #14) > Comment on attachment 399833 [details] > Patch > > Setting r+ as we have now two positive reviews :] (Also Carlos' comments have been all adressed, so I don't expect he would object to landing!)
(In reply to Adrian Perez from comment #15) > (In reply to Adrian Perez from comment #14) > > Comment on attachment 399833 [details] > > Patch > > > > Setting r+ as we have now two positive reviews :] > > (Also Carlos' comments have been all adressed, so I don't expect > he would object to landing!) Sure, LGTM. Thanks!
Committed r261986: <https://trac.webkit.org/changeset/261986> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399833 [details].
<rdar://problem/63482718>
Comment on attachment 399833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399833&action=review > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3854 > + * webkit_settings_set_media_content_types_requiring_hardware_support: > + * @settings: a #WebKitSettings > + * @content_types: (allow-none) list of media content types requiring hardware support split by semicolons (:) or %NULL to use the default value. > + * > + * Set the #WebKitSettings:media-content-types-requiring-hardware-support property. tbh I don't understand the documentation, what does it actually do? Does it cause software codecs to be disabled for these content types?
I'm not sure what to do about this: [2580/2713] Building CXX object Source...IProcess/API/glib/WebKitSettings.cpp.o /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1553:77: warning: "/*" within comment [-Wcomment] 1553 | * For example: 'video/webm; codecs="vp*":video/mp4; codecs="avc*":video/*; codecs="av1*"'. | It is currently the only build warning I see when building with GCC 10. Problem is that we literally want to get the /* into the generated gtk-doc. So normally I would attempt to suppress the warning using IGNORE_WARNINGS... but in this case, that is impossible, because the warning is actually generated by cpp, not by gcc. cpp warnings can only be suppressed via pragmas when compiling C, not when compiling C++. The two remaining solutions would be (a) build this file using -Wno-comment, or (b) change the example to avoid using /*, even if that would make the documentation less useful. (a) requires moving this file out of SourcesGTK.txt to PlatformGTK.cmake, so I'd prefer (b) if Enrique thinks it's OK to change the example.
Comment on attachment 399833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399833&action=review >> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3854 >> + * Set the #WebKitSettings:media-content-types-requiring-hardware-support property. > > tbh I don't understand the documentation, what does it actually do? Does it cause software codecs to be disabled for these content types? Yes, software codecs are disabled for these content types if there isn't a hardware decoder present. For desktop systems, the reasonable thing to do would be to keep this setting empty (play everything, because a desktop is supposed to have enough processing power to get by with software decoding). However, on embedded systems, that's not the case. The practical case that motivated this patch was a Raspberry Pi trying to play YouTube using vp9 software decoding (choppy) just because YouTube prefers that codec if "detected". The Raspberry has a hardware accelerated h264/avc decoder, so setting 'video/webm; codecs="vp9"' (better 'vp*' to match with vp9, vp09 and other variants) would prevent the vp9 sw decoder to be selected. Setting 'video/*' would prevent any sw video codec to be selected, only hw codecs. I hope it's clearer now. There's a full example in the documentation of the property: https://github.com/WebKit/webkit/blob/544abce992b5af06c77bf379411b6f7dd410c303/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp#L1553
(In reply to Michael Catanzaro from comment #20) > [2580/2713] Building CXX object > Source...IProcess/API/glib/WebKitSettings.cpp.o > /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/glib/ > WebKitSettings.cpp:1553:77: warning: "/*" within comment [-Wcomment] > 1553 | * For example: 'video/webm; codecs="vp*":video/mp4; > codecs="avc*":video/*; codecs="av1*"'. I found it before submitting the patch, tried to solve it, and ended up finding the same preprocessor limitation you talk about. In the end, I considered it a legitimate usage of "/*" and just moved on. I haven't tried, but... would it make sense to use "*" (the HTML entity of "*") instead? Or maybe there's a way to have a line break between "/" and "*" and get the doc building system join the lines in some way. The "video/*" is a really useful example. That's why I included it. If it ends up becoming a problem, I guess we can remove it.
I wonder if * would work, good idea. If not... since the example is useful, we don't have to remove it, we can move the source file to PlatformGTK.cmake and add -Wno-comment.
For some reason * didn't work, but * does work so let's go with that.
Reopening to attach new patch.
Created attachment 400355 [details] Patch for landing
Committed r262205: <https://trac.webkit.org/changeset/262205> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400355 [details].
(In reply to Michael Catanzaro from comment #25) > Reopening to attach new patch. Thanks for having taken care of this.