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

[Jacopo Mondi]

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

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

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.

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.


> +{
> +	/*
> +	 * 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
> + * and configure the ISP with new window on each emition.
> + */
> +
>  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;
> +	}
>  }
>
> -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 ?

> +
> +	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 */
> --
> 2.39.2
>