[libcamera-devel,4/4] libcamera: ipa: raspberrypi: Populate focus FoM from the ISP statistics

8440 diff mbox series

Switch FocusStatus::num to unsigned int for convenience.

Fill in libcamera::controls::FocusFoM with the average of the middle two
regions (across a 4x3 grid) FoM statistics from the ISP.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/focus_status.h |  2 +-
 src/ipa/raspberrypi/raspberrypi.cpp           | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

Hi Naush

Thanks for this change. It looks good to me, I think you're right to average
the two middle zones. Though I'm sure the clamour to make them
adjustable will follow at some point...!

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Best regards
David

On Fri, 26 Jun 2020 at 11:25, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Switch FocusStatus::num to unsigned int for convenience.
>
> Fill in libcamera::controls::FocusFoM with the average of the middle two
> regions (across a 4x3 grid) FoM statistics from the ISP.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/focus_status.h |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h
> index 3ad88777..ace2fe2c 100644
> --- a/src/ipa/raspberrypi/controller/focus_status.h
> +++ b/src/ipa/raspberrypi/controller/focus_status.h
> @@ -17,7 +17,7 @@ extern "C" {
>  #endif
>
>  struct FocusStatus {
> -       int num;
> +       unsigned int num;
>         uint32_t focus_measures[FOCUS_REGIONS];
>  };
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 62730198..b1f27861 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -42,6 +42,7 @@
>  #include "contrast_status.h"
>  #include "controller.hpp"
>  #include "dpc_status.h"
> +#include "focus_status.h"
>  #include "geq_status.h"
>  #include "lux_status.h"
>  #include "metadata.hpp"
> @@ -405,6 +406,17 @@ void IPARPi::reportMetadata()
>                                          static_cast<int32_t>(blackLevelStatus->black_level_g),
>                                          static_cast<int32_t>(blackLevelStatus->black_level_g),
>                                          static_cast<int32_t>(blackLevelStatus->black_level_b) });
> +
> +       FocusStatus *focusStatus = rpiMetadata_.GetLocked<FocusStatus>("focus.status");
> +       if (focusStatus && focusStatus->num == 12) {
> +               /*
> +                * 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->focus_measures[5] + focusStatus->focus_measures[6]) / 2;
> +               libcameraMetadata_.set(controls::FocusFoM, focusFoM);
> +       }
>  }
>
>  /*
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Naush,

Thank you for the patch.

On Fri, Jun 26, 2020 at 11:25:31AM +0100, Naushir Patuck wrote:
> Switch FocusStatus::num to unsigned int for convenience.
> 
> Fill in libcamera::controls::FocusFoM with the average of the middle two
> regions (across a 4x3 grid) FoM statistics from the ISP.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Changes may be needed depending on the outcome of of the discussion on
1/4, but otherwise,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/ipa/raspberrypi/controller/focus_status.h |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h
> index 3ad88777..ace2fe2c 100644
> --- a/src/ipa/raspberrypi/controller/focus_status.h
> +++ b/src/ipa/raspberrypi/controller/focus_status.h
> @@ -17,7 +17,7 @@ extern "C" {
>  #endif
>  
>  struct FocusStatus {
> -	int num;
> +	unsigned int num;
>  	uint32_t focus_measures[FOCUS_REGIONS];
>  };
>  
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 62730198..b1f27861 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -42,6 +42,7 @@
>  #include "contrast_status.h"
>  #include "controller.hpp"
>  #include "dpc_status.h"
> +#include "focus_status.h"
>  #include "geq_status.h"
>  #include "lux_status.h"
>  #include "metadata.hpp"
> @@ -405,6 +406,17 @@ void IPARPi::reportMetadata()
>  					 static_cast<int32_t>(blackLevelStatus->black_level_g),
>  					 static_cast<int32_t>(blackLevelStatus->black_level_g),
>  					 static_cast<int32_t>(blackLevelStatus->black_level_b) });
> +
> +	FocusStatus *focusStatus = rpiMetadata_.GetLocked<FocusStatus>("focus.status");
> +	if (focusStatus && focusStatus->num == 12) {
> +		/*
> +		 * 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->focus_measures[5] + focusStatus->focus_measures[6]) / 2;
> +		libcameraMetadata_.set(controls::FocusFoM, focusFoM);
> +	}
>  }
>  
>  /*