[{"id":26371,"web_url":"https://patchwork.libcamera.org/comment/26371/","msgid":"<167508504524.42371.97045623736344505@Monstersaurus>","date":"2023-01-30T13:24:05","subject":"Re: [libcamera-devel] [PATCH v3 4/5] ipa: raspberrypi: Use the\n\tgeneric statistics structure in the algorithms","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:35)\n> Repurpose the StatisticsPtr type from being a shared_ptr to\n> shared_ptr. This removes any hardware specific header\n> files and structures from the algorithms source code.\n> \n> Add a new function in the Raspberry Pi IPA to populate the generic statistics\n> structure from the values provided by the hardware in the bcm2835_isp_stats\n> structure.\n> \n> Update the Lux, AWB, AGC, ALSC, Contrast, and Focus algorithms to use the\n> generic statistics structure appropriately in their calculations. Additionally,\n> remove references to any hardware specific headers and defines in these source\n> files.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/ipa/raspberrypi/controller/controller.h | 4 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 27 +++++-------\n> src/ipa/raspberrypi/controller/rpi/agc.h | 2 +-\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 32 +++++++-------\n> src/ipa/raspberrypi/controller/rpi/alsc.h | 3 +-\n> src/ipa/raspberrypi/controller/rpi/awb.cpp | 20 ++++-----\n> src/ipa/raspberrypi/controller/rpi/awb.h | 1 +\n> .../raspberrypi/controller/rpi/contrast.cpp | 8 ++--\n> src/ipa/raspberrypi/controller/rpi/focus.cpp | 7 ++-\n> src/ipa/raspberrypi/controller/rpi/lux.cpp | 14 +-----\n> src/ipa/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++++++-\n> src/ipa/raspberrypi/statistics.h | 2 +\n> 12 files changed, 96 insertions(+), 68 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/controller.h b/src/ipa/raspberrypi/controller/controller.h\n> index 3e1e051703b3..e6c950c3a509 100644\n> --- a/src/ipa/raspberrypi/controller/controller.h\n> +++ b/src/ipa/raspberrypi/controller/controller.h\n> @@ -15,19 +15,17 @@\n> #include \n> #include \n> \n> -#include \n> -\n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"camera_mode.h\"\n> #include \"device_status.h\"\n> #include \"metadata.h\"\n> +#include \"statistics.h\"\n> \n> namespace RPiController {\n> \n> class Algorithm;\n> typedef std::unique_ptr AlgorithmPtr;\n> -typedef std::shared_ptr StatisticsPtr;\n> \n> /*\n> * The Controller holds a pointer to some global_metadata, which is how\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 46dcc81ae14c..868c30f03d66 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -9,8 +9,6 @@\n> #include \n> #include \n> \n> -#include \n> -\n> #include \n> \n> #include \"../awb_status.h\"\n> @@ -451,7 +449,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> fetchCurrentExposure(imageMetadata);\n> /* Compute the total gain we require relative to the current exposure. */\n> double gain, targetY;\n> - computeGain(stats.get(), imageMetadata, gain, targetY);\n> + computeGain(stats, imageMetadata, gain, targetY);\n> /* Now compute the target (final) exposure which we think we want. */\n> computeTargetExposure(gain);\n> /*\n> @@ -585,24 +583,23 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)\n> LOG(RPiAgc, Debug) << \"No AWB status found\";\n> }\n> \n> -static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,\n> +static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> double weights[], double gain)\n> {\n> - bcm2835_isp_stats_region *regions = stats->agc_stats;\n> /*\n> * Note how the calculation below means that equal weights give you\n> * \"average\" metering (i.e. all pixels equally important).\n> */\n> double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> - for (unsigned int i = 0; i < AgcStatsSize; i++) {\n> - double counted = regions[i].counted;\n> - double rAcc = std::min(regions[i].r_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> - double gAcc = std::min(regions[i].g_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> - double bAcc = std::min(regions[i].b_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> + for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {\n> + auto ®ion = stats->agcRegions.get(i);\n> + double rAcc = std::min(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> + double gAcc = std::min(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> + double bAcc = std::min(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> rSum += rAcc * weights[i];\n> gSum += gAcc * weights[i];\n> bSum += bAcc * weights[i];\n> - pixelSum += counted * weights[i];\n> + pixelSum += region.counted * weights[i];\n> }\n> if (pixelSum == 0.0) {\n> LOG(RPiAgc, Warning) << \"computeInitialY: pixelSum is zero\";\n> @@ -624,23 +621,23 @@ static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,\n> \n> static constexpr double EvGainYTargetLimit = 0.9;\n> \n> -static double constraintComputeGain(AgcConstraint &c, Histogram &h, double lux,\n> +static double constraintComputeGain(AgcConstraint &c, const Histogram &h, double lux,\n> double evGain, double &targetY)\n> {\n> targetY = c.yTarget.eval(c.yTarget.domain().clip(lux));\n> targetY = std::min(EvGainYTargetLimit, targetY * evGain);\n> double iqm = h.interQuantileMean(c.qLo, c.qHi);\n> - return (targetY * NUM_HISTOGRAM_BINS) / iqm;\n> + return (targetY * h.bins()) / iqm;\n> }\n> \n> -void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,\n> +void Agc::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> double &gain, double &targetY)\n> {\n> struct LuxStatus lux = {};\n> lux.lux = 400; /* default lux level to 400 in case no metadata found */\n> if (imageMetadata->get(\"lux.status\", lux) != 0)\n> LOG(RPiAgc, Warning) << \"No lux level found\";\n> - Histogram h(statistics->hist[0].g_hist, NUM_HISTOGRAM_BINS);\n> + const Histogram &h = statistics->yHist;\n> double evGain = status_.ev * config_.baseEv;\n> /*\n> * The initial gain and target_Y come from some of the regions. After\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h\n> index cf04da1973f1..f04896ca25ad 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> @@ -96,7 +96,7 @@ private:\n> void housekeepConfig();\n> void fetchCurrentExposure(Metadata *imageMetadata);\n> void fetchAwbStatus(Metadata *imageMetadata);\n> - void computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,\n> + void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> double &gain, double &targetY);\n> void computeTargetExposure(double gain);\n> bool applyDigitalGain(double gain, double targetY);\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index a4afaf841c41..eb4e2f9496e1 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -310,19 +310,21 @@ double getCt(Metadata *metadata, double defaultCt)\n> return awbStatus.temperatureK;\n> }\n> \n> -static void copyStats(bcm2835_isp_stats_region regions[XY], StatisticsPtr &stats,\n> +static void copyStats(RgbyRegions ®ions, StatisticsPtr &stats,\n> AlscStatus const &status)\n> {\n> - bcm2835_isp_stats_region *inputRegions = stats->awb_stats;\n> + if (!regions.numRegions())\n> + regions.init(stats->awbRegions.size());\n> +\n> double *rTable = (double *)status.r;\n> double *gTable = (double *)status.g;\n> double *bTable = (double *)status.b;\n> - for (int i = 0; i < XY; i++) {\n> - regions[i].r_sum = inputRegions[i].r_sum / rTable[i];\n> - regions[i].g_sum = inputRegions[i].g_sum / gTable[i];\n> - regions[i].b_sum = inputRegions[i].b_sum / bTable[i];\n> - regions[i].counted = inputRegions[i].counted;\n> - /* (don't care about the uncounted value) */\n> + for (unsigned int i = 0; i < stats->awbRegions.numRegions(); i++) {\n> + auto r = stats->awbRegions.get(i);\n> + r.val.rSum = static_cast(r.val.rSum / rTable[i]);\n> + r.val.gSum = static_cast(r.val.gSum / gTable[i]);\n> + r.val.bSum = static_cast(r.val.bSum / bTable[i]);\n> + regions.set(i, r);\n> }\n> }\n> \n> @@ -512,19 +514,19 @@ void resampleCalTable(double const calTableIn[XY],\n> }\n> \n> /* Calculate chrominance statistics (R/G and B/G) for each region. */\n> -static_assert(XY == AWB_REGIONS, \"ALSC/AWB statistics region mismatch\");\n> -static void calculateCrCb(bcm2835_isp_stats_region *awbRegion, double cr[XY],\n> +static void calculateCrCb(const RgbyRegions &awbRegion, double cr[XY],\n> double cb[XY], uint32_t minCount, uint16_t minG)\n> {\n> for (int i = 0; i < XY; i++) {\n> - bcm2835_isp_stats_region &zone = awbRegion[i];\n> - if (zone.counted <= minCount ||\n> - zone.g_sum / zone.counted <= minG) {\n> + auto s = awbRegion.get(i);\n> +\n> + if (s.counted <= minCount || s.val.gSum / s.counted <= minG) {\n> cr[i] = cb[i] = InsufficientData;\n> continue;\n> }\n> - cr[i] = zone.r_sum / (double)zone.g_sum;\n> - cb[i] = zone.b_sum / (double)zone.g_sum;\n> +\n> + cr[i] = s.val.rSum / (double)s.val.gSum;\n> + cb[i] = s.val.bSum / (double)s.val.gSum;\n> }\n> }\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> index a858ef5a6551..9167c9ffa2e3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> @@ -12,6 +12,7 @@\n> \n> #include \"../algorithm.h\"\n> #include \"../alsc_status.h\"\n> +#include \"../statistics.h\"\n> \n> namespace RPiController {\n> \n> @@ -98,7 +99,7 @@ private:\n> /* copy out the results from the async thread so that it can be restarted */\n> void fetchAsyncResults();\n> double ct_;\n> - bcm2835_isp_stats_region statistics_[AlscCellsY * AlscCellsX];\n> + RgbyRegions statistics_;\n> double asyncResults_[3][AlscCellsY][AlscCellsX];\n> double asyncLambdaR_[AlscCellsX * AlscCellsY];\n> double asyncLambdaB_[AlscCellsX * AlscCellsY];\n> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> index 04d1c8783654..ef3435d66106 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> @@ -21,9 +21,6 @@ LOG_DEFINE_CATEGORY(RPiAwb)\n> \n> #define NAME \"rpi.awb\"\n> \n> -static constexpr unsigned int AwbStatsSizeX = DEFAULT_AWB_REGIONS_X;\n> -static constexpr unsigned int AwbStatsSizeY = DEFAULT_AWB_REGIONS_Y;\n> -\n> /*\n> * todo - the locking in this algorithm needs some tidying up as has been done\n> * elsewhere (ALSC and AGC).\n> @@ -401,17 +398,16 @@ void Awb::asyncFunc()\n> }\n> \n> static void generateStats(std::vector &zones,\n> - bcm2835_isp_stats_region *stats, double minPixels,\n> + RgbyRegions &stats, double minPixels,\n> double minG)\n> {\n> - for (unsigned int i = 0; i < AwbStatsSizeX * AwbStatsSizeY; i++) {\n> + for (auto const ®ion : stats) {\n> Awb::RGB zone;\n> - double counted = stats[i].counted;\n> - if (counted >= minPixels) {\n> - zone.G = stats[i].g_sum / counted;\n> + if (region.counted >= minPixels) {\n> + zone.G = region.val.gSum / region.counted;\n> if (zone.G >= minG) {\n> - zone.R = stats[i].r_sum / counted;\n> - zone.B = stats[i].b_sum / counted;\n> + zone.R = region.val.rSum / region.counted;\n> + zone.B = region.val.bSum / region.counted;\n> zones.push_back(zone);\n> }\n> }\n> @@ -425,7 +421,7 @@ void Awb::prepareStats()\n> * LSC has already been applied to the stats in this pipeline, so stop\n> * any LSC compensation. We also ignore config_.fast in this version.\n> */\n> - generateStats(zones_, statistics_->awb_stats, config_.minPixels,\n> + generateStats(zones_, statistics_->awbRegions, config_.minPixels,\n> config_.minG);\n> /*\n> * apply sensitivities, so values appear to come from our \"canonical\"\n> @@ -641,7 +637,7 @@ void Awb::awbBayes()\n> * valid... not entirely sure about this.\n> */\n> Pwl prior = interpolatePrior();\n> - prior *= zones_.size() / (double)(AwbStatsSizeX * AwbStatsSizeY);\n> + prior *= zones_.size() / (double)(statistics_->awbRegions.numRegions());\n> prior.map([](double x, double y) {\n> LOG(RPiAwb, Debug) << \"(\" << x << \",\" << y << \")\";\n> });\n> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h b/src/ipa/raspberrypi/controller/rpi/awb.h\n> index 2254c3ed2cc4..e7d49cd8036b 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/awb.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/awb.h\n> @@ -13,6 +13,7 @@\n> #include \"../awb_algorithm.h\"\n> #include \"../pwl.h\"\n> #include \"../awb_status.h\"\n> +#include \"../statistics.h\"\n> \n> namespace RPiController {\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/contrast.cpp b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> index 5b37edcbd46a..a4f8c4f04fc4 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> @@ -106,7 +106,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> * bit.\n> */\n> double histLo = histogram.quantile(config.loHistogram) *\n> - (65536 / NUM_HISTOGRAM_BINS);\n> + (65536 / histogram.bins());\n> double levelLo = config.loLevel * 65536;\n> LOG(RPiContrast, Debug)\n> << \"Move histogram point \" << histLo << \" to \" << levelLo;\n> @@ -119,7 +119,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> * Keep the mid-point (median) in the same place, though, to limit the\n> * apparent amount of global brightness shift.\n> */\n> - double mid = histogram.quantile(0.5) * (65536 / NUM_HISTOGRAM_BINS);\n> + double mid = histogram.quantile(0.5) * (65536 / histogram.bins());\n> enhance.append(mid, mid);\n> \n> /*\n> @@ -127,7 +127,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> * there up.\n> */\n> double histHi = histogram.quantile(config.hiHistogram) *\n> - (65536 / NUM_HISTOGRAM_BINS);\n> + (65536 / histogram.bins());\n> double levelHi = config.hiLevel * 65536;\n> LOG(RPiContrast, Debug)\n> << \"Move histogram point \" << histHi << \" to \" << levelHi;\n> @@ -158,7 +158,7 @@ Pwl applyManualContrast(Pwl const &gammaCurve, double brightness,\n> void Contrast::process(StatisticsPtr &stats,\n> [[maybe_unused]] Metadata *imageMetadata)\n> {\n> - Histogram histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);\n> + Histogram &histogram = stats->yHist;\n> /*\n> * We look at the histogram and adjust the gamma curve in the following\n> * ways: 1. Adjust the gamma curve so as to pull the start of the\n> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> index 8c5029bd0e95..ea3cc00e42c3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> @@ -31,10 +31,9 @@ char const *Focus::name() const\n> void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> {\n> FocusStatus status;\n> - unsigned int i;\n> - for (i = 0; i < FOCUS_REGIONS; i++)\n> - status.focusMeasures[i] = stats->focus_stats[i].contrast_val[1][1] / 1000;\n> - status.num = i;\n> + for (unsigned int i = 0; i < stats->focusRegions.numRegions(); i++)\n> + status.focusMeasures[i] = stats->focusRegions.get(i).val;\n> + status.num = stats->focusRegions.numRegions();\n> imageMetadata->set(\"focus.status\", status);\n> \n> LOG(RPiFocus, Debug)\n> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> index 9759186afacf..06625f3a5ea3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> @@ -6,8 +6,6 @@\n> */\n> #include \n> \n> -#include \n> -\n> #include \n> \n> #include \"../device_status.h\"\n> @@ -83,20 +81,12 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> if (imageMetadata->get(\"device.status\", deviceStatus) == 0) {\n> double currentGain = deviceStatus.analogueGain;\n> double currentAperture = deviceStatus.aperture.value_or(currentAperture_);\n> - uint64_t sum = 0;\n> - uint32_t num = 0;\n> - uint32_t *bin = stats->hist[0].g_hist;\n> - const int numBins = sizeof(stats->hist[0].g_hist) /\n> - sizeof(stats->hist[0].g_hist[0]);\n> - for (int i = 0; i < numBins; i++)\n> - sum += bin[i] * (uint64_t)i, num += bin[i];\n> - /* add .5 to reflect the mid-points of bins */\n> - double currentY = sum / (double)num + .5;\n> + double currentY = stats->yHist.interQuantileMean(0, 1);\n> double gainRatio = referenceGain_ / currentGain;\n> double shutterSpeedRatio =\n> referenceShutterSpeed_ / deviceStatus.shutterSpeed;\n> double apertureRatio = referenceAperture_ / currentAperture;\n> - double yRatio = currentY * (65536 / numBins) / referenceY_;\n> + double yRatio = currentY * (65536 / stats->yHist.bins()) / referenceY_;\n> double estimatedLux = shutterSpeedRatio * gainRatio *\n> apertureRatio * apertureRatio *\n> yRatio * referenceLux_;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index bead436def3c..cff079bbafb3 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -51,6 +51,7 @@\n> #include \"metadata.h\"\n> #include \"sharpen_algorithm.h\"\n> #include \"sharpen_status.h\"\n> +#include \"statistics.h\"\n> \n> namespace libcamera {\n> \n> @@ -139,6 +140,7 @@ private:\n> void prepareISP(const ISPConfig &data);\n> void reportMetadata(unsigned int ipaContext);\n> void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> + RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> void processStats(unsigned int bufferId, unsigned int ipaContext);\n> void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> @@ -1141,6 +1143,46 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls, unsigned int ip\n> rpiMetadata_[ipaContext].set(\"device.status\", deviceStatus);\n> }\n> \n> +RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) const\n> +{\n> + using namespace RPiController;\n> +\n> + unsigned int i;\n> + StatisticsPtr statistics =\n> + std::make_unique(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc);\n> +\n> + /* RGB histograms are not used, so do not populate them. */\n> + statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));\n> +\n> + statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> + for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,\n> + stats->awb_stats[i].g_sum,\n> + stats->awb_stats[i].b_sum },\n> + stats->awb_stats[i].counted,\n> + stats->awb_stats[i].notcounted });\n> +\n> + /*\n> + * There are only ever 15 regions computed by the firmware due to zoning,\n> + * but the HW defines AGC_REGIONS == 16!\n> + */\n> + statistics->agcRegions.init(15);\n> + for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,\n> + stats->agc_stats[i].g_sum,\n> + stats->agc_stats[i].b_sum },\n> + stats->agc_stats[i].counted,\n> + stats->awb_stats[i].notcounted });\n> +\n> + statistics->focusRegions.init({ 4, 3 });\n> + for (i = 0; i < statistics->focusRegions.numRegions(); i++)\n> + statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,\n> + stats->focus_stats[i].contrast_val_num[1][1],\n> + stats->focus_stats[i].contrast_val_num[1][0] });\n> +\n> + return statistics;\n> +}\n> +\n> void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> {\n> RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> @@ -1153,7 +1195,7 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> \n> Span mem = it->second.planes()[0];\n> bcm2835_isp_stats *stats = reinterpret_cast(mem.data());\n> - RPiController::StatisticsPtr statistics = std::make_shared(*stats);\n> + RPiController::StatisticsPtr statistics = fillStatistics(stats);\n> helper_->process(statistics, rpiMetadata);\n> controller_.process(statistics, &rpiMetadata);\n> \n> diff --git a/src/ipa/raspberrypi/statistics.h b/src/ipa/raspberrypi/statistics.h\n> index a762bf3d41aa..affb7272c963 100644\n> --- a/src/ipa/raspberrypi/statistics.h\n> +++ b/src/ipa/raspberrypi/statistics.h\n> @@ -67,4 +67,6 @@ struct Statistics {\n> FocusRegions focusRegions;\n> };\n> \n> +using StatisticsPtr = std::shared_ptr;\n> +\n> } /* namespace RPiController */\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B91F9BDC71\n\tfor ;\n\tMon, 30 Jan 2023 13:24:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1608F625D8;\n\tMon, 30 Jan 2023 14:24:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB64460482\n\tfor ;\n\tMon, 30 Jan 2023 14:24:08 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 14BFF8B8;\n\tMon, 30 Jan 2023 14:24:08 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675085051;\n\tbh=ifvmhjSf72T7HAWYSZmAx/1VCEtNdzUhWwubBtgz2OA=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=Y6uNO8ub+A8h8a3v35kfW7LRO5kpQTjWdEK+E0Xd9OIVDKssw8GT8rAtjU8Ia7Ov+\n\toln3tk5KnoupVVNkaNugueTcMThPAWLgugdAOyYQqiBz0rO8bkzCzvwm4BEu+XEQpM\n\tgjMUA0NSE3ksXCG2VM5ph2fqdeNuWymjGVKtF2N0pk0bJIXmU3OyYfk3uAiOL6gDnx\n\tLOzNecOhqw977gDKWwxeIlhThm0J9P8iGMn8YJ5Ul2fhv2c27qOKZiYo/hrhbhy9tq\n\tcXSmI6sDdsLsJi7JxZM136Sclqmphtvo0yX7nZ+VIU0QerREuy5GOJOaX882qmoumI\n\t8T4c1CUmoWUBQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675085048;\n\tbh=ifvmhjSf72T7HAWYSZmAx/1VCEtNdzUhWwubBtgz2OA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=VwtygOY+xx2uN6+UekwDjtGFDkTQV/AOGtkU6dRNJkzDrVSCKVwrZcUVxxB2Rbyck\n\tooClKSNlzG/fdVtvn797pngoeudmuy3lF5Nkucesk7HJV9SS7fhWh8gJsA2sQK8h12\n\tKlVugACqdj2Rf/zCfu9xBijrig52zDq/3DjzI3qw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VwtygOY+\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221213114836.15473-5-naush@raspberrypi.com>","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-5-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 30 Jan 2023 13:24:05 +0000","Message-ID":"<167508504524.42371.97045623736344505@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] ipa: raspberrypi: Use the\n\tgeneric statistics structure in the algorithms","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26393,"web_url":"https://patchwork.libcamera.org/comment/26393/","msgid":"<167518450581.42371.12169486428273034452@Monstersaurus>","date":"2023-01-31T17:01:45","subject":"Re: [libcamera-devel] [PATCH v3 4/5] ipa: raspberrypi: Use the\n\tgeneric statistics structure in the algorithms","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nQuoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:35)\n> Repurpose the StatisticsPtr type from being a shared_ptr to\n> shared_ptr. This removes any hardware specific header\n> files and structures from the algorithms source code.\n> \n> Add a new function in the Raspberry Pi IPA to populate the generic statistics\n> structure from the values provided by the hardware in the bcm2835_isp_stats\n> structure.\n> \n> Update the Lux, AWB, AGC, ALSC, Contrast, and Focus algorithms to use the\n> generic statistics structure appropriately in their calculations. Additionally,\n> remove references to any hardware specific headers and defines in these source\n> files.\n\nI'm afraid this one fails to build as I've merged IMX708 support with\nthe AF algorithm additions.\n\nCould you rebase this series please?\n\n\n[5/119] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o\nFAILED: src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o\nc++ -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\nIn file included from ../../src/ipa/raspberrypi/controller/rpi/af.cpp:8:\n../../src/ipa/raspberrypi/controller/rpi/af.h:120:77: error: ‘FOCUS_REGIONS’ was not declared in this scope\n 120 | double getContrast(struct bcm2835_isp_stats_focus const focus_stats[FOCUS_REGIONS]) const;\n | ^~~~~~~~~~~~~\n../../src/ipa/raspberrypi/controller/rpi/af.h:141:35: error: ‘FOCUS_REGIONS’ was not declared in this scope\n 141 | uint16_t contrastWeights_[FOCUS_REGIONS];\n | ^~~~~~~~~~~~~\n../../src/ipa/raspberrypi/controller/rpi/af.cpp: In constructor ‘RPiController::Af::Af(RPiController::Controller*)’:\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:178:11: error: class ‘RPiController::Af’ does not have any field named ‘contrastWeights_’\n 178 | contrastWeights_{},\n | ^~~~~~~~~~~~~~~~\n../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘void RPiController::Af::computeWeights()’:\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:300:23: error: ‘FOCUS_REGIONS’ was not declared in this scope\n 300 | static_assert(FOCUS_REGIONS == FocusStatsRows * FocusStatsCols);\n | ^~~~~~~~~~~~~\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:313:25: error: ‘contrastWeights_’ was not declared in this scope; did you mean ‘phaseWeights_’?\n 313 | contrastWeights_[FocusStatsCols * i + j] = w;\n | ^~~~~~~~~~~~~~~~\n | phaseWeights_\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:316:38: error: ‘contrastWeights_’ was not declared in this scope; did you mean ‘phaseWeights_’?\n 316 | << contrastWeights_[FocusStatsCols * i + 0] << \" \"\n | ^~~~~~~~~~~~~~~~\n | phaseWeights_\n../../src/ipa/raspberrypi/controller/rpi/af.cpp: At global scope:\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:355:73: error: ‘FOCUS_REGIONS’ was not declared in this scope\n 355 | double Af::getContrast(struct bcm2835_isp_stats_focus const focus_stats[FOCUS_REGIONS]) const\n | ^~~~~~~~~~~~~\n../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘double RPiController::Af::getContrast(...) const’:\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:359:34: error: ‘FOCUS_REGIONS’ was not declared in this scope\n 359 | for (unsigned i = 0; i < FOCUS_REGIONS; ++i) {\n | ^~~~~~~~~~~~~\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:360:30: error: ‘contrastWeights_’ was not declared in this scope; did you mean ‘phaseWeights_’?\n 360 | unsigned w = contrastWeights_[i];\n | ^~~~~~~~~~~~~~~~\n | phaseWeights_\n../../src/ipa/raspberrypi/controller/rpi/af.cpp:361:31: error: ‘focus_stats’ was not declared in this scope\n 361 | sumWc += w * (focus_stats[i].contrast_val[1][1] >> 10);\n | ^~~~~~~~~~~\n../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘virtual void RPiController::Af::process(RPiController::StatisticsPtr&, RPiController::Metadata*)’:\n../../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’\n 669 | prevContrast_ = getContrast(stats->focus_stats);\n | ^~~~~~~~~~~\n[12/119] Compiling C++ object src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o\nFAILED: src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o\nc++ -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\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp: In member function ‘void CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)’:\n../../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’\n 332 | memcpy(stats->hist[0].g_hist, aeHistLinear_, sizeof(stats->hist[0].g_hist));\n | ^~~~\n../../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’\n 332 | memcpy(stats->hist[0].g_hist, aeHistLinear_, sizeof(stats->hist[0].g_hist));\n | ^~~~\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:336:29: error: ‘AGC_REGIONS’ was not declared in this scope\n 336 | for (int i = 0; i < AGC_REGIONS; i++) {\n | ^~~~~~~~~~~\n../../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’\n 337 | struct bcm2835_isp_stats_region &r = stats->agc_stats[i];\n | ^~~~~~~~~\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:17: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n | ^\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 337 | struct bcm2835_isp_stats_region &r = stats->agc_stats[i];\n | ^~~~~~~~~~~~~~~~~~~~~~~~\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:27: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n | ^\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 337 | struct bcm2835_isp_stats_region &r = stats->agc_stats[i];\n | ^~~~~~~~~~~~~~~~~~~~~~~~\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:37: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n | ^\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 337 | struct bcm2835_isp_stats_region &r = stats->agc_stats[i];\n | ^~~~~~~~~~~~~~~~~~~~~~~~\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:47: error: invalid use of incomplete type ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n | ^\n../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward declaration of ‘struct CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n 337 | struct bcm2835_isp_stats_region &r = stats->agc_stats[i];\n | ^~~~~~~~~~~~~~~~~~~~~~~~\n\n--\nKieran\n\n\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/controller.h | 4 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 27 +++++-------\n> src/ipa/raspberrypi/controller/rpi/agc.h | 2 +-\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 32 +++++++-------\n> src/ipa/raspberrypi/controller/rpi/alsc.h | 3 +-\n> src/ipa/raspberrypi/controller/rpi/awb.cpp | 20 ++++-----\n> src/ipa/raspberrypi/controller/rpi/awb.h | 1 +\n> .../raspberrypi/controller/rpi/contrast.cpp | 8 ++--\n> src/ipa/raspberrypi/controller/rpi/focus.cpp | 7 ++-\n> src/ipa/raspberrypi/controller/rpi/lux.cpp | 14 +-----\n> src/ipa/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++++++-\n> src/ipa/raspberrypi/statistics.h | 2 +\n> 12 files changed, 96 insertions(+), 68 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/controller.h b/src/ipa/raspberrypi/controller/controller.h\n> index 3e1e051703b3..e6c950c3a509 100644\n> --- a/src/ipa/raspberrypi/controller/controller.h\n> +++ b/src/ipa/raspberrypi/controller/controller.h\n> @@ -15,19 +15,17 @@\n> #include \n> #include \n> \n> -#include \n> -\n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"camera_mode.h\"\n> #include \"device_status.h\"\n> #include \"metadata.h\"\n> +#include \"statistics.h\"\n> \n> namespace RPiController {\n> \n> class Algorithm;\n> typedef std::unique_ptr AlgorithmPtr;\n> -typedef std::shared_ptr StatisticsPtr;\n> \n> /*\n> * The Controller holds a pointer to some global_metadata, which is how\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 46dcc81ae14c..868c30f03d66 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -9,8 +9,6 @@\n> #include \n> #include \n> \n> -#include \n> -\n> #include \n> \n> #include \"../awb_status.h\"\n> @@ -451,7 +449,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> fetchCurrentExposure(imageMetadata);\n> /* Compute the total gain we require relative to the current exposure. */\n> double gain, targetY;\n> - computeGain(stats.get(), imageMetadata, gain, targetY);\n> + computeGain(stats, imageMetadata, gain, targetY);\n> /* Now compute the target (final) exposure which we think we want. */\n> computeTargetExposure(gain);\n> /*\n> @@ -585,24 +583,23 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)\n> LOG(RPiAgc, Debug) << \"No AWB status found\";\n> }\n> \n> -static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,\n> +static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb,\n> double weights[], double gain)\n> {\n> - bcm2835_isp_stats_region *regions = stats->agc_stats;\n> /*\n> * Note how the calculation below means that equal weights give you\n> * \"average\" metering (i.e. all pixels equally important).\n> */\n> double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> - for (unsigned int i = 0; i < AgcStatsSize; i++) {\n> - double counted = regions[i].counted;\n> - double rAcc = std::min(regions[i].r_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> - double gAcc = std::min(regions[i].g_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> - double bAcc = std::min(regions[i].b_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> + for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) {\n> + auto ®ion = stats->agcRegions.get(i);\n> + double rAcc = std::min(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> + double gAcc = std::min(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> + double bAcc = std::min(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted);\n> rSum += rAcc * weights[i];\n> gSum += gAcc * weights[i];\n> bSum += bAcc * weights[i];\n> - pixelSum += counted * weights[i];\n> + pixelSum += region.counted * weights[i];\n> }\n> if (pixelSum == 0.0) {\n> LOG(RPiAgc, Warning) << \"computeInitialY: pixelSum is zero\";\n> @@ -624,23 +621,23 @@ static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,\n> \n> static constexpr double EvGainYTargetLimit = 0.9;\n> \n> -static double constraintComputeGain(AgcConstraint &c, Histogram &h, double lux,\n> +static double constraintComputeGain(AgcConstraint &c, const Histogram &h, double lux,\n> double evGain, double &targetY)\n> {\n> targetY = c.yTarget.eval(c.yTarget.domain().clip(lux));\n> targetY = std::min(EvGainYTargetLimit, targetY * evGain);\n> double iqm = h.interQuantileMean(c.qLo, c.qHi);\n> - return (targetY * NUM_HISTOGRAM_BINS) / iqm;\n> + return (targetY * h.bins()) / iqm;\n> }\n> \n> -void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,\n> +void Agc::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> double &gain, double &targetY)\n> {\n> struct LuxStatus lux = {};\n> lux.lux = 400; /* default lux level to 400 in case no metadata found */\n> if (imageMetadata->get(\"lux.status\", lux) != 0)\n> LOG(RPiAgc, Warning) << \"No lux level found\";\n> - Histogram h(statistics->hist[0].g_hist, NUM_HISTOGRAM_BINS);\n> + const Histogram &h = statistics->yHist;\n> double evGain = status_.ev * config_.baseEv;\n> /*\n> * The initial gain and target_Y come from some of the regions. After\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h\n> index cf04da1973f1..f04896ca25ad 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> @@ -96,7 +96,7 @@ private:\n> void housekeepConfig();\n> void fetchCurrentExposure(Metadata *imageMetadata);\n> void fetchAwbStatus(Metadata *imageMetadata);\n> - void computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,\n> + void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> double &gain, double &targetY);\n> void computeTargetExposure(double gain);\n> bool applyDigitalGain(double gain, double targetY);\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index a4afaf841c41..eb4e2f9496e1 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -310,19 +310,21 @@ double getCt(Metadata *metadata, double defaultCt)\n> return awbStatus.temperatureK;\n> }\n> \n> -static void copyStats(bcm2835_isp_stats_region regions[XY], StatisticsPtr &stats,\n> +static void copyStats(RgbyRegions ®ions, StatisticsPtr &stats,\n> AlscStatus const &status)\n> {\n> - bcm2835_isp_stats_region *inputRegions = stats->awb_stats;\n> + if (!regions.numRegions())\n> + regions.init(stats->awbRegions.size());\n> +\n> double *rTable = (double *)status.r;\n> double *gTable = (double *)status.g;\n> double *bTable = (double *)status.b;\n> - for (int i = 0; i < XY; i++) {\n> - regions[i].r_sum = inputRegions[i].r_sum / rTable[i];\n> - regions[i].g_sum = inputRegions[i].g_sum / gTable[i];\n> - regions[i].b_sum = inputRegions[i].b_sum / bTable[i];\n> - regions[i].counted = inputRegions[i].counted;\n> - /* (don't care about the uncounted value) */\n> + for (unsigned int i = 0; i < stats->awbRegions.numRegions(); i++) {\n> + auto r = stats->awbRegions.get(i);\n> + r.val.rSum = static_cast(r.val.rSum / rTable[i]);\n> + r.val.gSum = static_cast(r.val.gSum / gTable[i]);\n> + r.val.bSum = static_cast(r.val.bSum / bTable[i]);\n> + regions.set(i, r);\n> }\n> }\n> \n> @@ -512,19 +514,19 @@ void resampleCalTable(double const calTableIn[XY],\n> }\n> \n> /* Calculate chrominance statistics (R/G and B/G) for each region. */\n> -static_assert(XY == AWB_REGIONS, \"ALSC/AWB statistics region mismatch\");\n> -static void calculateCrCb(bcm2835_isp_stats_region *awbRegion, double cr[XY],\n> +static void calculateCrCb(const RgbyRegions &awbRegion, double cr[XY],\n> double cb[XY], uint32_t minCount, uint16_t minG)\n> {\n> for (int i = 0; i < XY; i++) {\n> - bcm2835_isp_stats_region &zone = awbRegion[i];\n> - if (zone.counted <= minCount ||\n> - zone.g_sum / zone.counted <= minG) {\n> + auto s = awbRegion.get(i);\n> +\n> + if (s.counted <= minCount || s.val.gSum / s.counted <= minG) {\n> cr[i] = cb[i] = InsufficientData;\n> continue;\n> }\n> - cr[i] = zone.r_sum / (double)zone.g_sum;\n> - cb[i] = zone.b_sum / (double)zone.g_sum;\n> +\n> + cr[i] = s.val.rSum / (double)s.val.gSum;\n> + cb[i] = s.val.bSum / (double)s.val.gSum;\n> }\n> }\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> index a858ef5a6551..9167c9ffa2e3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> @@ -12,6 +12,7 @@\n> \n> #include \"../algorithm.h\"\n> #include \"../alsc_status.h\"\n> +#include \"../statistics.h\"\n> \n> namespace RPiController {\n> \n> @@ -98,7 +99,7 @@ private:\n> /* copy out the results from the async thread so that it can be restarted */\n> void fetchAsyncResults();\n> double ct_;\n> - bcm2835_isp_stats_region statistics_[AlscCellsY * AlscCellsX];\n> + RgbyRegions statistics_;\n> double asyncResults_[3][AlscCellsY][AlscCellsX];\n> double asyncLambdaR_[AlscCellsX * AlscCellsY];\n> double asyncLambdaB_[AlscCellsX * AlscCellsY];\n> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> index 04d1c8783654..ef3435d66106 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> @@ -21,9 +21,6 @@ LOG_DEFINE_CATEGORY(RPiAwb)\n> \n> #define NAME \"rpi.awb\"\n> \n> -static constexpr unsigned int AwbStatsSizeX = DEFAULT_AWB_REGIONS_X;\n> -static constexpr unsigned int AwbStatsSizeY = DEFAULT_AWB_REGIONS_Y;\n> -\n> /*\n> * todo - the locking in this algorithm needs some tidying up as has been done\n> * elsewhere (ALSC and AGC).\n> @@ -401,17 +398,16 @@ void Awb::asyncFunc()\n> }\n> \n> static void generateStats(std::vector &zones,\n> - bcm2835_isp_stats_region *stats, double minPixels,\n> + RgbyRegions &stats, double minPixels,\n> double minG)\n> {\n> - for (unsigned int i = 0; i < AwbStatsSizeX * AwbStatsSizeY; i++) {\n> + for (auto const ®ion : stats) {\n> Awb::RGB zone;\n> - double counted = stats[i].counted;\n> - if (counted >= minPixels) {\n> - zone.G = stats[i].g_sum / counted;\n> + if (region.counted >= minPixels) {\n> + zone.G = region.val.gSum / region.counted;\n> if (zone.G >= minG) {\n> - zone.R = stats[i].r_sum / counted;\n> - zone.B = stats[i].b_sum / counted;\n> + zone.R = region.val.rSum / region.counted;\n> + zone.B = region.val.bSum / region.counted;\n> zones.push_back(zone);\n> }\n> }\n> @@ -425,7 +421,7 @@ void Awb::prepareStats()\n> * LSC has already been applied to the stats in this pipeline, so stop\n> * any LSC compensation. We also ignore config_.fast in this version.\n> */\n> - generateStats(zones_, statistics_->awb_stats, config_.minPixels,\n> + generateStats(zones_, statistics_->awbRegions, config_.minPixels,\n> config_.minG);\n> /*\n> * apply sensitivities, so values appear to come from our \"canonical\"\n> @@ -641,7 +637,7 @@ void Awb::awbBayes()\n> * valid... not entirely sure about this.\n> */\n> Pwl prior = interpolatePrior();\n> - prior *= zones_.size() / (double)(AwbStatsSizeX * AwbStatsSizeY);\n> + prior *= zones_.size() / (double)(statistics_->awbRegions.numRegions());\n> prior.map([](double x, double y) {\n> LOG(RPiAwb, Debug) << \"(\" << x << \",\" << y << \")\";\n> });\n> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h b/src/ipa/raspberrypi/controller/rpi/awb.h\n> index 2254c3ed2cc4..e7d49cd8036b 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/awb.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/awb.h\n> @@ -13,6 +13,7 @@\n> #include \"../awb_algorithm.h\"\n> #include \"../pwl.h\"\n> #include \"../awb_status.h\"\n> +#include \"../statistics.h\"\n> \n> namespace RPiController {\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/contrast.cpp b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> index 5b37edcbd46a..a4f8c4f04fc4 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> @@ -106,7 +106,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> * bit.\n> */\n> double histLo = histogram.quantile(config.loHistogram) *\n> - (65536 / NUM_HISTOGRAM_BINS);\n> + (65536 / histogram.bins());\n> double levelLo = config.loLevel * 65536;\n> LOG(RPiContrast, Debug)\n> << \"Move histogram point \" << histLo << \" to \" << levelLo;\n> @@ -119,7 +119,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> * Keep the mid-point (median) in the same place, though, to limit the\n> * apparent amount of global brightness shift.\n> */\n> - double mid = histogram.quantile(0.5) * (65536 / NUM_HISTOGRAM_BINS);\n> + double mid = histogram.quantile(0.5) * (65536 / histogram.bins());\n> enhance.append(mid, mid);\n> \n> /*\n> @@ -127,7 +127,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> * there up.\n> */\n> double histHi = histogram.quantile(config.hiHistogram) *\n> - (65536 / NUM_HISTOGRAM_BINS);\n> + (65536 / histogram.bins());\n> double levelHi = config.hiLevel * 65536;\n> LOG(RPiContrast, Debug)\n> << \"Move histogram point \" << histHi << \" to \" << levelHi;\n> @@ -158,7 +158,7 @@ Pwl applyManualContrast(Pwl const &gammaCurve, double brightness,\n> void Contrast::process(StatisticsPtr &stats,\n> [[maybe_unused]] Metadata *imageMetadata)\n> {\n> - Histogram histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);\n> + Histogram &histogram = stats->yHist;\n> /*\n> * We look at the histogram and adjust the gamma curve in the following\n> * ways: 1. Adjust the gamma curve so as to pull the start of the\n> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> index 8c5029bd0e95..ea3cc00e42c3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> @@ -31,10 +31,9 @@ char const *Focus::name() const\n> void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> {\n> FocusStatus status;\n> - unsigned int i;\n> - for (i = 0; i < FOCUS_REGIONS; i++)\n> - status.focusMeasures[i] = stats->focus_stats[i].contrast_val[1][1] / 1000;\n> - status.num = i;\n> + for (unsigned int i = 0; i < stats->focusRegions.numRegions(); i++)\n> + status.focusMeasures[i] = stats->focusRegions.get(i).val;\n> + status.num = stats->focusRegions.numRegions();\n> imageMetadata->set(\"focus.status\", status);\n> \n> LOG(RPiFocus, Debug)\n> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> index 9759186afacf..06625f3a5ea3 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> @@ -6,8 +6,6 @@\n> */\n> #include \n> \n> -#include \n> -\n> #include \n> \n> #include \"../device_status.h\"\n> @@ -83,20 +81,12 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> if (imageMetadata->get(\"device.status\", deviceStatus) == 0) {\n> double currentGain = deviceStatus.analogueGain;\n> double currentAperture = deviceStatus.aperture.value_or(currentAperture_);\n> - uint64_t sum = 0;\n> - uint32_t num = 0;\n> - uint32_t *bin = stats->hist[0].g_hist;\n> - const int numBins = sizeof(stats->hist[0].g_hist) /\n> - sizeof(stats->hist[0].g_hist[0]);\n> - for (int i = 0; i < numBins; i++)\n> - sum += bin[i] * (uint64_t)i, num += bin[i];\n> - /* add .5 to reflect the mid-points of bins */\n> - double currentY = sum / (double)num + .5;\n> + double currentY = stats->yHist.interQuantileMean(0, 1);\n> double gainRatio = referenceGain_ / currentGain;\n> double shutterSpeedRatio =\n> referenceShutterSpeed_ / deviceStatus.shutterSpeed;\n> double apertureRatio = referenceAperture_ / currentAperture;\n> - double yRatio = currentY * (65536 / numBins) / referenceY_;\n> + double yRatio = currentY * (65536 / stats->yHist.bins()) / referenceY_;\n> double estimatedLux = shutterSpeedRatio * gainRatio *\n> apertureRatio * apertureRatio *\n> yRatio * referenceLux_;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index bead436def3c..cff079bbafb3 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -51,6 +51,7 @@\n> #include \"metadata.h\"\n> #include \"sharpen_algorithm.h\"\n> #include \"sharpen_status.h\"\n> +#include \"statistics.h\"\n> \n> namespace libcamera {\n> \n> @@ -139,6 +140,7 @@ private:\n> void prepareISP(const ISPConfig &data);\n> void reportMetadata(unsigned int ipaContext);\n> void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> + RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> void processStats(unsigned int bufferId, unsigned int ipaContext);\n> void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> @@ -1141,6 +1143,46 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls, unsigned int ip\n> rpiMetadata_[ipaContext].set(\"device.status\", deviceStatus);\n> }\n> \n> +RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) const\n> +{\n> + using namespace RPiController;\n> +\n> + unsigned int i;\n> + StatisticsPtr statistics =\n> + std::make_unique(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc);\n> +\n> + /* RGB histograms are not used, so do not populate them. */\n> + statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS));\n> +\n> + statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> + for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum,\n> + stats->awb_stats[i].g_sum,\n> + stats->awb_stats[i].b_sum },\n> + stats->awb_stats[i].counted,\n> + stats->awb_stats[i].notcounted });\n> +\n> + /*\n> + * There are only ever 15 regions computed by the firmware due to zoning,\n> + * but the HW defines AGC_REGIONS == 16!\n> + */\n> + statistics->agcRegions.init(15);\n> + for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum,\n> + stats->agc_stats[i].g_sum,\n> + stats->agc_stats[i].b_sum },\n> + stats->agc_stats[i].counted,\n> + stats->awb_stats[i].notcounted });\n> +\n> + statistics->focusRegions.init({ 4, 3 });\n> + for (i = 0; i < statistics->focusRegions.numRegions(); i++)\n> + statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,\n> + stats->focus_stats[i].contrast_val_num[1][1],\n> + stats->focus_stats[i].contrast_val_num[1][0] });\n> +\n> + return statistics;\n> +}\n> +\n> void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> {\n> RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> @@ -1153,7 +1195,7 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> \n> Span mem = it->second.planes()[0];\n> bcm2835_isp_stats *stats = reinterpret_cast(mem.data());\n> - RPiController::StatisticsPtr statistics = std::make_shared(*stats);\n> + RPiController::StatisticsPtr statistics = fillStatistics(stats);\n> helper_->process(statistics, rpiMetadata);\n> controller_.process(statistics, &rpiMetadata);\n> \n> diff --git a/src/ipa/raspberrypi/statistics.h b/src/ipa/raspberrypi/statistics.h\n> index a762bf3d41aa..affb7272c963 100644\n> --- a/src/ipa/raspberrypi/statistics.h\n> +++ b/src/ipa/raspberrypi/statistics.h\n> @@ -67,4 +67,6 @@ struct Statistics {\n> FocusRegions focusRegions;\n> };\n> \n> +using StatisticsPtr = std::shared_ptr;\n> +\n> } /* namespace RPiController */\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EDE1BC323E\n\tfor ;\n\tTue, 31 Jan 2023 17:01:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32E68625D0;\n\tTue, 31 Jan 2023 18:01:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE7BD625D0\n\tfor ;\n\tTue, 31 Jan 2023 18:01:48 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38908D6;\n\tTue, 31 Jan 2023 18:01:48 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675184510;\n\tbh=6Cmptv7C43PTNHhfvCBAfNRFYH+0ptmjCkZCOWHirRU=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=XFg6LmDCQI3ieDhMCAzKqrSMnDuDEz8EBr+rntixHOPp+MWDJWYS0MTTC8eiUzdb1\n\tJaDxnde1mY12s+NDWaD7A09aDxDvbAZbjoSGV1CTakKCKKdwzVZEUIzyN2RyWTJk01\n\tuxWaGUUqfSC5h1ktJxXeMVpy0cI2mF7TxIUapYGuR4Y6bhI8ahiq0joidoeYiCqr1k\n\tjMZNLhFi709KET9fcamwPm6gFGqVHIkI0DmzOHId2dY5Fe++GF17/mspYtMLbw0pD5\n\t9Lpa0mGxLKi8zTRhXEAO1/C3r9k+FOaP6ro+sX8G1iWZ77lt5uQVXjIrFopW8dRdAM\n\tna6hHXVpzZUbA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675184508;\n\tbh=6Cmptv7C43PTNHhfvCBAfNRFYH+0ptmjCkZCOWHirRU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=eNFNcryJEHy9HYSM4btRqkWoGHpJ6w+PHcOkF5GkyjQPNcvTPYvWXF4OpAaQTODrC\n\to8zlPU6jkPSa7Nmo7cvAretsLuAZMC6544ig7+1wWyrn6dvX0zAuX5ojypqqBGFutr\n\t7099kjZHo1eTY5rvZ4qQ2Mri91C+mbsjD60uB8sY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eNFNcryJ\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221213114836.15473-5-naush@raspberrypi.com>","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-5-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 31 Jan 2023 17:01:45 +0000","Message-ID":"<167518450581.42371.12169486428273034452@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] ipa: raspberrypi: Use the\n\tgeneric statistics structure in the algorithms","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26394,"web_url":"https://patchwork.libcamera.org/comment/26394/","msgid":"","date":"2023-01-31T17:08:23","subject":"Re: [libcamera-devel] [PATCH v3 4/5] ipa: raspberrypi: Use the\n\tgeneric statistics structure in the algorithms","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Tue, 31 Jan 2023, 7:01 pm Kieran Bingham, <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:35)\n> > Repurpose the StatisticsPtr type from being a\n> shared_ptr to\n> > shared_ptr. This removes any hardware\n> specific header\n> > files and structures from the algorithms source code.\n> >\n> > Add a new function in the Raspberry Pi IPA to populate the generic\n> statistics\n> > structure from the values provided by the hardware in the\n> bcm2835_isp_stats\n> > structure.\n> >\n> > Update the Lux, AWB, AGC, ALSC, Contrast, and Focus algorithms to use the\n> > generic statistics structure appropriately in their calculations.\n> Additionally,\n> > remove references to any hardware specific headers and defines in these\n> source\n> > files.\n>\n> I'm afraid this one fails to build as I've merged IMX708 support with\n> the AF algorithm additions.\n>\n> Could you rebase this series please?\n>\n\n\nOuch!\n\nThis is because we've removed the bcm2835-isp.h header from the controller\nin this series.\n\nOnce this series has been merged, we do have the task of converting the\nfocus stats to a generic region struct. For now I'll probably post an\nupdated patch with the header included back.\n\nRegards,\nNaush\n\n>\n>\n> [5/119] Compiling C++ object\n> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o\n> FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o\n> c++ -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi\n> -I../../src/ipa/raspberrypi -Iinclude -I../../include -Isrc/ipa\n> -I../../src/ipa -I../../src/ipa/raspberrypi/controller\n> -Iinclude/libcamera/ipa -Iinclude/libcamera -fdiagnostics-color=always\n> -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra\n> -Werror -std=c++17 -g -Wshadow -include\n> /home/kbingham/iob/libcamera/libcamera/build/gcc/config.h -fPIC\n> -DLIBCAMERA_BASE_PRIVATE -MD -MQ\n> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o -MF\n> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o.d -o\n> src/ipa/raspberrypi/ipa_rpi.so.p/controller_rpi_af.cpp.o -c\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp\n> In file included from ../../src/ipa/raspberrypi/controller/rpi/af.cpp:8:\n> ../../src/ipa/raspberrypi/controller/rpi/af.h:120:77: error:\n> ‘FOCUS_REGIONS’ was not declared in this scope\n> 120 | double getContrast(struct bcm2835_isp_stats_focus const\n> focus_stats[FOCUS_REGIONS]) const;\n> |\n> ^~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/controller/rpi/af.h:141:35: error:\n> ‘FOCUS_REGIONS’ was not declared in this scope\n> 141 | uint16_t contrastWeights_[FOCUS_REGIONS];\n> | ^~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In constructor\n> ‘RPiController::Af::Af(RPiController::Controller*)’:\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:178:11: error: class\n> ‘RPiController::Af’ does not have any field named ‘contrastWeights_’\n> 178 | contrastWeights_{},\n> | ^~~~~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function ‘void\n> RPiController::Af::computeWeights()’:\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:300:23: error:\n> ‘FOCUS_REGIONS’ was not declared in this scope\n> 300 | static_assert(FOCUS_REGIONS == FocusStatsRows *\n> FocusStatsCols);\n> | ^~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:313:25: error:\n> ‘contrastWeights_’ was not declared in this scope; did you mean\n> ‘phaseWeights_’?\n> 313 | contrastWeights_[FocusStatsCols * i + j] =\n> w;\n> | ^~~~~~~~~~~~~~~~\n> | phaseWeights_\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:316:38: error:\n> ‘contrastWeights_’ was not declared in this scope; did you mean\n> ‘phaseWeights_’?\n> 316 | <<\n> contrastWeights_[FocusStatsCols * i + 0] << \" \"\n> | ^~~~~~~~~~~~~~~~\n> | phaseWeights_\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: At global scope:\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:355:73: error:\n> ‘FOCUS_REGIONS’ was not declared in this scope\n> 355 | double Af::getContrast(struct bcm2835_isp_stats_focus const\n> focus_stats[FOCUS_REGIONS]) const\n> |\n> ^~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function\n> ‘double RPiController::Af::getContrast(...) const’:\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:359:34: error:\n> ‘FOCUS_REGIONS’ was not declared in this scope\n> 359 | for (unsigned i = 0; i < FOCUS_REGIONS; ++i) {\n> | ^~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:360:30: error:\n> ‘contrastWeights_’ was not declared in this scope; did you mean\n> ‘phaseWeights_’?\n> 360 | unsigned w = contrastWeights_[i];\n> | ^~~~~~~~~~~~~~~~\n> | phaseWeights_\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:361:31: error:\n> ‘focus_stats’ was not declared in this scope\n> 361 | sumWc += w * (focus_stats[i].contrast_val[1][1] >>\n> 10);\n> | ^~~~~~~~~~~\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp: In member function\n> ‘virtual void RPiController::Af::process(RPiController::StatisticsPtr&,\n> RPiController::Metadata*)’:\n> ../../src/ipa/raspberrypi/controller/rpi/af.cpp:669:44: error: ‘using\n> element_type = struct RPiController::Statistics’ {aka ‘struct\n> RPiController::Statistics’} has no member named ‘focus_stats’\n> 669 | prevContrast_ = getContrast(stats->focus_stats);\n> | ^~~~~~~~~~~\n> [12/119] Compiling C++ object\n> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o\n> FAILED: src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o\n> c++ -Isrc/ipa/raspberrypi/ipa_rpi.so.p -Isrc/ipa/raspberrypi\n> -I../../src/ipa/raspberrypi -Iinclude -I../../include -Isrc/ipa\n> -I../../src/ipa -I../../src/ipa/raspberrypi/controller\n> -Iinclude/libcamera/ipa -Iinclude/libcamera -fdiagnostics-color=always\n> -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra\n> -Werror -std=c++17 -g -Wshadow -include\n> /home/kbingham/iob/libcamera/libcamera/build/gcc/config.h -fPIC\n> -DLIBCAMERA_BASE_PRIVATE -MD -MQ\n> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -MF\n> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o.d -o\n> src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_imx708.cpp.o -c\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp: In member function ‘void\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)’:\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:332:23: error: ‘using\n> element_type = struct RPiController::Statistics’ {aka ‘struct\n> RPiController::Statistics’} has no member named ‘hist’\n> 332 | memcpy(stats->hist[0].g_hist, aeHistLinear_,\n> sizeof(stats->hist[0].g_hist));\n> | ^~~~\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:332:68: error: ‘using\n> element_type = struct RPiController::Statistics’ {aka ‘struct\n> RPiController::Statistics’} has no member named ‘hist’\n> 332 | memcpy(stats->hist[0].g_hist, aeHistLinear_,\n> sizeof(stats->hist[0].g_hist));\n> |\n> ^~~~\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:336:29: error:\n> ‘AGC_REGIONS’ was not declared in this scope\n> 336 | for (int i = 0; i < AGC_REGIONS; i++) {\n> | ^~~~~~~~~~~\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:61: error: ‘using\n> element_type = struct RPiController::Statistics’ {aka ‘struct\n> RPiController::Statistics’} has no member named ‘agc_stats’\n> 337 | struct bcm2835_isp_stats_region &r =\n> stats->agc_stats[i];\n> |\n> ^~~~~~~~~\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:17: error: invalid use\n> of incomplete type ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n> | ^\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward\n> declaration of ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 337 | struct bcm2835_isp_stats_region &r =\n> stats->agc_stats[i];\n> | ^~~~~~~~~~~~~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:27: error: invalid use\n> of incomplete type ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n> | ^\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward\n> declaration of ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 337 | struct bcm2835_isp_stats_region &r =\n> stats->agc_stats[i];\n> | ^~~~~~~~~~~~~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:37: error: invalid use\n> of incomplete type ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n> | ^\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward\n> declaration of ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 337 | struct bcm2835_isp_stats_region &r =\n> stats->agc_stats[i];\n> | ^~~~~~~~~~~~~~~~~~~~~~~~\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:338:47: error: invalid use\n> of incomplete type ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 338 | r.r_sum = r.b_sum = r.g_sum = r.counted * v;\n> | ^\n> ../../src/ipa/raspberrypi/cam_helper_imx708.cpp:337:24: note: forward\n> declaration of ‘struct\n> CamHelperImx708::putAGCStatistics(RPiController::StatisticsPtr)::bcm2835_isp_stats_region’\n> 337 | struct bcm2835_isp_stats_region &r =\n> stats->agc_stats[i];\n> | ^~~~~~~~~~~~~~~~~~~~~~~~\n>\n> --\n> Kieran\n>\n>\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/controller.h | 4 +-\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 27 +++++-------\n> > src/ipa/raspberrypi/controller/rpi/agc.h | 2 +-\n> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 32 +++++++-------\n> > src/ipa/raspberrypi/controller/rpi/alsc.h | 3 +-\n> > src/ipa/raspberrypi/controller/rpi/awb.cpp | 20 ++++-----\n> > src/ipa/raspberrypi/controller/rpi/awb.h | 1 +\n> > .../raspberrypi/controller/rpi/contrast.cpp | 8 ++--\n> > src/ipa/raspberrypi/controller/rpi/focus.cpp | 7 ++-\n> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 14 +-----\n> > src/ipa/raspberrypi/raspberrypi.cpp | 44 ++++++++++++++++++-\n> > src/ipa/raspberrypi/statistics.h | 2 +\n> > 12 files changed, 96 insertions(+), 68 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/controller.h\n> b/src/ipa/raspberrypi/controller/controller.h\n> > index 3e1e051703b3..e6c950c3a509 100644\n> > --- a/src/ipa/raspberrypi/controller/controller.h\n> > +++ b/src/ipa/raspberrypi/controller/controller.h\n> > @@ -15,19 +15,17 @@\n> > #include \n> > #include \n> >\n> > -#include \n> > -\n> > #include \"libcamera/internal/yaml_parser.h\"\n> >\n> > #include \"camera_mode.h\"\n> > #include \"device_status.h\"\n> > #include \"metadata.h\"\n> > +#include \"statistics.h\"\n> >\n> > namespace RPiController {\n> >\n> > class Algorithm;\n> > typedef std::unique_ptr AlgorithmPtr;\n> > -typedef std::shared_ptr StatisticsPtr;\n> >\n> > /*\n> > * The Controller holds a pointer to some global_metadata, which is how\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 46dcc81ae14c..868c30f03d66 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -9,8 +9,6 @@\n> > #include \n> > #include \n> >\n> > -#include \n> > -\n> > #include \n> >\n> > #include \"../awb_status.h\"\n> > @@ -451,7 +449,7 @@ void Agc::process(StatisticsPtr &stats, Metadata\n> *imageMetadata)\n> > fetchCurrentExposure(imageMetadata);\n> > /* Compute the total gain we require relative to the current\n> exposure. */\n> > double gain, targetY;\n> > - computeGain(stats.get(), imageMetadata, gain, targetY);\n> > + computeGain(stats, imageMetadata, gain, targetY);\n> > /* Now compute the target (final) exposure which we think we\n> want. */\n> > computeTargetExposure(gain);\n> > /*\n> > @@ -585,24 +583,23 @@ void Agc::fetchAwbStatus(Metadata *imageMetadata)\n> > LOG(RPiAgc, Debug) << \"No AWB status found\";\n> > }\n> >\n> > -static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const\n> &awb,\n> > +static double computeInitialY(StatisticsPtr &stats, AwbStatus const\n> &awb,\n> > double weights[], double gain)\n> > {\n> > - bcm2835_isp_stats_region *regions = stats->agc_stats;\n> > /*\n> > * Note how the calculation below means that equal weights give\n> you\n> > * \"average\" metering (i.e. all pixels equally important).\n> > */\n> > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> > - for (unsigned int i = 0; i < AgcStatsSize; i++) {\n> > - double counted = regions[i].counted;\n> > - double rAcc = std::min(regions[i].r_sum * gain, ((1 <<\n> PipelineBits) - 1) * counted);\n> > - double gAcc = std::min(regions[i].g_sum * gain, ((1 <<\n> PipelineBits) - 1) * counted);\n> > - double bAcc = std::min(regions[i].b_sum * gain, ((1 <<\n> PipelineBits) - 1) * counted);\n> > + for (unsigned int i = 0; i < stats->agcRegions.numRegions();\n> i++) {\n> > + auto ®ion = stats->agcRegions.get(i);\n> > + double rAcc = std::min(region.val.rSum * gain,\n> ((1 << PipelineBits) - 1) * region.counted);\n> > + double gAcc = std::min(region.val.gSum * gain,\n> ((1 << PipelineBits) - 1) * region.counted);\n> > + double bAcc = std::min(region.val.bSum * gain,\n> ((1 << PipelineBits) - 1) * region.counted);\n> > rSum += rAcc * weights[i];\n> > gSum += gAcc * weights[i];\n> > bSum += bAcc * weights[i];\n> > - pixelSum += counted * weights[i];\n> > + pixelSum += region.counted * weights[i];\n> > }\n> > if (pixelSum == 0.0) {\n> > LOG(RPiAgc, Warning) << \"computeInitialY: pixelSum is\n> zero\";\n> > @@ -624,23 +621,23 @@ static double computeInitialY(bcm2835_isp_stats\n> *stats, AwbStatus const &awb,\n> >\n> > static constexpr double EvGainYTargetLimit = 0.9;\n> >\n> > -static double constraintComputeGain(AgcConstraint &c, Histogram &h,\n> double lux,\n> > +static double constraintComputeGain(AgcConstraint &c, const Histogram\n> &h, double lux,\n> > double evGain, double &targetY)\n> > {\n> > targetY = c.yTarget.eval(c.yTarget.domain().clip(lux));\n> > targetY = std::min(EvGainYTargetLimit, targetY * evGain);\n> > double iqm = h.interQuantileMean(c.qLo, c.qHi);\n> > - return (targetY * NUM_HISTOGRAM_BINS) / iqm;\n> > + return (targetY * h.bins()) / iqm;\n> > }\n> >\n> > -void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata\n> *imageMetadata,\n> > +void Agc::computeGain(StatisticsPtr &statistics, Metadata\n> *imageMetadata,\n> > double &gain, double &targetY)\n> > {\n> > struct LuxStatus lux = {};\n> > lux.lux = 400; /* default lux level to 400 in case no metadata\n> found */\n> > if (imageMetadata->get(\"lux.status\", lux) != 0)\n> > LOG(RPiAgc, Warning) << \"No lux level found\";\n> > - Histogram h(statistics->hist[0].g_hist, NUM_HISTOGRAM_BINS);\n> > + const Histogram &h = statistics->yHist;\n> > double evGain = status_.ev * config_.baseEv;\n> > /*\n> > * The initial gain and target_Y come from some of the regions.\n> After\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h\n> b/src/ipa/raspberrypi/controller/rpi/agc.h\n> > index cf04da1973f1..f04896ca25ad 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> > @@ -96,7 +96,7 @@ private:\n> > void housekeepConfig();\n> > void fetchCurrentExposure(Metadata *imageMetadata);\n> > void fetchAwbStatus(Metadata *imageMetadata);\n> > - void computeGain(bcm2835_isp_stats *statistics, Metadata\n> *imageMetadata,\n> > + void computeGain(StatisticsPtr &statistics, Metadata\n> *imageMetadata,\n> > double &gain, double &targetY);\n> > void computeTargetExposure(double gain);\n> > bool applyDigitalGain(double gain, double targetY);\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > index a4afaf841c41..eb4e2f9496e1 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > @@ -310,19 +310,21 @@ double getCt(Metadata *metadata, double defaultCt)\n> > return awbStatus.temperatureK;\n> > }\n> >\n> > -static void copyStats(bcm2835_isp_stats_region regions[XY],\n> StatisticsPtr &stats,\n> > +static void copyStats(RgbyRegions ®ions, StatisticsPtr &stats,\n> > AlscStatus const &status)\n> > {\n> > - bcm2835_isp_stats_region *inputRegions = stats->awb_stats;\n> > + if (!regions.numRegions())\n> > + regions.init(stats->awbRegions.size());\n> > +\n> > double *rTable = (double *)status.r;\n> > double *gTable = (double *)status.g;\n> > double *bTable = (double *)status.b;\n> > - for (int i = 0; i < XY; i++) {\n> > - regions[i].r_sum = inputRegions[i].r_sum / rTable[i];\n> > - regions[i].g_sum = inputRegions[i].g_sum / gTable[i];\n> > - regions[i].b_sum = inputRegions[i].b_sum / bTable[i];\n> > - regions[i].counted = inputRegions[i].counted;\n> > - /* (don't care about the uncounted value) */\n> > + for (unsigned int i = 0; i < stats->awbRegions.numRegions();\n> i++) {\n> > + auto r = stats->awbRegions.get(i);\n> > + r.val.rSum = static_cast(r.val.rSum /\n> rTable[i]);\n> > + r.val.gSum = static_cast(r.val.gSum /\n> gTable[i]);\n> > + r.val.bSum = static_cast(r.val.bSum /\n> bTable[i]);\n> > + regions.set(i, r);\n> > }\n> > }\n> >\n> > @@ -512,19 +514,19 @@ void resampleCalTable(double const calTableIn[XY],\n> > }\n> >\n> > /* Calculate chrominance statistics (R/G and B/G) for each region. */\n> > -static_assert(XY == AWB_REGIONS, \"ALSC/AWB statistics region mismatch\");\n> > -static void calculateCrCb(bcm2835_isp_stats_region *awbRegion, double\n> cr[XY],\n> > +static void calculateCrCb(const RgbyRegions &awbRegion, double cr[XY],\n> > double cb[XY], uint32_t minCount, uint16_t\n> minG)\n> > {\n> > for (int i = 0; i < XY; i++) {\n> > - bcm2835_isp_stats_region &zone = awbRegion[i];\n> > - if (zone.counted <= minCount ||\n> > - zone.g_sum / zone.counted <= minG) {\n> > + auto s = awbRegion.get(i);\n> > +\n> > + if (s.counted <= minCount || s.val.gSum / s.counted <=\n> minG) {\n> > cr[i] = cb[i] = InsufficientData;\n> > continue;\n> > }\n> > - cr[i] = zone.r_sum / (double)zone.g_sum;\n> > - cb[i] = zone.b_sum / (double)zone.g_sum;\n> > +\n> > + cr[i] = s.val.rSum / (double)s.val.gSum;\n> > + cb[i] = s.val.bSum / (double)s.val.gSum;\n> > }\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h\n> b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> > index a858ef5a6551..9167c9ffa2e3 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.h\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> > @@ -12,6 +12,7 @@\n> >\n> > #include \"../algorithm.h\"\n> > #include \"../alsc_status.h\"\n> > +#include \"../statistics.h\"\n> >\n> > namespace RPiController {\n> >\n> > @@ -98,7 +99,7 @@ private:\n> > /* copy out the results from the async thread so that it can be\n> restarted */\n> > void fetchAsyncResults();\n> > double ct_;\n> > - bcm2835_isp_stats_region statistics_[AlscCellsY * AlscCellsX];\n> > + RgbyRegions statistics_;\n> > double asyncResults_[3][AlscCellsY][AlscCellsX];\n> > double asyncLambdaR_[AlscCellsX * AlscCellsY];\n> > double asyncLambdaB_[AlscCellsX * AlscCellsY];\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> > index 04d1c8783654..ef3435d66106 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> > @@ -21,9 +21,6 @@ LOG_DEFINE_CATEGORY(RPiAwb)\n> >\n> > #define NAME \"rpi.awb\"\n> >\n> > -static constexpr unsigned int AwbStatsSizeX = DEFAULT_AWB_REGIONS_X;\n> > -static constexpr unsigned int AwbStatsSizeY = DEFAULT_AWB_REGIONS_Y;\n> > -\n> > /*\n> > * todo - the locking in this algorithm needs some tidying up as has\n> been done\n> > * elsewhere (ALSC and AGC).\n> > @@ -401,17 +398,16 @@ void Awb::asyncFunc()\n> > }\n> >\n> > static void generateStats(std::vector &zones,\n> > - bcm2835_isp_stats_region *stats, double\n> minPixels,\n> > + RgbyRegions &stats, double minPixels,\n> > double minG)\n> > {\n> > - for (unsigned int i = 0; i < AwbStatsSizeX * AwbStatsSizeY; i++)\n> {\n> > + for (auto const ®ion : stats) {\n> > Awb::RGB zone;\n> > - double counted = stats[i].counted;\n> > - if (counted >= minPixels) {\n> > - zone.G = stats[i].g_sum / counted;\n> > + if (region.counted >= minPixels) {\n> > + zone.G = region.val.gSum / region.counted;\n> > if (zone.G >= minG) {\n> > - zone.R = stats[i].r_sum / counted;\n> > - zone.B = stats[i].b_sum / counted;\n> > + zone.R = region.val.rSum /\n> region.counted;\n> > + zone.B = region.val.bSum /\n> region.counted;\n> > zones.push_back(zone);\n> > }\n> > }\n> > @@ -425,7 +421,7 @@ void Awb::prepareStats()\n> > * LSC has already been applied to the stats in this pipeline,\n> so stop\n> > * any LSC compensation. We also ignore config_.fast in this\n> version.\n> > */\n> > - generateStats(zones_, statistics_->awb_stats, config_.minPixels,\n> > + generateStats(zones_, statistics_->awbRegions, config_.minPixels,\n> > config_.minG);\n> > /*\n> > * apply sensitivities, so values appear to come from our\n> \"canonical\"\n> > @@ -641,7 +637,7 @@ void Awb::awbBayes()\n> > * valid... not entirely sure about this.\n> > */\n> > Pwl prior = interpolatePrior();\n> > - prior *= zones_.size() / (double)(AwbStatsSizeX * AwbStatsSizeY);\n> > + prior *= zones_.size() /\n> (double)(statistics_->awbRegions.numRegions());\n> > prior.map([](double x, double y) {\n> > LOG(RPiAwb, Debug) << \"(\" << x << \",\" << y << \")\";\n> > });\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.h\n> b/src/ipa/raspberrypi/controller/rpi/awb.h\n> > index 2254c3ed2cc4..e7d49cd8036b 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/awb.h\n> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.h\n> > @@ -13,6 +13,7 @@\n> > #include \"../awb_algorithm.h\"\n> > #include \"../pwl.h\"\n> > #include \"../awb_status.h\"\n> > +#include \"../statistics.h\"\n> >\n> > namespace RPiController {\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> > index 5b37edcbd46a..a4f8c4f04fc4 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> > @@ -106,7 +106,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> > * bit.\n> > */\n> > double histLo = histogram.quantile(config.loHistogram) *\n> > - (65536 / NUM_HISTOGRAM_BINS);\n> > + (65536 / histogram.bins());\n> > double levelLo = config.loLevel * 65536;\n> > LOG(RPiContrast, Debug)\n> > << \"Move histogram point \" << histLo << \" to \" <<\n> levelLo;\n> > @@ -119,7 +119,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> > * Keep the mid-point (median) in the same place, though, to\n> limit the\n> > * apparent amount of global brightness shift.\n> > */\n> > - double mid = histogram.quantile(0.5) * (65536 /\n> NUM_HISTOGRAM_BINS);\n> > + double mid = histogram.quantile(0.5) * (65536 /\n> histogram.bins());\n> > enhance.append(mid, mid);\n> >\n> > /*\n> > @@ -127,7 +127,7 @@ Pwl computeStretchCurve(Histogram const &histogram,\n> > * there up.\n> > */\n> > double histHi = histogram.quantile(config.hiHistogram) *\n> > - (65536 / NUM_HISTOGRAM_BINS);\n> > + (65536 / histogram.bins());\n> > double levelHi = config.hiLevel * 65536;\n> > LOG(RPiContrast, Debug)\n> > << \"Move histogram point \" << histHi << \" to \" <<\n> levelHi;\n> > @@ -158,7 +158,7 @@ Pwl applyManualContrast(Pwl const &gammaCurve,\n> double brightness,\n> > void Contrast::process(StatisticsPtr &stats,\n> > [[maybe_unused]] Metadata *imageMetadata)\n> > {\n> > - Histogram histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);\n> > + Histogram &histogram = stats->yHist;\n> > /*\n> > * We look at the histogram and adjust the gamma curve in the\n> following\n> > * ways: 1. Adjust the gamma curve so as to pull the start of the\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> b/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> > index 8c5029bd0e95..ea3cc00e42c3 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp\n> > @@ -31,10 +31,9 @@ char const *Focus::name() const\n> > void Focus::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> > {\n> > FocusStatus status;\n> > - unsigned int i;\n> > - for (i = 0; i < FOCUS_REGIONS; i++)\n> > - status.focusMeasures[i] =\n> stats->focus_stats[i].contrast_val[1][1] / 1000;\n> > - status.num = i;\n> > + for (unsigned int i = 0; i < stats->focusRegions.numRegions();\n> i++)\n> > + status.focusMeasures[i] = stats->focusRegions.get(i).val;\n> > + status.num = stats->focusRegions.numRegions();\n> > imageMetadata->set(\"focus.status\", status);\n> >\n> > LOG(RPiFocus, Debug)\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > index 9759186afacf..06625f3a5ea3 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp\n> > @@ -6,8 +6,6 @@\n> > */\n> > #include \n> >\n> > -#include \n> > -\n> > #include \n> >\n> > #include \"../device_status.h\"\n> > @@ -83,20 +81,12 @@ void Lux::process(StatisticsPtr &stats, Metadata\n> *imageMetadata)\n> > if (imageMetadata->get(\"device.status\", deviceStatus) == 0) {\n> > double currentGain = deviceStatus.analogueGain;\n> > double currentAperture =\n> deviceStatus.aperture.value_or(currentAperture_);\n> > - uint64_t sum = 0;\n> > - uint32_t num = 0;\n> > - uint32_t *bin = stats->hist[0].g_hist;\n> > - const int numBins = sizeof(stats->hist[0].g_hist) /\n> > - sizeof(stats->hist[0].g_hist[0]);\n> > - for (int i = 0; i < numBins; i++)\n> > - sum += bin[i] * (uint64_t)i, num += bin[i];\n> > - /* add .5 to reflect the mid-points of bins */\n> > - double currentY = sum / (double)num + .5;\n> > + double currentY = stats->yHist.interQuantileMean(0, 1);\n> > double gainRatio = referenceGain_ / currentGain;\n> > double shutterSpeedRatio =\n> > referenceShutterSpeed_ /\n> deviceStatus.shutterSpeed;\n> > double apertureRatio = referenceAperture_ /\n> currentAperture;\n> > - double yRatio = currentY * (65536 / numBins) /\n> referenceY_;\n> > + double yRatio = currentY * (65536 / stats->yHist.bins())\n> / referenceY_;\n> > double estimatedLux = shutterSpeedRatio * gainRatio *\n> > apertureRatio * apertureRatio *\n> > yRatio * referenceLux_;\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index bead436def3c..cff079bbafb3 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -51,6 +51,7 @@\n> > #include \"metadata.h\"\n> > #include \"sharpen_algorithm.h\"\n> > #include \"sharpen_status.h\"\n> > +#include \"statistics.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -139,6 +140,7 @@ private:\n> > void prepareISP(const ISPConfig &data);\n> > void reportMetadata(unsigned int ipaContext);\n> > void fillDeviceStatus(const ControlList &sensorControls,\n> unsigned int ipaContext);\n> > + RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats\n> *stats) const;\n> > void processStats(unsigned int bufferId, unsigned int\n> ipaContext);\n> > void applyFrameDurations(Duration minFrameDuration, Duration\n> maxFrameDuration);\n> > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> > @@ -1141,6 +1143,46 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls, unsigned int ip\n> > rpiMetadata_[ipaContext].set(\"device.status\", deviceStatus);\n> > }\n> >\n> > +RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats\n> *stats) const\n> > +{\n> > + using namespace RPiController;\n> > +\n> > + unsigned int i;\n> > + StatisticsPtr statistics =\n> > +\n> std::make_unique(Statistics::AgcStatsPos::PreWb,\n> Statistics::ColourStatsPos::PostLsc);\n> > +\n> > + /* RGB histograms are not used, so do not populate them. */\n> > + statistics->yHist =\n> std::move(RPiController::Histogram(stats->hist[0].g_hist,\n> NUM_HISTOGRAM_BINS));\n> > +\n> > + statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X,\n> DEFAULT_AWB_REGIONS_Y });\n> > + for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> > + statistics->awbRegions.set(i, { {\n> stats->awb_stats[i].r_sum,\n> > +\n> stats->awb_stats[i].g_sum,\n> > +\n> stats->awb_stats[i].b_sum },\n> > +\n> stats->awb_stats[i].counted,\n> > +\n> stats->awb_stats[i].notcounted });\n> > +\n> > + /*\n> > + * There are only ever 15 regions computed by the firmware due\n> to zoning,\n> > + * but the HW defines AGC_REGIONS == 16!\n> > + */\n> > + statistics->agcRegions.init(15);\n> > + for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> > + statistics->agcRegions.set(i, { {\n> stats->agc_stats[i].r_sum,\n> > +\n> stats->agc_stats[i].g_sum,\n> > +\n> stats->agc_stats[i].b_sum },\n> > +\n> stats->agc_stats[i].counted,\n> > +\n> stats->awb_stats[i].notcounted });\n> > +\n> > + statistics->focusRegions.init({ 4, 3 });\n> > + for (i = 0; i < statistics->focusRegions.numRegions(); i++)\n> > + statistics->focusRegions.set(i, {\n> stats->focus_stats[i].contrast_val[1][1] / 1000,\n> > +\n> stats->focus_stats[i].contrast_val_num[1][1],\n> > +\n> stats->focus_stats[i].contrast_val_num[1][0] });\n> > +\n> > + return statistics;\n> > +}\n> > +\n> > void IPARPi::processStats(unsigned int bufferId, unsigned int\n> ipaContext)\n> > {\n> > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > @@ -1153,7 +1195,7 @@ void IPARPi::processStats(unsigned int bufferId,\n> unsigned int ipaContext)\n> >\n> > Span mem = it->second.planes()[0];\n> > bcm2835_isp_stats *stats = reinterpret_cast *>(mem.data());\n> > - RPiController::StatisticsPtr statistics =\n> std::make_shared(*stats);\n> > + RPiController::StatisticsPtr statistics = fillStatistics(stats);\n> > helper_->process(statistics, rpiMetadata);\n> > controller_.process(statistics, &rpiMetadata);\n> >\n> > diff --git a/src/ipa/raspberrypi/statistics.h\n> b/src/ipa/raspberrypi/statistics.h\n> > index a762bf3d41aa..affb7272c963 100644\n> > --- a/src/ipa/raspberrypi/statistics.h\n> > +++ b/src/ipa/raspberrypi/statistics.h\n> > @@ -67,4 +67,6 @@ struct Statistics {\n> > FocusRegions focusRegions;\n> > };\n> >\n> > +using StatisticsPtr = std::shared_ptr;\n> > +\n> > } /* namespace RPiController */\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DF03ABEFBE\n\tfor ;\n\tTue, 31 Jan 2023 17:08:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41FC7625D8;\n\tTue, 31 Jan 2023 18:08:37 +0100 (CET)","from mail-yb1-xb2e.google.com (mail-yb1-xb2e.google.com\n\t[IPv6:2607:f8b0:4864:20::b2e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5BFE5625D0\n\tfor ;\n\tTue, 31 Jan 2023 18:08:35 +0100 (CET)","by mail-yb1-xb2e.google.com with SMTP id d8so17009340ybe.9\n\tfor ;\n\tTue, 31 Jan 2023 09:08:35 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675184917;\n\tbh=9qoOdMNdY/cGnzNlU4+zFfToXwnKZ25J5y9oSg341SA=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=qe+zHqlP/xudStQxA3hdYt2EJcEYl19UOMlED/Jqiq7t9XK8/VGw4UR6GuIdKx3Ez\n\tIJ/8++Lz9YNqB/mTKPEK7NuqwGk0hdYdpxg4g8iAyqsGLWRcgZoYCUFSKAXDimL2As\n\tln9jMRsCfaqrBcAOdwPLRhq45LeBPQ8MSJAdzZWgnMLV6AMAGQdM3KawgxcOIuOkBC\n\tCS2f1o8tAqEIYj2XGs31iK/hvChmhMEylQyoauxtuLpP5N7AYrfexh/WZ9QCy8hfy3\n\tMsSu7PGJDMDevFHFcALLPMNf0KUnVdVykV7r9ZsC5r4Loh1QPVWV9sABYoBfB/Qq59\n\thq+CjH5WiNfTA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=FeBH3BAIakkc8EEpYx9Xx7gdmWLKuxQunpK2Fg2nlvA=;\n\tb=heqWk1qUk0v4f1Apf+GIIjXmaL0jDNhY21rFxty2Av53AhpvcLaBbtnJMyHaY5IJL2\n\tUBIdd+OZlgiNMQMVTSS16XjQfxBGUYpRsi5EdCMw/gm4xJrsdeD1zgVphe4tQAlOXdtc\n\tLds8Y95ADTJBIcbEUiAqeyMZbNWt+SGiaJbktcpaRP6ZnY349hx7XBZ8485r3ZchFSie\n\tWMnmkZmo8P3c86gGmbm9rxuSoXhjj5zGbahoFHISS1jUW/CApqIxCd8UIagxGqv6U7S8\n\t1zWD2jsYVlsiSUPMVJzNqoDOLm6D/yUHhjRO0sU/NhQBHkz72iFjeDTPsIRWhou6sctN\n\tIZOQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"heqWk1qU\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=FeBH3BAIakkc8EEpYx9Xx7gdmWLKuxQunpK2Fg2nlvA=;\n\tb=VW/Z7Y7xNtq4Am8dNKX0UPVxybVyBDfzd9FThW9wZ+htI8D9+WdtWMKqX8cYJiHkO/\n\t9MR5LirsGp6/CkHWQ4jQKHkyoGyzvLRTYu2qEcvqnU8hav2RsjVac0PZpdNWYEpuQfuq\n\t19W5NLt1cLU6i/8Z5oVBXoitr11zfjHtomga/COHDyoBi11uFMWp4aBACtj67l5+91Oz\n\tsDK0MN/ZqaFmi+wjQy31013pUeJlWT248UwUJvrnefo/2cPo4VP2TtzXglqzK59ZkVzT\n\tCq3+dCcKuZJ1q+QM8CutEmO2EGqh9EtJhpElbm8p5Ql5ijdrwITZUFzvcKPYcsbe1xb2\n\tDqDg==","X-Gm-Message-State":"AFqh2kq3yvT8sO+8oZbpVXpTmoE/lfchLEm9QetIcAZ7Oc6wBQTV0SPK\n\tNqPRlX5pvZ/DN7DlBsiKKjIvTcoRUYWtaroh/hSrWqI7TTJNOEzf","X-Google-Smtp-Source":"AMrXdXu1AtC7U6Z1aUNBs+A6pex6Aqt53WaUTNWKzYC7tOtpGox+ICvy7F9WPdp5GITJZHSK/y3+Vr09x9QsTyNgBes=","X-Received":"by 2002:a25:1984:0:b0:7fe:e7f5:e228 with SMTP id\n\t126-20020a251984000000b007fee7f5e228mr3552193ybz.582.1675184913886;\n\tTue, 31 Jan 2023 09:08:33 -0800 (PST)","MIME-Version":"1.0","References":"<20221213114836.15473-1-naush@raspberrypi.com>\n\t<20221213114836.15473-5-naush@raspberrypi.com>\n\t<167518450581.42371.12169486428273034452@Monstersaurus>","In-Reply-To":"<167518450581.42371.12169486428273034452@Monstersaurus>","Date":"Tue, 31 Jan 2023 19:08:23 +0200","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000b9c8d105f392617b\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] ipa: raspberrypi: Use the\n\tgeneric statistics structure in the algorithms","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]