[libcamera-devel] [PATCH v2] android: camera_device: Add null check for ScalerCrop control

[Jacopo Mondi]

Hi Phi-Bang,

On Fri, Mar 26, 2021 at 11:04:10PM +0100, Phi-Bang Nguyen wrote:
> The ScalerCrop control does not contain the null check which can
> cause the camera HAL crash at boot. Fix it.
>
> Signed-off-by: Phi-Bang Nguyen <pnguyen at baylibre.com>

I'm a bit debated as I feel the control should be reported by the
Camera (aka registered by the pipeline handler). True that if you have
no use for that, it should be safely defaulted to 1, as you do here
below.

> ---
>  src/android/camera_device.cpp | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae693664..a8108e3a 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1107,11 +1107,14 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		 * use the maximum and minimum crop rectangles to calculate the
>  		 * digital zoom factor.
>  		 */
> +		float maxZoom = { 1 };

Why a braced list initializer for a variable of type float ? It
compiles as C++ allows it, but it's a bit unusual. Is this me ?

>  		const auto info = controlsInfo.find(&controls::ScalerCrop);
> -		Rectangle min = info->second.min().get<Rectangle>();
> -		Rectangle max = info->second.max().get<Rectangle>();
> -		float maxZoom = std::min(1.0f * max.width / min.width,
> -					 1.0f * max.height / min.height);
> +		if (info != controlsInfo.end()) {
> +			Rectangle min = info->second.min().get<Rectangle>();
> +			Rectangle max = info->second.max().get<Rectangle>();
> +			maxZoom = std::min(1.0f * max.width / min.width,
> +					   1.0f * max.height / min.height);
> +		}

One minor nit. As you can see this code now lives in its own scope

        {
                ...
                init max digital zoom
                ...

        }

That was because I wanted to use 'min', 'max', 'info' and not care
about name clashes with other variables in the function. Now that you
have made the control registration conditional, I think you can remove
the additional scope and make this looks like:

        float maxZoom = 1.0;
        const auto scalerCrop = controlsInfo.find(&controls::ScalerCrop);
        if (scalerCrop != controlsInfo.end()) {

                ....
        }

What do you think ?

The patch is good for the rest
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  		staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
>  					  &maxZoom, 1);
>  	}
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel