[{"id":11767,"web_url":"https://patchwork.libcamera.org/comment/11767/","msgid":"<20200801123417.GA11820@pendragon.ideasonboard.com>","date":"2020-08-01T12:34:17","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Sat, Aug 01, 2020 at 09:01:51AM +0100, David Plowman wrote:\n> Now that we stop the asynchronous thread on a SwitchMode, we would do\n> better to regenerate all the tables if the new camera mode crops in a\n> significantly different way to the old one. A few minor tweaks make\n> sense along with this:\n> \n> * Reset the lambda values when we reset everything. It wouldn't make\n> sense to re-start with the old mode's values.\n> \n> * Use the last recorded colour temperature to generate new tables rather\n> than any default value.\n> \n> * Set the frame \"phase\" counter to ensure the adaptive procedure will\n> run asap.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------\n> 1 file changed, 37 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 4df9934..5e55954 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -166,11 +166,8 @@ void Alsc::Initialise()\n> \tRPI_LOG(\"Alsc\");\n> \tframe_count2_ = frame_count_ = frame_phase_ = 0;\n> \tfirst_time_ = true;\n> -\t// Initialise the lambdas. Each call to Process then restarts from the\n> -\t// previous results. Also initialise the previous frame tables to the\n> -\t// same harmless values.\n> -\tfor (int i = 0; i < XY; i++)\n> -\t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n> +\tct_ = config_.default_ct;\n> +\t// The lambdas are initialised in the SwitchMode.\n> }\n> \n> void Alsc::waitForAysncThread()\n> @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()\n> \t}\n> }\n> \n> +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)\n> +{\n> +\t// Return true if the modes crop from the sensor significantly differently.\n> +\tint left_diff = fabs(cm0.crop_x - cm1.crop_x);\n> +\tint top_diff = fabs(cm0.crop_y - cm1.crop_y);\n\nNote that these two could still use abs(), it's only the two lines below\nthat are given a double argument. It doesn't matter much though, and if\nyou think abs() is better than fabs() here, I can fix when applying, no\nneed to send a new version.\n\n> +\tint right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width -\n> +\t\t\t\t cm1.crop_x - cm1.scale_x * cm1.width);\n> +\tint bottom_diff = fabs(cm0.crop_y + cm0.scale_y * cm0.height -\n> +\t\t\t\t cm1.crop_y - cm1.scale_y * cm1.height);\n> +\t// These thresholds are a rather arbitrary amount chosen to trigger\n> +\t// when carrying on with the previously calculated tables might be\n> +\t// worse than regenerating them (but without the adaptive algorithm).\n> +\tint threshold_x = cm0.sensor_width >> 4;\n> +\tint threshold_y = cm0.sensor_height >> 4;\n> +\treturn left_diff > threshold_x || right_diff > threshold_x ||\n> +\t top_diff > threshold_y || bottom_diff > threshold_y;\n> +}\n> +\n> void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> \t(void)metadata;\n> \n> +\t// We're going to start over with the tables if there's any \"significant\"\n> +\t// change.\n> +\tbool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);\n> +\n> \t// Ensure the other thread isn't running while we do this.\n> \twaitForAysncThread();\n> \n> -\t// There's a bit of a question what we should do if the \"crop\" of the\n> -\t// camera mode has changed. Should we effectively reset the algorithm\n> -\t// and start over?\n> \tcamera_mode_ = camera_mode;\n> \n> \t// We must resample the luminance table like we do the others, but it's\n> \t// fixed so we can simply do it up front here.\n> \tresample_cal_table(config_.luminance_lut, camera_mode_, luminance_table_);\n> \n> -\tif (first_time_) {\n> -\t\t// On the first time, arrange for something sensible in the\n> -\t\t// initial tables. Construct the tables for some default colour\n> -\t\t// temperature. This echoes the code in doAlsc, without the\n> -\t\t// adaptive algorithm.\n> +\tif (reset_tables) {\n> +\t\t// Upon every \"table reset\", arrange for something sensible to be\n> +\t\t// generated. Construct the tables for the previous recorded colour\n> +\t\t// temperature. In order to start over from scratch we initialise\n> +\t\t// the lambdas, but the rest of this code then echoes the code in\n> +\t\t// doAlsc, without the adaptive algorithm.\n> +\t\tfor (int i = 0; i < XY; i++)\n> +\t\t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n> \t\tdouble cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];\n> -\t\tget_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);\n> +\t\tget_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> \t\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> -\t\tget_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);\n> +\t\tget_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> \t\tresample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> \t\tcompensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> \t\t\t\t\t async_lambda_r_);\n> @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> \t\t\t\t\tconfig_.luminance_strength);\n> \t\tmemcpy(prev_sync_results_, sync_results_,\n> \t\t sizeof(prev_sync_results_));\n> +\t\tframe_phase_ = config_.frame_period; // run the algo again asap\n> \t\tfirst_time_ = false;\n> \t}\n> }\n> @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> {\n> \tRPI_LOG(\"Starting ALSC thread\");\n> \t// Get the current colour temperature. It's all we need from the\n> -\t// metadata.\n> -\tct_ = get_ct(image_metadata, config_.default_ct);\n> +\t// metadata. Default to the last CT value (which could be the default).\n> +\tct_ = get_ct(image_metadata, ct_);\n> \t// We have to copy the statistics here, dividing out our best guess of\n> \t// the LSC table that the pipeline applied to them.\n> \tAlscStatus alsc_status;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C8CE6BD878\n\tfor ;\n\tSat, 1 Aug 2020 12:34:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53D4C61F2E;\n\tSat, 1 Aug 2020 14:34:29 +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 0B67161F24\n\tfor ;\n\tSat, 1 Aug 2020 14:34:28 +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 76F1155E;\n\tSat, 1 Aug 2020 14:34:27 +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=\"aVc7BXss\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596285267;\n\tbh=kKZMmLQ77PST6UTExvy+ulEGDobzAig5ARU9RiYNNdA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aVc7BXssqQv2YUz/ZK0m4VZQludN3SfHJCk3t5V8CGbNmzMereBy2JYyanOtEVbzf\n\tzuunPuVNjuJ7PyLBmkPQSAyHcNkdIjbVp1v68qFJbqPHFfwPvBoE9hvuvvaRibUE6R\n\t1CxaiaooF7A/Aq94bRa15X9G7CHHE2mSt6RVFhzk=","Date":"Sat, 1 Aug 2020 15:34:17 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200801123417.GA11820@pendragon.ideasonboard.com>","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-5-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200801080151.4282-5-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11771,"web_url":"https://patchwork.libcamera.org/comment/11771/","msgid":"","date":"2020-08-01T15:21:41","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Ah. I think maybe abs is better than fabs for ints, no point turning\neverything into floats for no reason, but don't particularly mind. But\nthanks for taking care of this!\n\nBest regards\nDavid\n\nOn Sat, 1 Aug 2020 at 13:34, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Sat, Aug 01, 2020 at 09:01:51AM +0100, David Plowman wrote:\n> > Now that we stop the asynchronous thread on a SwitchMode, we would do\n> > better to regenerate all the tables if the new camera mode crops in a\n> > significantly different way to the old one. A few minor tweaks make\n> > sense along with this:\n> >\n> > * Reset the lambda values when we reset everything. It wouldn't make\n> > sense to re-start with the old mode's values.\n> >\n> > * Use the last recorded colour temperature to generate new tables rather\n> > than any default value.\n> >\n> > * Set the frame \"phase\" counter to ensure the adaptive procedure will\n> > run asap.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------\n> > 1 file changed, 37 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > index 4df9934..5e55954 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > @@ -166,11 +166,8 @@ void Alsc::Initialise()\n> > RPI_LOG(\"Alsc\");\n> > frame_count2_ = frame_count_ = frame_phase_ = 0;\n> > first_time_ = true;\n> > - // Initialise the lambdas. Each call to Process then restarts from the\n> > - // previous results. Also initialise the previous frame tables to the\n> > - // same harmless values.\n> > - for (int i = 0; i < XY; i++)\n> > - lambda_r_[i] = lambda_b_[i] = 1.0;\n> > + ct_ = config_.default_ct;\n> > + // The lambdas are initialised in the SwitchMode.\n> > }\n> >\n> > void Alsc::waitForAysncThread()\n> > @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()\n> > }\n> > }\n> >\n> > +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)\n> > +{\n> > + // Return true if the modes crop from the sensor significantly differently.\n> > + int left_diff = fabs(cm0.crop_x - cm1.crop_x);\n> > + int top_diff = fabs(cm0.crop_y - cm1.crop_y);\n>\n> Note that these two could still use abs(), it's only the two lines below\n> that are given a double argument. It doesn't matter much though, and if\n> you think abs() is better than fabs() here, I can fix when applying, no\n> need to send a new version.\n>\n> > + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width -\n> > + cm1.crop_x - cm1.scale_x * cm1.width);\n> > + int bottom_diff = fabs(cm0.crop_y + cm0.scale_y * cm0.height -\n> > + cm1.crop_y - cm1.scale_y * cm1.height);\n> > + // These thresholds are a rather arbitrary amount chosen to trigger\n> > + // when carrying on with the previously calculated tables might be\n> > + // worse than regenerating them (but without the adaptive algorithm).\n> > + int threshold_x = cm0.sensor_width >> 4;\n> > + int threshold_y = cm0.sensor_height >> 4;\n> > + return left_diff > threshold_x || right_diff > threshold_x ||\n> > + top_diff > threshold_y || bottom_diff > threshold_y;\n> > +}\n> > +\n> > void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > (void)metadata;\n> >\n> > + // We're going to start over with the tables if there's any \"significant\"\n> > + // change.\n> > + bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);\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. 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> > - // temperature. This echoes the code in doAlsc, without the\n> > - // adaptive algorithm.\n> > + if (reset_tables) {\n> > + // Upon every \"table reset\", arrange for something sensible to be\n> > + // generated. Construct the tables for the previous recorded colour\n> > + // temperature. In order to start over from scratch we initialise\n> > + // the lambdas, but the rest of this code then echoes the code in\n> > + // doAlsc, without the adaptive algorithm.\n> > + for (int i = 0; i < XY; i++)\n> > + lambda_r_[i] = lambda_b_[i] = 1.0;\n> > double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];\n> > - get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);\n> > + get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> > resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> > - get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);\n> > + get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> > resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> > compensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> > async_lambda_r_);\n> > @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > config_.luminance_strength);\n> > memcpy(prev_sync_results_, sync_results_,\n> > sizeof(prev_sync_results_));\n> > + frame_phase_ = config_.frame_period; // run the algo again asap\n> > first_time_ = false;\n> > }\n> > }\n> > @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> > {\n> > RPI_LOG(\"Starting ALSC thread\");\n> > // Get the current colour temperature. It's all we need from the\n> > - // metadata.\n> > - ct_ = get_ct(image_metadata, config_.default_ct);\n> > + // metadata. Default to the last CT value (which could be the default).\n> > + ct_ = get_ct(image_metadata, ct_);\n> > // We have to copy the statistics here, dividing out our best guess of\n> > // the LSC table that the pipeline applied to them.\n> > AlscStatus alsc_status;\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 18FAFBD86F\n\tfor ;\n\tSat, 1 Aug 2020 15:21:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9990361F88;\n\tSat, 1 Aug 2020 17:21:55 +0200 (CEST)","from mail-ot1-x342.google.com (mail-ot1-x342.google.com\n\t[IPv6:2607:f8b0:4864:20::342])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0BEE61F2E\n\tfor ;\n\tSat, 1 Aug 2020 17:21:53 +0200 (CEST)","by mail-ot1-x342.google.com with SMTP id x24so749668otp.3\n\tfor ;\n\tSat, 01 Aug 2020 08:21:53 -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=\"SJ/z0PTg\"; 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=Flj58hpEUeS8SH0fXvbs20zHD23oGgM8s6hTPOQiIJs=;\n\tb=SJ/z0PTgI2Hehescdn6He/Kah1sWqauc4ITveBqQ6gi2flDwBAQiLByqwlFNyqcEsT\n\tCQ2IMVptL6aOEUwNcglxBHKMdh9uoZu4cYepOjNs8eSIuQkdKvoquGBvlCppWKdEwr34\n\tlTTOETtUZvAHeEysT9cOFoWXaQKr79uwFGxIhWlm9mDtid65b4K5vbVvGxjb+Z2Xc67Q\n\thetCAHxCo5lRbtifTFhwqhx5UO/Y93G9TTIZKgxd/LIEhQSByL5tuXYQ1WJB4z9ZyvZL\n\t5T7xagjdiU9u8sKE3S1MDlV2z2tpj474DyDIb2B9OZEsyqhtrs0BUd1ypisCYkoA8qU7\n\t5ZBQ==","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=Flj58hpEUeS8SH0fXvbs20zHD23oGgM8s6hTPOQiIJs=;\n\tb=aM9svXknivTJrAyC5+uWOltk9eT+apZwbLQejqA17tdTeSB5SXCsaIW6+JFQn6vCTk\n\tMiG6vJaiarRh3Clik0F6rjmHHrSFS+3dZe4AVilE0jifVldIgOgjSLp5ZvjT9NAW1uqv\n\tLiw778Lgi3gWKUCWrACjDhOJzRnvmGFogxRTZ301pZDVbQt/hBMDblnmFqp5CE9V5kmj\n\trdecjfIIIv3BdZlF5U5yfatT0mjk8QO2D/8eCZBRhW4gQ0CNcEzP1HFPMTDdysvZRCMF\n\tA7FcyOgwxfs6D6RXVwne1KSM+XPePwRXNjZX5TuGc7DZJMKEp2HjYuE6gdrF+UyZDpFg\n\tlDZw==","X-Gm-Message-State":"AOAM532maiHz44CAaMdLoKWKeIkF9Rb6v396EJ4Ij5kjzvUVL4icenSz\n\tQyGHNVVCmRlCaPneL4HdKEOGGhO/VLfaadHY+CQNoVJ0vDE=","X-Google-Smtp-Source":"ABdhPJz4ay9CSvQ+CaXdn4r0f7b1gDB1GSbWAg5GN1C1Dph3nBBa9tBBYW0mjMxUJg/8w4riu1o0IQIIfJnBi5a+7Tw=","X-Received":"by 2002:a9d:7994:: with SMTP id\n\th20mr7004727otm.317.1596295312297; \n\tSat, 01 Aug 2020 08:21:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-5-david.plowman@raspberrypi.com>\n\t<20200801123417.GA11820@pendragon.ideasonboard.com>","In-Reply-To":"<20200801123417.GA11820@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Sat, 1 Aug 2020 16:21:41 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11810,"web_url":"https://patchwork.libcamera.org/comment/11810/","msgid":"","date":"2020-08-04T08:48:56","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","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.\n\nOn Sat, 1 Aug 2020 at 09:02, David Plowman\n wrote:\n>\n> Now that we stop the asynchronous thread on a SwitchMode, we would do\n> better to regenerate all the tables if the new camera mode crops in a\n> significantly different way to the old one. A few minor tweaks make\n> sense along with this:\n>\n> * Reset the lambda values when we reset everything. It wouldn't make\n> sense to re-start with the old mode's values.\n>\n> * Use the last recorded colour temperature to generate new tables rather\n> than any default value.\n>\n> * Set the frame \"phase\" counter to ensure the adaptive procedure will\n> run asap.\n>\n> Signed-off-by: David Plowman \n\nAll looks good to me. Apart from the fabs -> abs change:\n\nReviewed-by: Naushir Patuck \n\n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------\n> 1 file changed, 37 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 4df9934..5e55954 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -166,11 +166,8 @@ void Alsc::Initialise()\n> RPI_LOG(\"Alsc\");\n> frame_count2_ = frame_count_ = frame_phase_ = 0;\n> first_time_ = true;\n> - // Initialise the lambdas. Each call to Process then restarts from the\n> - // previous results. Also initialise the previous frame tables to the\n> - // same harmless values.\n> - for (int i = 0; i < XY; i++)\n> - lambda_r_[i] = lambda_b_[i] = 1.0;\n> + ct_ = config_.default_ct;\n> + // The lambdas are initialised in the SwitchMode.\n> }\n>\n> void Alsc::waitForAysncThread()\n> @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()\n> }\n> }\n>\n> +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)\n> +{\n> + // Return true if the modes crop from the sensor significantly differently.\n> + int left_diff = fabs(cm0.crop_x - cm1.crop_x);\n> + int top_diff = fabs(cm0.crop_y - cm1.crop_y);\n> + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width -\n> + cm1.crop_x - cm1.scale_x * cm1.width);\n> + int bottom_diff = fabs(cm0.crop_y + cm0.scale_y * cm0.height -\n> + cm1.crop_y - cm1.scale_y * cm1.height);\n> + // These thresholds are a rather arbitrary amount chosen to trigger\n> + // when carrying on with the previously calculated tables might be\n> + // worse than regenerating them (but without the adaptive algorithm).\n> + int threshold_x = cm0.sensor_width >> 4;\n> + int threshold_y = cm0.sensor_height >> 4;\n> + return left_diff > threshold_x || right_diff > threshold_x ||\n> + top_diff > threshold_y || bottom_diff > threshold_y;\n> +}\n> +\n> void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> (void)metadata;\n>\n> + // We're going to start over with the tables if there's any \"significant\"\n> + // change.\n> + bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);\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. 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> - // temperature. This echoes the code in doAlsc, without the\n> - // adaptive algorithm.\n> + if (reset_tables) {\n> + // Upon every \"table reset\", arrange for something sensible to be\n> + // generated. Construct the tables for the previous recorded colour\n> + // temperature. In order to start over from scratch we initialise\n> + // the lambdas, but the rest of this code then echoes the code in\n> + // doAlsc, without the adaptive algorithm.\n> + for (int i = 0; i < XY; i++)\n> + lambda_r_[i] = lambda_b_[i] = 1.0;\n> double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];\n> - get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);\n> + get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> - get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);\n> + get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> compensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> async_lambda_r_);\n> @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> config_.luminance_strength);\n> memcpy(prev_sync_results_, sync_results_,\n> sizeof(prev_sync_results_));\n> + frame_phase_ = config_.frame_period; // run the algo again asap\n> first_time_ = false;\n> }\n> }\n> @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> {\n> RPI_LOG(\"Starting ALSC thread\");\n> // Get the current colour temperature. It's all we need from the\n> - // metadata.\n> - ct_ = get_ct(image_metadata, config_.default_ct);\n> + // metadata. Default to the last CT value (which could be the default).\n> + ct_ = get_ct(image_metadata, ct_);\n> // We have to copy the statistics here, dividing out our best guess of\n> // the LSC table that the pipeline applied to them.\n> AlscStatus alsc_status;\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 479F3BD86F\n\tfor ;\n\tTue, 4 Aug 2020 08:49:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2EB361B3F;\n\tTue, 4 Aug 2020 10:49:15 +0200 (CEST)","from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3FDF6038F\n\tfor ;\n\tTue, 4 Aug 2020 10:49:13 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id w25so7216632ljo.12\n\tfor ;\n\tTue, 04 Aug 2020 01:49:13 -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=\"hvbHvEXc\"; 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=elm4wrytxEqEBUHHmb/CTbbp2cYsPzELMmNsasAweKM=;\n\tb=hvbHvEXc+bJjTwqn5mOOjT+St4eChNYoLmHUud+rKubPYxwpmnawln7pTPtD491smV\n\tnJZv2IvooXVF/IXfXYvrDOl4s7058VdY3inwu4Q0F6iQumylVKLxjY1yndUpz5NeJsFL\n\tOBt2lDXrOLT2C0X/aLHLPdHwsW8y2CLao98uiASWmPzaxhJDb0a/7oJLLquGqlvdGPWH\n\tGzMebPagSCDKVxouK53MM/6CczaaHckWsmCI1EqaX9b/QJlghzycwJxn90KeN39I8vGV\n\tWGRw78/CTiN/5DMXLn5AlLNFz+t99zNGzqXuKat6qFMQA9u97M5sU7V8PTf5wIMD2xA9\n\tPAvg==","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=elm4wrytxEqEBUHHmb/CTbbp2cYsPzELMmNsasAweKM=;\n\tb=l239vuhCFAhN4N+2aG6zy2JerAA4FSCCa2wGMIUhweNeOQm7x3dTEUMkX23/XGE9io\n\t7TV1j2S6c1UipejnOM3bC7V+VslIjioP4JeT/Sevidq8HBlI4imbIbDwKJdVmCelfNy1\n\ttUjyicpA1Vzlg+JttEKv9TIdLzzpFWJvqJHKYeECHLBW/Xz4Yx/1eSZ3QVO4aYeG/dBK\n\t8M852tyUlBNlA3eNxZ0t6RDapwAQwUZMJGDJTofXaFo3POsk3BzDFr8VUYhPJ0ugFgyC\n\tOAW8zCuYvL/RYxBlf7d47Vx0bq8HwXdCmGShSeMFsmox33w4DWn0v3NfpvjfcUze/7DZ\n\t8Rag==","X-Gm-Message-State":"AOAM532g00GpQFtyyA87wzXT83lnN2c/YEYn5h55kbeAaTjEL6TJEfsn\n\tBX9w7NVh6p/Wsz7TggqFYary/EIUzcdmbxwqFw0Z9g==","X-Google-Smtp-Source":"ABdhPJzFfBp1q/JvBIW1zvursP6TcfmkOFTWhg9HGG7sOT8Nm72xo0qtr463aJ4qJzX0sZxfbrNf1KDQzr/CaUPW8U0=","X-Received":"by 2002:a2e:9852:: with SMTP id\n\te18mr4709016ljj.415.1596530953120; \n\tTue, 04 Aug 2020 01:49:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-5-david.plowman@raspberrypi.com>","In-Reply-To":"<20200801080151.4282-5-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Tue, 4 Aug 2020 09:48:56 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11861,"web_url":"https://patchwork.libcamera.org/comment/11861/","msgid":"","date":"2020-08-05T06:51:54","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nOn Sat, 1 Aug 2020 at 09:02, David Plowman\n wrote:\n>\n> Now that we stop the asynchronous thread on a SwitchMode, we would do\n> better to regenerate all the tables if the new camera mode crops in a\n> significantly different way to the old one. A few minor tweaks make\n> sense along with this:\n>\n> * Reset the lambda values when we reset everything. It wouldn't make\n> sense to re-start with the old mode's values.\n>\n> * Use the last recorded colour temperature to generate new tables rather\n> than any default value.\n>\n> * Set the frame \"phase\" counter to ensure the adaptive procedure will\n> run asap.\n>\n> Signed-off-by: David Plowman \n\nReviewed-by: Naushir Patuck \n\n> ---\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------\n> 1 file changed, 37 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 4df9934..5e55954 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -166,11 +166,8 @@ void Alsc::Initialise()\n> RPI_LOG(\"Alsc\");\n> frame_count2_ = frame_count_ = frame_phase_ = 0;\n> first_time_ = true;\n> - // Initialise the lambdas. Each call to Process then restarts from the\n> - // previous results. Also initialise the previous frame tables to the\n> - // same harmless values.\n> - for (int i = 0; i < XY; i++)\n> - lambda_r_[i] = lambda_b_[i] = 1.0;\n> + ct_ = config_.default_ct;\n> + // The lambdas are initialised in the SwitchMode.\n> }\n>\n> void Alsc::waitForAysncThread()\n> @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()\n> }\n> }\n>\n> +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)\n> +{\n> + // Return true if the modes crop from the sensor significantly differently.\n> + int left_diff = fabs(cm0.crop_x - cm1.crop_x);\n> + int top_diff = fabs(cm0.crop_y - cm1.crop_y);\n> + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width -\n> + cm1.crop_x - cm1.scale_x * cm1.width);\n> + int bottom_diff = fabs(cm0.crop_y + cm0.scale_y * cm0.height -\n> + cm1.crop_y - cm1.scale_y * cm1.height);\n> + // These thresholds are a rather arbitrary amount chosen to trigger\n> + // when carrying on with the previously calculated tables might be\n> + // worse than regenerating them (but without the adaptive algorithm).\n> + int threshold_x = cm0.sensor_width >> 4;\n> + int threshold_y = cm0.sensor_height >> 4;\n> + return left_diff > threshold_x || right_diff > threshold_x ||\n> + top_diff > threshold_y || bottom_diff > threshold_y;\n> +}\n> +\n> void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> {\n> (void)metadata;\n>\n> + // We're going to start over with the tables if there's any \"significant\"\n> + // change.\n> + bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);\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. 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> - // temperature. This echoes the code in doAlsc, without the\n> - // adaptive algorithm.\n> + if (reset_tables) {\n> + // Upon every \"table reset\", arrange for something sensible to be\n> + // generated. Construct the tables for the previous recorded colour\n> + // temperature. In order to start over from scratch we initialise\n> + // the lambdas, but the rest of this code then echoes the code in\n> + // doAlsc, without the adaptive algorithm.\n> + for (int i = 0; i < XY; i++)\n> + lambda_r_[i] = lambda_b_[i] = 1.0;\n> double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];\n> - get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);\n> + get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> - get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);\n> + get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> compensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> async_lambda_r_);\n> @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> config_.luminance_strength);\n> memcpy(prev_sync_results_, sync_results_,\n> sizeof(prev_sync_results_));\n> + frame_phase_ = config_.frame_period; // run the algo again asap\n> first_time_ = false;\n> }\n> }\n> @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> {\n> RPI_LOG(\"Starting ALSC thread\");\n> // Get the current colour temperature. It's all we need from the\n> - // metadata.\n> - ct_ = get_ct(image_metadata, config_.default_ct);\n> + // metadata. Default to the last CT value (which could be the default).\n> + ct_ = get_ct(image_metadata, ct_);\n> // We have to copy the statistics here, dividing out our best guess of\n> // the LSC table that the pipeline applied to them.\n> AlscStatus alsc_status;\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 441F9BD87A\n\tfor ;\n\tWed, 5 Aug 2020 06:52:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 094CF6038E;\n\tWed, 5 Aug 2020 08:52:13 +0200 (CEST)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 624B96038E\n\tfor ;\n\tWed, 5 Aug 2020 08:52:11 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id s9so23719577lfs.4\n\tfor ;\n\tTue, 04 Aug 2020 23:52:11 -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=\"bg4v3lKH\"; 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=MsZDs+BxdtLbvDn+hJNSs0Ik4T5oWr7dw4bmdfy4QMI=;\n\tb=bg4v3lKH++6Ay+3hGn3JMkj7XAoCkOuFZ93Sva71udAFru/5KQCvPb7ZW4I0VFerHz\n\t99VqYfsAICmk0AMlJwinoAq0k8Qmxy24Hg4d7i7HCQwIueiRxcLOJ15Alye6tV4JBDm2\n\tO2/VcgtjNefbbizNMAMdQhnELMBeWiIJ38HomlASCA9YmOIpA7GQEjiKU6MREBBesTkv\n\tPbAqccN9fM90WbWK9Yn0eb8HMcPyK055vecV8f1lxOPqi8uhxhVjfteBhX/XKTa/9OKl\n\tydpcUNwfVY7hJgE1MwDXAEDgDYyJwIizzD0BgvBXlKGaMvMCVAE4I77/UPYtuEaTX7np\n\tBnLg==","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=MsZDs+BxdtLbvDn+hJNSs0Ik4T5oWr7dw4bmdfy4QMI=;\n\tb=ET9CTATvUeKjwIdkbMW8cOqyMFKetZNSYt95gcbTc5PHWmJeQuXRSJMT42R9hSaBaT\n\t92+zuhExzFTQm6nVI+zBJGRhNL/fO4QDbT8z/PYzdjrrakLEsu97UkqiKlYie18wItpu\n\tTgD+67fpOdh6vn+OC5t3RfqLYYA/6ir77jlp38HELZIIckvH7NbO9EDbo3+WOc4gB25/\n\tvEQ2ln9lLH/YdFq3j4cA3Jn8RzqNN0ykLqeeUoLZJsBBTeDNSbl4XGmMPqRCBFDJVD0r\n\teuZwNuCE7KyFkJOsU8maisdIlQk5bgC952jRn82fMWYM1uZTBc2RWwneQSrWH2pdR7F6\n\tr3Jw==","X-Gm-Message-State":"AOAM531UMlPL3vUbqeQUVjAIhELUVJVk1ElGQmVg/2l2CzlOMRpNKq7t\n\tr03MJTkRocUprfo89zIWSsXJzsUS9pgFwlXgdHtbQA==","X-Google-Smtp-Source":"ABdhPJymlhh0IfZlhNQXCorMLDJJc9Yc+OHmWxe7Kr2GYhS1lCKNswlKzAo5NU8h1Vi3X537A0g2VgTkd30q7WbjD4g=","X-Received":"by 2002:ac2:4c05:: with SMTP id t5mr893774lfq.89.1596610330544; \n\tTue, 04 Aug 2020 23:52:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-5-david.plowman@raspberrypi.com>","In-Reply-To":"<20200801080151.4282-5-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 5 Aug 2020 07:51:54 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11892,"web_url":"https://patchwork.libcamera.org/comment/11892/","msgid":"<20200805150131.GJ6751@pendragon.ideasonboard.com>","date":"2020-08-05T15:01:31","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Aug 05, 2020 at 07:51:54AM +0100, Naushir Patuck wrote:\n> On Sat, 1 Aug 2020 at 09:02, David Plowman wrote:\n> >\n> > Now that we stop the asynchronous thread on a SwitchMode, we would do\n> > better to regenerate all the tables if the new camera mode crops in a\n> > significantly different way to the old one. A few minor tweaks make\n> > sense along with this:\n> >\n> > * Reset the lambda values when we reset everything. It wouldn't make\n> > sense to re-start with the old mode's values.\n> >\n> > * Use the last recorded colour temperature to generate new tables rather\n> > than any default value.\n> >\n> > * Set the frame \"phase\" counter to ensure the adaptive procedure will\n> > run asap.\n> >\n> > Signed-off-by: David Plowman \n> \n> Reviewed-by: Naushir Patuck \n\nThank you. I've pushed the series to the master branch.\n\n> > ---\n> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 54 ++++++++++++++-------\n> > 1 file changed, 37 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > index 4df9934..5e55954 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> > @@ -166,11 +166,8 @@ void Alsc::Initialise()\n> > RPI_LOG(\"Alsc\");\n> > frame_count2_ = frame_count_ = frame_phase_ = 0;\n> > first_time_ = true;\n> > - // Initialise the lambdas. Each call to Process then restarts from the\n> > - // previous results. Also initialise the previous frame tables to the\n> > - // same harmless values.\n> > - for (int i = 0; i < XY; i++)\n> > - lambda_r_[i] = lambda_b_[i] = 1.0;\n> > + ct_ = config_.default_ct;\n> > + // The lambdas are initialised in the SwitchMode.\n> > }\n> >\n> > void Alsc::waitForAysncThread()\n> > @@ -185,31 +182,53 @@ void Alsc::waitForAysncThread()\n> > }\n> > }\n> >\n> > +static bool compare_modes(CameraMode const &cm0, CameraMode const &cm1)\n> > +{\n> > + // Return true if the modes crop from the sensor significantly differently.\n> > + int left_diff = fabs(cm0.crop_x - cm1.crop_x);\n> > + int top_diff = fabs(cm0.crop_y - cm1.crop_y);\n> > + int right_diff = fabs(cm0.crop_x + cm0.scale_x * cm0.width -\n> > + cm1.crop_x - cm1.scale_x * cm1.width);\n> > + int bottom_diff = fabs(cm0.crop_y + cm0.scale_y * cm0.height -\n> > + cm1.crop_y - cm1.scale_y * cm1.height);\n> > + // These thresholds are a rather arbitrary amount chosen to trigger\n> > + // when carrying on with the previously calculated tables might be\n> > + // worse than regenerating them (but without the adaptive algorithm).\n> > + int threshold_x = cm0.sensor_width >> 4;\n> > + int threshold_y = cm0.sensor_height >> 4;\n> > + return left_diff > threshold_x || right_diff > threshold_x ||\n> > + top_diff > threshold_y || bottom_diff > threshold_y;\n> > +}\n> > +\n> > void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > {\n> > (void)metadata;\n> >\n> > + // We're going to start over with the tables if there's any \"significant\"\n> > + // change.\n> > + bool reset_tables = first_time_ || compare_modes(camera_mode_, camera_mode);\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. 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> > - // temperature. This echoes the code in doAlsc, without the\n> > - // adaptive algorithm.\n> > + if (reset_tables) {\n> > + // Upon every \"table reset\", arrange for something sensible to be\n> > + // generated. Construct the tables for the previous recorded colour\n> > + // temperature. In order to start over from scratch we initialise\n> > + // the lambdas, but the rest of this code then echoes the code in\n> > + // doAlsc, without the adaptive algorithm.\n> > + for (int i = 0; i < XY; i++)\n> > + lambda_r_[i] = lambda_b_[i] = 1.0;\n> > double cal_table_r[XY], cal_table_b[XY], cal_table_tmp[XY];\n> > - get_cal_table(4000, config_.calibrations_Cr, cal_table_tmp);\n> > + get_cal_table(ct_, config_.calibrations_Cr, cal_table_tmp);\n> > resample_cal_table(cal_table_tmp, camera_mode_, cal_table_r);\n> > - get_cal_table(4000, config_.calibrations_Cb, cal_table_tmp);\n> > + get_cal_table(ct_, config_.calibrations_Cb, cal_table_tmp);\n> > resample_cal_table(cal_table_tmp, camera_mode_, cal_table_b);\n> > compensate_lambdas_for_cal(cal_table_r, lambda_r_,\n> > async_lambda_r_);\n> > @@ -220,6 +239,7 @@ void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > config_.luminance_strength);\n> > memcpy(prev_sync_results_, sync_results_,\n> > sizeof(prev_sync_results_));\n> > + frame_phase_ = config_.frame_period; // run the algo again asap\n> > first_time_ = false;\n> > }\n> > }\n> > @@ -265,8 +285,8 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)\n> > {\n> > RPI_LOG(\"Starting ALSC thread\");\n> > // Get the current colour temperature. It's all we need from the\n> > - // metadata.\n> > - ct_ = get_ct(image_metadata, config_.default_ct);\n> > + // metadata. Default to the last CT value (which could be the default).\n> > + ct_ = get_ct(image_metadata, ct_);\n> > // We have to copy the statistics here, dividing out our best guess of\n> > // the LSC table that the pipeline applied to them.\n> > AlscStatus alsc_status;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4DB35BD87A\n\tfor ;\n\tWed, 5 Aug 2020 15:01:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5F8260599;\n\tWed, 5 Aug 2020 17:01:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22B2F60392\n\tfor ;\n\tWed, 5 Aug 2020 17:01:49 +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 397E52C0;\n\tWed, 5 Aug 2020 17:01:43 +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=\"vRrsPpYt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596639703;\n\tbh=aZJHj1u79GKb4nG1aCeIVhqLDDPmLC74XXdsft/AIbM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vRrsPpYtPhg6pXkE1V7+4hVNCFAjjPZFPiujwv+pehvvGTaOc9j+KjIj/18wQ8VoV\n\tgOr+r0qn/AvnYOIYXJ2Kwhx5IKqeVOF+BHJxjbQjYezcfN7OgeQy8CwLYglnCR1RK0\n\tPPVezGvC3+Hs0GhvDzMxm6egCkJSGCbH8mE+3Hvs=","Date":"Wed, 5 Aug 2020 18:01:31 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200805150131.GJ6751@pendragon.ideasonboard.com>","References":"<20200801080151.4282-1-david.plowman@raspberrypi.com>\n\t<20200801080151.4282-5-david.plowman@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: ipa: raspberrypi:\n\tALSC: Improve behaviour when camera mode changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]