Bug 198569 – [GStreamer] AVC1 decoding capabilities probing support

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

The registry scanner should parse and handle "avc1.profile-level-info-here" codecs.

[Philippe Normand]

Created attachment 371416 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 371416 [details]
Patch

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

Lots of changes, event when they're purely cosmetic, I'd reupload the patch for review unless you're in a hurry.

> Source/WebCore/ChangeLog:13
> +        the codec will be advertized as supported if it complies with the contents of

AdvertiSe, weird case of American and British spelling agreeing.

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:295
> +    

Please, remove this initial space.

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:310
> +bool GStreamerRegistryScanner::areInputCapsAccepted(GList* factories, GRefPtr<GstCaps>&& caps) const

It does not matter a lot, but conceptually I think it would make more sense to use const GRefPtr<GstCaps>& and avoid moving in the caller since you don't need to take ownership of the caps, just "inspecting" them.

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:323
> +bool GStreamerRegistryScanner::isAVC1CodecSupported(String& codec) const

const String&

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:326
> +    long int spsAsInteger = strtol(components[1].utf8().data(), nullptr, 16);

Any reason to not use the to* methods of WTF::String?

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:327
> +    guint8 sps[3];

uint8_t

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:333
> +    const gchar* profile = gst_codec_utils_h264_get_profile(sps, 3);
> +    const gchar* level = gst_codec_utils_h264_get_level(sps, 3);

const char*

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:336
> +    if (const char* maxVideoResolution = getenv("WEBKIT_GST_MAX_AVC1_RESOLUTION")) {

g_getenv

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:337
> +        guint8 levelAsInteger = gst_codec_utils_h264_get_level_idc(level);

uint8_t

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:339
> +        guint8 maxLevel = 0;

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:344
> +        if (!strcmp(maxVideoResolution, "1080P"))
> +            maxLevel = 40;
> +        else if (!strcmp(maxVideoResolution, "720P"))
> +            maxLevel = 31;
> +        else if (!strcmp(maxVideoResolution, "480P"))

g_strcmp0

[Philippe Normand]

Comment on attachment 371416 [details]
Patch

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

Thanks, I've been reworking this a bit, I'll upload a new version :)

>> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:344
>> +        else if (!strcmp(maxVideoResolution, "480P"))
> 
> g_strcmp0

Why? There's a nice mix of strcmp and g_strcmp0 calls in the code currently. Here I see no reason to use g_strcmp0 because we're guaranteed maxVideoResolution is non-NULL :)

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #3)
> >> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:344
> >> +        else if (!strcmp(maxVideoResolution, "480P"))
> > 
> > g_strcmp0
> 
> Why? There's a nice mix of strcmp and g_strcmp0 calls in the code currently.
> Here I see no reason to use g_strcmp0 because we're guaranteed
> maxVideoResolution is non-NULL :)

Portability, security (just in case somebody messes up without us realizing) and it is not such a huge change anyway.

[Philippe Normand]

Comment on attachment 371416 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:326
>> +    long int spsAsInteger = strtol(components[1].utf8().data(), nullptr, 16);
> 
> Any reason to not use the to* methods of WTF::String?

I see no toLongInt() there.

[Philippe Normand]

Created attachment 371492 [details]
Patch

[Philippe Normand]

Created attachment 371493 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 371493 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:331
> +#define CHECK_H264_CAPS(capsString) do { \
> +    bool supported = false; \
> +    auto lookupResult = hasElementForMediaType(m_videoDecoderFactories, capsString, true); \
> +    supported = lookupResult; \
> +    if (shouldCheckForHardwareUse) \
> +        supported = lookupResult.isUsingHardware; \
> +    GST_DEBUG("%s decoding supported for codec %s: %s", shouldCheckForHardwareUse ? "Hardware" : "Software", codec.utf8().data(), boolForPrinting(supported)); \
> +    return supported; \
> +} while (0)

Creating and undeffing this macro looks a bit too convoluted to me. I'd either create a lambda inside this function or a static just before.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #8)
> Creating and undeffing this macro looks a bit too convoluted to me. I'd
> either create a lambda inside this function or a static just before.

Of course with a function you'd have to return the value of the function, but no big deal and code looks even better cause return points are explicit.

[Philippe Normand]

Committed r246194: <https://trac.webkit.org/changeset/246194>

[Radar WebKit Bug Importer]

<rdar://problem/51515570>