[v2,09/17] ipa: rkisp1: Refactor automatic/manual structure in IPAActiveState
diff mbox series

Message ID 20250319161152.63625-10-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Some rkisp1 awb improvements
Related show

Commit Message

Stefan Klug March 19, 2025, 4:11 p.m. UTC 
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@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(-)

Comments

Kieran Bingham March 31, 2025, 5:15 p.m. UTC | #1 
Quoting Stefan Klug (2025-03-19 16:11:14)
> 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@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 =
>                 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;

Later I wonder if we should have 'AwbState' defined as something in
libipa - so we can ensure all libipa users have the same control state
...

But that's on top/separate.

I'm fine with this if it helps simplify.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>                 unsigned int temperatureK;
>                 bool autoEnabled;
> -- 
> 2.43.0
>
Laurent Pinchart April 1, 2025, 1:16 a.m. UTC | #2 
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@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,

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

>  
>  		unsigned int temperatureK;
>  		bool autoEnabled;
Stefan Klug April 1, 2025, 9:11 p.m. UTC | #3 
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@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@ideasonboard.com>
> 
> >  
> >  		unsigned int temperatureK;
> >  		bool autoEnabled;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

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.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;