[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: Fix formats() returning only sizes for last mbus code

[Marian Cichy]

On 3/11/21 9:16 AM, Jacopo Mondi wrote:
> Hello Marian,
>
> On Tue, Mar 09, 2021 at 03:44:57PM +0100, Marian Cichy wrote:
>> V4L2Subdevice::formats() wants to enumerate all media bus codes and
>> frame sizes on a pad, however, the current implementation always
>> overwrites the SizeRange of the last mbus code, effectively returning
>> only this SizeRange.
>>
>> This can be even fatal, as camera_sensor is using this function to
>> enumerate all codes and sizes. If the last mbus code has no SizeRange
>> for whatever reason, the camera_sensor will error out, claiming that it
>> didn't found any image formats at all.
> What is the use case for a driver returning an mbus code without any
> image size associated ? Smells like a driver bug to me.

The driver I am using has a code which is only used by the 
hardware-internal test pattern generator and it doesn't return a size 
for this code. It might still very well a driver bug though.

>> Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
>> ---
>>   src/libcamera/v4l2_subdevice.cpp | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
>> index 9af1c0ab..37bafb9b 100644
>> --- a/src/libcamera/v4l2_subdevice.cpp
>> +++ b/src/libcamera/v4l2_subdevice.cpp
>> @@ -354,10 +354,10 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
>>   		return {};
>>   	}
>>
>> +	std::vector<SizeRange> sizes = {};
>>   	for (unsigned int code : enumPadCodes(pad)) {
>> -		std::vector<SizeRange> sizes = enumPadSizes(pad, code);
>> -		if (sizes.empty())
>> -			return {};
>> +		std::vector<SizeRange> codeSizeRange = enumPadSizes(pad, code);
>> +		sizes.insert(sizes.end(), codeSizeRange.begin(), codeSizeRange.end());
> Also, this builds 'sizes' incrementally, associating to the newly
> enumerated 'code' the sizes previously enumerated for other codes
> if I'm not mistaken.
>
> Is this intentional ?

Right, this merges all sizes and all codes together and is probably not 
wanted. The previous implementation is actually correct, expect that I 
fall into the empty sizes case.

>
> Thanks
>     j
>
>>   		const auto inserted = formats.insert({ code, sizes });
>>   		if (!inserted.second) {
>> --
>> 2.29.2
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel