[libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits to the session config

[Daniel Semkowicz]

Hi Jacopo,

On Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Daniel
>
> On Fri, Mar 31, 2023 at 10:19:22AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > Add information about focus lens position limits to the IPA session
> > configuration. These information can then be used by IPA algorithms
> > to know which focus positions are valid.
> >
> > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > ---
> >  src/ipa/rkisp1/ipa_context.cpp | 17 +++++++++++++++++
> >  src/ipa/rkisp1/ipa_context.h   |  9 +++++++++
> >  src/ipa/rkisp1/rkisp1.cpp      | 23 ++++++++++++++++++++++-
> >  3 files changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 9bbf3684..aea99299 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -14,6 +14,18 @@
> >
> >  namespace libcamera::ipa::rkisp1 {
> >
> > +/**
> > + * \struct LensConfiguration
> > + * \brief Lens-specific parameters
> > + *
> > + * \var LensConfiguration::minFocusPosition
> > + * \brief Minimum position supported by the camera focus lens
> > + *
> > + * \var LensConfiguration::maxFocusPosition
> > + * \brief Maximum position supported by the camera focus lens
> > + *
> > + */
> > +
> >  /**
> >   * \struct IPASessionConfiguration
> >   * \brief Session configuration for the IPA module
> > @@ -89,6 +101,11 @@ namespace libcamera::ipa::rkisp1 {
> >   * \brief Sensor output resolution
> >   */
> >
> > +/**
> > + * \var IPASessionConfiguration::lens
> > + * \brief Contains lens-specific parameters if lens was detected
> > + */
> > +
> >  /**
> >   * \var IPASessionConfiguration::raw
> >   * \brief Indicates if the camera is configured to capture raw frames
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index b9b20653..65b3fbfe 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -8,6 +8,8 @@
> >
> >  #pragma once
> >
> > +#include <optional>
> > +
> >  #include <linux/rkisp1-config.h>
> >
> >  #include <libcamera/base/utils.h>
> > @@ -20,6 +22,11 @@ namespace libcamera {
> >
> >  namespace ipa::rkisp1 {
> >
> > +struct LensConfiguration {
> > +     int32_t minFocusPosition = 0;
> > +     int32_t maxFocusPosition = 0;
> > +};
> > +
> >  struct IPASessionConfiguration {
> >       struct {
> >               struct rkisp1_cif_isp_window measureWindow;
> > @@ -45,6 +52,8 @@ struct IPASessionConfiguration {
> >               Size size;
> >       } sensor;
> >
> > +     std::optional<LensConfiguration> lens;
> > +
> >       struct {
> >               rkisp1_cif_isp_version revision;
> >       } hw;
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index d338d374..292768cf 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -164,9 +164,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >       context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
> >                                                  * 1.0s / sensorInfo.pixelRate;
> >
> > -     if (!lensControls.empty())
> > +     if (!lensControls.empty()) {
> >               lensControls_ = lensControls;
> >
> > +             const ControlInfo &focusAbsolute =
> > +                     lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);
> > +
> > +             LOG(IPARkISP1, Debug)
> > +                     << "Focus lens: " << focusAbsolute.toString();
> > +
> > +             context_.configuration.lens = {
> > +                     .minFocusPosition = focusAbsolute.min().get<int32_t>(),
> > +                     .maxFocusPosition = focusAbsolute.max().get<int32_t>()
> > +             };
>
> Do you need to initialize context_.configuration.lens at init() time ?
>
> It doesn't seem so to me as in updateControls() below you anyway
> re-fetch V4L2_CID_FOCUS_ABSOLUTE from lensControls_.
>
> Can't it be done in configure() to avoid ...
>
> > +     }
> > +
> >       /* Load the tuning data file. */
> >       File file(settings.configurationFile);
> >       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> > @@ -234,6 +246,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> >               << "Exposure: [" << minExposure << ", " << maxExposure
> >               << "], gain: [" << minGain << ", " << maxGain << "]";
> >
> > +     /*
> > +      * Save the existing lens configuration and restore it after context
> > +      * reset. It does not change since init().
> > +      */
> > +     const std::optional<LensConfiguration> lensConfig =
> > +             context_.configuration.lens;
> > +
>
> ... this ?
>
> The computation seems very cheap and camera configuration happens once
> per streaming session, so I wonder if the it wouldn't be better to
> maintain a cleaner coding patch at the expense of this little cost.

As you already noticed in the next commits, the
context_.configuration.lens is used to validate early in the Af::init()
if lens is available and fail if it is missing. I would like to have
the lens configuration available as early as possible to be able to
check it early and it should not change during runtime.

I can move the context_.configuration.lens initialization to a separate
function and call it twice (in the init() and configure()) to avoid
copying and code duplication.

However, I do not understand why the context_.configuration is cleared
in the configure(). Problems with setting the same config multiple
times could be easily avoided, if configuration persist between
function calls. Parameters that change can just overwrite the existing
value, without clearing the whole structure. And right now, only the
lineDuration is set in the init().

>
> The rest looks good!
>
>
> >       /* Clear the IPA context before the streaming session. */
> >       context_.configuration = {};
> >       context_.activeState = {};
> > @@ -272,6 +291,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> >                       return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> >               });
> >
> > +     context_.configuration.lens = lensConfig;
> > +
> >       for (auto const &a : algorithms()) {
> >               Algorithm *algo = static_cast<Algorithm *>(a.get());
> >
> > --
> > 2.39.2
> >

Best regards
Daniel