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

[David Plowman]

Hi everyone

I just wanted to pick up Nick's points, which I think are valid.

The default behaviour that you might want will often be hyperfocal,
but not always. Particularly if, as Nick said, the AfRange were set to
macro you'd probably want to define a specific default if the AF
algorithm is put into Macro on startup.

I'm always a bit paranoid about unnecessary lens movements too. As
things stand, if you wanted a different lens position, you couldn't
avoid sending the lens to hyperfocal, and then to the place that you
wanted. Could we delay the default movement until the camera starts,
then we can replace this by a single movement? It also lets us do
things like define a default which is appropriate for the range that
is being set, and gives an application control of it.

It would also help when you wanted to restart your camera app, but
leave the lens wherever it was when the app last finished. Delaying
the default movement until the user has a chance to set the lens
position where it was previously means you could avoid moving it
altogether. (There might still be a slight problem in finding out
where the lens ended up "last time"?)

Any thoughts?

Thanks!
David

On Mon, 5 Jun 2023 at 08:53, Laurent Pinchart via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Naush,
>
> On Fri, Jun 02, 2023 at 09:39:30AM +0100, Naushir Patuck wrote:
> > On Fri, 2 Jun 2023 at 07:43, Laurent Pinchart wrote:
> > > 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 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?
>
> As it changes the structure of the code a bit, I'd prefer a new version.
> I don't want to risk making a mistake with an untested change (I've made
> enough of those already).
>
> > > > > +                       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.
>
> I suppose we can enhance it later if needed.
>
> > > > > +               }
> > > > >         }
> > > > >
> > > > >         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