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

[Kieran Bingham]

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.


--
Kieran