[{"id":26728,"web_url":"https://patchwork.libcamera.org/comment/26728/","msgid":"<20230324102127.obz2cc33qasycazv@uno.localdomain>","date":"2023-03-24T10:21:27","subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Nick\n\nOn Wed, Mar 22, 2023 at 01:06:12PM +0000, Naushir Patuck via libcamera-devel wrote:\n> From: Nick Hollinghurst \n>\n> Remove any hard-coded assumptions about the target hardware platform\n> from the autofocus algorithm. Instead, use the \"target\" string provided\n> by the camera tuning config and generalised statistics structures to\n> determing parameters such as grid and region sizes.\n>\n> Additionally, PDAF statistics are represented by a generalised region\n> statistics structure to be device agnostic.\n>\n> These changes also require the autofocus algorithm to initialise\n> region weights on the first frame's prepare()/process() call rather\n> than during initialisation.\n>\n> Signed-off-by: Nick Hollinghurst \n> Signed-off-by: Naushir Patuck \n> Tested-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper_imx708.cpp | 23 ++-\n> src/ipa/raspberrypi/controller/pdaf_data.h | 21 +--\n> src/ipa/raspberrypi/controller/rpi/af.cpp | 161 ++++++++++-----------\n> src/ipa/raspberrypi/controller/rpi/af.h | 28 ++--\n> 4 files changed, 119 insertions(+), 114 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx708.cpp b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> index 1f213d3c0833..641ba18f4b9d 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> @@ -69,11 +69,14 @@ private:\n> \t/* Largest long exposure scale factor given as a left shift on the frame length. */\n> \tstatic constexpr int longExposureShiftMax = 7;\n>\n> +\tstatic constexpr int pdafStatsRows = 12;\n> +\tstatic constexpr int pdafStatsCols = 16;\n> +\n> \tvoid populateMetadata(const MdParser::RegisterMap ®isters,\n> \t\t\t Metadata &metadata) const override;\n>\n> \tstatic bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> -\t\t\t\t PdafData &pdaf);\n> +\t\t\t\t PdafRegions &pdaf);\n>\n> \tbool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);\n> \tvoid putAGCStatistics(StatisticsPtr stats);\n> @@ -120,11 +123,11 @@ void CamHelperImx708::prepare(libcamera::Span buffer, Metadata &m\n> \tsize_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n>\n> \tif (buffer.size() > 2 * bytesPerLine) {\n> -\t\tPdafData pdaf;\n> +\t\tPdafRegions pdaf;\n> \t\tif (parsePdafData(&buffer[2 * bytesPerLine],\n> \t\t\t\t buffer.size() - 2 * bytesPerLine,\n> \t\t\t\t mode_.bitdepth, pdaf))\n> -\t\t\tmetadata.set(\"pdaf.data\", pdaf);\n> +\t\t\tmetadata.set(\"pdaf.regions\", pdaf);\n> \t}\n>\n> \t/* Parse AE-HIST data where present */\n> @@ -239,7 +242,7 @@ void CamHelperImx708::populateMetadata(const MdParser::RegisterMap ®isters,\n> }\n>\n> bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> -\t\t\t\t unsigned bpp, PdafData &pdaf)\n> +\t\t\t\t unsigned bpp, PdafRegions &pdaf)\n> {\n> \tsize_t step = bpp >> 1; /* bytes per PDAF grid entry */\n>\n> @@ -248,13 +251,17 @@ bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> \t\treturn false;\n> \t}\n>\n> +\tpdaf.init({ pdafStatsCols, pdafStatsRows });\n> +\n> \tptr += 2 * step;\n> -\tfor (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> -\t\tfor (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> +\tfor (unsigned i = 0; i < pdafStatsRows; ++i) {\n> +\t\tfor (unsigned j = 0; j < pdafStatsCols; ++j) {\n> \t\t\tunsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> \t\t\tint p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> -\t\t\tpdaf.conf[i][j] = c;\n> -\t\t\tpdaf.phase[i][j] = c ? p : 0;\n> +\t\t\tPdafData pdafData;\n> +\t\t\tpdafData.conf = c;\n> +\t\t\tpdafData.phase = c ? p : 0;\n> +\t\t\tpdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> \t\t\tptr += step;\n> \t\t}\n> \t}\n> diff --git a/src/ipa/raspberrypi/controller/pdaf_data.h b/src/ipa/raspberrypi/controller/pdaf_data.h\n> index 03c00d72c0e8..ae6ab996ded0 100644\n> --- a/src/ipa/raspberrypi/controller/pdaf_data.h\n> +++ b/src/ipa/raspberrypi/controller/pdaf_data.h\n> @@ -2,20 +2,23 @@\n> /*\n> * Copyright (C) 2022, Raspberry Pi Ltd\n> *\n> - * pdaf_data.h - PDAF Metadata; for now this is\n> - * largely based on IMX708's PDAF \"Type 1\" output.\n> + * pdaf_data.h - PDAF Metadata\n> */\n> #pragma once\n>\n> #include \n>\n> -#define PDAF_DATA_ROWS 12\n> -#define PDAF_DATA_COLS 16\n> +#include \"region_stats.h\"\n>\n> -struct PdafData {\n> -\t/* Confidence values, in raster order, in arbitrary units */\n> -\tuint16_t conf[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> +namespace RPiController {\n>\n> -\t/* Phase error, in raster order, in s11 Q4 format (S.6.4) */\n> -\tint16_t phase[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> +struct PdafData {\n> +\t/* Confidence, in arbitrary units */\n> +\tuint16_t conf;\n> +\t/* Phase error, in s16 Q4 format (S.11.4) */\n> +\tint16_t phase;\n> };\n> +\n> +using PdafRegions = RegionStats;\n> +\n> +}; // namespace RPiController\n> diff --git a/src/ipa/raspberrypi/controller/rpi/af.cpp b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> index a623651875f2..3d2dad974d8b 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/af.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> @@ -174,9 +174,8 @@ Af::Af(Controller *controller)\n> \t statsRegion_(0, 0, 0, 0),\n> \t windows_(),\n> \t useWindows_(false),\n> -\t phaseWeights_{},\n> -\t contrastWeights_{},\n> -\t sumWeights_(0),\n> +\t phaseWeights_(),\n> +\t contrastWeights_(),\n> \t scanState_(ScanState::Idle),\n> \t initted_(false),\n> \t ftarget_(-1.0),\n> @@ -190,6 +189,8 @@ Af::Af(Controller *controller)\n> \t scanData_(),\n> \t reportState_(AfState::Idle)\n> {\n> +\tphaseWeights_.w.reserve(16 * 12);\n\nIsn't the point of the patch to get these values from the camHelper ?\n\n> +\tcontrastWeights_.w.reserve(4 * 3);\n> \tscanData_.reserve(24);\n> }\n>\n> @@ -226,7 +227,7 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> \t\t\t << statsRegion_.y << ','\n> \t\t\t << statsRegion_.width << ','\n> \t\t\t << statsRegion_.height;\n> -\tcomputeWeights();\n> +\tinvalidateWeights();\n>\n> \tif (scanState_ >= ScanState::Coarse && scanState_ < ScanState::Settle) {\n> \t\t/*\n> @@ -239,111 +240,94 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> \tskipCount_ = cfg_.skipFrames;\n> }\n>\n> -void Af::computeWeights()\n> +void Af::computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols)\n> {\n> -\tconstexpr int MaxCellWeight = 240 / (int)MaxWindows;\n> +\twgts->rows = rows;\n> +\twgts->cols = cols;\n> +\twgts->sum = 0;\n> +\twgts->w.resize(rows * cols);\n> +\tstd::fill(wgts->w.begin(), wgts->w.end(), 0);\n>\n> -\tsumWeights_ = 0;\n> -\tfor (int i = 0; i < PDAF_DATA_ROWS; ++i)\n> -\t\tstd::fill(phaseWeights_[i], phaseWeights_[i] + PDAF_DATA_COLS, 0);\n> -\n> -\tif (useWindows_ &&\n> -\t statsRegion_.width >= PDAF_DATA_COLS && statsRegion_.height >= PDAF_DATA_ROWS) {\n> +\tif (rows > 0 && cols > 0 && useWindows_ &&\n> +\t statsRegion_.height >= rows && statsRegion_.width >= cols) {\n> \t\t/*\n> \t\t * Here we just merge all of the given windows, weighted by area.\n> \t\t * \\todo Perhaps a better approach might be to find the phase in each\n> \t\t * window and choose either the closest or the highest-confidence one?\n> -\t\t *\n> -\t\t * Using mostly \"int\" arithmetic, because Rectangle has signed x, y\n> \t\t */\n> -\t\tint cellH = (int)(statsRegion_.height / PDAF_DATA_ROWS);\n> -\t\tint cellW = (int)(statsRegion_.width / PDAF_DATA_COLS);\n> -\t\tint cellA = cellH * cellW;\n> +\t\tconst unsigned maxCellWeight = 46080u / (MaxWindows * rows * cols);\n\nWhere does 46080u comes from ? I can't find it in the diff\n\n> +\t\tconst unsigned cellH = statsRegion_.height / rows;\n> +\t\tconst unsigned cellW = statsRegion_.width / cols;\n> +\t\tconst unsigned cellA = cellH * cellW;\n>\n> \t\tfor (auto &w : windows_) {\n> -\t\t\tfor (int i = 0; i < PDAF_DATA_ROWS; ++i) {\n> -\t\t\t\tint y0 = std::max(statsRegion_.y + cellH * i, w.y);\n> -\t\t\t\tint y1 = std::min(statsRegion_.y + cellH * (i + 1), w.y + (int)(w.height));\n> +\t\t\tfor (unsigned r = 0; r < rows; ++r) {\n> +\t\t\t\tint y0 = std::max(statsRegion_.y + (int)(cellH * r), w.y);\n> +\t\t\t\tint y1 = std::min(statsRegion_.y + (int)(cellH * (r + 1)), w.y + (int)(w.height));\n> \t\t\t\tif (y0 >= y1)\n> \t\t\t\t\tcontinue;\n> \t\t\t\ty1 -= y0;\n> -\t\t\t\tfor (int j = 0; j < PDAF_DATA_COLS; ++j) {\n> -\t\t\t\t\tint x0 = std::max(statsRegion_.x + cellW * j, w.x);\n> -\t\t\t\t\tint x1 = std::min(statsRegion_.x + cellW * (j + 1), w.x + (int)(w.width));\n> +\t\t\t\tfor (unsigned c = 0; c < cols; ++c) {\n> +\t\t\t\t\tint x0 = std::max(statsRegion_.x + (int)(cellW * c), w.x);\n> +\t\t\t\t\tint x1 = std::min(statsRegion_.x + (int)(cellW * (c + 1)), w.x + (int)(w.width));\n> \t\t\t\t\tif (x0 >= x1)\n> \t\t\t\t\t\tcontinue;\n> -\t\t\t\t\tint a = y1 * (x1 - x0);\n> -\t\t\t\t\ta = (MaxCellWeight * a + cellA - 1) / cellA;\n> -\t\t\t\t\tphaseWeights_[i][j] += a;\n> -\t\t\t\t\tsumWeights_ += a;\n> +\t\t\t\t\tunsigned a = y1 * (x1 - x0);\n> +\t\t\t\t\ta = (maxCellWeight * a + cellA - 1) / cellA;\n> +\t\t\t\t\twgts->w[r * cols + c] += a;\n> +\t\t\t\t\twgts->sum += a;\n> \t\t\t\t}\n> \t\t\t}\n> \t\t}\n> \t}\n>\n> -\tif (sumWeights_ == 0) {\n> -\t\t/*\n> -\t\t * Default AF window is the middle 1/2 width of the middle 1/3 height\n> -\t\t * since this maps nicely to both PDAF (16x12) and Focus (4x3) grids.\n> -\t\t */\n> -\t\tfor (int i = PDAF_DATA_ROWS / 3; i < 2 * PDAF_DATA_ROWS / 3; ++i) {\n> -\t\t\tfor (int j = PDAF_DATA_COLS / 4; j < 3 * PDAF_DATA_COLS / 4; ++j) {\n> -\t\t\t\tphaseWeights_[i][j] = MaxCellWeight;\n> -\t\t\t\tsumWeights_ += MaxCellWeight;\n> +\tif (wgts->sum == 0) {\n> +\t\t/* Default AF window is the middle 1/2 width of the middle 1/3 height */\n> +\t\tfor (unsigned r = rows / 3; r < rows - rows / 3; ++r) {\n> +\t\t\tfor (unsigned c = cols / 4; c < cols - cols / 4; ++c) {\n> +\t\t\t\twgts->w[r * cols + c] = 1;\n> +\t\t\t\twgts->sum += 1;\n\nThis seems functionally different, or was MaxCellWeight == 1 ?\n> \t\t\t}\n> \t\t}\n> \t}\n> +}\n>\n> -\t/* Scale from PDAF to Focus Statistics grid (which has fixed size 4x3) */\n> -\tconstexpr int FocusStatsRows = 3;\n> -\tconstexpr int FocusStatsCols = 4;\n> -\tstatic_assert(FOCUS_REGIONS == FocusStatsRows * FocusStatsCols);\n> -\tstatic_assert(PDAF_DATA_ROWS % FocusStatsRows == 0);\n> -\tstatic_assert(PDAF_DATA_COLS % FocusStatsCols == 0);\n> -\tconstexpr int YFactor = PDAF_DATA_ROWS / FocusStatsRows;\n> -\tconstexpr int XFactor = PDAF_DATA_COLS / FocusStatsCols;\n> -\n> -\tLOG(RPiAf, Debug) << \"Recomputed weights:\";\n> -\tfor (int i = 0; i < FocusStatsRows; ++i) {\n> -\t\tfor (int j = 0; j < FocusStatsCols; ++j) {\n> -\t\t\tunsigned w = 0;\n> -\t\t\tfor (int y = 0; y < YFactor; ++y)\n> -\t\t\t\tfor (int x = 0; x < XFactor; ++x)\n> -\t\t\t\t\tw += phaseWeights_[YFactor * i + y][XFactor * j + x];\n> -\t\t\tcontrastWeights_[FocusStatsCols * i + j] = w;\n> -\t\t}\n> -\t\tLOG(RPiAf, Debug) << \" \"\n> -\t\t\t\t << contrastWeights_[FocusStatsCols * i + 0] << \" \"\n> -\t\t\t\t << contrastWeights_[FocusStatsCols * i + 1] << \" \"\n> -\t\t\t\t << contrastWeights_[FocusStatsCols * i + 2] << \" \"\n> -\t\t\t\t << contrastWeights_[FocusStatsCols * i + 3];\n> -\t}\n\nI might have missed where re-scaling is gone.. as you've tested this I\npresum it's just me not fully getting what's happening here\n\n> +void Af::invalidateWeights()\n> +{\n> +\tphaseWeights_.sum = 0;\n> +\tcontrastWeights_.sum = 0;\n> }\n>\n> -bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> +bool Af::getPhase(PdafRegions const ®ions, double &phase, double &conf)\n> {\n> +\tlibcamera::Size size = regions.size();\n> +\tif (size.height != phaseWeights_.rows || size.width != phaseWeights_.cols ||\n> +\t phaseWeights_.sum == 0) {\n> +\t\tLOG(RPiAf, Debug) << \"Recompute Phase weights \" << size.width << 'x' << size.height;\n> +\t\tcomputeWeights(&phaseWeights_, size.height, size.width);\n> +\t}\n> +\n> \tuint32_t sumWc = 0;\n> \tint64_t sumWcp = 0;\n> -\n> -\tfor (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> -\t\tfor (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> -\t\t\tif (phaseWeights_[i][j]) {\n> -\t\t\t\tuint32_t c = data.conf[i][j];\n> -\t\t\t\tif (c >= cfg_.confThresh) {\n> -\t\t\t\t\tif (c > cfg_.confClip)\n> -\t\t\t\t\t\tc = cfg_.confClip;\n> -\t\t\t\t\tc -= (cfg_.confThresh >> 2);\n> -\t\t\t\t\tsumWc += phaseWeights_[i][j] * c;\n> -\t\t\t\t\tc -= (cfg_.confThresh >> 2);\n> -\t\t\t\t\tsumWcp += phaseWeights_[i][j] * data.phase[i][j] * (int64_t)c;\n> -\t\t\t\t}\n> +\tfor (unsigned i = 0; i < regions.numRegions(); ++i) {\n> +\t\tunsigned w = phaseWeights_.w[i];\n> +\t\tif (w) {\n> +\t\t\tconst PdafData &data = regions.get(i).val;\n> +\t\t\tunsigned c = data.conf;\n> +\t\t\tif (c >= cfg_.confThresh) {\n> +\t\t\t\tif (c > cfg_.confClip)\n> +\t\t\t\t\tc = cfg_.confClip;\n> +\t\t\t\tc -= (cfg_.confThresh >> 2);\n> +\t\t\t\tsumWc += w * c;\n> +\t\t\t\tc -= (cfg_.confThresh >> 2);\n> +\t\t\t\tsumWcp += (int64_t)(w * c) * (int64_t)data.phase;\n> \t\t\t}\n> \t\t}\n> \t}\n>\n> -\tif (0 < sumWeights_ && sumWeights_ <= sumWc) {\n> +\tif (0 < phaseWeights_.sum && phaseWeights_.sum <= sumWc) {\n> \t\tphase = (double)sumWcp / (double)sumWc;\n> -\t\tconf = (double)sumWc / (double)sumWeights_;\n> +\t\tconf = (double)sumWc / (double)phaseWeights_.sum;\n> \t\treturn true;\n> \t} else {\n> \t\tphase = 0.0;\n> @@ -352,14 +336,19 @@ bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> \t}\n> }\n>\n> -double Af::getContrast(const FocusRegions &focusStats) const\n> +double Af::getContrast(const FocusRegions &focusStats)\n> {\n> -\tuint32_t sumWc = 0;\n> +\tlibcamera::Size size = focusStats.size();\n> +\tif (size.height != contrastWeights_.rows || size.width != contrastWeights_.cols || contrastWeights_.sum == 0) {\n\nPlease break this to multiple lines\n\n> +\t\tLOG(RPiAf, Debug) << \"Recompute Contrast weights \" << size.width << 'x' << size.height;\n\nit's easy to do the same here\n\n> +\t\tcomputeWeights(&contrastWeights_, size.height, size.width);\n> +\t}\n>\n> +\tuint64_t sumWc = 0;\n> \tfor (unsigned i = 0; i < focusStats.numRegions(); ++i)\n> -\t\tsumWc += contrastWeights_[i] * focusStats.get(i).val;\n> +\t\tsumWc += contrastWeights_.w[i] * focusStats.get(i).val;\n>\n> -\treturn (sumWeights_ == 0) ? 0.0 : (double)sumWc / (double)sumWeights_;\n> +\treturn (contrastWeights_.sum > 0) ? ((double)sumWc / (double)contrastWeights_.sum) : 0.0;\n> }\n>\n> void Af::doPDAF(double phase, double conf)\n> @@ -623,14 +612,14 @@ void Af::prepare(Metadata *imageMetadata)\n>\n> \tif (initted_) {\n> \t\t/* Get PDAF from the embedded metadata, and run AF algorithm core */\n> -\t\tPdafData data;\n> +\t\tPdafRegions regions;\n> \t\tdouble phase = 0.0, conf = 0.0;\n> \t\tdouble oldFt = ftarget_;\n> \t\tdouble oldFs = fsmooth_;\n> \t\tScanState oldSs = scanState_;\n> \t\tuint32_t oldSt = stepCount_;\n> -\t\tif (imageMetadata->get(\"pdaf.data\", data) == 0)\n> -\t\t\tgetPhase(data, phase, conf);\n> +\t\tif (imageMetadata->get(\"pdaf.regions\", regions) == 0)\n> +\t\t\tgetPhase(regions, phase, conf);\n> \t\tdoAF(prevContrast_, phase, conf);\n> \t\tupdateLensPosition();\n> \t\tLOG(RPiAf, Debug) << std::fixed << std::setprecision(2)\n> @@ -691,7 +680,7 @@ void Af::setMetering(bool mode)\n> {\n> \tif (useWindows_ != mode) {\n> \t\tuseWindows_ = mode;\n> -\t\tcomputeWeights();\n> +\t\tinvalidateWeights();\n> \t}\n> }\n>\n> @@ -708,7 +697,9 @@ void Af::setWindows(libcamera::Span const &wins)\n> \t\tif (windows_.size() >= MaxWindows)\n> \t\t\tbreak;\n> \t}\n> -\tcomputeWeights();\n> +\n> +\tif (useWindows_)\n> +\t\tinvalidateWeights();\n> }\n>\n> bool Af::setLensPosition(double dioptres, int *hwpos)\n> diff --git a/src/ipa/raspberrypi/controller/rpi/af.h b/src/ipa/raspberrypi/controller/rpi/af.h\n> index 7959371baf64..b479feb88c39 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/af.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/af.h\n> @@ -11,12 +11,6 @@\n> #include \"../pdaf_data.h\"\n> #include \"../pwl.h\"\n>\n> -/*\n> - * \\todo FOCUS_REGIONS is taken from bcm2835-isp.h, but should be made as a\n> - * generic RegionStats structure.\n> - */\n> -#define FOCUS_REGIONS 12\n> -\n> /*\n> * This algorithm implements a hybrid of CDAF and PDAF, favouring PDAF.\n> *\n> @@ -121,9 +115,20 @@ private:\n> \t\tdouble conf;\n> \t};\n>\n> -\tvoid computeWeights();\n> -\tbool getPhase(PdafData const &data, double &phase, double &conf) const;\n> -\tdouble getContrast(const FocusRegions &focusStats) const;\n> +\tstruct RegionWeights {\n> +\t\tunsigned rows;\n> +\t\tunsigned cols;\n> +\t\tuint32_t sum;\n> +\t\tstd::vector w;\n> +\n> +\t\tRegionWeights()\n> +\t\t\t: rows(0), cols(0), sum(0), w() {}\n> +\t};\n> +\n> +\tvoid computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols);\n> +\tvoid invalidateWeights();\n> +\tbool getPhase(PdafRegions const ®ions, double &phase, double &conf);\n> +\tdouble getContrast(const FocusRegions &focusStats);\n> \tvoid doPDAF(double phase, double conf);\n> \tbool earlyTerminationByPhase(double phase);\n> \tdouble findPeak(unsigned index) const;\n> @@ -143,9 +148,8 @@ private:\n> \tlibcamera::Rectangle statsRegion_;\n> \tstd::vector windows_;\n> \tbool useWindows_;\n> -\tuint8_t phaseWeights_[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> -\tuint16_t contrastWeights_[FOCUS_REGIONS];\n> -\tuint32_t sumWeights_;\n> +\tRegionWeights phaseWeights_;\n> +\tRegionWeights contrastWeights_;\n>\n> \t/* Working state. */\n> \tScanState scanState_;\n> --\n> 2.34.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 39E69C0F1B\n\tfor ;\n\tFri, 24 Mar 2023 10:21:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66A3F626E3;\n\tFri, 24 Mar 2023 11:21:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4ED7561ED0\n\tfor ;\n\tFri, 24 Mar 2023 11:21:30 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE487A58;\n\tFri, 24 Mar 2023 11:21:29 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679653291;\n\tbh=2uBLiMU9TDSGqjHlE+nAE190jCasIxsMUDUgoFKNe48=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=dhJgdd8abqvRPvG8X3pdT2bEYimJhHPJMnilACFiSNz6lL9d/ICdyEW+7pocTZb5H\n\tx/PRhvc8G8Vh3T+ju22G60LgUEGFSnC6cUYDspXI9HydVPCWsWLBX7JUFmk8FbUQ4H\n\tfgbdrztzoRBIoL/Cb4w0B8JSZcOGio3OGNVSynHbmK0Gt3zYeXN91noT1SJcscQ2ax\n\tqhUNl1TSV81/YuHmZX2XI6kVGCnKxb9xCYf4RzW3qvmpScvxNioUrHZyUuKIZDU9GU\n\tBGPtjD2Xs/nNAGMqKnS9WF3UXMd1pGoLEqHRP+uZe4anFngEWx5H9ngg7mowDDR6yw\n\tZESbChsHEh0Ig==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679653289;\n\tbh=2uBLiMU9TDSGqjHlE+nAE190jCasIxsMUDUgoFKNe48=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B0++1A+n7TVUCRgIZjUfCPSWgYf/b1Iu5DenybSMWklXDMMi4vCaIxthlOrLU8bDw\n\t6+hDw2ExxdPKKY2WM/dWLnrxjwmDQX5s4uYNPIBuXkdOeMS/HAyCAMptDXKXn0Khv5\n\tMefRFa8sYpPWSMgnmR5LHcIFvcduobuf3aGXM1OI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"B0++1A+n\"; dkim-atps=neutral","Date":"Fri, 24 Mar 2023 11:21:27 +0100","To":"Naushir Patuck ","Message-ID":"<20230324102127.obz2cc33qasycazv@uno.localdomain>","References":"<20230322130612.5208-1-naush@raspberrypi.com>\n\t<20230322130612.5208-11-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230322130612.5208-11-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26729,"web_url":"https://patchwork.libcamera.org/comment/26729/","msgid":"","date":"2023-03-24T11:35:15","subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","submitter":{"id":130,"url":"https://patchwork.libcamera.org/api/people/130/","name":"Nick Hollinghurst","email":"nick.hollinghurst@raspberrypi.com"},"content":"Hi Jacopo,\n\nThanks - comments inline.\n\n Nick\n\nOn Fri, 24 Mar 2023 at 10:21, Jacopo Mondi\n wrote:\n>\n> Hi Nick\n>\n> On Wed, Mar 22, 2023 at 01:06:12PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > From: Nick Hollinghurst \n> >\n> > Remove any hard-coded assumptions about the target hardware platform\n> > from the autofocus algorithm. Instead, use the \"target\" string provided\n> > by the camera tuning config and generalised statistics structures to\n> > determing parameters such as grid and region sizes.\n> >\n> > Additionally, PDAF statistics are represented by a generalised region\n> > statistics structure to be device agnostic.\n> >\n> > These changes also require the autofocus algorithm to initialise\n> > region weights on the first frame's prepare()/process() call rather\n> > than during initialisation.\n> >\n> > Signed-off-by: Nick Hollinghurst \n> > Signed-off-by: Naushir Patuck \n> > Tested-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper_imx708.cpp | 23 ++-\n> > src/ipa/raspberrypi/controller/pdaf_data.h | 21 +--\n> > src/ipa/raspberrypi/controller/rpi/af.cpp | 161 ++++++++++-----------\n> > src/ipa/raspberrypi/controller/rpi/af.h | 28 ++--\n> > 4 files changed, 119 insertions(+), 114 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx708.cpp b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > index 1f213d3c0833..641ba18f4b9d 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > @@ -69,11 +69,14 @@ private:\n> > /* Largest long exposure scale factor given as a left shift on the frame length. */\n> > static constexpr int longExposureShiftMax = 7;\n> >\n> > + static constexpr int pdafStatsRows = 12;\n> > + static constexpr int pdafStatsCols = 16;\n> > +\n> > void populateMetadata(const MdParser::RegisterMap ®isters,\n> > Metadata &metadata) const override;\n> >\n> > static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> > - PdafData &pdaf);\n> > + PdafRegions &pdaf);\n> >\n> > bool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);\n> > void putAGCStatistics(StatisticsPtr stats);\n> > @@ -120,11 +123,11 @@ void CamHelperImx708::prepare(libcamera::Span buffer, Metadata &m\n> > size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n> >\n> > if (buffer.size() > 2 * bytesPerLine) {\n> > - PdafData pdaf;\n> > + PdafRegions pdaf;\n> > if (parsePdafData(&buffer[2 * bytesPerLine],\n> > buffer.size() - 2 * bytesPerLine,\n> > mode_.bitdepth, pdaf))\n> > - metadata.set(\"pdaf.data\", pdaf);\n> > + metadata.set(\"pdaf.regions\", pdaf);\n> > }\n> >\n> > /* Parse AE-HIST data where present */\n> > @@ -239,7 +242,7 @@ void CamHelperImx708::populateMetadata(const MdParser::RegisterMap ®isters,\n> > }\n> >\n> > bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> > - unsigned bpp, PdafData &pdaf)\n> > + unsigned bpp, PdafRegions &pdaf)\n> > {\n> > size_t step = bpp >> 1; /* bytes per PDAF grid entry */\n> >\n> > @@ -248,13 +251,17 @@ bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> > return false;\n> > }\n> >\n> > + pdaf.init({ pdafStatsCols, pdafStatsRows });\n> > +\n> > ptr += 2 * step;\n> > - for (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > - for (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> > + for (unsigned i = 0; i < pdafStatsRows; ++i) {\n> > + for (unsigned j = 0; j < pdafStatsCols; ++j) {\n> > unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> > int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> > - pdaf.conf[i][j] = c;\n> > - pdaf.phase[i][j] = c ? p : 0;\n> > + PdafData pdafData;\n> > + pdafData.conf = c;\n> > + pdafData.phase = c ? p : 0;\n> > + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> > ptr += step;\n> > }\n> > }\n> > diff --git a/src/ipa/raspberrypi/controller/pdaf_data.h b/src/ipa/raspberrypi/controller/pdaf_data.h\n> > index 03c00d72c0e8..ae6ab996ded0 100644\n> > --- a/src/ipa/raspberrypi/controller/pdaf_data.h\n> > +++ b/src/ipa/raspberrypi/controller/pdaf_data.h\n> > @@ -2,20 +2,23 @@\n> > /*\n> > * Copyright (C) 2022, Raspberry Pi Ltd\n> > *\n> > - * pdaf_data.h - PDAF Metadata; for now this is\n> > - * largely based on IMX708's PDAF \"Type 1\" output.\n> > + * pdaf_data.h - PDAF Metadata\n> > */\n> > #pragma once\n> >\n> > #include \n> >\n> > -#define PDAF_DATA_ROWS 12\n> > -#define PDAF_DATA_COLS 16\n> > +#include \"region_stats.h\"\n> >\n> > -struct PdafData {\n> > - /* Confidence values, in raster order, in arbitrary units */\n> > - uint16_t conf[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > +namespace RPiController {\n> >\n> > - /* Phase error, in raster order, in s11 Q4 format (S.6.4) */\n> > - int16_t phase[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > +struct PdafData {\n> > + /* Confidence, in arbitrary units */\n> > + uint16_t conf;\n> > + /* Phase error, in s16 Q4 format (S.11.4) */\n> > + int16_t phase;\n> > };\n> > +\n> > +using PdafRegions = RegionStats;\n> > +\n> > +}; // namespace RPiController\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/af.cpp b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > index a623651875f2..3d2dad974d8b 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > @@ -174,9 +174,8 @@ Af::Af(Controller *controller)\n> > statsRegion_(0, 0, 0, 0),\n> > windows_(),\n> > useWindows_(false),\n> > - phaseWeights_{},\n> > - contrastWeights_{},\n> > - sumWeights_(0),\n> > + phaseWeights_(),\n> > + contrastWeights_(),\n> > scanState_(ScanState::Idle),\n> > initted_(false),\n> > ftarget_(-1.0),\n> > @@ -190,6 +189,8 @@ Af::Af(Controller *controller)\n> > scanData_(),\n> > reportState_(AfState::Idle)\n> > {\n> > + phaseWeights_.w.reserve(16 * 12);\n>\n> Isn't the point of the patch to get these values from the camHelper ?\n\nIdeally it would.. But right now, the helper doesn't tell us the\ndimensions of the grids in advance, so I had to \"guess\".\n\nAll these reserve() calls are perhaps an unnecessary affectation, an\nattempt to reduce memory fragmentation -- should I remove them?\n\n\n> > + contrastWeights_.w.reserve(4 * 3);\n> > scanData_.reserve(24);\n> > }\n> >\n> > @@ -226,7 +227,7 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> > << statsRegion_.y << ','\n> > << statsRegion_.width << ','\n> > << statsRegion_.height;\n> > - computeWeights();\n> > + invalidateWeights();\n> >\n> > if (scanState_ >= ScanState::Coarse && scanState_ < ScanState::Settle) {\n> > /*\n> > @@ -239,111 +240,94 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> > skipCount_ = cfg_.skipFrames;\n> > }\n> >\n> > -void Af::computeWeights()\n> > +void Af::computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols)\n> > {\n> > - constexpr int MaxCellWeight = 240 / (int)MaxWindows;\n> > + wgts->rows = rows;\n> > + wgts->cols = cols;\n> > + wgts->sum = 0;\n> > + wgts->w.resize(rows * cols);\n> > + std::fill(wgts->w.begin(), wgts->w.end(), 0);\n> >\n> > - sumWeights_ = 0;\n> > - for (int i = 0; i < PDAF_DATA_ROWS; ++i)\n> > - std::fill(phaseWeights_[i], phaseWeights_[i] + PDAF_DATA_COLS, 0);\n> > -\n> > - if (useWindows_ &&\n> > - statsRegion_.width >= PDAF_DATA_COLS && statsRegion_.height >= PDAF_DATA_ROWS) {\n> > + if (rows > 0 && cols > 0 && useWindows_ &&\n> > + statsRegion_.height >= rows && statsRegion_.width >= cols) {\n> > /*\n> > * Here we just merge all of the given windows, weighted by area.\n> > * \\todo Perhaps a better approach might be to find the phase in each\n> > * window and choose either the closest or the highest-confidence one?\n> > - *\n> > - * Using mostly \"int\" arithmetic, because Rectangle has signed x, y\n> > */\n> > - int cellH = (int)(statsRegion_.height / PDAF_DATA_ROWS);\n> > - int cellW = (int)(statsRegion_.width / PDAF_DATA_COLS);\n> > - int cellA = cellH * cellW;\n> > + const unsigned maxCellWeight = 46080u / (MaxWindows * rows * cols);\n>\n> Where does 46080u comes from ? I can't find it in the diff\n\nIt's a \"highly divisible\" number, close to but not exceeding 65536,\nchosen to anticipate the grid dimensions having only factors of 2 and\n3, and MaxWindows==10. I'm not sure if there's a better way to express\nthat in the code? For the IMX708 PDAF grid it yields maxCellWeight =\n24, as before.\n\nActually, a numerator of 65535 or 65536 (for denominator > 1) would\nwork almost as well, but it would have different rounding behaviour in\ncases where a focus window intersects a fraction of a grid cell.\n\n\n> > + const unsigned cellH = statsRegion_.height / rows;\n> > + const unsigned cellW = statsRegion_.width / cols;\n> > + const unsigned cellA = cellH * cellW;\n> >\n> > for (auto &w : windows_) {\n> > - for (int i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > - int y0 = std::max(statsRegion_.y + cellH * i, w.y);\n> > - int y1 = std::min(statsRegion_.y + cellH * (i + 1), w.y + (int)(w.height));\n> > + for (unsigned r = 0; r < rows; ++r) {\n> > + int y0 = std::max(statsRegion_.y + (int)(cellH * r), w.y);\n> > + int y1 = std::min(statsRegion_.y + (int)(cellH * (r + 1)), w.y + (int)(w.height));\n> > if (y0 >= y1)\n> > continue;\n> > y1 -= y0;\n> > - for (int j = 0; j < PDAF_DATA_COLS; ++j) {\n> > - int x0 = std::max(statsRegion_.x + cellW * j, w.x);\n> > - int x1 = std::min(statsRegion_.x + cellW * (j + 1), w.x + (int)(w.width));\n> > + for (unsigned c = 0; c < cols; ++c) {\n> > + int x0 = std::max(statsRegion_.x + (int)(cellW * c), w.x);\n> > + int x1 = std::min(statsRegion_.x + (int)(cellW * (c + 1)), w.x + (int)(w.width));\n> > if (x0 >= x1)\n> > continue;\n> > - int a = y1 * (x1 - x0);\n> > - a = (MaxCellWeight * a + cellA - 1) / cellA;\n> > - phaseWeights_[i][j] += a;\n> > - sumWeights_ += a;\n> > + unsigned a = y1 * (x1 - x0);\n> > + a = (maxCellWeight * a + cellA - 1) / cellA;\n> > + wgts->w[r * cols + c] += a;\n> > + wgts->sum += a;\n> > }\n> > }\n> > }\n> > }\n> >\n> > - if (sumWeights_ == 0) {\n> > - /*\n> > - * Default AF window is the middle 1/2 width of the middle 1/3 height\n> > - * since this maps nicely to both PDAF (16x12) and Focus (4x3) grids.\n> > - */\n> > - for (int i = PDAF_DATA_ROWS / 3; i < 2 * PDAF_DATA_ROWS / 3; ++i) {\n> > - for (int j = PDAF_DATA_COLS / 4; j < 3 * PDAF_DATA_COLS / 4; ++j) {\n> > - phaseWeights_[i][j] = MaxCellWeight;\n> > - sumWeights_ += MaxCellWeight;\n> > + if (wgts->sum == 0) {\n> > + /* Default AF window is the middle 1/2 width of the middle 1/3 height */\n> > + for (unsigned r = rows / 3; r < rows - rows / 3; ++r) {\n> > + for (unsigned c = cols / 4; c < cols - cols / 4; ++c) {\n> > + wgts->w[r * cols + c] = 1;\n> > + wgts->sum += 1;\n>\n> This seems functionally different, or was MaxCellWeight == 1 ?\n\nWhen used, we divide by the total weight (rounding down). I realised\nthat the original MaxCellWeight factor was unnecessary.\n\n\n> > }\n> > }\n> > }\n> > +}\n> >\n> > - /* Scale from PDAF to Focus Statistics grid (which has fixed size 4x3) */\n> > - constexpr int FocusStatsRows = 3;\n> > - constexpr int FocusStatsCols = 4;\n> > - static_assert(FOCUS_REGIONS == FocusStatsRows * FocusStatsCols);\n> > - static_assert(PDAF_DATA_ROWS % FocusStatsRows == 0);\n> > - static_assert(PDAF_DATA_COLS % FocusStatsCols == 0);\n> > - constexpr int YFactor = PDAF_DATA_ROWS / FocusStatsRows;\n> > - constexpr int XFactor = PDAF_DATA_COLS / FocusStatsCols;\n> > -\n> > - LOG(RPiAf, Debug) << \"Recomputed weights:\";\n> > - for (int i = 0; i < FocusStatsRows; ++i) {\n> > - for (int j = 0; j < FocusStatsCols; ++j) {\n> > - unsigned w = 0;\n> > - for (int y = 0; y < YFactor; ++y)\n> > - for (int x = 0; x < XFactor; ++x)\n> > - w += phaseWeights_[YFactor * i + y][XFactor * j + x];\n> > - contrastWeights_[FocusStatsCols * i + j] = w;\n> > - }\n> > - LOG(RPiAf, Debug) << \" \"\n> > - << contrastWeights_[FocusStatsCols * i + 0] << \" \"\n> > - << contrastWeights_[FocusStatsCols * i + 1] << \" \"\n> > - << contrastWeights_[FocusStatsCols * i + 2] << \" \"\n> > - << contrastWeights_[FocusStatsCols * i + 3];\n> > - }\n>\n> I might have missed where re-scaling is gone.. as you've tested this I\n> presum it's just me not fully getting what's happening here\n\nIt's missing because we now call the function twice: to construct\nweights for each of the PDAF (from the sensor) and CDAF (from the ISP)\ngrids in turn.\nThe code no longer assumes or asserts that one grid's size is an exact\ninteger fraction of the other.\n\n\n>\n> > +void Af::invalidateWeights()\n> > +{\n> > + phaseWeights_.sum = 0;\n> > + contrastWeights_.sum = 0;\n> > }\n> >\n> > -bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> > +bool Af::getPhase(PdafRegions const ®ions, double &phase, double &conf)\n> > {\n> > + libcamera::Size size = regions.size();\n> > + if (size.height != phaseWeights_.rows || size.width != phaseWeights_.cols ||\n> > + phaseWeights_.sum == 0) {\n> > + LOG(RPiAf, Debug) << \"Recompute Phase weights \" << size.width << 'x' << size.height;\n> > + computeWeights(&phaseWeights_, size.height, size.width);\n> > + }\n> > +\n> > uint32_t sumWc = 0;\n> > int64_t sumWcp = 0;\n> > -\n> > - for (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > - for (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> > - if (phaseWeights_[i][j]) {\n> > - uint32_t c = data.conf[i][j];\n> > - if (c >= cfg_.confThresh) {\n> > - if (c > cfg_.confClip)\n> > - c = cfg_.confClip;\n> > - c -= (cfg_.confThresh >> 2);\n> > - sumWc += phaseWeights_[i][j] * c;\n> > - c -= (cfg_.confThresh >> 2);\n> > - sumWcp += phaseWeights_[i][j] * data.phase[i][j] * (int64_t)c;\n> > - }\n> > + for (unsigned i = 0; i < regions.numRegions(); ++i) {\n> > + unsigned w = phaseWeights_.w[i];\n> > + if (w) {\n> > + const PdafData &data = regions.get(i).val;\n> > + unsigned c = data.conf;\n> > + if (c >= cfg_.confThresh) {\n> > + if (c > cfg_.confClip)\n> > + c = cfg_.confClip;\n> > + c -= (cfg_.confThresh >> 2);\n> > + sumWc += w * c;\n> > + c -= (cfg_.confThresh >> 2);\n> > + sumWcp += (int64_t)(w * c) * (int64_t)data.phase;\n> > }\n> > }\n> > }\n> >\n> > - if (0 < sumWeights_ && sumWeights_ <= sumWc) {\n> > + if (0 < phaseWeights_.sum && phaseWeights_.sum <= sumWc) {\n> > phase = (double)sumWcp / (double)sumWc;\n> > - conf = (double)sumWc / (double)sumWeights_;\n> > + conf = (double)sumWc / (double)phaseWeights_.sum;\n> > return true;\n> > } else {\n> > phase = 0.0;\n> > @@ -352,14 +336,19 @@ bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> > }\n> > }\n> >\n> > -double Af::getContrast(const FocusRegions &focusStats) const\n> > +double Af::getContrast(const FocusRegions &focusStats)\n> > {\n> > - uint32_t sumWc = 0;\n> > + libcamera::Size size = focusStats.size();\n> > + if (size.height != contrastWeights_.rows || size.width != contrastWeights_.cols || contrastWeights_.sum == 0) {\n>\n> Please break this to multiple lines\n>\n> > + LOG(RPiAf, Debug) << \"Recompute Contrast weights \" << size.width << 'x' << size.height;\n>\n> it's easy to do the same here\n\nOK\n\n\n> > + computeWeights(&contrastWeights_, size.height, size.width);\n> > + }\n> >\n> > + uint64_t sumWc = 0;\n> > for (unsigned i = 0; i < focusStats.numRegions(); ++i)\n> > - sumWc += contrastWeights_[i] * focusStats.get(i).val;\n> > + sumWc += contrastWeights_.w[i] * focusStats.get(i).val;\n> >\n> > - return (sumWeights_ == 0) ? 0.0 : (double)sumWc / (double)sumWeights_;\n> > + return (contrastWeights_.sum > 0) ? ((double)sumWc / (double)contrastWeights_.sum) : 0.0;\n> > }\n> >\n> > void Af::doPDAF(double phase, double conf)\n> > @@ -623,14 +612,14 @@ void Af::prepare(Metadata *imageMetadata)\n> >\n> > if (initted_) {\n> > /* Get PDAF from the embedded metadata, and run AF algorithm core */\n> > - PdafData data;\n> > + PdafRegions regions;\n> > double phase = 0.0, conf = 0.0;\n> > double oldFt = ftarget_;\n> > double oldFs = fsmooth_;\n> > ScanState oldSs = scanState_;\n> > uint32_t oldSt = stepCount_;\n> > - if (imageMetadata->get(\"pdaf.data\", data) == 0)\n> > - getPhase(data, phase, conf);\n> > + if (imageMetadata->get(\"pdaf.regions\", regions) == 0)\n> > + getPhase(regions, phase, conf);\n> > doAF(prevContrast_, phase, conf);\n> > updateLensPosition();\n> > LOG(RPiAf, Debug) << std::fixed << std::setprecision(2)\n> > @@ -691,7 +680,7 @@ void Af::setMetering(bool mode)\n> > {\n> > if (useWindows_ != mode) {\n> > useWindows_ = mode;\n> > - computeWeights();\n> > + invalidateWeights();\n> > }\n> > }\n> >\n> > @@ -708,7 +697,9 @@ void Af::setWindows(libcamera::Span const &wins)\n> > if (windows_.size() >= MaxWindows)\n> > break;\n> > }\n> > - computeWeights();\n> > +\n> > + if (useWindows_)\n> > + invalidateWeights();\n> > }\n> >\n> > bool Af::setLensPosition(double dioptres, int *hwpos)\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/af.h b/src/ipa/raspberrypi/controller/rpi/af.h\n> > index 7959371baf64..b479feb88c39 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/af.h\n> > +++ b/src/ipa/raspberrypi/controller/rpi/af.h\n> > @@ -11,12 +11,6 @@\n> > #include \"../pdaf_data.h\"\n> > #include \"../pwl.h\"\n> >\n> > -/*\n> > - * \\todo FOCUS_REGIONS is taken from bcm2835-isp.h, but should be made as a\n> > - * generic RegionStats structure.\n> > - */\n> > -#define FOCUS_REGIONS 12\n> > -\n> > /*\n> > * This algorithm implements a hybrid of CDAF and PDAF, favouring PDAF.\n> > *\n> > @@ -121,9 +115,20 @@ private:\n> > double conf;\n> > };\n> >\n> > - void computeWeights();\n> > - bool getPhase(PdafData const &data, double &phase, double &conf) const;\n> > - double getContrast(const FocusRegions &focusStats) const;\n> > + struct RegionWeights {\n> > + unsigned rows;\n> > + unsigned cols;\n> > + uint32_t sum;\n> > + std::vector w;\n> > +\n> > + RegionWeights()\n> > + : rows(0), cols(0), sum(0), w() {}\n> > + };\n> > +\n> > + void computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols);\n> > + void invalidateWeights();\n> > + bool getPhase(PdafRegions const ®ions, double &phase, double &conf);\n> > + double getContrast(const FocusRegions &focusStats);\n> > void doPDAF(double phase, double conf);\n> > bool earlyTerminationByPhase(double phase);\n> > double findPeak(unsigned index) const;\n> > @@ -143,9 +148,8 @@ private:\n> > libcamera::Rectangle statsRegion_;\n> > std::vector windows_;\n> > bool useWindows_;\n> > - uint8_t phaseWeights_[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > - uint16_t contrastWeights_[FOCUS_REGIONS];\n> > - uint32_t sumWeights_;\n> > + RegionWeights phaseWeights_;\n> > + RegionWeights contrastWeights_;\n> >\n> > /* Working state. */\n> > ScanState scanState_;\n> > --\n> > 2.34.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 2B743C0F1B\n\tfor ;\n\tFri, 24 Mar 2023 11:35:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 525D461ECF;\n\tFri, 24 Mar 2023 12:35:29 +0100 (CET)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2963C61ECF\n\tfor ;\n\tFri, 24 Mar 2023 12:35:27 +0100 (CET)","by mail-ed1-x52e.google.com with SMTP id b20so6822530edd.1\n\tfor ;\n\tFri, 24 Mar 2023 04:35:27 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679657729;\n\tbh=bqwFNJQAKItWtM6kki5M85tUpP2Evk52ZEQQHH8xb5U=;\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=jIgoa539OA2JCHobBxoWVjq0voAVtImb2AXO3DvgXQGjG480OOsSyMAGcBBy3wI5R\n\twq/onxnYo3313F16Az/j3IkleKbtvh+MjFbGm4SjtGoxTmnFYfiBLaavamtsuQLIgj\n\tdWhPh9nI58JGrDj/LI3KZjvvH6H8WUbM8nftXlXNKmB68uvZ84r+EVg5yhUTZ6HgJL\n\tO1leCFmamK8mXDyeJ3c/RVipSCp5z3ky6ksrg+CxoW+FGp7bKshLsDoiFzglXKjtsy\n\taZgDYT8EGja1bdSc0yuKk020eQ01d9ebGK9hednnzrv0vOtJiAnc1itYgluChhq0nu\n\tfRszpVfxc+hXA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679657726;\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=AhzYwWl6wtbo2TB/KA/mc5V6DyNOR1UfNCfKY4J7wR8=;\n\tb=Kq/Gn92o/gCcU+UD+DyGjT3MV16V2FEEAIRrqwxxGhsBbTl0Jzmy2hzA/I9vqoaUlT\n\tqQaOoW3gZR6OTj76Z7YfnceXrCdIqqNqEU/6VFFC6GuKp64ONcMcmVBvHfGReZkbF2hh\n\tWgG6rHO8zOVKt2H5up+H07/t/fBgU0a4LJEqhimDapRlmTgYD8c+oqgt8UjhFLT/2WLr\n\tckhBrKjuBmHeZ6cZQZCC43nM3qDvIO42jYMqulb1XHHgH05KUhusUEG+m2nLXJr6UuS9\n\tprF2r4ukT/wlY8XfT1lDTmRpcZca6tI4vbH9bfBLj2r/mUHy6OyC7iCz2fDe4I/gE5cF\n\tg7EA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Kq/Gn92o\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679657726;\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=AhzYwWl6wtbo2TB/KA/mc5V6DyNOR1UfNCfKY4J7wR8=;\n\tb=vgNhWQOxGgZFbSTy4z+2MYibvY01dNarqCGOD0+jxVA2V2VFbV/+/E2K2zkPPwt1Ab\n\t3auhiHkyuTYBI23xkOjIGYdANbe9/7HNQ24Tt2xkF7mEEtBXGK7orPeSyDSTZ9iDIyNI\n\tuoQ/a9S6ouCgZEJhBtDNOiqTlEMDdlZzgZguX2CW5C/sz6T/bK86sdOp/0pG6GCE8x+F\n\tec270NI2diynp5HR9AnAC+j4IV6hVsTsd/jKdp+EZIN6XMeN8inDVJvxpXyLbq7/bRuq\n\thwRAbtJpv3OKYK/vSh+Sn7NTsXl0AEGCvJKfxNDRnduDEbpUoaYpNXYrNOl4TtjSu7+g\n\t0tLw==","X-Gm-Message-State":"AAQBX9cMhdjOgYgWZ16qQZuWqt2FId10Y/kfUhXrp7ZXJtAQ2RvLpmek\n\tUCoeY2lNg6+aR/p3nnXZ6B3zXPdIDwJB9HSXz+RcVQ==","X-Google-Smtp-Source":"AKy350bYlq1cNjg5m3UgOXyfsRZRH29p2YfbxtOPoI8FxZ/dFXa7EWfKxpUc3ka9V74sNyC59QOSBjdnJrU+MBNb6i8=","X-Received":"by 2002:a17:907:7614:b0:930:a3ee:f22e with SMTP id\n\tjx20-20020a170907761400b00930a3eef22emr1176446ejc.13.1679657726495;\n\tFri, 24 Mar 2023 04:35:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20230322130612.5208-1-naush@raspberrypi.com>\n\t<20230322130612.5208-11-naush@raspberrypi.com>\n\t<20230324102127.obz2cc33qasycazv@uno.localdomain>","In-Reply-To":"<20230324102127.obz2cc33qasycazv@uno.localdomain>","Date":"Fri, 24 Mar 2023 11:35:15 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","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":"Nick Hollinghurst via libcamera-devel\n\t","Reply-To":"Nick Hollinghurst ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26730,"web_url":"https://patchwork.libcamera.org/comment/26730/","msgid":"<20230324133955.dhn7iwz37z6b3k45@uno.localdomain>","date":"2023-03-24T13:39:55","subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Nick\n\nOn Fri, Mar 24, 2023 at 11:35:15AM +0000, Nick Hollinghurst wrote:\n> Hi Jacopo,\n>\n> Thanks - comments inline.\n>\n> Nick\n>\n> On Fri, 24 Mar 2023 at 10:21, Jacopo Mondi\n> wrote:\n> >\n> > Hi Nick\n> >\n> > On Wed, Mar 22, 2023 at 01:06:12PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > > From: Nick Hollinghurst \n> > >\n> > > Remove any hard-coded assumptions about the target hardware platform\n> > > from the autofocus algorithm. Instead, use the \"target\" string provided\n> > > by the camera tuning config and generalised statistics structures to\n> > > determing parameters such as grid and region sizes.\n> > >\n> > > Additionally, PDAF statistics are represented by a generalised region\n> > > statistics structure to be device agnostic.\n> > >\n> > > These changes also require the autofocus algorithm to initialise\n> > > region weights on the first frame's prepare()/process() call rather\n> > > than during initialisation.\n> > >\n> > > Signed-off-by: Nick Hollinghurst \n> > > Signed-off-by: Naushir Patuck \n> > > Tested-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper_imx708.cpp | 23 ++-\n> > > src/ipa/raspberrypi/controller/pdaf_data.h | 21 +--\n> > > src/ipa/raspberrypi/controller/rpi/af.cpp | 161 ++++++++++-----------\n> > > src/ipa/raspberrypi/controller/rpi/af.h | 28 ++--\n> > > 4 files changed, 119 insertions(+), 114 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx708.cpp b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > > index 1f213d3c0833..641ba18f4b9d 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > > @@ -69,11 +69,14 @@ private:\n> > > /* Largest long exposure scale factor given as a left shift on the frame length. */\n> > > static constexpr int longExposureShiftMax = 7;\n> > >\n> > > + static constexpr int pdafStatsRows = 12;\n> > > + static constexpr int pdafStatsCols = 16;\n> > > +\n> > > void populateMetadata(const MdParser::RegisterMap ®isters,\n> > > Metadata &metadata) const override;\n> > >\n> > > static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> > > - PdafData &pdaf);\n> > > + PdafRegions &pdaf);\n> > >\n> > > bool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);\n> > > void putAGCStatistics(StatisticsPtr stats);\n> > > @@ -120,11 +123,11 @@ void CamHelperImx708::prepare(libcamera::Span buffer, Metadata &m\n> > > size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n> > >\n> > > if (buffer.size() > 2 * bytesPerLine) {\n> > > - PdafData pdaf;\n> > > + PdafRegions pdaf;\n> > > if (parsePdafData(&buffer[2 * bytesPerLine],\n> > > buffer.size() - 2 * bytesPerLine,\n> > > mode_.bitdepth, pdaf))\n> > > - metadata.set(\"pdaf.data\", pdaf);\n> > > + metadata.set(\"pdaf.regions\", pdaf);\n> > > }\n> > >\n> > > /* Parse AE-HIST data where present */\n> > > @@ -239,7 +242,7 @@ void CamHelperImx708::populateMetadata(const MdParser::RegisterMap ®isters,\n> > > }\n> > >\n> > > bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> > > - unsigned bpp, PdafData &pdaf)\n> > > + unsigned bpp, PdafRegions &pdaf)\n> > > {\n> > > size_t step = bpp >> 1; /* bytes per PDAF grid entry */\n> > >\n> > > @@ -248,13 +251,17 @@ bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> > > return false;\n> > > }\n> > >\n> > > + pdaf.init({ pdafStatsCols, pdafStatsRows });\n> > > +\n> > > ptr += 2 * step;\n> > > - for (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > > - for (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> > > + for (unsigned i = 0; i < pdafStatsRows; ++i) {\n> > > + for (unsigned j = 0; j < pdafStatsCols; ++j) {\n> > > unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> > > int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> > > - pdaf.conf[i][j] = c;\n> > > - pdaf.phase[i][j] = c ? p : 0;\n> > > + PdafData pdafData;\n> > > + pdafData.conf = c;\n> > > + pdafData.phase = c ? p : 0;\n> > > + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> > > ptr += step;\n> > > }\n> > > }\n> > > diff --git a/src/ipa/raspberrypi/controller/pdaf_data.h b/src/ipa/raspberrypi/controller/pdaf_data.h\n> > > index 03c00d72c0e8..ae6ab996ded0 100644\n> > > --- a/src/ipa/raspberrypi/controller/pdaf_data.h\n> > > +++ b/src/ipa/raspberrypi/controller/pdaf_data.h\n> > > @@ -2,20 +2,23 @@\n> > > /*\n> > > * Copyright (C) 2022, Raspberry Pi Ltd\n> > > *\n> > > - * pdaf_data.h - PDAF Metadata; for now this is\n> > > - * largely based on IMX708's PDAF \"Type 1\" output.\n> > > + * pdaf_data.h - PDAF Metadata\n> > > */\n> > > #pragma once\n> > >\n> > > #include \n> > >\n> > > -#define PDAF_DATA_ROWS 12\n> > > -#define PDAF_DATA_COLS 16\n> > > +#include \"region_stats.h\"\n> > >\n> > > -struct PdafData {\n> > > - /* Confidence values, in raster order, in arbitrary units */\n> > > - uint16_t conf[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > > +namespace RPiController {\n> > >\n> > > - /* Phase error, in raster order, in s11 Q4 format (S.6.4) */\n> > > - int16_t phase[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > > +struct PdafData {\n> > > + /* Confidence, in arbitrary units */\n> > > + uint16_t conf;\n> > > + /* Phase error, in s16 Q4 format (S.11.4) */\n> > > + int16_t phase;\n> > > };\n> > > +\n> > > +using PdafRegions = RegionStats;\n> > > +\n> > > +}; // namespace RPiController\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/af.cpp b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > > index a623651875f2..3d2dad974d8b 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > > @@ -174,9 +174,8 @@ Af::Af(Controller *controller)\n> > > statsRegion_(0, 0, 0, 0),\n> > > windows_(),\n> > > useWindows_(false),\n> > > - phaseWeights_{},\n> > > - contrastWeights_{},\n> > > - sumWeights_(0),\n> > > + phaseWeights_(),\n> > > + contrastWeights_(),\n> > > scanState_(ScanState::Idle),\n> > > initted_(false),\n> > > ftarget_(-1.0),\n> > > @@ -190,6 +189,8 @@ Af::Af(Controller *controller)\n> > > scanData_(),\n> > > reportState_(AfState::Idle)\n> > > {\n> > > + phaseWeights_.w.reserve(16 * 12);\n> >\n> > Isn't the point of the patch to get these values from the camHelper ?\n>\n> Ideally it would.. But right now, the helper doesn't tell us the\n> dimensions of the grids in advance, so I had to \"guess\".\n>\n\nNot strictly necessary, but just for my understanding, can't the\nhelper return the sizes so that they can be retrieved with\nController::getHardwareConfig() ?\n\n> All these reserve() calls are perhaps an unnecessary affectation, an\n> attempt to reduce memory fragmentation -- should I remove them?\n>\n\nNo no, it's fine to reserve, maybe if you can't get sizes record with\na \\todo item those should be made configurable\n\n>\n> > > + contrastWeights_.w.reserve(4 * 3);\n> > > scanData_.reserve(24);\n> > > }\n> > >\n> > > @@ -226,7 +227,7 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> > > << statsRegion_.y << ','\n> > > << statsRegion_.width << ','\n> > > << statsRegion_.height;\n> > > - computeWeights();\n> > > + invalidateWeights();\n> > >\n> > > if (scanState_ >= ScanState::Coarse && scanState_ < ScanState::Settle) {\n> > > /*\n> > > @@ -239,111 +240,94 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> > > skipCount_ = cfg_.skipFrames;\n> > > }\n> > >\n> > > -void Af::computeWeights()\n> > > +void Af::computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols)\n> > > {\n> > > - constexpr int MaxCellWeight = 240 / (int)MaxWindows;\n> > > + wgts->rows = rows;\n> > > + wgts->cols = cols;\n> > > + wgts->sum = 0;\n> > > + wgts->w.resize(rows * cols);\n> > > + std::fill(wgts->w.begin(), wgts->w.end(), 0);\n> > >\n> > > - sumWeights_ = 0;\n> > > - for (int i = 0; i < PDAF_DATA_ROWS; ++i)\n> > > - std::fill(phaseWeights_[i], phaseWeights_[i] + PDAF_DATA_COLS, 0);\n> > > -\n> > > - if (useWindows_ &&\n> > > - statsRegion_.width >= PDAF_DATA_COLS && statsRegion_.height >= PDAF_DATA_ROWS) {\n> > > + if (rows > 0 && cols > 0 && useWindows_ &&\n> > > + statsRegion_.height >= rows && statsRegion_.width >= cols) {\n> > > /*\n> > > * Here we just merge all of the given windows, weighted by area.\n> > > * \\todo Perhaps a better approach might be to find the phase in each\n> > > * window and choose either the closest or the highest-confidence one?\n> > > - *\n> > > - * Using mostly \"int\" arithmetic, because Rectangle has signed x, y\n> > > */\n> > > - int cellH = (int)(statsRegion_.height / PDAF_DATA_ROWS);\n> > > - int cellW = (int)(statsRegion_.width / PDAF_DATA_COLS);\n> > > - int cellA = cellH * cellW;\n> > > + const unsigned maxCellWeight = 46080u / (MaxWindows * rows * cols);\n> >\n> > Where does 46080u comes from ? I can't find it in the diff\n>\n> It's a \"highly divisible\" number, close to but not exceeding 65536,\n> chosen to anticipate the grid dimensions having only factors of 2 and\n> 3, and MaxWindows==10. I'm not sure if there's a better way to express\n> that in the code? For the IMX708 PDAF grid it yields maxCellWeight =\n> 24, as before.\n>\n> Actually, a numerator of 65535 or 65536 (for denominator > 1) would\n> work almost as well, but it would have different rounding behaviour in\n> cases where a focus window intersects a fraction of a grid cell.\n>\n\nThanks for the explanation. Would you mind recording that in a\ncomment ? It would also help explaning how the weights are computed,\nbut this was already like this\n\n>\n> > > + const unsigned cellH = statsRegion_.height / rows;\n> > > + const unsigned cellW = statsRegion_.width / cols;\n> > > + const unsigned cellA = cellH * cellW;\n> > >\n> > > for (auto &w : windows_) {\n> > > - for (int i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > > - int y0 = std::max(statsRegion_.y + cellH * i, w.y);\n> > > - int y1 = std::min(statsRegion_.y + cellH * (i + 1), w.y + (int)(w.height));\n> > > + for (unsigned r = 0; r < rows; ++r) {\n> > > + int y0 = std::max(statsRegion_.y + (int)(cellH * r), w.y);\n> > > + int y1 = std::min(statsRegion_.y + (int)(cellH * (r + 1)), w.y + (int)(w.height));\n> > > if (y0 >= y1)\n> > > continue;\n> > > y1 -= y0;\n> > > - for (int j = 0; j < PDAF_DATA_COLS; ++j) {\n> > > - int x0 = std::max(statsRegion_.x + cellW * j, w.x);\n> > > - int x1 = std::min(statsRegion_.x + cellW * (j + 1), w.x + (int)(w.width));\n> > > + for (unsigned c = 0; c < cols; ++c) {\n> > > + int x0 = std::max(statsRegion_.x + (int)(cellW * c), w.x);\n> > > + int x1 = std::min(statsRegion_.x + (int)(cellW * (c + 1)), w.x + (int)(w.width));\n\nWhile at here could you:\n\t\t\t\t\t int x1 = std::min(statsRegion_.x + (int)(cellW * (c + 1)),\n\t\t\t\t\t\t\t w.x + (int)(w.width));\n\n> > > if (x0 >= x1)\n> > > continue;\n> > > - int a = y1 * (x1 - x0);\n> > > - a = (MaxCellWeight * a + cellA - 1) / cellA;\n> > > - phaseWeights_[i][j] += a;\n> > > - sumWeights_ += a;\n> > > + unsigned a = y1 * (x1 - x0);\n> > > + a = (maxCellWeight * a + cellA - 1) / cellA;\n> > > + wgts->w[r * cols + c] += a;\n> > > + wgts->sum += a;\n> > > }\n> > > }\n> > > }\n> > > }\n> > >\n> > > - if (sumWeights_ == 0) {\n> > > - /*\n> > > - * Default AF window is the middle 1/2 width of the middle 1/3 height\n> > > - * since this maps nicely to both PDAF (16x12) and Focus (4x3) grids.\n> > > - */\n> > > - for (int i = PDAF_DATA_ROWS / 3; i < 2 * PDAF_DATA_ROWS / 3; ++i) {\n> > > - for (int j = PDAF_DATA_COLS / 4; j < 3 * PDAF_DATA_COLS / 4; ++j) {\n> > > - phaseWeights_[i][j] = MaxCellWeight;\n> > > - sumWeights_ += MaxCellWeight;\n> > > + if (wgts->sum == 0) {\n> > > + /* Default AF window is the middle 1/2 width of the middle 1/3 height */\n> > > + for (unsigned r = rows / 3; r < rows - rows / 3; ++r) {\n> > > + for (unsigned c = cols / 4; c < cols - cols / 4; ++c) {\n> > > + wgts->w[r * cols + c] = 1;\n> > > + wgts->sum += 1;\n> >\n> > This seems functionally different, or was MaxCellWeight == 1 ?\n>\n> When used, we divide by the total weight (rounding down). I realised\n> that the original MaxCellWeight factor was unnecessary.\n>\n>\n> > > }\n> > > }\n> > > }\n> > > +}\n> > >\n> > > - /* Scale from PDAF to Focus Statistics grid (which has fixed size 4x3) */\n> > > - constexpr int FocusStatsRows = 3;\n> > > - constexpr int FocusStatsCols = 4;\n> > > - static_assert(FOCUS_REGIONS == FocusStatsRows * FocusStatsCols);\n> > > - static_assert(PDAF_DATA_ROWS % FocusStatsRows == 0);\n> > > - static_assert(PDAF_DATA_COLS % FocusStatsCols == 0);\n> > > - constexpr int YFactor = PDAF_DATA_ROWS / FocusStatsRows;\n> > > - constexpr int XFactor = PDAF_DATA_COLS / FocusStatsCols;\n> > > -\n> > > - LOG(RPiAf, Debug) << \"Recomputed weights:\";\n> > > - for (int i = 0; i < FocusStatsRows; ++i) {\n> > > - for (int j = 0; j < FocusStatsCols; ++j) {\n> > > - unsigned w = 0;\n> > > - for (int y = 0; y < YFactor; ++y)\n> > > - for (int x = 0; x < XFactor; ++x)\n> > > - w += phaseWeights_[YFactor * i + y][XFactor * j + x];\n> > > - contrastWeights_[FocusStatsCols * i + j] = w;\n> > > - }\n> > > - LOG(RPiAf, Debug) << \" \"\n> > > - << contrastWeights_[FocusStatsCols * i + 0] << \" \"\n> > > - << contrastWeights_[FocusStatsCols * i + 1] << \" \"\n> > > - << contrastWeights_[FocusStatsCols * i + 2] << \" \"\n> > > - << contrastWeights_[FocusStatsCols * i + 3];\n> > > - }\n> >\n> > I might have missed where re-scaling is gone.. as you've tested this I\n> > presum it's just me not fully getting what's happening here\n>\n> It's missing because we now call the function twice: to construct\n> weights for each of the PDAF (from the sensor) and CDAF (from the ISP)\n> grids in turn.\n> The code no longer assumes or asserts that one grid's size is an exact\n> integer fraction of the other.\n>\n>\n> >\n> > > +void Af::invalidateWeights()\n> > > +{\n> > > + phaseWeights_.sum = 0;\n> > > + contrastWeights_.sum = 0;\n> > > }\n> > >\n> > > -bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> > > +bool Af::getPhase(PdafRegions const ®ions, double &phase, double &conf)\n> > > {\n> > > + libcamera::Size size = regions.size();\n> > > + if (size.height != phaseWeights_.rows || size.width != phaseWeights_.cols ||\n> > > + phaseWeights_.sum == 0) {\n> > > + LOG(RPiAf, Debug) << \"Recompute Phase weights \" << size.width << 'x' << size.height;\n> > > + computeWeights(&phaseWeights_, size.height, size.width);\n> > > + }\n> > > +\n> > > uint32_t sumWc = 0;\n> > > int64_t sumWcp = 0;\n> > > -\n> > > - for (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > > - for (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> > > - if (phaseWeights_[i][j]) {\n> > > - uint32_t c = data.conf[i][j];\n> > > - if (c >= cfg_.confThresh) {\n> > > - if (c > cfg_.confClip)\n> > > - c = cfg_.confClip;\n> > > - c -= (cfg_.confThresh >> 2);\n> > > - sumWc += phaseWeights_[i][j] * c;\n> > > - c -= (cfg_.confThresh >> 2);\n> > > - sumWcp += phaseWeights_[i][j] * data.phase[i][j] * (int64_t)c;\n> > > - }\n> > > + for (unsigned i = 0; i < regions.numRegions(); ++i) {\n> > > + unsigned w = phaseWeights_.w[i];\n> > > + if (w) {\n> > > + const PdafData &data = regions.get(i).val;\n> > > + unsigned c = data.conf;\n> > > + if (c >= cfg_.confThresh) {\n> > > + if (c > cfg_.confClip)\n> > > + c = cfg_.confClip;\n> > > + c -= (cfg_.confThresh >> 2);\n> > > + sumWc += w * c;\n> > > + c -= (cfg_.confThresh >> 2);\n> > > + sumWcp += (int64_t)(w * c) * (int64_t)data.phase;\n> > > }\n> > > }\n> > > }\n> > >\n> > > - if (0 < sumWeights_ && sumWeights_ <= sumWc) {\n> > > + if (0 < phaseWeights_.sum && phaseWeights_.sum <= sumWc) {\n> > > phase = (double)sumWcp / (double)sumWc;\n> > > - conf = (double)sumWc / (double)sumWeights_;\n> > > + conf = (double)sumWc / (double)phaseWeights_.sum;\n> > > return true;\n> > > } else {\n> > > phase = 0.0;\n> > > @@ -352,14 +336,19 @@ bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> > > }\n> > > }\n> > >\n> > > -double Af::getContrast(const FocusRegions &focusStats) const\n> > > +double Af::getContrast(const FocusRegions &focusStats)\n> > > {\n> > > - uint32_t sumWc = 0;\n> > > + libcamera::Size size = focusStats.size();\n> > > + if (size.height != contrastWeights_.rows || size.width != contrastWeights_.cols || contrastWeights_.sum == 0) {\n> >\n> > Please break this to multiple lines\n> >\n> > > + LOG(RPiAf, Debug) << \"Recompute Contrast weights \" << size.width << 'x' << size.height;\n> >\n> > it's easy to do the same here\n>\n> OK\n>\n>\n> > > + computeWeights(&contrastWeights_, size.height, size.width);\n> > > + }\n> > >\n> > > + uint64_t sumWc = 0;\n> > > for (unsigned i = 0; i < focusStats.numRegions(); ++i)\n> > > - sumWc += contrastWeights_[i] * focusStats.get(i).val;\n> > > + sumWc += contrastWeights_.w[i] * focusStats.get(i).val;\n> > >\n> > > - return (sumWeights_ == 0) ? 0.0 : (double)sumWc / (double)sumWeights_;\n> > > + return (contrastWeights_.sum > 0) ? ((double)sumWc / (double)contrastWeights_.sum) : 0.0;\n> > > }\n> > >\n> > > void Af::doPDAF(double phase, double conf)\n> > > @@ -623,14 +612,14 @@ void Af::prepare(Metadata *imageMetadata)\n> > >\n> > > if (initted_) {\n> > > /* Get PDAF from the embedded metadata, and run AF algorithm core */\n> > > - PdafData data;\n> > > + PdafRegions regions;\n> > > double phase = 0.0, conf = 0.0;\n> > > double oldFt = ftarget_;\n> > > double oldFs = fsmooth_;\n> > > ScanState oldSs = scanState_;\n> > > uint32_t oldSt = stepCount_;\n> > > - if (imageMetadata->get(\"pdaf.data\", data) == 0)\n> > > - getPhase(data, phase, conf);\n> > > + if (imageMetadata->get(\"pdaf.regions\", regions) == 0)\n> > > + getPhase(regions, phase, conf);\n> > > doAF(prevContrast_, phase, conf);\n> > > updateLensPosition();\n> > > LOG(RPiAf, Debug) << std::fixed << std::setprecision(2)\n> > > @@ -691,7 +680,7 @@ void Af::setMetering(bool mode)\n> > > {\n> > > if (useWindows_ != mode) {\n> > > useWindows_ = mode;\n> > > - computeWeights();\n> > > + invalidateWeights();\n> > > }\n> > > }\n> > >\n> > > @@ -708,7 +697,9 @@ void Af::setWindows(libcamera::Span const &wins)\n> > > if (windows_.size() >= MaxWindows)\n> > > break;\n> > > }\n> > > - computeWeights();\n> > > +\n> > > + if (useWindows_)\n> > > + invalidateWeights();\n> > > }\n> > >\n> > > bool Af::setLensPosition(double dioptres, int *hwpos)\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/af.h b/src/ipa/raspberrypi/controller/rpi/af.h\n> > > index 7959371baf64..b479feb88c39 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/af.h\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/af.h\n> > > @@ -11,12 +11,6 @@\n> > > #include \"../pdaf_data.h\"\n> > > #include \"../pwl.h\"\n> > >\n> > > -/*\n> > > - * \\todo FOCUS_REGIONS is taken from bcm2835-isp.h, but should be made as a\n> > > - * generic RegionStats structure.\n> > > - */\n> > > -#define FOCUS_REGIONS 12\n> > > -\n> > > /*\n> > > * This algorithm implements a hybrid of CDAF and PDAF, favouring PDAF.\n> > > *\n> > > @@ -121,9 +115,20 @@ private:\n> > > double conf;\n> > > };\n> > >\n> > > - void computeWeights();\n> > > - bool getPhase(PdafData const &data, double &phase, double &conf) const;\n> > > - double getContrast(const FocusRegions &focusStats) const;\n> > > + struct RegionWeights {\n> > > + unsigned rows;\n> > > + unsigned cols;\n> > > + uint32_t sum;\n> > > + std::vector w;\n> > > +\n> > > + RegionWeights()\n> > > + : rows(0), cols(0), sum(0), w() {}\n> > > + };\n> > > +\n> > > + void computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols);\n> > > + void invalidateWeights();\n> > > + bool getPhase(PdafRegions const ®ions, double &phase, double &conf);\n> > > + double getContrast(const FocusRegions &focusStats);\n> > > void doPDAF(double phase, double conf);\n> > > bool earlyTerminationByPhase(double phase);\n> > > double findPeak(unsigned index) const;\n> > > @@ -143,9 +148,8 @@ private:\n> > > libcamera::Rectangle statsRegion_;\n> > > std::vector windows_;\n> > > bool useWindows_;\n> > > - uint8_t phaseWeights_[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > > - uint16_t contrastWeights_[FOCUS_REGIONS];\n> > > - uint32_t sumWeights_;\n> > > + RegionWeights phaseWeights_;\n> > > + RegionWeights contrastWeights_;\n> > >\n> > > /* Working state. */\n> > > ScanState scanState_;\n> > > --\n> > > 2.34.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 B1B27C0F2A\n\tfor ;\n\tFri, 24 Mar 2023 13:40:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB1CE626D7;\n\tFri, 24 Mar 2023 14:40:00 +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 B3B2061ECF\n\tfor ;\n\tFri, 24 Mar 2023 14:39:58 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1AE27A58;\n\tFri, 24 Mar 2023 14:39:58 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679665200;\n\tbh=Wz+aYaYNPFhxx7H5JAFFSvbdjXON2ZFXjxWv7ggZ/5Q=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=dm18hR397Yo9B/spLzQM0s2ShjralW4G1gGOdXqv4J4rfiLC29uPpFyee1H/44/xI\n\txIyipmn/gEIqFyDG8vlHYK9ruAv/xP2a7ARy3oxYqb4+IJdPMzcM0Djilr+T0w83Q/\n\tkh8H0tLac5mb13zMDuaYNy11rjZsz+Mscvmiy5/BqW6eDUVhKduJDQg3JlAfLRhIAA\n\tAAW92SBOruVlfFkTTbZ9plLuO7X9w4NWFHrvg3lHUjV1un9kkJkpVb8hPt1mPZV1Bt\n\tS7PEWnpk2cEjLrP+azwdIP8cxyHTXEtrBwYRfWGc9mkZE9TEmKy2/stE8eUjeWpGHG\n\txwS0OAxy3OfJw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679665198;\n\tbh=Wz+aYaYNPFhxx7H5JAFFSvbdjXON2ZFXjxWv7ggZ/5Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NStK26aoBD8mJgLLOuwR0FWv7mLPrhWsERy8F4Ig6epL4Iizd0Suc5+zpxAbbr9Vo\n\t+qlCvjrgnhUE+9sfzf+0fon7t48TJIQvF4oYwspP22YjN86bgLG/1TeLgwN94QXULq\n\tsqsc+22Y4f1olXMNGUxpWZDgugXG4mxBHrNpRZ0o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NStK26ao\"; dkim-atps=neutral","Date":"Fri, 24 Mar 2023 14:39:55 +0100","To":"Nick Hollinghurst ","Message-ID":"<20230324133955.dhn7iwz37z6b3k45@uno.localdomain>","References":"<20230322130612.5208-1-naush@raspberrypi.com>\n\t<20230322130612.5208-11-naush@raspberrypi.com>\n\t<20230324102127.obz2cc33qasycazv@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26757,"web_url":"https://patchwork.libcamera.org/comment/26757/","msgid":"","date":"2023-03-27T10:36:54","subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","submitter":{"id":130,"url":"https://patchwork.libcamera.org/api/people/130/","name":"Nick Hollinghurst","email":"nick.hollinghurst@raspberrypi.com"},"content":"Hi Jacopo,\n\nAnother patch is on its way.\n\nOn Fri, 24 Mar 2023 at 13:39, Jacopo Mondi\n wrote:\n>\n> Hi Nick\n>\n> On Fri, Mar 24, 2023 at 11:35:15AM +0000, Nick Hollinghurst wrote:\n> > Hi Jacopo,\n> >\n> > Thanks - comments inline.\n> >\n> > Nick\n> >\n> > On Fri, 24 Mar 2023 at 10:21, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Nick\n> > >\n> > > On Wed, Mar 22, 2023 at 01:06:12PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > > > From: Nick Hollinghurst \n> > > >\n> > > > Remove any hard-coded assumptions about the target hardware platform\n> > > > from the autofocus algorithm. Instead, use the \"target\" string provided\n> > > > by the camera tuning config and generalised statistics structures to\n> > > > determing parameters such as grid and region sizes.\n> > > >\n> > > > Additionally, PDAF statistics are represented by a generalised region\n> > > > statistics structure to be device agnostic.\n> > > >\n> > > > These changes also require the autofocus algorithm to initialise\n> > > > region weights on the first frame's prepare()/process() call rather\n> > > > than during initialisation.\n> > > >\n> > > > Signed-off-by: Nick Hollinghurst \n> > > > Signed-off-by: Naushir Patuck \n> > > > Tested-by: Naushir Patuck \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper_imx708.cpp | 23 ++-\n> > > > src/ipa/raspberrypi/controller/pdaf_data.h | 21 +--\n> > > > src/ipa/raspberrypi/controller/rpi/af.cpp | 161 ++++++++++-----------\n> > > > src/ipa/raspberrypi/controller/rpi/af.h | 28 ++--\n> > > > 4 files changed, 119 insertions(+), 114 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx708.cpp b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > > > index 1f213d3c0833..641ba18f4b9d 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx708.cpp\n> > > > @@ -69,11 +69,14 @@ private:\n> > > > /* Largest long exposure scale factor given as a left shift on the frame length. */\n> > > > static constexpr int longExposureShiftMax = 7;\n> > > >\n> > > > + static constexpr int pdafStatsRows = 12;\n> > > > + static constexpr int pdafStatsCols = 16;\n> > > > +\n> > > > void populateMetadata(const MdParser::RegisterMap ®isters,\n> > > > Metadata &metadata) const override;\n> > > >\n> > > > static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,\n> > > > - PdafData &pdaf);\n> > > > + PdafRegions &pdaf);\n> > > >\n> > > > bool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);\n> > > > void putAGCStatistics(StatisticsPtr stats);\n> > > > @@ -120,11 +123,11 @@ void CamHelperImx708::prepare(libcamera::Span buffer, Metadata &m\n> > > > size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;\n> > > >\n> > > > if (buffer.size() > 2 * bytesPerLine) {\n> > > > - PdafData pdaf;\n> > > > + PdafRegions pdaf;\n> > > > if (parsePdafData(&buffer[2 * bytesPerLine],\n> > > > buffer.size() - 2 * bytesPerLine,\n> > > > mode_.bitdepth, pdaf))\n> > > > - metadata.set(\"pdaf.data\", pdaf);\n> > > > + metadata.set(\"pdaf.regions\", pdaf);\n> > > > }\n> > > >\n> > > > /* Parse AE-HIST data where present */\n> > > > @@ -239,7 +242,7 @@ void CamHelperImx708::populateMetadata(const MdParser::RegisterMap ®isters,\n> > > > }\n> > > >\n> > > > bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> > > > - unsigned bpp, PdafData &pdaf)\n> > > > + unsigned bpp, PdafRegions &pdaf)\n> > > > {\n> > > > size_t step = bpp >> 1; /* bytes per PDAF grid entry */\n> > > >\n> > > > @@ -248,13 +251,17 @@ bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,\n> > > > return false;\n> > > > }\n> > > >\n> > > > + pdaf.init({ pdafStatsCols, pdafStatsRows });\n> > > > +\n> > > > ptr += 2 * step;\n> > > > - for (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > > > - for (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> > > > + for (unsigned i = 0; i < pdafStatsRows; ++i) {\n> > > > + for (unsigned j = 0; j < pdafStatsCols; ++j) {\n> > > > unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);\n> > > > int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);\n> > > > - pdaf.conf[i][j] = c;\n> > > > - pdaf.phase[i][j] = c ? p : 0;\n> > > > + PdafData pdafData;\n> > > > + pdafData.conf = c;\n> > > > + pdafData.phase = c ? p : 0;\n> > > > + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });\n> > > > ptr += step;\n> > > > }\n> > > > }\n> > > > diff --git a/src/ipa/raspberrypi/controller/pdaf_data.h b/src/ipa/raspberrypi/controller/pdaf_data.h\n> > > > index 03c00d72c0e8..ae6ab996ded0 100644\n> > > > --- a/src/ipa/raspberrypi/controller/pdaf_data.h\n> > > > +++ b/src/ipa/raspberrypi/controller/pdaf_data.h\n> > > > @@ -2,20 +2,23 @@\n> > > > /*\n> > > > * Copyright (C) 2022, Raspberry Pi Ltd\n> > > > *\n> > > > - * pdaf_data.h - PDAF Metadata; for now this is\n> > > > - * largely based on IMX708's PDAF \"Type 1\" output.\n> > > > + * pdaf_data.h - PDAF Metadata\n> > > > */\n> > > > #pragma once\n> > > >\n> > > > #include \n> > > >\n> > > > -#define PDAF_DATA_ROWS 12\n> > > > -#define PDAF_DATA_COLS 16\n> > > > +#include \"region_stats.h\"\n> > > >\n> > > > -struct PdafData {\n> > > > - /* Confidence values, in raster order, in arbitrary units */\n> > > > - uint16_t conf[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > > > +namespace RPiController {\n> > > >\n> > > > - /* Phase error, in raster order, in s11 Q4 format (S.6.4) */\n> > > > - int16_t phase[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > > > +struct PdafData {\n> > > > + /* Confidence, in arbitrary units */\n> > > > + uint16_t conf;\n> > > > + /* Phase error, in s16 Q4 format (S.11.4) */\n> > > > + int16_t phase;\n> > > > };\n> > > > +\n> > > > +using PdafRegions = RegionStats;\n> > > > +\n> > > > +}; // namespace RPiController\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/af.cpp b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > > > index a623651875f2..3d2dad974d8b 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/af.cpp\n> > > > @@ -174,9 +174,8 @@ Af::Af(Controller *controller)\n> > > > statsRegion_(0, 0, 0, 0),\n> > > > windows_(),\n> > > > useWindows_(false),\n> > > > - phaseWeights_{},\n> > > > - contrastWeights_{},\n> > > > - sumWeights_(0),\n> > > > + phaseWeights_(),\n> > > > + contrastWeights_(),\n> > > > scanState_(ScanState::Idle),\n> > > > initted_(false),\n> > > > ftarget_(-1.0),\n> > > > @@ -190,6 +189,8 @@ Af::Af(Controller *controller)\n> > > > scanData_(),\n> > > > reportState_(AfState::Idle)\n> > > > {\n> > > > + phaseWeights_.w.reserve(16 * 12);\n> > >\n> > > Isn't the point of the patch to get these values from the camHelper ?\n> >\n> > Ideally it would.. But right now, the helper doesn't tell us the\n> > dimensions of the grids in advance, so I had to \"guess\".\n> >\n>\n> Not strictly necessary, but just for my understanding, can't the\n> helper return the sizes so that they can be retrieved with\n> Controller::getHardwareConfig() ?\n>\n> > All these reserve() calls are perhaps an unnecessary affectation, an\n> > attempt to reduce memory fragmentation -- should I remove them?\n> >\n>\n> No no, it's fine to reserve, maybe if you can't get sizes record with\n> a \\todo item those should be made configurable\n\nFor PDAF stats (from the sensor) we currently lack the plumbing to\nretrieve this. 16 * 12 is the larger of two grid sizes supported by\nIMX708, and the largest we anticipate. I've added a comment about it.\nFor Contrast stats (from ISP) we can indeed use\nController::getHardwareConfig() -- so we'll do that.\n\n\n> >\n> > > > + contrastWeights_.w.reserve(4 * 3);\n> > > > scanData_.reserve(24);\n> > > > }\n> > > >\n> > > > @@ -226,7 +227,7 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> > > > << statsRegion_.y << ','\n> > > > << statsRegion_.width << ','\n> > > > << statsRegion_.height;\n> > > > - computeWeights();\n> > > > + invalidateWeights();\n> > > >\n> > > > if (scanState_ >= ScanState::Coarse && scanState_ < ScanState::Settle) {\n> > > > /*\n> > > > @@ -239,111 +240,94 @@ void Af::switchMode(CameraMode const &cameraMode, [[maybe_unused]] Metadata *met\n> > > > skipCount_ = cfg_.skipFrames;\n> > > > }\n> > > >\n> > > > -void Af::computeWeights()\n> > > > +void Af::computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols)\n> > > > {\n> > > > - constexpr int MaxCellWeight = 240 / (int)MaxWindows;\n> > > > + wgts->rows = rows;\n> > > > + wgts->cols = cols;\n> > > > + wgts->sum = 0;\n> > > > + wgts->w.resize(rows * cols);\n> > > > + std::fill(wgts->w.begin(), wgts->w.end(), 0);\n> > > >\n> > > > - sumWeights_ = 0;\n> > > > - for (int i = 0; i < PDAF_DATA_ROWS; ++i)\n> > > > - std::fill(phaseWeights_[i], phaseWeights_[i] + PDAF_DATA_COLS, 0);\n> > > > -\n> > > > - if (useWindows_ &&\n> > > > - statsRegion_.width >= PDAF_DATA_COLS && statsRegion_.height >= PDAF_DATA_ROWS) {\n> > > > + if (rows > 0 && cols > 0 && useWindows_ &&\n> > > > + statsRegion_.height >= rows && statsRegion_.width >= cols) {\n> > > > /*\n> > > > * Here we just merge all of the given windows, weighted by area.\n> > > > * \\todo Perhaps a better approach might be to find the phase in each\n> > > > * window and choose either the closest or the highest-confidence one?\n> > > > - *\n> > > > - * Using mostly \"int\" arithmetic, because Rectangle has signed x, y\n> > > > */\n> > > > - int cellH = (int)(statsRegion_.height / PDAF_DATA_ROWS);\n> > > > - int cellW = (int)(statsRegion_.width / PDAF_DATA_COLS);\n> > > > - int cellA = cellH * cellW;\n> > > > + const unsigned maxCellWeight = 46080u / (MaxWindows * rows * cols);\n> > >\n> > > Where does 46080u comes from ? I can't find it in the diff\n> >\n> > It's a \"highly divisible\" number, close to but not exceeding 65536,\n> > chosen to anticipate the grid dimensions having only factors of 2 and\n> > 3, and MaxWindows==10. I'm not sure if there's a better way to express\n> > that in the code? For the IMX708 PDAF grid it yields maxCellWeight =\n> > 24, as before.\n> >\n> > Actually, a numerator of 65535 or 65536 (for denominator > 1) would\n> > work almost as well, but it would have different rounding behaviour in\n> > cases where a focus window intersects a fraction of a grid cell.\n> >\n>\n> Thanks for the explanation. Would you mind recording that in a\n> comment ? It would also help explaning how the weights are computed,\n> but this was already like this\n>\n> >\n> > > > + const unsigned cellH = statsRegion_.height / rows;\n> > > > + const unsigned cellW = statsRegion_.width / cols;\n> > > > + const unsigned cellA = cellH * cellW;\n> > > >\n> > > > for (auto &w : windows_) {\n> > > > - for (int i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > > > - int y0 = std::max(statsRegion_.y + cellH * i, w.y);\n> > > > - int y1 = std::min(statsRegion_.y + cellH * (i + 1), w.y + (int)(w.height));\n> > > > + for (unsigned r = 0; r < rows; ++r) {\n> > > > + int y0 = std::max(statsRegion_.y + (int)(cellH * r), w.y);\n> > > > + int y1 = std::min(statsRegion_.y + (int)(cellH * (r + 1)), w.y + (int)(w.height));\n> > > > if (y0 >= y1)\n> > > > continue;\n> > > > y1 -= y0;\n> > > > - for (int j = 0; j < PDAF_DATA_COLS; ++j) {\n> > > > - int x0 = std::max(statsRegion_.x + cellW * j, w.x);\n> > > > - int x1 = std::min(statsRegion_.x + cellW * (j + 1), w.x + (int)(w.width));\n> > > > + for (unsigned c = 0; c < cols; ++c) {\n> > > > + int x0 = std::max(statsRegion_.x + (int)(cellW * c), w.x);\n> > > > + int x1 = std::min(statsRegion_.x + (int)(cellW * (c + 1)), w.x + (int)(w.width));\n>\n> While at here could you:\n> int x1 = std::min(statsRegion_.x + (int)(cellW * (c + 1)),\n> w.x + (int)(w.width));\n>\n> > > > if (x0 >= x1)\n> > > > continue;\n> > > > - int a = y1 * (x1 - x0);\n> > > > - a = (MaxCellWeight * a + cellA - 1) / cellA;\n> > > > - phaseWeights_[i][j] += a;\n> > > > - sumWeights_ += a;\n> > > > + unsigned a = y1 * (x1 - x0);\n> > > > + a = (maxCellWeight * a + cellA - 1) / cellA;\n> > > > + wgts->w[r * cols + c] += a;\n> > > > + wgts->sum += a;\n> > > > }\n> > > > }\n> > > > }\n> > > > }\n> > > >\n> > > > - if (sumWeights_ == 0) {\n> > > > - /*\n> > > > - * Default AF window is the middle 1/2 width of the middle 1/3 height\n> > > > - * since this maps nicely to both PDAF (16x12) and Focus (4x3) grids.\n> > > > - */\n> > > > - for (int i = PDAF_DATA_ROWS / 3; i < 2 * PDAF_DATA_ROWS / 3; ++i) {\n> > > > - for (int j = PDAF_DATA_COLS / 4; j < 3 * PDAF_DATA_COLS / 4; ++j) {\n> > > > - phaseWeights_[i][j] = MaxCellWeight;\n> > > > - sumWeights_ += MaxCellWeight;\n> > > > + if (wgts->sum == 0) {\n> > > > + /* Default AF window is the middle 1/2 width of the middle 1/3 height */\n> > > > + for (unsigned r = rows / 3; r < rows - rows / 3; ++r) {\n> > > > + for (unsigned c = cols / 4; c < cols - cols / 4; ++c) {\n> > > > + wgts->w[r * cols + c] = 1;\n> > > > + wgts->sum += 1;\n> > >\n> > > This seems functionally different, or was MaxCellWeight == 1 ?\n> >\n> > When used, we divide by the total weight (rounding down). I realised\n> > that the original MaxCellWeight factor was unnecessary.\n> >\n> >\n> > > > }\n> > > > }\n> > > > }\n> > > > +}\n> > > >\n> > > > - /* Scale from PDAF to Focus Statistics grid (which has fixed size 4x3) */\n> > > > - constexpr int FocusStatsRows = 3;\n> > > > - constexpr int FocusStatsCols = 4;\n> > > > - static_assert(FOCUS_REGIONS == FocusStatsRows * FocusStatsCols);\n> > > > - static_assert(PDAF_DATA_ROWS % FocusStatsRows == 0);\n> > > > - static_assert(PDAF_DATA_COLS % FocusStatsCols == 0);\n> > > > - constexpr int YFactor = PDAF_DATA_ROWS / FocusStatsRows;\n> > > > - constexpr int XFactor = PDAF_DATA_COLS / FocusStatsCols;\n> > > > -\n> > > > - LOG(RPiAf, Debug) << \"Recomputed weights:\";\n> > > > - for (int i = 0; i < FocusStatsRows; ++i) {\n> > > > - for (int j = 0; j < FocusStatsCols; ++j) {\n> > > > - unsigned w = 0;\n> > > > - for (int y = 0; y < YFactor; ++y)\n> > > > - for (int x = 0; x < XFactor; ++x)\n> > > > - w += phaseWeights_[YFactor * i + y][XFactor * j + x];\n> > > > - contrastWeights_[FocusStatsCols * i + j] = w;\n> > > > - }\n> > > > - LOG(RPiAf, Debug) << \" \"\n> > > > - << contrastWeights_[FocusStatsCols * i + 0] << \" \"\n> > > > - << contrastWeights_[FocusStatsCols * i + 1] << \" \"\n> > > > - << contrastWeights_[FocusStatsCols * i + 2] << \" \"\n> > > > - << contrastWeights_[FocusStatsCols * i + 3];\n> > > > - }\n> > >\n> > > I might have missed where re-scaling is gone.. as you've tested this I\n> > > presum it's just me not fully getting what's happening here\n> >\n> > It's missing because we now call the function twice: to construct\n> > weights for each of the PDAF (from the sensor) and CDAF (from the ISP)\n> > grids in turn.\n> > The code no longer assumes or asserts that one grid's size is an exact\n> > integer fraction of the other.\n> >\n> >\n> > >\n> > > > +void Af::invalidateWeights()\n> > > > +{\n> > > > + phaseWeights_.sum = 0;\n> > > > + contrastWeights_.sum = 0;\n> > > > }\n> > > >\n> > > > -bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> > > > +bool Af::getPhase(PdafRegions const ®ions, double &phase, double &conf)\n> > > > {\n> > > > + libcamera::Size size = regions.size();\n> > > > + if (size.height != phaseWeights_.rows || size.width != phaseWeights_.cols ||\n> > > > + phaseWeights_.sum == 0) {\n> > > > + LOG(RPiAf, Debug) << \"Recompute Phase weights \" << size.width << 'x' << size.height;\n> > > > + computeWeights(&phaseWeights_, size.height, size.width);\n> > > > + }\n> > > > +\n> > > > uint32_t sumWc = 0;\n> > > > int64_t sumWcp = 0;\n> > > > -\n> > > > - for (unsigned i = 0; i < PDAF_DATA_ROWS; ++i) {\n> > > > - for (unsigned j = 0; j < PDAF_DATA_COLS; ++j) {\n> > > > - if (phaseWeights_[i][j]) {\n> > > > - uint32_t c = data.conf[i][j];\n> > > > - if (c >= cfg_.confThresh) {\n> > > > - if (c > cfg_.confClip)\n> > > > - c = cfg_.confClip;\n> > > > - c -= (cfg_.confThresh >> 2);\n> > > > - sumWc += phaseWeights_[i][j] * c;\n> > > > - c -= (cfg_.confThresh >> 2);\n> > > > - sumWcp += phaseWeights_[i][j] * data.phase[i][j] * (int64_t)c;\n> > > > - }\n> > > > + for (unsigned i = 0; i < regions.numRegions(); ++i) {\n> > > > + unsigned w = phaseWeights_.w[i];\n> > > > + if (w) {\n> > > > + const PdafData &data = regions.get(i).val;\n> > > > + unsigned c = data.conf;\n> > > > + if (c >= cfg_.confThresh) {\n> > > > + if (c > cfg_.confClip)\n> > > > + c = cfg_.confClip;\n> > > > + c -= (cfg_.confThresh >> 2);\n> > > > + sumWc += w * c;\n> > > > + c -= (cfg_.confThresh >> 2);\n> > > > + sumWcp += (int64_t)(w * c) * (int64_t)data.phase;\n> > > > }\n> > > > }\n> > > > }\n> > > >\n> > > > - if (0 < sumWeights_ && sumWeights_ <= sumWc) {\n> > > > + if (0 < phaseWeights_.sum && phaseWeights_.sum <= sumWc) {\n> > > > phase = (double)sumWcp / (double)sumWc;\n> > > > - conf = (double)sumWc / (double)sumWeights_;\n> > > > + conf = (double)sumWc / (double)phaseWeights_.sum;\n> > > > return true;\n> > > > } else {\n> > > > phase = 0.0;\n> > > > @@ -352,14 +336,19 @@ bool Af::getPhase(PdafData const &data, double &phase, double &conf) const\n> > > > }\n> > > > }\n> > > >\n> > > > -double Af::getContrast(const FocusRegions &focusStats) const\n> > > > +double Af::getContrast(const FocusRegions &focusStats)\n> > > > {\n> > > > - uint32_t sumWc = 0;\n> > > > + libcamera::Size size = focusStats.size();\n> > > > + if (size.height != contrastWeights_.rows || size.width != contrastWeights_.cols || contrastWeights_.sum == 0) {\n> > >\n> > > Please break this to multiple lines\n> > >\n> > > > + LOG(RPiAf, Debug) << \"Recompute Contrast weights \" << size.width << 'x' << size.height;\n> > >\n> > > it's easy to do the same here\n> >\n> > OK\n> >\n> >\n> > > > + computeWeights(&contrastWeights_, size.height, size.width);\n> > > > + }\n> > > >\n> > > > + uint64_t sumWc = 0;\n> > > > for (unsigned i = 0; i < focusStats.numRegions(); ++i)\n> > > > - sumWc += contrastWeights_[i] * focusStats.get(i).val;\n> > > > + sumWc += contrastWeights_.w[i] * focusStats.get(i).val;\n> > > >\n> > > > - return (sumWeights_ == 0) ? 0.0 : (double)sumWc / (double)sumWeights_;\n> > > > + return (contrastWeights_.sum > 0) ? ((double)sumWc / (double)contrastWeights_.sum) : 0.0;\n> > > > }\n> > > >\n> > > > void Af::doPDAF(double phase, double conf)\n> > > > @@ -623,14 +612,14 @@ void Af::prepare(Metadata *imageMetadata)\n> > > >\n> > > > if (initted_) {\n> > > > /* Get PDAF from the embedded metadata, and run AF algorithm core */\n> > > > - PdafData data;\n> > > > + PdafRegions regions;\n> > > > double phase = 0.0, conf = 0.0;\n> > > > double oldFt = ftarget_;\n> > > > double oldFs = fsmooth_;\n> > > > ScanState oldSs = scanState_;\n> > > > uint32_t oldSt = stepCount_;\n> > > > - if (imageMetadata->get(\"pdaf.data\", data) == 0)\n> > > > - getPhase(data, phase, conf);\n> > > > + if (imageMetadata->get(\"pdaf.regions\", regions) == 0)\n> > > > + getPhase(regions, phase, conf);\n> > > > doAF(prevContrast_, phase, conf);\n> > > > updateLensPosition();\n> > > > LOG(RPiAf, Debug) << std::fixed << std::setprecision(2)\n> > > > @@ -691,7 +680,7 @@ void Af::setMetering(bool mode)\n> > > > {\n> > > > if (useWindows_ != mode) {\n> > > > useWindows_ = mode;\n> > > > - computeWeights();\n> > > > + invalidateWeights();\n> > > > }\n> > > > }\n> > > >\n> > > > @@ -708,7 +697,9 @@ void Af::setWindows(libcamera::Span const &wins)\n> > > > if (windows_.size() >= MaxWindows)\n> > > > break;\n> > > > }\n> > > > - computeWeights();\n> > > > +\n> > > > + if (useWindows_)\n> > > > + invalidateWeights();\n> > > > }\n> > > >\n> > > > bool Af::setLensPosition(double dioptres, int *hwpos)\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/af.h b/src/ipa/raspberrypi/controller/rpi/af.h\n> > > > index 7959371baf64..b479feb88c39 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/af.h\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/af.h\n> > > > @@ -11,12 +11,6 @@\n> > > > #include \"../pdaf_data.h\"\n> > > > #include \"../pwl.h\"\n> > > >\n> > > > -/*\n> > > > - * \\todo FOCUS_REGIONS is taken from bcm2835-isp.h, but should be made as a\n> > > > - * generic RegionStats structure.\n> > > > - */\n> > > > -#define FOCUS_REGIONS 12\n> > > > -\n> > > > /*\n> > > > * This algorithm implements a hybrid of CDAF and PDAF, favouring PDAF.\n> > > > *\n> > > > @@ -121,9 +115,20 @@ private:\n> > > > double conf;\n> > > > };\n> > > >\n> > > > - void computeWeights();\n> > > > - bool getPhase(PdafData const &data, double &phase, double &conf) const;\n> > > > - double getContrast(const FocusRegions &focusStats) const;\n> > > > + struct RegionWeights {\n> > > > + unsigned rows;\n> > > > + unsigned cols;\n> > > > + uint32_t sum;\n> > > > + std::vector w;\n> > > > +\n> > > > + RegionWeights()\n> > > > + : rows(0), cols(0), sum(0), w() {}\n> > > > + };\n> > > > +\n> > > > + void computeWeights(RegionWeights *wgts, unsigned rows, unsigned cols);\n> > > > + void invalidateWeights();\n> > > > + bool getPhase(PdafRegions const ®ions, double &phase, double &conf);\n> > > > + double getContrast(const FocusRegions &focusStats);\n> > > > void doPDAF(double phase, double conf);\n> > > > bool earlyTerminationByPhase(double phase);\n> > > > double findPeak(unsigned index) const;\n> > > > @@ -143,9 +148,8 @@ private:\n> > > > libcamera::Rectangle statsRegion_;\n> > > > std::vector windows_;\n> > > > bool useWindows_;\n> > > > - uint8_t phaseWeights_[PDAF_DATA_ROWS][PDAF_DATA_COLS];\n> > > > - uint16_t contrastWeights_[FOCUS_REGIONS];\n> > > > - uint32_t sumWeights_;\n> > > > + RegionWeights phaseWeights_;\n> > > > + RegionWeights contrastWeights_;\n> > > >\n> > > > /* Working state. */\n> > > > ScanState scanState_;\n> > > > --\n> > > > 2.34.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 97770C0F2A\n\tfor ;\n\tMon, 27 Mar 2023 10:37:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8BAA62722;\n\tMon, 27 Mar 2023 12:37:07 +0200 (CEST)","from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com\n\t[IPv6:2a00:1450:4864:20::52c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C3A2A626D7\n\tfor ;\n\tMon, 27 Mar 2023 12:37:06 +0200 (CEST)","by mail-ed1-x52c.google.com with SMTP id y4so34106133edo.2\n\tfor ;\n\tMon, 27 Mar 2023 03:37:06 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679913427;\n\tbh=3Sxi9Gh45pwXk+TtL7yWxoBd2Ehg1WBMl6aK9W66CtE=;\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=xBO8hVEzDcgKPBsaZnCE6fE6jwUzOz/5+HhzUn/rkeEhCK4/fT0IvVU1/oMJVrGtJ\n\tZFj67l/IVYm5BeBpfgC9M2BLowPEDGVnpdZgaUnGqKUUWB521YP4NscXq2j12naQdP\n\tLWNlNjQpr61VeYCGm+ZMDoKmO6Y9uC2xV5wW6y9INhQLHYAYUz6aMuhpJ/1lzt9xHY\n\t1C9BmMkm6AttrAKGUmMcqjyahJoFEYiuDB+g91V25rTAU4eypmHCywkdeA16rXzhaG\n\tYWeGt+7IXJh1fJq6vEmnD0ZxWYV+sIW8R7Cn8XX/Iy/b9VPT1ofAwSQExGUjZfy8vL\n\tCevUZS7NdhqEg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1679913426;\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=QL9RW7frPN8Zw4lPRy8ybgXMWbTy/1E4sYQ0MCPtcUw=;\n\tb=qPgPoBJWkLDR8+7EsMbIW03YCN9CtQUGLkpgYEjjzClobrwonZga2iJPQrQxwgtkm9\n\tXsS11F3PipX+a+kiwHV9SihNavevkKAtSgd8W4joFsFh3WKpdpMFOSeahwV96iwd9bpN\n\tW3KIgKM5/IOSNX6jrapu5vZ4eM3aODH/vNLmBQf5uj1ot+z6AnfzYO22HbYyiTHJPK1r\n\tdSvdHWWLc/+WsSBILhpJYPLGpvUGxVNtV2xYEsVTWvsP0kTPwFxRFCmpdpv+OPxPbGUQ\n\t0GjWH5fwZ0xDuA64nC2gEY2fv1El/7dKTvPnQOYPWbLP8wjHK4KWb5MJBDl98TZK+12b\n\trCWw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"qPgPoBJW\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679913426;\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=QL9RW7frPN8Zw4lPRy8ybgXMWbTy/1E4sYQ0MCPtcUw=;\n\tb=WidztfuKB0C+6UHytraK3jpLx2ASfZ5GBO25dppwiatV48cKE9I3cYw83MWHV/GXFe\n\tqh0m7BBEzKK+2yyoUpo3ezZ4QvFJnqoL7HdVOa1s0OqO/24Qn8AXvOeMuREW4BvFyru6\n\tQtBlK5aA+ecFoEauaLKNBTXs+MH/tPF34OzeYPyh6Z1DtZXAId3L6H5QzB1Q6wSu4ALc\n\tEDMu0KYctB02R/+J7hxxC9Zw5Xto+M5sFYhD4JkLzN7OXw/ig8sPeDfBZK9JFztF1XDL\n\tRzYSOUurnUrn1jis2JBUsjM1CfLdR2sT8GTLZ6bjIK65Vjxbjf4N4F7e6KVMmXfxJeml\n\tAFIg==","X-Gm-Message-State":"AAQBX9fQ772/DkFJzK28rViyGxhuSIRGPuwzzRsSA48bIYBIw2hR0scD\n\tR9qvo4WYoR8+oWlrRC1Rv5gxlnNoaEWyTTfqbgpIIw==","X-Google-Smtp-Source":"AKy350Y36IBG18vx75vizvUhoGG7e+/jogp59DRDbpZ5CvWz9iSpV+eo8vxhpFNWCPpuTSYK8p5w9k8IARB4hBf9oeg=","X-Received":"by 2002:a05:6402:40ca:b0:502:3e65:44f7 with SMTP id\n\tz10-20020a05640240ca00b005023e6544f7mr3226193edb.3.1679913426087;\n\tMon, 27 Mar 2023 03:37:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20230322130612.5208-1-naush@raspberrypi.com>\n\t<20230322130612.5208-11-naush@raspberrypi.com>\n\t<20230324102127.obz2cc33qasycazv@uno.localdomain>\n\t\n\t<20230324133955.dhn7iwz37z6b3k45@uno.localdomain>","In-Reply-To":"<20230324133955.dhn7iwz37z6b3k45@uno.localdomain>","Date":"Mon, 27 Mar 2023 11:36:54 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 10/10] ipa: raspberrypi: Generalise\n\tthe autofocus algorithm","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":"Nick Hollinghurst via libcamera-devel\n\t","Reply-To":"Nick Hollinghurst ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]