[libcamera-devel,v2,2/2] gstreamer: Add multiple colorimetry support

17041 diff mbox series

This patch provides the following:
- Add support to handle colorimetry in the GStreamer->libcamera
  direction.
- Add support for multiple colorimetry.

The function colorspace_from_colorimetry() takes in a
GstVideoColorimetry and returns libcamera::ColorSpace

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 | 152 ++++++++++++++++++++++++++-
 src/gstreamer/gstlibcamera-utils.h   |   4 +-
 src/gstreamer/gstlibcamerasrc.cpp    |   3 +-
 3 files changed, 153 insertions(+), 6 deletions(-)

Hi Rishikesh,

On 8/9/22 20:05, Rishikesh Donadkar wrote:
> This patch provides the following:
> - Add support to handle colorimetry in the GStreamer->libcamera
>    direction.
> - Add support for multiple colorimetry.
>
> The function colorspace_from_colorimetry() takes in a
> GstVideoColorimetry and returns libcamera::ColorSpace
>
> 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 | 152 ++++++++++++++++++++++++++-
>   src/gstreamer/gstlibcamera-utils.h   |   4 +-
>   src/gstreamer/gstlibcamerasrc.cpp    |   3 +-
>   3 files changed, 153 insertions(+), 6 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index dbb47c5a..078b9343 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -104,6 +104,93 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace)
>   	return colorimetry;
>   }
>   
> +static ColorSpace
> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)


As mentioned in my earlier reply, this is a part of colorimetry <> 
libcamera::Colorspace mappings which are currently handled in

[v3 PATCH 4/4] gstreamer: Provide colorimetry <> ColorSpace mappings

Can you please rebase over that series and provide a true multiple 
colorimetry support patch ? Which which then be easier to review 
(because it will contain multiple colorimetry bits only, replacing the 
single colorimetry handling) hence I believe there will some code sharing.

Meanwhile I have left a comments below which you can incorporate in next 
version.

> +{
> +	ColorSpace colorspace = ColorSpace::Raw;
> +
> +	switch (colorimetry.primaries) {
> +	case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:
> +		/* Unknown primaries map to raw colorspace in GStreamer */
> +		return ColorSpace::Raw;
> +	case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:
> +		colorspace.primaries = ColorSpace::Primaries::Smpte170m;
> +		break;
> +	case GST_VIDEO_COLOR_PRIMARIES_BT709:
> +		colorspace.primaries = ColorSpace::Primaries::Rec709;
> +		break;
> +	case GST_VIDEO_COLOR_PRIMARIES_BT2020:
> +		colorspace.primaries = ColorSpace::Primaries::Rec2020;
> +		break;
> +	default:
> +		GST_WARNING("Colorimetry primaries %d not mapped in gstlibcamera",
> +			    colorimetry.primaries);
> +		return ColorSpace::Raw;
> +	}
> +
> +	switch (colorimetry.transfer) {
> +	/* Transfer function mappings inspired from v4l2src plugin */
> +	case GST_VIDEO_TRANSFER_GAMMA18:
> +	case GST_VIDEO_TRANSFER_GAMMA20:
> +	case GST_VIDEO_TRANSFER_GAMMA22:
> +	case GST_VIDEO_TRANSFER_GAMMA28:
> +		GST_WARNING("GAMMA 18, 20, 22, 28 transfer functions not supported");
> +	/* fallthrough */
> +	case GST_VIDEO_TRANSFER_GAMMA10:
> +		colorspace.transferFunction = ColorSpace::TransferFunction::Linear;
> +		break;
> +	case GST_VIDEO_TRANSFER_SRGB:
> +		colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;
> +		break;
> +	case GST_VIDEO_TRANSFER_BT601:
> +	case GST_VIDEO_TRANSFER_BT2020_12:
> +	case GST_VIDEO_TRANSFER_BT2020_10:
> +	case GST_VIDEO_TRANSFER_BT709:
> +		colorspace.transferFunction = ColorSpace::TransferFunction::Rec709;
> +		break;
> +	default:
> +		GST_WARNING("Colorimetry transfer function %d not mapped in gstlibcamera",
> +			    colorimetry.transfer);
> +		return ColorSpace::Raw;
> +	}
> +
> +	switch (colorimetry.matrix) {
> +	case GST_VIDEO_COLOR_MATRIX_RGB:
> +		colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;
> +		break;
> +	/* FCC is about the same as BT601 with less digits */
> +	case GST_VIDEO_COLOR_MATRIX_FCC:
> +	case GST_VIDEO_COLOR_MATRIX_BT601:
> +		colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;
> +		break;
> +	case GST_VIDEO_COLOR_MATRIX_BT709:
> +		colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;
> +		break;
> +	case GST_VIDEO_COLOR_MATRIX_BT2020:
> +		colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;
> +		break;
> +	default:
> +		GST_WARNING("Colorimetry matrix %d not mapped in gstlibcamera",
> +			    colorimetry.matrix);
> +		return ColorSpace::Raw;
> +	}
> +
> +	switch (colorimetry.range) {
> +	case GST_VIDEO_COLOR_RANGE_0_255:
> +		colorspace.range = ColorSpace::Range::Full;
> +		break;
> +	case GST_VIDEO_COLOR_RANGE_16_235:
> +		colorspace.range = ColorSpace::Range::Limited;
> +		break;
> +	default:
> +		GST_WARNING("Colorimetry range %d not mapped in gstlibcamera",
> +			    colorimetry.range);
> +		return ColorSpace::Raw;
> +	}
> +
> +	return colorspace;
> +}
> +
>   static GstVideoFormat
>   pixel_format_to_gst_format(const PixelFormat &format)
>   {
> @@ -215,13 +302,47 @@ 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);
> +
> +		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;
> +		}


The rebase will reflect that this code block as been 'moved', which is 
not quite apparent right now.

> +	}
> +}
> +
> +static gboolean
> +check_colorspace(const ColorSpace colorSpace, const gchar *colorimetry_old)


More suitable : 'compare_colorspace(old, new)'

> +{
> +	GstVideoColorimetry colorimetry = colorimetry_from_colorspace(colorSpace);
> +	g_autofree gchar *colorimetry_new = gst_video_colorimetry_to_string(&colorimetry);
> +	if (!g_strcmp0(colorimetry_old, colorimetry_new)) {


Have you tried using the comparator gst_video_colorimetry_is_equal() ? I 
guess that shall be more appropriate than comparing strings?

> +		return true;
> +	}


No need for {} as only single line after "if"

> +	return false;
> +}
> +
>   void
> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> +gst_libcamera_configure_stream_from_caps(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;
>   
>   	/*
> @@ -267,10 +388,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);
> @@ -293,6 +417,26 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>   	gst_structure_get_int(s, "height", &height);
>   	stream_cfg.size.width = width;
>   	stream_cfg.size.height = height;
> +
> +	/* 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);
> +
> +	/* Configure Colorimetry */
> +	StreamConfiguration dup_stream_cfg = stream_cfg;


This is used when no colorimetry candidate is found, so probably rename 
as pristine_stream_cfg?

> +	for (i = 0; i < gst_caps_get_size(ncaps); i++) {
> +		GstStructure *ns = gst_caps_get_structure(ncaps, i);
> +		configure_colorspace_from_caps(stream_cfg, ns);
> +		g_autofree const gchar *colorimetry_old = gst_structure_get_string(ns, "colorimetry");


s/colorimetry_old/colorimetry

> +		if (cam_cfg.validate() != CameraConfiguration::Invalid) {
> +			if (check_colorspace(stream_cfg.colorSpace.value(), colorimetry_old))
> +				break;


Should we log a INFO log here: "Selected colorimetry: <candidate>" ?

> +			else
> +				stream_cfg = dup_stream_cfg;
> +		}
> +	}
>   }
>   
>   #if !GST_CHECK_VERSION(1, 17, 1)
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 164189a2..90be6abe 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(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..3617170e 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -492,6 +492,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>   	for (gsize i = 0; i < state->srcpads_.size(); i++) {
>   		GstPad *srcpad = state->srcpads_[i];
>   		StreamConfiguration &stream_cfg = state->config_->at(i);
> +		CameraConfiguration &cam_cfg = *(state->config_);
>   
>   		/* Retrieve the supported caps. */
>   		g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> @@ -503,7 +504,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(cam_cfg, stream_cfg, caps);


You can directly pass state->config_ here instead of taking it in a 
holder variable reference cam_cfg

>   	}
>   
>   	if (flow_ret != GST_FLOW_OK)

>
> Can you please rebase over that series and provide a true multiple
> colorimetry support patch ? Which which then be easier to review
> (because it will contain multiple colorimetry bits only, replacing the
> single colorimetry handling) hence I believe there will some code sharing.
>
Sure.

> More suitable : 'compare_colorspace(old, new)'
>
> > +{
> > +     GstVideoColorimetry colorimetry =
> colorimetry_from_colorspace(colorSpace);
> > +     g_autofree gchar *colorimetry_new =
> gst_video_colorimetry_to_string(&colorimetry);
> > +     if (!g_strcmp0(colorimetry_old, colorimetry_new)) {

Noted.

> Have you tried using the comparator gst_video_colorimetry_is_equal() ? I
> guess that shall be more appropriate than comparing strings?
>
> > +             return true;
> > +     }
>
Thanks for the pointer. Using this comparator eliminated the need for the
compare_colorspace() function.

> > +     /* 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);
> > +
> > +     /* Configure Colorimetry */
> > +     StreamConfiguration dup_stream_cfg = stream_cfg;
>
>
> This is used when no colorimetry candidate is found, so probably rename
> as pristine_stream_cfg?

Right.

> > +             if (cam_cfg.validate() != CameraConfiguration::Invalid) {
> > +                     if
> (check_colorspace(stream_cfg.colorSpace.value(), colorimetry_old))
> > +                             break;
>
>
> Should we log a INFO log here: "Selected colorimetry: <candidate>" ?
>
Yes, that would be quite useful.

> >       for (gsize i = 0; i < state->srcpads_.size(); i++) {
> >               GstPad *srcpad = state->srcpads_[i];
> >               StreamConfiguration &stream_cfg = state->config_->at(i);
> > +             CameraConfiguration &cam_cfg = *(state->config_);
> >
> >               /* Retrieve the supported caps. */
> >               g_autoptr(GstCaps) filter =
> gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> > @@ -503,7 +504,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(cam_cfg,
> stream_cfg, caps);
>
>
> You can directly pass state->config_ here instead of taking it in a
> holder variable reference cam_cfg

Noted.


On Wed, Aug 10, 2022 at 11:20 AM Umang Jain <umang.jain@ideasonboard.com>
wrote:

> Hi Rishikesh,
>
> On 8/9/22 20:05, Rishikesh Donadkar wrote:
> > This patch provides the following:
> > - Add support to handle colorimetry in the GStreamer->libcamera
> >    direction.
> > - Add support for multiple colorimetry.
> >
> > The function colorspace_from_colorimetry() takes in a
> > GstVideoColorimetry and returns libcamera::ColorSpace
> >
> > 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 | 152 ++++++++++++++++++++++++++-
> >   src/gstreamer/gstlibcamera-utils.h   |   4 +-
> >   src/gstreamer/gstlibcamerasrc.cpp    |   3 +-
> >   3 files changed, 153 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> b/src/gstreamer/gstlibcamera-utils.cpp
> > index dbb47c5a..078b9343 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -104,6 +104,93 @@ colorimetry_from_colorspace(const ColorSpace
> &colorSpace)
> >       return colorimetry;
> >   }
> >
> > +static ColorSpace
> > +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)
>
>
> As mentioned in my earlier reply, this is a part of colorimetry <>
> libcamera::Colorspace mappings which are currently handled in
>
> [v3 PATCH 4/4] gstreamer: Provide colorimetry <> ColorSpace mappings
>
> Can you please rebase over that series and provide a true multiple
> colorimetry support patch ? Which which then be easier to review
> (because it will contain multiple colorimetry bits only, replacing the
> single colorimetry handling) hence I believe there will some code sharing.
>
> Meanwhile I have left a comments below which you can incorporate in next
> version.
>
> > +{
> > +     ColorSpace colorspace = ColorSpace::Raw;
> > +
> > +     switch (colorimetry.primaries) {
> > +     case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:
> > +             /* Unknown primaries map to raw colorspace in GStreamer */
> > +             return ColorSpace::Raw;
> > +     case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:
> > +             colorspace.primaries = ColorSpace::Primaries::Smpte170m;
> > +             break;
> > +     case GST_VIDEO_COLOR_PRIMARIES_BT709:
> > +             colorspace.primaries = ColorSpace::Primaries::Rec709;
> > +             break;
> > +     case GST_VIDEO_COLOR_PRIMARIES_BT2020:
> > +             colorspace.primaries = ColorSpace::Primaries::Rec2020;
> > +             break;
> > +     default:
> > +             GST_WARNING("Colorimetry primaries %d not mapped in
> gstlibcamera",
> > +                         colorimetry.primaries);
> > +             return ColorSpace::Raw;
> > +     }
> > +
> > +     switch (colorimetry.transfer) {
> > +     /* Transfer function mappings inspired from v4l2src plugin */
> > +     case GST_VIDEO_TRANSFER_GAMMA18:
> > +     case GST_VIDEO_TRANSFER_GAMMA20:
> > +     case GST_VIDEO_TRANSFER_GAMMA22:
> > +     case GST_VIDEO_TRANSFER_GAMMA28:
> > +             GST_WARNING("GAMMA 18, 20, 22, 28 transfer functions not
> supported");
> > +     /* fallthrough */
> > +     case GST_VIDEO_TRANSFER_GAMMA10:
> > +             colorspace.transferFunction =
> ColorSpace::TransferFunction::Linear;
> > +             break;
> > +     case GST_VIDEO_TRANSFER_SRGB:
> > +             colorspace.transferFunction =
> ColorSpace::TransferFunction::Srgb;
> > +             break;
> > +     case GST_VIDEO_TRANSFER_BT601:
> > +     case GST_VIDEO_TRANSFER_BT2020_12:
> > +     case GST_VIDEO_TRANSFER_BT2020_10:
> > +     case GST_VIDEO_TRANSFER_BT709:
> > +             colorspace.transferFunction =
> ColorSpace::TransferFunction::Rec709;
> > +             break;
> > +     default:
> > +             GST_WARNING("Colorimetry transfer function %d not mapped
> in gstlibcamera",
> > +                         colorimetry.transfer);
> > +             return ColorSpace::Raw;
> > +     }
> > +
> > +     switch (colorimetry.matrix) {
> > +     case GST_VIDEO_COLOR_MATRIX_RGB:
> > +             colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;
> > +             break;
> > +     /* FCC is about the same as BT601 with less digits */
> > +     case GST_VIDEO_COLOR_MATRIX_FCC:
> > +     case GST_VIDEO_COLOR_MATRIX_BT601:
> > +             colorspace.ycbcrEncoding =
> ColorSpace::YcbcrEncoding::Rec601;
> > +             break;
> > +     case GST_VIDEO_COLOR_MATRIX_BT709:
> > +             colorspace.ycbcrEncoding =
> ColorSpace::YcbcrEncoding::Rec709;
> > +             break;
> > +     case GST_VIDEO_COLOR_MATRIX_BT2020:
> > +             colorspace.ycbcrEncoding =
> ColorSpace::YcbcrEncoding::Rec2020;
> > +             break;
> > +     default:
> > +             GST_WARNING("Colorimetry matrix %d not mapped in
> gstlibcamera",
> > +                         colorimetry.matrix);
> > +             return ColorSpace::Raw;
> > +     }
> > +
> > +     switch (colorimetry.range) {
> > +     case GST_VIDEO_COLOR_RANGE_0_255:
> > +             colorspace.range = ColorSpace::Range::Full;
> > +             break;
> > +     case GST_VIDEO_COLOR_RANGE_16_235:
> > +             colorspace.range = ColorSpace::Range::Limited;
> > +             break;
> > +     default:
> > +             GST_WARNING("Colorimetry range %d not mapped in
> gstlibcamera",
> > +                         colorimetry.range);
> > +             return ColorSpace::Raw;
> > +     }
> > +
> > +     return colorspace;
> > +}
> > +
> >   static GstVideoFormat
> >   pixel_format_to_gst_format(const PixelFormat &format)
> >   {
> > @@ -215,13 +302,47 @@ 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);
> > +
> > +             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;
> > +             }
>
>
> The rebase will reflect that this code block as been 'moved', which is
> not quite apparent right now.
>
> > +     }
> > +}
> > +
> > +static gboolean
> > +check_colorspace(const ColorSpace colorSpace, const gchar
> *colorimetry_old)
>
>
> More suitable : 'compare_colorspace(old, new)'
>
> > +{
> > +     GstVideoColorimetry colorimetry =
> colorimetry_from_colorspace(colorSpace);
> > +     g_autofree gchar *colorimetry_new =
> gst_video_colorimetry_to_string(&colorimetry);
> > +     if (!g_strcmp0(colorimetry_old, colorimetry_new)) {
>
>
> Have you tried using the comparator gst_video_colorimetry_is_equal() ? I
> guess that shall be more appropriate than comparing strings?
>
> > +             return true;
> > +     }
>
>
> No need for {} as only single line after "if"
>
> > +     return false;
> > +}
> > +
> >   void
> > -gst_libcamera_configure_stream_from_caps(StreamConfiguration
> &stream_cfg,
> > +gst_libcamera_configure_stream_from_caps(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;
> >
> >       /*
> > @@ -267,10 +388,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);
> > @@ -293,6 +417,26 @@
> gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> >       gst_structure_get_int(s, "height", &height);
> >       stream_cfg.size.width = width;
> >       stream_cfg.size.height = height;
> > +
> > +     /* 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);
> > +
> > +     /* Configure Colorimetry */
> > +     StreamConfiguration dup_stream_cfg = stream_cfg;
>
>
> This is used when no colorimetry candidate is found, so probably rename
> as pristine_stream_cfg?
>
> > +     for (i = 0; i < gst_caps_get_size(ncaps); i++) {
> > +             GstStructure *ns = gst_caps_get_structure(ncaps, i);
> > +             configure_colorspace_from_caps(stream_cfg, ns);
> > +             g_autofree const gchar *colorimetry_old =
> gst_structure_get_string(ns, "colorimetry");
>
>
> s/colorimetry_old/colorimetry
>
> > +             if (cam_cfg.validate() != CameraConfiguration::Invalid) {
> > +                     if
> (check_colorspace(stream_cfg.colorSpace.value(), colorimetry_old))
> > +                             break;
>
>
> Should we log a INFO log here: "Selected colorimetry: <candidate>" ?
>
> > +                     else
> > +                             stream_cfg = dup_stream_cfg;
> > +             }
> > +     }
> >   }
> >
> >   #if !GST_CHECK_VERSION(1, 17, 1)
> > diff --git a/src/gstreamer/gstlibcamera-utils.h
> b/src/gstreamer/gstlibcamera-utils.h
> > index 164189a2..90be6abe 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(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..3617170e 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -492,6 +492,7 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
> >       for (gsize i = 0; i < state->srcpads_.size(); i++) {
> >               GstPad *srcpad = state->srcpads_[i];
> >               StreamConfiguration &stream_cfg = state->config_->at(i);
> > +             CameraConfiguration &cam_cfg = *(state->config_);
> >
> >               /* Retrieve the supported caps. */
> >               g_autoptr(GstCaps) filter =
> gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> > @@ -503,7 +504,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(cam_cfg,
> stream_cfg, caps);
>
>
> You can directly pass state->config_ here instead of taking it in a
> holder variable reference cam_cfg
>
> >       }
> >
> >       if (flow_ret != GST_FLOW_OK)
>