[{"id":26722,"web_url":"https://patchwork.libcamera.org/comment/26722/","msgid":"<20230324085225.6pjqzwbcvi6h2rnr@uno.localdomain>","date":"2023-03-24T08:52:25","subject":"Re: [libcamera-devel] [PATCH v1 04/10] ipa: raspberrypi: alsc:\n\tReplace std::vectors by Array2D class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Mar 22, 2023 at 01:06:06PM +0000, Naushir Patuck via libcamera-devel wrote:\n> From: David Plowman \n>\n> The Array2D class is a very thin wrapper round std::vector that can be\n> used almost identically in the code, but it carries its 2D size with\n> it so that we aren't passing it around all the time.\n>\n> All the std::vectors that were X * Y in size (X and Y being the ALSC\n> grid size) have been replaced. The sparse matrices that are XY * 4 in\n> size have not been as they are somewhat different, are used\n> differently, require more code changes, and actually make things more\n> confusing if everything looks like an Array2D but are not the same.\n>\n> There should be no change in algorithm behaviour at all.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 218 ++++++++++----------\n> src/ipa/raspberrypi/controller/rpi/alsc.h | 68 +++++-\n> 2 files changed, 164 insertions(+), 122 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 51fe5d73f52d..524c48093590 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -49,11 +49,10 @@ char const *Alsc::name() const\n> \treturn NAME;\n> }\n>\n> -static int generateLut(std::vector &lut, const libcamera::YamlObject ¶ms,\n> -\t\t const Size &size)\n> +static int generateLut(Array2D &lut, const libcamera::YamlObject ¶ms)\n> {\n> \t/* These must be signed ints for the co-ordinate calculations below. */\n> -\tint X = size.width, Y = size.height;\n> +\tint X = lut.dimensions().width, Y = lut.dimensions().height;\n> \tdouble cstrength = params[\"corner_strength\"].get(2.0);\n> \tif (cstrength <= 1.0) {\n> \t\tLOG(RPiAlsc, Error) << \"corner_strength must be > 1.0\";\n> @@ -82,9 +81,9 @@ static int generateLut(std::vector &lut, const libcamera::YamlObject &pa\n> \treturn 0;\n> }\n>\n> -static int readLut(std::vector &lut, const libcamera::YamlObject ¶ms, const Size &size)\n> +static int readLut(Array2D &lut, const libcamera::YamlObject ¶ms)\n> {\n> -\tif (params.size() != size.width * size.height) {\n> +\tif (params.size() != lut.size()) {\n> \t\tLOG(RPiAlsc, Error) << \"Invalid number of entries in LSC table\";\n> \t\treturn -EINVAL;\n> \t}\n> @@ -128,7 +127,7 @@ static int readCalibrations(std::vector &calibrations,\n> \t\t\t}\n>\n> \t\t\tint num = 0;\n> -\t\t\tcalibration.table.resize(size.width * size.height);\n> +\t\t\tcalibration.table.resize(size);\n> \t\t\tfor (const auto &elem : table.asList()) {\n> \t\t\t\tvalue = elem.get();\n> \t\t\t\tif (!value)\n> @@ -160,13 +159,13 @@ int Alsc::read(const libcamera::YamlObject ¶ms)\n> \tconfig_.luminanceStrength =\n> \t\tparams[\"luminance_strength\"].get(1.0);\n>\n> -\tconfig_.luminanceLut.resize(config_.tableSize.width * config_.tableSize.height, 1.0);\n> +\tconfig_.luminanceLut.resize(config_.tableSize, 1.0);\n\nThis threw me off, but luminanceLut is now an Array2D which can be\nresized with a Size\n\n> \tint ret = 0;\n>\n> \tif (params.contains(\"corner_strength\"))\n> -\t\tret = generateLut(config_.luminanceLut, params, config_.tableSize);\n> +\t\tret = generateLut(config_.luminanceLut, params);\n> \telse if (params.contains(\"luminance_lut\"))\n> -\t\tret = readLut(config_.luminanceLut, params[\"luminance_lut\"], config_.tableSize);\n> +\t\tret = readLut(config_.luminanceLut, params[\"luminance_lut\"]);\n> \telse\n> \t\tLOG(RPiAlsc, Warning)\n> \t\t\t<< \"no luminance table - assume unity everywhere\";\n> @@ -191,16 +190,16 @@ int Alsc::read(const libcamera::YamlObject ¶ms)\n>\n> static double getCt(Metadata *metadata, double defaultCt);\n> static void getCalTable(double ct, std::vector const &calibrations,\n> -\t\t\tstd::vector &calTable);\n> -static void resampleCalTable(const std::vector &calTableIn, CameraMode const &cameraMode,\n> -\t\t\t const Size &size, std::vector &calTableOut);\n> -static void compensateLambdasForCal(const std::vector &calTable,\n> -\t\t\t\t const std::vector &oldLambdas,\n> -\t\t\t\t std::vector &newLambdas);\n> -static void addLuminanceToTables(std::array, 3> &results,\n> -\t\t\t\t const std::vector &lambdaR, double lambdaG,\n> -\t\t\t\t const std::vector &lambdaB,\n> -\t\t\t\t const std::vector &luminanceLut,\n> +\t\t\tArray2D &calTable);\n> +static void resampleCalTable(const Array2D &calTableIn, CameraMode const &cameraMode,\n> +\t\t\t Array2D &calTableOut);\n> +static void compensateLambdasForCal(const Array2D &calTable,\n> +\t\t\t\t const Array2D &oldLambdas,\n> +\t\t\t\t Array2D &newLambdas);\n> +static void addLuminanceToTables(std::array, 3> &results,\n> +\t\t\t\t const Array2D &lambdaR, double lambdaG,\n> +\t\t\t\t const Array2D &lambdaB,\n> +\t\t\t\t const Array2D &luminanceLut,\n> \t\t\t\t double luminanceStrength);\n>\n> void Alsc::initialise()\n> @@ -212,22 +211,22 @@ void Alsc::initialise()\n> \tconst size_t XY = config_.tableSize.width * config_.tableSize.height;\n>\n> \tfor (auto &r : syncResults_)\n> -\t\tr.resize(XY);\n> +\t\tr.resize(config_.tableSize);\n> \tfor (auto &r : prevSyncResults_)\n> -\t\tr.resize(XY);\n> +\t\tr.resize(config_.tableSize);\n> \tfor (auto &r : asyncResults_)\n> -\t\tr.resize(XY);\n> +\t\tr.resize(config_.tableSize);\n>\n> -\tluminanceTable_.resize(XY);\n> -\tasyncLambdaR_.resize(XY);\n> -\tasyncLambdaB_.resize(XY);\n> +\tluminanceTable_.resize(config_.tableSize);\n> +\tasyncLambdaR_.resize(config_.tableSize);\n> +\tasyncLambdaB_.resize(config_.tableSize);\n> \t/* The lambdas are initialised in the SwitchMode. */\n> -\tlambdaR_.resize(XY);\n> -\tlambdaB_.resize(XY);\n> +\tlambdaR_.resize(config_.tableSize);\n> +\tlambdaB_.resize(config_.tableSize);\n>\n> \t/* Temporaries for the computations, but sensible to allocate this up-front! */\n> \tfor (auto &c : tmpC_)\n> -\t\tc.resize(XY);\n> +\t\tc.resize(config_.tableSize);\n> \tfor (auto &m : tmpM_)\n> \t\tm.resize(XY);\n> }\n> @@ -290,7 +289,7 @@ void Alsc::switchMode(CameraMode const &cameraMode,\n> \t * We must resample the luminance table like we do the others, but it's\n> \t * fixed so we can simply do it up front here.\n> \t */\n> -\tresampleCalTable(config_.luminanceLut, cameraMode_, config_.tableSize, luminanceTable_);\n> +\tresampleCalTable(config_.luminanceLut, cameraMode_, luminanceTable_);\n>\n> \tif (resetTables) {\n> \t\t/*\n> @@ -302,11 +301,11 @@ void Alsc::switchMode(CameraMode const &cameraMode,\n> \t\t */\n> \t\tstd::fill(lambdaR_.begin(), lambdaR_.end(), 1.0);\n> \t\tstd::fill(lambdaB_.begin(), lambdaB_.end(), 1.0);\n> -\t\tstd::vector &calTableR = tmpC_[0], &calTableB = tmpC_[1], &calTableTmp = tmpC_[2];\n> +\t\tArray2D &calTableR = tmpC_[0], &calTableB = tmpC_[1], &calTableTmp = tmpC_[2];\n> \t\tgetCalTable(ct_, config_.calibrationsCr, calTableTmp);\n> -\t\tresampleCalTable(calTableTmp, cameraMode_, config_.tableSize, calTableR);\n> +\t\tresampleCalTable(calTableTmp, cameraMode_, calTableR);\n> \t\tgetCalTable(ct_, config_.calibrationsCb, calTableTmp);\n> -\t\tresampleCalTable(calTableTmp, cameraMode_, config_.tableSize, calTableB);\n> +\t\tresampleCalTable(calTableTmp, cameraMode_, calTableB);\n> \t\tcompensateLambdasForCal(calTableR, lambdaR_, asyncLambdaR_);\n> \t\tcompensateLambdasForCal(calTableB, lambdaB_, asyncLambdaB_);\n> \t\taddLuminanceToTables(syncResults_, asyncLambdaR_, 1.0, asyncLambdaB_,\n> @@ -411,9 +410,9 @@ void Alsc::prepare(Metadata *imageMetadata)\n> \t}\n> \t/* Put output values into status metadata. */\n> \tAlscStatus status;\n> -\tstatus.r = prevSyncResults_[0];\n> -\tstatus.g = prevSyncResults_[1];\n> -\tstatus.b = prevSyncResults_[2];\n> +\tstatus.r = prevSyncResults_[0].data();\n> +\tstatus.g = prevSyncResults_[1].data();\n> +\tstatus.b = prevSyncResults_[2].data();\n> \timageMetadata->set(\"alsc.status\", status);\n> }\n>\n> @@ -457,7 +456,7 @@ void Alsc::asyncFunc()\n> }\n>\n> void getCalTable(double ct, std::vector const &calibrations,\n> -\t\t std::vector &calTable)\n> +\t\t Array2D &calTable)\n> {\n> \tif (calibrations.empty()) {\n> \t\tstd::fill(calTable.begin(), calTable.end(), 1.0);\n> @@ -486,12 +485,12 @@ void getCalTable(double ct, std::vector const &calibrations,\n> \t}\n> }\n>\n> -void resampleCalTable(const std::vector &calTableIn,\n> -\t\t CameraMode const &cameraMode, const Size &size,\n> -\t\t std::vector &calTableOut)\n> +void resampleCalTable(const Array2D &calTableIn,\n> +\t\t CameraMode const &cameraMode,\n> +\t\t Array2D &calTableOut)\n> {\n> -\tint X = size.width;\n> -\tint Y = size.height;\n> +\tint X = calTableIn.dimensions().width;\n> +\tint Y = calTableIn.dimensions().height;\n>\n> \t/*\n> \t * Precalculate and cache the x sampling locations and phases to save\n> @@ -529,9 +528,9 @@ void resampleCalTable(const std::vector &calTableIn,\n> \t\t\tyLo = Y - 1 - yLo;\n> \t\t\tyHi = Y - 1 - yHi;\n> \t\t}\n> -\t\tdouble const *rowAbove = calTableIn.data() + X * yLo;\n> -\t\tdouble const *rowBelow = calTableIn.data() + X * yHi;\n> -\t\tdouble *out = calTableOut.data() + X * j;\n> +\t\tdouble const *rowAbove = calTableIn.ptr() + X * yLo;\n> +\t\tdouble const *rowBelow = calTableIn.ptr() + X * yHi;\n> +\t\tdouble *out = calTableOut.ptr() + X * j;\n> \t\tfor (int i = 0; i < X; i++) {\n> \t\t\tdouble above = rowAbove[xLo[i]] * (1 - xf[i]) +\n> \t\t\t\t rowAbove[xHi[i]] * xf[i];\n> @@ -543,8 +542,8 @@ void resampleCalTable(const std::vector &calTableIn,\n> }\n>\n> /* Calculate chrominance statistics (R/G and B/G) for each region. */\n> -static void calculateCrCb(const RgbyRegions &awbRegion, std::vector &cr,\n> -\t\t\t std::vector &cb, uint32_t minCount, uint16_t minG)\n> +static void calculateCrCb(const RgbyRegions &awbRegion, Array2D &cr,\n> +\t\t\t Array2D &cb, uint32_t minCount, uint16_t minG)\n> {\n> \tfor (unsigned int i = 0; i < cr.size(); i++) {\n> \t\tauto s = awbRegion.get(i);\n> @@ -559,16 +558,16 @@ static void calculateCrCb(const RgbyRegions &awbRegion, std::vector &cr,\n> \t}\n> }\n>\n> -static void applyCalTable(const std::vector &calTable, std::vector &C)\n> +static void applyCalTable(const Array2D &calTable, Array2D &C)\n> {\n> \tfor (unsigned int i = 0; i < C.size(); i++)\n> \t\tif (C[i] != InsufficientData)\n> \t\t\tC[i] *= calTable[i];\n> }\n>\n> -void compensateLambdasForCal(const std::vector &calTable,\n> -\t\t\t const std::vector &oldLambdas,\n> -\t\t\t std::vector &newLambdas)\n> +void compensateLambdasForCal(const Array2D &calTable,\n> +\t\t\t const Array2D &oldLambdas,\n> +\t\t\t Array2D &newLambdas)\n> {\n> \tdouble minNewLambda = std::numeric_limits::max();\n> \tfor (unsigned int i = 0; i < newLambdas.size(); i++) {\n> @@ -579,9 +578,9 @@ void compensateLambdasForCal(const std::vector &calTable,\n> \t\tnewLambdas[i] /= minNewLambda;\n> }\n>\n> -[[maybe_unused]] static void printCalTable(const std::vector &C,\n> -\t\t\t\t\t const Size &size)\n> +[[maybe_unused]] static void printCalTable(const Array2D &C)\n> {\n> +\tconst Size &size = C.dimensions();\n> \tprintf(\"table: [\\n\");\n> \tfor (unsigned int j = 0; j < size.height; j++) {\n> \t\tfor (unsigned int i = 0; i < size.width; i++) {\n> @@ -607,11 +606,11 @@ static double computeWeight(double Ci, double Cj, double sigma)\n> }\n>\n> /* Compute all weights. */\n> -static void computeW(const std::vector &C, double sigma,\n> -\t\t std::vector> &W, const Size &size)\n> +static void computeW(const Array2D &C, double sigma,\n> +\t\t std::vector> &W)\n> {\n> -\tsize_t XY = size.width * size.height;\n> -\tsize_t X = size.width;\n> +\tsize_t XY = C.size();\n> +\tsize_t X = C.dimensions().width;\n>\n> \tfor (unsigned int i = 0; i < XY; i++) {\n> \t\t/* Start with neighbour above and go clockwise. */\n> @@ -623,13 +622,12 @@ static void computeW(const std::vector &C, double sigma,\n> }\n>\n> /* Compute M, the large but sparse matrix such that M * lambdas = 0. */\n> -static void constructM(const std::vector &C,\n> +static void constructM(const Array2D &C,\n> \t\t const std::vector> &W,\n> -\t\t std::vector> &M,\n> -\t\t const Size &size)\n> +\t\t std::vector> &M)\n> {\n> -\tsize_t XY = size.width * size.height;\n> -\tsize_t X = size.width;\n> +\tsize_t XY = C.size();\n> +\tsize_t X = C.dimensions().width;\n>\n> \tdouble epsilon = 0.001;\n> \tfor (unsigned int i = 0; i < XY; i++) {\n> @@ -654,79 +652,78 @@ static void constructM(const std::vector &C,\n> * need to test the i value to exclude them.\n> */\n> static double computeLambdaBottom(int i, const std::vector> &M,\n> -\t\t\t\t std::vector &lambda, const Size &size)\n> +\t\t\t\t Array2D &lambda)\n> {\n> -\treturn M[i][1] * lambda[i + 1] + M[i][2] * lambda[i + size.width] +\n> +\treturn M[i][1] * lambda[i + 1] + M[i][2] * lambda[i + lambda.dimensions().width] +\n> \t M[i][3] * lambda[i - 1];\n> }\n> static double computeLambdaBottomStart(int i, const std::vector> &M,\n> -\t\t\t\t std::vector &lambda, const Size &size)\n> +\t\t\t\t Array2D &lambda)\n> {\n> -\treturn M[i][1] * lambda[i + 1] + M[i][2] * lambda[i + size.width];\n> +\treturn M[i][1] * lambda[i + 1] + M[i][2] * lambda[i + lambda.dimensions().width];\n> }\n> static double computeLambdaInterior(int i, const std::vector> &M,\n> -\t\t\t\t std::vector &lambda, const Size &size)\n> +\t\t\t\t Array2D &lambda)\n> {\n> -\treturn M[i][0] * lambda[i - size.width] + M[i][1] * lambda[i + 1] +\n> -\t M[i][2] * lambda[i + size.width] + M[i][3] * lambda[i - 1];\n> +\treturn M[i][0] * lambda[i - lambda.dimensions().width] + M[i][1] * lambda[i + 1] +\n> +\t M[i][2] * lambda[i + lambda.dimensions().width] + M[i][3] * lambda[i - 1];\n> }\n> static double computeLambdaTop(int i, const std::vector> &M,\n> -\t\t\t std::vector &lambda, const Size &size)\n> +\t\t\t Array2D &lambda)\n> {\n> -\treturn M[i][0] * lambda[i - size.width] + M[i][1] * lambda[i + 1] +\n> +\treturn M[i][0] * lambda[i - lambda.dimensions().width] + M[i][1] * lambda[i + 1] +\n> \t M[i][3] * lambda[i - 1];\n> }\n> static double computeLambdaTopEnd(int i, const std::vector> &M,\n> -\t\t\t\t std::vector &lambda, const Size &size)\n> +\t\t\t\t Array2D &lambda)\n> {\n> -\treturn M[i][0] * lambda[i - size.width] + M[i][3] * lambda[i - 1];\n> +\treturn M[i][0] * lambda[i - lambda.dimensions().width] + M[i][3] * lambda[i - 1];\n> }\n>\n> /* Gauss-Seidel iteration with over-relaxation. */\n> static double gaussSeidel2Sor(const std::vector> &M, double omega,\n> -\t\t\t std::vector &lambda, double lambdaBound,\n> -\t\t\t const Size &size)\n> +\t\t\t Array2D &lambda, double lambdaBound)\n> {\n> -\tint XY = size.width * size.height;\n> -\tint X = size.width;\n> +\tint XY = lambda.size();\n> +\tint X = lambda.dimensions().width;\n> \tconst double min = 1 - lambdaBound, max = 1 + lambdaBound;\n> -\tstd::vector oldLambda = lambda;\n> +\tArray2D oldLambda = lambda;\n> \tint i;\n> -\tlambda[0] = computeLambdaBottomStart(0, M, lambda, size);\n> +\tlambda[0] = computeLambdaBottomStart(0, M, lambda);\n> \tlambda[0] = std::clamp(lambda[0], min, max);\n> \tfor (i = 1; i < X; i++) {\n> -\t\tlambda[i] = computeLambdaBottom(i, M, lambda, size);\n> +\t\tlambda[i] = computeLambdaBottom(i, M, lambda);\n> \t\tlambda[i] = std::clamp(lambda[i], min, max);\n> \t}\n> \tfor (; i < XY - X; i++) {\n> -\t\tlambda[i] = computeLambdaInterior(i, M, lambda, size);\n> +\t\tlambda[i] = computeLambdaInterior(i, M, lambda);\n> \t\tlambda[i] = std::clamp(lambda[i], min, max);\n> \t}\n> \tfor (; i < XY - 1; i++) {\n> -\t\tlambda[i] = computeLambdaTop(i, M, lambda, size);\n> +\t\tlambda[i] = computeLambdaTop(i, M, lambda);\n> \t\tlambda[i] = std::clamp(lambda[i], min, max);\n> \t}\n> -\tlambda[i] = computeLambdaTopEnd(i, M, lambda, size);\n> +\tlambda[i] = computeLambdaTopEnd(i, M, lambda);\n> \tlambda[i] = std::clamp(lambda[i], min, max);\n> \t/*\n> \t * Also solve the system from bottom to top, to help spread the updates\n> \t * better.\n> \t */\n> -\tlambda[i] = computeLambdaTopEnd(i, M, lambda, size);\n> +\tlambda[i] = computeLambdaTopEnd(i, M, lambda);\n> \tlambda[i] = std::clamp(lambda[i], min, max);\n> \tfor (i = XY - 2; i >= XY - X; i--) {\n> -\t\tlambda[i] = computeLambdaTop(i, M, lambda, size);\n> +\t\tlambda[i] = computeLambdaTop(i, M, lambda);\n> \t\tlambda[i] = std::clamp(lambda[i], min, max);\n> \t}\n> \tfor (; i >= X; i--) {\n> -\t\tlambda[i] = computeLambdaInterior(i, M, lambda, size);\n> +\t\tlambda[i] = computeLambdaInterior(i, M, lambda);\n> \t\tlambda[i] = std::clamp(lambda[i], min, max);\n> \t}\n> \tfor (; i >= 1; i--) {\n> -\t\tlambda[i] = computeLambdaBottom(i, M, lambda, size);\n> +\t\tlambda[i] = computeLambdaBottom(i, M, lambda);\n> \t\tlambda[i] = std::clamp(lambda[i], min, max);\n> \t}\n> -\tlambda[0] = computeLambdaBottomStart(0, M, lambda, size);\n> +\tlambda[0] = computeLambdaBottomStart(0, M, lambda);\n> \tlambda[0] = std::clamp(lambda[0], min, max);\n> \tdouble maxDiff = 0;\n> \tfor (i = 0; i < XY; i++) {\n> @@ -738,7 +735,7 @@ static double gaussSeidel2Sor(const std::vector> &M, doubl\n> }\n>\n> /* Normalise the values so that the smallest value is 1. */\n> -static void normalise(std::vector &results)\n> +static void normalise(Array2D &results)\n> {\n> \tdouble minval = *std::min_element(results.begin(), results.end());\n> \tstd::for_each(results.begin(), results.end(),\n> @@ -746,7 +743,7 @@ static void normalise(std::vector &results)\n> }\n>\n> /* Rescale the values so that the average value is 1. */\n> -static void reaverage(std::vector &data)\n> +static void reaverage(Array2D &data)\n> {\n> \tdouble sum = std::accumulate(data.begin(), data.end(), 0.0);\n> \tdouble ratio = 1 / (sum / data.size());\n> @@ -754,17 +751,16 @@ static void reaverage(std::vector &data)\n> \t\t [ratio](double val) { return val * ratio; });\n> }\n>\n> -static void runMatrixIterations(const std::vector &C,\n> -\t\t\t\tstd::vector &lambda,\n> +static void runMatrixIterations(const Array2D &C,\n> +\t\t\t\tArray2D &lambda,\n> \t\t\t\tconst std::vector> &W,\n> \t\t\t\tstd::vector> &M, double omega,\n> -\t\t\t\tunsigned int nIter, double threshold, double lambdaBound,\n> -\t\t\t\tconst Size &size)\n> +\t\t\t\tunsigned int nIter, double threshold, double lambdaBound)\n> {\n> -\tconstructM(C, W, M, size);\n> +\tconstructM(C, W, M);\n> \tdouble lastMaxDiff = std::numeric_limits::max();\n> \tfor (unsigned int i = 0; i < nIter; i++) {\n> -\t\tdouble maxDiff = fabs(gaussSeidel2Sor(M, omega, lambda, lambdaBound, size));\n> +\t\tdouble maxDiff = fabs(gaussSeidel2Sor(M, omega, lambda, lambdaBound));\n> \t\tif (maxDiff < threshold) {\n> \t\t\tLOG(RPiAlsc, Debug)\n> \t\t\t\t<< \"Stop after \" << i + 1 << \" iterations\";\n> @@ -784,26 +780,26 @@ static void runMatrixIterations(const std::vector &C,\n> \treaverage(lambda);\n> }\n>\n> -static void addLuminanceRb(std::vector &result, const std::vector &lambda,\n> -\t\t\t const std::vector &luminanceLut,\n> +static void addLuminanceRb(Array2D &result, const Array2D &lambda,\n> +\t\t\t const Array2D &luminanceLut,\n> \t\t\t double luminanceStrength)\n> {\n> \tfor (unsigned int i = 0; i < result.size(); i++)\n> \t\tresult[i] = lambda[i] * ((luminanceLut[i] - 1) * luminanceStrength + 1);\n> }\n>\n> -static void addLuminanceG(std::vector &result, double lambda,\n> -\t\t\t const std::vector &luminanceLut,\n> +static void addLuminanceG(Array2D &result, double lambda,\n> +\t\t\t const Array2D &luminanceLut,\n> \t\t\t double luminanceStrength)\n> {\n> \tfor (unsigned int i = 0; i < result.size(); i++)\n> \t\tresult[i] = lambda * ((luminanceLut[i] - 1) * luminanceStrength + 1);\n> }\n>\n> -void addLuminanceToTables(std::array, 3> &results,\n> -\t\t\t const std::vector &lambdaR,\n> -\t\t\t double lambdaG, const std::vector &lambdaB,\n> -\t\t\t const std::vector &luminanceLut,\n> +void addLuminanceToTables(std::array, 3> &results,\n> +\t\t\t const Array2D &lambdaR,\n> +\t\t\t double lambdaG, const Array2D &lambdaB,\n> +\t\t\t const Array2D &luminanceLut,\n> \t\t\t double luminanceStrength)\n> {\n> \taddLuminanceRb(results[0], lambdaR, luminanceLut, luminanceStrength);\n> @@ -815,8 +811,8 @@ void addLuminanceToTables(std::array, 3> &results,\n>\n> void Alsc::doAlsc()\n> {\n> -\tstd::vector &cr = tmpC_[0], &cb = tmpC_[1], &calTableR = tmpC_[2],\n> -\t\t\t &calTableB = tmpC_[3], &calTableTmp = tmpC_[4];\n> +\tArray2D &cr = tmpC_[0], &cb = tmpC_[1], &calTableR = tmpC_[2],\n> +\t\t\t&calTableB = tmpC_[3], &calTableTmp = tmpC_[4];\n> \tstd::vector> &wr = tmpM_[0], &wb = tmpM_[1], &M = tmpM_[2];\n>\n> \t/*\n> @@ -829,9 +825,9 @@ void Alsc::doAlsc()\n> \t * case the camera mode is not full-frame.\n> \t */\n> \tgetCalTable(ct_, config_.calibrationsCr, calTableTmp);\n> -\tresampleCalTable(calTableTmp, cameraMode_, config_.tableSize, calTableR);\n> +\tresampleCalTable(calTableTmp, cameraMode_, calTableR);\n> \tgetCalTable(ct_, config_.calibrationsCb, calTableTmp);\n> -\tresampleCalTable(calTableTmp, cameraMode_, config_.tableSize, calTableB);\n> +\tresampleCalTable(calTableTmp, cameraMode_, calTableB);\n> \t/*\n> \t * You could print out the cal tables for this image here, if you're\n> \t * tuning the algorithm...\n> @@ -841,13 +837,13 @@ void Alsc::doAlsc()\n> \tapplyCalTable(calTableR, cr);\n> \tapplyCalTable(calTableB, cb);\n> \t/* Compute weights between zones. */\n> -\tcomputeW(cr, config_.sigmaCr, wr, config_.tableSize);\n> -\tcomputeW(cb, config_.sigmaCb, wb, config_.tableSize);\n> +\tcomputeW(cr, config_.sigmaCr, wr);\n> +\tcomputeW(cb, config_.sigmaCb, wb);\n> \t/* Run Gauss-Seidel iterations over the resulting matrix, for R and B. */\n> \trunMatrixIterations(cr, lambdaR_, wr, M, config_.omega, config_.nIter,\n> -\t\t\t config_.threshold, config_.lambdaBound, config_.tableSize);\n> +\t\t\t config_.threshold, config_.lambdaBound);\n> \trunMatrixIterations(cb, lambdaB_, wb, M, config_.omega, config_.nIter,\n> -\t\t\t config_.threshold, config_.lambdaBound, config_.tableSize);\n> +\t\t\t config_.threshold, config_.lambdaBound);\n> \t/*\n> \t * Fold the calibrated gains into our final lambda values. (Note that on\n> \t * the next run, we re-start with the lambda values that don't have the\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> index 85e998db40e9..1ab61299c4cd 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> @@ -22,9 +22,55 @@ namespace RPiController {\n>\n> /* Algorithm to generate automagic LSC (Lens Shading Correction) tables. */\n>\n> +/*\n> + * The Array2D class is a very thin wrapper round std::vector so that it can\n> + * be used in exactly the same way in the code but carries its correct width\n> + * and height (\"dimensions\") with it.\n> + */\n> +\n> +template\n> +class Array2D\n\nI would have\n class Array2D : private std::vector\n {\n public:\n\tusing std::vector::size;\n\tusing std::vector::data;\n\tusing std::vector::begin;\n\tusing std::vector::end;\n\tusing std::vector::cbegin;\n\tusing std::vector::cend;\n\n etc\n\n }\n\n> +{\n> +public:\n> +\tusing Size = libcamera::Size;\n> +\n> +\tconst Size &dimensions() const { return dimensions_; }\n\nand here exposed width() and height() and maintain the overall size\ninternal (if not per the std::vector::size() )\n\nit's your IPA internal stuff, so up to you\n\nThe rest looks ok\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\n> +\tsize_t size() const { return data_.size(); }\n> +\n> +\tconst std::vector &data() const { return data_; }\n> +\n> +\tvoid resize(const Size &dims)\n> +\t{\n> +\t\tdimensions_ = dims;\n> +\t\tdata_.resize(dims.width * dims.height);\n> +\t}\n> +\n> +\tvoid resize(const Size &dims, const T &value)\n> +\t{\n> +\t\tresize(dims);\n> +\t\tstd::fill(data_.begin(), data_.end(), value);\n> +\t}\n> +\n> +\tT &operator[](int index) { return data_[index]; }\n> +\n> +\tconst T &operator[](int index) const { return data_[index]; }\n> +\n> +\tT *ptr() { return data_.data(); }\n> +\n> +\tconst T *ptr() const { return data_.data(); }\n> +\n> +\tauto begin() { return data_.begin(); }\n> +\tauto end() { return data_.end(); }\n> +\n> +private:\n> +\tSize dimensions_;\n> +\tstd::vector data_;\n> +};\n> +\n> struct AlscCalibration {\n> \tdouble ct;\n> -\tstd::vector table;\n> +\tArray2D table;\n> };\n>\n> struct AlscConfig {\n> @@ -40,7 +86,7 @@ struct AlscConfig {\n> \tuint16_t minG;\n> \tdouble omega;\n> \tuint32_t nIter;\n> -\tstd::vector luminanceLut;\n> +\tArray2D luminanceLut;\n> \tdouble luminanceStrength;\n> \tstd::vector calibrationsCr;\n> \tstd::vector calibrationsCb;\n> @@ -67,7 +113,7 @@ private:\n> \tAlscConfig config_;\n> \tbool firstTime_;\n> \tCameraMode cameraMode_;\n> -\tstd::vector luminanceTable_;\n> +\tArray2D luminanceTable_;\n> \tstd::thread asyncThread_;\n> \tvoid asyncFunc(); /* asynchronous thread function */\n> \tstd::mutex mutex_;\n> @@ -93,8 +139,8 @@ private:\n> \tint frameCount_;\n> \t/* counts up to startupFrames for Process function */\n> \tint frameCount2_;\n> -\tstd::array, 3> syncResults_;\n> -\tstd::array, 3> prevSyncResults_;\n> +\tstd::array, 3> syncResults_;\n> +\tstd::array, 3> prevSyncResults_;\n> \tvoid waitForAysncThread();\n> \t/*\n> \t * The following are for the asynchronous thread to use, though the main\n> @@ -105,15 +151,15 @@ private:\n> \tvoid fetchAsyncResults();\n> \tdouble ct_;\n> \tRgbyRegions statistics_;\n> -\tstd::array, 3> asyncResults_;\n> -\tstd::vector asyncLambdaR_;\n> -\tstd::vector asyncLambdaB_;\n> +\tstd::array, 3> asyncResults_;\n> +\tArray2D asyncLambdaR_;\n> +\tArray2D asyncLambdaB_;\n> \tvoid doAlsc();\n> -\tstd::vector lambdaR_;\n> -\tstd::vector lambdaB_;\n> +\tArray2D lambdaR_;\n> +\tArray2D lambdaB_;\n>\n> \t/* Temporaries for the computations */\n> -\tstd::array, 5> tmpC_;\n> +\tstd::array, 5> tmpC_;\n> \tstd::array>, 3> tmpM_;\n> };\n>\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 CAB9FC0F1B\n\tfor ;\n\tFri, 24 Mar 2023 08:52:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB44A626E3;\n\tFri, 24 Mar 2023 09:52:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0EBF361ED0\n\tfor ;\n\tFri, 24 Mar 2023 09:52:29 +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 72A56A49;\n\tFri, 24 Mar 2023 09:52:28 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679647950;\n\tbh=yv8RktDzvTRcjlEdzdcsW7DDaU5EhOMNsoVtEmNbbOg=;\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=gVbwXufkHd55u+ozD85Rlsvgb5TI/5UJa8R58UrWB8+HB1AIqWH+HFbB9v90gr9az\n\tRlttZ8Kz0pLKTPbrLu5mSV2+N31nrerk2UOZ2fSJskzmUMPg7xiZAM9WLjAZ5MEspV\n\ttLu6wQnUfF3zplijx2w/ZETfQPEWebrON6SnWrAPPOdbfkiRejYUXU/rTbvTdbsxuN\n\to6v5s0aJA3diV5xLTGINnZITGcrdwhVnDptCDTt9noyVh47k6qIQgMJ2dc5e46PPvJ\n\tH0psaoacCWCv6vKtb/hiIknaQYp5lVELyK/HrbDetxrhJKjC4aoyAvGXjKBQOBHBUI\n\tPdoTQZy3A3+4w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679647948;\n\tbh=yv8RktDzvTRcjlEdzdcsW7DDaU5EhOMNsoVtEmNbbOg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q1ogKybzmE6k+xlbhs8uxA/O0qWGlsEuse+QrMRRAhM8eLYzkyQTNlkMgbiX7y3wi\n\t7MqRueG4/UGelPjx/8PCBNUcxVz7EVVGo8ha4ItbkIHmQRHp0dgT7wkaPRJLMkH6JD\n\tCUSLbmjMfHWq/YHF69LfPO6+LX2Qm6kQYNhh9qLM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Q1ogKybz\"; dkim-atps=neutral","Date":"Fri, 24 Mar 2023 09:52:25 +0100","To":"Naushir Patuck ","Message-ID":"<20230324085225.6pjqzwbcvi6h2rnr@uno.localdomain>","References":"<20230322130612.5208-1-naush@raspberrypi.com>\n\t<20230322130612.5208-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230322130612.5208-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 04/10] ipa: raspberrypi: alsc:\n\tReplace std::vectors by Array2D class","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":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]