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

[Jacopo Mondi]

Hi Daniel

On Mon, Mar 27, 2023 at 01:48:28PM +0200, Daniel Semkowicz wrote:
> 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?
>

mmm, that would make the whole IPA initialization fail if I'm not
mistaken. The algorithm is enabled by the sensor configuration file,
which indeed shouldn't include it if the device doesn't have a lens.

It also seems to me that if we don't fail, the AF algorithm
implementation would need to protect agains a missing lens with

        if (!context.configuration.lens)
                return 0;

in all its methods, as it will be part of the list of active
algorithms, and I'm not sure it's worth it.

If you agree with the above, then yes, I guess we should fail in
init() if no lens is available.


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