[PATCH 9/9] ipa: rkisp1: agc: Implement ExposureValue control

[Kieran Bingham]

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 at 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