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

[Nick Hollinghurst]

Hi Laurent, Daniel,

On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Daniel,
>
> (Nick, there's a question for you below)
>
> On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote:
> > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote:
> > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote:
> > > > >
> > > > > Hi Daniel
> > > > >    cc RPi folks which originally introduced the AF controls and which
> > > > >    have recently been working a new implementation
> > > > >
> > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote:
> > > > > > >
> > > > > > > Hi Daniel
> > > > > > >    sorry I have neglected the Windows related patches in previous
> > > > > > > reviews as I hoped someone would have shared an opinion on the
> > > > > > > Signal-based mechanism.
> > > > > > >
> > > > > > > I don't have better ideas to propose, so let's go with it
>
> I don't like the signal much either. We could simply implement a
> function to retrieve the window.
>
> > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel 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
>
> s/Each time,/Each time/
>
> > > > > > > > ISP, the signal is emitted. Platform layer that wants to use
>
> s/layer that wants/Layers that want/
>
> > > > > > > > the "Windows" metering mode, needs to connect to this signal
>
> s/mode, needs/mode need/
>
> > > > > > > > and configure the ISP on each emission.
> > > > > > > >
> > > > > > > > Currently only one window is supported.
> > > > > > > >
> > > > > > > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > > > > > > ---
> > > > > > > >  .../libipa/algorithms/af_hill_climbing.cpp    | 62 +++++++++++++++++--
> > > > > > > >  src/ipa/libipa/algorithms/af_hill_climbing.h  | 10 +++
> > > > > > > >  2 files changed, 66 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > > > > > > > index 244b8803..0fb17df3 100644
> > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af)
> > > > > > > >   * movement range rather than coarse search result.
> > > > > > > >   * \todo Implement setRange.
> > > > > > > >   * \todo Implement setSpeed.
> > > > > > > > - * \todo Implement setMeteringMode.
> > > > > > > > - * \todo Implement setWindows.
> > > > > > > >   * \todo Implement the AfPauseDeferred mode.
> > > > > > > > + * \todo Implement support for multiple AF windows.
> > > > > > > >   */
> > > > > > > >
> > > > > > > >  /**
> > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition,
> > > > > > > >       return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * \brief Configure the AfHillClimbing with sensor information
> > > > > > > > + * \param[in] outputSize Camera sensor output size
> > > > > > > > + *
> > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()
>
> s/method/function/
>
> > > > > > > > + * method of the platform layer.
> > > > > > > > + */
> > > > > > > > +void AfHillClimbing::configure(const Size &outputSize)
> > > > > > >
> > > > > > > According to the AfWindows control definition
> > > > > > >
> > > > > > >         Sets the focus windows used by the AF algorithm when AfMetering is set
> > > > > > >         to AfMeteringWindows. The units used are pixels within the rectangle
> > > > > > >         returned by the ScalerCropMaximum property.
> > > > > > >
> > > > > > > The final sensor's output size can be obtained by downscaling/cropping
> > > > > > > a larger portion of the pixel array. The portion of the pixel array
> > > > > > > processed to obtain the final image is named AnalogueCropRectangle and
> > > > > > > all valid crop rectangles lie inside it.
> > > > > > >
> > > > > > > Most of our controls, such as ScalerCrop, are expressed with the
> > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the
> > > > > > > current output size. It's up to the pipeline handler to re-scale any
> > > > > > > valid control in the current configured output.
> > > > > >
> > > > > > For the 1024x768 stream, I have the following parameters:
> > > > > > - activeAreaSize: 2592x1944
> > > > > > - analogCrop: (0, 0)/2592x1944
> > > > > > - outputSize: 1296x972
> > > > > >
> > > > > > When using analogCrop, I will also need to scale the AF window
> > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes
> > > > > > values in reference to the ISP input size
> > > > > > (IPACameraSensorInfo::outputSize).
> > > > > > Based on what you wrote earlier, I understand that it is supposed to be
> > > > > > like that?
> > > > >
> > > > > I presume so.
> > > > >
> > > > > I should check how RPi handles windowing
>
> Nick, that's the first question for you.

Our implementation is based on what we understand to be the current
libcamera specification: That AF windows are in the same coordinate
system as ScalerCropMaximum, i.e. raw sensor pixels.

I found the documentation phrase "The units used are pixels within the
rectangle..." ambiguous -- does it suggest that the origin of the
AfWIndows coordinate system is the top-left corner of
ScalerCropMaximum? I took the answer to be "no", that those rectangles
were in a common coordinate system, without offsetting. This in turn
means that Af Windows will stick to the same objects in the scene,
regardless of mode or digital zoom.


> > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most
> > > > > > > probably to allow pipeline handler express any additional processing
> > > > > > > margins required by any cropping/scaling that happens on the ISP.
> > > > > > >
> > > > > > > However as the RkISP1 pipeline doesn't express any of those margin
> > > > > > > yet, I would simply use here the sensor's analogue crop, which is part
> > > > > > > of the IPACameraSensorInfo you already use in the platform layer.
> > > > > > >
> > > > > >
> > > > > > I have some concerns about the ScalerCropMaximum.
> > > > > > What if the ISP have different size margins for cropping and auto-focus
> > > > > > windows? I see there are margins defined for the AF window, but nothing
> > > > > > about the cropping margins in the RK3399 documentation. In this case,
> > > > > > it will not be a problem, but what if some ISP will have margins for
> > > > > > cropping, but no margins for AF window?
> > > > >
> > > > > Good point. Do we need an AfWindowMaximum ?
> > > >
> > > > To make sense, AfWindowMaximum would need to be expressed in reference
> > > > to how ISP handles that (sensor output size on rkisp) to allow precise
> > > > AF window configuration. AF window margins on rkisp are small: 2px and
> > > > 5px. I would like to avoid scaling, so small values compared to
> > > > the output size.
> > >
> > > For ScalerCrop we decided to have a ScalerCropMaximum property which
> > > basically reports the AnalogueCrop rectangle, as this is the maximum
> > > rectangle an application can set as a cropping area. The reference is
> > > the PixelArrayActiveAreas size.
> > >
> > > For AfWindows we don't care about AnalogCrop but only about the
> > > sensor's output as, if my understanding is correct, this is the
> > > rectangle on which the ISP performs windowing on.
> > >
> > > As we don't expose the sensor's output size to applications, the only
> > > reference we can use is full PixelArrayActiveAreas and re-scale before
> > > setting the windows.
> > >
> > > > I am not sure if AfWindowMaximum as additional property is actually
> > > > needed. Maximum value for AfWindows would not be enough?

Encoding the largest useful window as the control maximum value (where
this means minimum X, Y and maximum width,height?) sounds useful. BTW
is there any way to encode the maximum number of windows?

> > >
> > > As a comment on the ScalerCropMaximum property reports
> > >
> > >         \todo Turn this property into a "maximum control value" for the
> > >         ScalerCrop control once "dynamic" controls have been implemented.
> > >
> > > I presume this is very old, as right now (at least for RkISP1) I see
> > > the "ControlInfoMap *ipaControls" parameter passed in to configure() and
> > > being overwritten completely with new control limits.
> > >
> > > So my suggestion for a new property was probably not correct.
> > >
> > > > Is there a need to expose the window margins to the user at all?
> > >
> > > I think so, it would avoid setting controls which can't be applied as
> > > they are.
> > >
> > > I presume you can initialize the maximum value of AfWindows as
> > >
> > >         { 0, 0, PixelArrayActiveAreas.width - 4,
> > >           PixelArrayActiveAreas.height - 10 }
> > >
> > > When re-scaling, you first need to translate it to take into
> > > consideration the processing margins then re-scale it using the
> > > activeArea-to-outputSize ratio.
> > >
> > > (only compiled, not run-time tested)
> > >
> > >         Span<const Rectangle> userWindows =
> > >                 *request->controls().get<Span<const Rectangle>>(controls::AfWindows);
> > >         std::vector<Rectangle> afWindows;
> > >
> > >         for (auto userWindow : userWindows) {
> > >                 Rectangle window = userWindow.translatedBy({2, 5});
> > >                 window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize);
> > >                 afWindows.push_back(window);
> > >         }
> > >
> > >         (as you only support one window, this could be even
> > >         simplified)
> > >
> > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm
> > > implementation, and you would need to pass to the algorithm the active
> > > area size. One way to do so, as algorithms have access to the
> > > context_, is to add the active area size to
> > > IPASessionConfiguration::sensor which already contains the sensor's
> > > output size.
> > >
> > > What do you think ?
> >
> > I am not fully happy with this approach as this requires scaling
> > the values two times, but as you said, expressing it directly
> > in the output size would require exposing additional controls.
> >
> > The other way, I was thinking of, is to express the AF window
> > in the final output image size (1024x768 in my already mentioned
> > example). This way, it is easier to think about from the user
> > perspective. However, this is an opposite approach than the already
> > existing one for the ScalerCrop.
> >
> > Maybe I am not 100% happy with this, but it looks that expressing
> > the AF window as you show above (in reference to the current
> > PixelArrayActiveAreas) makes the most sense for the existing code,
> > because it will be the common approach with the ScalerCrop.
> > I can change my implementation to this approach.
> >
> > Do we need to change also the documentation related to the AF Windows?
> >
> > > > We could allow the window size of the whole ISP input size to be set
> > > > by the user and clamp the size in the IPA before configuring the ISP?
> > > >
> > > > In summary, I see two options:
> > > > 1. Maximum AF window expressed in units that are directly set to ISP
> > > >   (output size for rkisp). Maximum AF window will include margins.
> > > > 2. Keep the current approach and express the AF window in reference to
> > > >    Analogue crop size. Just allow the full size instead of the
> > > >    ScalerCropMaximum. Clamp the margins inside the IPA if user
> > > >    requested larger window.
> > > >
> > > > > RPi any opinions here ?
>
> And this is the second question for Nick.

I don't have very strong opinions. Option 2 seems the most general.
(Again, I've been assuming there's no implicit offsetting for the
"reference" rectangle -- so the only difference between those
definitions is a question of cropping?)

I suspect there is not a one-size-fits-all approach. Some sensors may
be able to generate focus information outside the cropped region;
others may not. Some platforms may only be able to measure focus
within the final image. Cropping "as required" sounds like a
reasonable way to resolve that -- but we ought to give the user some
way to predict when a window might end up being cropped to nothing
(and thus ignored)!

> > > > > > > To remove ambiguities I would call the parameter here "maxWindow" or
> > > > > > > something similar.
> > > > > > >
> > > > > > > I would also initialize userWindow_ to the same value, so that if an
> > > > > > > application switches to AfMeteringWindows mode the rectangle is at
> > > > > > > least initialized to something.
> > > > > >
> > > > > > Good point!
> > > > > >
> > > > > > > > +{
> > > > > > > > +     /*
> > > > > > > > +      * Default AF window of 3/4 size of the camera sensor output,
> > > > > > > > +      * placed at the center
> > > > > > > > +      */
> > > > > > > > +     defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8),
> > > > > > > > +                                static_cast<int>(outputSize.height / 8),
> > > > > > > > +                                3 * outputSize.width / 4,
> > > > > > > > +                                3 * outputSize.height / 4);
> > > > > > > > +
> > > > > > > > +     windowUpdateRequested.emit(defaultWindow_);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  /**
> > > > > > > >   * \brief Run the auto focus algorithm loop
> > > > > > > >   * \param[in] currentContrast New value of contrast measured for current frame
> > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(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
>
> s/layer/layers/
> s/connect to/connect/
>
> > > > > > > > + * and configure the ISP with new window on each emition.
>
> s/new window/the new window/
> s/emition/emission/
>
> The new window will likely need a few frames to become effective. I'm
> worried that the AF algorithm doesn't take that into account at all (it
> also doesn't take the lens movement delays into account, which is
> another of my worries). This can affect the performance and stability of
> the auto-focus algorithm.
>
> How have you tested this ?
>
> > > > > > > > + */
> > > > > > > > +
> > > > > > > >  void AfHillClimbing::setMode(controls::AfModeEnum mode)
> > > > > > > >  {
> > > > > > > >       if (mode == mode_)
> > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)
> > > > > > > >       LOG(Af, Error) << "setSpeed() not implemented!";
> > > > > > > >  }
> > > > > > > >
> > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)
> > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering)
> > > > > > > >  {
> > > > > > > > -     LOG(Af, Error) << "setMeteringMode() not implemented!";
> > > > > > > > +     if (metering == meteringMode_)
> > > > > > > > +             return;
> > > > > > > > +
> > > > > > > > +     meteringMode_ = metering;
> > > > > > > > +
> > > > > > > > +     switch (metering) {
> > > > > > > > +     case controls::AfMeteringAuto:
> > > > > > > > +             windowUpdateRequested.emit(defaultWindow_);
> > > > > > > > +             break;
> > > > > > > > +     case controls::AfMeteringWindows:
> > > > > > > > +             windowUpdateRequested.emit(userWindow_);
> > > > > > > > +             break;
>
> This needs a default case to avoid modifying meteringMode_. We have two
> modes currently defined, if we add a third one, the implementation will
> break.
>
> When the window is changed the contrast value may drastically change.
> The optimal focus point will likely change too. I think you need to
> reset the AF and possibly trigger a rescan for continuous AF mode.
>
> > > > > > > > +     }
> > > > > > > >  }
> > > > > > > >
> > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows)
> > > > > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows)
> > > > > > > >  {
> > > > > > > > -     LOG(Af, Error) << "setWindows() not implemented!";
> > > > > > > > +     if (windows.size() != 1) {
> > > > > > > > +             LOG(Af, Error) << "Only one AF window is supported";
> > > > > > > > +             return;
> > > > > > > > +     }
> > > > > > >
> > > > > > > Should this be an hard error or should we just want and continue by
> > > > > > > only considering the first available rectangle ?
> > > > > >
> > > > > > I will change it to a warning that only the first window was used.
> > > > > >
> > > > > > > > +
> > > > > > > > +     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 2147939b..0f7c65db 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"
> > > > > > > >
> > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af
> > > > > > > >  public:
> > > > > > > >       int init(int32_t minFocusPosition, int32_t maxFocusPosition,
> > > > > > > >                const YamlObject &tuningData);
> > > > > > > > +     void configure(const Size &outputSize);
> > > > > > > >       int32_t process(double currentContrast);
> > > > > > > >       void skipFrames(uint32_t n);
> > > > > > > >
> > > > > > > >       controls::AfStateEnum state() override { return state_; }
> > > > > > > >       controls::AfPauseStateEnum pauseState() override { return pauseState_; }
> > > > > > > >
> > > > > > > > +     Signal<const Rectangle &> windowUpdateRequested;
> > > > > > > > +
> > > > > > > >  private:
> > > > > > > >       void setMode(controls::AfModeEnum mode) override;
> > > > > > > >       void setRange(controls::AfRangeEnum range) override;
> > > > > > > > @@ -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;
> > > > > > > >
> > > > > > > >       /* Current focus lens position. */
> > > > > > > >       int32_t lensPosition_ = 0;
> > > > > > > > @@ -84,6 +89,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::algorithms */
>
> --
> Regards,
>
> Laurent Pinchart

Regards,

 Nick