[libcamera-devel,v3,4/5] ipa: raspberrypi: Use the generic statistics structure in the algorithms

17999 diff mbox series

Repurpose the StatisticsPtr type from being a shared_ptr<bcm2835_isp_stats> to
shared_ptr<RPiController::Statistics>. This removes any hardware specific header
files and structures from the algorithms source code.

Add a new function in the Raspberry Pi IPA to populate the generic statistics
structure from the values provided by the hardware in the bcm2835_isp_stats
structure.

Update the Lux, AWB, AGC, ALSC, Contrast, and Focus algorithms to use the
generic statistics structure appropriately in their calculations. Additionally,
remove references to any hardware specific headers and defines in these source
files.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/controller.h   |  4 +-
 src/ipa/raspberrypi/controller/rpi/agc.cpp    | 27 +++++-------
 src/ipa/raspberrypi/controller/rpi/agc.h      |  2 +-
 src/ipa/raspberrypi/controller/rpi/alsc.cpp   | 32 +++++++-------
 src/ipa/raspberrypi/controller/rpi/alsc.h     |  3 +-
 src/ipa/raspberrypi/controller/rpi/awb.cpp    | 20 ++++-----
 src/ipa/raspberrypi/controller/rpi/awb.h      |  1 +
 .../raspberrypi/controller/rpi/contrast.cpp   |  8 ++--
 src/ipa/raspberrypi/controller/rpi/focus.cpp  |  7 ++-
 src/ipa/raspberrypi/controller/rpi/lux.cpp    | 14 +-----
 src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++++++++++-
 src/ipa/raspberrypi/statistics.h              |  2 +
 12 files changed, 96 insertions(+), 68 deletions(-)

Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:35)
> Repurpose the StatisticsPtr type from being a shared_ptr<bcm2835_isp_stats> to
> shared_ptr<RPiController::Statistics>. This removes any hardware specific header
> files and structures from the algorithms source code.
> 
> Add a new function in the Raspberry Pi IPA to populate the generic statistics
> structure from the values provided by the hardware in the bcm2835_isp_stats
> structure.
> 
> Update the Lux, AWB, AGC, ALSC, Contrast, and Focus algorithms to use the
> generic statistics structure appropriately in their calculations. Additionally,
> remove references to any hardware specific headers and defines in these source
> files.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/ipa/raspberrypi/controller/controller.h   |  4 +-
>  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 27 +++++-------
>  src/ipa/raspberrypi/controller/rpi/agc.h      |  2 +-
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp   | 32 +++++++-------
>  src/ipa/raspberrypi/controller/rpi/alsc.h     |  3 +-
>  src/ipa/raspberrypi/controller/rpi/awb.cpp    | 20 ++++-----
>  src/ipa/raspberrypi/controller/rpi/awb.h      |  1 +
>  .../raspberrypi/controller/rpi/contrast.cpp   |  8 ++--
>  src/ipa/raspberrypi/controller/rpi/focus.cpp  |  7 ++-
>  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 14 +-----
>  src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++++++++++-
>  src/ipa/raspberrypi/statistics.h              |  2 +
>  12 files changed, 96 insertions(+), 68 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/controller.h b/src/ipa/raspberrypi/controller/controller.h
> index 3e1e051703b3..e6c950c3a509 100644
> --- a/src/ipa/raspberrypi/controller/controller.h
> +++ b/src/ipa/raspberrypi/controller/controller.h
> @@ -15,19 +15,17 @@
>  #include <vector>
>  #include <string>
>  
> -#include <linux/bcm2835-isp.h>
> -
>  #include "libcamera/internal/yaml_parser.h"
>  
>  #include "camera_mode.h"
>  #include "device_status.h"
>  #include "metadata.h"
> +#include "statistics.h"
>  
>  namespace RPiController {
>  
>  class Algorithm;
>  typedef std::unique_ptr<Algorithm> AlgorithmPtr;
> -typedef std::shared_ptr<bcm2835_isp_stats> StatisticsPtr;
>  
>  /*
>   * The Controller holds a pointer to some global_metadata, which is how
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 46dcc81ae14c..868c30f03d66 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -9,8 +9,6 @@
>  #include <map>
>  #include <tuple>
>  
> -#include <linux/bcm2835-isp.h>
> -
>  #include <libcamera/base/log.h>
>  
>  #include "../awb_status.h"
> @@ -451,7 +449,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
>         fetchCurrentExposure(imageMetadata);
>         /* Compute the total gain we require relative to the current exposure. */
>         double gain, targetY;
> -       computeGain(stats.get(), imageMetadata, gain, targetY);
> +       computeGain(stats, imageMetadata, gain, targetY);
>         /* Now compute the target (final) exposure which we think we want. */
>         computeTargetExposure(gain);
>         /*
> @@ -585,24 +583,23 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)
>                 LOG(RPiAgc, Debug) << "No AWB status found";
>  }
>  
> -static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,
> +static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
>                               double weights[], double gain)
>  {
> -       bcm2835_isp_stats_region *regions = stats->agc_stats;
>         /*
>          * Note how the calculation below means that equal weights give you
>          * "average" metering (i.e. all pixels equally important).
>          */
>         double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;
> -       for (unsigned int i = 0; i < AgcStatsSize; i++) {
> -               double counted = regions[i].counted;
> -               double rAcc = std::min(regions[i].r_sum * gain, ((1 << PipelineBits) - 1) * counted);
> -               double gAcc = std::min(regions[i].g_sum * gain, ((1 << PipelineBits) - 1) * counted);
> -               double bAcc = std::min(regions[i].b_sum * gain, ((1 << PipelineBits) - 1) * counted);
> +       for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {
> +               auto &region = stats->agcRegions.get(i);
> +               double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);
> +               double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);
> +               double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);
>                 rSum += rAcc * weights[i];
>                 gSum += gAcc * weights[i];
>                 bSum += bAcc * weights[i];
> -               pixelSum += counted * weights[i];
> +               pixelSum += region.counted * weights[i];
>         }
>         if (pixelSum == 0.0) {
>                 LOG(RPiAgc, Warning) << "computeInitialY: pixelSum is zero";
> @@ -624,23 +621,23 @@ static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,
>  
>  static constexpr double EvGainYTargetLimit = 0.9;
>  
> -static double constraintComputeGain(AgcConstraint &c, Histogram &h, double lux,
> +static double constraintComputeGain(AgcConstraint &c, const Histogram &h, double lux,
>                                     double evGain, double &targetY)
>  {
>         targetY = c.yTarget.eval(c.yTarget.domain().clip(lux));
>         targetY = std::min(EvGainYTargetLimit, targetY * evGain);
>         double iqm = h.interQuantileMean(c.qLo, c.qHi);
> -       return (targetY * NUM_HISTOGRAM_BINS) / iqm;
> +       return (targetY * h.bins()) / iqm;
>  }
>  
> -void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,
> +void Agc::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,
>                       double &gain, double &targetY)
>  {
>         struct LuxStatus lux = {};
>         lux.lux = 400; /* default lux level to 400 in case no metadata found */
>         if (imageMetadata->get("lux.status", lux) != 0)
>                 LOG(RPiAgc, Warning) << "No lux level found";
> -       Histogram h(statistics->hist[0].g_hist, NUM_HISTOGRAM_BINS);
> +       const Histogram &h = statistics->yHist;
>         double evGain = status_.ev * config_.baseEv;
>         /*
>          * The initial gain and target_Y come from some of the regions. After
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h
> index cf04da1973f1..f04896ca25ad 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.h
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h
> @@ -96,7 +96,7 @@ private:
>         void housekeepConfig();
>         void fetchCurrentExposure(Metadata *imageMetadata);
>         void fetchAwbStatus(Metadata *imageMetadata);
> -       void computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,
> +       void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,
>                          double &gain, double &targetY);
>         void computeTargetExposure(double gain);
>         bool applyDigitalGain(double gain, double targetY);
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index a4afaf841c41..eb4e2f9496e1 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -310,19 +310,21 @@ double getCt(Metadata *metadata, double defaultCt)
>         return awbStatus.temperatureK;
>  }
>  
> -static void copyStats(bcm2835_isp_stats_region regions[XY], StatisticsPtr &stats,
> +static void copyStats(RgbyRegions &regions, StatisticsPtr &stats,
>                       AlscStatus const &status)
>  {
> -       bcm2835_isp_stats_region *inputRegions = stats->awb_stats;
> +       if (!regions.numRegions())
> +               regions.init(stats->awbRegions.size());
> +
>         double *rTable = (double *)status.r;
>         double *gTable = (double *)status.g;
>         double *bTable = (double *)status.b;
> -       for (int i = 0; i < XY; i++) {
> -               regions[i].r_sum = inputRegions[i].r_sum / rTable[i];
> -               regions[i].g_sum = inputRegions[i].g_sum / gTable[i];
> -               regions[i].b_sum = inputRegions[i].b_sum / bTable[i];
> -               regions[i].counted = inputRegions[i].counted;
> -               /* (don't care about the uncounted value) */
> +       for (unsigned int i = 0; i < stats->awbRegions.numRegions(); i++) {
> +               auto r = stats->awbRegions.get(i);
> +               r.val.rSum = static_cast<uint64_t>(r.val.rSum / rTable[i]);
> +               r.val.gSum = static_cast<uint64_t>(r.val.gSum / gTable[i]);
> +               r.val.bSum = static_cast<uint64_t>(r.val.bSum / bTable[i]);
> +               regions.set(i, r);
>         }
>  }
>  
> @@ -512,19 +514,19 @@ void resampleCalTable(double const calTableIn[XY],
>  }
>  
>  /* Calculate chrominance statistics (R/G and B/G) for each region. */
> -static_assert(XY == AWB_REGIONS, "ALSC/AWB statistics region mismatch");
> -static void calculateCrCb(bcm2835_isp_stats_region *awbRegion, double cr[XY],
> +static void calculateCrCb(const RgbyRegions &awbRegion, double cr[XY],
>                           double cb[XY], uint32_t minCount, uint16_t minG)
>  {
>         for (int i = 0; i < XY; i++) {
> -               bcm2835_isp_stats_region &zone = awbRegion[i];
> -               if (zone.counted <= minCount ||
> -                   zone.g_sum / zone.counted <= minG) {
> +               auto s = awbRegion.get(i);
> +
> +               if (s.counted <= minCount || s.val.gSum / s.counted <= minG) {
>                         cr[i] = cb[i] = InsufficientData;
>                         continue;
>                 }
> -               cr[i] = zone.r_sum / (double)zone.g_sum;
> -               cb[i] = zone.b_sum / (double)zone.g_sum;
> +
> +               cr[i] = s.val.rSum / (double)s.val.gSum;
> +               cb[i] = s.val.bSum / (double)s.val.gSum;
>         }
>  }
>  
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h b/src/ipa/raspberrypi/controller/rpi/alsc.h
> index a858ef5a6551..9167c9ffa2e3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.h
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h
> @@ -12,6 +12,7 @@
>  
>  #include "../algorithm.h"
>  #include "../alsc_status.h"
> +#include "../statistics.h"
>  
>  namespace RPiController {
>  
> @@ -98,7 +99,7 @@ private:
>         /* copy out the results from the async thread so that it can be restarted */
>         void fetchAsyncResults();
>         double ct_;
> -       bcm2835_isp_stats_region statistics_[AlscCellsY * AlscCellsX];
> +       RgbyRegions statistics_;
>         double asyncResults_[3][AlscCellsY][AlscCellsX];
>         double asyncLambdaR_[AlscCellsX * AlscCellsY];
>         double asyncLambdaB_[AlscCellsX * AlscCellsY];
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> index 04d1c8783654..ef3435d66106 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> @@ -21,9 +21,6 @@ LOG_DEFINE_CATEGORY(RPiAwb)
>  
>  #define NAME "rpi.awb"
>  
> -static constexpr unsigned int AwbStatsSizeX = DEFAULT_AWB_REGIONS_X;
> -static constexpr unsigned int AwbStatsSizeY = DEFAULT_AWB_REGIONS_Y;
> -
>  /*
>   * todo - the locking in this algorithm needs some tidying up as has been done
>   * elsewhere (ALSC and AGC).
> @@ -401,17 +398,16 @@ void Awb::asyncFunc()
>  }
>  
>  static void generateStats(std::vector<Awb::RGB> &zones,
> -                         bcm2835_isp_stats_region *stats, double minPixels,
> +                         RgbyRegions &stats, double minPixels,
>                           double minG)
>  {
> -       for (unsigned int i = 0; i < AwbStatsSizeX * AwbStatsSizeY; i++) {
> +       for (auto const &region : stats) {
>                 Awb::RGB zone;
> -               double counted = stats[i].counted;
> -               if (counted >= minPixels) {
> -                       zone.G = stats[i].g_sum / counted;
> +               if (region.counted >= minPixels) {
> +                       zone.G = region.val.gSum / region.counted;
>                         if (zone.G >= minG) {
> -                               zone.R = stats[i].r_sum / counted;
> -                               zone.B = stats[i].b_sum / counted;
> +                               zone.R = region.val.rSum / region.counted;
> +                               zone.B = region.val.bSum / region.counted;
>                                 zones.push_back(zone);
>                         }
>                 }
> @@ -425,7 +421,7 @@ void Awb::prepareStats()
>          * LSC has already been applied to the stats in this pipeline, so stop
>          * any LSC compensation.  We also ignore config_.fast in this version.
>          */
> -       generateStats(zones_, statistics_->awb_stats, config_.minPixels,
> +       generateStats(zones_, statistics_->awbRegions, config_.minPixels,
>                       config_.minG);
>         /*
>          * apply sensitivities, so values appear to come from our "canonical"
> @@ -641,7 +637,7 @@ void Awb::awbBayes()
>          * valid... not entirely sure about this.
>          */
>         Pwl prior = interpolatePrior();
> -       prior *= zones_.size() / (double)(AwbStatsSizeX * AwbStatsSizeY);
> +       prior *= zones_.size() / (double)(statistics_->awbRegions.numRegions());
>         prior.map([](double x, double y) {
>                 LOG(RPiAwb, Debug) << "(" << x << "," << y << ")";
>         });
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h b/src/ipa/raspberrypi/controller/rpi/awb.h
> index 2254c3ed2cc4..e7d49cd8036b 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.h
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.h
> @@ -13,6 +13,7 @@
>  #include "../awb_algorithm.h"
>  #include "../pwl.h"
>  #include "../awb_status.h"
> +#include "../statistics.h"
>  
>  namespace RPiController {
>  
> diff --git a/src/ipa/raspberrypi/controller/rpi/contrast.cpp b/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> index 5b37edcbd46a..a4f8c4f04fc4 100644
> --- a/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> @@ -106,7 +106,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
>          * bit.
>          */
>         double histLo = histogram.quantile(config.loHistogram) *
> -                       (65536 / NUM_HISTOGRAM_BINS);
> +                       (65536 / histogram.bins());
>         double levelLo = config.loLevel * 65536;
>         LOG(RPiContrast, Debug)
>                 << "Move histogram point " << histLo << " to " << levelLo;
> @@ -119,7 +119,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
>          * Keep the mid-point (median) in the same place, though, to limit the
>          * apparent amount of global brightness shift.
>          */
> -       double mid = histogram.quantile(0.5) * (65536 / NUM_HISTOGRAM_BINS);
> +       double mid = histogram.quantile(0.5) * (65536 / histogram.bins());
>         enhance.append(mid, mid);
>  
>         /*
> @@ -127,7 +127,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
>          * there up.
>          */
>         double histHi = histogram.quantile(config.hiHistogram) *
> -                       (65536 / NUM_HISTOGRAM_BINS);
> +                       (65536 / histogram.bins());
>         double levelHi = config.hiLevel * 65536;
>         LOG(RPiContrast, Debug)
>                 << "Move histogram point " << histHi << " to " << levelHi;
> @@ -158,7 +158,7 @@ Pwl applyManualContrast(Pwl const &gammaCurve, double brightness,
>  void Contrast::process(StatisticsPtr &stats,
>                        [[maybe_unused]] Metadata *imageMetadata)
>  {
> -       Histogram histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);
> +       Histogram &histogram = stats->yHist;
>         /*
>          * We look at the histogram and adjust the gamma curve in the following
>          * ways: 1. Adjust the gamma curve so as to pull the start of the
> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> index 8c5029bd0e95..ea3cc00e42c3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> @@ -31,10 +31,9 @@ char const *Focus::name() const
>  void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)
>  {
>         FocusStatus status;
> -       unsigned int i;
> -       for (i = 0; i < FOCUS_REGIONS; i++)
> -               status.focusMeasures[i] = stats->focus_stats[i].contrast_val[1][1] / 1000;
> -       status.num = i;
> +       for (unsigned int i = 0; i < stats->focusRegions.numRegions(); i++)
> +               status.focusMeasures[i] = stats->focusRegions.get(i).val;
> +       status.num = stats->focusRegions.numRegions();
>         imageMetadata->set("focus.status", status);
>  
>         LOG(RPiFocus, Debug)
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> index 9759186afacf..06625f3a5ea3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -6,8 +6,6 @@
>   */
>  #include <math.h>
>  
> -#include <linux/bcm2835-isp.h>
> -
>  #include <libcamera/base/log.h>
>  
>  #include "../device_status.h"
> @@ -83,20 +81,12 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)
>         if (imageMetadata->get("device.status", deviceStatus) == 0) {
>                 double currentGain = deviceStatus.analogueGain;
>                 double currentAperture = deviceStatus.aperture.value_or(currentAperture_);
> -               uint64_t sum = 0;
> -               uint32_t num = 0;
> -               uint32_t *bin = stats->hist[0].g_hist;
> -               const int numBins = sizeof(stats->hist[0].g_hist) /
> -                                   sizeof(stats->hist[0].g_hist[0]);
> -               for (int i = 0; i < numBins; i++)
> -                       sum += bin[i] * (uint64_t)i, num += bin[i];
> -               /* add .5 to reflect the mid-points of bins */
> -               double currentY = sum / (double)num + .5;
> +               double currentY = stats->yHist.interQuantileMean(0, 1);
>                 double gainRatio = referenceGain_ / currentGain;
>                 double shutterSpeedRatio =
>                         referenceShutterSpeed_ / deviceStatus.shutterSpeed;
>                 double apertureRatio = referenceAperture_ / currentAperture;
> -               double yRatio = currentY * (65536 / numBins) / referenceY_;
> +               double yRatio = currentY * (65536 / stats->yHist.bins()) / referenceY_;
>                 double estimatedLux = shutterSpeedRatio * gainRatio *
>                                       apertureRatio * apertureRatio *
>                                       yRatio * referenceLux_;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index bead436def3c..cff079bbafb3 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -51,6 +51,7 @@
>  #include "metadata.h"
>  #include "sharpen_algorithm.h"
>  #include "sharpen_status.h"
> +#include "statistics.h"
>  
>  namespace libcamera {
>  
> @@ -139,6 +140,7 @@ private:
>         void prepareISP(const ISPConfig &data);
>         void reportMetadata(unsigned int ipaContext);
>         void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> +       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
>         void processStats(unsigned int bufferId, unsigned int ipaContext);
>         void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
>         void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> @@ -1141,6 +1143,46 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls, unsigned int ip
>         rpiMetadata_[ipaContext].set("device.status", deviceStatus);
>  }
>  
> +RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) const
> +{
> +       using namespace RPiController;
> +
> +       unsigned int i;
> +       StatisticsPtr statistics =
> +               std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc);
> +
> +       /* RGB histograms are not used, so do not populate them. */
> +       statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));
> +
> +       statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });
> +       for (i = 0; i < statistics->awbRegions.numRegions(); i++)
> +               statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,
> +                                                 stats->awb_stats[i].g_sum,
> +                                                 stats->awb_stats[i].b_sum },
> +                                               stats->awb_stats[i].counted,
> +                                               stats->awb_stats[i].notcounted });
> +
> +       /*
> +        * There are only ever 15 regions computed by the firmware due to zoning,
> +        * but the HW defines AGC_REGIONS == 16!
> +        */
> +       statistics->agcRegions.init(15);
> +       for (i = 0; i < statistics->agcRegions.numRegions(); i++)
> +               statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,
> +                                                 stats->agc_stats[i].g_sum,
> +                                                 stats->agc_stats[i].b_sum },
> +                                               stats->agc_stats[i].counted,
> +                                               stats->awb_stats[i].notcounted });
> +
> +       statistics->focusRegions.init({ 4, 3 });
> +       for (i = 0; i < statistics->focusRegions.numRegions(); i++)
> +               statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,
> +                                                 stats->focus_stats[i].contrast_val_num[1][1],
> +                                                 stats->focus_stats[i].contrast_val_num[1][0] });
> +
> +       return statistics;
> +}
> +
>  void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
>  {
>         RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> @@ -1153,7 +1195,7 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
>  
>         Span<uint8_t> mem = it->second.planes()[0];
>         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());
> -       RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);
> +       RPiController::StatisticsPtr statistics = fillStatistics(stats);
>         helper_->process(statistics, rpiMetadata);
>         controller_.process(statistics, &rpiMetadata);
>  
> diff --git a/src/ipa/raspberrypi/statistics.h b/src/ipa/raspberrypi/statistics.h
> index a762bf3d41aa..affb7272c963 100644
> --- a/src/ipa/raspberrypi/statistics.h
> +++ b/src/ipa/raspberrypi/statistics.h
> @@ -67,4 +67,6 @@ struct Statistics {
>         FocusRegions focusRegions;
>  };
>  
> +using StatisticsPtr = std::shared_ptr<Statistics>;
> +
>  } /* namespace RPiController */
> -- 
> 2.25.1
>

Hi Naush,

Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:35)
> Repurpose the StatisticsPtr type from being a shared_ptr<bcm2835_isp_stats> to
> shared_ptr<RPiController::Statistics>. This removes any hardware specific header
> files and structures from the algorithms source code.
> 
> Add a new function in the Raspberry Pi IPA to populate the generic statistics
> structure from the values provided by the hardware in the bcm2835_isp_stats
> structure.
> 
> Update the Lux, AWB, AGC, ALSC, Contrast, and Focus algorithms to use the
> generic statistics structure appropriately in their calculations. Additionally,
> remove references to any hardware specific headers and defines in these source
> files.

I'm afraid this one fails to build as I've merged IMX708 support with
the AF algorithm additions.

Could you rebase this series please?


[5/119] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o
FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o
c++ -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi -I../../src/ipa/raspberrypi -Iinclude -I../../include -Isrc/ipa -I../../src/ipa -I../../src/ipa/raspberrypi/controller -Iinclude/libcamera/ipa -Iinclude/libcamera -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wshadow -include /home/kbingham/iob/libcamera/libcamera/build/gcc/config.h -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o -MF src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o.d -o src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o -c ../../src/ipa/raspberrypi/controller/rpi/af.cpp
In file included from ../../src/ipa/raspberrypi/controller/rpi/af.cpp:8:
../../src/ipa/raspberrypi/controller/rpi/af.h:120:77: error: ‘FOCUS_REGIONS’ was not declared in this scope
  120 |         double getContrast(struct bcm2835_isp_stats_focus const focus_stats[FOCUS_REGIONS]) const;
      |                                                                             ^~~~~~~~~~~~~
../../src/ipa/raspberrypi/controller/rpi/af.h:141:35: error: ‘FOCUS_REGIONS’ was not declared in this scope
  141 |         uint16_t contrastWeights_[FOCUS_REGIONS];
      |                                   ^~~~~~~~~~~~~
../../src/ipa/raspberrypi/controller/rpi/af.cpp: In constructor ‘RPiController::Af::Af(RPiController::Controller*)’:
../../src/ipa/raspberrypi/controller/rpi/af.cpp:178:11: error: class ‘RPiController::Af’ does not have any field named ‘contrastWeights_’
  178 |           contrastWeights_{},
      |           ^~~~~~~~~~~~~~~~
../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘void RPiController::Af::computeWeights()’:
../../src/ipa/raspberrypi/controller/rpi/af.cpp:300:23: error: ‘FOCUS_REGIONS’ was not declared in this scope
  300 |         static_assert(FOCUS_REGIONS == FocusStatsRows * FocusStatsCols);
      |                       ^~~~~~~~~~~~~
../../src/ipa/raspberrypi/controller/rpi/af.cpp:313:25: error: ‘contrastWeights_’ was not declared in this scope; did you mean ‘phaseWeights_’?
  313 |                         contrastWeights_[FocusStatsCols * i + j] = w;
      |                         ^~~~~~~~~~~~~~~~
      |                         phaseWeights_
../../src/ipa/raspberrypi/controller/rpi/af.cpp:316:38: error: ‘contrastWeights_’ was not declared in this scope; did you mean ‘phaseWeights_’?
  316 |                                   << contrastWeights_[FocusStatsCols * i + 0] << " "
      |                                      ^~~~~~~~~~~~~~~~
      |                                      phaseWeights_
../../src/ipa/raspberrypi/controller/rpi/af.cpp: At global scope:
../../src/ipa/raspberrypi/controller/rpi/af.cpp:355:73: error: ‘FOCUS_REGIONS’ was not declared in this scope
  355 | double Af::getContrast(struct bcm2835_isp_stats_focus const focus_stats[FOCUS_REGIONS]) const
      |                                                                         ^~~~~~~~~~~~~
../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘double RPiController::Af::getContrast(...) const’:
../../src/ipa/raspberrypi/controller/rpi/af.cpp:359:34: error: ‘FOCUS_REGIONS’ was not declared in this scope
  359 |         for (unsigned i = 0; i < FOCUS_REGIONS; ++i) {
      |                                  ^~~~~~~~~~~~~
../../src/ipa/raspberrypi/controller/rpi/af.cpp:360:30: error: ‘contrastWeights_’ was not declared in this scope; did you mean ‘phaseWeights_’?
  360 |                 unsigned w = contrastWeights_[i];
      |                              ^~~~~~~~~~~~~~~~
      |                              phaseWeights_
../../src/ipa/raspberrypi/controller/rpi/af.cpp:361:31: error: ‘focus_stats’ was not declared in this scope
  361 |                 sumWc += w * (focus_stats[i].contrast_val[1][1] >> 10);
      |                               ^~~~~~~~~~~
../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘virtual void RPiController::Af::process(RPiController::StatisticsPtr&, RPiController::Metadata*)’:
../../src/ipa/raspberrypi/controller/rpi/af.cpp:669:44: error: ‘using element_type = struct RPiController::Statistics’ {aka ‘struct RPiController::Statistics’} has no member named ‘focus_stats’
  669 |         prevContrast_ = getContrast(stats->focus_stats);
      |                                            ^~~~~~~~~~~
[12/119] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o
FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o
c++ -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi -I../../src/ipa/raspberrypi -Iinclude -I../../include -Isrc/ipa -I../../src/ipa -I../../src/ipa/raspberrypi/controller -Iinclude/libcamera/ipa -Iinclude/libcamera -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wshadow -include /home/kbingham/iob/libcamera/libcamera/build/gcc/config.h -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -MF src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o.d -o src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -c ../../src/ipa/raspberrypi/cam_helper_imx708.cpp
../../src/ipa/raspberrypi/cam_helper_imx708.cpp: In member function ‘void CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)’:
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:332:23: error: ‘using element_type = struct RPiController::Statistics’ {aka ‘struct RPiController::Statistics’} has no member named ‘hist’
  332 |         memcpy(stats->hist[0].g_hist, aeHistLinear_, sizeof(stats->hist[0].g_hist));
      |                       ^~~~
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:332:68: error: ‘using element_type = struct RPiController::Statistics’ {aka ‘struct RPiController::Statistics’} has no member named ‘hist’
  332 |         memcpy(stats->hist[0].g_hist, aeHistLinear_, sizeof(stats->hist[0].g_hist));
      |                                                                    ^~~~
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:336:29: error: ‘AGC_REGIONS’ was not declared in this scope
  336 |         for (int i = 0; i < AGC_REGIONS; i++) {
      |                             ^~~~~~~~~~~
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:61: error: ‘using element_type = struct RPiController::Statistics’ {aka ‘struct RPiController::Statistics’} has no member named ‘agc_stats’
  337 |                 struct bcm2835_isp_stats_region &r = stats->agc_stats[i];
      |                                                             ^~~~~~~~~
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:17: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
      |                 ^
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  337 |                 struct bcm2835_isp_stats_region &r = stats->agc_stats[i];
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:27: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
      |                           ^
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  337 |                 struct bcm2835_isp_stats_region &r = stats->agc_stats[i];
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:37: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
      |                                     ^
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  337 |                 struct bcm2835_isp_stats_region &r = stats->agc_stats[i];
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:47: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
      |                                               ^
../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
  337 |                 struct bcm2835_isp_stats_region &r = stats->agc_stats[i];
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~

--
Kieran


> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/controller.h   |  4 +-
>  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 27 +++++-------
>  src/ipa/raspberrypi/controller/rpi/agc.h      |  2 +-
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp   | 32 +++++++-------
>  src/ipa/raspberrypi/controller/rpi/alsc.h     |  3 +-
>  src/ipa/raspberrypi/controller/rpi/awb.cpp    | 20 ++++-----
>  src/ipa/raspberrypi/controller/rpi/awb.h      |  1 +
>  .../raspberrypi/controller/rpi/contrast.cpp   |  8 ++--
>  src/ipa/raspberrypi/controller/rpi/focus.cpp  |  7 ++-
>  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 14 +-----
>  src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++++++++++-
>  src/ipa/raspberrypi/statistics.h              |  2 +
>  12 files changed, 96 insertions(+), 68 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/controller.h b/src/ipa/raspberrypi/controller/controller.h
> index 3e1e051703b3..e6c950c3a509 100644
> --- a/src/ipa/raspberrypi/controller/controller.h
> +++ b/src/ipa/raspberrypi/controller/controller.h
> @@ -15,19 +15,17 @@
>  #include <vector>
>  #include <string>
>  
> -#include <linux/bcm2835-isp.h>
> -
>  #include "libcamera/internal/yaml_parser.h"
>  
>  #include "camera_mode.h"
>  #include "device_status.h"
>  #include "metadata.h"
> +#include "statistics.h"
>  
>  namespace RPiController {
>  
>  class Algorithm;
>  typedef std::unique_ptr<Algorithm> AlgorithmPtr;
> -typedef std::shared_ptr<bcm2835_isp_stats> StatisticsPtr;
>  
>  /*
>   * The Controller holds a pointer to some global_metadata, which is how
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 46dcc81ae14c..868c30f03d66 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -9,8 +9,6 @@
>  #include <map>
>  #include <tuple>
>  
> -#include <linux/bcm2835-isp.h>
> -
>  #include <libcamera/base/log.h>
>  
>  #include "../awb_status.h"
> @@ -451,7 +449,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
>         fetchCurrentExposure(imageMetadata);
>         /* Compute the total gain we require relative to the current exposure. */
>         double gain, targetY;
> -       computeGain(stats.get(), imageMetadata, gain, targetY);
> +       computeGain(stats, imageMetadata, gain, targetY);
>         /* Now compute the target (final) exposure which we think we want. */
>         computeTargetExposure(gain);
>         /*
> @@ -585,24 +583,23 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)
>                 LOG(RPiAgc, Debug) << "No AWB status found";
>  }
>  
> -static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,
> +static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,
>                               double weights[], double gain)
>  {
> -       bcm2835_isp_stats_region *regions = stats->agc_stats;
>         /*
>          * Note how the calculation below means that equal weights give you
>          * "average" metering (i.e. all pixels equally important).
>          */
>         double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;
> -       for (unsigned int i = 0; i < AgcStatsSize; i++) {
> -               double counted = regions[i].counted;
> -               double rAcc = std::min(regions[i].r_sum * gain, ((1 << PipelineBits) - 1) * counted);
> -               double gAcc = std::min(regions[i].g_sum * gain, ((1 << PipelineBits) - 1) * counted);
> -               double bAcc = std::min(regions[i].b_sum * gain, ((1 << PipelineBits) - 1) * counted);
> +       for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {
> +               auto &region = stats->agcRegions.get(i);
> +               double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);
> +               double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);
> +               double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);
>                 rSum += rAcc * weights[i];
>                 gSum += gAcc * weights[i];
>                 bSum += bAcc * weights[i];
> -               pixelSum += counted * weights[i];
> +               pixelSum += region.counted * weights[i];
>         }
>         if (pixelSum == 0.0) {
>                 LOG(RPiAgc, Warning) << "computeInitialY: pixelSum is zero";
> @@ -624,23 +621,23 @@ static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,
>  
>  static constexpr double EvGainYTargetLimit = 0.9;
>  
> -static double constraintComputeGain(AgcConstraint &c, Histogram &h, double lux,
> +static double constraintComputeGain(AgcConstraint &c, const Histogram &h, double lux,
>                                     double evGain, double &targetY)
>  {
>         targetY = c.yTarget.eval(c.yTarget.domain().clip(lux));
>         targetY = std::min(EvGainYTargetLimit, targetY * evGain);
>         double iqm = h.interQuantileMean(c.qLo, c.qHi);
> -       return (targetY * NUM_HISTOGRAM_BINS) / iqm;
> +       return (targetY * h.bins()) / iqm;
>  }
>  
> -void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,
> +void Agc::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,
>                       double &gain, double &targetY)
>  {
>         struct LuxStatus lux = {};
>         lux.lux = 400; /* default lux level to 400 in case no metadata found */
>         if (imageMetadata->get("lux.status", lux) != 0)
>                 LOG(RPiAgc, Warning) << "No lux level found";
> -       Histogram h(statistics->hist[0].g_hist, NUM_HISTOGRAM_BINS);
> +       const Histogram &h = statistics->yHist;
>         double evGain = status_.ev * config_.baseEv;
>         /*
>          * The initial gain and target_Y come from some of the regions. After
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h
> index cf04da1973f1..f04896ca25ad 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.h
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h
> @@ -96,7 +96,7 @@ private:
>         void housekeepConfig();
>         void fetchCurrentExposure(Metadata *imageMetadata);
>         void fetchAwbStatus(Metadata *imageMetadata);
> -       void computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,
> +       void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,
>                          double &gain, double &targetY);
>         void computeTargetExposure(double gain);
>         bool applyDigitalGain(double gain, double targetY);
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index a4afaf841c41..eb4e2f9496e1 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -310,19 +310,21 @@ double getCt(Metadata *metadata, double defaultCt)
>         return awbStatus.temperatureK;
>  }
>  
> -static void copyStats(bcm2835_isp_stats_region regions[XY], StatisticsPtr &stats,
> +static void copyStats(RgbyRegions &regions, StatisticsPtr &stats,
>                       AlscStatus const &status)
>  {
> -       bcm2835_isp_stats_region *inputRegions = stats->awb_stats;
> +       if (!regions.numRegions())
> +               regions.init(stats->awbRegions.size());
> +
>         double *rTable = (double *)status.r;
>         double *gTable = (double *)status.g;
>         double *bTable = (double *)status.b;
> -       for (int i = 0; i < XY; i++) {
> -               regions[i].r_sum = inputRegions[i].r_sum / rTable[i];
> -               regions[i].g_sum = inputRegions[i].g_sum / gTable[i];
> -               regions[i].b_sum = inputRegions[i].b_sum / bTable[i];
> -               regions[i].counted = inputRegions[i].counted;
> -               /* (don't care about the uncounted value) */
> +       for (unsigned int i = 0; i < stats->awbRegions.numRegions(); i++) {
> +               auto r = stats->awbRegions.get(i);
> +               r.val.rSum = static_cast<uint64_t>(r.val.rSum / rTable[i]);
> +               r.val.gSum = static_cast<uint64_t>(r.val.gSum / gTable[i]);
> +               r.val.bSum = static_cast<uint64_t>(r.val.bSum / bTable[i]);
> +               regions.set(i, r);
>         }
>  }
>  
> @@ -512,19 +514,19 @@ void resampleCalTable(double const calTableIn[XY],
>  }
>  
>  /* Calculate chrominance statistics (R/G and B/G) for each region. */
> -static_assert(XY == AWB_REGIONS, "ALSC/AWB statistics region mismatch");
> -static void calculateCrCb(bcm2835_isp_stats_region *awbRegion, double cr[XY],
> +static void calculateCrCb(const RgbyRegions &awbRegion, double cr[XY],
>                           double cb[XY], uint32_t minCount, uint16_t minG)
>  {
>         for (int i = 0; i < XY; i++) {
> -               bcm2835_isp_stats_region &zone = awbRegion[i];
> -               if (zone.counted <= minCount ||
> -                   zone.g_sum / zone.counted <= minG) {
> +               auto s = awbRegion.get(i);
> +
> +               if (s.counted <= minCount || s.val.gSum / s.counted <= minG) {
>                         cr[i] = cb[i] = InsufficientData;
>                         continue;
>                 }
> -               cr[i] = zone.r_sum / (double)zone.g_sum;
> -               cb[i] = zone.b_sum / (double)zone.g_sum;
> +
> +               cr[i] = s.val.rSum / (double)s.val.gSum;
> +               cb[i] = s.val.bSum / (double)s.val.gSum;
>         }
>  }
>  
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h b/src/ipa/raspberrypi/controller/rpi/alsc.h
> index a858ef5a6551..9167c9ffa2e3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.h
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h
> @@ -12,6 +12,7 @@
>  
>  #include "../algorithm.h"
>  #include "../alsc_status.h"
> +#include "../statistics.h"
>  
>  namespace RPiController {
>  
> @@ -98,7 +99,7 @@ private:
>         /* copy out the results from the async thread so that it can be restarted */
>         void fetchAsyncResults();
>         double ct_;
> -       bcm2835_isp_stats_region statistics_[AlscCellsY * AlscCellsX];
> +       RgbyRegions statistics_;
>         double asyncResults_[3][AlscCellsY][AlscCellsX];
>         double asyncLambdaR_[AlscCellsX * AlscCellsY];
>         double asyncLambdaB_[AlscCellsX * AlscCellsY];
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> index 04d1c8783654..ef3435d66106 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> @@ -21,9 +21,6 @@ LOG_DEFINE_CATEGORY(RPiAwb)
>  
>  #define NAME "rpi.awb"
>  
> -static constexpr unsigned int AwbStatsSizeX = DEFAULT_AWB_REGIONS_X;
> -static constexpr unsigned int AwbStatsSizeY = DEFAULT_AWB_REGIONS_Y;
> -
>  /*
>   * todo - the locking in this algorithm needs some tidying up as has been done
>   * elsewhere (ALSC and AGC).
> @@ -401,17 +398,16 @@ void Awb::asyncFunc()
>  }
>  
>  static void generateStats(std::vector<Awb::RGB> &zones,
> -                         bcm2835_isp_stats_region *stats, double minPixels,
> +                         RgbyRegions &stats, double minPixels,
>                           double minG)
>  {
> -       for (unsigned int i = 0; i < AwbStatsSizeX * AwbStatsSizeY; i++) {
> +       for (auto const &region : stats) {
>                 Awb::RGB zone;
> -               double counted = stats[i].counted;
> -               if (counted >= minPixels) {
> -                       zone.G = stats[i].g_sum / counted;
> +               if (region.counted >= minPixels) {
> +                       zone.G = region.val.gSum / region.counted;
>                         if (zone.G >= minG) {
> -                               zone.R = stats[i].r_sum / counted;
> -                               zone.B = stats[i].b_sum / counted;
> +                               zone.R = region.val.rSum / region.counted;
> +                               zone.B = region.val.bSum / region.counted;
>                                 zones.push_back(zone);
>                         }
>                 }
> @@ -425,7 +421,7 @@ void Awb::prepareStats()
>          * LSC has already been applied to the stats in this pipeline, so stop
>          * any LSC compensation.  We also ignore config_.fast in this version.
>          */
> -       generateStats(zones_, statistics_->awb_stats, config_.minPixels,
> +       generateStats(zones_, statistics_->awbRegions, config_.minPixels,
>                       config_.minG);
>         /*
>          * apply sensitivities, so values appear to come from our "canonical"
> @@ -641,7 +637,7 @@ void Awb::awbBayes()
>          * valid... not entirely sure about this.
>          */
>         Pwl prior = interpolatePrior();
> -       prior *= zones_.size() / (double)(AwbStatsSizeX * AwbStatsSizeY);
> +       prior *= zones_.size() / (double)(statistics_->awbRegions.numRegions());
>         prior.map([](double x, double y) {
>                 LOG(RPiAwb, Debug) << "(" << x << "," << y << ")";
>         });
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h b/src/ipa/raspberrypi/controller/rpi/awb.h
> index 2254c3ed2cc4..e7d49cd8036b 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.h
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.h
> @@ -13,6 +13,7 @@
>  #include "../awb_algorithm.h"
>  #include "../pwl.h"
>  #include "../awb_status.h"
> +#include "../statistics.h"
>  
>  namespace RPiController {
>  
> diff --git a/src/ipa/raspberrypi/controller/rpi/contrast.cpp b/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> index 5b37edcbd46a..a4f8c4f04fc4 100644
> --- a/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> @@ -106,7 +106,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
>          * bit.
>          */
>         double histLo = histogram.quantile(config.loHistogram) *
> -                       (65536 / NUM_HISTOGRAM_BINS);
> +                       (65536 / histogram.bins());
>         double levelLo = config.loLevel * 65536;
>         LOG(RPiContrast, Debug)
>                 << "Move histogram point " << histLo << " to " << levelLo;
> @@ -119,7 +119,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
>          * Keep the mid-point (median) in the same place, though, to limit the
>          * apparent amount of global brightness shift.
>          */
> -       double mid = histogram.quantile(0.5) * (65536 / NUM_HISTOGRAM_BINS);
> +       double mid = histogram.quantile(0.5) * (65536 / histogram.bins());
>         enhance.append(mid, mid);
>  
>         /*
> @@ -127,7 +127,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
>          * there up.
>          */
>         double histHi = histogram.quantile(config.hiHistogram) *
> -                       (65536 / NUM_HISTOGRAM_BINS);
> +                       (65536 / histogram.bins());
>         double levelHi = config.hiLevel * 65536;
>         LOG(RPiContrast, Debug)
>                 << "Move histogram point " << histHi << " to " << levelHi;
> @@ -158,7 +158,7 @@ Pwl applyManualContrast(Pwl const &gammaCurve, double brightness,
>  void Contrast::process(StatisticsPtr &stats,
>                        [[maybe_unused]] Metadata *imageMetadata)
>  {
> -       Histogram histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);
> +       Histogram &histogram = stats->yHist;
>         /*
>          * We look at the histogram and adjust the gamma curve in the following
>          * ways: 1. Adjust the gamma curve so as to pull the start of the
> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> index 8c5029bd0e95..ea3cc00e42c3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> @@ -31,10 +31,9 @@ char const *Focus::name() const
>  void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)
>  {
>         FocusStatus status;
> -       unsigned int i;
> -       for (i = 0; i < FOCUS_REGIONS; i++)
> -               status.focusMeasures[i] = stats->focus_stats[i].contrast_val[1][1] / 1000;
> -       status.num = i;
> +       for (unsigned int i = 0; i < stats->focusRegions.numRegions(); i++)
> +               status.focusMeasures[i] = stats->focusRegions.get(i).val;
> +       status.num = stats->focusRegions.numRegions();
>         imageMetadata->set("focus.status", status);
>  
>         LOG(RPiFocus, Debug)
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> index 9759186afacf..06625f3a5ea3 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -6,8 +6,6 @@
>   */
>  #include <math.h>
>  
> -#include <linux/bcm2835-isp.h>
> -
>  #include <libcamera/base/log.h>
>  
>  #include "../device_status.h"
> @@ -83,20 +81,12 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)
>         if (imageMetadata->get("device.status", deviceStatus) == 0) {
>                 double currentGain = deviceStatus.analogueGain;
>                 double currentAperture = deviceStatus.aperture.value_or(currentAperture_);
> -               uint64_t sum = 0;
> -               uint32_t num = 0;
> -               uint32_t *bin = stats->hist[0].g_hist;
> -               const int numBins = sizeof(stats->hist[0].g_hist) /
> -                                   sizeof(stats->hist[0].g_hist[0]);
> -               for (int i = 0; i < numBins; i++)
> -                       sum += bin[i] * (uint64_t)i, num += bin[i];
> -               /* add .5 to reflect the mid-points of bins */
> -               double currentY = sum / (double)num + .5;
> +               double currentY = stats->yHist.interQuantileMean(0, 1);
>                 double gainRatio = referenceGain_ / currentGain;
>                 double shutterSpeedRatio =
>                         referenceShutterSpeed_ / deviceStatus.shutterSpeed;
>                 double apertureRatio = referenceAperture_ / currentAperture;
> -               double yRatio = currentY * (65536 / numBins) / referenceY_;
> +               double yRatio = currentY * (65536 / stats->yHist.bins()) / referenceY_;
>                 double estimatedLux = shutterSpeedRatio * gainRatio *
>                                       apertureRatio * apertureRatio *
>                                       yRatio * referenceLux_;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index bead436def3c..cff079bbafb3 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -51,6 +51,7 @@
>  #include "metadata.h"
>  #include "sharpen_algorithm.h"
>  #include "sharpen_status.h"
> +#include "statistics.h"
>  
>  namespace libcamera {
>  
> @@ -139,6 +140,7 @@ private:
>         void prepareISP(const ISPConfig &data);
>         void reportMetadata(unsigned int ipaContext);
>         void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> +       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
>         void processStats(unsigned int bufferId, unsigned int ipaContext);
>         void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
>         void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> @@ -1141,6 +1143,46 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls, unsigned int ip
>         rpiMetadata_[ipaContext].set("device.status", deviceStatus);
>  }
>  
> +RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) const
> +{
> +       using namespace RPiController;
> +
> +       unsigned int i;
> +       StatisticsPtr statistics =
> +               std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc);
> +
> +       /* RGB histograms are not used, so do not populate them. */
> +       statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));
> +
> +       statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });
> +       for (i = 0; i < statistics->awbRegions.numRegions(); i++)
> +               statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,
> +                                                 stats->awb_stats[i].g_sum,
> +                                                 stats->awb_stats[i].b_sum },
> +                                               stats->awb_stats[i].counted,
> +                                               stats->awb_stats[i].notcounted });
> +
> +       /*
> +        * There are only ever 15 regions computed by the firmware due to zoning,
> +        * but the HW defines AGC_REGIONS == 16!
> +        */
> +       statistics->agcRegions.init(15);
> +       for (i = 0; i < statistics->agcRegions.numRegions(); i++)
> +               statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,
> +                                                 stats->agc_stats[i].g_sum,
> +                                                 stats->agc_stats[i].b_sum },
> +                                               stats->agc_stats[i].counted,
> +                                               stats->awb_stats[i].notcounted });
> +
> +       statistics->focusRegions.init({ 4, 3 });
> +       for (i = 0; i < statistics->focusRegions.numRegions(); i++)
> +               statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,
> +                                                 stats->focus_stats[i].contrast_val_num[1][1],
> +                                                 stats->focus_stats[i].contrast_val_num[1][0] });
> +
> +       return statistics;
> +}
> +
>  void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
>  {
>         RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> @@ -1153,7 +1195,7 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
>  
>         Span<uint8_t> mem = it->second.planes()[0];
>         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());
> -       RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);
> +       RPiController::StatisticsPtr statistics = fillStatistics(stats);
>         helper_->process(statistics, rpiMetadata);
>         controller_.process(statistics, &rpiMetadata);
>  
> diff --git a/src/ipa/raspberrypi/statistics.h b/src/ipa/raspberrypi/statistics.h
> index a762bf3d41aa..affb7272c963 100644
> --- a/src/ipa/raspberrypi/statistics.h
> +++ b/src/ipa/raspberrypi/statistics.h
> @@ -67,4 +67,6 @@ struct Statistics {
>         FocusRegions focusRegions;
>  };
>  
> +using StatisticsPtr = std::shared_ptr<Statistics>;
> +
>  } /* namespace RPiController */
> -- 
> 2.25.1
>

Hi Kieran,

On Tue, 31 Jan 2023, 7:01 pm Kieran Bingham, <
kieran.bingham@ideasonboard.com> wrote:

> Hi Naush,
>
> Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:35)
> > Repurpose the StatisticsPtr type from being a
> shared_ptr<bcm2835_isp_stats> to
> > shared_ptr<RPiController::Statistics>. This removes any hardware
> specific header
> > files and structures from the algorithms source code.
> >
> > Add a new function in the Raspberry Pi IPA to populate the generic
> statistics
> > structure from the values provided by the hardware in the
> bcm2835_isp_stats
> > structure.
> >
> > Update the Lux, AWB, AGC, ALSC, Contrast, and Focus algorithms to use the
> > generic statistics structure appropriately in their calculations.
> Additionally,
> > remove references to any hardware specific headers and defines in these
> source
> > files.
>
> I'm afraid this one fails to build as I've merged IMX708 support with
> the AF algorithm additions.
>
> Could you rebase this series please?
>


Ouch!

This is because we've removed the bcm2835-isp.h header from the controller
in this series.

Once this series has been merged, we do have the task of converting the
focus stats to a generic region struct. For now I'll probably post an
updated patch with the header included back.

Regards,
Naush

>
>
> [5/119] Compiling C++ object
> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o
> FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o
> c++ -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi
> -I../../src/ipa/raspberrypi -Iinclude -I../../include -Isrc/ipa
> -I../../src/ipa -I../../src/ipa/raspberrypi/controller
> -Iinclude/libcamera/ipa -Iinclude/libcamera -fdiagnostics-color=always
> -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra
> -Werror -std=c++17 -g -Wshadow -include
> /home/kbingham/iob/libcamera/libcamera/build/gcc/config.h -fPIC
> -DLIBCAMERA_BASE_PRIVATE -MD -MQ
> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o -MF
> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o.d -o
> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o -c
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp
> In file included from ../../src/ipa/raspberrypi/controller/rpi/af.cpp:8:
> ../../src/ipa/raspberrypi/controller/rpi/af.h:120:77: error:
> ‘FOCUS_REGIONS’ was not declared in this scope
>   120 |         double getContrast(struct bcm2835_isp_stats_focus const
> focus_stats[FOCUS_REGIONS]) const;
>       |
>          ^~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/controller/rpi/af.h:141:35: error:
> ‘FOCUS_REGIONS’ was not declared in this scope
>   141 |         uint16_t contrastWeights_[FOCUS_REGIONS];
>       |                                   ^~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In constructor
> ‘RPiController::Af::Af(RPiController::Controller*)’:
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:178:11: error: class
> ‘RPiController::Af’ does not have any field named ‘contrastWeights_’
>   178 |           contrastWeights_{},
>       |           ^~~~~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘void
> RPiController::Af::computeWeights()’:
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:300:23: error:
> ‘FOCUS_REGIONS’ was not declared in this scope
>   300 |         static_assert(FOCUS_REGIONS == FocusStatsRows *
> FocusStatsCols);
>       |                       ^~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:313:25: error:
> ‘contrastWeights_’ was not declared in this scope; did you mean
> ‘phaseWeights_’?
>   313 |                         contrastWeights_[FocusStatsCols * i + j] =
> w;
>       |                         ^~~~~~~~~~~~~~~~
>       |                         phaseWeights_
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:316:38: error:
> ‘contrastWeights_’ was not declared in this scope; did you mean
> ‘phaseWeights_’?
>   316 |                                   <<
> contrastWeights_[FocusStatsCols * i + 0] << " "
>       |                                      ^~~~~~~~~~~~~~~~
>       |                                      phaseWeights_
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: At global scope:
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:355:73: error:
> ‘FOCUS_REGIONS’ was not declared in this scope
>   355 | double Af::getContrast(struct bcm2835_isp_stats_focus const
> focus_stats[FOCUS_REGIONS]) const
>       |
>      ^~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function
> ‘double RPiController::Af::getContrast(...) const’:
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:359:34: error:
> ‘FOCUS_REGIONS’ was not declared in this scope
>   359 |         for (unsigned i = 0; i < FOCUS_REGIONS; ++i) {
>       |                                  ^~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:360:30: error:
> ‘contrastWeights_’ was not declared in this scope; did you mean
> ‘phaseWeights_’?
>   360 |                 unsigned w = contrastWeights_[i];
>       |                              ^~~~~~~~~~~~~~~~
>       |                              phaseWeights_
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:361:31: error:
> ‘focus_stats’ was not declared in this scope
>   361 |                 sumWc += w * (focus_stats[i].contrast_val[1][1] >>
> 10);
>       |                               ^~~~~~~~~~~
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function
> ‘virtual void RPiController::Af::process(RPiController::StatisticsPtr&,
> RPiController::Metadata*)’:
> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:669:44: error: ‘using
> element_type = struct RPiController::Statistics’ {aka ‘struct
> RPiController::Statistics’} has no member named ‘focus_stats’
>   669 |         prevContrast_ = getContrast(stats->focus_stats);
>       |                                            ^~~~~~~~~~~
> [12/119] Compiling C++ object
> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o
> FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o
> c++ -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi
> -I../../src/ipa/raspberrypi -Iinclude -I../../include -Isrc/ipa
> -I../../src/ipa -I../../src/ipa/raspberrypi/controller
> -Iinclude/libcamera/ipa -Iinclude/libcamera -fdiagnostics-color=always
> -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra
> -Werror -std=c++17 -g -Wshadow -include
> /home/kbingham/iob/libcamera/libcamera/build/gcc/config.h -fPIC
> -DLIBCAMERA_BASE_PRIVATE -MD -MQ
> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -MF
> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o.d -o
> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -c
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp: In member function ‘void
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)’:
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:332:23: error: ‘using
> element_type = struct RPiController::Statistics’ {aka ‘struct
> RPiController::Statistics’} has no member named ‘hist’
>   332 |         memcpy(stats->hist[0].g_hist, aeHistLinear_,
> sizeof(stats->hist[0].g_hist));
>       |                       ^~~~
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:332:68: error: ‘using
> element_type = struct RPiController::Statistics’ {aka ‘struct
> RPiController::Statistics’} has no member named ‘hist’
>   332 |         memcpy(stats->hist[0].g_hist, aeHistLinear_,
> sizeof(stats->hist[0].g_hist));
>       |
> ^~~~
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:336:29: error:
> ‘AGC_REGIONS’ was not declared in this scope
>   336 |         for (int i = 0; i < AGC_REGIONS; i++) {
>       |                             ^~~~~~~~~~~
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:61: error: ‘using
> element_type = struct RPiController::Statistics’ {aka ‘struct
> RPiController::Statistics’} has no member named ‘agc_stats’
>   337 |                 struct bcm2835_isp_stats_region &r =
> stats->agc_stats[i];
>       |
>  ^~~~~~~~~
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:17: error: invalid use
> of incomplete type ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
>       |                 ^
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward
> declaration of ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   337 |                 struct bcm2835_isp_stats_region &r =
> stats->agc_stats[i];
>       |                        ^~~~~~~~~~~~~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:27: error: invalid use
> of incomplete type ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
>       |                           ^
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward
> declaration of ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   337 |                 struct bcm2835_isp_stats_region &r =
> stats->agc_stats[i];
>       |                        ^~~~~~~~~~~~~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:37: error: invalid use
> of incomplete type ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
>       |                                     ^
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward
> declaration of ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   337 |                 struct bcm2835_isp_stats_region &r =
> stats->agc_stats[i];
>       |                        ^~~~~~~~~~~~~~~~~~~~~~~~
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:47: error: invalid use
> of incomplete type ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   338 |                 r.r_sum = r.b_sum = r.g_sum = r.counted * v;
>       |                                               ^
> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward
> declaration of ‘struct
> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’
>   337 |                 struct bcm2835_isp_stats_region &r =
> stats->agc_stats[i];
>       |                        ^~~~~~~~~~~~~~~~~~~~~~~~
>
> --
> Kieran
>
>
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/controller.h   |  4 +-
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 27 +++++-------
> >  src/ipa/raspberrypi/controller/rpi/agc.h      |  2 +-
> >  src/ipa/raspberrypi/controller/rpi/alsc.cpp   | 32 +++++++-------
> >  src/ipa/raspberrypi/controller/rpi/alsc.h     |  3 +-
> >  src/ipa/raspberrypi/controller/rpi/awb.cpp    | 20 ++++-----
> >  src/ipa/raspberrypi/controller/rpi/awb.h      |  1 +
> >  .../raspberrypi/controller/rpi/contrast.cpp   |  8 ++--
> >  src/ipa/raspberrypi/controller/rpi/focus.cpp  |  7 ++-
> >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 14 +-----
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++++++++++-
> >  src/ipa/raspberrypi/statistics.h              |  2 +
> >  12 files changed, 96 insertions(+), 68 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/controller.h
> b/src/ipa/raspberrypi/controller/controller.h
> > index 3e1e051703b3..e6c950c3a509 100644
> > --- a/src/ipa/raspberrypi/controller/controller.h
> > +++ b/src/ipa/raspberrypi/controller/controller.h
> > @@ -15,19 +15,17 @@
> >  #include <vector>
> >  #include <string>
> >
> > -#include <linux/bcm2835-isp.h>
> > -
> >  #include "libcamera/internal/yaml_parser.h"
> >
> >  #include "camera_mode.h"
> >  #include "device_status.h"
> >  #include "metadata.h"
> > +#include "statistics.h"
> >
> >  namespace RPiController {
> >
> >  class Algorithm;
> >  typedef std::unique_ptr<Algorithm> AlgorithmPtr;
> > -typedef std::shared_ptr<bcm2835_isp_stats> StatisticsPtr;
> >
> >  /*
> >   * The Controller holds a pointer to some global_metadata, which is how
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 46dcc81ae14c..868c30f03d66 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -9,8 +9,6 @@
> >  #include <map>
> >  #include <tuple>
> >
> > -#include <linux/bcm2835-isp.h>
> > -
> >  #include <libcamera/base/log.h>
> >
> >  #include "../awb_status.h"
> > @@ -451,7 +449,7 @@ void Agc::process(StatisticsPtr &stats, Metadata
> *imageMetadata)
> >         fetchCurrentExposure(imageMetadata);
> >         /* Compute the total gain we require relative to the current
> exposure. */
> >         double gain, targetY;
> > -       computeGain(stats.get(), imageMetadata, gain, targetY);
> > +       computeGain(stats, imageMetadata, gain, targetY);
> >         /* Now compute the target (final) exposure which we think we
> want. */
> >         computeTargetExposure(gain);
> >         /*
> > @@ -585,24 +583,23 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)
> >                 LOG(RPiAgc, Debug) << "No AWB status found";
> >  }
> >
> > -static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const
> &awb,
> > +static double computeInitialY(StatisticsPtr &stats, AwbStatus const
> &awb,
> >                               double weights[], double gain)
> >  {
> > -       bcm2835_isp_stats_region *regions = stats->agc_stats;
> >         /*
> >          * Note how the calculation below means that equal weights give
> you
> >          * "average" metering (i.e. all pixels equally important).
> >          */
> >         double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;
> > -       for (unsigned int i = 0; i < AgcStatsSize; i++) {
> > -               double counted = regions[i].counted;
> > -               double rAcc = std::min(regions[i].r_sum * gain, ((1 <<
> PipelineBits) - 1) * counted);
> > -               double gAcc = std::min(regions[i].g_sum * gain, ((1 <<
> PipelineBits) - 1) * counted);
> > -               double bAcc = std::min(regions[i].b_sum * gain, ((1 <<
> PipelineBits) - 1) * counted);
> > +       for (unsigned int i = 0; i < stats->agcRegions.numRegions();
> i++) {
> > +               auto &region = stats->agcRegions.get(i);
> > +               double rAcc = std::min<double>(region.val.rSum * gain,
> ((1 << PipelineBits) - 1) * region.counted);
> > +               double gAcc = std::min<double>(region.val.gSum * gain,
> ((1 << PipelineBits) - 1) * region.counted);
> > +               double bAcc = std::min<double>(region.val.bSum * gain,
> ((1 << PipelineBits) - 1) * region.counted);
> >                 rSum += rAcc * weights[i];
> >                 gSum += gAcc * weights[i];
> >                 bSum += bAcc * weights[i];
> > -               pixelSum += counted * weights[i];
> > +               pixelSum += region.counted * weights[i];
> >         }
> >         if (pixelSum == 0.0) {
> >                 LOG(RPiAgc, Warning) << "computeInitialY: pixelSum is
> zero";
> > @@ -624,23 +621,23 @@ static double computeInitialY(bcm2835_isp_stats
> *stats, AwbStatus const &awb,
> >
> >  static constexpr double EvGainYTargetLimit = 0.9;
> >
> > -static double constraintComputeGain(AgcConstraint &c, Histogram &h,
> double lux,
> > +static double constraintComputeGain(AgcConstraint &c, const Histogram
> &h, double lux,
> >                                     double evGain, double &targetY)
> >  {
> >         targetY = c.yTarget.eval(c.yTarget.domain().clip(lux));
> >         targetY = std::min(EvGainYTargetLimit, targetY * evGain);
> >         double iqm = h.interQuantileMean(c.qLo, c.qHi);
> > -       return (targetY * NUM_HISTOGRAM_BINS) / iqm;
> > +       return (targetY * h.bins()) / iqm;
> >  }
> >
> > -void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata
> *imageMetadata,
> > +void Agc::computeGain(StatisticsPtr &statistics, Metadata
> *imageMetadata,
> >                       double &gain, double &targetY)
> >  {
> >         struct LuxStatus lux = {};
> >         lux.lux = 400; /* default lux level to 400 in case no metadata
> found */
> >         if (imageMetadata->get("lux.status", lux) != 0)
> >                 LOG(RPiAgc, Warning) << "No lux level found";
> > -       Histogram h(statistics->hist[0].g_hist, NUM_HISTOGRAM_BINS);
> > +       const Histogram &h = statistics->yHist;
> >         double evGain = status_.ev * config_.baseEv;
> >         /*
> >          * The initial gain and target_Y come from some of the regions.
> After
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h
> b/src/ipa/raspberrypi/controller/rpi/agc.h
> > index cf04da1973f1..f04896ca25ad 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.h
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.h
> > @@ -96,7 +96,7 @@ private:
> >         void housekeepConfig();
> >         void fetchCurrentExposure(Metadata *imageMetadata);
> >         void fetchAwbStatus(Metadata *imageMetadata);
> > -       void computeGain(bcm2835_isp_stats *statistics, Metadata
> *imageMetadata,
> > +       void computeGain(StatisticsPtr &statistics, Metadata
> *imageMetadata,
> >                          double &gain, double &targetY);
> >         void computeTargetExposure(double gain);
> >         bool applyDigitalGain(double gain, double targetY);
> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > index a4afaf841c41..eb4e2f9496e1 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > @@ -310,19 +310,21 @@ double getCt(Metadata *metadata, double defaultCt)
> >         return awbStatus.temperatureK;
> >  }
> >
> > -static void copyStats(bcm2835_isp_stats_region regions[XY],
> StatisticsPtr &stats,
> > +static void copyStats(RgbyRegions &regions, StatisticsPtr &stats,
> >                       AlscStatus const &status)
> >  {
> > -       bcm2835_isp_stats_region *inputRegions = stats->awb_stats;
> > +       if (!regions.numRegions())
> > +               regions.init(stats->awbRegions.size());
> > +
> >         double *rTable = (double *)status.r;
> >         double *gTable = (double *)status.g;
> >         double *bTable = (double *)status.b;
> > -       for (int i = 0; i < XY; i++) {
> > -               regions[i].r_sum = inputRegions[i].r_sum / rTable[i];
> > -               regions[i].g_sum = inputRegions[i].g_sum / gTable[i];
> > -               regions[i].b_sum = inputRegions[i].b_sum / bTable[i];
> > -               regions[i].counted = inputRegions[i].counted;
> > -               /* (don't care about the uncounted value) */
> > +       for (unsigned int i = 0; i < stats->awbRegions.numRegions();
> i++) {
> > +               auto r = stats->awbRegions.get(i);
> > +               r.val.rSum = static_cast<uint64_t>(r.val.rSum /
> rTable[i]);
> > +               r.val.gSum = static_cast<uint64_t>(r.val.gSum /
> gTable[i]);
> > +               r.val.bSum = static_cast<uint64_t>(r.val.bSum /
> bTable[i]);
> > +               regions.set(i, r);
> >         }
> >  }
> >
> > @@ -512,19 +514,19 @@ void resampleCalTable(double const calTableIn[XY],
> >  }
> >
> >  /* Calculate chrominance statistics (R/G and B/G) for each region. */
> > -static_assert(XY == AWB_REGIONS, "ALSC/AWB statistics region mismatch");
> > -static void calculateCrCb(bcm2835_isp_stats_region *awbRegion, double
> cr[XY],
> > +static void calculateCrCb(const RgbyRegions &awbRegion, double cr[XY],
> >                           double cb[XY], uint32_t minCount, uint16_t
> minG)
> >  {
> >         for (int i = 0; i < XY; i++) {
> > -               bcm2835_isp_stats_region &zone = awbRegion[i];
> > -               if (zone.counted <= minCount ||
> > -                   zone.g_sum / zone.counted <= minG) {
> > +               auto s = awbRegion.get(i);
> > +
> > +               if (s.counted <= minCount || s.val.gSum / s.counted <=
> minG) {
> >                         cr[i] = cb[i] = InsufficientData;
> >                         continue;
> >                 }
> > -               cr[i] = zone.r_sum / (double)zone.g_sum;
> > -               cb[i] = zone.b_sum / (double)zone.g_sum;
> > +
> > +               cr[i] = s.val.rSum / (double)s.val.gSum;
> > +               cb[i] = s.val.bSum / (double)s.val.gSum;
> >         }
> >  }
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h
> b/src/ipa/raspberrypi/controller/rpi/alsc.h
> > index a858ef5a6551..9167c9ffa2e3 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.h
> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h
> > @@ -12,6 +12,7 @@
> >
> >  #include "../algorithm.h"
> >  #include "../alsc_status.h"
> > +#include "../statistics.h"
> >
> >  namespace RPiController {
> >
> > @@ -98,7 +99,7 @@ private:
> >         /* copy out the results from the async thread so that it can be
> restarted */
> >         void fetchAsyncResults();
> >         double ct_;
> > -       bcm2835_isp_stats_region statistics_[AlscCellsY * AlscCellsX];
> > +       RgbyRegions statistics_;
> >         double asyncResults_[3][AlscCellsY][AlscCellsX];
> >         double asyncLambdaR_[AlscCellsX * AlscCellsY];
> >         double asyncLambdaB_[AlscCellsX * AlscCellsY];
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > index 04d1c8783654..ef3435d66106 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > @@ -21,9 +21,6 @@ LOG_DEFINE_CATEGORY(RPiAwb)
> >
> >  #define NAME "rpi.awb"
> >
> > -static constexpr unsigned int AwbStatsSizeX = DEFAULT_AWB_REGIONS_X;
> > -static constexpr unsigned int AwbStatsSizeY = DEFAULT_AWB_REGIONS_Y;
> > -
> >  /*
> >   * todo - the locking in this algorithm needs some tidying up as has
> been done
> >   * elsewhere (ALSC and AGC).
> > @@ -401,17 +398,16 @@ void Awb::asyncFunc()
> >  }
> >
> >  static void generateStats(std::vector<Awb::RGB> &zones,
> > -                         bcm2835_isp_stats_region *stats, double
> minPixels,
> > +                         RgbyRegions &stats, double minPixels,
> >                           double minG)
> >  {
> > -       for (unsigned int i = 0; i < AwbStatsSizeX * AwbStatsSizeY; i++)
> {
> > +       for (auto const &region : stats) {
> >                 Awb::RGB zone;
> > -               double counted = stats[i].counted;
> > -               if (counted >= minPixels) {
> > -                       zone.G = stats[i].g_sum / counted;
> > +               if (region.counted >= minPixels) {
> > +                       zone.G = region.val.gSum / region.counted;
> >                         if (zone.G >= minG) {
> > -                               zone.R = stats[i].r_sum / counted;
> > -                               zone.B = stats[i].b_sum / counted;
> > +                               zone.R = region.val.rSum /
> region.counted;
> > +                               zone.B = region.val.bSum /
> region.counted;
> >                                 zones.push_back(zone);
> >                         }
> >                 }
> > @@ -425,7 +421,7 @@ void Awb::prepareStats()
> >          * LSC has already been applied to the stats in this pipeline,
> so stop
> >          * any LSC compensation.  We also ignore config_.fast in this
> version.
> >          */
> > -       generateStats(zones_, statistics_->awb_stats, config_.minPixels,
> > +       generateStats(zones_, statistics_->awbRegions, config_.minPixels,
> >                       config_.minG);
> >         /*
> >          * apply sensitivities, so values appear to come from our
> "canonical"
> > @@ -641,7 +637,7 @@ void Awb::awbBayes()
> >          * valid... not entirely sure about this.
> >          */
> >         Pwl prior = interpolatePrior();
> > -       prior *= zones_.size() / (double)(AwbStatsSizeX * AwbStatsSizeY);
> > +       prior *= zones_.size() /
> (double)(statistics_->awbRegions.numRegions());
> >         prior.map([](double x, double y) {
> >                 LOG(RPiAwb, Debug) << "(" << x << "," << y << ")";
> >         });
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h
> b/src/ipa/raspberrypi/controller/rpi/awb.h
> > index 2254c3ed2cc4..e7d49cd8036b 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.h
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.h
> > @@ -13,6 +13,7 @@
> >  #include "../awb_algorithm.h"
> >  #include "../pwl.h"
> >  #include "../awb_status.h"
> > +#include "../statistics.h"
> >
> >  namespace RPiController {
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> b/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> > index 5b37edcbd46a..a4f8c4f04fc4 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/contrast.cpp
> > @@ -106,7 +106,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
> >          * bit.
> >          */
> >         double histLo = histogram.quantile(config.loHistogram) *
> > -                       (65536 / NUM_HISTOGRAM_BINS);
> > +                       (65536 / histogram.bins());
> >         double levelLo = config.loLevel * 65536;
> >         LOG(RPiContrast, Debug)
> >                 << "Move histogram point " << histLo << " to " <<
> levelLo;
> > @@ -119,7 +119,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
> >          * Keep the mid-point (median) in the same place, though, to
> limit the
> >          * apparent amount of global brightness shift.
> >          */
> > -       double mid = histogram.quantile(0.5) * (65536 /
> NUM_HISTOGRAM_BINS);
> > +       double mid = histogram.quantile(0.5) * (65536 /
> histogram.bins());
> >         enhance.append(mid, mid);
> >
> >         /*
> > @@ -127,7 +127,7 @@ Pwl computeStretchCurve(Histogram const &histogram,
> >          * there up.
> >          */
> >         double histHi = histogram.quantile(config.hiHistogram) *
> > -                       (65536 / NUM_HISTOGRAM_BINS);
> > +                       (65536 / histogram.bins());
> >         double levelHi = config.hiLevel * 65536;
> >         LOG(RPiContrast, Debug)
> >                 << "Move histogram point " << histHi << " to " <<
> levelHi;
> > @@ -158,7 +158,7 @@ Pwl applyManualContrast(Pwl const &gammaCurve,
> double brightness,
> >  void Contrast::process(StatisticsPtr &stats,
> >                        [[maybe_unused]] Metadata *imageMetadata)
> >  {
> > -       Histogram histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);
> > +       Histogram &histogram = stats->yHist;
> >         /*
> >          * We look at the histogram and adjust the gamma curve in the
> following
> >          * ways: 1. Adjust the gamma curve so as to pull the start of the
> > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp
> b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> > index 8c5029bd0e95..ea3cc00e42c3 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> > @@ -31,10 +31,9 @@ char const *Focus::name() const
> >  void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)
> >  {
> >         FocusStatus status;
> > -       unsigned int i;
> > -       for (i = 0; i < FOCUS_REGIONS; i++)
> > -               status.focusMeasures[i] =
> stats->focus_stats[i].contrast_val[1][1] / 1000;
> > -       status.num = i;
> > +       for (unsigned int i = 0; i < stats->focusRegions.numRegions();
> i++)
> > +               status.focusMeasures[i] = stats->focusRegions.get(i).val;
> > +       status.num = stats->focusRegions.numRegions();
> >         imageMetadata->set("focus.status", status);
> >
> >         LOG(RPiFocus, Debug)
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > index 9759186afacf..06625f3a5ea3 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > @@ -6,8 +6,6 @@
> >   */
> >  #include <math.h>
> >
> > -#include <linux/bcm2835-isp.h>
> > -
> >  #include <libcamera/base/log.h>
> >
> >  #include "../device_status.h"
> > @@ -83,20 +81,12 @@ void Lux::process(StatisticsPtr &stats, Metadata
> *imageMetadata)
> >         if (imageMetadata->get("device.status", deviceStatus) == 0) {
> >                 double currentGain = deviceStatus.analogueGain;
> >                 double currentAperture =
> deviceStatus.aperture.value_or(currentAperture_);
> > -               uint64_t sum = 0;
> > -               uint32_t num = 0;
> > -               uint32_t *bin = stats->hist[0].g_hist;
> > -               const int numBins = sizeof(stats->hist[0].g_hist) /
> > -                                   sizeof(stats->hist[0].g_hist[0]);
> > -               for (int i = 0; i < numBins; i++)
> > -                       sum += bin[i] * (uint64_t)i, num += bin[i];
> > -               /* add .5 to reflect the mid-points of bins */
> > -               double currentY = sum / (double)num + .5;
> > +               double currentY = stats->yHist.interQuantileMean(0, 1);
> >                 double gainRatio = referenceGain_ / currentGain;
> >                 double shutterSpeedRatio =
> >                         referenceShutterSpeed_ /
> deviceStatus.shutterSpeed;
> >                 double apertureRatio = referenceAperture_ /
> currentAperture;
> > -               double yRatio = currentY * (65536 / numBins) /
> referenceY_;
> > +               double yRatio = currentY * (65536 / stats->yHist.bins())
> / referenceY_;
> >                 double estimatedLux = shutterSpeedRatio * gainRatio *
> >                                       apertureRatio * apertureRatio *
> >                                       yRatio * referenceLux_;
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index bead436def3c..cff079bbafb3 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -51,6 +51,7 @@
> >  #include "metadata.h"
> >  #include "sharpen_algorithm.h"
> >  #include "sharpen_status.h"
> > +#include "statistics.h"
> >
> >  namespace libcamera {
> >
> > @@ -139,6 +140,7 @@ private:
> >         void prepareISP(const ISPConfig &data);
> >         void reportMetadata(unsigned int ipaContext);
> >         void fillDeviceStatus(const ControlList &sensorControls,
> unsigned int ipaContext);
> > +       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats
> *stats) const;
> >         void processStats(unsigned int bufferId, unsigned int
> ipaContext);
> >         void applyFrameDurations(Duration minFrameDuration, Duration
> maxFrameDuration);
> >         void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> > @@ -1141,6 +1143,46 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls, unsigned int ip
> >         rpiMetadata_[ipaContext].set("device.status", deviceStatus);
> >  }
> >
> > +RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats
> *stats) const
> > +{
> > +       using namespace RPiController;
> > +
> > +       unsigned int i;
> > +       StatisticsPtr statistics =
> > +
>  std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb,
> Statistics::ColourStatsPos::PostLsc);
> > +
> > +       /* RGB histograms are not used, so do not populate them. */
> > +       statistics->yHist =
> std::move(RPiController::Histogram(stats->hist[0].g_hist,
> NUM_HISTOGRAM_BINS));
> > +
> > +       statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X,
> DEFAULT_AWB_REGIONS_Y });
> > +       for (i = 0; i < statistics->awbRegions.numRegions(); i++)
> > +               statistics->awbRegions.set(i, { {
> stats->awb_stats[i].r_sum,
> > +
>  stats->awb_stats[i].g_sum,
> > +
>  stats->awb_stats[i].b_sum },
> > +
>  stats->awb_stats[i].counted,
> > +
>  stats->awb_stats[i].notcounted });
> > +
> > +       /*
> > +        * There are only ever 15 regions computed by the firmware due
> to zoning,
> > +        * but the HW defines AGC_REGIONS == 16!
> > +        */
> > +       statistics->agcRegions.init(15);
> > +       for (i = 0; i < statistics->agcRegions.numRegions(); i++)
> > +               statistics->agcRegions.set(i, { {
> stats->agc_stats[i].r_sum,
> > +
>  stats->agc_stats[i].g_sum,
> > +
>  stats->agc_stats[i].b_sum },
> > +
>  stats->agc_stats[i].counted,
> > +
>  stats->awb_stats[i].notcounted });
> > +
> > +       statistics->focusRegions.init({ 4, 3 });
> > +       for (i = 0; i < statistics->focusRegions.numRegions(); i++)
> > +               statistics->focusRegions.set(i, {
> stats->focus_stats[i].contrast_val[1][1] / 1000,
> > +
>  stats->focus_stats[i].contrast_val_num[1][1],
> > +
>  stats->focus_stats[i].contrast_val_num[1][0] });
> > +
> > +       return statistics;
> > +}
> > +
> >  void IPARPi::processStats(unsigned int bufferId, unsigned int
> ipaContext)
> >  {
> >         RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > @@ -1153,7 +1195,7 @@ void IPARPi::processStats(unsigned int bufferId,
> unsigned int ipaContext)
> >
> >         Span<uint8_t> mem = it->second.planes()[0];
> >         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats
> *>(mem.data());
> > -       RPiController::StatisticsPtr statistics =
> std::make_shared<bcm2835_isp_stats>(*stats);
> > +       RPiController::StatisticsPtr statistics = fillStatistics(stats);
> >         helper_->process(statistics, rpiMetadata);
> >         controller_.process(statistics, &rpiMetadata);
> >
> > diff --git a/src/ipa/raspberrypi/statistics.h
> b/src/ipa/raspberrypi/statistics.h
> > index a762bf3d41aa..affb7272c963 100644
> > --- a/src/ipa/raspberrypi/statistics.h
> > +++ b/src/ipa/raspberrypi/statistics.h
> > @@ -67,4 +67,6 @@ struct Statistics {
> >         FocusRegions focusRegions;
> >  };
> >
> > +using StatisticsPtr = std::shared_ptr<Statistics>;
> > +
> >  } /* namespace RPiController */
> > --
> > 2.25.1
> >
>