[libcamera-devel] [PATCH v2 4/4] ipu3: cio2: Tweak sensor size selection policy

[Kieran Bingham]

On 25/08/2021 11:12, Jacopo Mondi wrote:
> On Tue, Aug 10, 2021 at 01:28:54PM +0530, Umang Jain wrote:
>> Do not compare higher precision of the ratios, as it might lead to
>> absurd selection of sensor size for a relatively low requested
>> resolution size.
> 
> Just "lead to size selection mismatches" maybe ?
> The example below is explicative enough
> 
>>
>> For example:
>> The imx258 driver supports the following sensor resolutions:
>>
>>  - 4208x3118 = 1.349583066
>>  - 2104x1560 = 1.348717949
>>  - 1048x780  = 1.343589744
>>
>> It can be inferred that, that the aspect ratio only differs by a small
> 
> s/that, that/that/
> 
>> factor with each other. It does not makes sense to select a 4208x3118
>> for a requested size of say 640x480 or 1280x720, which is what is
>> happening currently.
>> ($) cam -c1 -swidth=640,height=480,role=raw
>>     - CIO2 configuration: 4208x3118-SGRBG10_IPU3
>>
>> In order to address this constraint, only compare the ratio with single
>> precision to make a better decision on the sensor resolution policy
>> selection.
>>
>> ($) cam -c1 -srole=raw,width=640,height=480
>>     - CIO2 configuration: 1048x780-SGRBG10_IPU3
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> Tested-by: Umang Jain <umang.jain at ideasonboard.com>
>> Tested-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> I recall I asked about if we want rounding or truncating is fine. I
> think you confirmed truncating is fine, which I agree.
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Thanks
>   j
> 
>> ---
>>  src/libcamera/pipeline/ipu3/cio2.cpp | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
>> index 3c9331e3..6ccef301 100644
>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
>> @@ -290,6 +290,11 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int>
>>  				continue;
>>
>>  			float ratio = static_cast<float>(sz.width) / sz.height;
>> +			/*
>> +			 * Comparing ratios with a single precision digit
>> +			 * is enough.
>> +			 */

It might be worth expanding this to say why it's enough, or rather what
happens if greater precision is used.

It's covered by the commit message, but that might not be seen by
someone trying to work out 'why' single precision is used rather than
the full float.

But either way, I can see how this is a direct advantage.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



>> +			ratio = static_cast<unsigned int>(ratio * 10) / 10.0;
>>  			float ratioDiff = fabsf(ratio - desiredRatio);
>>  			unsigned int area = sz.width * sz.height;
>>  			unsigned int areaDiff = area - desiredArea;
>> --
>> 2.31.1
>>