[libcamera-devel] [PATCH v1 09/10] ipa: raspberrypi: Generalise the agc algorithm

[Jacopo Mondi]

Hi Naush

On Wed, Mar 22, 2023 at 01:06:11PM +0000, Naushir Patuck via libcamera-devel wrote:
> Remove any hard-coded assumptions about the target hardware platform
> from the AGC algorithm. Instead, use the "target" string provided by
> the camera tuning config and generalised statistics structures to
> determing parameters such as grid and region sizes.
>
> This change replaces all hard-coded arrays with equivalent std::vector
> types.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 19 ++++++++++++-------
>  src/ipa/raspberrypi/controller/rpi/agc.h   |  9 +--------
>  2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 4ea0dd41e66c..3e4a8149b9ae 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -31,17 +31,12 @@ LOG_DEFINE_CATEGORY(RPiAgc)
>  int AgcMeteringMode::read(const libcamera::YamlObject &params)
>  {
>  	const YamlObject &yamlWeights = params["weights"];
> -	if (yamlWeights.size() != AgcStatsSize) {
> -		LOG(RPiAgc, Error) << "AgcMeteringMode: Incorrect number of weights";
> -		return -EINVAL;
> -	}
>
> -	unsigned int num = 0;
>  	for (const auto &p : yamlWeights.asList()) {
>  		auto value = p.get<double>();
>  		if (!value)
>  			return -EINVAL;
> -		weights[num++] = *value;
> +		weights.push_back(*value);


>  	}
>
>  	return 0;
> @@ -249,6 +244,14 @@ int Agc::read(const libcamera::YamlObject &params)
>  	if (ret)
>  		return ret;
>
> +	const Size &size = getHardwareConfig().agcZoneWeights;
> +	for (auto const &modes : config_.meteringModes) {
> +		if (modes.second.weights.size() != size.width * size.height) {
> +			LOG(RPiAgc, Error) << "AgcMeteringMode: Incorrect number of weights";
> +			return -EINVAL;
> +		}
> +	}
> +
>  	/*
>  	 * Set the config's defaults (which are the first ones it read) as our
>  	 * current modes, until someone changes them.  (they're all known to
> @@ -582,9 +585,11 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)
>  }
>
>  static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
> -			      double weights[], double gain)
> +			      std::vector<double> &weights, double gain)
>  {
>  	constexpr uint64_t maxVal = 1 << Statistics::NormalisationFactorPow2;
> +
> +	ASSERT(weights.size() == stats->agcRegions.numRegions());

Maybe an empty line, but just if you have to resend

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

>  	/*
>  	 * Note how the calculation below means that equal weights give you
>  	 * "average" metering (i.e. all pixels equally important).
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h
> index f04896ca25ad..d11a49c9cd85 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.h
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h
> @@ -17,17 +17,10 @@
>
>  /* This is our implementation of AGC. */
>
> -/*
> - * This is the number actually set up by the firmware, not the maximum possible
> - * number (which is 16).
> - */
> -
> -constexpr unsigned int AgcStatsSize = 15;
> -
>  namespace RPiController {
>
>  struct AgcMeteringMode {
> -	double weights[AgcStatsSize];
> +	std::vector<double> weights;
>  	int read(const libcamera::YamlObject &params);
>  };
>
> --
> 2.34.1
>