[{"id":27675,"web_url":"https://patchwork.libcamera.org/comment/27675/","msgid":"","date":"2023-08-21T09:54:33","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","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 your work.\n\nOn Fri, 28 Jul 2023 at 14:37, David Plowman via libcamera-devel\n wrote:\n>\n> The Agc::process() function returns an AgcStatus object in the\n> metadata as before, but Agc::prepare() is changed to return the values\n> it computes in a separate AgcPrepareStatus object (under the new tag\n> \"agc.prepare_status\").\n>\n> The \"digitalGain\" and \"locked\" fields are moved from AgcStatus to\n> AgcPrepareStatus.\n>\n> This will be useful going forward as we can be more flexible about the\n> order in which prepare() and process() are called, without them\n> trampling on each other's results.\n>\n> Signed-off-by: David Plowman \n\nReviewed-by: Naushir Patuck \n\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 8 ++++----\n> src/ipa/rpi/controller/agc_status.h | 9 +++++++--\n> src/ipa/rpi/controller/rpi/agc.cpp | 20 +++++++++++---------\n> src/ipa/rpi/controller/rpi/agc.h | 2 +-\n> src/ipa/rpi/vc4/vc4.cpp | 6 +++---\n> 5 files changed, 26 insertions(+), 19 deletions(-)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index f40f2e71..6ae84cc6 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -1151,10 +1151,10 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> libcameraMetadata_.set(controls::LensPosition, *deviceStatus->lensPosition);\n> }\n>\n> - AgcStatus *agcStatus = rpiMetadata.getLocked(\"agc.status\");\n> - if (agcStatus) {\n> - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> - libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);\n> + AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> + if (agcPrepareStatus) {\n> + libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);\n> + libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);\n> }\n>\n> LuxStatus *luxStatus = rpiMetadata.getLocked(\"lux.status\");\n> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h\n> index 6c112e76..597eddd7 100644\n> --- a/src/ipa/rpi/controller/agc_status.h\n> +++ b/src/ipa/rpi/controller/agc_status.h\n> @@ -11,8 +11,10 @@\n> #include \n>\n> /*\n> - * The AGC algorithm should post the following structure into the image's\n> - * \"agc.status\" metadata.\n> + * The AGC algorithm process method should post an AgcStatus into the image\n> + * metadata under the tag \"agc.status\".\n> + * The AGC algorithm prepare method should post an AgcPrepareStatus instead\n> + * under \"agc.prepare_status\".\n> */\n>\n> /*\n> @@ -34,6 +36,9 @@ struct AgcStatus {\n> int floatingRegionEnable;\n> libcamera::utils::Duration fixedShutter;\n> double fixedAnalogueGain;\n> +};\n> +\n> +struct AgcPrepareStatus {\n> double digitalGain;\n> int locked;\n> };\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index 6087fc60..7b02972a 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -419,11 +419,13 @@ void Agc::prepare(Metadata *imageMetadata)\n> {\n> Duration totalExposureValue = status_.totalExposureValue;\n> AgcStatus delayedStatus;\n> + AgcPrepareStatus prepareStatus;\n>\n> if (!imageMetadata->get(\"agc.delayed_status\", delayedStatus))\n> totalExposureValue = delayedStatus.totalExposureValue;\n>\n> - status_.digitalGain = 1.0;\n> + prepareStatus.digitalGain = 1.0;\n> + prepareStatus.locked = false;\n>\n> if (status_.totalExposureValue) {\n> /* Process has run, so we have meaningful values. */\n> @@ -432,23 +434,23 @@ void Agc::prepare(Metadata *imageMetadata)\n> Duration actualExposure = deviceStatus.shutterSpeed *\n> deviceStatus.analogueGain;\n> if (actualExposure) {\n> - status_.digitalGain = totalExposureValue / actualExposure;\n> + double digitalGain = totalExposureValue / actualExposure;\n> LOG(RPiAgc, Debug) << \"Want total exposure \" << totalExposureValue;\n> /*\n> * Never ask for a gain < 1.0, and also impose\n> * some upper limit. Make it customisable?\n> */\n> - status_.digitalGain = std::max(1.0, std::min(status_.digitalGain, 4.0));\n> + prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, 4.0));\n> LOG(RPiAgc, Debug) << \"Actual exposure \" << actualExposure;\n> - LOG(RPiAgc, Debug) << \"Use digitalGain \" << status_.digitalGain;\n> + LOG(RPiAgc, Debug) << \"Use digitalGain \" << prepareStatus.digitalGain;\n> LOG(RPiAgc, Debug) << \"Effective exposure \"\n> - << actualExposure * status_.digitalGain;\n> + << actualExposure * prepareStatus.digitalGain;\n> /* Decide whether AEC/AGC has converged. */\n> - updateLockStatus(deviceStatus);\n> + prepareStatus.locked = updateLockStatus(deviceStatus);\n> }\n> } else\n> LOG(RPiAgc, Warning) << name() << \": no device metadata\";\n> - imageMetadata->set(\"agc.status\", status_);\n> + imageMetadata->set(\"agc.prepare_status\", prepareStatus);\n> }\n> }\n>\n> @@ -486,7 +488,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> writeAndFinish(imageMetadata, desaturate);\n> }\n>\n> -void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> +bool Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> {\n> const double errorFactor = 0.10; /* make these customisable? */\n> const int maxLockCount = 5;\n> @@ -522,7 +524,7 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> lastTargetExposure_ = status_.targetExposureValue;\n>\n> LOG(RPiAgc, Debug) << \"Lock count updated to \" << lockCount_;\n> - status_.locked = lockCount_ == maxLockCount;\n> + return lockCount_ == maxLockCount;\n> }\n>\n> void Agc::housekeepConfig()\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index b7122de3..aaf77c8f 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -85,7 +85,7 @@ public:\n> void process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n>\n> private:\n> - void updateLockStatus(DeviceStatus const &deviceStatus);\n> + bool updateLockStatus(DeviceStatus const &deviceStatus);\n> AgcConfig config_;\n> void housekeepConfig();\n> void fetchCurrentExposure(Metadata *imageMetadata);\n> diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> index 3eea40a6..1de0d3cc 100644\n> --- a/src/ipa/rpi/vc4/vc4.cpp\n> +++ b/src/ipa/rpi/vc4/vc4.cpp\n> @@ -60,7 +60,7 @@ private:\n> bool validateIspControls();\n>\n> void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> - void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> + void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);\n> void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n> void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> @@ -142,7 +142,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> if (ccmStatus)\n> applyCCM(ccmStatus, ctrls);\n>\n> - AgcStatus *dgStatus = rpiMetadata.getLocked(\"agc.status\");\n> + AgcPrepareStatus *dgStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> if (dgStatus)\n> applyDG(dgStatus, ctrls);\n>\n> @@ -284,7 +284,7 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> static_cast(awbStatus->gainB * 1000));\n> }\n>\n> -void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)\n> {\n> ctrls.set(V4L2_CID_DIGITAL_GAIN,\n> static_cast(dgStatus->digitalGain * 1000));\n> --\n> 2.30.2\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 3A601BDE17\n\tfor ;\n\tMon, 21 Aug 2023 09:54:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E5A1627DA;\n\tMon, 21 Aug 2023 11:54:25 +0200 (CEST)","from mail-yw1-x112a.google.com (mail-yw1-x112a.google.com\n\t[IPv6:2607:f8b0:4864:20::112a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6B7060379\n\tfor ;\n\tMon, 21 Aug 2023 11:54:22 +0200 (CEST)","by mail-yw1-x112a.google.com with SMTP id\n\t00721157ae682-58fc4d319d2so17658437b3.1\n\tfor ;\n\tMon, 21 Aug 2023 02:54:22 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692611665;\n\tbh=OrLNhGvN8j8ApLV1HDjTHDKqMpmE4wUXfGQZdZsMFcA=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YjUcyZZ6Xe+L1VLbgF0lDkLUDuncgbciMEAZQcM/aeJosdA/EH+/l3OWYe2mHsJDk\n\taNxCo//BDFNA7WmCtNCj5MijB2Hu3g5xnQUDISoQiNWKKY6r/tdsXNWoO35Rt0eVld\n\t3d6PSm30k6ztH2Oi3ocmU2B8wjFbey3qvr6MJLAjGkU10xwJpMBy0t4jvByZzS9Z5m\n\tMuK/tHBAX8SwD7Nw17JzlICr/7iR2/xXpOz9E1TPQ0w8uK1dXDX2itdyJMBt03N82a\n\tsKy9S0YAhU2lXdV3QQGIHIPWK+7v0l/uVoPFB6BXGUoO5v3Eurgtywor2QBcpVOVVm\n\tQn2ebgxLZLCvA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692611661; x=1693216461;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=JMyXFAng4RfzZcNa/qQW5uVrO+jn5XZmSn8pvAEc6sw=;\n\tb=OE8Ayu4KUjJ7i/AN5TzGgNmypqa4T5//e1RHkrfyFUdO1F32MVGYcOCnNnnEcWqnVK\n\tmRFfXqLN3cFLdbZsHWw3Uz5RAFgIjG9KjAZO7l7RtsnwHOYyGAA5RfJmmhFXG1DlZZfj\n\thPo8JaZ/beixtFASBkADQhK0PAR8KTbPD+fA69wp91uu8otJcc3JYcDHhtYD+ohi1ZE3\n\tRXRlUttyHhN0rPhCOeYhOh5K9I8dk/uxuHLLLu32WJh01xHu12HE82oxMBdqsGVw9HDr\n\tJi4FAzRM/07OERIakFtCF0Wib/GRdoIfStq2oioIHAh0QG/w0v489+0NzSNW1XNWBbTs\n\tjWOw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"OE8Ayu4K\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692611662; x=1693216462;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=JMyXFAng4RfzZcNa/qQW5uVrO+jn5XZmSn8pvAEc6sw=;\n\tb=T4Am4ttztOeRAJ2LkeZZ8dVz4JkZcfX9Lq4WuI5BxUSFEQ5Ige3Jc2caNQpB35SXJ6\n\t52yvd7qcTmErLENqCrYqZ5mFTANUfqcWWzmneCBLdkTc5DbPj6Z4hLValbURqyiYQFX3\n\tdMrkkzUF3d+PaR3gqGO7QxGbd2FCBs3MZop6quMKMFdwp66kHZtDC1UMpo1NKUKD4/mE\n\tif0LXZv5GoavYl9rt4hDVsKU3Bl2USlR3hJSBtMzWgwQI4ZjMaRsCBc70evRziRicmgI\n\twKqlDaC5p8tlM9/iTkk6HbuIhI4zquLurGEu5v2uFWmBrFp4fhl71D6Otsy6XAiSwjEp\n\tdF/w==","X-Gm-Message-State":"AOJu0YzOWbzTWZFdMBOLaoKLYI21m+jTcn/mcwHLDbyYedFOgDf94Dyq\n\tncyc9ZRz3ZQGAcexi8aGuG0tctlPhu9WK408sV/5PTr3WCyLa3n8YIc=","X-Google-Smtp-Source":"AGHT+IEClpitdHDgCAdLRZoj2OHwxY7hH/5Czv4FTyG3qRA40ku4fgEn7bZkzVA4RNqDf9jvxVEf61hwgEGcNonrAXM=","X-Received":"by 2002:a81:8882:0:b0:55a:3ce9:dc3d with SMTP id\n\ty124-20020a818882000000b0055a3ce9dc3dmr6511108ywf.13.1692611661675;\n\tMon, 21 Aug 2023 02:54:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20230728133700.3713-1-david.plowman@raspberrypi.com>\n\t<20230728133700.3713-4-david.plowman@raspberrypi.com>","In-Reply-To":"<20230728133700.3713-4-david.plowman@raspberrypi.com>","Date":"Mon, 21 Aug 2023 10:54:33 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27707,"web_url":"https://patchwork.libcamera.org/comment/27707/","msgid":"","date":"2023-08-29T14:01:01","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Fri, Jul 28, 2023 at 02:37:00PM +0100, David Plowman via libcamera-devel wrote:\n> The Agc::process() function returns an AgcStatus object in the\n> metadata as before, but Agc::prepare() is changed to return the values\n> it computes in a separate AgcPrepareStatus object (under the new tag\n> \"agc.prepare_status\").\n>\n> The \"digitalGain\" and \"locked\" fields are moved from AgcStatus to\n> AgcPrepareStatus.\n>\n> This will be useful going forward as we can be more flexible about the\n> order in which prepare() and process() are called, without them\n> trampling on each other's results.\n\nI presume this will be useful when dealing with multiple AGC channels ?\n\n>\n> Signed-off-by: David Plowman \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 8 ++++----\n> src/ipa/rpi/controller/agc_status.h | 9 +++++++--\n> src/ipa/rpi/controller/rpi/agc.cpp | 20 +++++++++++---------\n> src/ipa/rpi/controller/rpi/agc.h | 2 +-\n> src/ipa/rpi/vc4/vc4.cpp | 6 +++---\n> 5 files changed, 26 insertions(+), 19 deletions(-)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index f40f2e71..6ae84cc6 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -1151,10 +1151,10 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> \t\t\tlibcameraMetadata_.set(controls::LensPosition, *deviceStatus->lensPosition);\n> \t}\n>\n> -\tAgcStatus *agcStatus = rpiMetadata.getLocked(\"agc.status\");\n> -\tif (agcStatus) {\n> -\t\tlibcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> -\t\tlibcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);\n> +\tAgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> +\tif (agcPrepareStatus) {\n> +\t\tlibcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);\n> +\t\tlibcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);\n> \t}\n>\n> \tLuxStatus *luxStatus = rpiMetadata.getLocked(\"lux.status\");\n> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h\n> index 6c112e76..597eddd7 100644\n> --- a/src/ipa/rpi/controller/agc_status.h\n> +++ b/src/ipa/rpi/controller/agc_status.h\n> @@ -11,8 +11,10 @@\n> #include \n>\n> /*\n> - * The AGC algorithm should post the following structure into the image's\n> - * \"agc.status\" metadata.\n> + * The AGC algorithm process method should post an AgcStatus into the image\n> + * metadata under the tag \"agc.status\".\n> + * The AGC algorithm prepare method should post an AgcPrepareStatus instead\n> + * under \"agc.prepare_status\".\n> */\n>\n> /*\n> @@ -34,6 +36,9 @@ struct AgcStatus {\n> \tint floatingRegionEnable;\n> \tlibcamera::utils::Duration fixedShutter;\n> \tdouble fixedAnalogueGain;\n> +};\n> +\n> +struct AgcPrepareStatus {\n> \tdouble digitalGain;\n> \tint locked;\n> };\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index 6087fc60..7b02972a 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -419,11 +419,13 @@ void Agc::prepare(Metadata *imageMetadata)\n> {\n> \tDuration totalExposureValue = status_.totalExposureValue;\n> \tAgcStatus delayedStatus;\n> +\tAgcPrepareStatus prepareStatus;\n>\n> \tif (!imageMetadata->get(\"agc.delayed_status\", delayedStatus))\n> \t\ttotalExposureValue = delayedStatus.totalExposureValue;\n>\n> -\tstatus_.digitalGain = 1.0;\n> +\tprepareStatus.digitalGain = 1.0;\n> +\tprepareStatus.locked = false;\n>\n> \tif (status_.totalExposureValue) {\n> \t\t/* Process has run, so we have meaningful values. */\n> @@ -432,23 +434,23 @@ void Agc::prepare(Metadata *imageMetadata)\n> \t\t\tDuration actualExposure = deviceStatus.shutterSpeed *\n> \t\t\t\t\t\t deviceStatus.analogueGain;\n> \t\t\tif (actualExposure) {\n> -\t\t\t\tstatus_.digitalGain = totalExposureValue / actualExposure;\n> +\t\t\t\tdouble digitalGain = totalExposureValue / actualExposure;\n> \t\t\t\tLOG(RPiAgc, Debug) << \"Want total exposure \" << totalExposureValue;\n> \t\t\t\t/*\n> \t\t\t\t * Never ask for a gain < 1.0, and also impose\n> \t\t\t\t * some upper limit. Make it customisable?\n> \t\t\t\t */\n> -\t\t\t\tstatus_.digitalGain = std::max(1.0, std::min(status_.digitalGain, 4.0));\n> +\t\t\t\tprepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, 4.0));\n> \t\t\t\tLOG(RPiAgc, Debug) << \"Actual exposure \" << actualExposure;\n> -\t\t\t\tLOG(RPiAgc, Debug) << \"Use digitalGain \" << status_.digitalGain;\n> +\t\t\t\tLOG(RPiAgc, Debug) << \"Use digitalGain \" << prepareStatus.digitalGain;\n> \t\t\t\tLOG(RPiAgc, Debug) << \"Effective exposure \"\n> -\t\t\t\t\t\t << actualExposure * status_.digitalGain;\n> +\t\t\t\t\t\t << actualExposure * prepareStatus.digitalGain;\n> \t\t\t\t/* Decide whether AEC/AGC has converged. */\n> -\t\t\t\tupdateLockStatus(deviceStatus);\n> +\t\t\t\tprepareStatus.locked = updateLockStatus(deviceStatus);\n> \t\t\t}\n> \t\t} else\n> \t\t\tLOG(RPiAgc, Warning) << name() << \": no device metadata\";\n> -\t\timageMetadata->set(\"agc.status\", status_);\n> +\t\timageMetadata->set(\"agc.prepare_status\", prepareStatus);\n> \t}\n> }\n>\n> @@ -486,7 +488,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> \twriteAndFinish(imageMetadata, desaturate);\n> }\n>\n> -void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> +bool Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> {\n> \tconst double errorFactor = 0.10; /* make these customisable? */\n> \tconst int maxLockCount = 5;\n> @@ -522,7 +524,7 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> \tlastTargetExposure_ = status_.targetExposureValue;\n>\n> \tLOG(RPiAgc, Debug) << \"Lock count updated to \" << lockCount_;\n> -\tstatus_.locked = lockCount_ == maxLockCount;\n> +\treturn lockCount_ == maxLockCount;\n> }\n>\n> void Agc::housekeepConfig()\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index b7122de3..aaf77c8f 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -85,7 +85,7 @@ public:\n> \tvoid process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n>\n> private:\n> -\tvoid updateLockStatus(DeviceStatus const &deviceStatus);\n> +\tbool updateLockStatus(DeviceStatus const &deviceStatus);\n> \tAgcConfig config_;\n> \tvoid housekeepConfig();\n> \tvoid fetchCurrentExposure(Metadata *imageMetadata);\n> diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> index 3eea40a6..1de0d3cc 100644\n> --- a/src/ipa/rpi/vc4/vc4.cpp\n> +++ b/src/ipa/rpi/vc4/vc4.cpp\n> @@ -60,7 +60,7 @@ private:\n> \tbool validateIspControls();\n>\n> \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> -\tvoid applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> +\tvoid applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);\n> \tvoid applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n> \tvoid applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> \tvoid applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> @@ -142,7 +142,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> \tif (ccmStatus)\n> \t\tapplyCCM(ccmStatus, ctrls);\n>\n> -\tAgcStatus *dgStatus = rpiMetadata.getLocked(\"agc.status\");\n> +\tAgcPrepareStatus *dgStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> \tif (dgStatus)\n> \t\tapplyDG(dgStatus, ctrls);\n>\n> @@ -284,7 +284,7 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> \t\t static_cast(awbStatus->gainB * 1000));\n> }\n>\n> -void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)\n> {\n> \tctrls.set(V4L2_CID_DIGITAL_GAIN,\n> \t\t static_cast(dgStatus->digitalGain * 1000));\n> --\n> 2.30.2\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 260BBC0F1B\n\tfor ;\n\tTue, 29 Aug 2023 14:01:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CF4161F19;\n\tTue, 29 Aug 2023 16:01:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CB5961E02\n\tfor ;\n\tTue, 29 Aug 2023 16:01:05 +0200 (CEST)","from ideasonboard.com (mob-5-90-48-16.net.vodafone.it [5.90.48.16])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 092758CC;\n\tTue, 29 Aug 2023 15:59:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693317667;\n\tbh=i/h8n//isLq5XjrDbT0yZEM0gfnEIt4U82SVpjYOLw0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=XsSsa6QF+rOlCJIQ0iNVFPicIruw7iJBsj8FMWBvvntryLi5oPX8w/p2q5h92+g75\n\thvxdSLxYoxj7Kxew7kXiPpSOqG1VndGDPC/ll9zvQXiRzxtchG5OVQm7cF5FEpv1BC\n\tgQ+igq5LSIa5HyxQ00PPHIm4exp3CzUM2LkiZIlsyN2Pxizf4p9HNK2Bl46ATW8BHc\n\tHsKdCnfR2RkNMvHNlA96vocnHOjNAqs3trQa8tyBJnjFc3QlHZ3QCc4G+6Lc4bRQbD\n\tYCyh/3bnj/FBOZ0iKEvPFQ38RbRF9QjwK0RO6xU3GqTYR8buAOV9AzKogWJARnICgu\n\t3ZFME7B8EIs9Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693317583;\n\tbh=i/h8n//isLq5XjrDbT0yZEM0gfnEIt4U82SVpjYOLw0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=beqWRBmd5UWEynUQ3WeG6QIbVU8Ghh8PrrIBmKLVWU6JXbHSiLJaDtbtiwVA09QT8\n\tCOrWbQCPNvdZ0JxF83CBm/xS0KcefadHBK88rGWdtNWKauq3CUvgMU9SN4pIRwfcGq\n\tgOE6pMV2/OO/2sb569UTU4bWnyfzeqaZla+cSlLs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"beqWRBmd\"; dkim-atps=neutral","Date":"Tue, 29 Aug 2023 16:01:01 +0200","To":"David Plowman ","Message-ID":"","References":"<20230728133700.3713-1-david.plowman@raspberrypi.com>\n\t<20230728133700.3713-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230728133700.3713-4-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27748,"web_url":"https://patchwork.libcamera.org/comment/27748/","msgid":"","date":"2023-09-11T09:21:03","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for reviewing this series!\n\nOn Tue, 29 Aug 2023 at 15:01, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Fri, Jul 28, 2023 at 02:37:00PM +0100, David Plowman via libcamera-devel wrote:\n> > The Agc::process() function returns an AgcStatus object in the\n> > metadata as before, but Agc::prepare() is changed to return the values\n> > it computes in a separate AgcPrepareStatus object (under the new tag\n> > \"agc.prepare_status\").\n> >\n> > The \"digitalGain\" and \"locked\" fields are moved from AgcStatus to\n> > AgcPrepareStatus.\n> >\n> > This will be useful going forward as we can be more flexible about the\n> > order in which prepare() and process() are called, without them\n> > trampling on each other's results.\n>\n> I presume this will be useful when dealing with multiple AGC channels ?\n\nYes, exactly. The problem is because we generate new AE values for\neach channel in turn, in a round-robin fashion. And this channel is\nnot the same channel as the frame that's just arrived. So it becomes\nimportant not to muddle together information that actually relates to\ndifferent channels.\n\n>\n> >\n> > Signed-off-by: David Plowman \n>\n> Reviewed-by: Jacopo Mondi \n\nI'll probably repost this set with your review tags, so thanks for adding them!\n\nDavid\n\n>\n> Thanks\n> j\n>\n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 8 ++++----\n> > src/ipa/rpi/controller/agc_status.h | 9 +++++++--\n> > src/ipa/rpi/controller/rpi/agc.cpp | 20 +++++++++++---------\n> > src/ipa/rpi/controller/rpi/agc.h | 2 +-\n> > src/ipa/rpi/vc4/vc4.cpp | 6 +++---\n> > 5 files changed, 26 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index f40f2e71..6ae84cc6 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -1151,10 +1151,10 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> > libcameraMetadata_.set(controls::LensPosition, *deviceStatus->lensPosition);\n> > }\n> >\n> > - AgcStatus *agcStatus = rpiMetadata.getLocked(\"agc.status\");\n> > - if (agcStatus) {\n> > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> > - libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);\n> > + AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> > + if (agcPrepareStatus) {\n> > + libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);\n> > + libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);\n> > }\n> >\n> > LuxStatus *luxStatus = rpiMetadata.getLocked(\"lux.status\");\n> > diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h\n> > index 6c112e76..597eddd7 100644\n> > --- a/src/ipa/rpi/controller/agc_status.h\n> > +++ b/src/ipa/rpi/controller/agc_status.h\n> > @@ -11,8 +11,10 @@\n> > #include \n> >\n> > /*\n> > - * The AGC algorithm should post the following structure into the image's\n> > - * \"agc.status\" metadata.\n> > + * The AGC algorithm process method should post an AgcStatus into the image\n> > + * metadata under the tag \"agc.status\".\n> > + * The AGC algorithm prepare method should post an AgcPrepareStatus instead\n> > + * under \"agc.prepare_status\".\n> > */\n> >\n> > /*\n> > @@ -34,6 +36,9 @@ struct AgcStatus {\n> > int floatingRegionEnable;\n> > libcamera::utils::Duration fixedShutter;\n> > double fixedAnalogueGain;\n> > +};\n> > +\n> > +struct AgcPrepareStatus {\n> > double digitalGain;\n> > int locked;\n> > };\n> > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> > index 6087fc60..7b02972a 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> > @@ -419,11 +419,13 @@ void Agc::prepare(Metadata *imageMetadata)\n> > {\n> > Duration totalExposureValue = status_.totalExposureValue;\n> > AgcStatus delayedStatus;\n> > + AgcPrepareStatus prepareStatus;\n> >\n> > if (!imageMetadata->get(\"agc.delayed_status\", delayedStatus))\n> > totalExposureValue = delayedStatus.totalExposureValue;\n> >\n> > - status_.digitalGain = 1.0;\n> > + prepareStatus.digitalGain = 1.0;\n> > + prepareStatus.locked = false;\n> >\n> > if (status_.totalExposureValue) {\n> > /* Process has run, so we have meaningful values. */\n> > @@ -432,23 +434,23 @@ void Agc::prepare(Metadata *imageMetadata)\n> > Duration actualExposure = deviceStatus.shutterSpeed *\n> > deviceStatus.analogueGain;\n> > if (actualExposure) {\n> > - status_.digitalGain = totalExposureValue / actualExposure;\n> > + double digitalGain = totalExposureValue / actualExposure;\n> > LOG(RPiAgc, Debug) << \"Want total exposure \" << totalExposureValue;\n> > /*\n> > * Never ask for a gain < 1.0, and also impose\n> > * some upper limit. Make it customisable?\n> > */\n> > - status_.digitalGain = std::max(1.0, std::min(status_.digitalGain, 4.0));\n> > + prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, 4.0));\n> > LOG(RPiAgc, Debug) << \"Actual exposure \" << actualExposure;\n> > - LOG(RPiAgc, Debug) << \"Use digitalGain \" << status_.digitalGain;\n> > + LOG(RPiAgc, Debug) << \"Use digitalGain \" << prepareStatus.digitalGain;\n> > LOG(RPiAgc, Debug) << \"Effective exposure \"\n> > - << actualExposure * status_.digitalGain;\n> > + << actualExposure * prepareStatus.digitalGain;\n> > /* Decide whether AEC/AGC has converged. */\n> > - updateLockStatus(deviceStatus);\n> > + prepareStatus.locked = updateLockStatus(deviceStatus);\n> > }\n> > } else\n> > LOG(RPiAgc, Warning) << name() << \": no device metadata\";\n> > - imageMetadata->set(\"agc.status\", status_);\n> > + imageMetadata->set(\"agc.prepare_status\", prepareStatus);\n> > }\n> > }\n> >\n> > @@ -486,7 +488,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> > writeAndFinish(imageMetadata, desaturate);\n> > }\n> >\n> > -void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> > +bool Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> > {\n> > const double errorFactor = 0.10; /* make these customisable? */\n> > const int maxLockCount = 5;\n> > @@ -522,7 +524,7 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> > lastTargetExposure_ = status_.targetExposureValue;\n> >\n> > LOG(RPiAgc, Debug) << \"Lock count updated to \" << lockCount_;\n> > - status_.locked = lockCount_ == maxLockCount;\n> > + return lockCount_ == maxLockCount;\n> > }\n> >\n> > void Agc::housekeepConfig()\n> > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> > index b7122de3..aaf77c8f 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc.h\n> > +++ b/src/ipa/rpi/controller/rpi/agc.h\n> > @@ -85,7 +85,7 @@ public:\n> > void process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n> >\n> > private:\n> > - void updateLockStatus(DeviceStatus const &deviceStatus);\n> > + bool updateLockStatus(DeviceStatus const &deviceStatus);\n> > AgcConfig config_;\n> > void housekeepConfig();\n> > void fetchCurrentExposure(Metadata *imageMetadata);\n> > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> > index 3eea40a6..1de0d3cc 100644\n> > --- a/src/ipa/rpi/vc4/vc4.cpp\n> > +++ b/src/ipa/rpi/vc4/vc4.cpp\n> > @@ -60,7 +60,7 @@ private:\n> > bool validateIspControls();\n> >\n> > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> > - void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > + void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);\n> > void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n> > void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> > void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> > @@ -142,7 +142,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> > if (ccmStatus)\n> > applyCCM(ccmStatus, ctrls);\n> >\n> > - AgcStatus *dgStatus = rpiMetadata.getLocked(\"agc.status\");\n> > + AgcPrepareStatus *dgStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> > if (dgStatus)\n> > applyDG(dgStatus, ctrls);\n> >\n> > @@ -284,7 +284,7 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > static_cast(awbStatus->gainB * 1000));\n> > }\n> >\n> > -void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)\n> > {\n> > ctrls.set(V4L2_CID_DIGITAL_GAIN,\n> > static_cast(dgStatus->digitalGain * 1000));\n> > --\n> > 2.30.2\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 57D64BE080\n\tfor ;\n\tMon, 11 Sep 2023 09:21:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D848628EC;\n\tMon, 11 Sep 2023 11:21:17 +0200 (CEST)","from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com\n\t[IPv6:2607:f8b0:4864:20::f2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 799CE61DF3\n\tfor ;\n\tMon, 11 Sep 2023 11:21:15 +0200 (CEST)","by mail-qv1-xf2c.google.com with SMTP id\n\t6a1803df08f44-649a653479bso23774266d6.0\n\tfor ;\n\tMon, 11 Sep 2023 02:21:15 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694424077;\n\tbh=wyKwj0bgz9LxvBjlc03dX5ahyumEMpcyq2BlUvQCnVQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Qqj7/Sp2C3jBWsiMxf1jCwX/MerLMjPmxSe66PdQoJXhIJGZLMtBJKZ+qHgXnPEqe\n\tdnVXplxlViu09R1SABPpm3LnuuN8nvWJWN8pkqPqpTLcsss6JJtE0vfXTYSZ9TwvtC\n\t2rMbMr5gRHR62NQjCtOfVdfGKxaWGpSWJmTIa3ez+U1AnEKP/h3dKY926JtrckM3En\n\tIqYy8x9VlQg1USG3tnNx4m1dyZb9iNoMeZ1F/3ZrZ+kO8CtzWgpwK2Ui6EezEaUd9Y\n\tNfCOKSvDKQ51LAwc+Mteha6+o62Q3jieD92H/QBr04m0wLzewioOcMn2MZR6kNutTb\n\tZCB1Nd/KxiOdg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1694424074; x=1695028874;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=jQEWOeTUtbo3aMUqR0a13qBReClV7L0QKNKgjKSrA6M=;\n\tb=WMG2FEQPd7XNmRzt6AJRT1tcfGPmoarLHULyKNI+0niQQ0qhaqp4NW3a/1JFxSHClT\n\tbZBaWuLf0k/C7mbR+fsvNn0FaBL4mAZ3CylrsRlzSNM2NiKxyjFhb5uWCCTcTSZcy9DS\n\tV2tVpI0nYVjxc14AyjgAgm+zu1hs+6eoLXRgkWKYRqiOs4I2oW8jt9ixrbqomT+EdGYl\n\toEWDeCfLxkKKscpwE2ACUnA4FOVGSCmFfrxwBvLK7Q3Rk92VXlQhUWleFEzU/KGp+6gD\n\t5I6OJeaC6K0YrRqYdMf78gmvqwELHnFuHCYqA/errlMuH+GZb5d8gY6DWWHQsJ0wFAuG\n\to+9A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"WMG2FEQP\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1694424074; x=1695028874;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=jQEWOeTUtbo3aMUqR0a13qBReClV7L0QKNKgjKSrA6M=;\n\tb=SLYwPjRgi0ZnKhiMZ+yCXvqv7Trw+sdctan2KR0zSEWO0QAomPLZ/6VePDzYDGgEiZ\n\toaPN0s6tqYysU14QpoNXCRezLDsHEmnz9+WYRq0PKJt58OzouLN4HJT/JmUdIlTn3ZvV\n\tucLjfqpul/LcXPd0BuDJb5Itm23hUalt2n7k6XWHzhmUODqqk853XOzJibrPEjzJ7Rly\n\tij690ByGnjozYC6N22bzVflKBqOfp1Zl/0cCxHGYLbX6XkOQcetI/xV0lHZgV78ruhWk\n\tR9GzWHt6yDnVbbbVYPtqcf9jB7pJ/+YsPGSV242L87ao/JgvWP4i3GOoP43Ua2TvpeeL\n\tB6sg==","X-Gm-Message-State":"AOJu0YxMb0JIrhBxY4VxeSuDCmyM/TiUNRJ5DUMh0OVgbPKvFO1eHskJ\n\tiJ1S++gD0KDpMDqxVviaDHASrKeTj+7fiqq4sU/mdCL8sz144qcK","X-Google-Smtp-Source":"AGHT+IF7cWUhZ1iRJJ/Sze78aoclBI0fUANib+RaZDdfkDr7Z58xXdP4dKDjPCvfYduSg+Q2DNJSESdITW+Fon5lJK8=","X-Received":"by 2002:a0c:f34f:0:b0:64f:60ec:1105 with SMTP id\n\te15-20020a0cf34f000000b0064f60ec1105mr8349218qvm.36.1694424074225;\n\tMon, 11 Sep 2023 02:21:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20230728133700.3713-1-david.plowman@raspberrypi.com>\n\t<20230728133700.3713-4-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 11 Sep 2023 10:21:03 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27749,"web_url":"https://patchwork.libcamera.org/comment/27749/","msgid":"","date":"2023-09-11T09:25:37","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Ah, I see it's already been merged. All good then!\n\nDavid\n\nOn Mon, 11 Sept 2023 at 10:21, David Plowman\n wrote:\n>\n> Hi Jacopo\n>\n> Thanks for reviewing this series!\n>\n> On Tue, 29 Aug 2023 at 15:01, Jacopo Mondi\n> wrote:\n> >\n> > Hi David\n> >\n> > On Fri, Jul 28, 2023 at 02:37:00PM +0100, David Plowman via libcamera-devel wrote:\n> > > The Agc::process() function returns an AgcStatus object in the\n> > > metadata as before, but Agc::prepare() is changed to return the values\n> > > it computes in a separate AgcPrepareStatus object (under the new tag\n> > > \"agc.prepare_status\").\n> > >\n> > > The \"digitalGain\" and \"locked\" fields are moved from AgcStatus to\n> > > AgcPrepareStatus.\n> > >\n> > > This will be useful going forward as we can be more flexible about the\n> > > order in which prepare() and process() are called, without them\n> > > trampling on each other's results.\n> >\n> > I presume this will be useful when dealing with multiple AGC channels ?\n>\n> Yes, exactly. The problem is because we generate new AE values for\n> each channel in turn, in a round-robin fashion. And this channel is\n> not the same channel as the frame that's just arrived. So it becomes\n> important not to muddle together information that actually relates to\n> different channels.\n>\n> >\n> > >\n> > > Signed-off-by: David Plowman \n> >\n> > Reviewed-by: Jacopo Mondi \n>\n> I'll probably repost this set with your review tags, so thanks for adding them!\n>\n> David\n>\n> >\n> > Thanks\n> > j\n> >\n> > > ---\n> > > src/ipa/rpi/common/ipa_base.cpp | 8 ++++----\n> > > src/ipa/rpi/controller/agc_status.h | 9 +++++++--\n> > > src/ipa/rpi/controller/rpi/agc.cpp | 20 +++++++++++---------\n> > > src/ipa/rpi/controller/rpi/agc.h | 2 +-\n> > > src/ipa/rpi/vc4/vc4.cpp | 6 +++---\n> > > 5 files changed, 26 insertions(+), 19 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > index f40f2e71..6ae84cc6 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -1151,10 +1151,10 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> > > libcameraMetadata_.set(controls::LensPosition, *deviceStatus->lensPosition);\n> > > }\n> > >\n> > > - AgcStatus *agcStatus = rpiMetadata.getLocked(\"agc.status\");\n> > > - if (agcStatus) {\n> > > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> > > - libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);\n> > > + AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> > > + if (agcPrepareStatus) {\n> > > + libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);\n> > > + libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);\n> > > }\n> > >\n> > > LuxStatus *luxStatus = rpiMetadata.getLocked(\"lux.status\");\n> > > diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h\n> > > index 6c112e76..597eddd7 100644\n> > > --- a/src/ipa/rpi/controller/agc_status.h\n> > > +++ b/src/ipa/rpi/controller/agc_status.h\n> > > @@ -11,8 +11,10 @@\n> > > #include \n> > >\n> > > /*\n> > > - * The AGC algorithm should post the following structure into the image's\n> > > - * \"agc.status\" metadata.\n> > > + * The AGC algorithm process method should post an AgcStatus into the image\n> > > + * metadata under the tag \"agc.status\".\n> > > + * The AGC algorithm prepare method should post an AgcPrepareStatus instead\n> > > + * under \"agc.prepare_status\".\n> > > */\n> > >\n> > > /*\n> > > @@ -34,6 +36,9 @@ struct AgcStatus {\n> > > int floatingRegionEnable;\n> > > libcamera::utils::Duration fixedShutter;\n> > > double fixedAnalogueGain;\n> > > +};\n> > > +\n> > > +struct AgcPrepareStatus {\n> > > double digitalGain;\n> > > int locked;\n> > > };\n> > > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> > > index 6087fc60..7b02972a 100644\n> > > --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> > > @@ -419,11 +419,13 @@ void Agc::prepare(Metadata *imageMetadata)\n> > > {\n> > > Duration totalExposureValue = status_.totalExposureValue;\n> > > AgcStatus delayedStatus;\n> > > + AgcPrepareStatus prepareStatus;\n> > >\n> > > if (!imageMetadata->get(\"agc.delayed_status\", delayedStatus))\n> > > totalExposureValue = delayedStatus.totalExposureValue;\n> > >\n> > > - status_.digitalGain = 1.0;\n> > > + prepareStatus.digitalGain = 1.0;\n> > > + prepareStatus.locked = false;\n> > >\n> > > if (status_.totalExposureValue) {\n> > > /* Process has run, so we have meaningful values. */\n> > > @@ -432,23 +434,23 @@ void Agc::prepare(Metadata *imageMetadata)\n> > > Duration actualExposure = deviceStatus.shutterSpeed *\n> > > deviceStatus.analogueGain;\n> > > if (actualExposure) {\n> > > - status_.digitalGain = totalExposureValue / actualExposure;\n> > > + double digitalGain = totalExposureValue / actualExposure;\n> > > LOG(RPiAgc, Debug) << \"Want total exposure \" << totalExposureValue;\n> > > /*\n> > > * Never ask for a gain < 1.0, and also impose\n> > > * some upper limit. Make it customisable?\n> > > */\n> > > - status_.digitalGain = std::max(1.0, std::min(status_.digitalGain, 4.0));\n> > > + prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, 4.0));\n> > > LOG(RPiAgc, Debug) << \"Actual exposure \" << actualExposure;\n> > > - LOG(RPiAgc, Debug) << \"Use digitalGain \" << status_.digitalGain;\n> > > + LOG(RPiAgc, Debug) << \"Use digitalGain \" << prepareStatus.digitalGain;\n> > > LOG(RPiAgc, Debug) << \"Effective exposure \"\n> > > - << actualExposure * status_.digitalGain;\n> > > + << actualExposure * prepareStatus.digitalGain;\n> > > /* Decide whether AEC/AGC has converged. */\n> > > - updateLockStatus(deviceStatus);\n> > > + prepareStatus.locked = updateLockStatus(deviceStatus);\n> > > }\n> > > } else\n> > > LOG(RPiAgc, Warning) << name() << \": no device metadata\";\n> > > - imageMetadata->set(\"agc.status\", status_);\n> > > + imageMetadata->set(\"agc.prepare_status\", prepareStatus);\n> > > }\n> > > }\n> > >\n> > > @@ -486,7 +488,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> > > writeAndFinish(imageMetadata, desaturate);\n> > > }\n> > >\n> > > -void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> > > +bool Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> > > {\n> > > const double errorFactor = 0.10; /* make these customisable? */\n> > > const int maxLockCount = 5;\n> > > @@ -522,7 +524,7 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)\n> > > lastTargetExposure_ = status_.targetExposureValue;\n> > >\n> > > LOG(RPiAgc, Debug) << \"Lock count updated to \" << lockCount_;\n> > > - status_.locked = lockCount_ == maxLockCount;\n> > > + return lockCount_ == maxLockCount;\n> > > }\n> > >\n> > > void Agc::housekeepConfig()\n> > > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> > > index b7122de3..aaf77c8f 100644\n> > > --- a/src/ipa/rpi/controller/rpi/agc.h\n> > > +++ b/src/ipa/rpi/controller/rpi/agc.h\n> > > @@ -85,7 +85,7 @@ public:\n> > > void process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n> > >\n> > > private:\n> > > - void updateLockStatus(DeviceStatus const &deviceStatus);\n> > > + bool updateLockStatus(DeviceStatus const &deviceStatus);\n> > > AgcConfig config_;\n> > > void housekeepConfig();\n> > > void fetchCurrentExposure(Metadata *imageMetadata);\n> > > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> > > index 3eea40a6..1de0d3cc 100644\n> > > --- a/src/ipa/rpi/vc4/vc4.cpp\n> > > +++ b/src/ipa/rpi/vc4/vc4.cpp\n> > > @@ -60,7 +60,7 @@ private:\n> > > bool validateIspControls();\n> > >\n> > > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> > > - void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > > + void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);\n> > > void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n> > > void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> > > void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> > > @@ -142,7 +142,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> > > if (ccmStatus)\n> > > applyCCM(ccmStatus, ctrls);\n> > >\n> > > - AgcStatus *dgStatus = rpiMetadata.getLocked(\"agc.status\");\n> > > + AgcPrepareStatus *dgStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> > > if (dgStatus)\n> > > applyDG(dgStatus, ctrls);\n> > >\n> > > @@ -284,7 +284,7 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > > static_cast(awbStatus->gainB * 1000));\n> > > }\n> > >\n> > > -void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > > +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)\n> > > {\n> > > ctrls.set(V4L2_CID_DIGITAL_GAIN,\n> > > static_cast(dgStatus->digitalGain * 1000));\n> > > --\n> > > 2.30.2\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 49152BD160\n\tfor ;\n\tMon, 11 Sep 2023 09:25:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9537B628E7;\n\tMon, 11 Sep 2023 11:25:50 +0200 (CEST)","from mail-qk1-x730.google.com (mail-qk1-x730.google.com\n\t[IPv6:2607:f8b0:4864:20::730])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AD8061DF3\n\tfor ;\n\tMon, 11 Sep 2023 11:25:49 +0200 (CEST)","by mail-qk1-x730.google.com with SMTP id\n\taf79cd13be357-76f0807acb6so263247385a.1\n\tfor ;\n\tMon, 11 Sep 2023 02:25:49 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694424350;\n\tbh=yDc6fQBw5OxccHPUfztTwnsVddmtEioVEqbMquaCjCQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=r/IpzVH+t/8j/PYUypOTYlK71uUpHqatH9pBvLPNUvYXKJNdca8Ngo3e9QDBcGVOw\n\t3DJJ+RDtGQbq9Q7Dl6djpf+BAptLQwQbMGgUXsKbeEW8THWTIx4j6Psf/m18/8wsTP\n\tCYiHalwLy1d73rBHZlG7Z7VsicG8F8aY3PCYhQaG9zwb8MgI8+pY5UYRkcsnYoP1LS\n\tBXtfv1B5ab2i0dryBKMbO6XCY0JPWsSOcHYEcI51Tu+wZ6rrEQjfdGEZLQkxhtgAPw\n\ty8LBW/bczZKaFbHkoCkBSaiCITzkOCVukspbEggp7rgM2B4LSakjLV3oLs4VOoelYv\n\ti+gaiMMcOLhFw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1694424348; x=1695029148;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Anf0PedVrazlh62G5VQiIiEuI2OgCl8dOkpUQ9qe4bc=;\n\tb=EPnYzLzW3i1kLRFQhayJViAwCtlFoVGBM/TnZmt+drOKlUfC1Bzx2Q8aKmNs1EfZ8O\n\tEY496meTdWT2pzYOV4XsS6IO+ly/fUoYQnLqy3AigYIDaPJm58Odir3q3eYzozdfIW3T\n\t4Y0I98evA/hZTCd0GF7pOSHlEp9CCv6sGYm/u5yYvyz2K4+O8HrZqYr4JH9S+LKFJUl9\n\tMemFA9EgXHVzUHSalDd9yMVQl9fi93Ft5lHWmwYooQHHiqAw4tZRcx783dp9uEW9myw5\n\t5YhEgb5DADkrPmfbaCBzM+VjcpN5RYplX9Iu/Y/k9LaS0WCpjjfBDYJQP+JafzZK1dr4\n\t4QPQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EPnYzLzW\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1694424348; x=1695029148;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Anf0PedVrazlh62G5VQiIiEuI2OgCl8dOkpUQ9qe4bc=;\n\tb=uH8ZnJMXPfL6Ecd5AC280yOKQVFVWHQLgZmNMmU/4qs91odnG+G2y2HeSvKSD3hWs+\n\tZWNbySD9A5WWmbvpvOfjIiw9hDnApuD8bWXuZkYfFQbnrnshnP8xKcqojTAeHkvYEo4T\n\tly9dA14wYqGhT7Gm3VxZhj/ECh7kFxMJnWbhVhwzZdTxLlyJkMtDo9jp0dy2elCrHuyB\n\tz5TaPiioqFUjiwzrn5U4AiL8M4rQy4k8g/aryQ1OpmoBQdbfDYe7OCsX/glOLdf44nGP\n\tK+pwDfy4dVGrTm+GdUIOSvV8p8m8jJuzk/qTr74CS2LsU3qvWQToyUCoRfF0ibv8T36W\n\t0rKg==","X-Gm-Message-State":"AOJu0Yw+PPeSeKUKs4/zmFpj/PFtVyh9paJH8L1w2cG7FBawKNPdM6tN\n\tMujlOta71QXJBiPjF0u+9vKP9WZXJWWyBriYaDhzjg==","X-Google-Smtp-Source":"AGHT+IEgdd8H68/enmNbfkXuY8pAJelFjTDRL7YV/ihwFIMMEw9X98kp32YPP7Sf15SwptxD7elWKUY6FMRa6u7BN9A=","X-Received":"by 2002:ad4:57a2:0:b0:64f:4d8e:574e with SMTP id\n\tg2-20020ad457a2000000b0064f4d8e574emr8014138qvx.7.1694424348106;\n\tMon, 11 Sep 2023 02:25:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20230728133700.3713-1-david.plowman@raspberrypi.com>\n\t<20230728133700.3713-4-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","Date":"Mon, 11 Sep 2023 10:25:37 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus\n\tinto AgcStatus and AgcPrepareStatus","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]