[libcamera-devel,v2,08/11] ipa: rkisp1: Add "Windows" Metering mode to auto focus algorithm

16606 diff mbox series

Allow manually setting auto focus window. Currently only one window is
enabled, but ISP allows up to three of them.

Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
---
 src/ipa/rkisp1/algorithms/af.cpp         | 74 ++++++++++++++++++++++--
 src/ipa/rkisp1/algorithms/af.h           |  9 +++
 src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  2 +
 4 files changed, 99 insertions(+), 6 deletions(-)

Hi Daniel

On Wed, Jul 13, 2022 at 10:43:14AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> Allow manually setting auto focus window. Currently only one window is
> enabled, but ISP allows up to three of them.
>
> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> ---
>  src/ipa/rkisp1/algorithms/af.cpp         | 74 ++++++++++++++++++++++--
>  src/ipa/rkisp1/algorithms/af.h           |  9 +++
>  src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  2 +
>  4 files changed, 99 insertions(+), 6 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp
> index 6e047804..cd4f0e08 100644
> --- a/src/ipa/rkisp1/algorithms/af.cpp
> +++ b/src/ipa/rkisp1/algorithms/af.cpp
> @@ -20,20 +20,54 @@ namespace libcamera::ipa::rkisp1::algorithms {
>
>  LOG_DEFINE_CATEGORY(RkISP1Af)
>
> +
> +static rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)
> +{
> +	rkisp1_cif_isp_window ispwindow;
> +
> +	ispwindow.h_offs = rectangle.x;
> +	ispwindow.v_offs = rectangle.y;
> +	ispwindow.h_size = rectangle.width;
> +	ispwindow.v_size = rectangle.height;
> +
> +	return ispwindow;
> +}
> +

nit: libcamera uses anonymous namspaces for static functions

>  /**
>   * \copydoc libcamera::ipa::Algorithm::configure
>   */
>  int Af::configure([[maybe_unused]] IPAContext &context,
> -		  [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> +		  const IPACameraSensorInfo &configInfo)
>  {
> +	/* Default AF window of 3/4 size of the screen placed at the center */
> +	defaultWindow_ = Rectangle(configInfo.outputSize.width / 8,
> +				   configInfo.outputSize.height / 8,
> +				   3 * configInfo.outputSize.width / 4,
> +				   3 * configInfo.outputSize.height / 4);

This is never changed, could be made a class constexpr

> +	updateCurrentWindow(defaultWindow_);
> +
>  	return 0;
>  }
>
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void Af::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] rkisp1_params_cfg *params)
> +void Af::prepare([[maybe_unused]] IPAContext &context, rkisp1_params_cfg *params)
>  {
> +	if (updateAfwindow_) {

        if (!updateAfwindow_)
                return;

        And save one indentation level

Actually this function could likely grow and handle other controls, so
maybe it's fine like this

> +		params->meas.afc_config.num_afm_win = 1;
> +		params->meas.afc_config.thres = 128;
> +		params->meas.afc_config.var_shift = 4;
> +		/* \todo Allow setting thres and var_shift in tuning file */
> +
> +		params->meas.afc_config.afm_win[0] = rectangleToIspWindow(currentWindow_);

This function is only used here and basically initialize afm_win[0].
Should it be inlined here ?

> +
> +		params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;
> +		params->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;
> +		params->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;
> +
> +		updateAfwindow_ = false;

I wonder how this would look like with an std::optional<Rectangle> in
which could both store the value and the state to replacef
updateAfwindow_ and currentWindow_ with a single flag.

Only if you like the idea though :)

> +	}
>  }
>
>  /**
> @@ -55,14 +89,42 @@ void Af::process(IPAContext &context,
>  	context.frameContext.af.focus = lensPosition;
>  }
>
> -void Af::setMetering([[maybe_unused]] controls::AfMeteringEnum metering)
> +void Af::setMetering(controls::AfMeteringEnum metering)

setMeteringMode() ?

>  {
> -	LOG(RkISP1Af, Error) << __FUNCTION__ << " not implemented!";
> +	if (metering == meteringMode_)
> +		return;
> +
> +	if (metering == controls::AfMeteringWindows) {
> +		updateCurrentWindow(userWindow_);
> +	} else {
> +		updateCurrentWindow(defaultWindow_);
> +	}

No braces needed

> +
> +	meteringMode_ = metering;
> +}
> +
> +void Af::setWindows(Span<const Rectangle> windows)
> +{
> +	if (windows.size() != 1) {

 >= ?

Actually I wonder if there's any risk the Span<> of windows created by
the application can be of dynamic extent and give back INT_MAX here..

> +		LOG(RkISP1Af, Error) << "Only one AF window is supported";
> +		return;
> +	}
> +
> +	/* \todo Check if window size is valid for ISP */
> +
> +	LOG(RkISP1Af, Debug) << "setWindows: " << userWindow_;
> +
> +	userWindow_ = windows[0];
> +
> +	if (meteringMode_ == controls::AfMeteringWindows) {
> +		updateCurrentWindow(userWindow_);
> +	}

No braces and let's check this before assigning userWindow_.

Actually this might be subtle as it defines how the algorithm behaves
when it receives conflicting controls (as here it receives a windows
while it cannot apply it). This is something we should better define
in the controls description actually (for all controls, not just AF,
ideally they will all work the same).

Anyway, what should the algorithm do here ? Forget it and when later the
mode is switched back to manual wait for a new one, or cache it and
apply it as soon as the mode is switched ? I feel the first one is
more logic, but I guess we haven't reached full consensus.

Any opinion ?

>  }
>
> -void Af::setWindows([[maybe_unused]] Span<const Rectangle> windows)
> +void Af::updateCurrentWindow(const Rectangle &window)
>  {
> -	LOG(RkISP1Af, Error) << __FUNCTION__ << " not implemented!";
> +	currentWindow_ = window;
> +	updateAfwindow_ = true;
>  }
>
>  REGISTER_IPA_ALGORITHM(Af, "Af")
> diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h
> index e9afbb98..eeb806c0 100644
> --- a/src/ipa/rkisp1/algorithms/af.h
> +++ b/src/ipa/rkisp1/algorithms/af.h
> @@ -27,6 +27,15 @@ public:
>
>  	void setMetering(controls::AfMeteringEnum metering) override;
>  	void setWindows(Span<const Rectangle> windows) override;
> +
> +private:
> +	void updateCurrentWindow(const Rectangle &window);
> +
> +	controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto;
> +	Rectangle currentWindow_;
> +	Rectangle defaultWindow_;
> +	Rectangle userWindow_;
> +	bool updateAfwindow_ = false;
>  };
>
>  } /* namespace libcamera::ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 53b53f12..99ac1fb7 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -319,6 +319,26 @@ void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,
>  			af->setMode(static_cast<controls::AfModeEnum>(ctrlValue.get<int32_t>()));
>  			break;
>  		}
> +		case controls::AF_METERING: {
> +			Af *af = getAlgorithm<Af>();
> +			if (!af) {
> +				LOG(IPARkISP1, Warning) << "Could not set AF_WINDOWS - no AF algorithm";
> +				break;
> +			}
> +
> +			af->setMetering(static_cast<controls::AfMeteringEnum>(ctrlValue.get<int32_t>()));
> +			break;
> +		}
> +		case controls::AF_WINDOWS: {
> +			Af *af = getAlgorithm<Af>();
> +			if (!af) {
> +				LOG(IPARkISP1, Warning) << "Could not set AF_WINDOWS - no AF algorithm";
> +				break;
> +			}
> +
> +			af->setWindows(ctrlValue.get<Span<const Rectangle>>());
> +			break;
> +		}
>  		case controls::AF_TRIGGER: {
>  			Af *af = getAlgorithm<Af>();
>  			if (!af) {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 99d66b1d..7ab03057 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -961,6 +961,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	ControlInfoMap::Map ctrls({
>  		{ &controls::AeEnable, ControlInfo(false, true) },
>  		{ &controls::AfMode, ControlInfo(controls::AfModeValues) },
> +		{ &controls::AfMetering, ControlInfo(controls::AfMeteringValues) },
> +		{ &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },

This should be initialized to match the sensor's active pixel area if
I'm not mistaken

  - AfWindows:
      type: Rectangle
      description: |
        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.

  - ScalerCropMaximum:
      type: Rectangle
      description: |
        The maximum valid rectangle for the controls::ScalerCrop control. This
        reflects the minimum mandatory cropping applied in the camera sensor and
        the rest of the pipeline. Just as the ScalerCrop control, it defines a
        rectangle taken from the sensor's active pixel array.

>  		{ &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) },
>  		{ &controls::AfPause, ControlInfo(controls::AfPauseValues) }
>  	});
> --
> 2.34.1
>