Bug 198569

Created attachment 371416 [details] Patch

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 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 :)

(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 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.

Created attachment 371492 [details] Patch

Created attachment 371493 [details] Patch

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.

(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.

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

<rdar://problem/51515570>