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

[Kieran Bingham]

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);
> +
> +			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"
>