<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 5 Apr 2022 at 15:53, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Apr 05, 2022 at 07:57:58AM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> Under the right circumstances, the alsc calculations could spread the colour<br>
<br>
Sounds like the wrong circumstances, not the right ones :-)<br>
<br>
> errors across the entire image as lambda remains unbound. This would cause the<br>
> corrected image chroma values to slowly drift to incorrect values.<br>
> <br>
> This change adds a config parameter (alsc.lambda_bound) that provides an upper<br>
> and lower bound to the lambda value at every stage of the calculation. With this<br>
> change, we now adjust the lambda values so that the average across the entire<br>
> grid is 1 instead of normalising to the minimum value.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 57 +++++++++++++++------<br>
> src/ipa/raspberrypi/controller/rpi/alsc.hpp | 1 +<br>
> 2 files changed, 43 insertions(+), 15 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp<br>
> index be3d1ae476cd..a88fee9f6d94 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp<br>
> @@ -149,6 +149,7 @@ void Alsc::Read(boost::property_tree::ptree const ¶ms)<br>
> read_calibrations(config_.calibrations_Cb, params, "calibrations_Cb");<br>
> config_.default_ct = params.get<double>("default_ct", 4500.0);<br>
> config_.threshold = params.get<double>("threshold", 1e-3);<br>
> + config_.lambda_bound = params.get<double>("lambda_bound", 0.05);<br>
> }<br>
> <br>
> static double get_ct(Metadata *metadata, double default_ct);<br>
> @@ -610,30 +611,47 @@ static double compute_lambda_top_end(int i, double const M[XY][4],<br>
> <br>
> // Gauss-Seidel iteration with over-relaxation.<br>
> static double gauss_seidel2_SOR(double const M[XY][4], double omega,<br>
> - double lambda[XY])<br>
> + double lambda[XY], double lambda_bound)<br>
> {<br>
> + const double min = 1 - lambda_bound, max = 1 + lambda_bound;<br>
> double old_lambda[XY];<br>
> int i;<br>
> for (i = 0; i < XY; i++)<br>
> old_lambda[i] = lambda[i];<br>
> lambda[0] = compute_lambda_bottom_start(0, M, lambda);<br>
> - for (i = 1; i < X; i++)<br>
> + lambda[0] = std::clamp(lambda[0], min, max);<br>
> + for (i = 1; i < X; i++) {<br>
> lambda[i] = compute_lambda_bottom(i, M, lambda);<br>
> - for (; i < XY - X; i++)<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> + }<br>
> + for (; i < XY - X; i++) {<br>
> lambda[i] = compute_lambda_interior(i, M, lambda);<br>
> - for (; i < XY - 1; i++)<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> + }<br>
> + for (; i < XY - 1; i++) {<br>
> lambda[i] = compute_lambda_top(i, M, lambda);<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> + }<br>
> lambda[i] = compute_lambda_top_end(i, M, lambda);<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> // Also solve the system from bottom to top, to help spread the updates<br>
> // better.<br>
> lambda[i] = compute_lambda_top_end(i, M, lambda);<br>
> - for (i = XY - 2; i >= XY - X; i--)<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> + for (i = XY - 2; i >= XY - X; i--) {<br>
> lambda[i] = compute_lambda_top(i, M, lambda);<br>
> - for (; i >= X; i--)<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> + }<br>
> + for (; i >= X; i--) {<br>
> lambda[i] = compute_lambda_interior(i, M, lambda);<br>
> - for (; i >= 1; i--)<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> + }<br>
> + for (; i >= 1; i--) {<br>
> lambda[i] = compute_lambda_bottom(i, M, lambda);<br>
> + lambda[i] = std::clamp(lambda[i], min, max);<br>
> + }<br>
> lambda[0] = compute_lambda_bottom_start(0, M, lambda);<br>
> + lambda[0] = std::clamp(lambda[0], min, max);<br>
> double max_diff = 0;<br>
> for (i = 0; i < XY; i++) {<br>
> lambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) * omega;<br>
> @@ -653,15 +671,26 @@ static void normalise(double *ptr, size_t n)<br>
> ptr[i] /= minval;<br>
> }<br>
> <br>
> +// Rescale the values so that the avarage value is 1.<br>
<br>
s/avarage/average/<br>
<br>
> +static void reaverage(double *ptr, size_t n)<br>
<br>
Could you please use a span ?<br></blockquote><div><br></div><div>Sure, no problem.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +{<br>
> + double sum = 0;<br>
> + for (size_t i = 0; i < n; i++)<br>
> + sum += ptr[i];<br>
<br>
This could become<br>
<br>
double = std::accumulate(data.begin(), data.end(), 0.0);<br>
<br>
> + double ratio = 1 / (sum / n);<br>
> + for (size_t i = 0; i < n; i++)<br>
> + ptr[i] *= ratio;<br>
<br>
And possibly<br>
<br>
std::for_each(data.begin(), data.end(), [ratio](double &n){ n *= ratio; });<br>
<br>
but I'm not entirely sure that's more readable :-)<br>
<br>
for (double &d : data)<br>
d *= ratio;<br>
<br>
works for me too.<br></blockquote><div><br></div><div>I'd probably prefer the latter for readability.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +}<br>
> +<br>
> static void run_matrix_iterations(double const C[XY], double lambda[XY],<br>
> double const W[XY][4], double omega,<br>
> - int n_iter, double threshold)<br>
> + int n_iter, double threshold, double lambda_bound)<br>
> {<br>
> double M[XY][4];<br>
> construct_M(C, W, M);<br>
> double last_max_diff = std::numeric_limits<double>::max();<br>
> for (int i = 0; i < n_iter; i++) {<br>
> - double max_diff = fabs(gauss_seidel2_SOR(M, omega, lambda));<br>
> + double max_diff = fabs(gauss_seidel2_SOR(M, omega, lambda, lambda_bound));<br>
> if (max_diff < threshold) {<br>
> LOG(RPiAlsc, Debug)<br>
> << "Stop after " << i + 1 << " iterations";<br>
> @@ -675,10 +704,8 @@ static void run_matrix_iterations(double const C[XY], double lambda[XY],<br>
> << last_max_diff << " to " << max_diff;<br>
> last_max_diff = max_diff;<br>
> }<br>
> - // We're going to normalise the lambdas so the smallest is 1. Not sure<br>
> - // this is really necessary as they get renormalised later, but I<br>
> - // suppose it does stop these quantities from wandering off...<br>
> - normalise(lambda, XY);<br>
> + // We're going to normalise the lambdas so the total average is 1.<br>
> + reaverage(lambda, XY);<br>
<br>
That seems to make sense, although I wonder how it will affect the next<br>
ALSC iteration. I suppose it doesn't matter too much, but have you<br>
checked if this change has an effect on the number of iterations of the<br>
SOR (I'm mostly curious about the impact on the normal case, not the<br>
"right circumstances") ?<br></blockquote><div><br></div><div>This change has no direct impact on the number of iterations run. It limits</div><div>the amount of error "bleeding" across cells through the iterations. In general,</div><div>we want to ensure our calibration table is a good fit and our auto correction</div><div>should not deviate much from that table.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
I'll wait for your reply to the above questions before applying, please<br>
let me know if you want to send a v3 or if I should make modifications<br>
locally.<br></blockquote><div><br></div><div>Happy for you to merge with the suggested changes if you have them available locally :)</div><div><br></div><div>Regard,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> }<br>
> <br>
> static void add_luminance_rb(double result[XY], double const lambda[XY],<br>
> @@ -737,9 +764,9 @@ void Alsc::doAlsc()<br>
> compute_W(Cb, config_.sigma_Cb, Wb);<br>
> // Run Gauss-Seidel iterations over the resulting matrix, for R and B.<br>
> run_matrix_iterations(Cr, lambda_r_, Wr, config_.omega, config_.n_iter,<br>
> - config_.threshold);<br>
> + config_.threshold, config_.lambda_bound);<br>
> run_matrix_iterations(Cb, lambda_b_, Wb, config_.omega, config_.n_iter,<br>
> - config_.threshold);<br>
> + config_.threshold, config_.lambda_bound);<br>
> // Fold the calibrated gains into our final lambda values. (Note that on<br>
> // the next run, we re-start with the lambda values that don't have the<br>
> // calibration gains included.)<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp<br>
> index 9616b99ea7ca..d1dbe0d1d22d 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp<br>
> @@ -41,6 +41,7 @@ struct AlscConfig {<br>
> std::vector<AlscCalibration> calibrations_Cb;<br>
> double default_ct; // colour temperature if no metadata found<br>
> double threshold; // iteration termination threshold<br>
> + double lambda_bound; // upper/lower bound for lambda from a value of 1<br>
> };<br>
> <br>
> class Alsc : public Algorithm<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>