[libcamera-devel,v2] android: camera_device: Add null check for ScalerCrop control
diff mbox series

Message ID 20210326220410.329299-1-pnguyen@baylibre.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2] android: camera_device: Add null check for ScalerCrop control
Related show

Commit Message

Phi-bang Nguyen March 26, 2021, 10:04 p.m. UTC 
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@baylibre.com>
---
 src/android/camera_device.cpp | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart March 28, 2021, 1:39 a.m. UTC | #1 
Hi Phi-Bang,

Thank you for the patch.

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.

The ScalerCrop control is indeed not mandatory. I'll add

Fixes: 31a1a628cd0e ("android: camera_device: Register MAX_DIGITAL_ZOOM")

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Jacopo, could you confirm that I'm not missing anything and that this is
correct ?

> Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com>
> ---
>  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 };
>  		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);
> +		}
>  		staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
>  					  &maxZoom, 1);
>  	}
Jacopo Mondi March 29, 2021, 7:56 a.m. UTC | #2 
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@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@jmondi.org>

Thanks
  j

>  		staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
>  					  &maxZoom, 1);
>  	}
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart March 29, 2021, 8:17 a.m. UTC | #3 
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@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@jmondi.org>
> 
> >  		staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
> >  					  &maxZoom, 1);
> >  	}
Phi-bang Nguyen March 29, 2021, 5:36 p.m. UTC | #4 
Hi Jacopo & Laurent,

Thank you for your reviews !
I take your comments in v3.

On Mon, Mar 29, 2021 at 10:17 AM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> 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@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@jmondi.org>
> >
> > >
>  staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
> > >                                       &maxZoom, 1);
> > >     }
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

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 };
 		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);
+		}
 		staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
 					  &maxZoom, 1);
 	}