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

17124 diff mbox series

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@gmail.com>
Signed-off-by: Umang Jain <umang.jain@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(-)

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@gmail.com>
> Signed-off-by: Umang Jain <umang.jain@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)

> > +     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 ?
>
Noted.
>
> > -     /* 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 ?

 Yes, expanding height/width range would create a lot of structures.
The first idea I thought of was to enumerate the colorimetry list
itself, but I don't know how ;-;
This is the reason I had to go with the approach of creating duplicate
caps and normalizing it.
If there is a way to enumerate the colorimetry list in your knowledge
please let me know, that would be helpful.

Regards,

Rishikesh Donadkar

Le mardi 16 août 2022 à 11:11 +0530, Rishikesh Donadkar a écrit :
> > > +     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 ?
> > 
> Noted.
> > 
> > > -     /* 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 ?
> 
>  Yes, expanding height/width range would create a lot of structures.
> The first idea I thought of was to enumerate the colorimetry list
> itself, but I don't know how ;-;

It is something along these lines, saving from copying anything, and creating
doing multiple trial for the same value.

const GstStructure *s = gst_caps_get_structure(caps, 0);
const GValue *colorimetry = gst_structure_get_value (s, "colorimetry");

if (GST_VALUE_HOLDS_LIST (colorimetry)) {
	int i;
	for (i = 0; i < g_value_list_get_size (colorimetry); i++) {
		const GValue *val = g_value_list_get_value (colorimetry, i);
		GstVideoColorimetry colorimetry;

		if (gst_video_colorimetry_from_string (&colorimetry, g_value_get_string (val))) {
			// Got a valid candidate, try it out !
		}
	}
}

> This is the reason I had to go with the approach of creating duplicate
> caps and normalizing it.
> If there is a way to enumerate the colorimetry list in your knowledge
> please let me know, that would be helpful.
> 
> Regards,
> 
> Rishikesh Donadkar