[libcamera-devel,v1,08/10] ipa: raspberrypi: Generalise the focus algorithm
diff mbox series

Message ID 20230322130612.5208-9-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Generalised algorithms
Related show

Commit Message

Naushir Patuck March 22, 2023, 1:06 p.m. UTC 
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@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

Comments

David Plowman March 22, 2023, 2:10 p.m. UTC | #1 
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@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@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
>
Naushir Patuck March 22, 2023, 2:39 p.m. UTC | #2 
Hi David,

Thanks for the feedback.

On Wed, 22 Mar 2023 at 14:10, David Plowman <david.plowman@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@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@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
> >
>
Jacopo Mondi March 24, 2023, 9:20 a.m. UTC | #3 
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@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@ideasonboard.com>

Thanks
   j

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

Patch
diff mbox series 

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);
 	}