Bug 211950 - [GStreamer][GTK][WPE] Expose and honor the media content types requiring hardware support setting
Summary: [GStreamer][GTK][WPE] Expose and honor the media content types requiring hard...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrique Ocaña
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-15 04:24 PDT by Enrique Ocaña
Modified: 2020-05-21 00:48 PDT (History)
18 users (show)

See Also:


Attachments
Patch (24.25 KB, patch)
2020-05-15 08:02 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (24.26 KB, patch)
2020-05-15 08:26 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (25.04 KB, patch)
2020-05-15 08:38 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (25.66 KB, patch)
2020-05-20 04:48 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (25.44 KB, patch)
2020-05-20 06:32 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrique Ocaña 2020-05-15 04:24:52 PDT
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.
Comment 1 Philippe Normand 2020-05-15 07:21:16 PDT
(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
Comment 2 Enrique Ocaña 2020-05-15 08:02:23 PDT
Created attachment 399478 [details]
Patch
Comment 3 EWS Watchlist 2020-05-15 08:03:17 PDT
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
Comment 4 Enrique Ocaña 2020-05-15 08:16:52 PDT
(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.
Comment 5 Enrique Ocaña 2020-05-15 08:26:13 PDT
Created attachment 399480 [details]
Patch
Comment 6 Enrique Ocaña 2020-05-15 08:38:05 PDT
Created attachment 399481 [details]
Patch
Comment 7 Carlos Garcia Campos 2020-05-16 02:42:41 PDT
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 8 Enrique Ocaña 2020-05-20 04:44:04 PDT
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 ""?
Comment 9 Enrique Ocaña 2020-05-20 04:48:16 PDT
Created attachment 399829 [details]
Patch
Comment 10 Carlos Garcia Campos 2020-05-20 05:25:29 PDT
(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
Comment 11 Enrique Ocaña 2020-05-20 06:32:49 PDT
Created attachment 399833 [details]
Patch
Comment 12 Adrian Perez 2020-05-20 07:00:10 PDT
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 13 Xabier Rodríguez Calvar 2020-05-20 07:20:37 PDT
Comment on attachment 399833 [details]
Patch

lgtm
Comment 14 Adrian Perez 2020-05-20 07:32:04 PDT
Comment on attachment 399833 [details]
Patch

Setting r+ as we have now two positive reviews :]
Comment 15 Adrian Perez 2020-05-20 07:33:10 PDT
(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!)
Comment 16 Carlos Garcia Campos 2020-05-20 08:08:59 PDT
(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!
Comment 17 EWS 2020-05-21 00:47:10 PDT
Committed r261986: <https://trac.webkit.org/changeset/261986>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399833 [details].
Comment 18 Radar WebKit Bug Importer 2020-05-21 00:48:14 PDT
<rdar://problem/63482718>