<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thanks for the feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 22 Mar 2023 at 14:10, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for the patch.<br>
<br>
Mostly I guess I'm OK with this, I find I'm just wondering a little<br>
bit whether there's any point in the "rpi.focus" algorithm at all, and<br>
whether we should just set the FocusFoM metadata unconditionally?<br>
<br>
I suppose one thing it used to enable was for folks running headless<br>
(or without X Windows) to set RPiFocus debug and get numbers printed<br>
out to the console. Are we happy that we have other ways to do that<br>
now?<br></blockquote><div><br></div><div>I do agree that rpi.focus is a bit redundant, and we ought to unconditionally<br>add the focus stats into the metadata. If folks are using debug log messages to<br>get the numbers out, we should encourage them to pull it out of the metadata in<br>their apps. Conveniently, libcamera-apps had a recent change to output stuff<br>like to the console without preview :-)<br></div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
David<br>
<br>
On Wed, 22 Mar 2023 at 13:06, Naushir Patuck via libcamera-devel<br>
<<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
><br>
> Generalise the focus reporting algorithm code by removing any hard-coded<br>
> assumptions about the target hardware platform. Instead, the algorithm<br>
> code uses the generalised statistics structure to determine focus region<br>
> sizes.<br>
><br>
> Remove focus_status.h as it is no longer needed with this change.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/controller/focus_status.h | 20 -------------------<br>
> src/ipa/raspberrypi/controller/rpi/focus.cpp | 12 ++---------<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 11 ++++++----<br>
> 3 files changed, 9 insertions(+), 34 deletions(-)<br>
> delete mode 100644 src/ipa/raspberrypi/controller/focus_status.h<br>
><br>
> diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h<br>
> deleted file mode 100644<br>
> index 8b74e59840c1..000000000000<br>
> --- a/src/ipa/raspberrypi/controller/focus_status.h<br>
> +++ /dev/null<br>
> @@ -1,20 +0,0 @@<br>
> -/* SPDX-License-Identifier: BSD-2-Clause */<br>
> -/*<br>
> - * Copyright (C) 2020, Raspberry Pi Ltd<br>
> - *<br>
> - * focus_status.h - focus measurement status<br>
> - */<br>
> -#pragma once<br>
> -<br>
> -#include <linux/bcm2835-isp.h><br>
> -<br>
> -/*<br>
> - * The focus algorithm should post the following structure into the image's<br>
> - * "focus.status" metadata. Recall that it's only reporting focus (contrast)<br>
> - * measurements, it's not driving any kind of auto-focus algorithm!<br>
> - */<br>
> -<br>
> -struct FocusStatus {<br>
> - unsigned int num;<br>
> - uint32_t focusMeasures[FOCUS_REGIONS];<br>
> -};<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp<br>
> index ea3cc00e42c3..683d460ad6f7 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp<br>
> @@ -8,7 +8,6 @@<br>
><br>
> #include <libcamera/base/log.h><br>
><br>
> -#include "../focus_status.h"<br>
> #include "focus.h"<br>
><br>
> using namespace RPiController;<br>
> @@ -30,15 +29,8 @@ char const *Focus::name() const<br>
><br>
> void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)<br>
> {<br>
> - FocusStatus status;<br>
> - for (unsigned int i = 0; i < stats->focusRegions.numRegions(); i++)<br>
> - status.focusMeasures[i] = stats->focusRegions.get(i).val;<br>
> - status.num = stats->focusRegions.numRegions();<br>
> - imageMetadata->set("focus.status", status);<br>
> -<br>
> - LOG(RPiFocus, Debug)<br>
> - << "Focus contrast measure: "<br>
> - << (status.focusMeasures[5] + status.focusMeasures[6]) / 10;<br>
> + /* Pass the stats directly to the IPA to report out to the application */<br>
> + imageMetadata->set("focus.status", stats->focusRegions);<br>
> }<br>
><br>
> /* Register algorithm with the system. */<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index b6e5465ca32a..181512de74ad 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -50,7 +50,6 @@<br>
> #include "denoise_algorithm.h"<br>
> #include "denoise_status.h"<br>
> #include "dpc_status.h"<br>
> -#include "focus_status.h"<br>
> #include "geq_status.h"<br>
> #include "lux_status.h"<br>
> #include "metadata.h"<br>
> @@ -640,14 +639,18 @@ void IPARPi::reportMetadata(unsigned int ipaContext)<br>
> static_cast<int32_t>(blackLevelStatus->blackLevelG),<br>
> static_cast<int32_t>(blackLevelStatus->blackLevelB) });<br>
><br>
> - FocusStatus *focusStatus = rpiMetadata.getLocked<FocusStatus>("focus.status");<br>
> - if (focusStatus && focusStatus->num == 12) {<br>
> + RPiController::FocusRegions *focusStatus =<br>
> + rpiMetadata.getLocked<RPiController::FocusRegions>("focus.status");<br>
> + if (focusStatus) {<br>
> + libcamera::Size size = focusStatus->size();<br>
> /*<br>
> * We get a 4x3 grid of regions by default. Calculate the average<br>
> * FoM over the central two positions to give an overall scene FoM.<br>
> * This can change later if it is not deemed suitable.<br>
> */<br>
> - int32_t focusFoM = (focusStatus->focusMeasures[5] + focusStatus->focusMeasures[6]) / 2;<br>
> + ASSERT(size == Size(4, 3));<br>
> + int32_t focusFoM = (focusStatus->get({ 1, 1 }).val +<br>
> + focusStatus->get({ 2, 1 }).val) / 2;<br>
> libcameraMetadata_.set(controls::FocusFoM, focusFoM);<br>
> }<br>
><br>
> --<br>
> 2.34.1<br>
><br>
</blockquote></div></div>