[webkit-reviews] review granted: [Bug 198569] [GStreamer] AVC1 decoding capabilities probing support : [Attachment 371416] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 198569: [GStreamer] AVC1 decoding capabilities probing support
https://bugs.webkit.org/show_bug.cgi?id=198569

Attachment 371416: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 371416
  --> https://bugs.webkit.org/attachment.cgi?id=371416
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