[RFC,v1,03/17] ipa: simple: agc: Do not overwrite sensor exposure/gain
diff mbox series

Message ID 20260703153819.1088752-4-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: libipa: agc rework
Related show

Commit Message

Barnabás Pőcze July 3, 2026, 3:38 p.m. UTC 
In the agc algorithm do not overwrite the sensor exposure/gain values,
instead add a new "agc" member to the frame context and update that.

No functional changes are intended as the agc algorithm runs last.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/simple/algorithms/agc.cpp | 11 +++++++----
 src/ipa/simple/ipa_context.h      |  5 +++++
 src/ipa/simple/soft_simple.cpp    |  9 +++++----
 3 files changed, 17 insertions(+), 8 deletions(-)

Comments

Kieran Bingham July 4, 2026, 4:48 a.m. UTC | #1 
Quoting Barnabás Pőcze (2026-07-03 16:38:05)
> In the agc algorithm do not overwrite the sensor exposure/gain values,
> instead add a new "agc" member to the frame context and update that.
> 
> No functional changes are intended as the agc algorithm runs last.
> 

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

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/simple/algorithms/agc.cpp | 11 +++++++----
>  src/ipa/simple/ipa_context.h      |  5 +++++
>  src/ipa/simple/soft_simple.cpp    |  9 +++++----
>  3 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> index a13a755280..e9bcb2c032 100644
> --- a/src/ipa/simple/algorithms/agc.cpp
> +++ b/src/ipa/simple/algorithms/agc.cpp
> @@ -67,8 +67,8 @@ Agc::Agc()
>  
>  void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV)
>  {
> -       int32_t &exposure = frameContext.sensor.exposure;
> -       double &again = frameContext.sensor.gain;
> +       int32_t exposure = frameContext.sensor.exposure;
> +       double again = frameContext.sensor.gain;
>  
>         double error = kExposureOptimal - exposureMSV;
>  
> @@ -115,6 +115,9 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>         again = std::clamp(again, context.configuration.agc.againMin,
>                            context.configuration.agc.againMax);
>  
> +       frameContext.agc.exposure = exposure;
> +       frameContext.agc.gain = again;
> +
>         context.activeState.agc.exposure = exposure;
>         context.activeState.agc.again = again;
>  
> @@ -150,8 +153,8 @@ void Agc::process(IPAContext &context,
>                  * Use the new exposure and gain values calculated the last time
>                  * there were valid stats.
>                  */
> -               frameContext.sensor.exposure = context.activeState.agc.exposure;
> -               frameContext.sensor.gain = context.activeState.agc.again;
> +               frameContext.agc.exposure = context.activeState.agc.exposure;
> +               frameContext.agc.gain = context.activeState.agc.again;
>                 return;
>         }
>  
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 8ccfacb46a..d35fb1e91d 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -66,6 +66,11 @@ struct IPAActiveState {
>  struct IPAFrameContext : public FrameContext {
>         Matrix<float, 3, 3> ccm;
>  
> +       struct {
> +               int32_t exposure;
> +               double gain;
> +       } agc;
> +
>         struct {
>                 int32_t exposure;
>                 double gain;
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 2a41b5bac8..22702638bb 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -320,10 +320,11 @@ void IPASoftSimple::processStats(const uint32_t frame,
>  
>         ControlList ctrls(sensorInfoMap_);
>  
> -       auto &againNew = frameContext.sensor.gain;
> -       ctrls.set(V4L2_CID_EXPOSURE, frameContext.sensor.exposure);
> -       ctrls.set(V4L2_CID_ANALOGUE_GAIN,
> -                 static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(againNew) : againNew));
> +       int32_t againNew = camHelper_
> +               ? camHelper_->gainCode(frameContext.agc.gain)
> +               : static_cast<int32_t>(frameContext.agc.gain);
> +       ctrls.set(V4L2_CID_EXPOSURE, frameContext.agc.exposure);
> +       ctrls.set(V4L2_CID_ANALOGUE_GAIN, againNew);
>  
>         setSensorControls.emit(ctrls);
>  }
> -- 
> 2.54.0
>
Jacopo Mondi July 15, 2026, 3:24 p.m. UTC | #2 
Hi Barnabás

On Fri, Jul 03, 2026 at 05:38:05PM +0200, Barnabás Pőcze wrote:
> In the agc algorithm do not overwrite the sensor exposure/gain values,
> instead add a new "agc" member to the frame context and update that.
>
> No functional changes are intended as the agc algorithm runs last.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

It seems correct to me

frameContext.sensor is updated by the IPA module before calling
process()

frameContext.agc and activeState.agc are updated here

the IPA module uses frameContext.agc to send controls

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  src/ipa/simple/algorithms/agc.cpp | 11 +++++++----
>  src/ipa/simple/ipa_context.h      |  5 +++++
>  src/ipa/simple/soft_simple.cpp    |  9 +++++----
>  3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> index a13a755280..e9bcb2c032 100644
> --- a/src/ipa/simple/algorithms/agc.cpp
> +++ b/src/ipa/simple/algorithms/agc.cpp
> @@ -67,8 +67,8 @@ Agc::Agc()
>
>  void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV)
>  {
> -	int32_t &exposure = frameContext.sensor.exposure;
> -	double &again = frameContext.sensor.gain;
> +	int32_t exposure = frameContext.sensor.exposure;
> +	double again = frameContext.sensor.gain;
>
>  	double error = kExposureOptimal - exposureMSV;
>
> @@ -115,6 +115,9 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>  	again = std::clamp(again, context.configuration.agc.againMin,
>  			   context.configuration.agc.againMax);
>
> +	frameContext.agc.exposure = exposure;
> +	frameContext.agc.gain = again;
> +
>  	context.activeState.agc.exposure = exposure;
>  	context.activeState.agc.again = again;
>
> @@ -150,8 +153,8 @@ void Agc::process(IPAContext &context,
>  		 * Use the new exposure and gain values calculated the last time
>  		 * there were valid stats.
>  		 */
> -		frameContext.sensor.exposure = context.activeState.agc.exposure;
> -		frameContext.sensor.gain = context.activeState.agc.again;
> +		frameContext.agc.exposure = context.activeState.agc.exposure;
> +		frameContext.agc.gain = context.activeState.agc.again;
>  		return;
>  	}
>
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 8ccfacb46a..d35fb1e91d 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -66,6 +66,11 @@ struct IPAActiveState {
>  struct IPAFrameContext : public FrameContext {
>  	Matrix<float, 3, 3> ccm;
>
> +	struct {
> +		int32_t exposure;
> +		double gain;
> +	} agc;
> +
>  	struct {
>  		int32_t exposure;
>  		double gain;
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 2a41b5bac8..22702638bb 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -320,10 +320,11 @@ void IPASoftSimple::processStats(const uint32_t frame,
>
>  	ControlList ctrls(sensorInfoMap_);
>
> -	auto &againNew = frameContext.sensor.gain;
> -	ctrls.set(V4L2_CID_EXPOSURE, frameContext.sensor.exposure);
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN,
> -		  static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(againNew) : againNew));
> +	int32_t againNew = camHelper_
> +		? camHelper_->gainCode(frameContext.agc.gain)
> +		: static_cast<int32_t>(frameContext.agc.gain);
> +	ctrls.set(V4L2_CID_EXPOSURE, frameContext.agc.exposure);
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, againNew);
>
>  	setSensorControls.emit(ctrls);
>  }
> --
> 2.54.0
>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
index a13a755280..e9bcb2c032 100644
--- a/src/ipa/simple/algorithms/agc.cpp
+++ b/src/ipa/simple/algorithms/agc.cpp
@@ -67,8 +67,8 @@  Agc::Agc()
 
 void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV)
 {
-	int32_t &exposure = frameContext.sensor.exposure;
-	double &again = frameContext.sensor.gain;
+	int32_t exposure = frameContext.sensor.exposure;
+	double again = frameContext.sensor.gain;
 
 	double error = kExposureOptimal - exposureMSV;
 
@@ -115,6 +115,9 @@  void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
 	again = std::clamp(again, context.configuration.agc.againMin,
 			   context.configuration.agc.againMax);
 
+	frameContext.agc.exposure = exposure;
+	frameContext.agc.gain = again;
+
 	context.activeState.agc.exposure = exposure;
 	context.activeState.agc.again = again;
 
@@ -150,8 +153,8 @@  void Agc::process(IPAContext &context,
 		 * Use the new exposure and gain values calculated the last time
 		 * there were valid stats.
 		 */
-		frameContext.sensor.exposure = context.activeState.agc.exposure;
-		frameContext.sensor.gain = context.activeState.agc.again;
+		frameContext.agc.exposure = context.activeState.agc.exposure;
+		frameContext.agc.gain = context.activeState.agc.again;
 		return;
 	}
 
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index 8ccfacb46a..d35fb1e91d 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -66,6 +66,11 @@  struct IPAActiveState {
 struct IPAFrameContext : public FrameContext {
 	Matrix<float, 3, 3> ccm;
 
+	struct {
+		int32_t exposure;
+		double gain;
+	} agc;
+
 	struct {
 		int32_t exposure;
 		double gain;
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index 2a41b5bac8..22702638bb 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -320,10 +320,11 @@  void IPASoftSimple::processStats(const uint32_t frame,
 
 	ControlList ctrls(sensorInfoMap_);
 
-	auto &againNew = frameContext.sensor.gain;
-	ctrls.set(V4L2_CID_EXPOSURE, frameContext.sensor.exposure);
-	ctrls.set(V4L2_CID_ANALOGUE_GAIN,
-		  static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(againNew) : againNew));
+	int32_t againNew = camHelper_
+		? camHelper_->gainCode(frameContext.agc.gain)
+		: static_cast<int32_t>(frameContext.agc.gain);
+	ctrls.set(V4L2_CID_EXPOSURE, frameContext.agc.exposure);
+	ctrls.set(V4L2_CID_ANALOGUE_GAIN, againNew);
 
 	setSensorControls.emit(ctrls);
 }