[{"id":11758,"web_url":"https://patchwork.libcamera.org/comment/11758/","msgid":"<20200731211459.GE24315@pendragon.ideasonboard.com>","date":"2020-07-31T21:18:36","subject":"Re: [libcamera-devel] [PATCH v2 3/4] 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 Fri, Jul 31, 2020 at 03:08:00PM +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> Signed-off-by: David Plowman \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 12359dc..4df9934 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> +\tif (async_started_) {\n> +\t\tasync_started_ = false;\n> +\t\tstd::unique_lock lock(mutex_);\n> +\t\tsync_signal_.wait(lock, [&] {\n> +\t\t\treturn async_finished_;\n> +\t\t});\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_started_ = true;\n> \t{\n> \t\tstd::lock_guard lock(mutex_);\n> @@ -672,9 +685,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> @@ -697,7 +710,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 CCEACBD86F\n\tfor ;\n\tFri, 31 Jul 2020 21:18:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D5B761F1B;\n\tFri, 31 Jul 2020 23:18:48 +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 4D02D60398\n\tfor ;\n\tFri, 31 Jul 2020 23:18:46 +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 BBB9253C;\n\tFri, 31 Jul 2020 23:18:45 +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=\"UOGa/Yoo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596230325;\n\tbh=pO+Bb5XGcgfCSmtvLVLkv/WBWuL9cFNrVF5dDOaMXvc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UOGa/YooxF5Wh2G6477j3j1e+cixnAEb7Ixf4QYJx1cmEATJ3niKkb1JFD/8Mxycc\n\tMECLbVx25aULvsh7Ah7h6+cmYHgvxAjR/laFNibza8OUzOT5E4yNYk/wcA5080wq54\n\tdpg5rqVyn3beXnZl0Xi7yyo2SRx/jnPhYdf8/AJs=","Date":"Sat, 1 Aug 2020 00:18:36 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200731211459.GE24315@pendragon.ideasonboard.com>","References":"<20200731140801.13253-1-david.plowman@raspberrypi.com>\n\t<20200731140801.13253-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200731140801.13253-4-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] 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\" "}}]