[{"id":11813,"web_url":"https://patchwork.libcamera.org/comment/11813/","msgid":"","date":"2020-08-04T08:49:17","subject":"Re: [libcamera-devel] [PATCH v3 3/4] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for the patch, LGTM.\n\nOn Sat, 1 Aug 2020 at 09:02, David Plowman\n wrote:\n>\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> Reviewed-by: Laurent Pinchart \n\nReviewed-by: Naushir Patuck \n\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> lambda_r_[i] = lambda_b_[i] = 1.0;\n> }\n>\n> +void Alsc::waitForAysncThread()\n> +{\n> + if (async_started_) {\n> + async_started_ = false;\n> + std::unique_lock lock(mutex_);\n> + sync_signal_.wait(lock, [&] {\n> + return async_finished_;\n> + });\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_started_ = true;\n> {\n> std::lock_guard lock(mutex_);\n> @@ -672,9 +685,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> @@ -697,7 +710,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> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 0ED66BD86F\n\tfor ;\n\tTue, 4 Aug 2020 08:49:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D027E61C1F;\n\tTue, 4 Aug 2020 10:49:34 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C51D6038F\n\tfor ;\n\tTue, 4 Aug 2020 10:49:34 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id t6so29735588ljk.9\n\tfor ;\n\tTue, 04 Aug 2020 01:49:34 -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=\"In5Qwp1q\"; 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=YZquQmSInJJGQn+PCswk8dDTiybo9EGOK5rtcf9zDkM=;\n\tb=In5Qwp1qO2XG87MNGaJgscGZHsVkshhHiHRLSULG7iynuXfXUsxabY5ycTjeP8bfy9\n\tle9GwI3nz4Cta84HtvCMEi5PMnhEjaskpQirBnwMXgAv+UI5b5hSGXDcfTi7ElR3P0dl\n\tktZ2T5m+hJvp6xH85cUxA62EPhXCa+hhBsQpz8dl8PfYspnqLSCRxsSELwgryF+bDdeR\n\t393xxHnH8WzJ3LYdBBClTb9ZWYsuh5M+ctueDPz1RCaIGIjREl35yQZko0hbsveKFcwi\n\tNL+veuwtiQ14mAZeiy7qc0bjk3JfY11jAnHkB6NMkHjtKufg8HlyAFe7yYtfhprgzxrZ\n\tTunw==","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=YZquQmSInJJGQn+PCswk8dDTiybo9EGOK5rtcf9zDkM=;\n\tb=gJqLFF37kBQV/OJMykA70zCym7/UJU8A5ErGWvwl1PymiF9CIAfox49uEQ3Wyya4I5\n\th558umILwJaD69OU9V7UKCWvPpfOkCNii5VrNK/TtDJYtM/w6mb/xck+zVXIZGwFrML1\n\tP/wo0IN4rLUnZoVoQN5MQfcVDIgTpurvA0T4drqG72ANh4/Qo3oYZ9v92DIO0cNOlCji\n\t1vwAp5OpAUgbO7yNdBpMnY45dA16O2VOVWgKc0b9LKbWDuLZrr270kwPAJc0U4pBYhxV\n\tGn7z0utNv5c7Tn2Fq7cPKU8WRc+alKknMjmpIZ4NDIo4Ilzy1aRYTICIoh9OFeXTVxlB\n\ta9oQ==","X-Gm-Message-State":"AOAM530dwnBcKLkLf03KYQVcyzG4Upc7LlkftTdBUybD/lQEqZIlQBAF\n\tQeH/m/W+ua/MWkd7j/KcNtGqpj8UGELb7w0q2pKwCW0VqjQ=","X-Google-Smtp-Source":"ABdhPJwDw6CsSmQYXVqGTPZd2egOWWO+fWTj+UpsW/HgDuCGBoUozBebwbcUCjMzFnHifhdz6IyZBzst94kaAquLXMY=","X-Received":"by 2002:a2e:9b95:: with SMTP id\n\tz21mr10029440lji.83.1596530973410; \n\tTue, 04 Aug 2020 01:49:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-4-david.plowman@raspberrypi.com>","In-Reply-To":"<20200801080151.4282-4-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Tue, 4 Aug 2020 09:49:17 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}},{"id":11853,"web_url":"https://patchwork.libcamera.org/comment/11853/","msgid":"<20200804234714.GT6075@pendragon.ideasonboard.com>","date":"2020-08-04T23:47:14","subject":"Re: [libcamera-devel] [PATCH v3 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 Naush,\n\nThanks for the reviews. Do you plan to review 4/4 too ?\n\nOn Tue, Aug 04, 2020 at 09:49:17AM +0100, Naushir Patuck wrote:\n> Hi David,\n> \n> Thank you for the patch, LGTM.\n> \n> On Sat, 1 Aug 2020 at 09:02, David Plowman wrote:\n> >\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> > Reviewed-by: Laurent Pinchart \n> \n> Reviewed-by: Naushir Patuck \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> > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > }\n> >\n> > +void Alsc::waitForAysncThread()\n> > +{\n> > + if (async_started_) {\n> > + async_started_ = false;\n> > + std::unique_lock lock(mutex_);\n> > + sync_signal_.wait(lock, [&] {\n> > + return async_finished_;\n> > + });\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_started_ = true;\n> > {\n> > std::lock_guard lock(mutex_);\n> > @@ -672,9 +685,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> > @@ -697,7 +710,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 ABBBEBD86F\n\tfor ;\n\tTue, 4 Aug 2020 23:47:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41C0A6054A;\n\tWed, 5 Aug 2020 01:47:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DAF260392\n\tfor ;\n\tWed, 5 Aug 2020 01:47:27 +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 665D727B;\n\tWed, 5 Aug 2020 01:47:26 +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=\"MyS5PhPc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596584846;\n\tbh=zi8jwndVpx3EL5Aif5sw4ZKGVY8ug84k6pgb2WiT3xM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MyS5PhPcsk/q0hKO7gsbcHFvLV/PkRKmIKeist59DOfvmG8Z7Vpy8jiCK9J+1Ehxg\n\tWMu6lpylSKBsguCIntAkVL0bp5Qv5KSXg1aZtuwk/7mGvreA+xann4AF8Ta8HBrNKv\n\t/7wndUbLf3iLTpnmVscuU6QTXxTrwXJzCvx2NrSo=","Date":"Wed, 5 Aug 2020 02:47:14 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200804234714.GT6075@pendragon.ideasonboard.com>","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-4-david.plowman@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}},{"id":11860,"web_url":"https://patchwork.libcamera.org/comment/11860/","msgid":"","date":"2020-08-05T06:51:14","subject":"Re: [libcamera-devel] [PATCH v3 3/4] libcamera: ipa: raspberrypi:\n\tALSC: Resample luminance table","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent and David\n\nOn Wed, 5 Aug 2020 at 00:47, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thanks for the reviews. Do you plan to review 4/4 too ?\n\nMy sent mail folder shows I had replied with the Reviewed-by tag for\npatch 4/4. Perhaps something got lost in the way. I will resend.\n\nRegards,\nNaush\n\n\n>\n> On Tue, Aug 04, 2020 at 09:49:17AM +0100, Naushir Patuck wrote:\n> > Hi David,\n> >\n> > Thank you for the patch, LGTM.\n> >\n> > On Sat, 1 Aug 2020 at 09:02, David Plowman wrote:\n> > >\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> > > Reviewed-by: Laurent Pinchart \n> >\n> > Reviewed-by: Naushir Patuck \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> > > lambda_r_[i] = lambda_b_[i] = 1.0;\n> > > }\n> > >\n> > > +void Alsc::waitForAysncThread()\n> > > +{\n> > > + if (async_started_) {\n> > > + async_started_ = false;\n> > > + std::unique_lock lock(mutex_);\n> > > + sync_signal_.wait(lock, [&] {\n> > > + return async_finished_;\n> > > + });\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_started_ = true;\n> > > {\n> > > std::lock_guard lock(mutex_);\n> > > @@ -672,9 +685,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> > > @@ -697,7 +710,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 060B0BD87A\n\tfor ;\n\tWed, 5 Aug 2020 06:51:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 715B56055A;\n\tWed, 5 Aug 2020 08:51:32 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7AB0C6038E\n\tfor ;\n\tWed, 5 Aug 2020 08:51:31 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id x9so46445839ljc.5\n\tfor ;\n\tTue, 04 Aug 2020 23:51:31 -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=\"rLMcijwJ\"; 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=00JlN6hgAFFkAOTxrAqEp9JwpNUkA/eAL8PxNJLuhRU=;\n\tb=rLMcijwJOqU/7zTAzAqUdiCoz2MAO0fkS4vcJEQMhbVuMnYYRxMv5sIauvJZqRDdFn\n\tt5P26Xdb+7oVj/osJyIqtt3QMEaIHBrd/D6ZRglaSSizVtvfPvxbp/HwEV+Om/bzTTB0\n\t7xt+oDMCZkLql3NQBSmkbHlCit88oCAp+aKzMRz/TvtkvIPmRkZz3VM2XdQECDpPRCV8\n\tU+VDuljWL53myMhqHgAMxK3Zlb5dWS7ybkiU1vyUmSYMMqst3K/0fnXpllq+R5foqSnr\n\tfBiri65SedNdjkgCotnv2StLzmflq0rg+pkMBlzlxqEa4HpQM8fZnaJHGRIx4H1OeKrE\n\tS7PQ==","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=00JlN6hgAFFkAOTxrAqEp9JwpNUkA/eAL8PxNJLuhRU=;\n\tb=LYLC9M+OEmVBsMxTa/h0AIxVPAYzAF0uWhtJE29U14eGImu+0vmwhKSglVN8EfR1L9\n\tbiODe2PxsmoSSncFUiGHllg7VgUOul/5jFRmKu0zF6qySzzPR2jo2dN7iHO+jBjzSu+c\n\tXKOs1tPSdFuiDEXuqwD5R5Nd89PYxctPzpmcDQhDxpLrEwIlV2rlML1+HLhJ84iF2Q1u\n\t0YNoe6Q+R1M8RUsDt8A+9wlvtY1wKbsYQUO/uAyZO3O5cxu3cxsObGTPlc1l7Y0rCRgA\n\tOn48j6xa/uUN3FwBluJx4X7HDkVQdWzAoPtQMX2J4MMy2gKA8BupHTw97dRNPSnX6ucv\n\thoaw==","X-Gm-Message-State":"AOAM533G6ghSkPuMDrDJ9Hy5zXhQmPXat9/8BNdZU1vyU/CjjR9SAgd7\n\twflvRvEqa5sQl4HqYwiMoidPTZKYKXGHAUCywat19A==","X-Google-Smtp-Source":"ABdhPJyYD0X5OUGeqyj0tYWuev4jEIkqhuX8ZQKUafnbwsvniuzMXFzvYwR9Tz5c0ZL+GH7eVYuACNlHHOd0NRmJSJk=","X-Received":"by 2002:a2e:9852:: with SMTP id e18mr718476ljj.415.1596610290688;\n\tTue, 04 Aug 2020 23:51:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-4-david.plowman@raspberrypi.com>\n\t\n\t<20200804234714.GT6075@pendragon.ideasonboard.com>","In-Reply-To":"<20200804234714.GT6075@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 5 Aug 2020 07:51:14 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]