[PATCH v2 09/17] ipa: rkisp1: Refactor automatic/manual structure in IPAActiveState

[Stefan Klug]

On Tue, Apr 01, 2025 at 04:16:23AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wed, Mar 19, 2025 at 05:11:14PM +0100, 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>
> > 
> > ---
> > 
> > Changes in v2:
> > - Use one named struct instead of two anonymous ones
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 24 ++++++++++++------------
> >  src/ipa/rkisp1/ipa_context.h      | 10 ++++++----
> >  2 files changed, 18 insertions(+), 16 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 =
> 
> The current order reads better than the new one, but I suppose easing
> addition of new fields is more important.
> 
> >  		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.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;
> 
> You're missing the documentation update. With that,

Ouch. Turns out the doxygen documentation doesn't include ipa/rkisp1 at
all. Adding that shows that we are missing waaay more things here. So I
removed it again and keep it for a later patch :-)

Nevertheless the v3 will contain updated docs for the fields at stake
here.

Best regards,
Stefan

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  
> >  		unsigned int temperatureK;
> >  		bool autoEnabled;
> 
> -- 
> Regards,
> 
> Laurent Pinchart