[libcamera-devel,RFC] camera_sensor: Do not always prioritize aspect-ratios
diff mbox series

Message ID 20210706142220.747614-1-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • [libcamera-devel,RFC] camera_sensor: Do not always prioritize aspect-ratios
Related show

Commit Message

Umang Jain July 6, 2021, 2:22 p.m. UTC 
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
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.

Signed-off-by: Umang Jain <umang.jain@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
---
 src/libcamera/camera_sensor.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Kieran Bingham July 7, 2021, 10:35 a.m. UTC | #1 
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@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) {
>
Umang Jain July 7, 2021, 10:57 a.m. UTC | #2 
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@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) {
>>

Patch
diff mbox series 

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;
 
 			if (areaDiff < bestArea) {