[libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to hyperfocal on startup

[Naushir Patuck]

Hi Laurent,

Thank you for the feedback!

On Fri, 2 Jun 2023 at 07:43, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Thu, Jun 01, 2023 at 11:33:15AM +0100, Nick Hollinghurst via libcamera-devel wrote:
> > On Thu, 1 Jun 2023 at 10:56, Naushir Patuck <naush at raspberrypi.com> wrote:
> > >
> > > On the first ipa->configure() call, set the lens position (if a lens is
> > > present) to the default position. Typically this would be the hyperfocal
> > > position based on the tuning data.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > > index 599ad146a863..4413689b7bef 100644
> > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
> > >                 agcStatus.shutterTime = defaultExposureTime;
> > >                 agcStatus.analogueGain = defaultAnalogueGain;
> > >                 applyAGC(&agcStatus, ctrls);
> > > +
> > > +               /* Set the lens to the default (typically hyperfocal) position on first start. */
> > > +               RPiController::AfAlgorithm *af =
> > > +                       dynamic_cast<RPiController::AfAlgorithm *>(controller_.getAlgorithm("af"));
> > > +               if (lensPresent_ && af) {
>
> You could avoid looking up the AF algorithm is lensPresent_ is false.
> That's a small optimization in a non-hot path, so it's not mandatory.

That's fine to do.  Would you want me to post another patch or can it be done
when applying?

>
> > > +                       float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get<float>();
> > > +                       ControlList lensCtrl(lensCtrls_);
> > > +                       int32_t hwpos;
> > > +
> > > +                       af->setLensPosition(defaultPos, &hwpos);
> > > +                       lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);
> > > +                       result->lensControls = std::move(lensCtrl);
>
> Will this work as expected if the user sets the AF mode to continuous at
> start time ? Would it cause the lens to first be moved to the hyperfocal
> distance, to be overridden right after ?

Yes, it does exactly that.  It's only a minor inconvenience, and in reality the
user is not going to get to see this movement as the startup frames are dropped
anyway.

Regards,
Naush

>
> > > +               }
> > >         }
> > >
> > >         result->sensorControls = std::move(ctrls);
> >
> > I think this is good, but it suggests future cleanups:
> >
> > There's an opportunity to simplify the RPI AF algorithm, which
> > deliberately supports an initial state in which it doesn't touch the
> > lens and doesn't know where it is. If lens position is always
> > initialized, a flag and some code paths can be removed there. But
> > there's currently no way to retrieve "hwpos" without either setting a
> > lens position or processing a frame; so it might entail a change to
> > AfAlgorithm interface.
> >
> > Based on the existing interface, this patch is doing the best thing.
> >
> > There's a complication with the concept of "default" too: The tuning
> > file can define defaults for each range ("normal", "macro", "full"),
> > but none of those match the static default and limits in the
> > ControlInfo. This might be part of a more general issue with control
> > limits, so I don't think it needs addressing here.
>
> --
> Regards,
>
> Laurent Pinchart