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

[Laurent Pinchart]

Hello,

(CC'ing David)

On Thu, May 18, 2023 at 03:14:04PM +0200, Daniel Semkowicz wrote:
> On Wed, Apr 26, 2023 at 6:30 PM Nick Hollinghurst wrote:
> > On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart wrote:
> > > 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.
> 
> Signal has the advantage that it will be invoked only when the window
> has changed. With regular function, the function will need to be called
> on each frame to check if there is a new window.
> What are the disadvantages of using the signal?

I was initially thinking we could always set the AF parameters in the
ISP configuration buffer, but that would burn extra CPU cycles in the
kernel in interrupt context (for rkisp1 at least) to reprogram
registers, which should be avoided.

The issue with a signal is that it creates a mid-layer, in the sense
that the AF implementation mandates a particular implementation of the
platform-specific code. You would need to clearly specify where the
signal could be emitted from, as the platform-specific code may not
operate properly when getting the window changed signalled at random
times.

For instance, you're currently emitting the signal from

- AfHillClimbing::configure()
- AfHillClimbing::setMeteringMode()
- AfHillClimbing::setWindows()

The last two functions are called from algorithms::Af::queueRequest().
The signal is connect to the
rkisp1::algorithms::Af::updateCurrentWindow() function, which just
stores the rectangle in the updateWindow_ member variable, and that
variable is used in rkisp1::algorithms::Af::prepare() to set the ISP
parameters buffer. This means that rkisp1::algorithms::Af::prepare()
operates with the parameters set by the very latest request, which isn't
right. It should operate with the parameters set for the request that
corresponds to the frame being prepared. Fixing that is possible, using
the frame context (we do so in the AWB algorithm for instance), but that
will require the signal handler to know exactly where the signal can be
emitted from, and get the right context. The context isn't passed to the
signal handler (it doesn't get told for which request the window is
set), so we'll have a problem. The rkisp1::algorithms::Af class could
still implement this correctly by knowing the signal is emitted from the
algorithms::Af::queueRequest() function, and using updateWindow_ in
rkisp1::algorithms::Af::queueRequest() after calling
algorithms::Af::queueRequest(), but that's implicit knowledge of the
internal algorithms::Af helper implementation, which could change in the
future.

I think it will be simpler if the platform queries the AF window
manually, as it will then be in full control of when it retrieves the
value and how it uses it. This means the new value will need to be
compared with a cached value to decide whether or not to configure the
ISP, but that's simpler than going through a signal in the end.

> > > > > > > > > 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 had the same problems with understanding this documentation part, so
> probably it should be rephrased to clearly state what is the reference
> coordination system for AfWindows.

David, git pointed to you as the author of the text :-) I share Nick's
understanding here, what is yours ?

The documentation should be clarified, and I would like to propose
changing the wording to make AfWindows relative to the
PixelArrayActiveAreas property, not the ScalerCropMaximum. The
coordinate system for ScalerCropMaximum isn't defined, but the
coordinate system for ScalerCrop is documented as relative to
PixelArrayActiveAreas. Having both ScalerCrop and AfWindows expressed
in the same coordinate system would make sense to me.

There's still a question of what to do with windows outside (or
partially outside) of the ScalerCrop. Does it make sense to allow the
user to focus on something that isn't visible in the output image ? Use
cases seem dubious to me, and well-written applications will likely
ensure that the AF windows full within the visible image, but should we
enforce this in libcamera ?

> > > > > > > > > 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?
> 
> That is a good point. If we specify the maximum windows size, we should
> also specify the maximum number of windows.
> The only solution I can think of is something like this:
> 
>     Rectangle minWindow(0, 0, 0, 0);
>     ControlValue min(Span<const Rectangle>({minWindow}));
> 
>     Rectangle maxWindow(0, 0, 640, 480);
>     ControlValue max(Span<const Rectangle>({maxWindow, maxWindow, maxWindow}));
> 
>     Rectangle defWindow(100, 100, 200, 200);
>     ControlValue def(Span<const Rectangle>({defWindow}));
> 
>     ctrls[&controls::AfWindows] = ControlInfo(min, max,def);
> 
> cam lists such control as:
> 
>     Control: AfWindows: [[ (0, 0)/0x0 ]..[ (0, 0)/640x480, (0,
> 0)/640x480, (0, 0)/640x480 ]]
> 
> I am not convinced this is a readable solution for the user...
> Especially, if there will be bigger number of windows supported.
> I expect that minimum and maximum size of will be the same for all
> windows, regardless of the number of windows.
> From this, it can be deduced that we could expose only the simple pair:
> { window size, number of windows } for each min, max and def ControlInfo
> value.

How about simply adding a AfMaxWindows property ?

> > > > >
> > > > > 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)!
> 
> Ok, so my idea with cropping the margins without the user knowledge
> is not a good solution... Then, probably We end up with something
> similar to the ScalerCrop that is already implemented in the Raspberry
> implementation, if I understand it correctly:
> 
> The (x,y) location of the AfWindow is relative to the
> PixelArrayActiveAreas that is being used. The units remain native sensor
> pixels. Maximum value of the AfWindows ControlInfo (position + size),
> already includes all the margins that need to be considered
> (e.g. required by the ISP).
> 
> Would you agree with the above definition?

I think so. Depending on the platform, the AF statistics can be computed
right away on the image received from the sensor, or after some ISP
processing steps that will crop a few lines and columns on the edges. I
would like to avoid a need for applications to be aware of that
cropping, as it should be small. If the window(s) need to be expressed,
at the hardware level, in a cropped coordinate system, the IPA module
should perform the translation between PixelArrayActiveAreas and the
hardware coordinates.

If analog crop and/or digital crop gets applied in the sensor, there may
be significant cropping, and the application may need to be aware of
this. I think you can ignore this for now, we'll need to revisit the
processing pipeline model exposed to applications at some point.

For platforms that calculate the AF statistics directly on the sensor
(or more precisely, where the sensor generates AF data, consumed by the
algorithms directly or after processing, as in the PDAF case), the
windows could even exceed the sensor digital crop (I doubt they could
exceed the analog crop, as pixels outside the analog crop rectangle are
not read out by definition). We will have to take that into account too
I suppose, but I think you can ignore it for now as you're not dealing
with sensor-side AF data.

> > > > > > > > > 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.
> 
> The delay when configuring new window is set in the platform layer in
> the patch 07/10. The delay can be different for different ISPs, so I
> decided it will be better to leave the delay control to the platform
> layer.

The ISP doesn't add a delay as such. What happens is that a set of ISP
parameters buffers are queued to the ISP, and they are applied one by
one to consecutive frames. There's no internal ISP delay, the delay
comes from the fact that we queue parameters buffers, so the parameters
in the last queued buffer will only be applied after all other buffers
get processed. This should be handled by programming the AF window in an
ISP parameters buffer not right away when it gets queued in a request,
but at the right time so that it will be applied to the correct frame.

> Lens delay is a more complex problem and there were discussions
> previously about it. We need a proper solution for that.
> As a workaround, I introduced  the "wait-frames-lens" tuning file
> parameter in 06/10.

The physical properties of the lens movement is indeed a separate and
more complex question, which I'm fine handling separately (and later).

> > > How have you tested this ?
> 
> Yes, and most of the delays configured using AfHillClimbing::skipFrames()
> base on my experiments on RK3399 and camera lens available on my device.
> Unfortunately, the documentation I have access to, says very little
> about the delays in ISP.

Good news, there's no delay in the ISP itself :-)

> > > > > > > > > > + */
> > > > > > > > > > +
> > > > > > > > > >  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.
> 
> Well, in v4 there was a suggestion to remove the default case from
> switch statements, so the compiler will complain early about the missing
> implementation.

Good point, please ignore my comment :-)

> > > 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.
> 
> This is true and sounds like a general problem. Do you think this
> behaviour should be documented, so other platforms will also follow this
> rule?

That's a good idea I think. I'm not sure where to best document it
though, the documentation in control_ids.yaml shouldn't assume a
particular type of AF implementation, and PDAF-based platforms don't
need a rescan as such as they don't really scan (well, they may as a
fallback...). We could write the requirement in a generic way that
doesn't imply a particular AF method, or document it elsewhere. David,
any opinion ?

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