[libcamera-devel] [PATCH v4 2/5] ipa: rpi: agc: Reorganise code for multi-channel AGC
David Plowman
david.plowman at raspberrypi.com
Fri Sep 15 16:39:07 CEST 2023
Hi Kieran
Yes, I was just looking at that when your 2nd email arrived. It's a
bug, exactly as you describe it!
I'll make the fix and give it some testing, though I can't believe it
will do anything bad!
David
On Fri, 15 Sept 2023 at 15:36, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Kieran Bingham (2023-09-15 15:29:35)
> > Quoting David Plowman via libcamera-devel (2023-09-15 13:29:51)
> > > This commit does the basic reorganisation of the code in order to
> > > implement multi-channel AGC. The main changes are:
> > >
> > > * The previous Agc class (in agc.cpp) has become the AgcChannel class
> > > in (agc_channel.cpp).
> > >
> > > * A new Agc class is introduced which is a wrapper round a number of
> > > AgcChannels.
> > >
> > > * The basic plumbing from ipa_base.cpp to Agc is updated to include a
> > > channel number. All the existing controls are hardwired to talk
> > > directly to channel 0.
> > >
> > > There are a couple of limitations which we expect to apply to
> > > multi-channel AGC. We're not allowing different frame durations to be
> > > applied to the channels, nor are we allowing separate metering
> > > modes. To be fair, supporting these things is not impossible, but
> > > there are reasons why it may be tricky so they remain "TBD" for now.
> > >
> > > This patch only includes the basic reorganisation and plumbing. It
> > > does not yet update the important methods (switchMode, prepare and
> > > process) to implement multi-channel AGC properly. This will appear in
> > > a subsequent commit. For now, these functions are hard-coded just to
> > > use channel 0, thereby preserving the existing behaviour.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > ---
> >
> > <snippity>
> >
> > > +static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
> > > + std::vector<double> &weights, double gain)
> > > +{
> > > + constexpr uint64_t maxVal = 1 << Statistics::NormalisationFactorPow2;
> > > +
> > > + /*
> > > + * If we have no AGC region stats, but do have a a Y histogram, use that
> > > + * directly to caluclate the mean Y value of the image.
> > > + */
> > > + if (!stats->agcRegions.numRegions() && stats->yHist.bins()) {
> > > + /*
> > > + * When the gain is applied to the histogram, anything below minBin
> > > + * will scale up directly with the gain, but anything above that
> > > + * will saturate into the top bin.
> > > + */
> > > + auto &hist = stats->yHist;
> > > + double minBin = std::min(1.0, 1.0 / gain) * hist.bins();
> > > + double binMean = hist.interBinMean(0.0, minBin);
> > > + double numUnsaturated = hist.cumulativeFreq(minBin);
> > > + /* This term is from all the pixels that won't saturate. */
> > > + double ySum = binMean * gain * numUnsaturated;
> > > + /* And add the ones that will saturate. */
> > > + ySum += (hist.total() - numUnsaturated) * hist.bins();
> > > + return ySum / hist.total() / hist.bins();
> > > + }
> > > +
> > > + ASSERT(weights.size() == stats->agcRegions.numRegions());
> > > +
> > > + /*
> > > + * Note that the weights are applied by the IPA to the statistics directly,
> > > + * before they are given to us here.
> > > + */
> > > + double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;
> >
> > FAILED: src/ipa/rpi/controller/librpi_ipa_controller.a.p/rpi_agc_channel.cpp.o
> > clang++-13 -Isrc/ipa/rpi/controller/librpi_ipa_controller.a.p -Isrc/ipa/rpi/controller -I../../../src/libcamera/src/ipa/rpi/controller -Iinclude -I../../../src/libcamera/include -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -O0 -g -Wextra-semi -Wthread-safety -Wshadow -include /home/kbingham/iob/libcamera/ci/integrator/builds/build-matrix/clang-13/config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/rpi/controller/librpi_ipa_controller.a.p/rpi_agc_channel.cpp.o -MF src/ipa/rpi/controller/librpi_ipa_controller.a.p/rpi_agc_channel.cpp.o.d -o src/ipa/rpi/controller/librpi_ipa_controller.a.p/rpi_agc_channel.cpp.o -c ../../../src/libcamera/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > ../../../src/libcamera/src/ipa/rpi/controller/rpi/agc_channel.cpp:675:29: error: variable 'bSum' set but not used [-Werror,-Wunused-but-set-variable]
> > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;
> > ^
> > 1 error generated.
> >
> > I think we can just remove this bSum = 0, local var.
> >
> > Likely no need for a resend.
> >
> >
> > > + for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {
> > > + auto ®ion = stats->agcRegions.get(i);
> > > + rSum += std::min<double>(region.val.rSum * gain, (maxVal - 1) * region.counted);
> > > + gSum += std::min<double>(region.val.gSum * gain, (maxVal - 1) * region.counted);
> > > + bSum += std::min<double>(region.val.bSum * gain, (maxVal - 1) * region.counted);
>
> David,
>
> We accumulate the bSum here - but don't then use it. Do you foresee
> needing this bSum anywhere else? or is this safe to drop?
>
>
> > > + pixelSum += region.counted;
> > > + }
> > > + if (pixelSum == 0.0) {
> > > + LOG(RPiAgc, Warning) << "computeInitialY: pixelSum is zero";
> > > + return 0;
> > > + }
> > > +
> > > + double ySum;
> > > + /* Factor in the AWB correction if needed. */
> > > + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {
> > > + ySum = rSum * awb.gainR * .299 +
> > > + gSum * awb.gainG * .587 +
> > > + gSum * awb.gainB * .114;
> > > + } else
> > > + ySum = rSum * .299 + gSum * .587 + gSum * .114;
>
> Or ... is the fault here, that it should be bSum * .114 ? (Looks quite
> likely given the block above it).
>
>
> --
> Kieran
>
>
> > > +
> > > + return ySum / pixelSum / (1 << 16);
> > > +}
> > > +
More information about the libcamera-devel
mailing list