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

[Umang Jain]

Hi Laurent,

Thank you for the comments
On 9/19/22 11:22 PM, Laurent Pinchart wrote:
> 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.

We indeed had a discussion around it I think but I think it was my idea 
to develop on top of this actually.

Rishikesh, what do you think about handling this?  We can have a 
exploratory session together as I think I haven't been much exposed to 
multi-stream usage via gstreamer plugin.
>
>> +				/* 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.

For single colorimetry, the validate happens right after the function's 
call in  src/gstreamer/gstlibcamerasrc.cpp and then whatever is returned 
back, is exposed as caps. If the user-defined colorimetry as whatever is 
exposed in caps, negotiation shall pass.

For multiple colorimetry, we need to "select" a supported colorspace 
candidate from the user-provided list, hence need to run through 
validate() to see what sensor agrees on any-one of them. Which is found 
to be suitable, is set in the stream configuration and re-validated 
(although futile) in 
src/gstreamer/gstlibcamerasrc.cpp::gst_libcamera_src_task_enter() and 
then exposed through caps.

Hence, an extra validation is not required for the single-colorimetry 
case (which currently is the behaviour on master as well).  Does that 
makes sense?

>
>> +	} 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,

ah good point, ack
>
>> +					      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)