[libcamera-devel] [PATCH v1 08/10] ipa: raspberrypi: Generalise the focus algorithm

[Jacopo Mondi]

Hi Naush

On Wed, Mar 22, 2023 at 01:06:10PM +0000, Naushir Patuck via libcamera-devel wrote:
> Generalise the focus reporting algorithm code by removing any hard-coded
> assumptions about the target hardware platform. Instead, the algorithm
> code uses the generalised statistics structure to determine focus region
> sizes.
>
> Remove focus_status.h as it is no longer needed with this change.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/focus_status.h | 20 -------------------
>  src/ipa/raspberrypi/controller/rpi/focus.cpp  | 12 ++---------
>  src/ipa/raspberrypi/raspberrypi.cpp           | 11 ++++++----
>  3 files changed, 9 insertions(+), 34 deletions(-)
>  delete mode 100644 src/ipa/raspberrypi/controller/focus_status.h
>
> diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h
> deleted file mode 100644
> index 8b74e59840c1..000000000000
> --- a/src/ipa/raspberrypi/controller/focus_status.h
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -/* SPDX-License-Identifier: BSD-2-Clause */
> -/*
> - * Copyright (C) 2020, Raspberry Pi Ltd
> - *
> - * focus_status.h - focus measurement status
> - */
> -#pragma once
> -
> -#include <linux/bcm2835-isp.h>
> -
> -/*
> - * The focus algorithm should post the following structure into the image's
> - * "focus.status" metadata. Recall that it's only reporting focus (contrast)
> - * measurements, it's not driving any kind of auto-focus algorithm!
> - */
> -
> -struct FocusStatus {
> -	unsigned int num;
> -	uint32_t focusMeasures[FOCUS_REGIONS];
> -};
> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> index ea3cc00e42c3..683d460ad6f7 100644
> --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> @@ -8,7 +8,6 @@
>
>  #include <libcamera/base/log.h>
>
> -#include "../focus_status.h"
>  #include "focus.h"
>
>  using namespace RPiController;
> @@ -30,15 +29,8 @@ char const *Focus::name() const
>
>  void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)
>  {
> -	FocusStatus status;
> -	for (unsigned int i = 0; i < stats->focusRegions.numRegions(); i++)
> -		status.focusMeasures[i] = stats->focusRegions.get(i).val;
> -	status.num = stats->focusRegions.numRegions();
> -	imageMetadata->set("focus.status", status);
> -
> -	LOG(RPiFocus, Debug)
> -		<< "Focus contrast measure: "
> -		<< (status.focusMeasures[5] + status.focusMeasures[6]) / 10;
> +	/* Pass the stats directly to the IPA to report out to the application */
> +	imageMetadata->set("focus.status", stats->focusRegions);
>  }
>
>  /* Register algorithm with the system. */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index b6e5465ca32a..181512de74ad 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -50,7 +50,6 @@
>  #include "denoise_algorithm.h"
>  #include "denoise_status.h"
>  #include "dpc_status.h"
> -#include "focus_status.h"
>  #include "geq_status.h"
>  #include "lux_status.h"
>  #include "metadata.h"
> @@ -640,14 +639,18 @@ void IPARPi::reportMetadata(unsigned int ipaContext)
>  					 static_cast<int32_t>(blackLevelStatus->blackLevelG),
>  					 static_cast<int32_t>(blackLevelStatus->blackLevelB) });
>
> -	FocusStatus *focusStatus = rpiMetadata.getLocked<FocusStatus>("focus.status");
> -	if (focusStatus && focusStatus->num == 12) {
> +	RPiController::FocusRegions *focusStatus =
> +		rpiMetadata.getLocked<RPiController::FocusRegions>("focus.status");
> +	if (focusStatus) {
> +		libcamera::Size size = focusStatus->size();
>  		/*
>  		 * We get a 4x3 grid of regions by default. Calculate the average
>  		 * FoM over the central two positions to give an overall scene FoM.
>  		 * This can change later if it is not deemed suitable.
>  		 */
> -		int32_t focusFoM = (focusStatus->focusMeasures[5] + focusStatus->focusMeasures[6]) / 2;
> +		ASSERT(size == Size(4, 3));
> +		int32_t focusFoM = (focusStatus->get({ 1, 1 }).val +
> +				    focusStatus->get({ 2, 1 }).val) / 2;

Not a 100% sure I understand this one, but
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
   j

>  		libcameraMetadata_.set(controls::FocusFoM, focusFoM);
>  	}
>
> --
> 2.34.1
>