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

20230 diff mbox series

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@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@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(-)

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@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@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 */

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@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@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 */

On Wed, Jun 12, 2024 at 07:50:45PM +0900, Paul Elder wrote:
> On Tue, Jun 11, 2024 at 07:08:18PM +0300, Laurent Pinchart wrote:
> > 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@ideasonboard.com>
> > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > Reviewed-by: Daniel Scally <dan.scally@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)

One step in the right direction :-)

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

I don't like this much. Could we have a single table that we adapt
between the versions ?

On a more general note, we will have algorithms that will differ on
i.MX8MP compared to v10 and v12, as the ISP8000Nano has additional
hardware blocks. I think this will require different tuning files on
different platforms.

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

The weights are specified as an integer in the [0, 16] range, which maps
to [0.0, 1.0] scaling factors. The histogram accumulates weighted
pixels. If all zones use a 1/16 weight for instance, then the histogram
values will effectively be divided by 16, which influences the need for
a predivider.

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

If you drop the +0.01, can the condition ever be false ?

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