[PATCH v3 1/3] ipa: rkisp1: agc: Read histogram weights from tuning file

[Dan Scally]

Hi Paul

On 17/05/2024 09:07, Paul Elder wrote:
> Add support to the rkisp1 AGC to read histogram weights from the tuning
> file. As controls for selecting the metering mode are not yet supported,
> for now hardcode the matrix metering mode, which is the same as what the
> AGC previously hardcoded.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v3:
> - support the new tuning file layout for interpolated matrices
> - support both v10 and v12
>
> Changes in v2:
> - add default metering mode if none are read successfully from the
>    tuning file
> - compute the predivider instead of using a table
> ---
>   src/ipa/rkisp1/algorithms/agc.cpp | 94 ++++++++++++++++++++++++++++++-
>   src/ipa/rkisp1/algorithms/agc.h   |  6 ++
>   2 files changed, 97 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 5aad5c7c2..4af397bdc 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -17,6 +17,8 @@
>   #include <libcamera/control_ids.h>
>   #include <libcamera/ipa/core_ipa_interface.h>
>   
> +#include "libcamera/internal/yaml_parser.h"
> +
>   #include "libipa/histogram.h"
>   
>   /**
> @@ -36,6 +38,76 @@ namespace ipa::rkisp1::algorithms {
>   
>   LOG_DEFINE_CATEGORY(RkISP1Agc)
>   
> +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
> +			    const char *prop)
I think a comment with an example of the format that it'll parse would be good. Also; is there a 
particular reason to have the property name as a parameter instead of just hardcoding it in here 
(rather than the caller)? Doesn't particularly matter, just curious.
> +{
> +	const YamlObject &yamlMeteringModes = tuningData[prop];
> +	if (!yamlMeteringModes.isDictionary()) {
> +		LOG(RkISP1Agc, Error)
> +			<< "'" << prop << "' parameter not found in tuning file";
> +		return -EINVAL;
> +	}
> +
> +	for (const auto &[key, value] : yamlMeteringModes.asDict()) {
> +		if (controls::AeMeteringModeNameValueMap.find(key) ==
> +		    controls::AeMeteringModeNameValueMap.end()) {
> +			LOG(RkISP1Agc, Warning)
> +				<< "Skipping unknown metering mode '" << key << "'";
> +			continue;
> +		}
> +
> +		for (const auto &[version, matrix] : value.asDict()) {
> +			std::vector<uint8_t> weights =
> +				matrix.getList<uint8_t>().value_or(std::vector<uint8_t>{});
> +			if (weights.size() != context.hw->numHistogramWeights)
> +				continue;

Worth a debug printout imo - took me a little while to figure out that this was skipping arrays of 
weights for the other hardware revision.


With those: Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>

> +
> +			LOG(RkISP1Agc, Debug)
> +				<< "Matched metering matrix mode "
> +				<< key << ", version " << version;
> +
> +			meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
> +		}
> +	}
> +
> +	if (meteringModes_.empty()) {
> +		int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
> +		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
> +
> +		meteringModes_[meteringModeId] = weights;
> +	}
> +
> +	return 0;
> +}
> +
> +uint8_t Agc::predivider(Size &size)
> +{
> +	/*
> +	 * The maximum number of pixels that could potentially be in one bin is
> +	 * if all the pixels of the image are in it, multiplied by 3 for the
> +	 * three color channels. The counter for each bin is 16 bits wide, so
> +	 * `factor` thus contains the number of times we'd wrap around. This is
> +	 * obviously the number of pixels that we need to skip to make sure
> +	 * that we don't wrap around, but we compute the square root of it
> +	 * instead, as the skip that we need to program is for both the x and y
> +	 * directions.
> +	 *
> +	 * There's a bit of extra rounding math to make sure the rounding goes
> +	 * the correct direction so that the square of the step is big enough
> +	 * to encompass the `factor` number of pixels that we need to skip.
> +	 */
> +	double factor = size.width * size.height * 3 / 65535.0;
> +	double root = std::sqrt(factor);
> +	uint8_t ret;
> +
> +	if (std::pow(std::floor(root), 2) + 0.01 < factor)
> +		ret = static_cast<uint8_t>(std::ceil(root));
> +	else
> +		ret = static_cast<uint8_t>(std::floor(root));
> +
> +	return std::clamp<uint8_t>(ret, 3, 127);
> +}
> +
>   Agc::Agc()
>   {
>   	supportsRaw_ = true;
> @@ -59,6 +131,10 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>   	if (ret)
>   		return ret;
>   
> +	ret = parseMeteringModes(context, tuningData, "AeMeteringMode");
> +	if (ret)
> +		return ret;
> +
>   	context.ctrlMap.merge(controls());
>   
>   	return 0;
> @@ -160,6 +236,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>   		frameContext.agc.gain = context.activeState.agc.automatic.gain;
>   	}
>   
> +	/* \todo Remove this when we can set the below with controls */
>   	if (frame > 0)
>   		return;
>   
> @@ -178,14 +255,25 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>   	params->meas.hst_config.meas_window = context.configuration.agc.measureWindow;
>   	/* Produce the luminance histogram. */
>   	params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;
> +
>   	/* Set an average weighted histogram. */
>   	Span<uint8_t> weights{
>   		params->meas.hst_config.hist_weight,
>   		context.hw->numHistogramWeights
>   	};
> -	std::fill(weights.begin(), weights.end(), 1);
> -	/* Step size can't be less than 3. */
> -	params->meas.hst_config.histogram_predivider = 4;
> +	/* \todo Get this from control */
> +	std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix);
> +	std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
> +
> +	std::stringstream str;
> +	str << "Histogram weights : ";
> +	for (size_t i = 0; i < context.hw->numHistogramWeights; i++)
> +		str << (int)params->meas.hst_config.hist_weight[i] << " ";
> +	LOG(RkISP1Agc, Debug) << str.str();
> +
> +	/* \todo Add a control for this? */
> +	params->meas.hst_config.histogram_predivider =
> +		predivider(context.configuration.sensor.size);
>   
>   	/* Update the configuration for histogram. */
>   	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST;
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index f2f5b59d0..77d944237 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -44,11 +44,17 @@ public:
>   		     ControlList &metadata) override;
>   
>   private:
> +	int parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
> +			       const char *prop);
> +	uint8_t predivider(Size &size);
> +
>   	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>   			  ControlList &metadata);
>   	double estimateLuminance(double gain) const override;
>   
>   	Span<const uint8_t> expMeans_;
> +
> +	std::map<int32_t, std::vector<uint8_t>> meteringModes_;
>   };
>   
>   } /* namespace ipa::rkisp1::algorithms */