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

[Kieran Bingham]

Quoting David Plowman via libcamera-devel (2023-06-12 10:08:43)
> 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,

It sounds reasonable that the first update to the lens position could be
set during camera->start() - perhaps by having a hook into something
like CameraLens->start() which would be the first point any actual
change is made?

(I wonder if this means we should also have a CameraSensor->start() ....
would that let us clean up controls that currently get passed during
camera->start()?)

But are there any conditions where this would be undesirable? Do we need
to wait for the lens to settle before handling any 3a for instance? I
expect 'most' of the AWB/AEGC shouldn't be too affected by the focus...

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

I could expect that during configure the CameraLens class could be
configured accordingly indeed. It would then cache the most recent state
until start, and would mean multiple configure calls wouldn't actually
cause an unnecessary movement...


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

I think that would be up to the camera application to save metadata
state and continue from there if it desired.

--
Kieran


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