[webkit-reviews] review denied: [Bug 181990] [EME][GStreamer] Add support for the encrypted Caps in GStreamerUtilities : [Attachment 332984] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Yacine Bandou
<bandou.yacine at gmail.com>'s request for review:
Bug 181990: [EME][GStreamer] Add support for the encrypted Caps in
GStreamerUtilities
https://bugs.webkit.org/show_bug.cgi?id=181990

Attachment 332984: Patch

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




--- Comment #11 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 332984
  --> https://bugs.webkit.org/attachment.cgi?id=332984
Patch

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

> Source/WebCore/ChangeLog:10
> +	   Add the support of encrypted Caps in GStreamerUtilities.
> +	   Refactor the manner that the Caps are handled, such as how to
extract the resolution
> +	   from the video Caps or how to check if the Caps are encrypted.

caps can be non-capital

> Source/WebCore/ChangeLog:24
> +

Extra line not needed.

> Source/WebCore/ChangeLog:28
> +	   (WebCore::areEncryptedCaps): Add a new functions in order to handle
the Caps properly.

caps

> Source/WebCore/ChangeLog:29
> +

Extra line not needed.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:63
> +	   GST_ERROR("Failed to get the video size and foramt, they are not a
video caps");

foramt -> format

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:68
> +

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:70
> +	   gint width = 0, height = 0;

gint -> int and variables need to be moved to just before where they are used.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:76
> +	   gst_structure_get_fraction(structure, "pixel-aspect-ratio",
&pixelAspectRatioNumerator, &pixelAspectRatioDenominator);

Assign these two variables to 1 before calling this.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:80
> +

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:97
> +bool getVideoResolutionFromCaps(GstCaps* caps, float& width, float& height)

>From what I see about this function it is only used to populate FloatSize
attributes. Please turn it into std::optional<FloatSize>
videoResolutionFromCaps(const GstCaps* caps)

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:100
> +    gint lWidth = 0, lHeight = 0;
> +    gint pixelAspectRatioNumerator = 1, pixelAspectRatioDenominator = 1;

Move this variables to where they are used.

gint -> int

l* -> gstreamer*

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:103
> +	   GST_ERROR("Failed to get the video resolution, they are not a video
caps");

ERROR -> WARNING

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:108
> +

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:115
>  

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:126
> +    }
> +    width = lWidth;

You love not needed extra lines and here, where you could use one, you don't :)

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:127
> +    height = lHeight * ((float) pixelAspectRatioNumerator / (float)
pixelAspectRatioDenominator);

use c++ casts.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:169
> +const char* getMediaTypeFromCaps(GstCaps* caps)

getMediaTypeFromCaps -> capsMediaType and make it tame const caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:176
> +    if (!structure) {
> +	   GST_ERROR("caps are empty");
> +	   return nullptr;
> +    }

Did you ever find any case where this error was hit? I doubt it, so I think it
is better to just ASSERT on this. If this is something that really can happen
sometimes, we should not just ERROR, it looks the kind of situation that should
be managed by the caller instead, so we could WARNING here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:184
> +bool capsIsType(GstCaps* caps, const char* type)

capsIsType -> doCapsHaveType and make it take const caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:188
> +	   GST_ERROR("Failed to get MediaType");

Contrary to the other two functions here, I don't think we should ASSERT here,
but ERROR is too string, let's do WARNING.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:194
> +bool areEncryptedCaps(GstCaps* caps)

const caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:202
> +    if (!structure) {
> +	   GST_ERROR("caps are empty");
> +	   return false;
> +    }

Same as in capsMediaType.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:55
> +#define GST_VIDEO_CAPS_TYPE	"video/"
> +#define GST_AUDIO_CAPS_TYPE	"audio/"
> +#define GST_TEXT_CAPS_TYPE	"text/"

GST_*_CAPS_TYPE_PREFIX

>
Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:57
> +    GRefPtr<GstCaps> originalCap = m_caps;

originalCaps

>
Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:64
> +	       return String();

you can return AtomicString() here.

>
Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:86
> +	       codecName.remove(braceStart, braceEnd-braceStart);

braceEnd - braceStart