[{"id":11759,"web_url":"https://patchwork.libcamera.org/comment/11759/","msgid":"<20200731213801.GF24315@pendragon.ideasonboard.com>","date":"2020-07-31T21:38:01","subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOne more thing I'm afraid.\n\nOn Fri, Jul 31, 2020 at 03:08:01PM +0100, David Plowman wrote:\n> Now that we stop the asynchronous thread on a SwitchMode, we would do\n> better to regenerate all the tables if the new camera mode crops in a\n> significantly different way to the old one. A few minor tweaks make\n> sense along with this:\n> \n> * Reset the lambda values when we reset everything. It wouldn't make\n> sense to re-start with the old mode's values.\n> \n> * Use the last recorded colour temperature to generate new tables rather\n> than any default value.\n> \n> * Set the frame \"phase\" counter to ensure the adaptive procedure will\n> run asap.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------\n> 1 file changed, 37 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 4df9934..6e781bd 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -166,11 +166,8 @@ void Alsc::Initialise()\n> \tRPI_LOG(\"Alsc\");\n> \tframe_count2_ = frame_count_ = frame_phase_ = 0;\n> \tfirst_time_ = true;\n> -\t// Initialise the lambdas. Each call to Process then restarts from the\n> -\t// previous results. Also initialise the previous frame tables to the\n> -\t// same harmless values.\n> -\tfor (int i = 0; i < XY; i++)\n> -\t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n> +\tct_ = config_.default_ct;\n> +\t// The lambdas are initialised in the SwitchMode.\n> }\n> \n> void Alsc::waitForAysncThread()\n> @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()\n> \t}\n> }\n> \n> +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)\n> +{\n> +\t// Return true if the modes crop from the sensor significantly differently.\n> +\tint left_diff = std::abs(cm0.crop_x - cm1.crop_x);\n> +\tint top_diff = std::abs(cm0.crop_y - cm1.crop_y);\n> +\tint right_diff = std::abs(cm0.crop_x + cm0.scale_x * cm0.width -\n> +\t\t\t\t cm1.crop_x - cm1.scale_x * cm1.width);\n> +\tint bottom_diff = std::abs(cm0.crop_y + cm0.scale_y * cm0.height -\n> +\t\t\t\t cm1.crop_y - cm1.scale_y * cm1.height);\n\ngcc5 complains here:\n\n../../src/ipa/raspberrypi/controller/rpi/alsc.cpp: In function ‘bool compare_modes(const CameraMode&, const CameraMode&)’:\n../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:191:43: error: call of overloaded ‘abs(double)’ is ambiguous\n cm1.crop_x - cm1.scale_x * cm1.width);\n ^\nIn file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n/usr/include/stdlib.h:840:12: note: candidate: int abs(int)\n extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;\n ^\nIn file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)\n abs(long __i) { return __builtin_labs(__i); }\n ^\n/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)\n abs(long long __x) { return __builtin_llabs (__x); }\n ^\n../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:193:45: error: call of overloaded ‘abs(double)’ is ambiguous\n cm1.crop_y - cm1.scale_y * cm1.height);\n ^\nIn file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n/usr/include/stdlib.h:840:12: note: candidate: int abs(int)\n extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;\n ^\nIn file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)\n abs(long __i) { return __builtin_labs(__i); }\n ^\n/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)\n abs(long long __x) { return __builtin_llabs (__x); }\n ^\n\nWe may drop support for gcc 5 and 6 soon, but until we do, I would like\nto avoid breaking the build. Should these two lines use std::fabs() ?\n\n> +\t// These thresholds are a rather arbitrary amount chosen to trigger\n> +\t// when carrying on with the previously calculated tables might be\n> +\t// worse than regenerating them (but without the adaptive algorithm).\n> +\tint threshold_x = cm0.sensor_width >> 4;\n> +\tint threshold_y = cm0.sensor_height >> 4;\n> +\treturn left_diff > threshold_x || right_diff > threshold_x ||\n> +\t top_diff > threshold_y || bottom_diff > threshold_y;\n> +}\n> +\n> void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> \t(void)metadata;\n> \n> +\t// We're going to start over with the tables if there's any \"significant\"\n> +\t// change.\n> +\tbool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);\n> +\n> \t// Ensure the other thread isn't running while we do this.\n> \twaitForAysncThread();\n> \n> -\t// There's a bit of a question what we should do if the \"crop\" of the\n> -\t// camera mode has changed. Should we effectively reset the algorithm\n> -\t// and start over?\n> \tcamera_mode_ = camera_mode;\n> \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> \tresample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);\n> \n> -\tif (first_time_) {\n> -\t\t// On the first time, arrange for something sensible in the\n> -\t\t// initial tables. Construct the tables for some default colour\n> -\t\t// temperature. This echoes the code in doAlsc, without the\n> -\t\t// adaptive algorithm.\n> +\tif (reset_tables) {\n> +\t\t// Upon every \"table reset\", arrange for something sensible to be\n> +\t\t// generated. Construct the tables for the previous recorded colour\n> +\t\t// temperature. In order to start over from scratch we initialise\n> +\t\t// the lambdas, but the rest of this code then echoes the code in\n> +\t\t// doAlsc, without the adaptive algorithm.\n> +\t\tfor (int i = 0; i < XY; i++)\n> +\t\t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n> \t\tdouble cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];\n> -\t\tget_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);\n> +\t\tget_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> \t\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> -\t\tget_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);\n> +\t\tget_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> \t\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> \t\tcompensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> \t\t\t\t\t async_lambda_r_);\n> @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> \t\t\t\t\tconfig_.luminance_strength);\n> \t\tmemcpy(prev_sync_results_, sync_results_,\n> \t\t sizeof(prev_sync_results_));\n> +\t\tframe_phase_ = config_.frame_period; // run the algo again asap\n> \t\tfirst_time_ = false;\n> \t}\n> }\n> @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> {\n> \tRPI_LOG(\"Starting ALSC thread\");\n> \t// Get the current colour temperature. It's all we need from the\n> -\t// metadata.\n> -\tct_ = get_ct(image_metadata, config_.default_ct);\n> +\t// metadata. Default to the last CT value (which could be the default).\n> +\tct_ = get_ct(image_metadata, ct_);\n> \t// We have to copy the statistics here, dividing out our best guess of\n> \t// the LSC table that the pipeline applied to them.\n> \tAlscStatus alsc_status;","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 DDB89BD878\n\tfor ;\n\tFri, 31 Jul 2020 21:38:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5432A61F1F;\n\tFri, 31 Jul 2020 23:38:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2CD04616FF\n\tfor ;\n\tFri, 31 Jul 2020 23:38:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AC7EC53C;\n\tFri, 31 Jul 2020 23:38:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JnWeWXQM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596231490;\n\tbh=rPPO9ax8WsxHRoT2k7Djhh+WeF8yCMkGPHIuqT5oaKQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JnWeWXQMSbormjDk4zVb8kaifqUterL9PocxsE7vXWL53EcpZmGPaOMZUR9DSZcQ2\n\tNFgI9l+LUTFD5WeDO3+4XexOJDKNdtF27p6QPaw+O31aULenuOat4baw/KS1FFLiwt\n\tZy+OITLzErsTzRsMp7csND7qCLMN7x+tLFTDNyrU=","Date":"Sat, 1 Aug 2020 00:38:01 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200731213801.GF24315@pendragon.ideasonboard.com>","References":"<20200731140801.13253-1-david.plowman@raspberrypi.com>\n\t<20200731140801.13253-5-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200731140801.13253-5-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11760,"web_url":"https://patchwork.libcamera.org/comment/11760/","msgid":"<20200731214230.GG24315@pendragon.ideasonboard.com>","date":"2020-07-31T21:42:30","subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sat, Aug 01, 2020 at 12:38:01AM +0300, Laurent Pinchart wrote:\n> Hi David,\n> \n> One more thing I'm afraid.\n> \n> On Fri, Jul 31, 2020 at 03:08:01PM +0100, David Plowman wrote:\n> > Now that we stop the asynchronous thread on a SwitchMode, we would do\n> > better to regenerate all the tables if the new camera mode crops in a\n> > significantly different way to the old one. A few minor tweaks make\n> > sense along with this:\n> > \n> > * Reset the lambda values when we reset everything. It wouldn't make\n> > sense to re-start with the old mode's values.\n> > \n> > * Use the last recorded colour temperature to generate new tables rather\n> > than any default value.\n> > \n> > * Set the frame \"phase\" counter to ensure the adaptive procedure will\n> > run asap.\n> > \n> > Signed-off-by: David Plowman \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------\n> > 1 file changed, 37 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > index 4df9934..6e781bd 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > @@ -166,11 +166,8 @@ void Alsc::Initialise()\n> > \tRPI_LOG(\"Alsc\");\n> > \tframe_count2_ = frame_count_ = frame_phase_ = 0;\n> > \tfirst_time_ = true;\n> > -\t// Initialise the lambdas. Each call to Process then restarts from the\n> > -\t// previous results. Also initialise the previous frame tables to the\n> > -\t// same harmless values.\n> > -\tfor (int i = 0; i < XY; i++)\n> > -\t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n> > +\tct_ = config_.default_ct;\n> > +\t// The lambdas are initialised in the SwitchMode.\n> > }\n> > \n> > void Alsc::waitForAysncThread()\n> > @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()\n> > \t}\n> > }\n> > \n> > +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)\n> > +{\n> > +\t// Return true if the modes crop from the sensor significantly differently.\n> > +\tint left_diff = std::abs(cm0.crop_x - cm1.crop_x);\n> > +\tint top_diff = std::abs(cm0.crop_y - cm1.crop_y);\n> > +\tint right_diff = std::abs(cm0.crop_x + cm0.scale_x * cm0.width -\n> > +\t\t\t\t cm1.crop_x - cm1.scale_x * cm1.width);\n> > +\tint bottom_diff = std::abs(cm0.crop_y + cm0.scale_y * cm0.height -\n> > +\t\t\t\t cm1.crop_y - cm1.scale_y * cm1.height);\n> \n> gcc5 complains here:\n> \n> ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp: In function ‘bool compare_modes(const CameraMode&, const CameraMode&)’:\n> ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:191:43: error: call of overloaded ‘abs(double)’ is ambiguous\n> cm1.crop_x - cm1.scale_x * cm1.width);\n> ^\n> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n> /usr/include/stdlib.h:840:12: note: candidate: int abs(int)\n> extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;\n> ^\n> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)\n> abs(long __i) { return __builtin_labs(__i); }\n> ^\n> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)\n> abs(long long __x) { return __builtin_llabs (__x); }\n> ^\n> ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:193:45: error: call of overloaded ‘abs(double)’ is ambiguous\n> cm1.crop_y - cm1.scale_y * cm1.height);\n> ^\n> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:72:0,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n> /usr/include/stdlib.h:840:12: note: candidate: int abs(int)\n> extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;\n> ^\n> In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/ext/string_conversions.h:41:0,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/bits/basic_string.h:5249,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/string:52,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/stdexcept:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/array:38,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/tuple:39,\n> from /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/mutex:38,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.hpp:9,\n> from ../../src/ipa/raspberrypi/controller/rpi/alsc.cpp:10:\n> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:166:3: note: candidate: long int std::abs(long int)\n> abs(long __i) { return __builtin_labs(__i); }\n> ^\n> /usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/include/g++-v5/cstdlib:174:3: note: candidate: long long int std::abs(long long int)\n> abs(long long __x) { return __builtin_llabs (__x); }\n> ^\n> \n> We may drop support for gcc 5 and 6 soon, but until we do, I would like\n> to avoid breaking the build. Should these two lines use std::fabs() ?\n\nShould be fabs() as this file includes math.h.\n\n> > +\t// These thresholds are a rather arbitrary amount chosen to trigger\n> > +\t// when carrying on with the previously calculated tables might be\n> > +\t// worse than regenerating them (but without the adaptive algorithm).\n> > +\tint threshold_x = cm0.sensor_width >> 4;\n> > +\tint threshold_y = cm0.sensor_height >> 4;\n> > +\treturn left_diff > threshold_x || right_diff > threshold_x ||\n> > +\t top_diff > threshold_y || bottom_diff > threshold_y;\n> > +}\n> > +\n> > void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > \t(void)metadata;\n> > \n> > +\t// We're going to start over with the tables if there's any \"significant\"\n> > +\t// change.\n> > +\tbool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);\n> > +\n> > \t// Ensure the other thread isn't running while we do this.\n> > \twaitForAysncThread();\n> > \n> > -\t// There's a bit of a question what we should do if the \"crop\" of the\n> > -\t// camera mode has changed. Should we effectively reset the algorithm\n> > -\t// and start over?\n> > \tcamera_mode_ = camera_mode;\n> > \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> > \tresample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);\n> > \n> > -\tif (first_time_) {\n> > -\t\t// On the first time, arrange for something sensible in the\n> > -\t\t// initial tables. Construct the tables for some default colour\n> > -\t\t// temperature. This echoes the code in doAlsc, without the\n> > -\t\t// adaptive algorithm.\n> > +\tif (reset_tables) {\n> > +\t\t// Upon every \"table reset\", arrange for something sensible to be\n> > +\t\t// generated. Construct the tables for the previous recorded colour\n> > +\t\t// temperature. In order to start over from scratch we initialise\n> > +\t\t// the lambdas, but the rest of this code then echoes the code in\n> > +\t\t// doAlsc, without the adaptive algorithm.\n> > +\t\tfor (int i = 0; i < XY; i++)\n> > +\t\t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n> > \t\tdouble cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];\n> > -\t\tget_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);\n> > +\t\tget_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> > \t\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> > -\t\tget_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);\n> > +\t\tget_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> > \t\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> > \t\tcompensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> > \t\t\t\t\t async_lambda_r_);\n> > @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > \t\t\t\t\tconfig_.luminance_strength);\n> > \t\tmemcpy(prev_sync_results_, sync_results_,\n> > \t\t sizeof(prev_sync_results_));\n> > +\t\tframe_phase_ = config_.frame_period; // run the algo again asap\n> > \t\tfirst_time_ = false;\n> > \t}\n> > }\n> > @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> > {\n> > \tRPI_LOG(\"Starting ALSC thread\");\n> > \t// Get the current colour temperature. It's all we need from the\n> > -\t// metadata.\n> > -\tct_ = get_ct(image_metadata, config_.default_ct);\n> > +\t// metadata. Default to the last CT value (which could be the default).\n> > +\tct_ = get_ct(image_metadata, ct_);\n> > \t// We have to copy the statistics here, dividing out our best guess of\n> > \t// the LSC table that the pipeline applied to them.\n> > \tAlscStatus alsc_status;","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 CE88FBD86F\n\tfor ;\n\tFri, 31 Jul 2020 21:42:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5844161F1F;\n\tFri, 31 Jul 2020 23:42:42 +0200 (CEST)","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 D63C5616FF\n\tfor ;\n\tFri, 31 Jul 2020 23:42:40 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4729F53C;\n\tFri, 31 Jul 2020 23:42:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gIrf4WFc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596231760;\n\tbh=2SR3C26Q3pmYOS/R1DHePnZzpX/NejA/Uh/+8Eaw8tE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gIrf4WFch7VtXraXAWuN0Kr2MUezprbj3R5J0TPkBrKoFAtmsU2sDjP+w+4SNg5Fw\n\tedFuw23VOcqnmOkD/gzGDgs5nWdY8UGGY6IxX+eMKoA967296jzIbKKtWfW+mwI0ew\n\tP4cJHKi+3CcoEYKuLbXu4HPtDQ1ar9RoQP/dQb+A=","Date":"Sat, 1 Aug 2020 00:42:30 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200731214230.GG24315@pendragon.ideasonboard.com>","References":"<20200731140801.13253-1-david.plowman@raspberrypi.com>\n\t<20200731140801.13253-5-david.plowman@raspberrypi.com>\n\t<20200731213801.GF24315@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200731213801.GF24315@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]