[libcamera-devel] [PATCH v2 2/4] gstreamer: Update the obtained colorimetry in caps.

[Vedant Paranjape]

Hello Rishikesh,

Thanks for the patch,

On Thu, Jul 7, 2022 at 11:45 AM Rishikesh Donadkar
<rishikeshdonadkar at gmail.com> wrote:
>
> If the colorspace is set in the StreamConfiguration, get the colorimetry as
> a result of conversion from the previous patch and update it into the caps.

this sounds better:

get the colorimetry after converting colorspace using functions
from the previous patch and update it into the GStreamer caps.

>
> If the colorimetry corresponding to the colorspace set in the
> StreamConfiguration is not available in GStreamer set the colorimetry
> field to nullptr in the caps (this will fail the negotiation).
>
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 60ac8c8e..eb9c49da 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -166,11 +166,26 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
>  {
>         GstCaps *caps = gst_caps_new_empty();
>         GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat);
> +       gchar *colorimetry = nullptr;
> +       std::optional<ColorSpace> colorspace = stream_cfg.colorSpace;
>
>         gst_structure_set(s,
>                           "width", G_TYPE_INT, stream_cfg.size.width,
>                           "height", G_TYPE_INT, stream_cfg.size.height,
>                           nullptr);
> +
> +       if (colorspace) {

You might want to use

> colorspace.has_value()

Even though it's one and the same, this seems more verbose. Just a
personal preference, anyone has comments about this ?

> +               colorimetry = colorimerty_from_colorspace(colorspace);
> +               if (colorimetry) {
> +                       gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr);
> +               } else {
> +                       gst_structure_set(s, "colorimetry", G_TYPE_STRING, nullptr, nullptr);
> +                       g_free(colorimetry);
> +               }
> +       } else {
> +               g_free(colorimetry);
> +       }
> +

This if-else block can be further simplified as follows:

<snip>
if (colorspace) {
    colorimetry = colorimerty_from_colorspace(colorspace);
    gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry, nullptr);
    g_free(colorimetry);
}
</snip>

Regards,
Vedant Paranjape


+
+       if (colorspace) {
+               colorimetry = colorimerty_from_colorspace(co
lorspace);
+               if (colorimetry) {
+                       gst_structure_set(s, "colorimetry",
G_TYPE_STRING, colorimetry, nullptr);
+               } else {
+                       gst_structure_set(s, "colorimetry",
G_TYPE_STRING, nullptr, nullptr);
+                       g_free(colorimetry);
+               }
+       } else {
+               g_free(colorimetry);
+       }
+

>         gst_caps_append_structure(caps, s);
>
>         return caps;
> --
> 2.25.1
>