[libcamera-devel] [PATCH 1/3] ipa: rkisp1: Add enable field for AWB algorithm in IPA context

[Laurent Pinchart]

Hi Florian,

Thank you for the patch.

On Thu, Aug 04, 2022 at 04:12:26PM +0200, Florian Sylvestre via libcamera-devel wrote:
> Add an enable variable in the awb struct in IPASessionConfiguration which
> indicates if the awb algorithm has been configured. This will allow other
> algorithms to retrieve this information.
> 
> Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 2 ++
>  src/ipa/rkisp1/ipa_context.cpp    | 3 +++
>  src/ipa/rkisp1/ipa_context.h      | 1 +
>  3 files changed, 6 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 9f00364d..d1328f01 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -48,6 +48,8 @@ int Awb::configure(IPAContext &context,
>  	context.configuration.awb.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
>  	context.configuration.awb.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
>  
> +	context.configuration.awb.enabled = true;
> +
>  	return 0;
>  }
>  
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index ef8bb8e9..d833b8c7 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -87,6 +87,9 @@ namespace libcamera::ipa::rkisp1 {
>   *
>   * \var IPASessionConfiguration::awb.measureWindow
>   * \brief AWB measure window
> + *
> + * \var IPASessionConfiguration::awb.enabled
> + * \brief Indicates if AWB is enabled

I'm pretty sure someone reading this will interpret the comment as
meaning that auto white balance is enabled, as opposed to running in
manual mode, while the field indicates if the Algorithm is enabled,
regardless of whether it runs in manual or automatic mode. I'm OK
keeping the member name (unless you can think of a better name), but I'd
at least update the documentation. Maybe

 * \brief Indicates if the AWB hardware is enabled to apply colour gains

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

I've tested it by the way, and I can confirm that the gains are not
applied when the module is disabled.

>   */
>  
>  /**
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 2bdb6a81..7f7b3e4d 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -29,6 +29,7 @@ struct IPASessionConfiguration {
>  
>  	struct {
>  		struct rkisp1_cif_isp_window measureWindow;
> +		bool enabled;
>  	} awb;
>  
>  	struct {

-- 
Regards,

Laurent Pinchart