[v2,7/8] ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc

19899 diff mbox series

Now that we have a MeanLuminanceAgc class that centralises our AEGC
algorithm, derive the RkISP1's Agc class from it and plumb in the
necessary framework to enable it to be used. For simplicities sake
this commit switches the algorithm to use the derived class, but
does not remove the bespoke functions at this time.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

	- Squashed the patch adding parseStatistics()
	- Used the first available constraint and exposure modes rather than
	  assuming the "Normal" mode was available
	- Stopped storing a Histogram in the class members when it's only needed
	  during ::process()
	- Remembered to report the controls out to IPARkISP1::updateControls()

 src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
 src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
 src/ipa/rkisp1/ipa_context.h      |  5 ++
 src/ipa/rkisp1/rkisp1.cpp         |  3 +-
 4 files changed, 92 insertions(+), 7 deletions(-)

Hi Daniel,

thanks for the patch.
Looks good to me now.

On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
> Now that we have a MeanLuminanceAgc class that centralises our AEGC
> algorithm, derive the RkISP1's Agc class from it and plumb in the
> necessary framework to enable it to be used. For simplicities sake
> this commit switches the algorithm to use the derived class, but
> does not remove the bespoke functions at this time.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

Cheers,
Stefan

> ---
> Changes in v2:
> 
> 	- Squashed the patch adding parseStatistics()
> 	- Used the first available constraint and exposure modes rather than
> 	  assuming the "Normal" mode was available
> 	- Stopped storing a Histogram in the class members when it's only needed
> 	  during ::process()
> 	- Remembered to report the controls out to IPARkISP1::updateControls()
> 
>  src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
>  src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
>  src/ipa/rkisp1/ipa_context.h      |  5 ++
>  src/ipa/rkisp1/rkisp1.cpp         |  3 +-
>  4 files changed, 92 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 47a6f7b2..0d66fcca 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -64,6 +64,30 @@ Agc::Agc()
>  	supportsRaw_ = true;
>  }
>  
> +/**
> + * \brief Initialise the AGC algorithm from tuning files
> + *
> + * \param[in] context The shared IPA context
> + * \param[in] tuningData The YamlObject containing Agc tuning data
> + *
> + * This function calls the base class' tuningData parsers to discover which
> + * control values are supported.
> + *
> + * \return 0 on success or errors from the base class
> + */
> +int Agc::init(IPAContext &context, const YamlObject &tuningData)
> +{
> +	int ret;
> +
> +	ret = parseTuningData(tuningData);
> +	if (ret)
> +		return ret;
> +
> +	context.ctrlMap.merge(controls());
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Configure the AGC given a configInfo
>   * \param[in] context The shared IPA context
> @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
>  	context.activeState.agc.autoEnabled = !context.configuration.raw;
>  
> +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> +
>  	/*
>  	 * Define the measurement window for AGC as a centered rectangle
>  	 * covering 3/4 of the image width and height.
> @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	 * frame index
>  	 */
>  	frameCount_ = 0;
> +
> +	/* \todo Run this again when FrameDurationLimits is passed in */
> +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> +				     context.configuration.sensor.maxShutterSpeed,
> +				     context.configuration.sensor.minAnalogueGain,
> +				     context.configuration.sensor.maxAnalogueGain);
> +
> +	resetFrameCount();
> +
>  	return 0;
>  }
>  
> @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  			  double yGain, double iqMeanGain)
>  {
>  	IPASessionConfiguration &configuration = context.configuration;
> -	IPAActiveState &activeState = context.activeState;
>  
>  	/* Get the effective exposure and gain applied on the sensor. */
>  	uint32_t exposure = frameContext.sensor.exposure;
> @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>  			      << shutterTime << " and "
>  			      << stepGain;
> -
> -	/* Update the estimated exposure and gain. */
> -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> -	activeState.agc.automatic.gain = stepGain;
>  }
>  
>  /**
> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>  }
>  
> +double Agc::estimateLuminance(double gain)
> +{
> +	double ySum = 0.0;
> +
> +	/* Sum the averages, saturated to 255. */
> +	for (uint8_t expMean : expMeans_)
> +		ySum += std::min(expMean * gain, 255.0);
> +
> +	/* \todo Weight with the AWB gains */
> +
> +	return ySum / expMeans_.size() / 255;
> +}
> +
>  /**
>   * \brief Process RkISP1 statistics, and run AGC operations
>   * \param[in] context The shared IPA context
> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	computeExposure(context, frameContext, yGain, iqMeanGain);
>  	frameCount_++;
>  
> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> +
> +	/*
> +	 * The Agc algorithm needs to know the effective exposure value that was
> +	 * applied to the sensor when the statistics were collected.
> +	 */
> +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> +				       * frameContext.sensor.exposure;
> +	double analogueGain = frameContext.sensor.gain;
> +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> +
> +	utils::Duration shutterTime;
> +	double aGain, dGain;
> +	std::tie(shutterTime, aGain, dGain) =
> +		calculateNewEv(context.activeState.agc.constraintMode,
> +			       context.activeState.agc.exposureMode,
> +			       Histogram(hist), effectiveExposureValue);
> +
> +	LOG(RkISP1Agc, Debug)
> +		<< "Divided up shutter, analogue gain and digital gain are "
> +		<< shutterTime << ", " << aGain << " and " << dGain;
> +
> +	IPAActiveState &activeState = context.activeState;
> +	/* Update the estimated exposure and gain. */
> +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> +	activeState.agc.automatic.gain = aGain;
> +
>  	fillMetadata(context, frameContext, metadata);
> +	expMeans_ = {};
>  }
>  
>  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index fb82a33f..a8080228 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -14,18 +14,22 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libipa/agc_mean_luminance.h"
> +#include "libipa/histogram.h"
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
>  
>  namespace ipa::rkisp1::algorithms {
>  
> -class Agc : public Algorithm
> +class Agc : public Algorithm, public AgcMeanLuminance
>  {
>  public:
>  	Agc();
>  	~Agc() = default;
>  
> +	int init(IPAContext &context, const YamlObject &tuningData) override;
>  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>  	void queueRequest(IPAContext &context,
>  			  const uint32_t frame,
> @@ -47,10 +51,13 @@ private:
>  	double measureBrightness(Span<const uint32_t> hist) const;
>  	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  			  ControlList &metadata);
> +	double estimateLuminance(double gain) override;
>  
>  	uint64_t frameCount_;
>  
>  	utils::Duration filteredExposure_;
> +
> +	Span<const uint8_t> expMeans_;
>  };
>  
>  } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 10d8f38c..256b75eb 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -12,6 +12,7 @@
>  
>  #include <libcamera/base/utils.h>
>  
> +#include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
>  
>  #include <libipa/fc_queue.h>
> @@ -67,6 +68,8 @@ struct IPAActiveState {
>  		} automatic;
>  
>  		bool autoEnabled;
> +		uint32_t constraintMode;
> +		uint32_t exposureMode;
>  	} agc;
>  
>  	struct {
> @@ -151,6 +154,8 @@ struct IPAContext {
>  	IPAActiveState activeState;
>  
>  	FCQueue<IPAFrameContext> frameContexts;
> +
> +	ControlInfoMap::Map ctrlMap;
>  };
>  
>  } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 9dc5f53c..d8610095 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
>  } /* namespace */
>  
>  IPARkISP1::IPARkISP1()
> -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
> +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
>  {
>  }
>  
> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>  							      frameDurations[1],
>  							      frameDurations[2]);
>  
> +	ctrlMap.merge(context_.ctrlMap);
>  	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
>  }
>  
> -- 
> 2.34.1
>

Hi Dan,

On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
> Now that we have a MeanLuminanceAgc class that centralises our AEGC
> algorithm, derive the RkISP1's Agc class from it and plumb in the
> necessary framework to enable it to be used. For simplicities sake
> this commit switches the algorithm to use the derived class, but
> does not remove the bespoke functions at this time.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> Changes in v2:
> 
> 	- Squashed the patch adding parseStatistics()
> 	- Used the first available constraint and exposure modes rather than
> 	  assuming the "Normal" mode was available
> 	- Stopped storing a Histogram in the class members when it's only needed
> 	  during ::process()
> 	- Remembered to report the controls out to IPARkISP1::updateControls()
> 
>  src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
>  src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
>  src/ipa/rkisp1/ipa_context.h      |  5 ++
>  src/ipa/rkisp1/rkisp1.cpp         |  3 +-
>  4 files changed, 92 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 47a6f7b2..0d66fcca 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -64,6 +64,30 @@ Agc::Agc()
>  	supportsRaw_ = true;
>  }
>  
> +/**
> + * \brief Initialise the AGC algorithm from tuning files
> + *
> + * \param[in] context The shared IPA context
> + * \param[in] tuningData The YamlObject containing Agc tuning data
> + *
> + * This function calls the base class' tuningData parsers to discover which
> + * control values are supported.
> + *
> + * \return 0 on success or errors from the base class
> + */
> +int Agc::init(IPAContext &context, const YamlObject &tuningData)
> +{
> +	int ret;
> +
> +	ret = parseTuningData(tuningData);
> +	if (ret)
> +		return ret;
> +
> +	context.ctrlMap.merge(controls());
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Configure the AGC given a configInfo
>   * \param[in] context The shared IPA context
> @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
>  	context.activeState.agc.autoEnabled = !context.configuration.raw;
>  
> +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> +
>  	/*
>  	 * Define the measurement window for AGC as a centered rectangle
>  	 * covering 3/4 of the image width and height.
> @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	 * frame index
>  	 */
>  	frameCount_ = 0;
> +
> +	/* \todo Run this again when FrameDurationLimits is passed in */
> +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> +				     context.configuration.sensor.maxShutterSpeed,
> +				     context.configuration.sensor.minAnalogueGain,
> +				     context.configuration.sensor.maxAnalogueGain);
> +
> +	resetFrameCount();
> +
>  	return 0;
>  }
>  
> @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  			  double yGain, double iqMeanGain)
>  {
>  	IPASessionConfiguration &configuration = context.configuration;
> -	IPAActiveState &activeState = context.activeState;
>  
>  	/* Get the effective exposure and gain applied on the sensor. */
>  	uint32_t exposure = frameContext.sensor.exposure;
> @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>  			      << shutterTime << " and "
>  			      << stepGain;
> -
> -	/* Update the estimated exposure and gain. */
> -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> -	activeState.agc.automatic.gain = stepGain;
>  }
>  
>  /**
> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>  }
>  
> +double Agc::estimateLuminance(double gain)
> +{
> +	double ySum = 0.0;
> +
> +	/* Sum the averages, saturated to 255. */
> +	for (uint8_t expMean : expMeans_)
> +		ySum += std::min(expMean * gain, 255.0);
> +
> +	/* \todo Weight with the AWB gains */
> +
> +	return ySum / expMeans_.size() / 255;
> +}
> +
>  /**
>   * \brief Process RkISP1 statistics, and run AGC operations
>   * \param[in] context The shared IPA context
> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	computeExposure(context, frameContext, yGain, iqMeanGain);
>  	frameCount_++;
>  
> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> +
> +	/*
> +	 * The Agc algorithm needs to know the effective exposure value that was
> +	 * applied to the sensor when the statistics were collected.
> +	 */
> +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> +				       * frameContext.sensor.exposure;
> +	double analogueGain = frameContext.sensor.gain;
> +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> +
> +	utils::Duration shutterTime;
> +	double aGain, dGain;
> +	std::tie(shutterTime, aGain, dGain) =
> +		calculateNewEv(context.activeState.agc.constraintMode,
> +			       context.activeState.agc.exposureMode,
> +			       Histogram(hist), effectiveExposureValue);
> +
> +	LOG(RkISP1Agc, Debug)
> +		<< "Divided up shutter, analogue gain and digital gain are "
> +		<< shutterTime << ", " << aGain << " and " << dGain;
> +
> +	IPAActiveState &activeState = context.activeState;
> +	/* Update the estimated exposure and gain. */
> +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> +	activeState.agc.automatic.gain = aGain;
> +
>  	fillMetadata(context, frameContext, metadata);
> +	expMeans_ = {};
>  }
>  
>  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index fb82a33f..a8080228 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -14,18 +14,22 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libipa/agc_mean_luminance.h"
> +#include "libipa/histogram.h"
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
>  
>  namespace ipa::rkisp1::algorithms {
>  
> -class Agc : public Algorithm
> +class Agc : public Algorithm, public AgcMeanLuminance
>  {
>  public:
>  	Agc();
>  	~Agc() = default;
>  
> +	int init(IPAContext &context, const YamlObject &tuningData) override;
>  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>  	void queueRequest(IPAContext &context,
>  			  const uint32_t frame,
> @@ -47,10 +51,13 @@ private:
>  	double measureBrightness(Span<const uint32_t> hist) const;
>  	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  			  ControlList &metadata);
> +	double estimateLuminance(double gain) override;
>  
>  	uint64_t frameCount_;
>  
>  	utils::Duration filteredExposure_;
> +
> +	Span<const uint8_t> expMeans_;
>  };
>  
>  } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 10d8f38c..256b75eb 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -12,6 +12,7 @@
>  
>  #include <libcamera/base/utils.h>
>  
> +#include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
>  
>  #include <libipa/fc_queue.h>
> @@ -67,6 +68,8 @@ struct IPAActiveState {
>  		} automatic;
>  
>  		bool autoEnabled;
> +		uint32_t constraintMode;
> +		uint32_t exposureMode;
>  	} agc;
>  
>  	struct {
> @@ -151,6 +154,8 @@ struct IPAContext {
>  	IPAActiveState activeState;
>  
>  	FCQueue<IPAFrameContext> frameContexts;
> +
> +	ControlInfoMap::Map ctrlMap;
>  };
>  
>  } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 9dc5f53c..d8610095 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
>  } /* namespace */
>  
>  IPARkISP1::IPARkISP1()
> -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
> +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
>  {
>  }
>  
> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>  							      frameDurations[1],
>  							      frameDurations[2]);
>  
> +	ctrlMap.merge(context_.ctrlMap);
>  	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
>  }
>  
> -- 
> 2.34.1
>

Hi Dan

On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
> Now that we have a MeanLuminanceAgc class that centralises our AEGC
> algorithm, derive the RkISP1's Agc class from it and plumb in the
> necessary framework to enable it to be used. For simplicities sake
> this commit switches the algorithm to use the derived class, but
> does not remove the bespoke functions at this time.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
>
> 	- Squashed the patch adding parseStatistics()
> 	- Used the first available constraint and exposure modes rather than
> 	  assuming the "Normal" mode was available
> 	- Stopped storing a Histogram in the class members when it's only needed
> 	  during ::process()
> 	- Remembered to report the controls out to IPARkISP1::updateControls()
>
>  src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
>  src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
>  src/ipa/rkisp1/ipa_context.h      |  5 ++
>  src/ipa/rkisp1/rkisp1.cpp         |  3 +-
>  4 files changed, 92 insertions(+), 7 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 47a6f7b2..0d66fcca 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -64,6 +64,30 @@ Agc::Agc()
>  	supportsRaw_ = true;
>  }
>
> +/**
> + * \brief Initialise the AGC algorithm from tuning files
> + *

no empty line

> + * \param[in] context The shared IPA context
> + * \param[in] tuningData The YamlObject containing Agc tuning data
> + *
> + * This function calls the base class' tuningData parsers to discover which
> + * control values are supported.
> + *
> + * \return 0 on success or errors from the base class
> + */
> +int Agc::init(IPAContext &context, const YamlObject &tuningData)
> +{
> +	int ret;
> +
> +	ret = parseTuningData(tuningData);
> +	if (ret)
> +		return ret;
> +
> +	context.ctrlMap.merge(controls());
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Configure the AGC given a configInfo
>   * \param[in] context The shared IPA context
> @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
>  	context.activeState.agc.autoEnabled = !context.configuration.raw;
>
> +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> +
>  	/*
>  	 * Define the measurement window for AGC as a centered rectangle
>  	 * covering 3/4 of the image width and height.
> @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	 * frame index
>  	 */
>  	frameCount_ = 0;
> +
> +	/* \todo Run this again when FrameDurationLimits is passed in */
> +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> +				     context.configuration.sensor.maxShutterSpeed,
> +				     context.configuration.sensor.minAnalogueGain,
> +				     context.configuration.sensor.maxAnalogueGain);
> +

I think I also commented this on the base class, but this should just
be "setLimits" or something similar (iow do not expose the
"ExposureModeHelpers" name to the derived classes)

> +	resetFrameCount();
> +

As this HAS to be run at configure time, I would also consider a base
class 'configure()' which sets limits and resets the frame counter and
have derived classes call it

>  	return 0;
>  }
>
> @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  			  double yGain, double iqMeanGain)
>  {
>  	IPASessionConfiguration &configuration = context.configuration;
> -	IPAActiveState &activeState = context.activeState;
>
>  	/* Get the effective exposure and gain applied on the sensor. */
>  	uint32_t exposure = frameContext.sensor.exposure;
> @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>  			      << shutterTime << " and "
>  			      << stepGain;
> -
> -	/* Update the estimated exposure and gain. */
> -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> -	activeState.agc.automatic.gain = stepGain;
>  }
>
>  /**
> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>  }
>
> +double Agc::estimateLuminance(double gain)
> +{
> +	double ySum = 0.0;
> +
> +	/* Sum the averages, saturated to 255. */
> +	for (uint8_t expMean : expMeans_)
> +		ySum += std::min(expMean * gain, 255.0);
> +
> +	/* \todo Weight with the AWB gains */
> +
> +	return ySum / expMeans_.size() / 255;
> +}
> +
>  /**
>   * \brief Process RkISP1 statistics, and run AGC operations
>   * \param[in] context The shared IPA context
> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	computeExposure(context, frameContext, yGain, iqMeanGain);
>  	frameCount_++;
>
> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> +
> +	/*
> +	 * The Agc algorithm needs to know the effective exposure value that was
> +	 * applied to the sensor when the statistics were collected.
> +	 */
> +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> +				       * frameContext.sensor.exposure;
> +	double analogueGain = frameContext.sensor.gain;
> +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;

same question, is this a duration ?

> +
> +	utils::Duration shutterTime;
> +	double aGain, dGain;
> +	std::tie(shutterTime, aGain, dGain) =
> +		calculateNewEv(context.activeState.agc.constraintMode,
> +			       context.activeState.agc.exposureMode,
> +			       Histogram(hist), effectiveExposureValue);
> +
> +	LOG(RkISP1Agc, Debug)
> +		<< "Divided up shutter, analogue gain and digital gain are "
> +		<< shutterTime << ", " << aGain << " and " << dGain;
> +
> +	IPAActiveState &activeState = context.activeState;
> +	/* Update the estimated exposure and gain. */
> +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> +	activeState.agc.automatic.gain = aGain;
> +
>  	fillMetadata(context, frameContext, metadata);
> +	expMeans_ = {};

isn't this reset at the beginning of this function ?

Or am I confused ? I see two estimateLuminance() (this is expected)
one uses expMeans_ the other doesn't, but expMeans_ is assigned after
the call to estimateLuminance() ?

>  }
>
>  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index fb82a33f..a8080228 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -14,18 +14,22 @@
>
>  #include <libcamera/geometry.h>
>
> +#include "libipa/agc_mean_luminance.h"
> +#include "libipa/histogram.h"
> +
>  #include "algorithm.h"
>
>  namespace libcamera {
>
>  namespace ipa::rkisp1::algorithms {
>
> -class Agc : public Algorithm
> +class Agc : public Algorithm, public AgcMeanLuminance
>  {
>  public:
>  	Agc();
>  	~Agc() = default;
>
> +	int init(IPAContext &context, const YamlObject &tuningData) override;
>  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>  	void queueRequest(IPAContext &context,
>  			  const uint32_t frame,
> @@ -47,10 +51,13 @@ private:
>  	double measureBrightness(Span<const uint32_t> hist) const;
>  	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  			  ControlList &metadata);
> +	double estimateLuminance(double gain) override;
>
>  	uint64_t frameCount_;
>
>  	utils::Duration filteredExposure_;
> +
> +	Span<const uint8_t> expMeans_;
>  };
>
>  } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 10d8f38c..256b75eb 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -12,6 +12,7 @@
>
>  #include <libcamera/base/utils.h>
>
> +#include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
>
>  #include <libipa/fc_queue.h>
> @@ -67,6 +68,8 @@ struct IPAActiveState {
>  		} automatic;
>
>  		bool autoEnabled;
> +		uint32_t constraintMode;
> +		uint32_t exposureMode;
>  	} agc;
>
>  	struct {
> @@ -151,6 +154,8 @@ struct IPAContext {
>  	IPAActiveState activeState;
>
>  	FCQueue<IPAFrameContext> frameContexts;
> +
> +	ControlInfoMap::Map ctrlMap;
>  };
>
>  } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 9dc5f53c..d8610095 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
>  } /* namespace */
>
>  IPARkISP1::IPARkISP1()
> -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
> +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
>  {
>  }
>
> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>  							      frameDurations[1],
>  							      frameDurations[2]);
>
> +	ctrlMap.merge(context_.ctrlMap);
>  	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
>  }
>
> --
> 2.34.1
>

Hi again

On Mon, Apr 22, 2024 at 12:49:28PM +0200, Jacopo Mondi wrote:
> Hi Dan
>
> On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
> > Now that we have a MeanLuminanceAgc class that centralises our AEGC
> > algorithm, derive the RkISP1's Agc class from it and plumb in the
> > necessary framework to enable it to be used. For simplicities sake
> > this commit switches the algorithm to use the derived class, but
> > does not remove the bespoke functions at this time.
> >
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > Changes in v2:
> >
> > 	- Squashed the patch adding parseStatistics()
> > 	- Used the first available constraint and exposure modes rather than
> > 	  assuming the "Normal" mode was available
> > 	- Stopped storing a Histogram in the class members when it's only needed
> > 	  during ::process()
> > 	- Remembered to report the controls out to IPARkISP1::updateControls()
> >
> >  src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
> >  src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
> >  src/ipa/rkisp1/ipa_context.h      |  5 ++
> >  src/ipa/rkisp1/rkisp1.cpp         |  3 +-
> >  4 files changed, 92 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 47a6f7b2..0d66fcca 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -64,6 +64,30 @@ Agc::Agc()
> >  	supportsRaw_ = true;
> >  }
> >
> > +/**
> > + * \brief Initialise the AGC algorithm from tuning files
> > + *
>
> no empty line
>
> > + * \param[in] context The shared IPA context
> > + * \param[in] tuningData The YamlObject containing Agc tuning data
> > + *
> > + * This function calls the base class' tuningData parsers to discover which
> > + * control values are supported.
> > + *
> > + * \return 0 on success or errors from the base class
> > + */
> > +int Agc::init(IPAContext &context, const YamlObject &tuningData)
> > +{
> > +	int ret;
> > +
> > +	ret = parseTuningData(tuningData);
> > +	if (ret)
> > +		return ret;
> > +
> > +	context.ctrlMap.merge(controls());
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Configure the AGC given a configInfo
> >   * \param[in] context The shared IPA context
> > @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >  	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> >  	context.activeState.agc.autoEnabled = !context.configuration.raw;
> >
> > +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> > +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > +
> >  	/*
> >  	 * Define the measurement window for AGC as a centered rectangle
> >  	 * covering 3/4 of the image width and height.
> > @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >  	 * frame index
> >  	 */
> >  	frameCount_ = 0;
> > +
> > +	/* \todo Run this again when FrameDurationLimits is passed in */
> > +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> > +				     context.configuration.sensor.maxShutterSpeed,
> > +				     context.configuration.sensor.minAnalogueGain,
> > +				     context.configuration.sensor.maxAnalogueGain);
> > +
>
> I think I also commented this on the base class, but this should just
> be "setLimits" or something similar (iow do not expose the
> "ExposureModeHelpers" name to the derived classes)
>
> > +	resetFrameCount();
> > +
>
> As this HAS to be run at configure time, I would also consider a base
> class 'configure()' which sets limits and resets the frame counter and
> have derived classes call it
>
> >  	return 0;
> >  }
> >
> > @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> >  			  double yGain, double iqMeanGain)
> >  {
> >  	IPASessionConfiguration &configuration = context.configuration;
> > -	IPAActiveState &activeState = context.activeState;
> >
> >  	/* Get the effective exposure and gain applied on the sensor. */
> >  	uint32_t exposure = frameContext.sensor.exposure;
> > @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> >  	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
> >  			      << shutterTime << " and "
> >  			      << stepGain;
> > -
> > -	/* Update the estimated exposure and gain. */
> > -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> > -	activeState.agc.automatic.gain = stepGain;
> >  }
> >
> >  /**
> > @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> >  	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> >  }
> >
> > +double Agc::estimateLuminance(double gain)
> > +{
> > +	double ySum = 0.0;
> > +
> > +	/* Sum the averages, saturated to 255. */
> > +	for (uint8_t expMean : expMeans_)
> > +		ySum += std::min(expMean * gain, 255.0);
> > +
> > +	/* \todo Weight with the AWB gains */
> > +
> > +	return ySum / expMeans_.size() / 255;
> > +}
> > +
> >  /**
> >   * \brief Process RkISP1 statistics, and run AGC operations
> >   * \param[in] context The shared IPA context
> > @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  	computeExposure(context, frameContext, yGain, iqMeanGain);
> >  	frameCount_++;
> >
> > +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > +
> > +	/*
> > +	 * The Agc algorithm needs to know the effective exposure value that was
> > +	 * applied to the sensor when the statistics were collected.
> > +	 */
> > +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > +				       * frameContext.sensor.exposure;
> > +	double analogueGain = frameContext.sensor.gain;
> > +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>
> same question, is this a duration ?
>
> > +
> > +	utils::Duration shutterTime;
> > +	double aGain, dGain;
> > +	std::tie(shutterTime, aGain, dGain) =
> > +		calculateNewEv(context.activeState.agc.constraintMode,
> > +			       context.activeState.agc.exposureMode,
> > +			       Histogram(hist), effectiveExposureValue);
> > +
> > +	LOG(RkISP1Agc, Debug)
> > +		<< "Divided up shutter, analogue gain and digital gain are "
> > +		<< shutterTime << ", " << aGain << " and " << dGain;
> > +
> > +	IPAActiveState &activeState = context.activeState;
> > +	/* Update the estimated exposure and gain. */
> > +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> > +	activeState.agc.automatic.gain = aGain;
> > +
> >  	fillMetadata(context, frameContext, metadata);
> > +	expMeans_ = {};
>
> isn't this reset at the beginning of this function ?
>
> Or am I confused ? I see two estimateLuminance() (this is expected)
> one uses expMeans_ the other doesn't, but expMeans_ is assigned after
> the call to estimateLuminance() ?

Sorry, I was confused, this is used by the base class and it is called
by calculateNewEv(), which is called here after expMeans_ is
assigned...

I think the documentation of the pure virtual declaration in the base class
should maybe specify what is the call path and that derived classes
should prepare for the override to be called during calculateNewEv()

>
> >  }
> >
> >  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > index fb82a33f..a8080228 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -14,18 +14,22 @@
> >
> >  #include <libcamera/geometry.h>
> >
> > +#include "libipa/agc_mean_luminance.h"
> > +#include "libipa/histogram.h"
> > +
> >  #include "algorithm.h"
> >
> >  namespace libcamera {
> >
> >  namespace ipa::rkisp1::algorithms {
> >
> > -class Agc : public Algorithm
> > +class Agc : public Algorithm, public AgcMeanLuminance
> >  {
> >  public:
> >  	Agc();
> >  	~Agc() = default;
> >
> > +	int init(IPAContext &context, const YamlObject &tuningData) override;
> >  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> >  	void queueRequest(IPAContext &context,
> >  			  const uint32_t frame,
> > @@ -47,10 +51,13 @@ private:
> >  	double measureBrightness(Span<const uint32_t> hist) const;
> >  	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> >  			  ControlList &metadata);
> > +	double estimateLuminance(double gain) override;
> >
> >  	uint64_t frameCount_;
> >
> >  	utils::Duration filteredExposure_;
> > +
> > +	Span<const uint8_t> expMeans_;
> >  };
> >
> >  } /* namespace ipa::rkisp1::algorithms */
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 10d8f38c..256b75eb 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -12,6 +12,7 @@
> >
> >  #include <libcamera/base/utils.h>
> >
> > +#include <libcamera/controls.h>
> >  #include <libcamera/geometry.h>
> >
> >  #include <libipa/fc_queue.h>
> > @@ -67,6 +68,8 @@ struct IPAActiveState {
> >  		} automatic;
> >
> >  		bool autoEnabled;
> > +		uint32_t constraintMode;
> > +		uint32_t exposureMode;
> >  	} agc;
> >
> >  	struct {
> > @@ -151,6 +154,8 @@ struct IPAContext {
> >  	IPAActiveState activeState;
> >
> >  	FCQueue<IPAFrameContext> frameContexts;
> > +
> > +	ControlInfoMap::Map ctrlMap;
> >  };
> >
> >  } /* namespace ipa::rkisp1 */
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 9dc5f53c..d8610095 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
> >  } /* namespace */
> >
> >  IPARkISP1::IPARkISP1()
> > -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
> > +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
> >  {
> >  }
> >
> > @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> >  							      frameDurations[1],
> >  							      frameDurations[2]);
> >
> > +	ctrlMap.merge(context_.ctrlMap);
> >  	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> >  }
> >
> > --
> > 2.34.1
> >

Hi Jacopo

On 22/04/2024 11:49, Jacopo Mondi wrote:
> Hi Dan
>
> On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
>> Now that we have a MeanLuminanceAgc class that centralises our AEGC
>> algorithm, derive the RkISP1's Agc class from it and plumb in the
>> necessary framework to enable it to be used. For simplicities sake
>> this commit switches the algorithm to use the derived class, but
>> does not remove the bespoke functions at this time.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v2:
>>
>> 	- Squashed the patch adding parseStatistics()
>> 	- Used the first available constraint and exposure modes rather than
>> 	  assuming the "Normal" mode was available
>> 	- Stopped storing a Histogram in the class members when it's only needed
>> 	  during ::process()
>> 	- Remembered to report the controls out to IPARkISP1::updateControls()
>>
>>   src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
>>   src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
>>   src/ipa/rkisp1/ipa_context.h      |  5 ++
>>   src/ipa/rkisp1/rkisp1.cpp         |  3 +-
>>   4 files changed, 92 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>> index 47a6f7b2..0d66fcca 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>> @@ -64,6 +64,30 @@ Agc::Agc()
>>   	supportsRaw_ = true;
>>   }
>>
>> +/**
>> + * \brief Initialise the AGC algorithm from tuning files
>> + *
> no empty line
>
>> + * \param[in] context The shared IPA context
>> + * \param[in] tuningData The YamlObject containing Agc tuning data
>> + *
>> + * This function calls the base class' tuningData parsers to discover which
>> + * control values are supported.
>> + *
>> + * \return 0 on success or errors from the base class
>> + */
>> +int Agc::init(IPAContext &context, const YamlObject &tuningData)
>> +{
>> +	int ret;
>> +
>> +	ret = parseTuningData(tuningData);
>> +	if (ret)
>> +		return ret;
>> +
>> +	context.ctrlMap.merge(controls());
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * \brief Configure the AGC given a configInfo
>>    * \param[in] context The shared IPA context
>> @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>>   	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
>>   	context.activeState.agc.autoEnabled = !context.configuration.raw;
>>
>> +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
>> +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
>> +
>>   	/*
>>   	 * Define the measurement window for AGC as a centered rectangle
>>   	 * covering 3/4 of the image width and height.
>> @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>>   	 * frame index
>>   	 */
>>   	frameCount_ = 0;
>> +
>> +	/* \todo Run this again when FrameDurationLimits is passed in */
>> +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
>> +				     context.configuration.sensor.maxShutterSpeed,
>> +				     context.configuration.sensor.minAnalogueGain,
>> +				     context.configuration.sensor.maxAnalogueGain);
>> +
> I think I also commented this on the base class, but this should just
> be "setLimits" or something similar (iow do not expose the
> "ExposureModeHelpers" name to the derived classes)
>
>> +	resetFrameCount();
>> +
> As this HAS to be run at configure time, I would also consider a base
> class 'configure()' which sets limits and resets the frame counter and
> have derived classes call it
>
>>   	return 0;
>>   }
>>
>> @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>>   			  double yGain, double iqMeanGain)
>>   {
>>   	IPASessionConfiguration &configuration = context.configuration;
>> -	IPAActiveState &activeState = context.activeState;
>>
>>   	/* Get the effective exposure and gain applied on the sensor. */
>>   	uint32_t exposure = frameContext.sensor.exposure;
>> @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>>   	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>>   			      << shutterTime << " and "
>>   			      << stepGain;
>> -
>> -	/* Update the estimated exposure and gain. */
>> -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
>> -	activeState.agc.automatic.gain = stepGain;
>>   }
>>
>>   /**
>> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>>   	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>>   }
>>
>> +double Agc::estimateLuminance(double gain)
>> +{
>> +	double ySum = 0.0;
>> +
>> +	/* Sum the averages, saturated to 255. */
>> +	for (uint8_t expMean : expMeans_)
>> +		ySum += std::min(expMean * gain, 255.0);
>> +
>> +	/* \todo Weight with the AWB gains */
>> +
>> +	return ySum / expMeans_.size() / 255;
>> +}
>> +
>>   /**
>>    * \brief Process RkISP1 statistics, and run AGC operations
>>    * \param[in] context The shared IPA context
>> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>   	computeExposure(context, frameContext, yGain, iqMeanGain);
>>   	frameCount_++;
>>
>> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>> +
>> +	/*
>> +	 * The Agc algorithm needs to know the effective exposure value that was
>> +	 * applied to the sensor when the statistics were collected.
>> +	 */
>> +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
>> +				       * frameContext.sensor.exposure;
>> +	double analogueGain = frameContext.sensor.gain;
>> +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> same question, is this a duration ?
>
>> +
>> +	utils::Duration shutterTime;
>> +	double aGain, dGain;
>> +	std::tie(shutterTime, aGain, dGain) =
>> +		calculateNewEv(context.activeState.agc.constraintMode,
>> +			       context.activeState.agc.exposureMode,
>> +			       Histogram(hist), effectiveExposureValue);
>> +
>> +	LOG(RkISP1Agc, Debug)
>> +		<< "Divided up shutter, analogue gain and digital gain are "
>> +		<< shutterTime << ", " << aGain << " and " << dGain;
>> +
>> +	IPAActiveState &activeState = context.activeState;
>> +	/* Update the estimated exposure and gain. */
>> +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
>> +	activeState.agc.automatic.gain = aGain;
>> +
>>   	fillMetadata(context, frameContext, metadata);
>> +	expMeans_ = {};
> isn't this reset at the beginning of this function ?
>
> Or am I confused ? I see two estimateLuminance() (this is expected)
> one uses expMeans_ the other doesn't, but expMeans_ is assigned after
> the call to estimateLuminance() ?


It's assigned after the call to the bespoke estimateLuminance() - the overriding function for the 
base class is called later from calculateNewEv(). Resetting expMeans_ at the end of ::process() was 
a suggestion from Stefan, since it's holding references to stats->params->ae that might not be valid 
after process() is over.

>
>>   }
>>
>>   REGISTER_IPA_ALGORITHM(Agc, "Agc")
>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
>> index fb82a33f..a8080228 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.h
>> +++ b/src/ipa/rkisp1/algorithms/agc.h
>> @@ -14,18 +14,22 @@
>>
>>   #include <libcamera/geometry.h>
>>
>> +#include "libipa/agc_mean_luminance.h"
>> +#include "libipa/histogram.h"
>> +
>>   #include "algorithm.h"
>>
>>   namespace libcamera {
>>
>>   namespace ipa::rkisp1::algorithms {
>>
>> -class Agc : public Algorithm
>> +class Agc : public Algorithm, public AgcMeanLuminance
>>   {
>>   public:
>>   	Agc();
>>   	~Agc() = default;
>>
>> +	int init(IPAContext &context, const YamlObject &tuningData) override;
>>   	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>>   	void queueRequest(IPAContext &context,
>>   			  const uint32_t frame,
>> @@ -47,10 +51,13 @@ private:
>>   	double measureBrightness(Span<const uint32_t> hist) const;
>>   	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>>   			  ControlList &metadata);
>> +	double estimateLuminance(double gain) override;
>>
>>   	uint64_t frameCount_;
>>
>>   	utils::Duration filteredExposure_;
>> +
>> +	Span<const uint8_t> expMeans_;
>>   };
>>
>>   } /* namespace ipa::rkisp1::algorithms */
>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
>> index 10d8f38c..256b75eb 100644
>> --- a/src/ipa/rkisp1/ipa_context.h
>> +++ b/src/ipa/rkisp1/ipa_context.h
>> @@ -12,6 +12,7 @@
>>
>>   #include <libcamera/base/utils.h>
>>
>> +#include <libcamera/controls.h>
>>   #include <libcamera/geometry.h>
>>
>>   #include <libipa/fc_queue.h>
>> @@ -67,6 +68,8 @@ struct IPAActiveState {
>>   		} automatic;
>>
>>   		bool autoEnabled;
>> +		uint32_t constraintMode;
>> +		uint32_t exposureMode;
>>   	} agc;
>>
>>   	struct {
>> @@ -151,6 +154,8 @@ struct IPAContext {
>>   	IPAActiveState activeState;
>>
>>   	FCQueue<IPAFrameContext> frameContexts;
>> +
>> +	ControlInfoMap::Map ctrlMap;
>>   };
>>
>>   } /* namespace ipa::rkisp1 */
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 9dc5f53c..d8610095 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
>>   } /* namespace */
>>
>>   IPARkISP1::IPARkISP1()
>> -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
>> +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
>>   {
>>   }
>>
>> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>>   							      frameDurations[1],
>>   							      frameDurations[2]);
>>
>> +	ctrlMap.merge(context_.ctrlMap);
>>   	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
>>   }
>>
>> --
>> 2.34.1
>>

Hi Jacopo

On 22/04/2024 11:49, Jacopo Mondi wrote:
> Hi Dan
>
> On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
>> Now that we have a MeanLuminanceAgc class that centralises our AEGC
>> algorithm, derive the RkISP1's Agc class from it and plumb in the
>> necessary framework to enable it to be used. For simplicities sake
>> this commit switches the algorithm to use the derived class, but
>> does not remove the bespoke functions at this time.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v2:
>>
>> 	- Squashed the patch adding parseStatistics()
>> 	- Used the first available constraint and exposure modes rather than
>> 	  assuming the "Normal" mode was available
>> 	- Stopped storing a Histogram in the class members when it's only needed
>> 	  during ::process()
>> 	- Remembered to report the controls out to IPARkISP1::updateControls()
>>
>>   src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
>>   src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
>>   src/ipa/rkisp1/ipa_context.h      |  5 ++
>>   src/ipa/rkisp1/rkisp1.cpp         |  3 +-
>>   4 files changed, 92 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>> index 47a6f7b2..0d66fcca 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>> @@ -64,6 +64,30 @@ Agc::Agc()
>>   	supportsRaw_ = true;
>>   }
>>
>> +/**
>> + * \brief Initialise the AGC algorithm from tuning files
>> + *
> no empty line
>
>> + * \param[in] context The shared IPA context
>> + * \param[in] tuningData The YamlObject containing Agc tuning data
>> + *
>> + * This function calls the base class' tuningData parsers to discover which
>> + * control values are supported.
>> + *
>> + * \return 0 on success or errors from the base class
>> + */
>> +int Agc::init(IPAContext &context, const YamlObject &tuningData)
>> +{
>> +	int ret;
>> +
>> +	ret = parseTuningData(tuningData);
>> +	if (ret)
>> +		return ret;
>> +
>> +	context.ctrlMap.merge(controls());
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * \brief Configure the AGC given a configInfo
>>    * \param[in] context The shared IPA context
>> @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>>   	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
>>   	context.activeState.agc.autoEnabled = !context.configuration.raw;
>>
>> +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
>> +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
>> +
>>   	/*
>>   	 * Define the measurement window for AGC as a centered rectangle
>>   	 * covering 3/4 of the image width and height.
>> @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>>   	 * frame index
>>   	 */
>>   	frameCount_ = 0;
>> +
>> +	/* \todo Run this again when FrameDurationLimits is passed in */
>> +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
>> +				     context.configuration.sensor.maxShutterSpeed,
>> +				     context.configuration.sensor.minAnalogueGain,
>> +				     context.configuration.sensor.maxAnalogueGain);
>> +
> I think I also commented this on the base class, but this should just
> be "setLimits" or something similar (iow do not expose the
> "ExposureModeHelpers" name to the derived classes)
Why not, particularly?  Deriving from AgcMeanLuminance inherently means relying on 
ExposureModeHelpers, why would we have to hide them?
>> +	resetFrameCount();
>> +
> As this HAS to be run at configure time, I would also consider a base
> class 'configure()' which sets limits and resets the frame counter and
> have derived classes call it

Sure, we can do that
>
>>   	return 0;
>>   }
>>
>> @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>>   			  double yGain, double iqMeanGain)
>>   {
>>   	IPASessionConfiguration &configuration = context.configuration;
>> -	IPAActiveState &activeState = context.activeState;
>>
>>   	/* Get the effective exposure and gain applied on the sensor. */
>>   	uint32_t exposure = frameContext.sensor.exposure;
>> @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>>   	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>>   			      << shutterTime << " and "
>>   			      << stepGain;
>> -
>> -	/* Update the estimated exposure and gain. */
>> -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
>> -	activeState.agc.automatic.gain = stepGain;
>>   }
>>
>>   /**
>> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>>   	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>>   }
>>
>> +double Agc::estimateLuminance(double gain)
>> +{
>> +	double ySum = 0.0;
>> +
>> +	/* Sum the averages, saturated to 255. */
>> +	for (uint8_t expMean : expMeans_)
>> +		ySum += std::min(expMean * gain, 255.0);
>> +
>> +	/* \todo Weight with the AWB gains */
>> +
>> +	return ySum / expMeans_.size() / 255;
>> +}
>> +
>>   /**
>>    * \brief Process RkISP1 statistics, and run AGC operations
>>    * \param[in] context The shared IPA context
>> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>   	computeExposure(context, frameContext, yGain, iqMeanGain);
>>   	frameCount_++;
>>
>> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>> +
>> +	/*
>> +	 * The Agc algorithm needs to know the effective exposure value that was
>> +	 * applied to the sensor when the statistics were collected.
>> +	 */
>> +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
>> +				       * frameContext.sensor.exposure;
>> +	double analogueGain = frameContext.sensor.gain;
>> +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> same question, is this a duration ?
>
>> +
>> +	utils::Duration shutterTime;
>> +	double aGain, dGain;
>> +	std::tie(shutterTime, aGain, dGain) =
>> +		calculateNewEv(context.activeState.agc.constraintMode,
>> +			       context.activeState.agc.exposureMode,
>> +			       Histogram(hist), effectiveExposureValue);
>> +
>> +	LOG(RkISP1Agc, Debug)
>> +		<< "Divided up shutter, analogue gain and digital gain are "
>> +		<< shutterTime << ", " << aGain << " and " << dGain;
>> +
>> +	IPAActiveState &activeState = context.activeState;
>> +	/* Update the estimated exposure and gain. */
>> +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
>> +	activeState.agc.automatic.gain = aGain;
>> +
>>   	fillMetadata(context, frameContext, metadata);
>> +	expMeans_ = {};
> isn't this reset at the beginning of this function ?
>
> Or am I confused ? I see two estimateLuminance() (this is expected)
> one uses expMeans_ the other doesn't, but expMeans_ is assigned after
> the call to estimateLuminance() ?
>
>>   }
>>
>>   REGISTER_IPA_ALGORITHM(Agc, "Agc")
>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
>> index fb82a33f..a8080228 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.h
>> +++ b/src/ipa/rkisp1/algorithms/agc.h
>> @@ -14,18 +14,22 @@
>>
>>   #include <libcamera/geometry.h>
>>
>> +#include "libipa/agc_mean_luminance.h"
>> +#include "libipa/histogram.h"
>> +
>>   #include "algorithm.h"
>>
>>   namespace libcamera {
>>
>>   namespace ipa::rkisp1::algorithms {
>>
>> -class Agc : public Algorithm
>> +class Agc : public Algorithm, public AgcMeanLuminance
>>   {
>>   public:
>>   	Agc();
>>   	~Agc() = default;
>>
>> +	int init(IPAContext &context, const YamlObject &tuningData) override;
>>   	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>>   	void queueRequest(IPAContext &context,
>>   			  const uint32_t frame,
>> @@ -47,10 +51,13 @@ private:
>>   	double measureBrightness(Span<const uint32_t> hist) const;
>>   	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>>   			  ControlList &metadata);
>> +	double estimateLuminance(double gain) override;
>>
>>   	uint64_t frameCount_;
>>
>>   	utils::Duration filteredExposure_;
>> +
>> +	Span<const uint8_t> expMeans_;
>>   };
>>
>>   } /* namespace ipa::rkisp1::algorithms */
>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
>> index 10d8f38c..256b75eb 100644
>> --- a/src/ipa/rkisp1/ipa_context.h
>> +++ b/src/ipa/rkisp1/ipa_context.h
>> @@ -12,6 +12,7 @@
>>
>>   #include <libcamera/base/utils.h>
>>
>> +#include <libcamera/controls.h>
>>   #include <libcamera/geometry.h>
>>
>>   #include <libipa/fc_queue.h>
>> @@ -67,6 +68,8 @@ struct IPAActiveState {
>>   		} automatic;
>>
>>   		bool autoEnabled;
>> +		uint32_t constraintMode;
>> +		uint32_t exposureMode;
>>   	} agc;
>>
>>   	struct {
>> @@ -151,6 +154,8 @@ struct IPAContext {
>>   	IPAActiveState activeState;
>>
>>   	FCQueue<IPAFrameContext> frameContexts;
>> +
>> +	ControlInfoMap::Map ctrlMap;
>>   };
>>
>>   } /* namespace ipa::rkisp1 */
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 9dc5f53c..d8610095 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
>>   } /* namespace */
>>
>>   IPARkISP1::IPARkISP1()
>> -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
>> +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
>>   {
>>   }
>>
>> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>>   							      frameDurations[1],
>>   							      frameDurations[2]);
>>
>> +	ctrlMap.merge(context_.ctrlMap);
>>   	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
>>   }
>>
>> --
>> 2.34.1
>>

Hi Jacopo

On 22/04/2024 11:49, Jacopo Mondi wrote:
> Hi Dan
>
> On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
>> Now that we have a MeanLuminanceAgc class that centralises our AEGC
>> algorithm, derive the RkISP1's Agc class from it and plumb in the
>> necessary framework to enable it to be used. For simplicities sake
>> this commit switches the algorithm to use the derived class, but
>> does not remove the bespoke functions at this time.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v2:
>>
>> 	- Squashed the patch adding parseStatistics()
>> 	- Used the first available constraint and exposure modes rather than
>> 	  assuming the "Normal" mode was available
>> 	- Stopped storing a Histogram in the class members when it's only needed
>> 	  during ::process()
>> 	- Remembered to report the controls out to IPARkISP1::updateControls()
>>
>>   src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
>>   src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
>>   src/ipa/rkisp1/ipa_context.h      |  5 ++
>>   src/ipa/rkisp1/rkisp1.cpp         |  3 +-
>>   4 files changed, 92 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>> index 47a6f7b2..0d66fcca 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>> @@ -64,6 +64,30 @@ Agc::Agc()
>>   	supportsRaw_ = true;
>>   }
>>
>> +/**
>> + * \brief Initialise the AGC algorithm from tuning files
>> + *
> no empty line
>
>> + * \param[in] context The shared IPA context
>> + * \param[in] tuningData The YamlObject containing Agc tuning data
>> + *
>> + * This function calls the base class' tuningData parsers to discover which
>> + * control values are supported.
>> + *
>> + * \return 0 on success or errors from the base class
>> + */
>> +int Agc::init(IPAContext &context, const YamlObject &tuningData)
>> +{
>> +	int ret;
>> +
>> +	ret = parseTuningData(tuningData);
>> +	if (ret)
>> +		return ret;
>> +
>> +	context.ctrlMap.merge(controls());
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * \brief Configure the AGC given a configInfo
>>    * \param[in] context The shared IPA context
>> @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>>   	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
>>   	context.activeState.agc.autoEnabled = !context.configuration.raw;
>>
>> +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
>> +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
>> +
>>   	/*
>>   	 * Define the measurement window for AGC as a centered rectangle
>>   	 * covering 3/4 of the image width and height.
>> @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>>   	 * frame index
>>   	 */
>>   	frameCount_ = 0;
>> +
>> +	/* \todo Run this again when FrameDurationLimits is passed in */
>> +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
>> +				     context.configuration.sensor.maxShutterSpeed,
>> +				     context.configuration.sensor.minAnalogueGain,
>> +				     context.configuration.sensor.maxAnalogueGain);
>> +
> I think I also commented this on the base class, but this should just
> be "setLimits" or something similar (iow do not expose the
> "ExposureModeHelpers" name to the derived classes)
>
>> +	resetFrameCount();
>> +
> As this HAS to be run at configure time, I would also consider a base
> class 'configure()' which sets limits and resets the frame counter and
> have derived classes call it


Actually I'm vacillating on this, because we have to call configureExposureModeHelpers() (or 
setLimits() or whatever it ends up to be) outside of the derived class' ::configure() call too, like 
in queueRequest() if FrameDurationLimits are set that will change the minimum and maximum shutter 
speeds...is it worth bundling them into an AgcMeanLuminance::configure() function if it'll be called 
outside that too?

>>   	return 0;
>>   }
>>
>> @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>>   			  double yGain, double iqMeanGain)
>>   {
>>   	IPASessionConfiguration &configuration = context.configuration;
>> -	IPAActiveState &activeState = context.activeState;
>>
>>   	/* Get the effective exposure and gain applied on the sensor. */
>>   	uint32_t exposure = frameContext.sensor.exposure;
>> @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>>   	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>>   			      << shutterTime << " and "
>>   			      << stepGain;
>> -
>> -	/* Update the estimated exposure and gain. */
>> -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
>> -	activeState.agc.automatic.gain = stepGain;
>>   }
>>
>>   /**
>> @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>>   	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>>   }
>>
>> +double Agc::estimateLuminance(double gain)
>> +{
>> +	double ySum = 0.0;
>> +
>> +	/* Sum the averages, saturated to 255. */
>> +	for (uint8_t expMean : expMeans_)
>> +		ySum += std::min(expMean * gain, 255.0);
>> +
>> +	/* \todo Weight with the AWB gains */
>> +
>> +	return ySum / expMeans_.size() / 255;
>> +}
>> +
>>   /**
>>    * \brief Process RkISP1 statistics, and run AGC operations
>>    * \param[in] context The shared IPA context
>> @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>>   	computeExposure(context, frameContext, yGain, iqMeanGain);
>>   	frameCount_++;
>>
>> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>> +
>> +	/*
>> +	 * The Agc algorithm needs to know the effective exposure value that was
>> +	 * applied to the sensor when the statistics were collected.
>> +	 */
>> +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
>> +				       * frameContext.sensor.exposure;
>> +	double analogueGain = frameContext.sensor.gain;
>> +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> same question, is this a duration ?
>
>> +
>> +	utils::Duration shutterTime;
>> +	double aGain, dGain;
>> +	std::tie(shutterTime, aGain, dGain) =
>> +		calculateNewEv(context.activeState.agc.constraintMode,
>> +			       context.activeState.agc.exposureMode,
>> +			       Histogram(hist), effectiveExposureValue);
>> +
>> +	LOG(RkISP1Agc, Debug)
>> +		<< "Divided up shutter, analogue gain and digital gain are "
>> +		<< shutterTime << ", " << aGain << " and " << dGain;
>> +
>> +	IPAActiveState &activeState = context.activeState;
>> +	/* Update the estimated exposure and gain. */
>> +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
>> +	activeState.agc.automatic.gain = aGain;
>> +
>>   	fillMetadata(context, frameContext, metadata);
>> +	expMeans_ = {};
> isn't this reset at the beginning of this function ?
>
> Or am I confused ? I see two estimateLuminance() (this is expected)
> one uses expMeans_ the other doesn't, but expMeans_ is assigned after
> the call to estimateLuminance() ?
>
>>   }
>>
>>   REGISTER_IPA_ALGORITHM(Agc, "Agc")
>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
>> index fb82a33f..a8080228 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.h
>> +++ b/src/ipa/rkisp1/algorithms/agc.h
>> @@ -14,18 +14,22 @@
>>
>>   #include <libcamera/geometry.h>
>>
>> +#include "libipa/agc_mean_luminance.h"
>> +#include "libipa/histogram.h"
>> +
>>   #include "algorithm.h"
>>
>>   namespace libcamera {
>>
>>   namespace ipa::rkisp1::algorithms {
>>
>> -class Agc : public Algorithm
>> +class Agc : public Algorithm, public AgcMeanLuminance
>>   {
>>   public:
>>   	Agc();
>>   	~Agc() = default;
>>
>> +	int init(IPAContext &context, const YamlObject &tuningData) override;
>>   	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>>   	void queueRequest(IPAContext &context,
>>   			  const uint32_t frame,
>> @@ -47,10 +51,13 @@ private:
>>   	double measureBrightness(Span<const uint32_t> hist) const;
>>   	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>>   			  ControlList &metadata);
>> +	double estimateLuminance(double gain) override;
>>
>>   	uint64_t frameCount_;
>>
>>   	utils::Duration filteredExposure_;
>> +
>> +	Span<const uint8_t> expMeans_;
>>   };
>>
>>   } /* namespace ipa::rkisp1::algorithms */
>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
>> index 10d8f38c..256b75eb 100644
>> --- a/src/ipa/rkisp1/ipa_context.h
>> +++ b/src/ipa/rkisp1/ipa_context.h
>> @@ -12,6 +12,7 @@
>>
>>   #include <libcamera/base/utils.h>
>>
>> +#include <libcamera/controls.h>
>>   #include <libcamera/geometry.h>
>>
>>   #include <libipa/fc_queue.h>
>> @@ -67,6 +68,8 @@ struct IPAActiveState {
>>   		} automatic;
>>
>>   		bool autoEnabled;
>> +		uint32_t constraintMode;
>> +		uint32_t exposureMode;
>>   	} agc;
>>
>>   	struct {
>> @@ -151,6 +154,8 @@ struct IPAContext {
>>   	IPAActiveState activeState;
>>
>>   	FCQueue<IPAFrameContext> frameContexts;
>> +
>> +	ControlInfoMap::Map ctrlMap;
>>   };
>>
>>   } /* namespace ipa::rkisp1 */
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 9dc5f53c..d8610095 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
>>   } /* namespace */
>>
>>   IPARkISP1::IPARkISP1()
>> -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
>> +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
>>   {
>>   }
>>
>> @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>>   							      frameDurations[1],
>>   							      frameDurations[2]);
>>
>> +	ctrlMap.merge(context_.ctrlMap);
>>   	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
>>   }
>>
>> --
>> 2.34.1
>>

Hi Dan

On Wed, Apr 24, 2024 at 11:01:18AM +0100, Dan Scally wrote:
> Hi Jacopo
>
> On 22/04/2024 11:49, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
> > > Now that we have a MeanLuminanceAgc class that centralises our AEGC
> > > algorithm, derive the RkISP1's Agc class from it and plumb in the
> > > necessary framework to enable it to be used. For simplicities sake
> > > this commit switches the algorithm to use the derived class, but
> > > does not remove the bespoke functions at this time.
> > >
> > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > ---
> > > Changes in v2:
> > >
> > > 	- Squashed the patch adding parseStatistics()
> > > 	- Used the first available constraint and exposure modes rather than
> > > 	  assuming the "Normal" mode was available
> > > 	- Stopped storing a Histogram in the class members when it's only needed
> > > 	  during ::process()
> > > 	- Remembered to report the controls out to IPARkISP1::updateControls()
> > >
> > >   src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
> > >   src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
> > >   src/ipa/rkisp1/ipa_context.h      |  5 ++
> > >   src/ipa/rkisp1/rkisp1.cpp         |  3 +-
> > >   4 files changed, 92 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 47a6f7b2..0d66fcca 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -64,6 +64,30 @@ Agc::Agc()
> > >   	supportsRaw_ = true;
> > >   }
> > >
> > > +/**
> > > + * \brief Initialise the AGC algorithm from tuning files
> > > + *
> > no empty line
> >
> > > + * \param[in] context The shared IPA context
> > > + * \param[in] tuningData The YamlObject containing Agc tuning data
> > > + *
> > > + * This function calls the base class' tuningData parsers to discover which
> > > + * control values are supported.
> > > + *
> > > + * \return 0 on success or errors from the base class
> > > + */
> > > +int Agc::init(IPAContext &context, const YamlObject &tuningData)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = parseTuningData(tuningData);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	context.ctrlMap.merge(controls());
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   /**
> > >    * \brief Configure the AGC given a configInfo
> > >    * \param[in] context The shared IPA context
> > > @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >   	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > >   	context.activeState.agc.autoEnabled = !context.configuration.raw;
> > >
> > > +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> > > +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > > +
> > >   	/*
> > >   	 * Define the measurement window for AGC as a centered rectangle
> > >   	 * covering 3/4 of the image width and height.
> > > @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >   	 * frame index
> > >   	 */
> > >   	frameCount_ = 0;
> > > +
> > > +	/* \todo Run this again when FrameDurationLimits is passed in */
> > > +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> > > +				     context.configuration.sensor.maxShutterSpeed,
> > > +				     context.configuration.sensor.minAnalogueGain,
> > > +				     context.configuration.sensor.maxAnalogueGain);
> > > +
> > I think I also commented this on the base class, but this should just
> > be "setLimits" or something similar (iow do not expose the
> > "ExposureModeHelpers" name to the derived classes)
> Why not, particularly?  Deriving from AgcMeanLuminance inherently means
> relying on ExposureModeHelpers, why would we have to hide them?

The base class uses it, the derived classes simply rely on the base
class calculateNewEv() function, right ?

How the base class does that it's opaque to the derived classes,
henche I don't see any benefits in mentioning the helper is a function
name. Isn't 'setLimits()' enough ? What is the benfint of mentioning
the ExposureModeHelper here ?

> > > +	resetFrameCount();
> > > +
> > As this HAS to be run at configure time, I would also consider a base
> > class 'configure()' which sets limits and resets the frame counter and
> > have derived classes call it
>
> Sure, we can do that

And that would also hide the helper

> >
> > >   	return 0;
> > >   }
> > >
> > > @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > >   			  double yGain, double iqMeanGain)
> > >   {
> > >   	IPASessionConfiguration &configuration = context.configuration;
> > > -	IPAActiveState &activeState = context.activeState;
> > >
> > >   	/* Get the effective exposure and gain applied on the sensor. */
> > >   	uint32_t exposure = frameContext.sensor.exposure;
> > > @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > >   	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
> > >   			      << shutterTime << " and "
> > >   			      << stepGain;
> > > -
> > > -	/* Update the estimated exposure and gain. */
> > > -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> > > -	activeState.agc.automatic.gain = stepGain;
> > >   }
> > >
> > >   /**
> > > @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > >   	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > >   }
> > >
> > > +double Agc::estimateLuminance(double gain)
> > > +{
> > > +	double ySum = 0.0;
> > > +
> > > +	/* Sum the averages, saturated to 255. */
> > > +	for (uint8_t expMean : expMeans_)
> > > +		ySum += std::min(expMean * gain, 255.0);
> > > +
> > > +	/* \todo Weight with the AWB gains */
> > > +
> > > +	return ySum / expMeans_.size() / 255;
> > > +}
> > > +
> > >   /**
> > >    * \brief Process RkISP1 statistics, and run AGC operations
> > >    * \param[in] context The shared IPA context
> > > @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >   	computeExposure(context, frameContext, yGain, iqMeanGain);
> > >   	frameCount_++;
> > >
> > > +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > > +
> > > +	/*
> > > +	 * The Agc algorithm needs to know the effective exposure value that was
> > > +	 * applied to the sensor when the statistics were collected.
> > > +	 */
> > > +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > > +				       * frameContext.sensor.exposure;
> > > +	double analogueGain = frameContext.sensor.gain;
> > > +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > same question, is this a duration ?
> >
> > > +
> > > +	utils::Duration shutterTime;
> > > +	double aGain, dGain;
> > > +	std::tie(shutterTime, aGain, dGain) =
> > > +		calculateNewEv(context.activeState.agc.constraintMode,
> > > +			       context.activeState.agc.exposureMode,
> > > +			       Histogram(hist), effectiveExposureValue);
> > > +
> > > +	LOG(RkISP1Agc, Debug)
> > > +		<< "Divided up shutter, analogue gain and digital gain are "
> > > +		<< shutterTime << ", " << aGain << " and " << dGain;
> > > +
> > > +	IPAActiveState &activeState = context.activeState;
> > > +	/* Update the estimated exposure and gain. */
> > > +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> > > +	activeState.agc.automatic.gain = aGain;
> > > +
> > >   	fillMetadata(context, frameContext, metadata);
> > > +	expMeans_ = {};
> > isn't this reset at the beginning of this function ?
> >
> > Or am I confused ? I see two estimateLuminance() (this is expected)
> > one uses expMeans_ the other doesn't, but expMeans_ is assigned after
> > the call to estimateLuminance() ?
> >
> > >   }
> > >
> > >   REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > > index fb82a33f..a8080228 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.h
> > > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > > @@ -14,18 +14,22 @@
> > >
> > >   #include <libcamera/geometry.h>
> > >
> > > +#include "libipa/agc_mean_luminance.h"
> > > +#include "libipa/histogram.h"
> > > +
> > >   #include "algorithm.h"
> > >
> > >   namespace libcamera {
> > >
> > >   namespace ipa::rkisp1::algorithms {
> > >
> > > -class Agc : public Algorithm
> > > +class Agc : public Algorithm, public AgcMeanLuminance
> > >   {
> > >   public:
> > >   	Agc();
> > >   	~Agc() = default;
> > >
> > > +	int init(IPAContext &context, const YamlObject &tuningData) override;
> > >   	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > >   	void queueRequest(IPAContext &context,
> > >   			  const uint32_t frame,
> > > @@ -47,10 +51,13 @@ private:
> > >   	double measureBrightness(Span<const uint32_t> hist) const;
> > >   	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > >   			  ControlList &metadata);
> > > +	double estimateLuminance(double gain) override;
> > >
> > >   	uint64_t frameCount_;
> > >
> > >   	utils::Duration filteredExposure_;
> > > +
> > > +	Span<const uint8_t> expMeans_;
> > >   };
> > >
> > >   } /* namespace ipa::rkisp1::algorithms */
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 10d8f38c..256b75eb 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -12,6 +12,7 @@
> > >
> > >   #include <libcamera/base/utils.h>
> > >
> > > +#include <libcamera/controls.h>
> > >   #include <libcamera/geometry.h>
> > >
> > >   #include <libipa/fc_queue.h>
> > > @@ -67,6 +68,8 @@ struct IPAActiveState {
> > >   		} automatic;
> > >
> > >   		bool autoEnabled;
> > > +		uint32_t constraintMode;
> > > +		uint32_t exposureMode;
> > >   	} agc;
> > >
> > >   	struct {
> > > @@ -151,6 +154,8 @@ struct IPAContext {
> > >   	IPAActiveState activeState;
> > >
> > >   	FCQueue<IPAFrameContext> frameContexts;
> > > +
> > > +	ControlInfoMap::Map ctrlMap;
> > >   };
> > >
> > >   } /* namespace ipa::rkisp1 */
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 9dc5f53c..d8610095 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
> > >   } /* namespace */
> > >
> > >   IPARkISP1::IPARkISP1()
> > > -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
> > > +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
> > >   {
> > >   }
> > >
> > > @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > >   							      frameDurations[1],
> > >   							      frameDurations[2]);
> > >
> > > +	ctrlMap.merge(context_.ctrlMap);
> > >   	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > >   }
> > >
> > > --
> > > 2.34.1
> > >

Hi Dan

On Wed, Apr 24, 2024 at 11:05:18AM +0100, Dan Scally wrote:
> Hi Jacopo
>
> On 22/04/2024 11:49, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Wed, Apr 17, 2024 at 02:15:35PM +0100, Daniel Scally wrote:
> > > Now that we have a MeanLuminanceAgc class that centralises our AEGC
> > > algorithm, derive the RkISP1's Agc class from it and plumb in the
> > > necessary framework to enable it to be used. For simplicities sake
> > > this commit switches the algorithm to use the derived class, but
> > > does not remove the bespoke functions at this time.
> > >
> > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > ---
> > > Changes in v2:
> > >
> > > 	- Squashed the patch adding parseStatistics()
> > > 	- Used the first available constraint and exposure modes rather than
> > > 	  assuming the "Normal" mode was available
> > > 	- Stopped storing a Histogram in the class members when it's only needed
> > > 	  during ::process()
> > > 	- Remembered to report the controls out to IPARkISP1::updateControls()
> > >
> > >   src/ipa/rkisp1/algorithms/agc.cpp | 82 +++++++++++++++++++++++++++++--
> > >   src/ipa/rkisp1/algorithms/agc.h   |  9 +++-
> > >   src/ipa/rkisp1/ipa_context.h      |  5 ++
> > >   src/ipa/rkisp1/rkisp1.cpp         |  3 +-
> > >   4 files changed, 92 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 47a6f7b2..0d66fcca 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -64,6 +64,30 @@ Agc::Agc()
> > >   	supportsRaw_ = true;
> > >   }
> > >
> > > +/**
> > > + * \brief Initialise the AGC algorithm from tuning files
> > > + *
> > no empty line
> >
> > > + * \param[in] context The shared IPA context
> > > + * \param[in] tuningData The YamlObject containing Agc tuning data
> > > + *
> > > + * This function calls the base class' tuningData parsers to discover which
> > > + * control values are supported.
> > > + *
> > > + * \return 0 on success or errors from the base class
> > > + */
> > > +int Agc::init(IPAContext &context, const YamlObject &tuningData)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = parseTuningData(tuningData);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	context.ctrlMap.merge(controls());
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   /**
> > >    * \brief Configure the AGC given a configInfo
> > >    * \param[in] context The shared IPA context
> > > @@ -81,6 +105,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >   	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > >   	context.activeState.agc.autoEnabled = !context.configuration.raw;
> > >
> > > +	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> > > +	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > > +
> > >   	/*
> > >   	 * Define the measurement window for AGC as a centered rectangle
> > >   	 * covering 3/4 of the image width and height.
> > > @@ -95,6 +122,15 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >   	 * frame index
> > >   	 */
> > >   	frameCount_ = 0;
> > > +
> > > +	/* \todo Run this again when FrameDurationLimits is passed in */
> > > +	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> > > +				     context.configuration.sensor.maxShutterSpeed,
> > > +				     context.configuration.sensor.minAnalogueGain,
> > > +				     context.configuration.sensor.maxAnalogueGain);
> > > +
> > I think I also commented this on the base class, but this should just
> > be "setLimits" or something similar (iow do not expose the
> > "ExposureModeHelpers" name to the derived classes)
> >
> > > +	resetFrameCount();
> > > +
> > As this HAS to be run at configure time, I would also consider a base
> > class 'configure()' which sets limits and resets the frame counter and
> > have derived classes call it
>
>
> Actually I'm vacillating on this, because we have to call
> configureExposureModeHelpers() (or setLimits() or whatever it ends up to be)
> outside of the derived class' ::configure() call too, like in queueRequest()
> if FrameDurationLimits are set that will change the minimum and maximum
> shutter speeds...is it worth bundling them into an
> AgcMeanLuminance::configure() function if it'll be called outside that too?
>

Ok, maybe not then, but be sure in the documentation of the base class
to mention that derived classes have to set limits and reset the frame
counter during their configure() implementation.


> > >   	return 0;
> > >   }
> > >
> > > @@ -234,7 +270,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > >   			  double yGain, double iqMeanGain)
> > >   {
> > >   	IPASessionConfiguration &configuration = context.configuration;
> > > -	IPAActiveState &activeState = context.activeState;
> > >
> > >   	/* Get the effective exposure and gain applied on the sensor. */
> > >   	uint32_t exposure = frameContext.sensor.exposure;
> > > @@ -300,10 +335,6 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > >   	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
> > >   			      << shutterTime << " and "
> > >   			      << stepGain;
> > > -
> > > -	/* Update the estimated exposure and gain. */
> > > -	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> > > -	activeState.agc.automatic.gain = stepGain;
> > >   }
> > >
> > >   /**
> > > @@ -373,6 +404,19 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > >   	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > >   }
> > >
> > > +double Agc::estimateLuminance(double gain)
> > > +{
> > > +	double ySum = 0.0;
> > > +
> > > +	/* Sum the averages, saturated to 255. */
> > > +	for (uint8_t expMean : expMeans_)
> > > +		ySum += std::min(expMean * gain, 255.0);
> > > +
> > > +	/* \todo Weight with the AWB gains */
> > > +
> > > +	return ySum / expMeans_.size() / 255;
> > > +}
> > > +
> > >   /**
> > >    * \brief Process RkISP1 statistics, and run AGC operations
> > >    * \param[in] context The shared IPA context
> > > @@ -438,7 +482,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >   	computeExposure(context, frameContext, yGain, iqMeanGain);
> > >   	frameCount_++;
> > >
> > > +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > > +
> > > +	/*
> > > +	 * The Agc algorithm needs to know the effective exposure value that was
> > > +	 * applied to the sensor when the statistics were collected.
> > > +	 */
> > > +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > > +				       * frameContext.sensor.exposure;
> > > +	double analogueGain = frameContext.sensor.gain;
> > > +	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > same question, is this a duration ?
> >
> > > +
> > > +	utils::Duration shutterTime;
> > > +	double aGain, dGain;
> > > +	std::tie(shutterTime, aGain, dGain) =
> > > +		calculateNewEv(context.activeState.agc.constraintMode,
> > > +			       context.activeState.agc.exposureMode,
> > > +			       Histogram(hist), effectiveExposureValue);
> > > +
> > > +	LOG(RkISP1Agc, Debug)
> > > +		<< "Divided up shutter, analogue gain and digital gain are "
> > > +		<< shutterTime << ", " << aGain << " and " << dGain;
> > > +
> > > +	IPAActiveState &activeState = context.activeState;
> > > +	/* Update the estimated exposure and gain. */
> > > +	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> > > +	activeState.agc.automatic.gain = aGain;
> > > +
> > >   	fillMetadata(context, frameContext, metadata);
> > > +	expMeans_ = {};
> > isn't this reset at the beginning of this function ?
> >
> > Or am I confused ? I see two estimateLuminance() (this is expected)
> > one uses expMeans_ the other doesn't, but expMeans_ is assigned after
> > the call to estimateLuminance() ?
> >
> > >   }
> > >
> > >   REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > > index fb82a33f..a8080228 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.h
> > > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > > @@ -14,18 +14,22 @@
> > >
> > >   #include <libcamera/geometry.h>
> > >
> > > +#include "libipa/agc_mean_luminance.h"
> > > +#include "libipa/histogram.h"
> > > +
> > >   #include "algorithm.h"
> > >
> > >   namespace libcamera {
> > >
> > >   namespace ipa::rkisp1::algorithms {
> > >
> > > -class Agc : public Algorithm
> > > +class Agc : public Algorithm, public AgcMeanLuminance
> > >   {
> > >   public:
> > >   	Agc();
> > >   	~Agc() = default;
> > >
> > > +	int init(IPAContext &context, const YamlObject &tuningData) override;
> > >   	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > >   	void queueRequest(IPAContext &context,
> > >   			  const uint32_t frame,
> > > @@ -47,10 +51,13 @@ private:
> > >   	double measureBrightness(Span<const uint32_t> hist) const;
> > >   	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > >   			  ControlList &metadata);
> > > +	double estimateLuminance(double gain) override;
> > >
> > >   	uint64_t frameCount_;
> > >
> > >   	utils::Duration filteredExposure_;
> > > +
> > > +	Span<const uint8_t> expMeans_;
> > >   };
> > >
> > >   } /* namespace ipa::rkisp1::algorithms */
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 10d8f38c..256b75eb 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -12,6 +12,7 @@
> > >
> > >   #include <libcamera/base/utils.h>
> > >
> > > +#include <libcamera/controls.h>
> > >   #include <libcamera/geometry.h>
> > >
> > >   #include <libipa/fc_queue.h>
> > > @@ -67,6 +68,8 @@ struct IPAActiveState {
> > >   		} automatic;
> > >
> > >   		bool autoEnabled;
> > > +		uint32_t constraintMode;
> > > +		uint32_t exposureMode;
> > >   	} agc;
> > >
> > >   	struct {
> > > @@ -151,6 +154,8 @@ struct IPAContext {
> > >   	IPAActiveState activeState;
> > >
> > >   	FCQueue<IPAFrameContext> frameContexts;
> > > +
> > > +	ControlInfoMap::Map ctrlMap;
> > >   };
> > >
> > >   } /* namespace ipa::rkisp1 */
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 9dc5f53c..d8610095 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -119,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{
> > >   } /* namespace */
> > >
> > >   IPARkISP1::IPARkISP1()
> > > -	: context_({ {}, {}, {}, { kMaxFrameContexts } })
> > > +	: context_({ {}, {}, {}, { kMaxFrameContexts }, {} })
> > >   {
> > >   }
> > >
> > > @@ -427,6 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > >   							      frameDurations[1],
> > >   							      frameDurations[2]);
> > >
> > > +	ctrlMap.merge(context_.ctrlMap);
> > >   	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > >   }
> > >
> > > --
> > > 2.34.1
> > >