Bug 198569

Summary: [GStreamer] AVC1 decoding capabilities probing support
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch calvaris: review+, calvaris: commit-queue-

Description Philippe Normand 2019-06-05 09:23:08 PDT
The registry scanner should parse and handle "avc1.profile-level-info-here" codecs.
Comment 1 Philippe Normand 2019-06-05 10:01:43 PDT
Created attachment 371416 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2019-06-06 04:36:12 PDT
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
Comment 3 Philippe Normand 2019-06-06 05:21:31 PDT
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 :)
Comment 4 Xabier Rodríguez Calvar 2019-06-06 06:15:57 PDT
(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.
Comment 5 Philippe Normand 2019-06-06 06:34:58 PDT
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.
Comment 6 Philippe Normand 2019-06-06 06:35:27 PDT
Created attachment 371492 [details]
Patch
Comment 7 Philippe Normand 2019-06-06 06:36:59 PDT
Created attachment 371493 [details]
Patch
Comment 8 Xabier Rodríguez Calvar 2019-06-06 07:28:18 PDT
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.
Comment 9 Xabier Rodríguez Calvar 2019-06-06 07:29:37 PDT
(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.
Comment 10 Philippe Normand 2019-06-07 02:00:19 PDT
Committed r246194: <https://trac.webkit.org/changeset/246194>
Comment 11 Radar WebKit Bug Importer 2019-06-07 02:01:28 PDT
<rdar://problem/51515570>