[{"id":22572,"web_url":"https://patchwork.libcamera.org/comment/22572/","msgid":"","date":"2022-04-05T14:53:48","subject":"Re: [libcamera-devel] [PATCH v2] ipa: raspberrypi: alsc: Limit the\n\tcalculated lambda values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Apr 05, 2022 at 07:57:58AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Under the right circumstances, the alsc calculations could spread the colour\n\nSounds like the wrong circumstances, not the right ones :-)\n\n> errors across the entire image as lambda remains unbound. This would cause the\n> corrected image chroma values to slowly drift to incorrect values.\n> \n> This change adds a config parameter (alsc.lambda_bound) that provides an upper\n> and lower bound to the lambda value at every stage of the calculation. With this\n> change, we now adjust the lambda values so that the average across the entire\n> grid is 1 instead of normalising to the minimum value.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 57 +++++++++++++++------\n> src/ipa/raspberrypi/controller/rpi/alsc.hpp | 1 +\n> 2 files changed, 43 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index be3d1ae476cd..a88fee9f6d94 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -149,6 +149,7 @@ void Alsc::Read(boost::property_tree::ptree const ¶ms)\n> \tread_calibrations(config_.calibrations_Cb, params, \"calibrations_Cb\");\n> \tconfig_.default_ct = params.get(\"default_ct\", 4500.0);\n> \tconfig_.threshold = params.get(\"threshold\", 1e-3);\n> +\tconfig_.lambda_bound = params.get(\"lambda_bound\", 0.05);\n> }\n> \n> static double get_ct(Metadata *metadata, double default_ct);\n> @@ -610,30 +611,47 @@ static double compute_lambda_top_end(int i, double const M[XY][4],\n> \n> // Gauss-Seidel iteration with over-relaxation.\n> static double gauss_seidel2_SOR(double const M[XY][4], double omega,\n> -\t\t\t\tdouble lambda[XY])\n> +\t\t\t\tdouble lambda[XY], double lambda_bound)\n> {\n> +\tconst double min = 1 - lambda_bound, max = 1 + lambda_bound;\n> \tdouble old_lambda[XY];\n> \tint i;\n> \tfor (i = 0; i < XY; i++)\n> \t\told_lambda[i] = lambda[i];\n> \tlambda[0] = compute_lambda_bottom_start(0, M, lambda);\n> -\tfor (i = 1; i < X; i++)\n> +\tlambda[0] = std::clamp(lambda[0], min, max);\n> +\tfor (i = 1; i < X; i++) {\n> \t\tlambda[i] = compute_lambda_bottom(i, M, lambda);\n> -\tfor (; i < XY - X; i++)\n> +\t\tlambda[i] = std::clamp(lambda[i], min, max);\n> +\t}\n> +\tfor (; i < XY - X; i++) {\n> \t\tlambda[i] = compute_lambda_interior(i, M, lambda);\n> -\tfor (; i < XY - 1; i++)\n> +\t\tlambda[i] = std::clamp(lambda[i], min, max);\n> +\t}\n> +\tfor (; i < XY - 1; i++) {\n> \t\tlambda[i] = compute_lambda_top(i, M, lambda);\n> +\t\tlambda[i] = std::clamp(lambda[i], min, max);\n> +\t}\n> \tlambda[i] = compute_lambda_top_end(i, M, lambda);\n> +\tlambda[i] = std::clamp(lambda[i], min, max);\n> \t// Also solve the system from bottom to top, to help spread the updates\n> \t// better.\n> \tlambda[i] = compute_lambda_top_end(i, M, lambda);\n> -\tfor (i = XY - 2; i >= XY - X; i--)\n> +\tlambda[i] = std::clamp(lambda[i], min, max);\n> +\tfor (i = XY - 2; i >= XY - X; i--) {\n> \t\tlambda[i] = compute_lambda_top(i, M, lambda);\n> -\tfor (; i >= X; i--)\n> +\t\tlambda[i] = std::clamp(lambda[i], min, max);\n> +\t}\n> +\tfor (; i >= X; i--) {\n> \t\tlambda[i] = compute_lambda_interior(i, M, lambda);\n> -\tfor (; i >= 1; i--)\n> +\t\tlambda[i] = std::clamp(lambda[i], min, max);\n> +\t}\n> +\tfor (; i >= 1; i--) {\n> \t\tlambda[i] = compute_lambda_bottom(i, M, lambda);\n> +\t\tlambda[i] = std::clamp(lambda[i], min, max);\n> +\t}\n> \tlambda[0] = compute_lambda_bottom_start(0, M, lambda);\n> +\tlambda[0] = std::clamp(lambda[0], min, max);\n> \tdouble max_diff = 0;\n> \tfor (i = 0; i < XY; i++) {\n> \t\tlambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) * omega;\n> @@ -653,15 +671,26 @@ static void normalise(double *ptr, size_t n)\n> \t\tptr[i] /= minval;\n> }\n> \n> +// Rescale the values so that the avarage value is 1.\n\ns/avarage/average/\n\n> +static void reaverage(double *ptr, size_t n)\n\nCould you please use a span ?\n\n> +{\n> +\tdouble sum = 0;\n> +\tfor (size_t i = 0; i < n; i++)\n> +\t\tsum += ptr[i];\n\nThis could become\n\n\tdouble = std::accumulate(data.begin(), data.end(), 0.0);\n\n> +\tdouble ratio = 1 / (sum / n);\n> +\tfor (size_t i = 0; i < n; i++)\n> +\t\tptr[i] *= ratio;\n\nAnd possibly\n\n\tstd::for_each(data.begin(), data.end(), [ratio](double &n){ n *= ratio; });\n\nbut I'm not entirely sure that's more readable :-)\n\n\tfor (double &d : data)\n\t\td *= ratio;\n\nworks for me too.\n\n> +}\n> +\n> static void run_matrix_iterations(double const C[XY], double lambda[XY],\n> \t\t\t\t double const W[XY][4], double omega,\n> -\t\t\t\t int n_iter, double threshold)\n> +\t\t\t\t int n_iter, double threshold, double lambda_bound)\n> {\n> \tdouble M[XY][4];\n> \tconstruct_M(C, W, M);\n> \tdouble last_max_diff = std::numeric_limits::max();\n> \tfor (int i = 0; i < n_iter; i++) {\n> -\t\tdouble max_diff = fabs(gauss_seidel2_SOR(M, omega, lambda));\n> +\t\tdouble max_diff = fabs(gauss_seidel2_SOR(M, omega, lambda, lambda_bound));\n> \t\tif (max_diff < threshold) {\n> \t\t\tLOG(RPiAlsc, Debug)\n> \t\t\t\t<< \"Stop after \" << i + 1 << \" iterations\";\n> @@ -675,10 +704,8 @@ static void run_matrix_iterations(double const C[XY], double lambda[XY],\n> \t\t\t\t<< last_max_diff << \" to \" << max_diff;\n> \t\tlast_max_diff = max_diff;\n> \t}\n> -\t// We're going to normalise the lambdas so the smallest is 1. Not sure\n> -\t// this is really necessary as they get renormalised later, but I\n> -\t// suppose it does stop these quantities from wandering off...\n> -\tnormalise(lambda, XY);\n> +\t// We're going to normalise the lambdas so the total average is 1.\n> +\treaverage(lambda, XY);\n\nThat seems to make sense, although I wonder how it will affect the next\nALSC iteration. I suppose it doesn't matter too much, but have you\nchecked if this change has an effect on the number of iterations of the\nSOR (I'm mostly curious about the impact on the normal case, not the\n\"right circumstances\") ?\n\nReviewed-by: Laurent Pinchart \n\nI'll wait for your reply to the above questions before applying, please\nlet me know if you want to send a v3 or if I should make modifications\nlocally.\n\n> }\n> \n> static void add_luminance_rb(double result[XY], double const lambda[XY],\n> @@ -737,9 +764,9 @@ void Alsc::doAlsc()\n> \tcompute_W(Cb, config_.sigma_Cb, Wb);\n> \t// Run Gauss-Seidel iterations over the resulting matrix, for R and B.\n> \trun_matrix_iterations(Cr, lambda_r_, Wr, config_.omega, config_.n_iter,\n> -\t\t\t config_.threshold);\n> +\t\t\t config_.threshold, config_.lambda_bound);\n> \trun_matrix_iterations(Cb, lambda_b_, Wb, config_.omega, config_.n_iter,\n> -\t\t\t config_.threshold);\n> +\t\t\t config_.threshold, config_.lambda_bound);\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> \t// calibration gains included.)\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> index 9616b99ea7ca..d1dbe0d1d22d 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> @@ -41,6 +41,7 @@ struct AlscConfig {\n> \tstd::vector calibrations_Cb;\n> \tdouble default_ct; // colour temperature if no metadata found\n> \tdouble threshold; // iteration termination threshold\n> +\tdouble lambda_bound; // upper/lower bound for lambda from a value of 1\n> };\n> \n> class Alsc : public Algorithm","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 1EF81C0F1B\n\tfor ;\n\tTue, 5 Apr 2022 14:53:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D9A16563F;\n\tTue, 5 Apr 2022 16:53:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7FA4604BB\n\tfor ;\n\tTue, 5 Apr 2022 16:53:51 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69FB45D;\n\tTue, 5 Apr 2022 16:53:51 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649170433;\n\tbh=G+W9YcOlPJCPO5U48H2UeUEiawXS/rqBW/feo8QodvY=;\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=1bTaU5B4UVvnL9Asaded9aAgmWCAuPWccUzsKrY6aY9ktbVScGfEl99S+gPNo5fym\n\t5j8osV2UB9Dvwx6kYG1sTsJh4AX6v/zGMYdKGtE/hHpsnAymnN/pTnh8KaFr6KgXbp\n\tcbDlaHbkP4W6XNayKAD03/R6iLlZievyjPDRd28HFpzQnrZe4Qhi6gaL2NEKc19OR3\n\tdAiz3XSWd6LF4ifeoXUkdFhlL/OdvjWp1l2OPlVTQAQ82/Y11KqhpCmY800Ehp3qs9\n\tg98fMFLBCMWnk4IaIQfvjpBmw0Ooukf5rSQKv56ihzMyFaAbZzegJj8t0bmXTcUDOL\n\tfUbhzfUxPRkRA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649170431;\n\tbh=G+W9YcOlPJCPO5U48H2UeUEiawXS/rqBW/feo8QodvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Do7KItkOkNJg517CW+metOCkdPTAnsoHHwBcMkIo10Dlxv3hVlNz8rDyPkBkNnCVV\n\tJEzBlvi2Aas3dbW026PuhjV02x21jRO1vGPppzIRYtFg/5Bvma7xXoW5hCAdFkI/0c\n\tLqYdJ4cACxAmCFdy1PsKtlBvXAEBWzxv8LoqLv74="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Do7KItkO\"; dkim-atps=neutral","Date":"Tue, 5 Apr 2022 17:53:48 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220405065758.2166956-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220405065758.2166956-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: raspberrypi: alsc: Limit the\n\tcalculated lambda values","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22605,"web_url":"https://patchwork.libcamera.org/comment/22605/","msgid":"","date":"2022-04-06T07:02:12","subject":"Re: [libcamera-devel] [PATCH v2] ipa: raspberrypi: alsc: Limit the\n\tcalculated lambda values","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 5 Apr 2022 at 15:53, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 05, 2022 at 07:57:58AM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > Under the right circumstances, the alsc calculations could spread the\n> colour\n>\n> Sounds like the wrong circumstances, not the right ones :-)\n>\n> > errors across the entire image as lambda remains unbound. This would\n> cause the\n> > corrected image chroma values to slowly drift to incorrect values.\n> >\n> > This change adds a config parameter (alsc.lambda_bound) that provides an\n> upper\n> > and lower bound to the lambda value at every stage of the calculation.\n> With this\n> > change, we now adjust the lambda values so that the average across the\n> entire\n> > grid is 1 instead of normalising to the minimum value.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 57 +++++++++++++++------\n> > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 1 +\n> > 2 files changed, 43 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > index be3d1ae476cd..a88fee9f6d94 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > @@ -149,6 +149,7 @@ void Alsc::Read(boost::property_tree::ptree const\n> ¶ms)\n> > read_calibrations(config_.calibrations_Cb, params,\n> \"calibrations_Cb\");\n> > config_.default_ct = params.get(\"default_ct\", 4500.0);\n> > config_.threshold = params.get(\"threshold\", 1e-3);\n> > + config_.lambda_bound = params.get(\"lambda_bound\", 0.05);\n> > }\n> >\n> > static double get_ct(Metadata *metadata, double default_ct);\n> > @@ -610,30 +611,47 @@ static double compute_lambda_top_end(int i, double\n> const M[XY][4],\n> >\n> > // Gauss-Seidel iteration with over-relaxation.\n> > static double gauss_seidel2_SOR(double const M[XY][4], double omega,\n> > - double lambda[XY])\n> > + double lambda[XY], double lambda_bound)\n> > {\n> > + const double min = 1 - lambda_bound, max = 1 + lambda_bound;\n> > double old_lambda[XY];\n> > int i;\n> > for (i = 0; i < XY; i++)\n> > old_lambda[i] = lambda[i];\n> > lambda[0] = compute_lambda_bottom_start(0, M, lambda);\n> > - for (i = 1; i < X; i++)\n> > + lambda[0] = std::clamp(lambda[0], min, max);\n> > + for (i = 1; i < X; i++) {\n> > lambda[i] = compute_lambda_bottom(i, M, lambda);\n> > - for (; i < XY - X; i++)\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > + }\n> > + for (; i < XY - X; i++) {\n> > lambda[i] = compute_lambda_interior(i, M, lambda);\n> > - for (; i < XY - 1; i++)\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > + }\n> > + for (; i < XY - 1; i++) {\n> > lambda[i] = compute_lambda_top(i, M, lambda);\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > + }\n> > lambda[i] = compute_lambda_top_end(i, M, lambda);\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > // Also solve the system from bottom to top, to help spread the\n> updates\n> > // better.\n> > lambda[i] = compute_lambda_top_end(i, M, lambda);\n> > - for (i = XY - 2; i >= XY - X; i--)\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > + for (i = XY - 2; i >= XY - X; i--) {\n> > lambda[i] = compute_lambda_top(i, M, lambda);\n> > - for (; i >= X; i--)\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > + }\n> > + for (; i >= X; i--) {\n> > lambda[i] = compute_lambda_interior(i, M, lambda);\n> > - for (; i >= 1; i--)\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > + }\n> > + for (; i >= 1; i--) {\n> > lambda[i] = compute_lambda_bottom(i, M, lambda);\n> > + lambda[i] = std::clamp(lambda[i], min, max);\n> > + }\n> > lambda[0] = compute_lambda_bottom_start(0, M, lambda);\n> > + lambda[0] = std::clamp(lambda[0], min, max);\n> > double max_diff = 0;\n> > for (i = 0; i < XY; i++) {\n> > lambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) *\n> omega;\n> > @@ -653,15 +671,26 @@ static void normalise(double *ptr, size_t n)\n> > ptr[i] /= minval;\n> > }\n> >\n> > +// Rescale the values so that the avarage value is 1.\n>\n> s/avarage/average/\n>\n> > +static void reaverage(double *ptr, size_t n)\n>\n> Could you please use a span ?\n>\n\nSure, no problem.\n\n\n>\n> > +{\n> > + double sum = 0;\n> > + for (size_t i = 0; i < n; i++)\n> > + sum += ptr[i];\n>\n> This could become\n>\n> double = std::accumulate(data.begin(), data.end(), 0.0);\n>\n> > + double ratio = 1 / (sum / n);\n> > + for (size_t i = 0; i < n; i++)\n> > + ptr[i] *= ratio;\n>\n> And possibly\n>\n> std::for_each(data.begin(), data.end(), [ratio](double &n){ n *=\n> ratio; });\n>\n> but I'm not entirely sure that's more readable :-)\n>\n> for (double &d : data)\n> d *= ratio;\n>\n> works for me too.\n>\n\nI'd probably prefer the latter for readability.\n\n\n>\n> > +}\n> > +\n> > static void run_matrix_iterations(double const C[XY], double lambda[XY],\n> > double const W[XY][4], double omega,\n> > - int n_iter, double threshold)\n> > + int n_iter, double threshold, double\n> lambda_bound)\n> > {\n> > double M[XY][4];\n> > construct_M(C, W, M);\n> > double last_max_diff = std::numeric_limits::max();\n> > for (int i = 0; i < n_iter; i++) {\n> > - double max_diff = fabs(gauss_seidel2_SOR(M, omega,\n> lambda));\n> > + double max_diff = fabs(gauss_seidel2_SOR(M, omega, lambda,\n> lambda_bound));\n> > if (max_diff < threshold) {\n> > LOG(RPiAlsc, Debug)\n> > << \"Stop after \" << i + 1 << \" iterations\";\n> > @@ -675,10 +704,8 @@ static void run_matrix_iterations(double const\n> C[XY], double lambda[XY],\n> > << last_max_diff << \" to \" << max_diff;\n> > last_max_diff = max_diff;\n> > }\n> > - // We're going to normalise the lambdas so the smallest is 1. Not\n> sure\n> > - // this is really necessary as they get renormalised later, but I\n> > - // suppose it does stop these quantities from wandering off...\n> > - normalise(lambda, XY);\n> > + // We're going to normalise the lambdas so the total average is 1.\n> > + reaverage(lambda, XY);\n>\n> That seems to make sense, although I wonder how it will affect the next\n> ALSC iteration. I suppose it doesn't matter too much, but have you\n> checked if this change has an effect on the number of iterations of the\n> SOR (I'm mostly curious about the impact on the normal case, not the\n> \"right circumstances\") ?\n>\n\nThis change has no direct impact on the number of iterations run. It limits\nthe amount of error \"bleeding\" across cells through the iterations. In\ngeneral,\nwe want to ensure our calibration table is a good fit and our auto\ncorrection\nshould not deviate much from that table.\n\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> I'll wait for your reply to the above questions before applying, please\n> let me know if you want to send a v3 or if I should make modifications\n> locally.\n>\n\nHappy for you to merge with the suggested changes if you have them\navailable locally :)\n\nRegard,\nNaush\n\n\n\n>\n> > }\n> >\n> > static void add_luminance_rb(double result[XY], double const lambda[XY],\n> > @@ -737,9 +764,9 @@ void Alsc::doAlsc()\n> > compute_W(Cb, config_.sigma_Cb, Wb);\n> > // Run Gauss-Seidel iterations over the resulting matrix, for R\n> and B.\n> > run_matrix_iterations(Cr, lambda_r_, Wr, config_.omega,\n> config_.n_iter,\n> > - config_.threshold);\n> > + config_.threshold, config_.lambda_bound);\n> > run_matrix_iterations(Cb, lambda_b_, Wb, config_.omega,\n> config_.n_iter,\n> > - config_.threshold);\n> > + config_.threshold, config_.lambda_bound);\n> > // Fold the calibrated gains into our final lambda values. (Note\n> that on\n> > // the next run, we re-start with the lambda values that don't\n> have the\n> > // calibration gains included.)\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > index 9616b99ea7ca..d1dbe0d1d22d 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > @@ -41,6 +41,7 @@ struct AlscConfig {\n> > std::vector calibrations_Cb;\n> > double default_ct; // colour temperature if no metadata found\n> > double threshold; // iteration termination threshold\n> > + double lambda_bound; // upper/lower bound for lambda from a value\n> of 1\n> > };\n> >\n> > class Alsc : public Algorithm\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 0C9E2C0F1B\n\tfor ;\n\tWed, 6 Apr 2022 07:02:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A41F65642;\n\tWed, 6 Apr 2022 09:02:30 +0200 (CEST)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A9A6C604B8\n\tfor ;\n\tWed, 6 Apr 2022 09:02:28 +0200 (CEST)","by mail-lf1-x131.google.com with SMTP id e16so2386121lfc.13\n\tfor ;\n\tWed, 06 Apr 2022 00:02:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649228550;\n\tbh=WWbOUrDFSwNK/go82ikNYqBCXluGva4h6nZs9fMGrZI=;\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=Pzm1YnJMsQ6aBYWQnbiJMFbZkuLPjQvQ0UAnkdl1KR7NNNgM78UbtyLuwSigV4O/4\n\tqJC9uANipbbzY7SwPaP3V4NA8wUkcdrQaOIxGvtfpzrhxjy0nd+m2lExUKl2ziX+qE\n\tFbcd/eUElPLcI2v0HefUW8eBgU3478I7Pt9E3Kjr2aMcc29G4jZ4Ie7hFbW5eOpnGR\n\tijWeacVIbTgzFWIjdhHuxRq6Y/I+yyOcaPpgQa85XGYeItfK3DDln3ZHA4DplahiR6\n\tgwOKQNKvc5imENzipDrRbW9N7iqGa1Nz2O1pLMFAz0DoWqY7j3bVzUiFvmG9H7JAyW\n\t2RQe+KfhxBqYw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oI2oBSxkCpuCWqdZURoTWh8nJWV9Pupay0cJWX2nEOs=;\n\tb=UyWnUjkLRtoju8gHC2UPE6gp6rdCBJAd5q55B+QGCOTKU+JLf2ZUPUCqjyIKBc/ofa\n\tiyFTKpooguP61O0FkziQVGr4EVOGQ3mIjczpugf7XSAQ6sqp8gvm/NJB2OO9s5xxsQKZ\n\tixgFcSq6yvUI+r+spu/FuONCwTbVUCfJryrISj2Q90z26CyASksBkMhq1nrDUMC5ystM\n\tLJROFiOmUvA5ww6UC9AE39rtQIl/bISy6d/0YfKSkF4S3Klm1inhmH/Qwj1T6wDZpMcd\n\tmVb31DpTdB/Dt9ovFTeg6lUkYeN7LW4gfdOayRLFMo6UfA0NBpCwovljSCCcp75EJ9sM\n\tuXmQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"UyWnUjkL\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oI2oBSxkCpuCWqdZURoTWh8nJWV9Pupay0cJWX2nEOs=;\n\tb=XI98CN6tcH/XokDyBR1VufNhyqJDbF7xNQM3fAHgII2t0/c/nrVteGxS3O8Tj5aIaY\n\tHclyoc4FyFFgY3zWMY+1ieONJfN7/N/gFLYKtXoblmd4q/HIwmu03SuEi1wgHz2uCbBg\n\t7+2XpjTuol34C2HOBid1fFMpVxXZwhUyl2DNHFEnVXetXJaRRYHL9dgWI/cNk8yP+xLq\n\t/H2sc4KFCEGkBAGrtzTxawPZ3ynMc2OoJS9PcEHzyjSf2EUJrBmwcWWyggzQj4AxFo47\n\tRZHm/MlTK+y41G6nT18JUxk1kMad9uOQMbxbuN5nRy2UVOju5ae5ojpJaKkipUB7ruqm\n\tzAfQ==","X-Gm-Message-State":"AOAM532UEGUL7k0iDsUdIxZ/RscmzZEDnW19ljyeF3PSFI3CdiVzTVqM\n\tgAcmuDWRp4yirPYaDl+rcrd7iqf7cOZCh0tiAyFGmMsu+wM=","X-Google-Smtp-Source":"ABdhPJyVTyKi2SE4sAKKNzAWOdWH1vGr+olL4Q/fAfZVI/8kS8rN6D7sbZ9qy2HRCSPiAPbFG0T4aN9rXvBFkrDGVMw=","X-Received":"by 2002:ac2:5cd9:0:b0:44a:2586:a2a2 with SMTP id\n\tf25-20020ac25cd9000000b0044a2586a2a2mr5292484lfq.611.1649228548005;\n\tWed, 06 Apr 2022 00:02:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20220405065758.2166956-1-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Wed, 6 Apr 2022 08:02:12 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000c1f53105dbf6f145\"","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: raspberrypi: alsc: Limit the\n\tcalculated lambda values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22612,"web_url":"https://patchwork.libcamera.org/comment/22612/","msgid":"","date":"2022-04-06T09:52:18","subject":"Re: [libcamera-devel] [PATCH v2] ipa: raspberrypi: alsc: Limit the\n\tcalculated lambda values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Apr 06, 2022 at 08:02:12AM +0100, Naushir Patuck wrote:\n> On Tue, 5 Apr 2022 at 15:53, Laurent Pinchart wrote:\n> > On Tue, Apr 05, 2022 at 07:57:58AM +0100, Naushir Patuck via\n> > libcamera-devel wrote:\n> > > Under the right circumstances, the alsc calculations could spread the colour\n> >\n> > Sounds like the wrong circumstances, not the right ones :-)\n> >\n> > > errors across the entire image as lambda remains unbound. This would cause the\n> > > corrected image chroma values to slowly drift to incorrect values.\n> > >\n> > > This change adds a config parameter (alsc.lambda_bound) that provides an upper\n> > > and lower bound to the lambda value at every stage of the calculation. With this\n> > > change, we now adjust the lambda values so that the average across the entire\n> > > grid is 1 instead of normalising to the minimum value.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 57 +++++++++++++++------\n> > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 1 +\n> > > 2 files changed, 43 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > index be3d1ae476cd..a88fee9f6d94 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > @@ -149,6 +149,7 @@ void Alsc::Read(boost::property_tree::ptree const ¶ms)\n> > > read_calibrations(config_.calibrations_Cb, params, \"calibrations_Cb\");\n> > > config_.default_ct = params.get(\"default_ct\", 4500.0);\n> > > config_.threshold = params.get(\"threshold\", 1e-3);\n> > > + config_.lambda_bound = params.get(\"lambda_bound\", 0.05);\n> > > }\n> > >\n> > > static double get_ct(Metadata *metadata, double default_ct);\n> > > @@ -610,30 +611,47 @@ static double compute_lambda_top_end(int i, double const M[XY][4],\n> > >\n> > > // Gauss-Seidel iteration with over-relaxation.\n> > > static double gauss_seidel2_SOR(double const M[XY][4], double omega,\n> > > - double lambda[XY])\n> > > + double lambda[XY], double lambda_bound)\n> > > {\n> > > + const double min = 1 - lambda_bound, max = 1 + lambda_bound;\n> > > double old_lambda[XY];\n> > > int i;\n> > > for (i = 0; i < XY; i++)\n> > > old_lambda[i] = lambda[i];\n> > > lambda[0] = compute_lambda_bottom_start(0, M, lambda);\n> > > - for (i = 1; i < X; i++)\n> > > + lambda[0] = std::clamp(lambda[0], min, max);\n> > > + for (i = 1; i < X; i++) {\n> > > lambda[i] = compute_lambda_bottom(i, M, lambda);\n> > > - for (; i < XY - X; i++)\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > + }\n> > > + for (; i < XY - X; i++) {\n> > > lambda[i] = compute_lambda_interior(i, M, lambda);\n> > > - for (; i < XY - 1; i++)\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > + }\n> > > + for (; i < XY - 1; i++) {\n> > > lambda[i] = compute_lambda_top(i, M, lambda);\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > + }\n> > > lambda[i] = compute_lambda_top_end(i, M, lambda);\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > // Also solve the system from bottom to top, to help spread the updates\n> > > // better.\n> > > lambda[i] = compute_lambda_top_end(i, M, lambda);\n> > > - for (i = XY - 2; i >= XY - X; i--)\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > + for (i = XY - 2; i >= XY - X; i--) {\n> > > lambda[i] = compute_lambda_top(i, M, lambda);\n> > > - for (; i >= X; i--)\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > + }\n> > > + for (; i >= X; i--) {\n> > > lambda[i] = compute_lambda_interior(i, M, lambda);\n> > > - for (; i >= 1; i--)\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > + }\n> > > + for (; i >= 1; i--) {\n> > > lambda[i] = compute_lambda_bottom(i, M, lambda);\n> > > + lambda[i] = std::clamp(lambda[i], min, max);\n> > > + }\n> > > lambda[0] = compute_lambda_bottom_start(0, M, lambda);\n> > > + lambda[0] = std::clamp(lambda[0], min, max);\n> > > double max_diff = 0;\n> > > for (i = 0; i < XY; i++) {\n> > > lambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) * omega;\n> > > @@ -653,15 +671,26 @@ static void normalise(double *ptr, size_t n)\n> > > ptr[i] /= minval;\n> > > }\n> > >\n> > > +// Rescale the values so that the avarage value is 1.\n> >\n> > s/avarage/average/\n> >\n> > > +static void reaverage(double *ptr, size_t n)\n> >\n> > Could you please use a span ?\n> \n> Sure, no problem.\n> \n> > > +{\n> > > + double sum = 0;\n> > > + for (size_t i = 0; i < n; i++)\n> > > + sum += ptr[i];\n> >\n> > This could become\n> >\n> > double = std::accumulate(data.begin(), data.end(), 0.0);\n> >\n> > > + double ratio = 1 / (sum / n);\n> > > + for (size_t i = 0; i < n; i++)\n> > > + ptr[i] *= ratio;\n> >\n> > And possibly\n> >\n> > std::for_each(data.begin(), data.end(), [ratio](double &n){ n *= ratio; });\n> >\n> > but I'm not entirely sure that's more readable :-)\n> >\n> > for (double &d : data)\n> > d *= ratio;\n> >\n> > works for me too.\n> \n> I'd probably prefer the latter for readability.\n> \n> > > +}\n> > > +\n> > > static void run_matrix_iterations(double const C[XY], double lambda[XY],\n> > > double const W[XY][4], double omega,\n> > > - int n_iter, double threshold)\n> > > + int n_iter, double threshold, double lambda_bound)\n> > > {\n> > > double M[XY][4];\n> > > construct_M(C, W, M);\n> > > double last_max_diff = std::numeric_limits::max();\n> > > for (int i = 0; i < n_iter; i++) {\n> > > - double max_diff = fabs(gauss_seidel2_SOR(M, omega, lambda));\n> > > + double max_diff = fabs(gauss_seidel2_SOR(M, omega, lambda, lambda_bound));\n> > > if (max_diff < threshold) {\n> > > LOG(RPiAlsc, Debug)\n> > > << \"Stop after \" << i + 1 << \" iterations\";\n> > > @@ -675,10 +704,8 @@ static void run_matrix_iterations(double const C[XY], double lambda[XY],\n> > > << last_max_diff << \" to \" << max_diff;\n> > > last_max_diff = max_diff;\n> > > }\n> > > - // We're going to normalise the lambdas so the smallest is 1. Not sure\n> > > - // this is really necessary as they get renormalised later, but I\n> > > - // suppose it does stop these quantities from wandering off...\n> > > - normalise(lambda, XY);\n> > > + // We're going to normalise the lambdas so the total average is 1.\n> > > + reaverage(lambda, XY);\n> >\n> > That seems to make sense, although I wonder how it will affect the next\n> > ALSC iteration. I suppose it doesn't matter too much, but have you\n> > checked if this change has an effect on the number of iterations of the\n> > SOR (I'm mostly curious about the impact on the normal case, not the\n> > \"right circumstances\") ?\n> \n> This change has no direct impact on the number of iterations run. It limits\n> the amount of error \"bleeding\" across cells through the iterations.\n\nAs the code iterates up to n_iter, but stops when the delta is below the\nthreshold, I thought that restricting the lambda values could result in\nbreaking from the loop later, as avoid the \"bleeding\" means changes can\nspread more slowly and the delta could stay above the threshold for\nlonger. I could be wrong of course.\n\n> In general,\n> we want to ensure our calibration table is a good fit and our auto correction\n> should not deviate much from that table.\n> \n> > Reviewed-by: Laurent Pinchart \n> >\n> > I'll wait for your reply to the above questions before applying, please\n> > let me know if you want to send a v3 or if I should make modifications\n> > locally.\n> \n> Happy for you to merge with the suggested changes if you have them\n> available locally :)\n\nNot yet, but I can easily fix that.\n\n> > > }\n> > >\n> > > static void add_luminance_rb(double result[XY], double const lambda[XY],\n> > > @@ -737,9 +764,9 @@ void Alsc::doAlsc()\n> > > compute_W(Cb, config_.sigma_Cb, Wb);\n> > > // Run Gauss-Seidel iterations over the resulting matrix, for R and B.\n> > > run_matrix_iterations(Cr, lambda_r_, Wr, config_.omega, config_.n_iter,\n> > > - config_.threshold);\n> > > + config_.threshold, config_.lambda_bound);\n> > > run_matrix_iterations(Cb, lambda_b_, Wb, config_.omega, config_.n_iter,\n> > > - config_.threshold);\n> > > + config_.threshold, config_.lambda_bound);\n> > > // Fold the calibrated gains into our final lambda values. (Note that on\n> > > // the next run, we re-start with the lambda values that don't have the\n> > > // calibration gains included.)\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > index 9616b99ea7ca..d1dbe0d1d22d 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > @@ -41,6 +41,7 @@ struct AlscConfig {\n> > > std::vector calibrations_Cb;\n> > > double default_ct; // colour temperature if no metadata found\n> > > double threshold; // iteration termination threshold\n> > > + double lambda_bound; // upper/lower bound for lambda from a value of 1\n> > > };\n> > >\n> > > class Alsc : public Algorithm","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 B0E57C3256\n\tfor ;\n\tWed, 6 Apr 2022 09:52:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FAF565646;\n\tWed, 6 Apr 2022 11:52:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D9BC65640\n\tfor ;\n\tWed, 6 Apr 2022 11:52:22 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1E9A482;\n\tWed, 6 Apr 2022 11:52:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649238743;\n\tbh=wQ+cvaDJPmN46DZkiUjO2yuOof5CdlK4IcVi/g4HIAg=;\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=G5PIQBbcDdtQwvCLkA7Vcm7yMeLcPAOL/O7fa7UNR1nMBGh8y5OsswSj1ZnbXMPMO\n\tJMIFb2w10WfJ9y7OFGHNyQF/rn1SyJIaVpmpsIiLLEmvjJXZgH4/YRJVxw7+txilZj\n\t8AykzcmAWX2zpt1oTIb1FY41y7aSyonieKg8Fz4NJCRhmJPzrR1BXNTxzJycV2JXEx\n\t3gZxBceU9BX/cS0LARj5opXcWoCSPjq7Yi9YnujHi8Vq4BsKQ30yb6nbtPZRD7L2m5\n\t2ErHL/v9miHLlUdmV0zivpxj+gg9OZseVmuSWEwtQqjAVVd4aNRFYOo7nAJamNhQy4\n\tg5fa9Vp6kcC0A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649238741;\n\tbh=wQ+cvaDJPmN46DZkiUjO2yuOof5CdlK4IcVi/g4HIAg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P6qPGFuWsAAI7+AZ2Z3HcDY4r3K2+Qevgf1LuxVvQOoHYhR7xckd+A03xjd0re8Ec\n\tZBbxY9TnySilXNgXY0BaVU6/UfppAiYOQ5RvjeJNOXdbJj69pBynby/Xe3SYdqcUpd\n\tFuubJi+3JafgzYu5uwNZZZLwo2CjnzqAg+V1Cto0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"P6qPGFuW\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 12:52:18 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220405065758.2166956-1-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: raspberrypi: alsc: Limit the\n\tcalculated lambda values","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]