[{"id":11723,"web_url":"https://patchwork.libcamera.org/comment/11723/","msgid":"<20200729153405.GK6183@pendragon.ideasonboard.com>","date":"2020-07-29T15:34:05","subject":"Re: [libcamera-devel] [PATCH 5/5] libcamera: raspberrypi: ALSC: Fix\n\tcrop/transform of 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 Wed, Jul 29, 2020 at 10:31:54AM +0100, David Plowman wrote:\n> The code was not handling a change of crop correctly in the luminance\n> table. This change fixes that, and also makes it work for a change of\n> transform.\n\nI'm not familiar with this code at all, so this is hard to review for\nme. Splitting the patch in two, with the fix first (which could be moved\nat the top of the series), and plumbing the transform handling in a\nseparate step, would help. A slightly more detailed description of the\nfix in the commit message could also help. And finally, a review from\nNaush would clearly help :-)\n\n> As part of this it seemed sensible to ensure the asynchronous thread\n> is paused in the SwitchMode, at which point it also makes sense to\n> detect \"significant\" mode changes and to restart the whole ALSC\n> algorithm when that happens.\n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 87 ++++++++++++++-------\n> src/ipa/raspberrypi/controller/rpi/alsc.hpp | 3 +-\n> 2 files changed, 61 insertions(+), 29 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 33c1a88..fa53314 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -168,46 +168,79 @@ 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> +{\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> +static bool compare_crops(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\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\t\t\t cm1.crop_y - cm1.scale_y * cm1.height);\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\ttop_diff > threshold_y || bottom_diff > threshold_y;\n> }\n> \n> void Alsc::SwitchMode(CameraMode const &camera_mode, Transform transform, Metadata *metadata)\n> {\n> \t(void)metadata;\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// We're going to start over with the tables if there's any \"significant\"\n> +\t// change.\n> +\tbool reset_tables = first_time_ || transform_ != transform ||\n> +\t\tcompare_crops(camera_mode_, camera_mode);\n> +\n> +\t// Ensure the other thread isn't running while we do this.\n> +\twaitForAysncThread();\n> +\n> \tcamera_mode_ = camera_mode;\n> \ttransform_ = transform;\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> +\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_, transform_, luminance_table_);\n> +\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_, transform_, 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_, transform_, cal_table_b);\n> \t\tcompensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> \t\t\t\t\t async_lambda_r_);\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> +\t\tframe_phase_ = config_.frame_period; // run the algo again asap\n> \t\tfirst_time_ = false;\n> \t}\n> }\n> @@ -253,8 +286,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. Use the previous value if none found.\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;\n> @@ -269,8 +302,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> @@ -679,9 +710,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_, transform_, cal_table_r);\n> +\tresample_cal_table(cal_table_tmp, camera_mode_, transform_, cal_table_r);\n> \tget_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> -\tresample_cal_table(cal_table_tmp, async_camera_mode_, transform_, cal_table_b);\n> +\tresample_cal_table(cal_table_tmp, camera_mode_, transform_, 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> @@ -704,7 +735,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 9001e8a..63bf28f 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> @@ -62,10 +62,10 @@ private:\n> \tbool first_time_;\n> \tCameraMode camera_mode_;\n> \tlibcamera::Transform transform_;\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> @@ -88,6 +88,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 9825DBD86F\n\tfor ;\n\tWed, 29 Jul 2020 15:34:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2ADE8613C6;\n\tWed, 29 Jul 2020 17:34:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 363E56039D\n\tfor ;\n\tWed, 29 Jul 2020 17:34:15 +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 AB6FC31F;\n\tWed, 29 Jul 2020 17:34:14 +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=\"YR3oaeiD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596036854;\n\tbh=jLKsj8SmHDYR/fv0UA44pJ1eOsm/cqzOXrsviLI690k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YR3oaeiDYh6WkjuyioiWCMHYZVKfN+xWXDx5bx31ClI62PhYIXIO0C6XoZZMdn8ie\n\tweaDsKX+yeEmr/jUOGJjHHg45/CoSs+BnbUTIPgf68H1oi4Ld9zRdf9qgmdI8LG48X\n\t/iPrcfZVb2jJMzcIDLofFkuGqqPN+ushmBWfSNlE=","Date":"Wed, 29 Jul 2020 18:34:05 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200729153405.GK6183@pendragon.ideasonboard.com>","References":"<20200729093154.12296-1-david.plowman@raspberrypi.com>\n\t<20200729093154.12296-6-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729093154.12296-6-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 5/5] libcamera: raspberrypi: ALSC: Fix\n\tcrop/transform of 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\" "}}]