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

[Paul Elder]

On Tue, Jun 11, 2024 at 07:08:18PM +0300, Laurent Pinchart wrote:
> 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.

It's "v10" or "v12". It is indeed not used because...

> 
> 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.

(there's a schema patch from Stefan)

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

...this is the "real" version checker.

> > +				continue;
> 
> I'd be very vocal about this, or even fail parsing completely.

The tuning file will have something like:
- v10: <v10 metering mode>
- v12: <v12 metering mode>

So it'll skip the non-matching version and only keep the matching. We
only have to be vocal if none are found... in which case a default one
is used. So I guess I should add a message when using a default one
below then.

> 
> > +
> > +			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 there are multiple version then they'd get skipped because of the
above "real" version checker.

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

Ah, indeed.

> 
> I think we should also take the weights into account.

How so? The documentation doesn't mention having to do so.

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

Turns out I didn't need the +0.01.


Paul

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