<div dir="ltr"><div>Hi Jacopo & Laurent,</div><div><br></div><div>Thank you for your reviews !</div><div>I take your comments in v3.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 29, 2021 at 10:17 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Mar 29, 2021 at 09:56:17AM +0200, Jacopo Mondi wrote:<br>
> Hi Phi-Bang,<br>
> <br>
> On Fri, Mar 26, 2021 at 11:04:10PM +0100, Phi-Bang Nguyen wrote:<br>
> > The ScalerCrop control does not contain the null check which can<br>
> > cause the camera HAL crash at boot. Fix it.<br>
> ><br>
> > Signed-off-by: Phi-Bang Nguyen <<a href="mailto:pnguyen@baylibre.com" target="_blank">pnguyen@baylibre.com</a>><br>
> <br>
> I'm a bit debated as I feel the control should be reported by the<br>
> Camera (aka registered by the pipeline handler). True that if you have<br>
> no use for that, it should be safely defaulted to 1, as you do here<br>
> below.<br>
<br>
We should really add a mandatory flag in controls_ids.yaml at some<br>
point. I'm sure we'll then spend quite a bit of time discussing which<br>
controls should be mandatory :-) For ScalerCrop, my initial opinion is<br>
that it should be optional, as not all platforms can scale.<br>
<br>
> > ---<br>
> >  src/android/camera_device.cpp | 11 +++++++----<br>
> >  1 file changed, 7 insertions(+), 4 deletions(-)<br>
> ><br>
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> > index ae693664..a8108e3a 100644<br>
> > --- a/src/android/camera_device.cpp<br>
> > +++ b/src/android/camera_device.cpp<br>
> > @@ -1107,11 +1107,14 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()<br>
> >  Â  Â  Â  Â  Â  Â  * use the maximum and minimum crop rectangles to calculate the<br>
> >  Â  Â  Â  Â  Â  Â  * digital zoom factor.<br>
> >  Â  Â  Â  Â  Â  Â  */<br>
> > +  Â  Â  Â  Â  Â float maxZoom = { 1 };<br>
> <br>
> Why a braced list initializer for a variable of type float ? It<br>
> compiles as C++ allows it, but it's a bit unusual. Is this me ?<br>
<br>
Indeed, just = 1.0f would be better.<br>
<br>
> >  Â  Â  Â  Â  Â  Â const auto info = controlsInfo.find(&controls::ScalerCrop);<br>
> > -  Â  Â  Â  Â  Â Rectangle min = info->second.min().get<Rectangle>();<br>
> > -  Â  Â  Â  Â  Â Rectangle max = info->second.max().get<Rectangle>();<br>
> > -  Â  Â  Â  Â  Â float maxZoom = std::min(1.0f * max.width / min.width,<br>
> > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  1.0f * max.height / min.height);<br>
> > +  Â  Â  Â  Â  Â if (info != controlsInfo.end()) {<br>
> > +  Â  Â  Â  Â  Â  Â  Â  Â  Â Rectangle min = info->second.min().get<Rectangle>();<br>
> > +  Â  Â  Â  Â  Â  Â  Â  Â  Â Rectangle max = info->second.max().get<Rectangle>();<br>
> > +  Â  Â  Â  Â  Â  Â  Â  Â  Â maxZoom = std::min(1.0f * max.width / min.width,<br>
> > +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  1.0f * max.height / min.height);<br>
> > +  Â  Â  Â  Â  Â }<br>
> <br>
> One minor nit. As you can see this code now lives in its own scope<br>
> <br>
>  Â  Â  Â  Â {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â ...<br>
>  Â  Â  Â  Â  Â  Â  Â  Â init max digital zoom<br>
>  Â  Â  Â  Â  Â  Â  Â  Â ...<br>
> <br>
>  Â  Â  Â  Â }<br>
> <br>
> That was because I wanted to use 'min', 'max', 'info' and not care<br>
> about name clashes with other variables in the function. Now that you<br>
> have made the control registration conditional, I think you can remove<br>
> the additional scope and make this looks like:<br>
> <br>
>  Â  Â  Â  Â float maxZoom = 1.0;<br>
>  Â  Â  Â  Â const auto scalerCrop = controlsInfo.find(&controls::ScalerCrop);<br>
>  Â  Â  Â  Â if (scalerCrop != controlsInfo.end()) {<br>
> <br>
>  Â  Â  Â  Â  Â  Â  Â  Â ....<br>
>  Â  Â  Â  Â }<br>
> <br>
> What do you think ?<br>
<br>
Looks good to me.<br>
<br>
> The patch is good for the rest<br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> <br>
> >  Â  Â  Â  Â  Â  Â staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,<br>
> >  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â &maxZoom, 1);<br>
> >  Â  Â }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div>Regards,</div><div><br></div><div>Phi-Bang Nguyen</div></div></div></div>