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

[Kieran Bingham]

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?

>  			if (areaDiff < bestArea) {
>