[libcamera-devel] [RFC PATCH] camera_sensor: Do not always prioritize aspect-ratios

Umang Jain umang.jain at ideasonboard.com
Wed Jul 7 12:57:52 CEST 2021 

Hi Kieran,

On 7/7/21 4:05 PM, Kieran Bingham wrote:
> Hi Umang,
>
> On 06/07/2021 15:22, Umang Jain wrote:
>> In some cases, the maximum sensor resolution will provide the best
>> aspect-ratio for a requested stream size. It may also happen that,
>> the difference between max sensor resolution's aspect-ratio vs a lower
>> sensor resolution aspect-ratio is very marginal(for e.g. <1%).
>> In such cases, we should actually lean towards the lower sensor
>> resolution.
>>
>> One of such cases is observed on nautilus, where all requested stream
> s/nautilus/$SENSOR_NAME/ (presumably IMX258
>
>> sizes seems to map to 4208x3118-SGRBG10_IPU3. Even though, the aspect
>> ratios for lower sensor resolution aren't that far from 4208x3118.
>> For a stream size request of 1080p:
>>   - 1080p     = 1.777777778 (requested)
>>
>>   - 4208x3118 = 1.349583066 (originally selected)
>>   - 2104x1560 = 1.348717949
>>   - 1048x780  = 1.343589744
>>
>> This patch introduces some flexibility on part of sensor resolution
>> selection procedure. It attempts to provide a sensor resolution,
>> closest to the requested stream size, whilst keeping best aspect-ratio.
> /stream size/area/ ?
>
>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>
>> ---
>> - This patch has been written with "[PATCH] camera_sensor: Remove
>>    redundant aspect-ratio check" applied.
>>
>> - There is also known issue/handle on why IPU3 currently selects max
>>    sensor resolution config possible, we will use "raw" to see this patch
>>    in action.
>>
>> - Findings via cam:
>>
>> (master)
>>
>> ($)  cam -c1 -swidth=640,height=320,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:12:57.975110123] [550]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:12:57.975199686] [550]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=1280,height=720,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:14:23.492963456] [651]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:14:23.493028736] [651]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=1920,height=1080,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:14:34.020592838] [660]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:14:34.020636493] [660]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=3840,height=2160,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:15:20.530557765] [727]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:15:20.530625404] [727]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=4200,height=3000,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:15:33.027468976] [737]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:15:33.027558684] [737]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>>
>> (With this Patch)
>>
>> ($)  cam -c1 -swidth=640,height=320,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:19:04.270812870] [1457]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:19:04.270859868] [1457]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 1048x780-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=1280,height=720,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:19:15.515222045] [1465]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:19:15.515265834] [1465]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 2104x1560-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=1920,height=1080,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:19:23.333048232] [1471]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:19:23.333101431] [1471]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 2104x1560-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=3840,height=2160,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:19:29.774700339] [1480]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:19:29.774800035] [1480]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> Camera configuration adjusted
>>
>> ($)  cam -c1 -swidth=4200,height=3000,role=raw
>> Using camera \_SB_.PCI0.I2C2.CAM0
>> [10:19:59.786068223] [1541]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> [10:19:59.786176204] [1541]  INFO IPU3 ipu3.cpp:277 CIO2 configuration: 4208x3118-SGRBG10_IPU3
>> Camera configuration adjusted
>
> Those findings looks quite encouraging indeed, and shows the requirement.
>
>
>> ---
>>   src/libcamera/camera_sensor.cpp | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
>> index 1bf42acf..fb24480b 100644
>> --- a/src/libcamera/camera_sensor.cpp
>> +++ b/src/libcamera/camera_sensor.cpp
>> @@ -569,7 +569,14 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>>   			unsigned int area = sz.width * sz.height;
>>   			unsigned int areaDiff = area - desiredArea;
>>   
>> -			if (ratioDiff > bestRatio)
>> +			/*
>> +			 * Check if we have a better aspect ratio match than
>> +			 * whatever we have seen before. ~1% change is acceptable
>> +			 * if it leads to a selection of lower resolution below.
>> +			 */
>> +			if (bestRatio == FLT_MAX)
>> +				bestRatio = ratioDiff;
>> +			else if (fabsf(ratioDiff - bestRatio) > 0.01)
>>   				continue;
>>   
> I'm a little curious to be sure of the effects between this patch and
> the preceding one.
>
> Have you run the checks above with only the previous patch?
>
> I.e. - could the lower resolutions that get picked be due to the change
> that you no longer take a preference on aspect ratio (even ignoring the
> 0.01) when the previous patch is applied?

For context: the loop starts with the highest-to-lowest 
sensor-resolution in the size-range possible.

With only the previous patch applied, there is no change w.r.t lower 
sensor resolution being selected (I checked). This is because 4208x3118 
provides the best(or closest match) aspect-ratio than the others, even 
lower resolutions are not much further away (check the commit message of 
this patch for values).

Hence, it will satisfy the condition in

	if (ratioDiff > bestRatio)
		continue;

from the start, and won't bother to look at areaDiff comparison below.

Does this address your curiosity?


>
>>   			if (areaDiff < bestArea) {
>>

More information about the libcamera-devel mailing list