[libcamera-devel] [PATCH] gstreamer: Fix usage of default size for fixation

[Kieran Bingham]

One more catch ...

On 26/08/2021 14:44, Kieran Bingham wrote:
> On 25/08/2021 16:07, Nicolas Dufresne wrote:
>> From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>>
>> Even thought this is not formaly documented, all pipeline managers sets a
> 
> s/thought/though/
> 
> s/formaly/formally/
> 
>> default value to StreamConfiguration::size. The original fixation code was
> 
> This is documented at by the function Camera::generateConfiguration()
> 
> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d
> 
> I suspect the real issue (of documentation) is that - we have lots of
> documentation, but it's hard to find the parts that are needed at the
> time they are needed.
> 
> 
>> attempting to use it, but as it was truncating the caps to its first structure
>> it would never actually find a best match.
>>
>> In this patch, instead of truncating, we weight various matches using the
>> product of the width and height delta. We also split delta from ranges
>> appart and prefer fixed size over them as ranges are not reliable.
> 
> s/appart/apart/
> 
> 
>> This patch also removes the related todo, as it seems that libcamera core won't
>> go further then providing this default value and won't be sorting the format and
>> size lists.
> 
> The formats and sizes might be sorted numerically ... but there's no
> indication of preference there indeed. They don't even really mean that
> the frame size is supported by the device (though the pixel format is a
> bit more indicative).
> 
> 
> Maybe the Sizes and Formats shouldn't be used at all by gstreamer, it
> might be that they are providing information which is seen as more
> expressive than it really is, and gstreamer should do it's own
> try/validate negotiation phase to determine the possibilities.
> 
> 
> 
>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>> ---
>>  src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---
>>  src/gstreamer/gstlibcamerasrc.cpp    |  4 ---
>>  2 files changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
>> index 61370d5f..007d6a64 100644
>> --- a/src/gstreamer/gstlibcamera-utils.cpp
>> +++ b/src/gstreamer/gstlibcamera-utils.cpp
>> @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(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;
>> +	GstStructure *s;
>> +
>> +	/*
>> +	 * These are delta weight computed from:
>> +	 *   ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)
>> +	 */
>> +	guint best_fixed_delta = G_MAXUINT;
>> +	guint best_in_range_delta = G_MAXUINT;
>>  
>>  	/* First fixate the caps using default configuration value. */
>>  	g_assert(gst_caps_is_writable(caps));
>> -	caps = gst_caps_truncate(caps);
>> -	GstStructure *s = gst_caps_get_structure(caps, 0);
>>  
>> -	gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
>> -	gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
>> +	/* Lookup the structure for a close match to the stream_cfg.size */
> 
> I'm worried that this is placing too much weight on the caps which have
> been generated from the StreamFormats.
> 
> https://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html
> states:
> 
>> All sizes retrieved from StreamFormats shall be treated as advisory
>> and no size shall be considered to be supported until it has been 
>> verified using CameraConfiguration::validate().
> 
> If a role has been passed to the generateConfiguration() [0], then the
> Streams (and therefore their StreamConfiguration) are provided as
> suggested default configurations (which should have already been run
> through validation).
> 
> 
> [0]
> https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d
> 
> 
> However, having discussed this with you last night, I think the code
> here is correct, as it matches against the caps of the whole pipeline,
> but we might want to have a validation phase either during
> gst_libcamera_stream_formats_to_caps() or after it to further
> filter/restrict what is actually added as supported formats.
> 
> I've already seen that this is reporting formats to the caps, that are
> not actually available on the IPU3, and so its' failing to correctly
> configure the pipeline as expected.
> 
> But I think that's a separate issue on top... Or I wonder if we could
> handle that better at the libcamera layer ...
> 
> 
>> +	for (i = 0; i < gst_caps_get_size(caps); i++) {
>> +		s = gst_caps_get_structure(caps, i);
>> +		gint width, height;
>> +		guint delta;
>> +
>> +		if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) &&
>> +		    gst_structure_has_field_typed(s, "height", G_TYPE_INT)) {
>> +			gst_structure_get_int(s, "width", &width);
>> +			gst_structure_get_int(s, "width", &height);
> 
> s/width/height/ here, looks like your setting width * width.
> 
> In fact, that might explain one of the bugs I hit with this...
> 
> The IPU3 was ending up using an odd default size which was possible
> based on the best match of 720x720 now I see this ...
> 
> (I'll test again later).
> 
> 
> Anyway, with s/width/height/ fixed above here:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
> 
>> +
>> +			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
>> +
>> +			if (delta < best_fixed_delta) {
>> +				best_fixed_delta = delta;
>> +				best_fixed = i;
>> +			}
>> +		} else {
>> +			gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
>> +			gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
>> +			gst_structure_get_int(s, "width", &width);
>> +			gst_structure_get_int(s, "width", &height);
>> +

And s/width/height/ here also.


With those two fixed, I now get:


gst-launch-1.0 libcamerasrc ! queue ! glimagesink
...
 INFO Camera camera.cpp:937 configuring streams: (0) 1280x720-NV12

on the IPU3.
and

gst-launch-1.0 libcamerasrc ! queue ! glimagesink
...
 INFO Camera camera.cpp:937 configuring streams: (0) 1920x1080-NV12

on my Brio.

Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



>> +			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
>> +
>> +			if (delta < best_in_range_delta) {
>> +				best_in_range_delta = delta;
>> +				best_in_range = i;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* Prefer reliable fixed value over ranges */
>> +	if (best_fixed >= 0)
>> +		s = gst_caps_get_structure(caps, best_fixed);
>> +	else
>> +		s = gst_caps_get_structure(caps, best_in_range);
>>  
>>  	if (gst_structure_has_name(s, "video/x-raw")) {
>>  		const gchar *format = gst_video_format_to_string(gst_format);
>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
>> index b243aeb3..4c7b36ae 100644
>> --- a/src/gstreamer/gstlibcamerasrc.cpp
>> +++ b/src/gstreamer/gstlibcamerasrc.cpp
>> @@ -25,10 +25,6 @@
>>   *  - Add timestamp support
>>   *  - Use unique names to select the camera devices
>>   *  - Add GstVideoMeta support (strides and offsets)
>> - *
>> - * \todo libcamera UVC drivers picks the lowest possible resolution first, this
>> - * should be fixed so that we get a decent resolution and framerate for the
>> - * role by default.
>>   */
>>  
>>  #include "gstlibcamerasrc.h"
>>

-- 
Regards
--
Kieran