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

[Kieran Bingham]

On 26/08/2021 15:34, Nicolas Dufresne wrote:
> Le jeudi 26 août 2021 à 15:31 +0100, Kieran Bingham a écrit :
>>
>> On 26/08/2021 15:04, Nicolas Dufresne wrote:
>>> Le jeudi 26 août 2021 à 14:44 +0100, Kieran Bingham a écrit :
>>>> 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).
>>>
>>> Feel free to edit this part out, or ask for a V2. What "other project" (of
>>> course I mean gst here) do is that we add links with "Refer to ...". In that
>>> specific case, instead of leaving StreamConfiguration::size and
>>> StreamConfiguration::format blank, or duplicating the doc, it could be extended
>>> with a link that refers to were these are documented.
>>
>> I don't think size and format need to reference elsewhere, they /are/
>> the size and format of that StreamConfiguration.
> 
> Well they have a very special semantic, as before being set and validate, they
> meant to be a "suggested default value". I also believe that me being tricked
> thinking this was undocumented is enough to justify improving it.


Absolutely, that's what I'm trying to get to the bottom of.

But ... It's not the size and format that are special. It's the whole
StreamConfiguration.


The size and format are just two parts of that, though admittedly, they
are possibly the only parts of relevance to an application at this stage
in the configuration process.


But also it's more specific to the StreamRole that is given. So the
StreamConfiguration is not set as a default arbitrarily, it's given as a
default for the requested StreamRole.

If there is no StreamRole, then there would be no StreamConfiguration
returned, and so there would be no given size or format.

So this discussion really, is about StreamRoles - ... And their use with
generateConfiguration() ... so perhaps it's the StreamRoles that need
better documentation.


>>
>> The StreamConfiguration (as a whole) is the object which is returned,
>> and it 'is' the default suggested configuration.
>>
>> The given default StreamConfiguration has a size and a format ...
>>
>> There's nothing special about 'size' and 'format' of the
>> StreamConfiguration - it's the object as a whole that we're discussing here.
>>
>>
>> Perhaps a better way to describe it is
>>
>> "Pipeline handlers will return a default StreamConfiguration for a given
>> StreamRole. That StreamConfiguration comprises of a validated Size and
>> Format which can be used to start the device".
>>
>>
>> I think what is confusing is that a StreamConfiguration has this 'extra'
>> thing called formats, and sizes. They are /not/ the active
>> StreamConfiguration - but they are possible values (hints) that can be
>> put back into the StreamConfiguration to try validating.
>>
>>
>>
>>>> [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 ...
>>>
>>> What I was saying is that you don't want to fix this theoretically, as you'll
>>> never see the end of it. At the moment, libcamera provides format/size(fixed)
>>> pairs and then unreliable ranges.
>>>
>>> What new sine I wrote that gst code, is that we generate unreliable fixed sizes
>>> from ranges now. I suggest to check IPU code, that might be one of these that
>>> endup being picked and failed.
>>>
>>> I was expected that using the default size would greatly help avoid bad pick,
>>> since, can you check if you have managed to hit that default but you had a
>>> format miss-match ? If that was the case instead of a "unreliable" generated
>>> fixed size, we could make improve it short term by adding preferred format
>>> around this.
>>
>>
>> I think the default size was picked - but the width * width bug gave a
>> different default, that's identified and fixed now though ...
>>
>> What does still happen is that the 'arbitrary unvalidated list' is
>> exposed to cheese, so it lets me pick one of those sizes.
>>
>> I select it, and it seems to fail validation - and that causes the
>> gstreamer pipeline to stop with:
>>
>> (cheese:93188): cheese-WARNING **: 15:29:24.060: Internal data stream
>> error.: ../../src/gstreamer/gstlibcamerasrc.cpp(323):
>> gst_libcamera_src_task_run ():
>> /GstCameraBin:camerabin/GstWrapperCameraBinSrc:camera_source/GstBin:bin19/GstLibcameraSrc:libcamerasrc2:
>> streaming stopped, reason not-negotiated (-4)
>>
>>
>> But it doesn't resume when changing back to a supported size.
>>
>> Anyway, I don't think that's an issue caused by this patch, and this
>> patch already brings improvements, so I'm going to move to the v2 patch
>> that you've posted ;-)
>>
>>
>>>>
>>>>
>>>>> +	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.
>>>
>>> Interesting, indeed, will fix.
>>>
>>>>
>>>> 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"
>>>>>
>>>
>>>
> 
>