[libcamera-devel] [PATCH v5 08/10] ipa: rkisp1: Add AF controls to the RkISP1 IPA

[Daniel Semkowicz]

On Mon, Mar 27, 2023 at 1:07 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Daniel
>
> On Mon, Mar 27, 2023 at 12:13:51PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > Hi Jacopo, Hi Kieran,
> >
> > On Sat, Mar 25, 2023 at 10:27 AM Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > Hi Kieran
> > >
> > > On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote:
> > > >
> > > >
> > > > On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote:
> > > > > Hi daniel
> > > > >
> > > > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > > > > > Add controls supported by the AF algorithm to the list of controls
> > > > > > supported by the RkISP1 IPA. This exposes the AF controls to the user
> > > > > > and allows controlling the AF algorithm using the top level API.
> > > > > >
> > > > > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > > > > ---
> > > > > >   src/ipa/rkisp1/rkisp1.cpp | 6 ++++++
> > > > > >   1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > index cd1fbae3..4b30844f 100644
> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > @@ -101,10 +101,16 @@ namespace {
> > > > > >   /* List of controls handled by the RkISP1 IPA */
> > > > > >   const ControlInfoMap::Map rkisp1Controls{
> > > > > >           { &controls::AeEnable, ControlInfo(false, true) },
> > > > > > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) },
> > > > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) },
> > > > >
> > > > > AfPauseDeferred is not supported
> > > > >
> > > >
> > > > What happens if there's no VCM though - Should we only expose the controls
> > >
> > > You're very right, I overlooked that.
> > >
> > > > if a VCM is identified? (Though that requires reworking the creation of this
> > > > rkisp1Controls list to be handled by the algorithms)
> > >
> > > I'm not sure it needs to be done by algorithms, but the IPA should
> > > certainly be made aware of the lens presence at init() time.
> >
> > This is indeed something that is needed, especially that number
> > of algorithm implementations is increasing. The most appropriate
> > solution seems for the algorithms to expose the supported control list
> > or to populate the list. This would also fix the problem of checking
> > outside the algorithm, if certain configuration is available, like
> > lens.
> >
> > I see at least two problems with the current approach:
> > 1. controls are exposed by the IPA, even if the algorithm is not
> >    enabled in the tuning file. This is confusing.
>
> Agreed
>
> > 2. What if there are, for example, two AF algorithm implementations,
> >    with different supported controls? Which controls should be added
> >    to the control list?
>
> Handling multiple algos for the same functions is not something I
> think there's a design for at the moment.
>
> If we defer registering ControlInfo to the single algorithms I guess
> the problem then reduces on how to run-time select which algorithm
> implementation to use.

I meant the current situation with ControlInfo map populated
in the IPA instead in algorithms itself. As you said, this should not
be a problem with the controls handled in the algorithms.

>
> >
> > >
> > > To be honest I would be fine with what RPi does in this regard. have a
> > > look at the usage of lensPresent_ in their IPA module init() function.
> > > This looks like the less invasive change.
> > >
> > > I presume this could even be made better by passing the lens controls'
> > > from the PH to the IPA at init() time so that the AF ControlInfo
> > > elements could be initialized with the proper limits and not with
> > > default values as it happens here and on RPi.
> >
> > For me, moving it to init() is also a better solution. I would like to
> > avoid intermediate steps and initialize IPA lens config in one place
> > for clarity. This means the whole lens configuration that's currently
> > done in the configure(), will need to be moved to init().
>
> The part about the lens in configure() intializes
>
>         lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>();
>         lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>();
>
> As you said, the lens min/max position -shouldn't- change between
> configurations so this part could be moved at init() time
>
> > However, I see one problem. context_.configuration struct is reset
> > in configure() for some reason. Even that
> > context_.configuration.sensor.lineDuration is set in init().  It is
> > reset and set again.
>
> The other parameters (such as the gains and shutter speed) change
> depending on the currently configured sensor's mode, that's why
> they're re-calculated at each configure() call.
>
> >
> > What about making the IPASessionConfiguration::lens structure
> > std::optional, so it will be explicitly stated in the session config
> > if the lens is available? Then, AF can fail in init() if lens
> > are not available.
> >
>
> Let's take a step back for a second.
>
> We have an updateControls() function, that starting for the static map
> rkisp1Controls augments it with controls whose values are computed by
> inspecting the sensorControls.
>
> Can't we do the same for the lens controls ? Add them only
> conditionally to the validity of lensControls ? Then if we deem it
> better we can pass to each algorithm the sensor's and lens
> configuration and have them populate the list of controls they handle.
>

Right, this makes sense. I can rework the code like this.
Also, don't you think it is worth to fail the AF init if it was
enabled on camera without moveable lens?

>
> > This will work for lens, but I am afraid We will not be able to
> > evaluate all AF ControlInfo elements in the init() phase.
> > For example, AF window size limits depend on the sensor output size,
> > which is only available in the configure().
>
> updateControls() has access to sensorInfo which contains the sensor's
> output size

Ah, I didn't notice that... In such case this is not a problem :)

>
> >
> > Best regards
> > Daniel
> >
> > >
> > > >
> > > > --
> > > > Kieran
> > > >
> > > >
> > > > > Otherwise
> > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > >
> > > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) },
> > > > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) },
> > > > > > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > > > >
> > > > > We should -really- move to instantiate control limits directly based
> > > > > on the sensor configuration...
> > > > >
> > > > > >           { &controls::AwbEnable, ControlInfo(false, true) },
> > > > > >           { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> > > > > >           { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
> > > > > >           { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) },
> > > > > > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) },
> > > > > >           { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) },
> > > > > >           { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> > > > > >           { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> > > > > > --
> > > > > > 2.39.2
> > > > > >