[RFC,2/4] ipa: rkisp1: agc: Do not derive from AgcMeanLuminance

24152 diff mbox series

There is no need to derive from AgcMeanLuminance anymore. Use
composition for easier understanding.

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

Quoting Stefan Klug (2025-08-18 09:28:40)
> There is no need to derive from AgcMeanLuminance anymore. Use
> composition for easier understanding.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 24 ++++++++++++------------
>  src/ipa/rkisp1/algorithms/agc.h   |  3 ++-
>  2 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 3a078f9753f6..f7dc025ee050 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -138,7 +138,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>  {
>         int ret;
>  
> -       ret = parseTuningData(tuningData);
> +       ret = agc_.parseTuningData(tuningData);
>         if (ret)
>                 return ret;
>  
> @@ -158,7 +158,7 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>         /* \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());
> +       context.ctrlMap.merge(agc_.controls());

Having worked through similar things - this is absolutely where it's
beneficial.

I was confused last week - not realising controls() was actually coming
from AgcMeanLuiminance.


In my experimental branch - I think this entire function can be a single
call itno AgcMeanLuminance.

	agc_->init(....)

Then the AgcMeanLuminance can parse it's tuning data and register the
controls it provides, letting the platform specific IPA handle anything
extra if it needs. Which I don't think is much if ever.

I think
https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18/files#diff-eb66f1d79de37124aabcc706393ce9516891401a
shows you my current diff (all very much still WIP).

I also think handling FrameDurationLimits needs to move *out* of AGC and
into a sensor specific handler.


>  
>         return 0;
>  }
> @@ -183,9 +183,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>         context.activeState.agc.exposureValue = 0.0;
>  
>         context.activeState.agc.constraintMode =
> -               static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> +               static_cast<controls::AeConstraintModeEnum>(agc_.constraintModes().begin()->first);

So this is just validating my theory that all of this handling should
move inside the AgcMeanLuminance class itself !


>         context.activeState.agc.exposureMode =
> -               static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first);
> +               static_cast<controls::AeExposureModeEnum>(agc_.exposureModeHelpers().begin()->first);
>         context.activeState.agc.meteringMode =
>                 static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first);
>  
> @@ -199,12 +199,12 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>         context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
>         context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
>  
> -       setLimits(context.configuration.sensor.minExposureTime,
> +       agc_.setLimits(context.configuration.sensor.minExposureTime,
>                   context.configuration.sensor.maxExposureTime,
>                   context.configuration.sensor.minAnalogueGain,
>                   context.configuration.sensor.maxAnalogueGain);
>  
> -       resetFrameCount();
> +       agc_.resetFrameCount();
>  
>         return 0;
>  }
> @@ -553,7 +553,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>                 maxAnalogueGain = frameContext.agc.gain;
>         }
>  
> -       setLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);
> +       agc_.setLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);
>  
>         /*
>          * The Agc algorithm needs to know the effective exposure value that was
> @@ -563,7 +563,7 @@ 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));
> +       agc_.setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
>  
>         AgcMeanLuminance::EstimateLuminanceFn estimateLuminanceFn = std::bind(
>                 &Agc::estimateLuminance, this,
> @@ -574,10 +574,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>         utils::Duration newExposureTime;
>         double aGain, dGain;
>         std::tie(newExposureTime, aGain, dGain) =
> -               calculateNewEv(estimateLuminanceFn,
> -                              frameContext.agc.constraintMode,
> -                              frameContext.agc.exposureMode,
> -                              hist, effectiveExposureValue);
> +               agc_.calculateNewEv(estimateLuminanceFn,
> +                                   frameContext.agc.constraintMode,
> +                                   frameContext.agc.exposureMode,
> +                                   hist, effectiveExposureValue);
>  
>         LOG(RkISP1Agc, Debug)
>                 << "Divided up exposure time, analogue gain and digital gain are "
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index 742c8f7371f3..02ba04bd2641 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -22,7 +22,7 @@ namespace libcamera {
>  
>  namespace ipa::rkisp1::algorithms {
>  
> -class Agc : public Algorithm, public AgcMeanLuminance
> +class Agc : public Algorithm
>  {
>  public:
>         Agc();
> @@ -55,6 +55,7 @@ private:
>                                   IPAFrameContext &frameContext,
>                                   utils::Duration frameDuration);
>  
> +       AgcMeanLuminance agc_;

Yes ;-) ok - so that's what I thought I'd see before the previous patch
- but I understand the ordering.


There is /so/ much overlap with what I've been trying to work on in my
spare time lately here - that I think I should offload my spare time
tasks to you ;-)



>  
>         std::map<int32_t, std::vector<uint8_t>> meteringModes_;
>  };
> -- 
> 2.48.1
>