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

12811 diff mbox series

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@ideasonboard.com>
---
 src/libcamera/camera_sensor.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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@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.


--
Kieran

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@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