[PATCH v3 09/16] ipa: rkisp1: Refactor automatic/manual structure in IPAActiveState

[Paul Elder]

On Thu, Apr 03, 2025 at 05:49:14PM +0200, Stefan Klug wrote:
> Swap gains and automatic/manual in the IPAActiveState structure. This is
> in preparation to adding another member, which is easier in the new
> structure. The patch contains no functional changes.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Use one named struct instead of two anonymous ones
> 
> Changes in v3:
> - Collected tags
> - Updated documentation
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 24 ++++++++++++------------
>  src/ipa/rkisp1/ipa_context.cpp    | 13 ++++++++-----
>  src/ipa/rkisp1/ipa_context.h      | 10 ++++++----
>  3 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index eafe93081bb1..a9759e53f593 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -124,8 +124,8 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
>  int Awb::configure(IPAContext &context,
>  		   const IPACameraSensorInfo &configInfo)
>  {
> -	context.activeState.awb.gains.manual = RGB<double>{ 1.0 };
> -	context.activeState.awb.gains.automatic =
> +	context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> +	context.activeState.awb.automatic.gains =
>  		awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
>  	context.activeState.awb.autoEnabled = true;
>  	context.activeState.awb.temperatureK = kDefaultColourTemperature;
> @@ -173,8 +173,8 @@ void Awb::queueRequest(IPAContext &context,
>  	const auto &colourTemperature = controls.get(controls::ColourTemperature);
>  	bool update = false;
>  	if (colourGains) {
> -		awb.gains.manual.r() = (*colourGains)[0];
> -		awb.gains.manual.b() = (*colourGains)[1];
> +		awb.manual.gains.r() = (*colourGains)[0];
> +		awb.manual.gains.b() = (*colourGains)[1];
>  		/*
>  		 * \todo Colour temperature reported in metadata is now
>  		 * incorrect, as we can't deduce the temperature from the gains.
> @@ -183,17 +183,17 @@ void Awb::queueRequest(IPAContext &context,
>  		update = true;
>  	} else if (colourTemperature) {
>  		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> -		awb.gains.manual.r() = gains.r();
> -		awb.gains.manual.b() = gains.b();
> +		awb.manual.gains.r() = gains.r();
> +		awb.manual.gains.b() = gains.b();
>  		awb.temperatureK = *colourTemperature;
>  		update = true;
>  	}
>  
>  	if (update)
>  		LOG(RkISP1Awb, Debug)
> -			<< "Set colour gains to " << awb.gains.manual;
> +			<< "Set colour gains to " << awb.manual.gains;
>  
> -	frameContext.awb.gains = awb.gains.manual;
> +	frameContext.awb.gains = awb.manual.gains;
>  	frameContext.awb.temperatureK = awb.temperatureK;
>  }
>  
> @@ -208,7 +208,7 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,
>  	 * most up-to-date automatic values we can read.
>  	 */
>  	if (frameContext.awb.autoEnabled) {
> -		frameContext.awb.gains = context.activeState.awb.gains.automatic;
> +		frameContext.awb.gains = context.activeState.awb.automatic.gains;
>  		frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
>  	}
>  
> @@ -325,14 +325,14 @@ void Awb::process(IPAContext &context,
>  	/* Filter the values to avoid oscillations. */
>  	double speed = 0.2;
>  	awbResult.gains = awbResult.gains * speed +
> -			  activeState.awb.gains.automatic * (1 - speed);
> +			  activeState.awb.automatic.gains * (1 - speed);
>  
> -	activeState.awb.gains.automatic = awbResult.gains;
> +	activeState.awb.automatic.gains = awbResult.gains;
>  
>  	LOG(RkISP1Awb, Debug)
>  		<< std::showpoint
>  		<< "Means " << rgbMeans << ", gains "
> -		<< activeState.awb.gains.automatic << ", temp "
> +		<< activeState.awb.automatic.gains << ", temp "
>  		<< activeState.awb.temperatureK << "K";
>  }
>  
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 99611bd5b390..39b97d143e95 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -191,14 +191,17 @@ namespace libcamera::ipa::rkisp1 {
>   * \var IPAActiveState::awb
>   * \brief State for the Automatic White Balance algorithm
>   *
> - * \struct IPAActiveState::awb.gains
> + * \var IPAActiveState::awb::AwbState

Shouldn't this be \struct ?

> + * \brief Struct for the AWB regulation state
> + *
> + * \struct IPAActiveState::awb::AwbState.gains

And this \var ?


Paul

>   * \brief White balance gains
>   *
> - * \var IPAActiveState::awb.gains.manual
> - * \brief Manual white balance gains (set through requests)
> + * \var IPAActiveState::awb.manual
> + * \brief Manual regulation state (set through requests)
>   *
> - * \var IPAActiveState::awb.gains.automatic
> - * \brief Automatic white balance gains (computed by the algorithm)
> + * \var IPAActiveState::awb.automatic
> + * \brief Automatic regulation state (computed by the algorithm)
>   *
>   * \var IPAActiveState::awb.temperatureK
>   * \brief Estimated color temperature
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 474f7036f003..6bc922a82971 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -89,10 +89,12 @@ struct IPAActiveState {
>  	} agc;
>  
>  	struct {
> -		struct {
> -			RGB<double> manual;
> -			RGB<double> automatic;
> -		} gains;
> +		struct AwbState {
> +			RGB<double> gains;
> +		};
> +
> +		AwbState manual;
> +		AwbState automatic;
>  
>  		unsigned int temperatureK;
>  		bool autoEnabled;
> -- 
> 2.43.0
>