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

[David Plowman]

Hi Naush

Thanks for the patch.

Mostly I guess I'm OK with this, I find I'm just wondering a little
bit whether there's any point in the "rpi.focus" algorithm at all, and
whether we should just set the FocusFoM metadata unconditionally?

I suppose one thing it used to enable was for folks running headless
(or without X Windows) to set RPiFocus debug and get numbers printed
out to the console. Are we happy that we have other ways to do that
now?

David

On Wed, 22 Mar 2023 at 13:06, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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;
>                 libcameraMetadata_.set(controls::FocusFoM, focusFoM);
>         }
>
> --
> 2.34.1
>