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

[Laurent Pinchart]

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.

> > +                       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 ?

> > +               }
> >         }
> >
> >         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