[PATCH v5 1/3] ipa: rkisp1: agc: Read histogram weights from tuning file
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 12 13:01:57 CEST 2024
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 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)
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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list