[libcamera-devel] [PATCH v6 08/10] ipa: rkisp1: Add AF controls to the RkISP1 IPA

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Tue, Apr 04, 2023 at 11:49:59AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> On Mon, Apr 3, 2023 at 11:37 AM Jacopo Mondi wrote:
> > On Fri, Mar 31, 2023 at 10:19:28AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > Add controls supported by the AF algorithm to the list of controls
> > > supported by the RkISP1 IPA. This exposes the AF controls to the user
> > > and allows controlling the AF algorithm using the top level API.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 292768cf..9c8b4a82 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -456,6 +456,28 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > >                                                             frameDurations[1],
> > >                                                             frameDurations[2]);
> > >
> > > +     if (lensControls_) {
> > > +             const ControlInfo &focusAbsolute =
> > > +                     lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);
> > > +
> > > +             using namespace controls;
> > > +
> > > +             ctrlMap[&AfMetering] = ControlInfo(AfMeteringValues);
> > > +             ctrlMap[&AfMode] = ControlInfo(AfModeValues);
> > > +             ctrlMap[&AfPause] = ControlInfo(
> > > +                     Span<const ControlValue>{
> > > +                             { static_cast<int32_t>(AfPauseImmediate),
> > > +                               static_cast<int32_t>(AfPauseResume) } });
> > > +             ctrlMap[&AfTrigger] = ControlInfo(AfTriggerValues);
> > > +             ctrlMap[&AfWindows] = ControlInfo(
> > > +                     Rectangle(), Rectangle(sensorInfo.outputSize),
> >
> > I wonder if the ISP doesn't have a minimum valid window size..
> 
> Unfortunately, I did not find any documentation on that.
> Only the window margins.

There doesn't seem to be a minimum size other than (1, 1), as it makes
no sense to compute AF stats on an empty window. I'd set the minimum to
that value.

> > > +                     Rectangle());

The default should match the 3/4 of the image used by default by the AF
algorithm.

> > > +             ctrlMap[&LensPosition] = ControlInfo(
> > > +                     static_cast<float>(focusAbsolute.min().get<int32_t>()),
> > > +                     static_cast<float>(focusAbsolute.max().get<int32_t>()),
> > > +                     static_cast<float>(focusAbsolute.def().get<int32_t>()));
> >
> > We're here exposing the values as they come from the v4l2 control
> > interface. We should get to a point where we have a CameraLensHelper
> > like we have CameraSensorHelpers to translate the platform-specific
> > value to a generic value. I can add a \todo when applying if you agree
> > with this.
> 
> Yes, I agree. For now, let's start with something basic that works :)

We don't have to calibrate the values yet, but they should already be
expressed in 1/distance units.

> > The rest looks good
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > > +     }
> > > +
> > >       *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > >  }
> > >

-- 
Regards,

Laurent Pinchart