[libcamera-devel] [PATCH v4 2/5] ipa: rpi: agc: Reorganise code for multi-channel AGC

[Kieran Bingham]

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 &region = 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);
> +               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;
> +
> +       return ySum / pixelSum / (1 << 16);
> +}
> +