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

[Naushir Patuck]

Hi David,

Thanks for the feedback.

On Wed, 22 Mar 2023 at 14:10, David Plowman <david.plowman at raspberrypi.com>
wrote:

> 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?
>

I do agree that rpi.focus is a bit redundant, and we ought to
unconditionally
add the focus stats into the metadata.  If folks are using debug log
messages to
get the numbers out, we should encourage them to pull it out of the
metadata in
their apps.  Conveniently, libcamera-apps had a recent change to output
stuff
like to the console without preview :-)

Regards,
Naush


>
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230322/c4d54478/attachment.htm>