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

[Daniel Semkowicz]

Hi,

On Fri, Jul 15, 2022 at 2:44 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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)

Algorithm supports continuous autofocus, but this part requires more
improvement, as currently this works exactly the same as for triggered
AF, just the trigger is automatically generated by detecting major
change in contrast value. And for manual trigger lens is always set to
the initial position. While for manual trigger this is not so big problem,
for the continuous AF, you certainly would like to have smooth lens control.


> >
> > 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").
>

I have seen these changes and I also think this is a good idea.

Best regards
Daniel

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