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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jun 07, 2024 at 05:03:28PM +0900, 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>
> Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
> 
> ---
> No change in v5
> 
> Changes in v4:
> - add debug print when parsing the yaml file
> 
> 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 50e0690fe..3bbafd978 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)
> +{
> +	const YamlObject &yamlMeteringModes = tuningData[prop];

I think this belongs to the caller.

> +	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()) {

What is the version ? It isn't used below except in a debugging message.

More generally, where do we document the format of the tuning data ? If
the answer is '\todo' then could we have at least one example yaml file
in the source tree that includes all tuning blocks ? This can be done on
top of this series, but please sooner than later.

> +			std::vector<uint8_t> weights =
> +				matrix.getList<uint8_t>().value_or(std::vector<uint8_t>{});
> +			if (weights.size() != context.hw->numHistogramWeights)
> +				continue;

I'd be very vocal about this, or even fail parsing completely.

> +
> +			LOG(RkISP1Agc, Debug)
> +				<< "Matched metering matrix mode "
> +				<< key << ", version " << version;
> +
> +			meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;

If there are multiple "versions", you'll overwrite the metering modes.
Is that really intended ?

> +		}
> +	}
> +
> +	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)

Agc::computeHistogramPredivider()

(or s/compute/calculate/)

> +{
> +	/*
> +	 * 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

I think you should pass the histogram mode to this function, to pick the
right multiplier. We currently hardcode usage of the Y histogram, so the
*3 factor below will result in higher predividers and loss of accuracy.

I think we should also take the weights into account.

> +	 * 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;

s/ret/predivider/

> +
> +	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));

I have trouble validating the math here. Let's assume we have an image
size of 760x460 pixels. This leads to

	factor = 760 * 460* 3 / 65535.0 = 16.003662165255207

(I'm using Python to do the math, I don't think the differences in
rounding with C++ double, if any, will make a difference here)

	root = 4.000457744465652
	std::pow(std::floor(root), 2) + 0.01 = 16.01

This is higher than factor, so

	predivider = static_cast<uint8_t>(std::floor(root)) = 4

Skipping by a factor of 4 in both directions gives us 190x115 pixels,
and multiplied by 3, that's 65550, which overflows. It may not be the
biggest deal, as the hardware clamps the histogram values instead of
rolling over, and the risk of *all* pixels being in the same bin is
virtually 0, but it's still not correct.

> +
> +	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 */

Patch 3/3 enables setting the metering mode through controls, but
doesn't remove this. Is that intended ? Note that we should reprogram
the histogram engine only in the case where the metering mode has
actually changed, we shouldn't do it on every frame.

>  	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();

You will construct the string unconditionally, which is not very nice.
I'd drop this. If you want to log the weight I would do it at init()
time, and only log the metering mode here.

> +
> +	/* \todo Add a control for this? */

I don't think that's needed, but I may be missing something. What would
be the rationale ? It should be captured in the todo comment if we think
it can be useful.

> +	params->meas.hst_config.histogram_predivider =
> +		predivider(context.configuration.sensor.size);

Shouldn't you pass the measurement window size here, not the 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 04b3247e1..4ab56ead5 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 */

-- 
Regards,

Laurent Pinchart