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

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted 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 333381: Patch

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




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

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

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

ERROR -> WARNING. caps is always plural so "these are not video caps".

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:75
> +	       pixelAspectRatioNumerator = pixelAspectRatioDenominator = 1;

This goes against coding style, one instruction at a line.

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

these are not video caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:100
> +	   return std::optional<FloatSize>();

You need to return std::nullopt here, otherwise asking for has_value will
answer always yes.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:123
> +    return std::optional<FloatSize>(FloatSize(width, height *
(static_cast<float>(pixelAspectRatioNumerator) /
static_cast<float>(pixelAspectRatioDenominator))));

You can use std::make_optional