[webkit-reviews] review granted: [Bug 191191] [GStreamer] Decoding media-capabilities configuration initial support : [Attachment 362011] 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 191191: [GStreamer] Decoding media-capabilities configuration initial
support
https://bugs.webkit.org/show_bug.cgi?id=191191

Attachment 362011: Patch

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




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