[libcamera-devel] [PATCH v4 05/10] ipa: af_hill_climbing: Add "Windows" metering mode

[Daniel Semkowicz]

Hi Jacopo,

On Mon, Mar 20, 2023 at 9:54 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Daniel
>
> On Tue, Mar 14, 2023 at 03:48:29PM +0100, Daniel Semkowicz wrote:
> > Add support for setting user defined auto focus window in the
> > AfHillClimbing. This enables usage of AfMetering and AfWindows controls.
> > Each time, there is a need for changing the window configuration in the
> > ISP, the signal is emitted. Platform layer that wants to use
> > the "Windows" metering mode, needs to connect to this signal
> > and configure the ISP on each emission.
> >
> > Currently only one window is supported.
>
> I wonder... shouldn't window hadling be left to the platform layer ?
> In other words, should the "common" Af::queueRequest() parse the
> windowing controls and then signal the platform layers, or either
> should parsing of windowing controls be done in
> RkISP1::Af::queueRequest() before (or after) having called the common
> class implementation of queueRequest ?

There is a simple state machine for window mode. This will need to be
duplicated in each platform implementation. This is why I see it fits
better in this place.
Do you see any obstacles that could make this code unusable for other
platforms?

> >
> > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > ---
> >  .../libipa/algorithms/af_hill_climbing.cpp    | 52 +++++++++++++++++--
> >  src/ipa/libipa/algorithms/af_hill_climbing.h  | 12 ++++-
> >  2 files changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > index 2b99afef..3421f240 100644
> > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > @@ -83,15 +83,28 @@ int AfHillClimbing::init(const YamlObject &tuningData)
> >   * \brief Configure the AfHillClimbing with sensor and lens information
> >   * \param[in] minFocusPosition Minimum position supported by camera lens
> >   * \param[in] maxFocusPosition Maximum position supported by camera lens
> > + * \param[in] outputSize Camera sensor output size
> >   *
> >   * This method should be called in the libcamera::ipa::Algorithm::configure()
> >   * method of the platform layer.
> >   */
> >  void AfHillClimbing::configure(int32_t minFocusPosition,
> > -                            int32_t maxFocusPosition)
> > +                            int32_t maxFocusPosition,
> > +                            const Size &outputSize)
> >  {
> >       minLensPosition_ = minFocusPosition;
> >       maxLensPosition_ = maxFocusPosition;
> > +
> > +     /*
> > +      * Default AF window of 3/4 size of the camera sensor output,
> > +      * placed at the center
> > +      */
> > +     defaultWindow_ = Rectangle(outputSize.width / 8,
> > +                                outputSize.height / 8,
> > +                                3 * outputSize.width / 4,
> > +                                3 * outputSize.height / 4);
> > +
> > +     windowUpdateRequested.emit(defaultWindow_);
> >  }
> >
> >  /**
> > @@ -165,6 +178,14 @@ void AfHillClimbing::setFramesToSkip(uint32_t n)
> >               framesToSkip_ = n;
> >  }
> >
> > +/**
> > + * \var AfHillClimbing::windowUpdateRequested
> > + * \brief Signal emitted when change in AF window was requested
> > + *
> > + * Platform layer supporting AF windows should connect to this signal
> > + * and configure the ISP with new window on each emition.
> > + */
> > +
> >  void AfHillClimbing::setMode(controls::AfModeEnum mode)
> >  {
> >       if (mode == mode_)
> > @@ -190,14 +211,35 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)
> >       LOG(Af, Error) << __FUNCTION__ << " not implemented!";
> >  }
> >
> > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)
> > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)
> >  {
> > -     LOG(Af, Error) << __FUNCTION__ << " not implemented!";
> > +     if (metering == meteringMode_)
> > +             return;
> > +
> > +     if (metering == controls::AfMeteringWindows) {
> > +             windowUpdateRequested.emit(userWindow_);
> > +     } else {
> > +             windowUpdateRequested.emit(defaultWindow_);
> > +     }
> > +
> > +     meteringMode_ = metering;
> >  }
> >
> > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows)
> > +void AfHillClimbing::setWindows(Span<const Rectangle> windows)
> >  {
> > -     LOG(Af, Error) << __FUNCTION__ << " not implemented!";
> > +     if (windows.size() != 1) {
> > +             LOG(Af, Error) << "Only one AF window is supported";
> > +             return;
> > +     }
> > +
> > +     /* \todo Check if window size is valid for ISP */
> > +
> > +     LOG(Af, Debug) << "setWindows: " << windows[0];
> > +
> > +     userWindow_ = windows[0];
> > +
> > +     if (meteringMode_ == controls::AfMeteringWindows)
> > +             windowUpdateRequested.emit(userWindow_);
> >  }
> >
> >  void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)
> > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h
> > index b361e5a1..4f507010 100644
> > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h
> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h
> > @@ -10,6 +10,7 @@
> >  #pragma once
> >
> >  #include <libcamera/base/log.h>
> > +#include <libcamera/base/signal.h>
> >
> >  #include "af.h"
> >
> > @@ -25,13 +26,16 @@ class AfHillClimbing : public Af
> >  {
> >  public:
> >       int init(const YamlObject &tuningData);
> > -     void configure(int32_t minFocusPosition, int32_t maxFocusPosition);
> > +     void configure(int32_t minFocusPosition, int32_t maxFocusPosition,
> > +                    const Size &outputSize);
> >       int32_t process(double currentContrast);
> >       void setFramesToSkip(uint32_t n);
> >
> >       controls::AfStateEnum state() final { return state_; }
> >       controls::AfPauseStateEnum pauseState() final { return pauseState_; }
> >
> > +     Signal<const Rectangle &> windowUpdateRequested;
> > +
> >  private:
> >       void setMode(controls::AfModeEnum mode) final;
> >       void setRange(controls::AfRangeEnum range) final;
> > @@ -54,6 +58,7 @@ private:
> >       controls::AfModeEnum mode_ = controls::AfModeManual;
> >       controls::AfStateEnum state_ = controls::AfStateIdle;
> >       controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;
> > +     controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;
> >
> >       /* VCM step configuration. It is the current setting of the VCM step. */
> >       int32_t lensPosition_ = 0;
> > @@ -87,6 +92,11 @@ private:
> >
> >       /* Max ratio of variance change, 0.0 < maxChange_ < 1.0 */
> >       double maxChange_;
> > +
> > +     /* Default AF window */
> > +     Rectangle defaultWindow_;
> > +     /* AF window set by user using AF_WINDOWS control */
> > +     Rectangle userWindow_;
> >  };
> >
> >  } /* namespace ipa::common::algorithms */
> > --
> > 2.39.2
> >

Best regards
Daniel