[libcamera-devel] [PATCH v4 1/1] gstreamer: Provide mulitple colorimetry support

[Nicolas Dufresne]

Hi Rishi,

I like this version, I've provided some very minor feedback. With these
improved:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>

Le mercredi 17 août 2022 à 22:55 +0530, Rishikesh Donadkar a écrit :
> Iterate over each GValue in the colorimetry list,
> retrieve the colorimetry string, convert to colorspace using the
> function colorspace_from_colorimetry() and validate the camera configuration.
> Retrieve the colorspace after validation, convert to colorimetry and
> check if the it is same as the colorimetry requested.
> 
> If none of the colorimetry requested is supported by the camera (i.e. not
> the same after validation) then set the stream_cfg to the previous
> configuration that was present before trying new colorimetry.
> 
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 75 +++++++++++++++++++++-------
>  src/gstreamer/gstlibcamera-utils.h   |  4 +-
>  src/gstreamer/gstlibcamerasrc.cpp    |  2 +-
>  3 files changed, 62 insertions(+), 19 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index d895a67d..2a88444c 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -302,13 +302,39 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
>  	return caps;
>  }
>  
> +static int

Looking at the code, it seems like this is a boolean, not sure if hiding that
fact behind a int is usual in libcamera, but wanted to mention this looks a
little strange.

> +configure_colorspace_from_caps(StreamConfiguration &stream_cfg,
> +			       const gchar *colorimetry_str)
> +{
> +	gint colorimetry_valid = 1, colorimetry_invalid = 0;
> +	if (colorimetry_str) {
> +		GstVideoColorimetry colorimetry;
> +
> +		if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) {
> +			g_critical("Invalid colorimetry %s", colorimetry_str);
> +			return colorimetry_invalid;
> +		}
> +
> +		stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);
> +		/* Check if colorimetry had any identifiers which did not map */
> +		if (colorimetry.primaries != GST_VIDEO_COLOR_PRIMARIES_UNKNOWN &&
> +		    stream_cfg.colorSpace == ColorSpace::Raw) {
> +			GST_ERROR("One or more identifiers could not be mapped for %s colorimetry",
> +				  colorimetry_str);
> +			stream_cfg.colorSpace = std::nullopt;
> +		}
> +	}
> +	return colorimetry_valid;

The variable does not seem to be written too, compiler will optimize it out, but
still, I'd return true/false, or use constant or defines if readability was your
goal.

> +}
> +
>  void
> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> +gst_libcamera_configure_stream_from_caps(std::unique_ptr<CameraConfiguration> &cam_cfg,
> +					 StreamConfiguration &stream_cfg,
>  					 GstCaps *caps)
>  {
>  	GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
>  	guint i;
> -	gint best_fixed = -1, best_in_range = -1;
> +	gint best_fixed = -1, best_in_range = -1, ret;
>  	GstStructure *s;
>  
>  	/*
> @@ -381,23 +407,38 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>  	stream_cfg.size.width = width;
>  	stream_cfg.size.height = height;
>  
> -	/* Configure colorimetry */
> -	if (gst_structure_has_field(s, "colorimetry")) {
> -		const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry");
> -		GstVideoColorimetry colorimetry;
> -
> -		if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
> -			g_critical("Invalid colorimetry %s", colorimetry_str);
> -
> -		stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);
> -		/* Check if colorimetry had any identifiers which did not map */
> -		if (colorimetry.primaries != GST_VIDEO_COLOR_PRIMARIES_UNKNOWN &&
> -		    stream_cfg.colorSpace == ColorSpace::Raw) {
> -			GST_ERROR("One or more identifiers could not be mapped for %s colorimetry",
> -				  colorimetry_str);
> -			stream_cfg.colorSpace = std::nullopt;
> +	const GValue *colorimetry = gst_structure_get_value(s, "colorimetry");
> +	if (!colorimetry)
> +		return;
> +
> +	/* Configure Colorimetry */
> +	if (GST_VALUE_HOLDS_LIST(colorimetry)) {
> +		StreamConfiguration pristine_stream_cfg = stream_cfg;
> +		for (i = 0; i < gst_value_list_get_size(colorimetry); i++) {
> +			const GValue *val = gst_value_list_get_value(colorimetry, i);
> +			GstVideoColorimetry colorimetry_old, colorimetry_new;
> +
> +			ret = configure_colorspace_from_caps(stream_cfg, g_value_get_string(val));
> +			if (!ret)
> +				continue;
> +
> +			colorimetry_old = colorimetry_from_colorspace(stream_cfg.colorSpace.value());
> +
> +			/* Validate the configuration and check if the requested
> +			 * colorimetry can be applied to the sensor.
> +			 */
> +			if (cam_cfg->validate() != CameraConfiguration::Invalid) {
> +				colorimetry_new = colorimetry_from_colorspace(stream_cfg.colorSpace.value());
> +				if (gst_video_colorimetry_is_equal(&colorimetry_old, &colorimetry_new)) {
> +					g_print("Selected colorimetry %s\n", gst_video_colorimetry_to_string(&colorimetry_new));
> +					break;
> +				} else
> +					stream_cfg = pristine_stream_cfg;
> +			}
>  		}
>  	}
> +	if (G_VALUE_TYPE(colorimetry) == G_TYPE_STRING)

nit: Alternatively, G_VALUE_HOLDS_STRING(colorimetry). This branch could be
"else if", as it mutually exclusive to the previous branch. A final else could
be nice to trace that something is wrong, colorimetry field type should only be
string or list, anything else is user an error (though coming from outside the
element/plugin).

> +		configure_colorspace_from_caps(stream_cfg, g_value_get_string(colorimetry));
>  }
>  
>  #if !GST_CHECK_VERSION(1, 17, 1)
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 164189a2..7afad576 100644
> --- a/src/gstreamer/gstlibcamera-utils.h
> +++ b/src/gstreamer/gstlibcamera-utils.h
> @@ -8,6 +8,7 @@
>  
>  #pragma once
>  
> +#include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/stream.h>
>  
> @@ -16,7 +17,8 @@
>  
>  GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);
>  GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);
> -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,
> +void gst_libcamera_configure_stream_from_caps(std::unique_ptr<libcamera::CameraConfiguration> &cam_cfg,
> +					      libcamera::StreamConfiguration &stream_cfg,
>  					      GstCaps *caps);
>  #if !GST_CHECK_VERSION(1, 17, 1)
>  gboolean gst_task_resume(GstTask *task);
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 16d70fea..25059914 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -503,7 +503,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  
>  		/* Fixate caps and configure the stream. */
>  		caps = gst_caps_make_writable(caps);
> -		gst_libcamera_configure_stream_from_caps(stream_cfg, caps);
> +		gst_libcamera_configure_stream_from_caps(state->config_, stream_cfg, caps);
>  	}
>  
>  	if (flow_ret != GST_FLOW_OK)