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

[Daniel Semkowicz]

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.
2. What if there are, for example, two AF algorithm implementations,
   with different supported controls? Which controls should be added
   to the control list?

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

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.

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

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