[9/9] ipa: rkisp1: agc: Implement ExposureValue control
diff mbox series

Message ID 20250331144352.736700-10-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Wdr preparations
Related show

Commit Message

Stefan Klug March 31, 2025, 2:43 p.m. UTC 
Now that agc_mean_luminance supports exposure correction, implement the
corresponding ExposureValue control for rkisp1.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
 src/ipa/rkisp1/ipa_context.h      |  2 ++
 2 files changed, 12 insertions(+)

Comments

Kieran Bingham March 31, 2025, 4:50 p.m. UTC | #1 
Quoting Stefan Klug (2025-03-31 15:43:48)
> Now that agc_mean_luminance supports exposure correction, implement the
> corresponding ExposureValue control for rkisp1.

Oh, that's an exciting development ...

> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
>  src/ipa/rkisp1/ipa_context.h      |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index b3ac9400b74f..8e77455e7afd 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -158,6 +158,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>                             ControlValue(controls::AnalogueGainModeAuto));
>         /* \todo Move this to the Camera class */
>         context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
> +       context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
>         context.ctrlMap.merge(controls());
>  
>         return 0;
> @@ -180,6 +181,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>         context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
>         context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
>         context.activeState.agc.autoGainEnabled = !context.configuration.raw;
> +       context.activeState.agc.exposureValue = 0.0;
>  
>         context.activeState.agc.constraintMode =
>                 static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> @@ -302,6 +304,11 @@ void Agc::queueRequest(IPAContext &context,
>                         static_cast<controls::AeConstraintModeEnum>(*constraintMode);
>         frameContext.agc.constraintMode = agc.constraintMode;
>  
> +       const auto &exposureValue = controls.get(controls::ExposureValue);
> +       if (exposureValue)
> +               agc.exposureValue = *exposureValue;
> +       frameContext.agc.exposureValue = agc.exposureValue;
> +
>         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>         if (frameDurationLimits) {
>                 /* Limit the control value to the limits in ControlInfo */
> @@ -408,6 +415,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>         metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
>         metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
>         metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> +       metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
>  }
>  
>  /**
> @@ -557,6 +565,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>         double analogueGain = frameContext.sensor.gain;
>         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>  
> +       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> +

while the patch overall sounds and looks pretty good - I'm not sure I've
seen setExposureCompensation() added to the helpers yet - so perhaps
this patch is supposed to be on a different series?

--
Kieran


>         utils::Duration newExposureTime;
>         double aGain, dGain;
>         std::tie(newExposureTime, aGain, dGain) =
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index f0d504215d34..7ccc7b501aff 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -81,6 +81,7 @@ struct IPAActiveState {
>  
>                 bool autoExposureEnabled;
>                 bool autoGainEnabled;
> +               double exposureValue;
>                 controls::AeConstraintModeEnum constraintMode;
>                 controls::AeExposureModeEnum exposureMode;
>                 controls::AeMeteringModeEnum meteringMode;
> @@ -129,6 +130,7 @@ struct IPAFrameContext : public FrameContext {
>         struct {
>                 uint32_t exposure;
>                 double gain;
> +               double exposureValue;
>                 uint32_t vblank;
>                 bool autoExposureEnabled;
>                 bool autoGainEnabled;
> -- 
> 2.43.0
>
Stefan Klug April 11, 2025, 10:42 a.m. UTC | #2 
Hi Kieran,

Thank you for the review. 

On Mon, Mar 31, 2025 at 05:50:54PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-03-31 15:43:48)
> > Now that agc_mean_luminance supports exposure correction, implement the
> > corresponding ExposureValue control for rkisp1.
> 
> Oh, that's an exciting development ...
> 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
> >  src/ipa/rkisp1/ipa_context.h      |  2 ++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index b3ac9400b74f..8e77455e7afd 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -158,6 +158,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> >                             ControlValue(controls::AnalogueGainModeAuto));
> >         /* \todo Move this to the Camera class */
> >         context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
> > +       context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> >         context.ctrlMap.merge(controls());
> >  
> >         return 0;
> > @@ -180,6 +181,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >         context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> >         context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
> >         context.activeState.agc.autoGainEnabled = !context.configuration.raw;
> > +       context.activeState.agc.exposureValue = 0.0;
> >  
> >         context.activeState.agc.constraintMode =
> >                 static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> > @@ -302,6 +304,11 @@ void Agc::queueRequest(IPAContext &context,
> >                         static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> >         frameContext.agc.constraintMode = agc.constraintMode;
> >  
> > +       const auto &exposureValue = controls.get(controls::ExposureValue);
> > +       if (exposureValue)
> > +               agc.exposureValue = *exposureValue;
> > +       frameContext.agc.exposureValue = agc.exposureValue;
> > +
> >         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> >         if (frameDurationLimits) {
> >                 /* Limit the control value to the limits in ControlInfo */
> > @@ -408,6 +415,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> >         metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> >         metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> >         metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> > +       metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
> >  }
> >  
> >  /**
> > @@ -557,6 +565,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >         double analogueGain = frameContext.sensor.gain;
> >         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> >  
> > +       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> > +
> 
> while the patch overall sounds and looks pretty good - I'm not sure I've
> seen setExposureCompensation() added to the helpers yet - so perhaps
> this patch is supposed to be on a different series?

setExposureCompensation() was added in the patch before. It works
because the algorithm is derived from AgcMeanLuminance.

Best regards,
Stefan

> 
> --
> Kieran
> 
> 
> >         utils::Duration newExposureTime;
> >         double aGain, dGain;
> >         std::tie(newExposureTime, aGain, dGain) =
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index f0d504215d34..7ccc7b501aff 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -81,6 +81,7 @@ struct IPAActiveState {
> >  
> >                 bool autoExposureEnabled;
> >                 bool autoGainEnabled;
> > +               double exposureValue;
> >                 controls::AeConstraintModeEnum constraintMode;
> >                 controls::AeExposureModeEnum exposureMode;
> >                 controls::AeMeteringModeEnum meteringMode;
> > @@ -129,6 +130,7 @@ struct IPAFrameContext : public FrameContext {
> >         struct {
> >                 uint32_t exposure;
> >                 double gain;
> > +               double exposureValue;
> >                 uint32_t vblank;
> >                 bool autoExposureEnabled;
> >                 bool autoGainEnabled;
> > -- 
> > 2.43.0
> >
Kieran Bingham April 12, 2025, 12:47 p.m. UTC | #3 
Quoting Stefan Klug (2025-04-11 11:42:33)
> Hi Kieran,
> 
> Thank you for the review. 
> 
> On Mon, Mar 31, 2025 at 05:50:54PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-03-31 15:43:48)
> > > Now that agc_mean_luminance supports exposure correction, implement the
> > > corresponding ExposureValue control for rkisp1.
> > 
> > Oh, that's an exciting development ...
> > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
> > >  src/ipa/rkisp1/ipa_context.h      |  2 ++
> > >  2 files changed, 12 insertions(+)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index b3ac9400b74f..8e77455e7afd 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -158,6 +158,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> > >                             ControlValue(controls::AnalogueGainModeAuto));
> > >         /* \todo Move this to the Camera class */
> > >         context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
> > > +       context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> > >         context.ctrlMap.merge(controls());
> > >  
> > >         return 0;
> > > @@ -180,6 +181,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >         context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > >         context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
> > >         context.activeState.agc.autoGainEnabled = !context.configuration.raw;
> > > +       context.activeState.agc.exposureValue = 0.0;
> > >  
> > >         context.activeState.agc.constraintMode =
> > >                 static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> > > @@ -302,6 +304,11 @@ void Agc::queueRequest(IPAContext &context,
> > >                         static_cast<controls::AeConstraintModeEnum>(*constraintMode);
> > >         frameContext.agc.constraintMode = agc.constraintMode;
> > >  
> > > +       const auto &exposureValue = controls.get(controls::ExposureValue);
> > > +       if (exposureValue)
> > > +               agc.exposureValue = *exposureValue;
> > > +       frameContext.agc.exposureValue = agc.exposureValue;
> > > +
> > >         const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > >         if (frameDurationLimits) {
> > >                 /* Limit the control value to the limits in ControlInfo */
> > > @@ -408,6 +415,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > >         metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> > >         metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> > >         metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> > > +       metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
> > >  }
> > >  
> > >  /**
> > > @@ -557,6 +565,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >         double analogueGain = frameContext.sensor.gain;
> > >         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > >  
> > > +       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> > > +
> > 
> > while the patch overall sounds and looks pretty good - I'm not sure I've
> > seen setExposureCompensation() added to the helpers yet - so perhaps
> > this patch is supposed to be on a different series?
> 
> setExposureCompensation() was added in the patch before. It works
> because the algorithm is derived from AgcMeanLuminance.

Aha, it looks like I completely missed that :D Sorry - but indeed.

I'll take a look at the new version I see for any tagging opportunities
;-)

--
Kieran

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index b3ac9400b74f..8e77455e7afd 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -158,6 +158,7 @@  int Agc::init(IPAContext &context, const YamlObject &tuningData)
 			    ControlValue(controls::AnalogueGainModeAuto));
 	/* \todo Move this to the Camera class */
 	context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
+	context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
 	context.ctrlMap.merge(controls());
 
 	return 0;
@@ -180,6 +181,7 @@  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
 	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
 	context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
 	context.activeState.agc.autoGainEnabled = !context.configuration.raw;
+	context.activeState.agc.exposureValue = 0.0;
 
 	context.activeState.agc.constraintMode =
 		static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
@@ -302,6 +304,11 @@  void Agc::queueRequest(IPAContext &context,
 			static_cast<controls::AeConstraintModeEnum>(*constraintMode);
 	frameContext.agc.constraintMode = agc.constraintMode;
 
+	const auto &exposureValue = controls.get(controls::ExposureValue);
+	if (exposureValue)
+		agc.exposureValue = *exposureValue;
+	frameContext.agc.exposureValue = agc.exposureValue;
+
 	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
 	if (frameDurationLimits) {
 		/* Limit the control value to the limits in ControlInfo */
@@ -408,6 +415,7 @@  void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
 	metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
 	metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
 	metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
+	metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
 }
 
 /**
@@ -557,6 +565,8 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	double analogueGain = frameContext.sensor.gain;
 	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
 
+	setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
+
 	utils::Duration newExposureTime;
 	double aGain, dGain;
 	std::tie(newExposureTime, aGain, dGain) =
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index f0d504215d34..7ccc7b501aff 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -81,6 +81,7 @@  struct IPAActiveState {
 
 		bool autoExposureEnabled;
 		bool autoGainEnabled;
+		double exposureValue;
 		controls::AeConstraintModeEnum constraintMode;
 		controls::AeExposureModeEnum exposureMode;
 		controls::AeMeteringModeEnum meteringMode;
@@ -129,6 +130,7 @@  struct IPAFrameContext : public FrameContext {
 	struct {
 		uint32_t exposure;
 		double gain;
+		double exposureValue;
 		uint32_t vblank;
 		bool autoExposureEnabled;
 		bool autoGainEnabled;