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

[Phi-bang Nguyen]

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


-- 
Regards,

Phi-Bang Nguyen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210329/3ac23dd8/attachment.htm>