[libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple colorimetry support for libcamerasrc

[Laurent Pinchart]

Hi Rishikesh,

Thank you for the patch.

On Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote:
> Iterate over each GValue in the colorimetry list,
> retrieve the colorimetry string, convert to colorspace,
> set the colorspace in the  stream_cfg and validate the camera configuration.
> Retrieve the colorspace after validation, convert to colorimetry and
> check if 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>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++-----
>  src/gstreamer/gstlibcamera-utils.h   |  4 ++-
>  src/gstreamer/gstlibcamerasrc.cpp    |  2 +-
>  3 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 244a4a79..f73b6349 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -316,9 +316,9 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
>  	return caps;
>  }
>  
> -void
> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> -					 GstCaps *caps)
> +void 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;
> @@ -395,15 +395,46 @@ 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;
> +	const GValue *colorimetry_ = gst_structure_get_value(s, "colorimetry");

Trailing underscores are reserved for class members.

> +	if (!colorimetry_)
> +		return;
>  
> -		if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
> -			g_critical("Invalid colorimetry %s", colorimetry_str);
> +	/* Configure Colorimetry */
> +	GstVideoColorimetry 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);
> +
> +			if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) {
> +				g_warning("Invalid colorimetry %s, trying next option", g_value_get_string(val));
> +				continue;
> +			}
> +
> +			std::optional<ColorSpace> clrSpace = colorspace_from_colorimetry(colorimetry);
> +			stream_cfg.colorSpace = clrSpace;
> +
> +			/* Validate the configuration and check if the requested

Wrong style. On this topic, lines are too long. Line wraps can help, but
it may also indicate that some code could be split to separate functions
(I haven't checked).

> +			 * colorimetry can be applied to the sensor. Otherwise,
> +			 * restore the configuration back to pristine one.
> +			 */
> +			if (cam_cfg->validate() != CameraConfiguration::Invalid) {

The RPi pipeline handler (and other pipeline handlers) requires the same
color space to be used for all streams, and it will pick (if I recall
correctly) the color space of the largest non-raw stream and apply it to
all other streams. For multi-stream use cases, validating the color
space on streams individually will probably not work as you're
expecting. You'll need a more complex logic.

> +				/* Check the colorspace returned after validation, with the colorspace before validation */
> +				if (stream_cfg.colorSpace == clrSpace) {
> +					g_print("Selected colorimetry %s\n", gst_video_colorimetry_to_string(&colorimetry));
> +					break;
> +				} else {
> +					stream_cfg = pristine_stream_cfg;
> +				}
> +			}
> +		}
> +	} else if (G_VALUE_HOLDS_STRING(colorimetry_)) {
> +		if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_)))
> +			g_critical("Invalid colorimetry %s", g_value_get_string(colorimetry_));
>  
>  		stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);

Why does this not use validate() ? The asymmetry between the two makes
me think something is wrong.

> +	} else {
> +		GST_WARNING("colorimetry field type should only be string or list.");
>  	}
>  }
>  
> 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,

Let's pass a normal reference to the function:

void gst_libcamera_configure_stream_from_caps(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);

		gst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps);

>  	}
>  
>  	if (flow_ret != GST_FLOW_OK)

-- 
Regards,

Laurent Pinchart