[{"id":13895,"web_url":"https://patchwork.libcamera.org/comment/13895/","msgid":"","date":"2020-11-26T09:45:34","subject":"Re: [libcamera-devel] [PATCH 2/3] src: ipa: raspberrypi: Improve\n\tbehaviour when AE disabled","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 Wed, 25 Nov 2020 at 11:36, David Plowman \nwrote:\n\n> AE/AGC \"disabled\" is now implemented by leaving it running but fixing\n> the exposure/gain to the last values we requested. This behaves better\n> when, for example, a fixed shutter or gain is then specified - the\n> other value remains fixed and doesn't start floating again.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 48 ++++++++++++++++++-----------\n> 1 file changed, 30 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 9853a343..30516665 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -67,7 +67,8 @@ public:\n> IPARPi()\n> : lastMode_({}), controller_(), controllerInit_(false),\n> frameCount_(0), checkCount_(0), mistrustCount_(0),\n> - lsTable_(nullptr)\n> + lsTable_(nullptr),\n> + lastShutter_(0), lastGain_(0)\n> {\n> }\n>\n> @@ -145,6 +146,10 @@ private:\n> /* LS table allocation passed in from the pipeline handler. */\n> FileDescriptor lsTableHandle_;\n> void *lsTable_;\n> +\n> + /* For enabling/disabling the AEC/AGC algorithm. */\n> + uint32_t lastShutter_;\n> + float lastGain_;\n> };\n>\n> int IPARPi::init(const IPASettings &settings)\n> @@ -493,12 +498,22 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n>\n> switch (ctrl.first) {\n> case controls::AE_ENABLE: {\n> - RPiController::Algorithm *agc =\n> controller_.GetAlgorithm(\"agc\");\n> + RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> + controller_.GetAlgorithm(\"agc\"));\n> ASSERT(agc);\n> - if (ctrl.second.get() == false)\n> - agc->Pause();\n> - else\n> - agc->Resume();\n> +\n> + /*\n> + * We always run the AGC even if it's \"disabled\".\n> + * Both exposure and gain are \"fixed\" to the last\n> + * values it produced.\n> + */\n> + if (ctrl.second.get() == false) {\n> + agc->SetFixedShutter(lastShutter_);\n> + agc->SetFixedAnalogueGain(lastGain_);\n> + } else {\n> + agc->SetFixedShutter(0);\n> + agc->SetFixedAnalogueGain(0);\n> + }\n>\n\nI wonder if we could do this within the AGC controller itself? So we keep\nthe existing Pause()/Resume() logic in the IPA, and have the AGC do the\nabove block on the appropriate API call. This way the IPA would not need\nto track the last used exposure/gain values that are already stored within\nthe AGC block?\n\nThe rest of the changes would still be valid with this tweak.\n\nRegards,\nNaush\n\n\n\n>\n> libcameraMetadata_.set(controls::AeEnable,\n> ctrl.second.get());\n> break;\n> @@ -510,13 +525,10 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> ASSERT(agc);\n>\n> /* This expects units of micro-seconds. */\n> - agc->SetFixedShutter(ctrl.second.get());\n> + int32_t shutter = ctrl.second.get();\n> + agc->SetFixedShutter(shutter);\n>\n> - /* For the manual values to take effect, AGC must\n> be unpaused. */\n> - if (agc->IsPaused())\n> - agc->Resume();\n> -\n> - libcameraMetadata_.set(controls::ExposureTime,\n> ctrl.second.get());\n> + libcameraMetadata_.set(controls::ExposureTime,\n> shutter);\n> break;\n> }\n>\n> @@ -524,14 +536,11 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> controller_.GetAlgorithm(\"agc\"));\n> ASSERT(agc);\n> -\n> agc->SetFixedAnalogueGain(ctrl.second.get());\n>\n> - /* For the manual values to take effect, AGC must\n> be unpaused. */\n> - if (agc->IsPaused())\n> - agc->Resume();\n> + float gain = ctrl.second.get();\n> + agc->SetFixedAnalogueGain(gain);\n>\n> - libcameraMetadata_.set(controls::AnalogueGain,\n> - ctrl.second.get());\n> + libcameraMetadata_.set(controls::AnalogueGain,\n> gain);\n> break;\n> }\n>\n> @@ -861,6 +870,9 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n>\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> {\n> + lastShutter_ = agcStatus->shutter_time;\n> + lastGain_ = agcStatus->analogue_gain;\n> +\n> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> int32_t exposureLines =\n> helper_->ExposureLines(agcStatus->shutter_time);\n>\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\n>","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 45660BE176\n\tfor ;\n\tThu, 26 Nov 2020 09:45:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8C6F6343F;\n\tThu, 26 Nov 2020 10:45:52 +0100 (CET)","from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4669B60331\n\tfor ;\n\tThu, 26 Nov 2020 10:45:51 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id z21so1571698lfe.12\n\tfor ;\n\tThu, 26 Nov 2020 01:45:51 -0800 (PST)"],"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=\"pcPVW4UT\"; 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=tasf9gYBwaKLaWMRhM1MU8hty83SodARO6dgs3d8+xw=;\n\tb=pcPVW4UT1Cs6e4eQUmZVqAVzefg2aPf1XYHg0vBVE8EVpWbwlOCjgO72tocGBWaArz\n\ta9Oqa4PRNZ+sSEl+1M7nIqcM7o6N1t47MMRk+uq5A0Y6ruVSM8Q4NnbQmQiv/lOv3GSW\n\tKtyq0f2490t5iqddQ/RSZaji69KkJPhFbiGJDGtRIKcW1Uft7Mcg3dgSkJRsJYx7LFB1\n\tqJiFVSXCvfdKPZwOIdDsBv8XbeVFCWbMfO9ljQbOTZyBzmez2fIuyNlU+kloSjSZPkg9\n\tldepOmX5LPhVzwEOtuQPTbxxpu+k82oFnYr3Viv9mqXSj5N0hQWVMW40WwbjD07dvQfH\n\tPmFA==","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=tasf9gYBwaKLaWMRhM1MU8hty83SodARO6dgs3d8+xw=;\n\tb=ftqAzDJldvhDis/dV1xr3xY+fCUrzHOAcf0ecGulJIIYgXqGqW2NLWp08aHrwiMNHd\n\tF/Olo/dmhSTihn0YziTNJIMmv/G38z9x/soXe8xWRdAmIsmCyBzkUbftYvNZ0oUH2cn6\n\tyKQXOgJ+qxk4dEV7Kz9aW1H0iNN9Jsy2038FgGnMg0La33zXTZ3ME2EvKMEgEIVloV3Q\n\t79wcbyovE60nvjeIBFy4YlZh/S27FVvfBAXWOAyjoSiIdS2SnlbZFEHDzfHhy0pa18CG\n\tpCsc/yxJ1uewcG0gQmHf8E/L8VeD1bHBv+lG9HNFQGq+2hubhykTc68nRfoBypBmzCQ+\n\tdOkg==","X-Gm-Message-State":"AOAM531u3wGvQZHdDwxCr2fqETmfvjn+0WvS7uSqmJn8eNXtVYswuH+Z\n\tUheSLsw6fbxQ6MF40xWHc/Vx13YhjOYzSKnJr8PBhCFvcxU=","X-Google-Smtp-Source":"ABdhPJzgmB6RVCoC6UHdupm2osy48XIjRL4Jif7PQO4InZs/SEPcVF0u0JdOUtuUtN2VRjDT+kRmOtulsEtUY41+mMQ=","X-Received":"by 2002:a05:6512:210b:: with SMTP id\n\tq11mr1088917lfr.238.1606383950025; \n\tThu, 26 Nov 2020 01:45:50 -0800 (PST)","MIME-Version":"1.0","References":"<20201125113640.20246-1-david.plowman@raspberrypi.com>\n\t<20201125113640.20246-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20201125113640.20246-3-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 26 Nov 2020 09:45:34 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 2/3] src: ipa: raspberrypi: Improve\n\tbehaviour when AE disabled","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============6193086851734238051==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13899,"web_url":"https://patchwork.libcamera.org/comment/13899/","msgid":"","date":"2020-11-26T09:53:25","subject":"Re: [libcamera-devel] [PATCH 2/3] src: ipa: raspberrypi: Improve\n\tbehaviour when AE disabled","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the suggestion.\n\nOn Thu, 26 Nov 2020 at 09:45, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Wed, 25 Nov 2020 at 11:36, David Plowman wrote:\n>>\n>> AE/AGC \"disabled\" is now implemented by leaving it running but fixing\n>> the exposure/gain to the last values we requested. This behaves better\n>> when, for example, a fixed shutter or gain is then specified - the\n>> other value remains fixed and doesn't start floating again.\n>>\n>> Signed-off-by: David Plowman \n>> ---\n>> src/ipa/raspberrypi/raspberrypi.cpp | 48 ++++++++++++++++++-----------\n>> 1 file changed, 30 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 9853a343..30516665 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -67,7 +67,8 @@ public:\n>> IPARPi()\n>> : lastMode_({}), controller_(), controllerInit_(false),\n>> frameCount_(0), checkCount_(0), mistrustCount_(0),\n>> - lsTable_(nullptr)\n>> + lsTable_(nullptr),\n>> + lastShutter_(0), lastGain_(0)\n>> {\n>> }\n>>\n>> @@ -145,6 +146,10 @@ private:\n>> /* LS table allocation passed in from the pipeline handler. */\n>> FileDescriptor lsTableHandle_;\n>> void *lsTable_;\n>> +\n>> + /* For enabling/disabling the AEC/AGC algorithm. */\n>> + uint32_t lastShutter_;\n>> + float lastGain_;\n>> };\n>>\n>> int IPARPi::init(const IPASettings &settings)\n>> @@ -493,12 +498,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n>>\n>> switch (ctrl.first) {\n>> case controls::AE_ENABLE: {\n>> - RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n>> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n>> + controller_.GetAlgorithm(\"agc\"));\n>> ASSERT(agc);\n>> - if (ctrl.second.get() == false)\n>> - agc->Pause();\n>> - else\n>> - agc->Resume();\n>> +\n>> + /*\n>> + * We always run the AGC even if it's \"disabled\".\n>> + * Both exposure and gain are \"fixed\" to the last\n>> + * values it produced.\n>> + */\n>> + if (ctrl.second.get() == false) {\n>> + agc->SetFixedShutter(lastShutter_);\n>> + agc->SetFixedAnalogueGain(lastGain_);\n>> + } else {\n>> + agc->SetFixedShutter(0);\n>> + agc->SetFixedAnalogueGain(0);\n>> + }\n>\n>\n> I wonder if we could do this within the AGC controller itself? So we keep the existing Pause()/Resume() logic in the IPA, and have the AGC do the above block on the appropriate API call. This way the IPA would not need to track the last used exposure/gain values that are already stored within the AGC block?\n>\n> The rest of the changes would still be valid with this tweak.\n>\n> Regards,\n> Naush\n\nYes, good idea, that might look a bit tidier. Let me do a v2 set including this.\n\nBest regards\nDavid\n\n>\n>\n>>\n>>\n>> libcameraMetadata_.set(controls::AeEnable, ctrl.second.get());\n>> break;\n>> @@ -510,13 +525,10 @@ void IPARPi::queueRequest(const ControlList &controls)\n>> ASSERT(agc);\n>>\n>> /* This expects units of micro-seconds. */\n>> - agc->SetFixedShutter(ctrl.second.get());\n>> + int32_t shutter = ctrl.second.get();\n>> + agc->SetFixedShutter(shutter);\n>>\n>> - /* For the manual values to take effect, AGC must be unpaused. */\n>> - if (agc->IsPaused())\n>> - agc->Resume();\n>> -\n>> - libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get());\n>> + libcameraMetadata_.set(controls::ExposureTime, shutter);\n>> break;\n>> }\n>>\n>> @@ -524,14 +536,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n>> RPiController::AgcAlgorithm *agc = dynamic_cast(\n>> controller_.GetAlgorithm(\"agc\"));\n>> ASSERT(agc);\n>> - agc->SetFixedAnalogueGain(ctrl.second.get());\n>>\n>> - /* For the manual values to take effect, AGC must be unpaused. */\n>> - if (agc->IsPaused())\n>> - agc->Resume();\n>> + float gain = ctrl.second.get();\n>> + agc->SetFixedAnalogueGain(gain);\n>>\n>> - libcameraMetadata_.set(controls::AnalogueGain,\n>> - ctrl.second.get());\n>> + libcameraMetadata_.set(controls::AnalogueGain, gain);\n>> break;\n>> }\n>>\n>> @@ -861,6 +870,9 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>>\n>> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>> {\n>> + lastShutter_ = agcStatus->shutter_time;\n>> + lastGain_ = agcStatus->analogue_gain;\n>> +\n>> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n>> int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n>>\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 3F161BE176\n\tfor ;\n\tThu, 26 Nov 2020 09:53:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACB8B63442;\n\tThu, 26 Nov 2020 10:53:37 +0100 (CET)","from mail-ot1-x343.google.com (mail-ot1-x343.google.com\n\t[IPv6:2607:f8b0:4864:20::343])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C54616343D\n\tfor ;\n\tThu, 26 Nov 2020 10:53:36 +0100 (CET)","by mail-ot1-x343.google.com with SMTP id f16so1360128otl.11\n\tfor ;\n\tThu, 26 Nov 2020 01:53:36 -0800 (PST)"],"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=\"dBBz/cYC\"; 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:content-transfer-encoding;\n\tbh=n2gx9xVL77kTNg8klMYgLaIlu5YnM5nQDM0btK+5WOg=;\n\tb=dBBz/cYCEQG7MkBSbQXHwQawSgZQfReWh52tDS9a/for6cqm40MkMe9QqPgsbQku2m\n\t0J4IvGmK0KVyIYc9ybxi9zjpDgsTx1/veKwXYHGvuEL37gGnOGyIT1PUL7BPeuv3gvOi\n\tYjgsjOhM2dIJAt4CmNcT55QURTUH/J9CpHE118TkQle//GPMDJFszQweduvVwI9ctA0t\n\tjQKEFaBD8bOASeNAmMuHyHGoe63Y74gmmeN7JwWvLQ2AmX9Do0MqRi3DiiVQh7ZoodX3\n\tJ7vKXtuPYuRSHn84bkHWE7WR4nkpfUoWCu4dtkAWCM+WBfnhfaCNZBt/LgCoZYxqP15L\n\tYm9A==","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:content-transfer-encoding;\n\tbh=n2gx9xVL77kTNg8klMYgLaIlu5YnM5nQDM0btK+5WOg=;\n\tb=kQjWN1ZcZ6+5S/1dlZ3p1HtYkVaG9CfQddV4H4FxR5KQkeuO2yTFUFIb8XQnUn+tTl\n\tXf+7yQB9W6l4PHZyTLYi9g7ay3uPnTpXpSoeBPPAjuzaIjbPvqxdjyWVhQUTpEPgQkh4\n\tBrsho37Qxh9gVZEvvIJgaxIunL4x5RRXCefyn+4jOmFFIcUZIIuZnxtyLoevAZghLw0f\n\tDKqxHuqfVApxRsf9qbjc36mZHugABtyixgy7+6RjOtlib8FuuGULxNmb6egw6U5o7KOz\n\tuEawyEHOOb09H1CMa1XPOdYhTVn81H4TBXaDC5E8Y/eBEcC2oXdNY205eBQXTpzP+YEE\n\tfIpg==","X-Gm-Message-State":"AOAM533ZkjqbkThzVC7wnQtaGqikVhWwnY6GbSJgoWOoU5X27elfq3g4\n\tQouvIR1HQMyZTVf3Na9cvi+KQxG4GzuZtmX0zPceYg==","X-Google-Smtp-Source":"ABdhPJxRYSyxi7L3jNByc83wnJOtGadNQKvga4Fj3rTtGd0VUW12uA+KjZ1ftW0+rzBoXmk03nEEY8XKAFT0hLoSNcU=","X-Received":"by 2002:a05:6830:1ad0:: with SMTP id\n\tr16mr1726455otc.160.1606384415712; \n\tThu, 26 Nov 2020 01:53:35 -0800 (PST)","MIME-Version":"1.0","References":"<20201125113640.20246-1-david.plowman@raspberrypi.com>\n\t<20201125113640.20246-3-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Thu, 26 Nov 2020 09:53:25 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 2/3] src: ipa: raspberrypi: Improve\n\tbehaviour when AE disabled","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]