[libcamera-devel] [PATCH v2 04/11] pipeline: rkisp1: Add basic AF controls to the supported controls list

[Laurent Pinchart]

Hello,

On Thu, Jul 14, 2022 at 11:41:29PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:30:03)
> > On Wed, Jul 13, 2022 at 10:43:10AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > This will expose the AF controls and will allow controlling them using
> > > the top level API.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 7ee80192..99d66b1d 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -958,10 +958,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >       std::unique_ptr<RkISP1CameraData> data =
> > >               std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);
> > >
> > > -     ControlInfoMap::Map ctrls;
> > > -     ctrls.emplace(std::piecewise_construct,
> > > -                   std::forward_as_tuple(&controls::AeEnable),
> > > -                   std::forward_as_tuple(false, true));
> > > +     ControlInfoMap::Map ctrls({
> > > +             { &controls::AeEnable, ControlInfo(false, true) },
> > > +             { &controls::AfMode, ControlInfo(controls::AfModeValues) },
> > > +             { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) },
> > > +             { &controls::AfPause, ControlInfo(controls::AfPauseValues) }
> > 
> > Does the algorithm support continuous auto-focus ? Has it been tested ?
> > I've been told by those who tried implement it (RPi) that without PDAF
> > statistics from the sensor it's rather hard to get it right.
> > 
> > If the algorithm does not support it I would drop AfPause as it's
> > only useful for CAF (and the associated functions should probably be
> > removed along the class hierarchy as well maybe)
> 
> As this algorithm implementation is going into libipa, I wonder if we
> can get it (the common algorithm implementation) to report the controls
> it supports and merge that into the ControlList of supported controls
> exposed by the IPA?
> 
> That would help towards keeping the support modular, and choosing a
> different algorithm implementation (perhaps at runtime) could then
> correctly reflect the supported controls?

Yes, that would be better. The RPi IPA module has recently gone that
route, see commit 53ada24e63f4 ("pipeline: ipa: raspberrypi: Move
ControlInfoMap to the IPA").

> > > +     });
> > >
> > >       data->controlInfo_ = ControlInfoMap(std::move(ctrls),
> > >                                           controls::controls);

-- 
Regards,

Laurent Pinchart