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

[Laurent Pinchart]

On Mon, Mar 29, 2021 at 09:56:17AM +0200, Jacopo Mondi wrote:
> 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.

We should really add a mandatory flag in controls_ids.yaml at some
point. I'm sure we'll then spend quite a bit of time discussing which
controls should be mandatory :-) For ScalerCrop, my initial opinion is
that it should be optional, as not all platforms can scale.

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

Indeed, just = 1.0f would be better.

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

Looks good to me.

> The patch is good for the rest
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  		staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
> >  					  &maxZoom, 1);
> >  	}

-- 
Regards,

Laurent Pinchart