Bug 191191 – [GStreamer] Decoding media-capabilities configuration initial support

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

.

[Philippe Normand]

Created attachment 353693 [details]
Patch

[Philippe Normand]

Comment on attachment 353693 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353693&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:575
> +    auto& gstRegistryScanner = GstRegistryScanner::singleton();

oops should use MSE scanner here. Will fix up

[Philippe Normand]

Created attachment 353695 [details]
Patch

[Philippe Normand]

Created attachment 353700 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 353700 [details]
Patch

As spoken with Phil in private, it looks like this patch has issues so it needs more work. Removing flag.

[Philippe Normand]

Created attachment 353707 [details]
Patch

[Philippe Normand]

Comment on attachment 353707 [details]
Patch

webkit-patch screwed up

[Philippe Normand]

Created attachment 353714 [details]
Patch

[Philippe Normand]

Created attachment 353838 [details]
Patch

[Philippe Normand]

EWS should process the patch now. Please review!

[Xabier Rodríguez Calvar]

Comment on attachment 353838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353838&action=review

There are still some things, mainly style I'd like to discuss, but it's overall a very nice rework! Thx!

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:23
> +#include "GstRegistryScanner.h"

This include should be just below config.h

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:53
> +    GST_DEBUG("%s registry scanner initialized", m_isMediaSource ? "MSE":"non-MSE");

"MSE"  : "Regular playback", mind the space around the semicolon

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:57
> +        GST_DEBUG("%s codec pattern registered: %s", item.value ? "Hardware":"Software", item.key.string().utf8().data());

mind the space around the semicolon

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:70
> +GstRegistryScanner::RegistryLookupResult GstRegistryScanner::gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier)

shouldCheckHardwareClassifier

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:75
> +    bool supported = candidates;
> +    bool usingHardware = false;

isSupported
isUsingHardware

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:274
> +bool GstRegistryScanner::supportsCodec(String codec, bool usingHardware) const

isUsingHardware or from what I understand, shouldCheckForHardwareUse

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:285
> +            return usingHardware ? item.value : true;

An alternative way could be: return !usingHardware || item.value;

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:288
> +    GST_LOG("Unsupported");

You're logging only if a codec is not supported and I think it would expect all checks to be logged. Maybe something like :

...

bool supported { false };
for (...) {
   if (!fnmatch(...)) { // Note you don't need the variable
        supported = !usingHardware || item.value;
        break;
    }
}

GST_DEBUG("Checked %s codec \"%s\" supported %s", usingHardware ? "hardware" : "software", codec.utf8().data(), boolForPrinting(supported));

return supported;

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:292
> +bool GstRegistryScanner::supportsAllCodecs(const Vector<String>& codecs, bool usingHardware) const

ditto

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:302
> +bool GstRegistryScanner::isDecodingSupported(MediaConfiguration& configuration, bool* usingHardware) const

From what I see in this patch, we always check for hardware when calling this, so a better signature would be:

RegistryLookupResult GstRegisterScanner::isDecodingSupported(MediaConfiguracion& configuration) const

Either that our use std::pair<bool, bool>.

Of course, please get rid of all checks for nullptr.

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:47
> +    bool supportsCodec(String codec, bool usingHardware = false) const;
> +    bool supportsAllCodecs(const Vector<String>& codecs, bool usingHardware = false) const;

isCodecSupported
areAllCodecsSupported

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:70
> +        bool usingHardware;

Considering that there are some methods returning this that can answer without checking for hardware support (if requested in the function signature) I would type this as std::optional<bool> so that you can leave it as nullopt if you are not checking for it.

Actually I don't kind of like the assumption of considering a bunch of codecs just as software because we don't check them, I think we should consider them as hardware support unknown. This is not a strong request, just a suggestion.

If you still prefer to consider unknown hardware support as false, I would recommend to turn this into an enum like NotSupported, SoftwareSupported, HardwareSupported.

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:74
> +    RegistryLookupResult gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier = false);

I think doesHaveElementForMediaType is a better name. I don't understand why you prefixed the class name here.

> Source/WebCore/platform/graphics/gstreamer/MediaEngineConfigurationFactoryGStreamer.cpp:57
> +    bool usingHardware = false;

isUsingHardware

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2244
> +        if (codecs.isEmpty())
> +            result = MediaPlayer::MayBeSupported;
> +        else
> +            result = gstRegistryScanner.supportsAllCodecs(codecs) ? MediaPlayer::IsSupported : MediaPlayer::IsNotSupported;

We can write it as result = codecs.isEmpty() ? ... : ...

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:167
> +    if (type.endsWith("mp4") || type.endsWith("aac"))

This seems to be new, out of refactor scope, right? why?

[Philippe Normand]

Comment on attachment 353838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353838&action=review

>> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:285
>> +            return usingHardware ? item.value : true;
> 
> An alternative way could be: return !usingHardware || item.value;

How is it better? I think I prefer mine because it's more explicit :)

>> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:70
>> +        bool usingHardware;
> 
> Considering that there are some methods returning this that can answer without checking for hardware support (if requested in the function signature) I would type this as std::optional<bool> so that you can leave it as nullopt if you are not checking for it.
> 
> Actually I don't kind of like the assumption of considering a bunch of codecs just as software because we don't check them, I think we should consider them as hardware support unknown. This is not a strong request, just a suggestion.
> 
> If you still prefer to consider unknown hardware support as false, I would recommend to turn this into an enum like NotSupported, SoftwareSupported, HardwareSupported.

I'm sorry, I don't see what's wrong with this code :)

>> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:74
>> +    RegistryLookupResult gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier = false);
> 
> I think doesHaveElementForMediaType is a better name. I don't understand why you prefixed the class name here.

As many things in this patch, this is the result of a refactoring. gstRegistryHasElementForMediaType was the name used for the previous implementation in GStreamerCommon.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:167
>> +    if (type.endsWith("mp4") || type.endsWith("aac"))
> 
> This seems to be new, out of refactor scope, right? why?

As documented in the ChangeLog:
* platform/graphics/gstreamer/mse/AppendPipeline.cpp: Ditto. Also
        plug qtdemux for AAC containers, this is an explicit consequence
        of finer-grained codecs probing.

[Philippe Normand]

Created attachment 362011 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 362011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review

I will answer your comments in another entry.

> Source/WebCore/platform/GStreamer.cmake:31
> +        platform/graphics/gstreamer/mse/GstRegistryScannerMSE.cpp

I think we should call this GStreamerRegistryScanner.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1621
> +String convertEnumerationToString(MediaPlayerEnums::SupportsType enumerationValue)

You could most probably make this a const String&, but we could leave this for a follow up patch for this and the rest of overloads.

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:25
> +#include "GRefPtrGStreamer.h"

I'd recommend GStreamerCommon.h instead.

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73
> +    bool isSupported = candidates;

If I am not wrong you can move this below.

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:77
> +        for (GList* factories = candidates; factories != nullptr; factories = g_list_next(factories)) {

!factories

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:145
> +        m_codecMap.add(AtomicString("mpeg"), false);
> +        m_codecMap.add(AtomicString("mp4a*"), false);

Why are we always assuming that this does not have hardware support? Isn't it better to check it the result for hardware support? Same for opus and vorbis, speex, etc

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:278
> +        codec = codec.substring(slashIndex+1);

spaces around +

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:310
> +        GST_DEBUG("Checking support for video configuration: \"%s\" size: %ux%u bitrate: %" G_GUINT64_FORMAT " framerate: %f", videoConfiguration.contentType.utf8().data(), videoConfiguration.width, videoConfiguration.height, videoConfiguration.bitrate, videoConfiguration.framerate);

This line is too long

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:321
> +        GST_DEBUG("Checking support for audio configuration: \"%s\" %s channels, bitrate: %" G_GUINT64_FORMAT " samplerate: %u", audioConfiguration.contentType.utf8().data(), audioConfiguration.channels.utf8().data(), audioConfiguration.bitrate, audioConfiguration.samplerate);

ditto

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:43
> +    bool supportsContainerType(String containerType) const { return m_mimeTypeSet.contains(containerType); }

isContainerTypeSupported

> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:47
> +        bool supported;
> +        bool usingHardware;

isSupported and isUsingHardware.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #12)
> >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:285
> >> +            return usingHardware ? item.value : true;
> > 
> > An alternative way could be: return !usingHardware || item.value;
> 
> How is it better? I think I prefer mine because it's more explicit :)

Just a preference, your call.

> >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:70
> >> +        bool usingHardware;
> > 
> > Considering that there are some methods returning this that can answer without checking for hardware support (if requested in the function signature) I would type this as std::optional<bool> so that you can leave it as nullopt if you are not checking for it.
> > 
> > Actually I don't kind of like the assumption of considering a bunch of codecs just as software because we don't check them, I think we should consider them as hardware support unknown. This is not a strong request, just a suggestion.
> > 
> > If you still prefer to consider unknown hardware support as false, I would recommend to turn this into an enum like NotSupported, SoftwareSupported, HardwareSupported.
> 
> I'm sorry, I don't see what's wrong with this code :)

What I see wrong here is two things. The first I already commented about inline in the new patch, for some codec you're not checking if they have hardware support, just assuming false.

The second is that IMHO, I think you shouldn't assume false. IMHO you should have three values, Unknown, No and Yes. Another way of having three values is using WTF::Optional and considering nullopt as non-checked.

If you still think having three values is not useful, my initial recommendation was to use an enum NotSupported, SoftwareSupported and HardwareSupported but now that I had a closed look to the result struct, I find it nicer than before so I won't tell you anything about this.

Summing up, I don't understand why you don't always initialize hardware support and assume false in some cases or consider having three values if you don't always check for hardware. This is my opinion about the code, I won't object if you land it as it is now either.

> >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.h:74
> >> +    RegistryLookupResult gstRegistryHasElementForMediaType(GList* elementFactories, const char* capsString, bool checkHardwareClassifier = false);
> > 
> > I think doesHaveElementForMediaType is a better name. I don't understand why you prefixed the class name here.
> 
> As many things in this patch, this is the result of a refactoring.
> gstRegistryHasElementForMediaType was the name used for the previous
> implementation in GStreamerCommon.

Yes, I know it is a refactoring and a nice one. But this being a refactoring is not a reason to unprefix a function when you move it inside a class with other unprefixed methods. Please change this.

> >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:167
> >> +    if (type.endsWith("mp4") || type.endsWith("aac"))
> > 
> > This seems to be new, out of refactor scope, right? why?
> 
> As documented in the ChangeLog:
> * platform/graphics/gstreamer/mse/AppendPipeline.cpp: Ditto. Also
>         plug qtdemux for AAC containers, this is an explicit consequence
>         of finer-grained codecs probing.

Ok.

[Philippe Normand]

I'm checking for hardware support only for video decoders because the gst element klass Hardware classifier is currently used only for video decoders and moreover, audio decoding usually doesn't rely on specific hardware anyway (please correct me if this is wrong)

[Philippe Normand]

Comment on attachment 362011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review

>> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73
>> +    bool isSupported = candidates;
> 
> If I am not wrong you can move this below.

Just before the gst_plugin_feature_list_free() then? It doesn't make much difference, I also prefer to have both vars initialized at the same place. It's a nit anyway :)

>> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:77
>> +        for (GList* factories = candidates; factories != nullptr; factories = g_list_next(factories)) {
> 
> !factories

s/!// :)

>> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:145
>> +        m_codecMap.add(AtomicString("mp4a*"), false);
> 
> Why are we always assuming that this does not have hardware support? Isn't it better to check it the result for hardware support? Same for opus and vorbis, speex, etc

Because audio decoders don't require special hardware

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #16)
> I'm checking for hardware support only for video decoders because the gst
> element klass Hardware classifier is currently used only for video decoders
> and moreover, audio decoding usually doesn't rely on specific hardware
> anyway (please correct me if this is wrong)

Never checked that but it you think so, feel free to keep it.

(In reply to Philippe Normand from comment #17)
> >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73
> >> +    bool isSupported = candidates;
> > 
> > If I am not wrong you can move this below.
> 
> Just before the gst_plugin_feature_list_free() then? It doesn't make much
> difference, I also prefer to have both vars initialized at the same place.
> It's a nit anyway :)

WebKit coding style, right?

> >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:77
> >> +        for (GList* factories = candidates; factories != nullptr; factories = g_list_next(factories)) {
> > 
> > !factories
> 
> s/!// :)

True!

> >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:145
> >> +        m_codecMap.add(AtomicString("mp4a*"), false);
> > 
> > Why are we always assuming that this does not have hardware support? Isn't it better to check it the result for hardware support? Same for opus and vorbis, speex, etc
> 
> Because audio decoders don't require special hardware

[Philippe Normand]

(In reply to Xabier Rodríguez Calvar from comment #18)
> 
> (In reply to Philippe Normand from comment #17)
> > >> Source/WebCore/platform/graphics/gstreamer/GstRegistryScanner.cpp:73
> > >> +    bool isSupported = candidates;
> > > 
> > > If I am not wrong you can move this below.
> > 
> > Just before the gst_plugin_feature_list_free() then? It doesn't make much
> > difference, I also prefer to have both vars initialized at the same place.
> > It's a nit anyway :)
> 
> WebKit coding style, right?
> 

https://webkit.org/code-style-guidelines/ doesn't mention this.

The style bot is green as well.

[Philippe Normand]

Committed r241585: <https://trac.webkit.org/changeset/241585>

[Radar WebKit Bug Importer]

<rdar://problem/48109225>

[Alicia Boya García]

Comment on attachment 362011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:146
> +    if (type.endsWith("mp4") || type.endsWith("aac"))

audio/x-aac? Where have you found that?

[Philippe Normand]

Comment on attachment 362011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362011&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:146
>> +    if (type.endsWith("mp4") || type.endsWith("aac"))
> 
> audio/x-aac? Where have you found that?

why are you talking about x-aac? IIRC I changed this because some MSE layout tests were failing after my changes.