[{"id":11737,"web_url":"https://patchwork.libcamera.org/comment/11737/","msgid":"<20200730225728.GH6107@pendragon.ideasonboard.com>","date":"2020-07-30T22:57:28","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Thu, Jul 30, 2020 at 12:11:33PM +0100, David Plowman wrote:\n> This fixes a bug where the luminance correction table was not being\n> resampled according to the camera mode, in the same way as the colour\n> tables. This could be noticeable if any camera modes crop\n> aggressively.\n> \n> This resampling can be done \"up front\" in the SwitchMode, as we have\n> only a single fixed luminance table. In order to protect the\n> recalculation of the table from the asynchronous thread (which reads\n> it) I've elected to wait for that thread to go idle (though I doubt it\n> would have mattered much). As a by-product of stopping the thread, it\n> no longer needs its own copy of the camera mode (async_camera_mode_).\n\nThis looks good to me. There are however a couple of potential issues\nI've noticed when reading the ALSC code and that you may want to check:\n\n- In Alsc::restartAsync(), async_start_ and async_started_ are set\n without holding the mutex_. They could thus potentially be reordered,\n possibly causing a race condition.\n\n- In Alsc::asyncFunc(), sync_signal_.notify_one() is called with the\n mutex_ held. The function will cause the thread waiting on the\n sync_signal_ condition variable to wake up, to then go back to sleep\n immediatelly when it tries to lock the mutex_. The notify_one() call\n should be moved after the locked scope.\n\n- sync_signal_ is actually never used :-) Or rather was never used, as\n this patch now waits on it in Alsc::waitForAysncThread(). The\n sync_signal_ in the AWB algorithm is however completely unused.\n\n> Signed-off-by: David Plowman \n\nFor this patch,\n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 39 ++++++++++++++-------\n> src/ipa/raspberrypi/controller/rpi/alsc.hpp | 3 +-\n> 2 files changed, 28 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 76e2f04..8f7720b 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -173,19 +173,34 @@ void Alsc::Initialise()\n> \t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n> }\n> \n> +void Alsc::waitForAysncThread()\n> +{\n> +\tstd::unique_lock lock(mutex_);\n> +\tif (async_started_) {\n> +\t\tsync_signal_.wait(lock, [&] {\n> +\t\t\treturn async_finished_;\n> +\t\t});\n> +\t\tasync_started_ = false;\n> +\t\tasync_finished_ = false;\n> +\t}\n> +}\n> +\n> void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> \t(void)metadata;\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. Any calculation currently in flight would\n> -\t// not be useful to the new mode, so arguably we should abort it, and\n> -\t// generate a new table (like the \"first_time\" code already here). When\n> -\t// the crop doesn't change, we can presumably just leave things\n> -\t// alone. For now, I think we'll just wait and see. When the crop does\n> -\t// change, any effects should be transient, and if they're not transient\n> -\t// enough, we'll revisit the question then.\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> @@ -201,7 +216,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> \t\tcompensate_lambdas_for_cal(cal_table_b, lambda_b_,\n> \t\t\t\t\t async_lambda_b_);\n> \t\tadd_luminance_to_tables(sync_results_, async_lambda_r_, 1.0,\n> -\t\t\t\t\tasync_lambda_b_, config_.luminance_lut,\n> +\t\t\t\t\tasync_lambda_b_, luminance_table_,\n> \t\t\t\t\tconfig_.luminance_strength);\n> \t\tmemcpy(prev_sync_results_, sync_results_,\n> \t\t sizeof(prev_sync_results_));\n> @@ -266,8 +281,6 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> \t}\n> \tcopy_stats(statistics_, stats, alsc_status);\n> \tframe_phase_ = 0;\n> -\t// copy the camera mode so it won't change during the calculations\n> -\tasync_camera_mode_ = camera_mode_;\n> \tasync_start_ = true;\n> \tasync_started_ = true;\n> \tasync_signal_.notify_one();\n> @@ -670,9 +683,9 @@ void Alsc::doAlsc()\n> \t// Fetch the new calibrations (if any) for this CT. Resample them in\n> \t// case the camera mode is not full-frame.\n> \tget_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> -\tresample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_r);\n> +\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> \tget_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> -\tresample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_b);\n> +\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> \t// You could print out the cal tables for this image here, if you're\n> \t// tuning the algorithm...\n> \t(void)print_cal_table;\n> @@ -695,7 +708,7 @@ void Alsc::doAlsc()\n> \tcompensate_lambdas_for_cal(cal_table_b, lambda_b_, async_lambda_b_);\n> \t// Fold in the luminance table at the appropriate strength.\n> \tadd_luminance_to_tables(async_results_, async_lambda_r_, 1.0,\n> -\t\t\t\tasync_lambda_b_, config_.luminance_lut,\n> +\t\t\t\tasync_lambda_b_, luminance_table_,\n> \t\t\t\tconfig_.luminance_strength);\n> }\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> index e895913..95572af 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> @@ -60,10 +60,10 @@ private:\n> \tAlscConfig config_;\n> \tbool first_time_;\n> \tCameraMode camera_mode_;\n> +\tdouble luminance_table_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> \tstd::thread async_thread_;\n> \tvoid asyncFunc(); // asynchronous thread function\n> \tstd::mutex mutex_;\n> -\tCameraMode async_camera_mode_;\n> \t// condvar for async thread to wait on\n> \tstd::condition_variable async_signal_;\n> \t// condvar for synchronous thread to wait on\n> @@ -86,6 +86,7 @@ private:\n> \tint frame_count2_;\n> \tdouble sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> \tdouble prev_sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> +\tvoid waitForAysncThread();\n> \t// The following are for the asynchronous thread to use, though the main\n> \t// thread can set/reset them if the async thread is known to be idle:\n> \tvoid restartAsync(StatisticsPtr &stats, Metadata *image_metadata);","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 62277BD878\n\tfor ;\n\tThu, 30 Jul 2020 22:57:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFDF861988;\n\tFri, 31 Jul 2020 00:57:39 +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 8AB5760540\n\tfor ;\n\tFri, 31 Jul 2020 00:57:38 +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 D68039B1;\n\tFri, 31 Jul 2020 00:57:37 +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=\"s9nrS7G2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596149858;\n\tbh=Acds0652Bepwmy/AQTlnJRoLbvsASDDOgBJ4i2z7J9k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s9nrS7G2SscB0MHmzsQfFMEipaw171syR496zQGfvgpGBZsj2B4v7vjsCNbSjq543\n\tzCQ/29Axii45VlraLoyMImuWSbKxBtj1Ry4OLSXueW3gDD4rPoWyvcClC2JSMidl29\n\t/E0GrvJnft4QlSo8kkq8nckomIddg/6fw7p5OXd8=","Date":"Fri, 31 Jul 2020 01:57:28 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200730225728.GH6107@pendragon.ideasonboard.com>","References":"<20200730111134.641-1-david.plowman@raspberrypi.com>\n\t<20200730111134.641-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200730111134.641-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11742,"web_url":"https://patchwork.libcamera.org/comment/11742/","msgid":"","date":"2020-07-31T08:03:35","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the review!\n\nOn Thu, 30 Jul 2020 at 23:57, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Thu, Jul 30, 2020 at 12:11:33PM +0100, David Plowman wrote:\n> > This fixes a bug where the luminance correction table was not being\n> > resampled according to the camera mode, in the same way as the colour\n> > tables. This could be noticeable if any camera modes crop\n> > aggressively.\n> >\n> > This resampling can be done \"up front\" in the SwitchMode, as we have\n> > only a single fixed luminance table. In order to protect the\n> > recalculation of the table from the asynchronous thread (which reads\n> > it) I've elected to wait for that thread to go idle (though I doubt it\n> > would have mattered much). As a by-product of stopping the thread, it\n> > no longer needs its own copy of the camera mode (async_camera_mode_).\n>\n> This looks good to me. There are however a couple of potential issues\n> I've noticed when reading the ALSC code and that you may want to check:\n>\n> - In Alsc::restartAsync(), async_start_ and async_started_ are set\n> without holding the mutex_. They could thus potentially be reordered,\n> possibly causing a race condition.\n\nasync_started_ is used only by the main thread, so all we have to\nworry about is async_start_. Presumably this is safe assuming updating\nbools is atomic. But I have some recollection that nothing is\nguaranteed atomic if you don't mark it explicitly, so it probably\nwants wrapping in std::atomic, is that right?\n\nSame comments probably true about async_abort_. If that's atomic (and\nmarked explicitly), no need for the lock in the destructor.\n\n>\n> - In Alsc::asyncFunc(), sync_signal_.notify_one() is called with the\n> mutex_ held. The function will cause the thread waiting on the\n> sync_signal_ condition variable to wake up, to then go back to sleep\n> immediatelly when it tries to lock the mutex_. The notify_one() call\n> should be moved after the locked scope.\n\nHere too, presumably the lock is in fact redundant if we believe the\nupdate to async_finished_ is atomic (though again, the correct thing\nwould be to wrap it in std::atomic)\n\n>\n> - sync_signal_ is actually never used :-) Or rather was never used, as\n> this patch now waits on it in Alsc::waitForAysncThread(). The\n> sync_signal_ in the AWB algorithm is however completely unused.\n\nYes indeed. All this code was ported from somewhere where you could\nforce these algorithms to run synchronously (i.e. wait for the\ncalculations to finish and use them immediately), so these other\nsignals were indeed being used. I removed that for the libcamera/VC4\nplatform and never did anything about this signal. Though I'm quite\nhappy to have left it in now! I'm not sure about the AWB one, I can't\nthink of any case where waiting for that thread to stop would be\nuseful like it is here, so maybe that will be one to remove.\n\nThanks for all the comments!\n\nBest regards\nDavid\n\n>\n> > Signed-off-by: David Plowman \n>\n> For this patch,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > ---\n> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 39 ++++++++++++++-------\n> > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 3 +-\n> > 2 files changed, 28 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > index 76e2f04..8f7720b 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > @@ -173,19 +173,34 @@ void Alsc::Initialise()\n> > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > }\n> >\n> > +void Alsc::waitForAysncThread()\n> > +{\n> > + std::unique_lock lock(mutex_);\n> > + if (async_started_) {\n> > + sync_signal_.wait(lock, [&] {\n> > + return async_finished_;\n> > + });\n> > + async_started_ = false;\n> > + async_finished_ = false;\n> > + }\n> > +}\n> > +\n> > void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > (void)metadata;\n> >\n> > + // Ensure the other thread isn't running while we do this.\n> > + waitForAysncThread();\n> > +\n> > // There's a bit of a question what we should do if the \"crop\" of the\n> > - // camera mode has changed. Any calculation currently in flight would\n> > - // not be useful to the new mode, so arguably we should abort it, and\n> > - // generate a new table (like the \"first_time\" code already here). When\n> > - // the crop doesn't change, we can presumably just leave things\n> > - // alone. For now, I think we'll just wait and see. When the crop does\n> > - // change, any effects should be transient, and if they're not transient\n> > - // enough, we'll revisit the question then.\n> > + // camera mode has changed. Should we effectively reset the algorithm\n> > + // and start over?\n> > camera_mode_ = camera_mode;\n> > +\n> > + // We must resample the luminance table like we do the others, but it's\n> > + // fixed so we can simply do it up front here.\n> > + resample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);\n> > +\n> > if (first_time_) {\n> > // On the first time, arrange for something sensible in the\n> > // initial tables. Construct the tables for some default colour\n> > @@ -201,7 +216,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > compensate_lambdas_for_cal(cal_table_b, lambda_b_,\n> > async_lambda_b_);\n> > add_luminance_to_tables(sync_results_, async_lambda_r_, 1.0,\n> > - async_lambda_b_, config_.luminance_lut,\n> > + async_lambda_b_, luminance_table_,\n> > config_.luminance_strength);\n> > memcpy(prev_sync_results_, sync_results_,\n> > sizeof(prev_sync_results_));\n> > @@ -266,8 +281,6 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> > }\n> > copy_stats(statistics_, stats, alsc_status);\n> > frame_phase_ = 0;\n> > - // copy the camera mode so it won't change during the calculations\n> > - async_camera_mode_ = camera_mode_;\n> > async_start_ = true;\n> > async_started_ = true;\n> > async_signal_.notify_one();\n> > @@ -670,9 +683,9 @@ void Alsc::doAlsc()\n> > // Fetch the new calibrations (if any) for this CT. Resample them in\n> > // case the camera mode is not full-frame.\n> > get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_r);\n> > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> > get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_b);\n> > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> > // You could print out the cal tables for this image here, if you're\n> > // tuning the algorithm...\n> > (void)print_cal_table;\n> > @@ -695,7 +708,7 @@ void Alsc::doAlsc()\n> > compensate_lambdas_for_cal(cal_table_b, lambda_b_, async_lambda_b_);\n> > // Fold in the luminance table at the appropriate strength.\n> > add_luminance_to_tables(async_results_, async_lambda_r_, 1.0,\n> > - async_lambda_b_, config_.luminance_lut,\n> > + async_lambda_b_, luminance_table_,\n> > config_.luminance_strength);\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > index e895913..95572af 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > @@ -60,10 +60,10 @@ private:\n> > AlscConfig config_;\n> > bool first_time_;\n> > CameraMode camera_mode_;\n> > + double luminance_table_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> > std::thread async_thread_;\n> > void asyncFunc(); // asynchronous thread function\n> > std::mutex mutex_;\n> > - CameraMode async_camera_mode_;\n> > // condvar for async thread to wait on\n> > std::condition_variable async_signal_;\n> > // condvar for synchronous thread to wait on\n> > @@ -86,6 +86,7 @@ private:\n> > int frame_count2_;\n> > double sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> > double prev_sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> > + void waitForAysncThread();\n> > // The following are for the asynchronous thread to use, though the main\n> > // thread can set/reset them if the async thread is known to be idle:\n> > void restartAsync(StatisticsPtr &stats, Metadata *image_metadata);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 339E5BD86F\n\tfor ;\n\tFri, 31 Jul 2020 08:03:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FAF761DC3;\n\tFri, 31 Jul 2020 10:03:50 +0200 (CEST)","from mail-oo1-xc42.google.com (mail-oo1-xc42.google.com\n\t[IPv6:2607:f8b0:4864:20::c42])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1CED60395\n\tfor ;\n\tFri, 31 Jul 2020 10:03:48 +0200 (CEST)","by mail-oo1-xc42.google.com with SMTP id x1so3555860oox.6\n\tfor ;\n\tFri, 31 Jul 2020 01:03:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"B1x1/Ke4\"; dkim-atps=neutral","DKIM-Signature":"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=iRaHJc7UQzIY0fQRbliCDGcKRD7Z8agYkutzl616gmk=;\n\tb=B1x1/Ke4YHDCsBbSzi45CD9v8st6QgOnoeNGYHdAlIffisPFZ8b7SdozwJrXttmw0l\n\tLsbEoU8fiDoZBNXHt7wOfQgMw1/+RHM1TkdoPfGkfn1YE9qHDTnc2O2DvZNkG3tfYAG3\n\tnfjiI7+nQOoN9zGEqy40J9GDEEFk2U0+s0x+vLg/wxoxsnv91xl5imWQdljYMl0Kis9o\n\t/ARtH0gps+8qq1Fu2EEU2MxtUkJa28M+qX4r5yZr52nvuIl1Ji/ViI+knuESuneMbFDA\n\tv6hL3cMKZRRklDxZcBvvlLE76tb3IAMl9TQxbV/K/PBEKKrYkFBGsTlC7JNNTt+WYK0g\n\t2M9g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=iRaHJc7UQzIY0fQRbliCDGcKRD7Z8agYkutzl616gmk=;\n\tb=spiTC8hD2ZosXFRVLew29V1wr2t7g4Ney1SkO0LJ5bWpluVcJTVZi2yzHPYGRB21hE\n\tX89R8IFsONGlPucatYj8/2pRKkVY+Sh+v1oP8sT0jEJ0wBAU850CbVaPPWyiBVA4aNzb\n\txGxYhvIcPfqYnAxIklvKqU/aTlfiuhbHKkBBJbOKvPiiHhYJx4QeernjXSVC1TES0Lp3\n\tdhdfKauYE53ZZRnyseSLeGf7lvaUtj7h8fB9CrSoh7ywR6uvcdeM5GtGgkSyqEvnjNWo\n\toJkM0GpyaVr4SmQHPE1x1YDOEwGWBSnxZzA9eoqwDWdCVYpg1ZJJS0lwf0EBX18XR8aJ\n\tKzlA==","X-Gm-Message-State":"AOAM531r9+zbaTEb9TMkgL5U+2BL55QIAxV7lCFJRWbkJoaT/k7Ro2Cs\n\trS3aSx0JAXdKSHeJn8gi2YK0Dx8j5z6swsdwMkECif5L4u8=","X-Google-Smtp-Source":"ABdhPJzf+LdO1HNYgQTd9CiWNVu9oOxsARwN6I7CIlmA3MTZQnMbq+Oea3jDOoYVN9+3mHOzVN7kIwDIgdTKrxbnVCM=","X-Received":"by 2002:a4a:ea29:: with SMTP id y9mr2046841ood.72.1596182627350; \n\tFri, 31 Jul 2020 01:03:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20200730111134.641-1-david.plowman@raspberrypi.com>\n\t<20200730111134.641-3-david.plowman@raspberrypi.com>\n\t<20200730225728.GH6107@pendragon.ideasonboard.com>","In-Reply-To":"<20200730225728.GH6107@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Fri, 31 Jul 2020 09:03:35 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11747,"web_url":"https://patchwork.libcamera.org/comment/11747/","msgid":"<20200731120006.GC6218@pendragon.ideasonboard.com>","date":"2020-07-31T12:00:06","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Fri, Jul 31, 2020 at 09:03:35AM +0100, David Plowman wrote:\n> On Thu, 30 Jul 2020 at 23:57, Laurent Pinchart wrote:\n> > On Thu, Jul 30, 2020 at 12:11:33PM +0100, David Plowman wrote:\n> > > This fixes a bug where the luminance correction table was not being\n> > > resampled according to the camera mode, in the same way as the colour\n> > > tables. This could be noticeable if any camera modes crop\n> > > aggressively.\n> > >\n> > > This resampling can be done \"up front\" in the SwitchMode, as we have\n> > > only a single fixed luminance table. In order to protect the\n> > > recalculation of the table from the asynchronous thread (which reads\n> > > it) I've elected to wait for that thread to go idle (though I doubt it\n> > > would have mattered much). As a by-product of stopping the thread, it\n> > > no longer needs its own copy of the camera mode (async_camera_mode_).\n> >\n> > This looks good to me. There are however a couple of potential issues\n> > I've noticed when reading the ALSC code and that you may want to check:\n> >\n> > - In Alsc::restartAsync(), async_start_ and async_started_ are set\n> > without holding the mutex_. They could thus potentially be reordered,\n> > possibly causing a race condition.\n> \n> async_started_ is used only by the main thread, so all we have to\n> worry about is async_start_.\n\nOops, indeed, my bad.\n\n> Presumably this is safe assuming updating bools is atomic. But I have\n> some recollection that nothing is guaranteed atomic if you don't mark\n> it explicitly, so it probably wants wrapping in std::atomic, is that\n> right?\n\nIf it's used in a single thread there's no issue, atomicity isn't\nrequired.\n\n> Same comments probably true about async_abort_. If that's atomic (and\n> marked explicitly), no need for the lock in the destructor.\n\nasync_abort_ is used in the ALSC thread, so for that we need a lock.\nIt's not just about atomicity, it's also about ensuring that memory\naccesses are not reordered. If you write\n\n\tasync_abort_ = true;\n\tasync_signal_.notify_one();\n\nwithout taking a lock, there is no guarantee that the async_abort_ write\nwill be visible to the reader before it gets woken up by\nasync_signal_.notify_one().\n\nThe documentation of std::condition_variable::notify_one() states\n\n\"The effects of notify_one()/notify_all() and each of the three atomic\nparts of wait()/wait_for()/wait_until() (unlock+wait, wakeup, and lock)\ntake place in a single total order that can be viewed as modification\norder of an atomic variable: the order is specific to this individual\ncondition_variable. This makes it impossible for notify_one() to, for\nexample, be delayed and unblock a thread that started waiting just after\nthe call to notify_one() was made.\"\n\nThis doesn't offer any memory order guarantee (as in\nhttps://en.cppreference.com/w/cpp/atomic/memory_order) for the\nmodification of the shared variable.\n\nThe std::condition_variable documentation state\n\n\"Even if the shared variable is atomic, it must be modified under the\nmutex in order to correctly publish the modification to the waiting\nthread.\"\n\nThe lock is thus needed, but the notify_one() call can be moved after\nunlocking the mutex.\n\n> > - In Alsc::asyncFunc(), sync_signal_.notify_one() is called with the\n> > mutex_ held. The function will cause the thread waiting on the\n> > sync_signal_ condition variable to wake up, to then go back to sleep\n> > immediatelly when it tries to lock the mutex_. The notify_one() call\n> > should be moved after the locked scope.\n> \n> Here too, presumably the lock is in fact redundant if we believe the\n> update to async_finished_ is atomic (though again, the correct thing\n> would be to wrap it in std::atomic)\n\nSee above :-) Memory ordering is a tricky topic.\n\n> > - sync_signal_ is actually never used :-) Or rather was never used, as\n> > this patch now waits on it in Alsc::waitForAysncThread(). The\n> > sync_signal_ in the AWB algorithm is however completely unused.\n> \n> Yes indeed. All this code was ported from somewhere where you could\n> force these algorithms to run synchronously (i.e. wait for the\n> calculations to finish and use them immediately), so these other\n> signals were indeed being used. I removed that for the libcamera/VC4\n> platform and never did anything about this signal. Though I'm quite\n> happy to have left it in now! I'm not sure about the AWB one, I can't\n> think of any case where waiting for that thread to stop would be\n> useful like it is here, so maybe that will be one to remove.\n> \n> Thanks for all the comments!\n> \n> > > Signed-off-by: David Plowman \n> >\n> > For this patch,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > ---\n> > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 39 ++++++++++++++-------\n> > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 3 +-\n> > > 2 files changed, 28 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > index 76e2f04..8f7720b 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > @@ -173,19 +173,34 @@ void Alsc::Initialise()\n> > > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > > }\n> > >\n> > > +void Alsc::waitForAysncThread()\n> > > +{\n> > > + std::unique_lock lock(mutex_);\n> > > + if (async_started_) {\n> > > + sync_signal_.wait(lock, [&] {\n> > > + return async_finished_;\n> > > + });\n> > > + async_started_ = false;\n> > > + async_finished_ = false;\n> > > + }\n> > > +}\n> > > +\n> > > void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > {\n> > > (void)metadata;\n> > >\n> > > + // Ensure the other thread isn't running while we do this.\n> > > + waitForAysncThread();\n> > > +\n> > > // There's a bit of a question what we should do if the \"crop\" of the\n> > > - // camera mode has changed. Any calculation currently in flight would\n> > > - // not be useful to the new mode, so arguably we should abort it, and\n> > > - // generate a new table (like the \"first_time\" code already here). When\n> > > - // the crop doesn't change, we can presumably just leave things\n> > > - // alone. For now, I think we'll just wait and see. When the crop does\n> > > - // change, any effects should be transient, and if they're not transient\n> > > - // enough, we'll revisit the question then.\n> > > + // camera mode has changed. Should we effectively reset the algorithm\n> > > + // and start over?\n> > > camera_mode_ = camera_mode;\n> > > +\n> > > + // We must resample the luminance table like we do the others, but it's\n> > > + // fixed so we can simply do it up front here.\n> > > + resample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);\n> > > +\n> > > if (first_time_) {\n> > > // On the first time, arrange for something sensible in the\n> > > // initial tables. Construct the tables for some default colour\n> > > @@ -201,7 +216,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > compensate_lambdas_for_cal(cal_table_b, lambda_b_,\n> > > async_lambda_b_);\n> > > add_luminance_to_tables(sync_results_, async_lambda_r_, 1.0,\n> > > - async_lambda_b_, config_.luminance_lut,\n> > > + async_lambda_b_, luminance_table_,\n> > > config_.luminance_strength);\n> > > memcpy(prev_sync_results_, sync_results_,\n> > > sizeof(prev_sync_results_));\n> > > @@ -266,8 +281,6 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> > > }\n> > > copy_stats(statistics_, stats, alsc_status);\n> > > frame_phase_ = 0;\n> > > - // copy the camera mode so it won't change during the calculations\n> > > - async_camera_mode_ = camera_mode_;\n> > > async_start_ = true;\n> > > async_started_ = true;\n> > > async_signal_.notify_one();\n> > > @@ -670,9 +683,9 @@ void Alsc::doAlsc()\n> > > // Fetch the new calibrations (if any) for this CT. Resample them in\n> > > // case the camera mode is not full-frame.\n> > > get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> > > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_r);\n> > > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> > > get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> > > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_b);\n> > > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> > > // You could print out the cal tables for this image here, if you're\n> > > // tuning the algorithm...\n> > > (void)print_cal_table;\n> > > @@ -695,7 +708,7 @@ void Alsc::doAlsc()\n> > > compensate_lambdas_for_cal(cal_table_b, lambda_b_, async_lambda_b_);\n> > > // Fold in the luminance table at the appropriate strength.\n> > > add_luminance_to_tables(async_results_, async_lambda_r_, 1.0,\n> > > - async_lambda_b_, config_.luminance_lut,\n> > > + async_lambda_b_, luminance_table_,\n> > > config_.luminance_strength);\n> > > }\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > index e895913..95572af 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > @@ -60,10 +60,10 @@ private:\n> > > AlscConfig config_;\n> > > bool first_time_;\n> > > CameraMode camera_mode_;\n> > > + double luminance_table_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> > > std::thread async_thread_;\n> > > void asyncFunc(); // asynchronous thread function\n> > > std::mutex mutex_;\n> > > - CameraMode async_camera_mode_;\n> > > // condvar for async thread to wait on\n> > > std::condition_variable async_signal_;\n> > > // condvar for synchronous thread to wait on\n> > > @@ -86,6 +86,7 @@ private:\n> > > int frame_count2_;\n> > > double sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> > > double prev_sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> > > + void waitForAysncThread();\n> > > // The following are for the asynchronous thread to use, though the main\n> > > // thread can set/reset them if the async thread is known to be idle:\n> > > void restartAsync(StatisticsPtr &stats, Metadata *image_metadata);","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 AFC78BD86F\n\tfor ;\n\tFri, 31 Jul 2020 12:00:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38BAE61A14;\n\tFri, 31 Jul 2020 14:00: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 27415611A2\n\tfor ;\n\tFri, 31 Jul 2020 14:00:52 +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 D145C53C;\n\tFri, 31 Jul 2020 14:00:47 +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=\"Jg9Q9eLa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596196851;\n\tbh=zaEcIrQ4rBnHm+bS8IGnqnNTpeUt/auuMS089JC20gc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Jg9Q9eLa6wzASOS754S07K6LIrbdJ5oYe1jKgsgS6c2lL9tsy2JEbhpTVbCYD3OvM\n\tNFa/bf3php3P14KxWfbtZIpD5QEZqNhXh7EgtWf7mfbK7y7ELlyeqGkYxOQUtQfAgg\n\tJ1EbtTDOf+LKn8y57N8UH/OCQy8XlwA4crXJiz3I=","Date":"Fri, 31 Jul 2020 15:00:06 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200731120006.GC6218@pendragon.ideasonboard.com>","References":"<20200730111134.641-1-david.plowman@raspberrypi.com>\n\t<20200730111134.641-3-david.plowman@raspberrypi.com>\n\t<20200730225728.GH6107@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11749,"web_url":"https://patchwork.libcamera.org/comment/11749/","msgid":"","date":"2020-07-31T12:27:42","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again\n\nThanks for that. Let me circulate a second version of these patches,\nand we can also wait for Naush to review next week.\n\nDavid\n\nOn Fri, 31 Jul 2020 at 13:01, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Fri, Jul 31, 2020 at 09:03:35AM +0100, David Plowman wrote:\n> > On Thu, 30 Jul 2020 at 23:57, Laurent Pinchart wrote:\n> > > On Thu, Jul 30, 2020 at 12:11:33PM +0100, David Plowman wrote:\n> > > > This fixes a bug where the luminance correction table was not being\n> > > > resampled according to the camera mode, in the same way as the colour\n> > > > tables. This could be noticeable if any camera modes crop\n> > > > aggressively.\n> > > >\n> > > > This resampling can be done \"up front\" in the SwitchMode, as we have\n> > > > only a single fixed luminance table. In order to protect the\n> > > > recalculation of the table from the asynchronous thread (which reads\n> > > > it) I've elected to wait for that thread to go idle (though I doubt it\n> > > > would have mattered much). As a by-product of stopping the thread, it\n> > > > no longer needs its own copy of the camera mode (async_camera_mode_).\n> > >\n> > > This looks good to me. There are however a couple of potential issues\n> > > I've noticed when reading the ALSC code and that you may want to check:\n> > >\n> > > - In Alsc::restartAsync(), async_start_ and async_started_ are set\n> > > without holding the mutex_. They could thus potentially be reordered,\n> > > possibly causing a race condition.\n> >\n> > async_started_ is used only by the main thread, so all we have to\n> > worry about is async_start_.\n>\n> Oops, indeed, my bad.\n>\n> > Presumably this is safe assuming updating bools is atomic. But I have\n> > some recollection that nothing is guaranteed atomic if you don't mark\n> > it explicitly, so it probably wants wrapping in std::atomic, is that\n> > right?\n>\n> If it's used in a single thread there's no issue, atomicity isn't\n> required.\n>\n> > Same comments probably true about async_abort_. If that's atomic (and\n> > marked explicitly), no need for the lock in the destructor.\n>\n> async_abort_ is used in the ALSC thread, so for that we need a lock.\n> It's not just about atomicity, it's also about ensuring that memory\n> accesses are not reordered. If you write\n>\n> async_abort_ = true;\n> async_signal_.notify_one();\n>\n> without taking a lock, there is no guarantee that the async_abort_ write\n> will be visible to the reader before it gets woken up by\n> async_signal_.notify_one().\n>\n> The documentation of std::condition_variable::notify_one() states\n>\n> \"The effects of notify_one()/notify_all() and each of the three atomic\n> parts of wait()/wait_for()/wait_until() (unlock+wait, wakeup, and lock)\n> take place in a single total order that can be viewed as modification\n> order of an atomic variable: the order is specific to this individual\n> condition_variable. This makes it impossible for notify_one() to, for\n> example, be delayed and unblock a thread that started waiting just after\n> the call to notify_one() was made.\"\n>\n> This doesn't offer any memory order guarantee (as in\n> https://en.cppreference.com/w/cpp/atomic/memory_order) for the\n> modification of the shared variable.\n>\n> The std::condition_variable documentation state\n>\n> \"Even if the shared variable is atomic, it must be modified under the\n> mutex in order to correctly publish the modification to the waiting\n> thread.\"\n>\n> The lock is thus needed, but the notify_one() call can be moved after\n> unlocking the mutex.\n>\n> > > - In Alsc::asyncFunc(), sync_signal_.notify_one() is called with the\n> > > mutex_ held. The function will cause the thread waiting on the\n> > > sync_signal_ condition variable to wake up, to then go back to sleep\n> > > immediatelly when it tries to lock the mutex_. The notify_one() call\n> > > should be moved after the locked scope.\n> >\n> > Here too, presumably the lock is in fact redundant if we believe the\n> > update to async_finished_ is atomic (though again, the correct thing\n> > would be to wrap it in std::atomic)\n>\n> See above :-) Memory ordering is a tricky topic.\n>\n> > > - sync_signal_ is actually never used :-) Or rather was never used, as\n> > > this patch now waits on it in Alsc::waitForAysncThread(). The\n> > > sync_signal_ in the AWB algorithm is however completely unused.\n> >\n> > Yes indeed. All this code was ported from somewhere where you could\n> > force these algorithms to run synchronously (i.e. wait for the\n> > calculations to finish and use them immediately), so these other\n> > signals were indeed being used. I removed that for the libcamera/VC4\n> > platform and never did anything about this signal. Though I'm quite\n> > happy to have left it in now! I'm not sure about the AWB one, I can't\n> > think of any case where waiting for that thread to stop would be\n> > useful like it is here, so maybe that will be one to remove.\n> >\n> > Thanks for all the comments!\n> >\n> > > > Signed-off-by: David Plowman \n> > >\n> > > For this patch,\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > > ---\n> > > > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 39 ++++++++++++++-------\n> > > > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 3 +-\n> > > > 2 files changed, 28 insertions(+), 14 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > > index 76e2f04..8f7720b 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > > > @@ -173,19 +173,34 @@ void Alsc::Initialise()\n> > > > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > > > }\n> > > >\n> > > > +void Alsc::waitForAysncThread()\n> > > > +{\n> > > > + std::unique_lock lock(mutex_);\n> > > > + if (async_started_) {\n> > > > + sync_signal_.wait(lock, [&] {\n> > > > + return async_finished_;\n> > > > + });\n> > > > + async_started_ = false;\n> > > > + async_finished_ = false;\n> > > > + }\n> > > > +}\n> > > > +\n> > > > void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > {\n> > > > (void)metadata;\n> > > >\n> > > > + // Ensure the other thread isn't running while we do this.\n> > > > + waitForAysncThread();\n> > > > +\n> > > > // There's a bit of a question what we should do if the \"crop\" of the\n> > > > - // camera mode has changed. Any calculation currently in flight would\n> > > > - // not be useful to the new mode, so arguably we should abort it, and\n> > > > - // generate a new table (like the \"first_time\" code already here). When\n> > > > - // the crop doesn't change, we can presumably just leave things\n> > > > - // alone. For now, I think we'll just wait and see. When the crop does\n> > > > - // change, any effects should be transient, and if they're not transient\n> > > > - // enough, we'll revisit the question then.\n> > > > + // camera mode has changed. Should we effectively reset the algorithm\n> > > > + // and start over?\n> > > > camera_mode_ = camera_mode;\n> > > > +\n> > > > + // We must resample the luminance table like we do the others, but it's\n> > > > + // fixed so we can simply do it up front here.\n> > > > + resample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);\n> > > > +\n> > > > if (first_time_) {\n> > > > // On the first time, arrange for something sensible in the\n> > > > // initial tables. Construct the tables for some default colour\n> > > > @@ -201,7 +216,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > > > compensate_lambdas_for_cal(cal_table_b, lambda_b_,\n> > > > async_lambda_b_);\n> > > > add_luminance_to_tables(sync_results_, async_lambda_r_, 1.0,\n> > > > - async_lambda_b_, config_.luminance_lut,\n> > > > + async_lambda_b_, luminance_table_,\n> > > > config_.luminance_strength);\n> > > > memcpy(prev_sync_results_, sync_results_,\n> > > > sizeof(prev_sync_results_));\n> > > > @@ -266,8 +281,6 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> > > > }\n> > > > copy_stats(statistics_, stats, alsc_status);\n> > > > frame_phase_ = 0;\n> > > > - // copy the camera mode so it won't change during the calculations\n> > > > - async_camera_mode_ = camera_mode_;\n> > > > async_start_ = true;\n> > > > async_started_ = true;\n> > > > async_signal_.notify_one();\n> > > > @@ -670,9 +683,9 @@ void Alsc::doAlsc()\n> > > > // Fetch the new calibrations (if any) for this CT. Resample them in\n> > > > // case the camera mode is not full-frame.\n> > > > get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> > > > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_r);\n> > > > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> > > > get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> > > > - resample_cal_table(cal_table_tmp, async_camera_mode_, cal_table_b);\n> > > > + resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> > > > // You could print out the cal tables for this image here, if you're\n> > > > // tuning the algorithm...\n> > > > (void)print_cal_table;\n> > > > @@ -695,7 +708,7 @@ void Alsc::doAlsc()\n> > > > compensate_lambdas_for_cal(cal_table_b, lambda_b_, async_lambda_b_);\n> > > > // Fold in the luminance table at the appropriate strength.\n> > > > add_luminance_to_tables(async_results_, async_lambda_r_, 1.0,\n> > > > - async_lambda_b_, config_.luminance_lut,\n> > > > + async_lambda_b_, luminance_table_,\n> > > > config_.luminance_strength);\n> > > > }\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > > index e895913..95572af 100644\n> > > > --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> > > > @@ -60,10 +60,10 @@ private:\n> > > > AlscConfig config_;\n> > > > bool first_time_;\n> > > > CameraMode camera_mode_;\n> > > > + double luminance_table_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> > > > std::thread async_thread_;\n> > > > void asyncFunc(); // asynchronous thread function\n> > > > std::mutex mutex_;\n> > > > - CameraMode async_camera_mode_;\n> > > > // condvar for async thread to wait on\n> > > > std::condition_variable async_signal_;\n> > > > // condvar for synchronous thread to wait on\n> > > > @@ -86,6 +86,7 @@ private:\n> > > > int frame_count2_;\n> > > > double sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> > > > double prev_sync_results_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> > > > + void waitForAysncThread();\n> > > > // The following are for the asynchronous thread to use, though the main\n> > > > // thread can set/reset them if the async thread is known to be idle:\n> > > > void restartAsync(StatisticsPtr &stats, Metadata *image_metadata);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 58989BD878\n\tfor ;\n\tFri, 31 Jul 2020 12:27:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C86A661988;\n\tFri, 31 Jul 2020 14:27:56 +0200 (CEST)","from mail-ot1-x341.google.com (mail-ot1-x341.google.com\n\t[IPv6:2607:f8b0:4864:20::341])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C46660396\n\tfor ;\n\tFri, 31 Jul 2020 14:27:55 +0200 (CEST)","by mail-ot1-x341.google.com with SMTP id l26so5935546otj.4\n\tfor ;\n\tFri, 31 Jul 2020 05:27:55 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"OTaHQnAs\"; dkim-atps=neutral","DKIM-Signature":"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=2sIyRBl1xuPDe90vE1w44Z4bxV2duvKi0CwopnI7XxQ=;\n\tb=OTaHQnAsVCmGOb8J0AZYQ/VfgRYkdaNbhpsBgE7upujQv+5JldrknF0A4T/5IFkOn3\n\tLzhAt4EQsIPZ4wzOwX/sYHPz2gNjpfDA5L9wTxHVVTO/GKX2OdOux3ToW6UsFNG2rj6i\n\t4jLbLJ5F6c3rPhIBLiRKlulQ0PRHrYfePAyzEhcpcvB6FZ3QpJLlkNzpZV+Fwg2Ou4dt\n\ta7rEkqZ5bEJnVVDgivYfIGb/LLX23MlDktCFX3bedv0EiEe05104mZX1ql7K8hK8uyIM\n\t2ck4ODfE8io3gBd4kxnjAFAEYKpRNFQcnCKUwzeAA583SaJZHlZjnha1kGuFfz7Dz+1a\n\t0xpw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=2sIyRBl1xuPDe90vE1w44Z4bxV2duvKi0CwopnI7XxQ=;\n\tb=ubJxEB7RlYSkuX8dUgy00zdTFPTGmCJva6kY7ggD+MNVt0FAXdSaQHa2bIRlU01BAx\n\twqln0ykgwIazH4yH8ngjWXutrpbyGardkPJob0NDIYKETMal9VWwWiTfubqzLFC+vm3g\n\t+IaaruOPf4Aj3QvEwGDoEIwwXjwM2ENhAltKC6TyI4m24ntCf3axCqcRs7G5CxO1/vwu\n\t1Pm/jBCPsNSFY/Z88Dd/Un89XVPZ5E4m0GWHV+58spqm1SaWa6qxZ0tcxyhBQYE2F6hu\n\t29oF0bpXkqjpDDL4rcDidlIJnl+YheMXGF6k+0WWrmR8CZ7ib2CNT3lP5sPlunhTMY+F\n\tI0Jw==","X-Gm-Message-State":"AOAM53078OxBHQgndUsEkm6EkN0SOLMLjvtvZwd4+KE0rNxgZW7QBgvG\n\t3N2FoueJcT2Ew+mkwaH7CNGnjigzxRl/PSWe8glrQg==","X-Google-Smtp-Source":"ABdhPJyDQx19ldMYfB9sUlrSOkviLBVCSFXM/QiVg4Gn6mBgGHCG1pVUP4hIvMN/ZTyRsls3xgftLmD1tkzTZssOsWU=","X-Received":"by 2002:a9d:7994:: with SMTP id\n\th20mr2245124otm.317.1596198473482; \n\tFri, 31 Jul 2020 05:27:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20200730111134.641-1-david.plowman@raspberrypi.com>\n\t<20200730111134.641-3-david.plowman@raspberrypi.com>\n\t<20200730225728.GH6107@pendragon.ideasonboard.com>\n\t\n\t<20200731120006.GC6218@pendragon.ideasonboard.com>","In-Reply-To":"<20200731120006.GC6218@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Fri, 31 Jul 2020 13:27:42 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]