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

[Laurent Pinchart]

On Mon, Apr 15, 2024 at 03:22:51PM +0200, Stefan Klug wrote:
> Hi Paul,
> 
> thanks for the patch.
> 
> On Fri, Apr 05, 2024 at 11:47:25PM +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>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 80 +++++++++++++++++++++++++++++--
> >  src/ipa/rkisp1/algorithms/agc.h   |  6 +++
> >  2 files changed, 83 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 1cd977a0..fd47ba4e 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"
> >  
> >  /**
> > @@ -42,6 +44,62 @@ static constexpr double kMinAnalogueGain = 1.0;
> >  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> >  static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> >  
> > +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
> > +			    const char *prop)
> > +{
> > +	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;
> > +		}
> > +
> > +		std::vector<uint8_t> weights =
> > +			value.getList<uint8_t>().value_or(std::vector<uint8_t>{});
> > +		if (weights.size() != context.hw->numHistogramWeights) {
> > +			LOG(RkISP1Agc, Error)
> > +				<< "Invalid '" << key << "' values: expected "
> > +				<< context.hw->numHistogramWeights
> > +				<< " elements, got " << weights.size();
> > +			return -ENODATA;
> > +		}
> > +
> > +		meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
> > +	}
> 
> Kieran already mentioned that. What about adding the matrix metering
> mode (+ warning) in case meteringModes_ is still empty.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +uint8_t Agc::predivider(Size &size)
> > +{
> > +	static const std::vector<std::pair<unsigned int, unsigned int>>
> > +		pixelCountThresholds = {
> > +			{  640 *  480,  3 },
> > +			{  800 *  600,  4 },
> > +			{ 1024 *  768,  5 },
> > +			{ 1280 *  960,  6 },
> > +			{ 1600 * 1200,  8 },
> > +			{ 2048 * 1536, 10 },
> > +			{ 2600 * 1950, 12 },
> > +			{ 4096 * 3072, 16 },
> > +		};
> 
> Where do these values come from? What would happen if we keep the predivider
> of 4?

The predivider is meant to avoid overflows in the worst case situation.
I'd rather compute them in code instead of using a table.

> > +
> > +	unsigned long pixels = size.width * size.height;
> > +	for (auto &pair : pixelCountThresholds)
> > +		if (pixels < pair.first)
> > +			return pair.second;
> > +
> > +	return 24;
> > +}
> > +
> >  Agc::Agc()
> >  {
> >  	supportsRaw_ = true;
> > @@ -72,6 +130,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;
> > @@ -177,6 +239,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;
> >  
> > @@ -195,14 +258,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 311d4e94..43e3d5b2 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -44,6 +44,10 @@ 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);
> >  	void parseStatistics(const rkisp1_stat_buffer *stats,
> > @@ -52,6 +56,8 @@ private:
> >  
> >  	Histogram hist_;
> >  	Span<const uint8_t> expMeans_;
> > +
> > +	std::map<int32_t, std::vector<uint8_t>> meteringModes_;
> >  };
> >  
> >  } /* namespace ipa::rkisp1::algorithms */

-- 
Regards,

Laurent Pinchart