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

[Nicolas Dufresne]

Le lundi 15 août 2022 à 19:01 +0530, Rishikesh Donadkar a écrit :
> Append copy of the structure with best suitable resolution into a
> fresh new caps and normalize the caps using gst_caps_normalize().
> This will return caps where the colorimetry list is expanded.
> The ncaps will contain as many structures as the number of
> colorimetry specified in the GStreamer pipeline.
> 
> Iterate over each structure in the ncaps, 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 | 73 ++++++++++++++++++++--------
>  src/gstreamer/gstlibcamera-utils.h   |  4 +-
>  src/gstreamer/gstlibcamerasrc.cpp    |  2 +-
>  3 files changed, 58 insertions(+), 21 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index d895a67d..5dc3efcf 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -302,13 +302,36 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
>  	return caps;
>  }
>  
> +static void
> +configure_colorspace_from_caps(StreamConfiguration &stream_cfg,
> +			       GstStructure *s)
> +{
> +	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);


If the parsing failed, the colorimetry structure will be left uninitialized.
Perhaps you should just early return if the caps string is 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;
> +		}
> +	}
> +}
> +
>  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, colorimetry_index = -1;
>  	GstStructure *s;
>  
>  	/*
> @@ -354,10 +377,13 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>  	}
>  
>  	/* Prefer reliable fixed value over ranges */
> -	if (best_fixed >= 0)
> +	if (best_fixed >= 0) {
>  		s = gst_caps_get_structure(caps, best_fixed);
> -	else
> +		colorimetry_index = best_fixed;
> +	} else {
>  		s = gst_caps_get_structure(caps, best_in_range);
> +		colorimetry_index = best_in_range;
> +	}
>  
>  	if (gst_structure_has_name(s, "video/x-raw")) {
>  		const gchar *format = gst_video_format_to_string(gst_format);
> @@ -381,21 +407,30 @@ 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;
> +	/* Create new caps, copy the structure with best resolutions
> +	 * and normalize the caps.
> +	 */
> +	GstCaps *ncaps = gst_caps_copy_nth(caps, colorimetry_index);
> +	ncaps = gst_caps_normalize(ncaps);

I'm worried that original caps may have width/height ranges which will also get
expended, leading to a really large number of caps. Any reason not to enumerate
the colorimetry field instead instead ?

> +
> +	/* Configure Colorimetry */
> +	StreamConfiguration pristine_stream_cfg = stream_cfg;
> +	for (i = 0; i < gst_caps_get_size(ncaps); i++) {
> +		GstStructure *ns = gst_caps_get_structure(ncaps, i);
> +		GstVideoColorimetry colorimetry_old, colorimetry_new;
> +		colorimetry_old = colorimetry_from_colorspace(stream_cfg.colorSpace.value());
> +		configure_colorspace_from_caps(stream_cfg, ns);
> +
> +		/* 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;
>  		}
>  	}
>  }
> 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)