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

[Umang Jain]

Hi Jacopo

On 8/30/21 3:53 PM, Jacopo Mondi wrote:
> Hello Umang,
>
> On Mon, Aug 30, 2021 at 03:38:44PM +0530, Umang Jain wrote:
>> Hi Jacopo
>>
>> On 8/30/21 2:22 PM, Jacopo Mondi wrote:
>>> On Mon, Aug 30, 2021 at 02:02:27PM +0530, Umang Jain wrote:
>>>> Hi Me,
>>>>
>>>> On 8/30/21 1:51 PM, 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.
>>>>>
>>>>> 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
>>>>> 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
>>>> This cam command is invalid as of today. It was fine when I wrote the
>>>> series.
>>>>
>>>> The invalidation comes from 4fec25a4b ("ipu3: Disallow raw only camera

ops, wrong commit message here, it should have been 0536a9aa7189f75c898


>>>> configuration"). I am tempted to remove this cam command (since it's not
>>>> valid today), but I don't think I would have a better explanation than this.
>>>> All I can do is to replace it with a block of text (essentially making the
>>>> commit message still longer)
>>>>
>>>> Any ideas anyone?
>>>>
>>> What about adding a YUV stream of the same size to the command ?
>>
>> As of master, 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame
>> size") is in effect  right? So it's always selecting the highest sensor
>> format resolution.
> Ah ups, I'm running with my branch :)
>
>> I cherry-picked your "DNI: libcamera: ipu3: Use the optimal sensor side"
>> which generates the same results as the commit message, to double-check.
>>
>> So I don't adding YUV stream helps. Probably I'll just need to drop these
>> commands and explain everything in a text.
> You're right!
> Or maybe, for the sake of the example, just revert the commit that
> blocks the single RAW stream ?


Yes, I can probably mention that with a '*' on the command.

     * Please revert 0536a9aa718(" ipu3: Disallow raw only camera 
configuration ") if the configuration is reported invalid

It seems we will have raw-only configs in the future (it's a acceptable 
case to have, but just not sure when/priority), so at the time those 
command will be fine.


>
> Anyway, up to you :)
>
>>>>> 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: Jacopo Mondi <jacopo at jmondi.org>
>>>>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>>>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>>> ---
>>>>>     src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++
>>>>>     1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
>>>>> index ceaf665e..9cedcb5b 100644
>>>>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
>>>>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
>>>>> @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int>
>>>>>     				continue;
>>>>>     			float ratio = static_cast<float>(sz.width) / sz.height;
>>>>> +			/*
>>>>> +			 * Ratios can differ by small mantissa difference which
>>>>> +			 * can affect the selection of the sensor output size
>>>>> +			 * wildly. We are interested in selection of the closest
>>>>> +			 * size with respect to the desired output size, hence
>>>>> +			 * comparing it with a single precision digit is enough.
>>>>> +			 */
>>>>> +			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;