[libcamera-devel] [PATCH] camera_sensor: Remove redundant aspect-ratio check

[Umang Jain]

hi Kieran,

On 7/7/21 3:03 PM, Kieran Bingham wrote:
> Hi Umang,
>
> On 06/07/2021 11:29, Umang Jain wrote:
>> While trying to find the best sensor resolution,
>> CameraSensor::getFormat() tracks best aspect-ratio of the sensor-format
>> it has seen beforehand, among other things. If a better aspect-ratio is
>> found, the code shall proceed to check other best-fit parameters.
>> However, the check for aspect-ratio is occuring twice, eliminate one of
>> them.
>>
>> This commit does not introduces any functional changes.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/libcamera/camera_sensor.cpp | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
>> index cde431cc..1bf42acf 100644
>> --- a/src/libcamera/camera_sensor.cpp
>> +++ b/src/libcamera/camera_sensor.cpp
>> @@ -572,7 +572,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>>   			if (ratioDiff > bestRatio)
>>   				continue;
>>   
>> -			if (ratioDiff < bestRatio || areaDiff < bestArea) {
>> +			if (areaDiff < bestArea) {
>>   				bestRatio = ratioDiff;
>>   				bestArea = areaDiff;
>>   				bestSize = &sz;
>>
>
> I don't think this change is doing what you expect I'm afraid.
>
> Previously if the ratioDiff < bestRatio /or/ areaDiff < bestArea, - we
> would enter this context and set the new bestRatio .. granted the check
> above means that we know ratioDiff < (or =) bestRatio ...
>
> But now - with your change it will /only/ do so if areaDiff < bestArea.
>
> It is true that the ratioDiff > bestRatio check will mean that the code
> path can only flow down here if the ratioDiff is improved, but now, we
> are cutting off the actual setting of those values unless areaDiff <
> bestArea as well.
>
> So I'm not sure this is quite correct. It is essentially turning the
> line from
>
>> -	if (ratioDiff < bestRatio || areaDiff < bestArea) {
> to
>> +	if (ratioDiff <= bestRatio && areaDiff < bestArea) {
> Which is looks like a functional change to me, because crucially,
> previously to this patch the values could be updated if the bestRatio
> improved, regardless of the bestArea check.

Ok, so I agree, this is a functional change and I should remove the 
claim that it's not, from the commit message.

Speaking on the change itself, you'll find "[RFC PATCH] camera_sensor: 
Do not always prioritize aspect-ratios" interesting, with respect to

    if (ratioDiff <= bestRatio && areaDiff < bestArea) {

It tries to look at both ratioDiff and areaDiff, and see if a current 
sensor resolution configuration seems more apt or not, rather than 
ignoring the areaDiff(s), well in most cases :-)


>
>
> --
> Kieran
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210707/701fc8b6/attachment.htm>