[04/10] ipa: rkisp1: Refactor automatic/manual structure in IPAActiveState
diff mbox series

Message ID 20250217100203.297894-5-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Some rkisp1 awb improvements
Related show

Commit Message

Stefan Klug Feb. 17, 2025, 10:01 a.m. UTC 
Swap gains and automatic/manual in the IPAActiveState structure. This is
in preparation to adding another member, which is easier in this
structure.  This is also in sync with how it is modeled in agc. This
patch contains no functional changes.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/awb.cpp | 24 ++++++++++++------------
 src/ipa/rkisp1/ipa_context.h      |  9 ++++++---
 2 files changed, 18 insertions(+), 15 deletions(-)

Comments

Kieran Bingham Feb. 17, 2025, 12:06 p.m. UTC | #1 
Quoting Stefan Klug (2025-02-17 10:01:45)
> Swap gains and automatic/manual in the IPAActiveState structure. This is
> in preparation to adding another member, which is easier in this
> structure.  This is also in sync with how it is modeled in agc. This
> patch contains no functional changes.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 24 ++++++++++++------------
>  src/ipa/rkisp1/ipa_context.h      |  9 ++++++---
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index cffaa06a22c1..147277c98bb2 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -74,8 +74,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 = RGB<double>{ 1.0 };
> +       context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> +       context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
>         context.activeState.awb.autoEnabled = true;
>         context.activeState.awb.temperatureK = kDefaultColourTemperature;
>  
> @@ -120,8 +120,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.
> @@ -130,17 +130,17 @@ void Awb::queueRequest(IPAContext &context,
>                 update = true;
>         } else if (colourTemperature && colourGainCurve_) {
>                 const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);
> -               awb.gains.manual.r() = gains[0];
> -               awb.gains.manual.b() = gains[1];
> +               awb.manual.gains.r() = gains[0];
> +               awb.manual.gains.b() = gains[1];
>                 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;
>  }
>  
> @@ -155,7 +155,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;
>         }
>  
> @@ -332,14 +332,14 @@ void Awb::process(IPAContext &context,
>  
>         /* Filter the values to avoid oscillations. */
>         double speed = 0.2;
> -       gains = gains * speed + activeState.awb.gains.automatic * (1 - speed);
> +       gains = gains * speed + activeState.awb.automatic.gains * (1 - speed);
>  
> -       activeState.awb.gains.automatic = gains;
> +       activeState.awb.automatic.gains = 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 c765b928a55f..1a374c96cd1a 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -89,9 +89,12 @@ struct IPAActiveState {
>  
>         struct {
>                 struct {
> -                       RGB<double> manual;
> -                       RGB<double> automatic;
> -               } gains;
> +                       RGB<double> gains;
> +               } manual;
> +
> +               struct {
> +                       RGB<double> gains;
> +               } automatic;

Will we always add everything to both manual and automatic? I.e. should
should this be a defined as a structure struct AgcState or something
with
	struct AgcState {
		RGB<double> gains;
	};

	AgcState manual;
	AgcState automatic;

?

>  
>                 unsigned int temperatureK;
>                 bool autoEnabled;
> -- 
> 2.43.0
>
Kieran Bingham Feb. 17, 2025, 12:23 p.m. UTC | #2 
Quoting Kieran Bingham (2025-02-17 12:06:05)
> Quoting Stefan Klug (2025-02-17 10:01:45)
> > Swap gains and automatic/manual in the IPAActiveState structure. This is
> > in preparation to adding another member, which is easier in this
> > structure.  This is also in sync with how it is modeled in agc. This
> > patch contains no functional changes.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 24 ++++++++++++------------

<snip>

> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index c765b928a55f..1a374c96cd1a 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -89,9 +89,12 @@ struct IPAActiveState {
> >  
> >         struct {
> >                 struct {
> > -                       RGB<double> manual;
> > -                       RGB<double> automatic;
> > -               } gains;
> > +                       RGB<double> gains;
> > +               } manual;
> > +
> > +               struct {
> > +                       RGB<double> gains;
> > +               } automatic;
> 
> Will we always add everything to both manual and automatic? I.e. should
> should this be a defined as a structure struct AgcState or something
> with
>         struct AgcState {
>                 RGB<double> gains;
>         };
> 
>         AgcState manual;
>         AgcState automatic;
> 
> ?

Perhaps even more so if we should then just put an AgcState agc into the
FrameContext for directly assigning the 
	
	FrameContext.agc = ManualMode ?
			   activeState.agc.manual : activeState.agc.automatic;

> 
> >  
> >                 unsigned int temperatureK;
> >                 bool autoEnabled;
> > -- 
> > 2.43.0
> >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
index cffaa06a22c1..147277c98bb2 100644
--- a/src/ipa/rkisp1/algorithms/awb.cpp
+++ b/src/ipa/rkisp1/algorithms/awb.cpp
@@ -74,8 +74,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 = RGB<double>{ 1.0 };
+	context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
+	context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
 	context.activeState.awb.autoEnabled = true;
 	context.activeState.awb.temperatureK = kDefaultColourTemperature;
 
@@ -120,8 +120,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.
@@ -130,17 +130,17 @@  void Awb::queueRequest(IPAContext &context,
 		update = true;
 	} else if (colourTemperature && colourGainCurve_) {
 		const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);
-		awb.gains.manual.r() = gains[0];
-		awb.gains.manual.b() = gains[1];
+		awb.manual.gains.r() = gains[0];
+		awb.manual.gains.b() = gains[1];
 		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;
 }
 
@@ -155,7 +155,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;
 	}
 
@@ -332,14 +332,14 @@  void Awb::process(IPAContext &context,
 
 	/* Filter the values to avoid oscillations. */
 	double speed = 0.2;
-	gains = gains * speed + activeState.awb.gains.automatic * (1 - speed);
+	gains = gains * speed + activeState.awb.automatic.gains * (1 - speed);
 
-	activeState.awb.gains.automatic = gains;
+	activeState.awb.automatic.gains = 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 c765b928a55f..1a374c96cd1a 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -89,9 +89,12 @@  struct IPAActiveState {
 
 	struct {
 		struct {
-			RGB<double> manual;
-			RGB<double> automatic;
-		} gains;
+			RGB<double> gains;
+		} manual;
+
+		struct {
+			RGB<double> gains;
+		} automatic;
 
 		unsigned int temperatureK;
 		bool autoEnabled;