[{"id":32772,"web_url":"https://patchwork.libcamera.org/comment/32772/","msgid":"<20241216150353.GA2131@pendragon.ideasonboard.com>","date":"2024-12-16T15:03:53","subject":"Re: [PATCH v5 5/8] ipa: raspberry: Port to the new AEGC controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nAs this patch touches the Rapsberry Pi IPA, Naush, David, could you\nreview it ?\n\nOn Mon, Dec 16, 2024 at 01:39:51PM +0900, Paul Elder wrote:\n> From: Jacopo Mondi \n> \n> The newly introduced controls to drive the AEGC algorithm allow\n> to control the computation of the exposure time and analogue gain\n> separately.\n> \n> The RPi AEGC implementation already computes the shutter and gain\n> values separately but does not expose separate functions to control\n> them.\n> \n> Augment the AgcAlgorithm interface to allow pausing/resuming the shutter\n> and gain automatic computations separately and plumb them to the newly\n> introduced controls.\n> \n> Add safety checks to ignore ExposureTime and AnalogueGain values if the\n> algorithms are not paused, and report the correct AeState value by\n> checking if both algorithms have been paused or if they have converged.\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Paul Elder \n> Reviewed-by: Paul Elder \n> \n> ---\n> No change in v5\n> \n> Changes in v4:\n> - s/shutter/exposure/\n> \n> Changes in v3:\n> - recovered from 2-year-old bitrot\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 74 +++++++++++++++++++---\n> src/ipa/rpi/controller/agc_algorithm.h | 8 ++-\n> src/ipa/rpi/controller/rpi/agc.cpp | 52 +++++++++++++--\n> src/ipa/rpi/controller/rpi/agc.h | 8 ++-\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 24 ++++++-\n> src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> 6 files changed, 150 insertions(+), 24 deletions(-)\n> \n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 5fce17e67bd6..8924c3a52c26 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -55,8 +55,13 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> \n> /* List of controls handled by the Raspberry Pi IPA */\n> const ControlInfoMap::Map ipaControls{\n> -\t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t{ &controls::ExposureTimeMode,\n> +\t ControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> +\t\t static_cast(controls::ExposureTimeModeManual)) },\n> \t{ &controls::ExposureTime, ControlInfo(0, 66666) },\n> +\t{ &controls::AnalogueGainMode,\n> +\t ControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> +\t\t static_cast(controls::AnalogueGainModeManual)) },\n> \t{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> \t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> @@ -764,21 +769,22 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\t\t << \" = \" << ctrl.second.toString();\n> \n> \t\tswitch (ctrl.first) {\n> -\t\tcase controls::AE_ENABLE: {\n> +\t\tcase controls::EXPOSURE_TIME_MODE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> \t\t\tif (!agc) {\n> \t\t\t\tLOG(IPARPI, Warning)\n> -\t\t\t\t\t<< \"Could not set AE_ENABLE - no AGC algorithm\";\n> +\t\t\t\t\t<< \"Could not set EXPOSURE_TIME_MODE - no AGC algorithm\";\n> \t\t\t\tbreak;\n> \t\t\t}\n> \n> -\t\t\tif (ctrl.second.get() == false)\n> -\t\t\t\tagc->disableAuto();\n> +\t\t\tif (ctrl.second.get() == controls::ExposureTimeModeManual)\n> +\t\t\t\tagc->disableAutoExposure();\n> \t\t\telse\n> -\t\t\t\tagc->enableAuto();\n> +\t\t\t\tagc->enableAutoExposure();\n> \n> -\t\t\tlibcameraMetadata_.set(controls::AeEnable, ctrl.second.get());\n> +\t\t\tlibcameraMetadata_.set(controls::ExposureTimeMode,\n> +\t\t\t\t\t ctrl.second.get());\n> \t\t\tbreak;\n> \t\t}\n> \n> @@ -791,6 +797,13 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\t\tbreak;\n> \t\t\t}\n> \n> +\t\t\t/*\n> +\t\t\t * Ignore manual exposure time when the auto exposure\n> +\t\t\t * algorithm is running.\n> +\t\t\t */\n> +\t\t\tif (agc->autoExposureEnabled())\n> +\t\t\t\tbreak;\n> +\n> \t\t\t/* The control provides units of microseconds. */\n> \t\t\tagc->setFixedExposureTime(0, ctrl.second.get() * 1.0us);\n> \n> @@ -798,6 +811,25 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::ANALOGUE_GAIN_MODE: {\n> +\t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set ANALOGUE_GAIN_MODE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tif (ctrl.second.get() == controls::AnalogueGainModeManual)\n> +\t\t\t\tagc->disableAutoGain();\n> +\t\t\telse\n> +\t\t\t\tagc->enableAutoGain();\n> +\n> +\t\t\tlibcameraMetadata_.set(controls::AnalogueGainMode,\n> +\t\t\t\t\t ctrl.second.get());\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tcase controls::ANALOGUE_GAIN: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> @@ -807,6 +839,13 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\t\tbreak;\n> \t\t\t}\n> \n> +\t\t\t/*\n> +\t\t\t * Ignore manual analogue gain value when the auto gain\n> +\t\t\t * algorithm is running.\n> +\t\t\t */\n> +\t\t\tif (agc->autoGainEnabled())\n> +\t\t\t\tbreak;\n> +\n> \t\t\tagc->setFixedAnalogueGain(0, ctrl.second.get());\n> \n> \t\t\tlibcameraMetadata_.set(controls::AnalogueGain,\n> @@ -863,6 +902,13 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\t\tbreak;\n> \t\t\t}\n> \n> +\t\t\t/*\n> +\t\t\t * Ignore AE_EXPOSURE_MODE if the shutter or the gain\n> +\t\t\t * are in auto mode.\n> +\t\t\t */\n> +\t\t\tif (agc->autoExposureEnabled() || agc->autoGainEnabled())\n> +\t\t\t\tbreak;\n> +\n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (ExposureModeTable.count(idx)) {\n> \t\t\t\tagc->setExposureMode(ExposureModeTable.at(idx));\n> @@ -1323,9 +1369,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> \t}\n> \n> \tAgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> -\tif (agcPrepareStatus) {\n> -\t\tlibcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);\n> +\tif (agcPrepareStatus)\n> \t\tlibcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);\n> +\n> +\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\tcontroller_.getAlgorithm(\"agc\"));\n> +\tif (agc) {\n> +\t\tif (!agc->autoExposureEnabled() && !agc->autoGainEnabled())\n> +\t\t\tlibcameraMetadata_.set(controls::AeState, controls::AeStateIdle);\n> +\t\telse if (agcPrepareStatus)\n> +\t\t\tlibcameraMetadata_.set(controls::AeState,\n> +\t\t\t\t\t agcPrepareStatus->locked\n> +\t\t\t\t\t\t ? controls::AeStateConverged\n> +\t\t\t\t\t\t : controls::AeStateSearching);\n> \t}\n> \n> \tLuxStatus *luxStatus = rpiMetadata.getLocked(\"lux.status\");\n> diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h\n> index c97828577602..fdaa10e6c176 100644\n> --- a/src/ipa/rpi/controller/agc_algorithm.h\n> +++ b/src/ipa/rpi/controller/agc_algorithm.h\n> @@ -30,8 +30,12 @@ public:\n> \tvirtual void setMeteringMode(std::string const &meteringModeName) = 0;\n> \tvirtual void setExposureMode(std::string const &exposureModeName) = 0;\n> \tvirtual void setConstraintMode(std::string const &contraintModeName) = 0;\n> -\tvirtual void enableAuto() = 0;\n> -\tvirtual void disableAuto() = 0;\n> +\tvirtual void enableAutoExposure() = 0;\n> +\tvirtual void disableAutoExposure() = 0;\n> +\tvirtual bool autoExposureEnabled() const = 0;\n> +\tvirtual void enableAutoGain() = 0;\n> +\tvirtual void disableAutoGain() = 0;\n> +\tvirtual bool autoGainEnabled() const = 0;\n> \tvirtual void setActiveChannels(const std::vector &activeChannels) = 0;\n> };\n> \n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index c48fdf156591..02bfdb4a5e22 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const\n> \treturn 0;\n> }\n> \n> -void Agc::disableAuto()\n> +void Agc::disableAutoExposure()\n> {\n> -\tLOG(RPiAgc, Debug) << \"disableAuto\";\n> +\tLOG(RPiAgc, Debug) << \"disableAutoExposure\";\n> \n> \t/* All channels are enabled/disabled together. */\n> \tfor (auto &data : channelData_)\n> -\t\tdata.channel.disableAuto();\n> +\t\tdata.channel.disableAutoExposure();\n> }\n> \n> -void Agc::enableAuto()\n> +void Agc::enableAutoExposure()\n> {\n> -\tLOG(RPiAgc, Debug) << \"enableAuto\";\n> +\tLOG(RPiAgc, Debug) << \"enableAutoExposure\";\n> \n> \t/* All channels are enabled/disabled together. */\n> \tfor (auto &data : channelData_)\n> -\t\tdata.channel.enableAuto();\n> +\t\tdata.channel.enableAutoExposure();\n> +}\n> +\n> +bool Agc::autoExposureEnabled() const\n> +{\n> +\tLOG(RPiAgc, Debug) << \"autoExposureEnabled\";\n> +\n> +\t/*\n> +\t * We always have at least one channel, and since all channels are\n> +\t * enabled and disabled together we can simply check the first one.\n> +\t */\n> +\treturn channelData_[0].channel.autoExposureEnabled();\n> +}\n> +\n> +void Agc::disableAutoGain()\n> +{\n> +\tLOG(RPiAgc, Debug) << \"disableAutoGain\";\n> +\n> +\t/* All channels are enabled/disabled together. */\n> +\tfor (auto &data : channelData_)\n> +\t\tdata.channel.disableAutoGain();\n> +}\n> +\n> +void Agc::enableAutoGain()\n> +{\n> +\tLOG(RPiAgc, Debug) << \"enableAutoGain\";\n> +\n> +\t/* All channels are enabled/disabled together. */\n> +\tfor (auto &data : channelData_)\n> +\t\tdata.channel.enableAutoGain();\n> +}\n> +\n> +bool Agc::autoGainEnabled() const\n> +{\n> +\tLOG(RPiAgc, Debug) << \"autoGainEnabled\";\n> +\n> +\t/*\n> +\t * We always have at least one channel, and since all channels are\n> +\t * enabled and disabled together we can simply check the first one.\n> +\t */\n> +\treturn channelData_[0].channel.autoGainEnabled();\n> }\n> \n> unsigned int Agc::getConvergenceFrames() const\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index 3aca000bb4c2..c3a940bf697a 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -40,8 +40,12 @@ public:\n> \tvoid setMeteringMode(std::string const &meteringModeName) override;\n> \tvoid setExposureMode(std::string const &exposureModeName) override;\n> \tvoid setConstraintMode(std::string const &contraintModeName) override;\n> -\tvoid enableAuto() override;\n> -\tvoid disableAuto() override;\n> +\tvoid enableAutoExposure() override;\n> +\tvoid disableAutoExposure() override;\n> +\tbool autoExposureEnabled() const override;\n> +\tvoid enableAutoGain() override;\n> +\tvoid disableAutoGain() override;\n> +\tbool autoGainEnabled() const override;\n> \tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata) override;\n> \tvoid prepare(Metadata *imageMetadata) override;\n> \tvoid process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index 79c459735dfd..e79184b7ac74 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -319,18 +319,36 @@ int AgcChannel::read(const libcamera::YamlObject ¶ms,\n> \treturn 0;\n> }\n> \n> -void AgcChannel::disableAuto()\n> +void AgcChannel::disableAutoExposure()\n> {\n> \tfixedExposureTime_ = status_.exposureTime;\n> -\tfixedAnalogueGain_ = status_.analogueGain;\n> }\n> \n> -void AgcChannel::enableAuto()\n> +void AgcChannel::enableAutoExposure()\n> {\n> \tfixedExposureTime_ = 0s;\n> +}\n> +\n> +bool AgcChannel::autoExposureEnabled() const\n> +{\n> +\treturn fixedExposureTime_ == 0s;\n> +}\n> +\n> +void AgcChannel::disableAutoGain()\n> +{\n> +\tfixedAnalogueGain_ = status_.analogueGain;\n> +}\n> +\n> +void AgcChannel::enableAutoGain()\n> +{\n> \tfixedAnalogueGain_ = 0;\n> }\n> \n> +bool AgcChannel::autoGainEnabled() const\n> +{\n> +\treturn fixedAnalogueGain_ == 0;\n> +}\n> +\n> unsigned int AgcChannel::getConvergenceFrames() const\n> {\n> \t/*\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> index 734e5efd3266..fa697e6fa57d 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> @@ -96,8 +96,12 @@ public:\n> \tvoid setMeteringMode(std::string const &meteringModeName);\n> \tvoid setExposureMode(std::string const &exposureModeName);\n> \tvoid setConstraintMode(std::string const &contraintModeName);\n> -\tvoid enableAuto();\n> -\tvoid disableAuto();\n> +\tvoid enableAutoExposure();\n> +\tvoid disableAutoExposure();\n> +\tbool autoExposureEnabled() const;\n> +\tvoid enableAutoGain();\n> +\tvoid disableAutoGain();\n> +\tbool autoGainEnabled() const;\n> \tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata);\n> \tvoid prepare(Metadata *imageMetadata);\n> \tvoid process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,","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 1BEE7C32DA\n\tfor ;\n\tMon, 16 Dec 2024 15:04:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F2A867F6B;\n\tMon, 16 Dec 2024 16:04:11 +0100 (CET)","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 8D49F67F55\n\tfor ;\n\tMon, 16 Dec 2024 16:04:10 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 908DE675;\n\tMon, 16 Dec 2024 16:03:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aYRmzrfo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734361413;\n\tbh=mF916FPuSmdvJK6q/I/AzrjazX/mbdxynpxUJaT3RVU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aYRmzrfo94flBJcH0rwl60/z5/ekAkEVT9wvBvIOayyLFT5ZSw+a3T2ocZJ7C4TrO\n\t8YhBN7by/1JYiIilVwbc5ATRNx6mwvkwSq78JI0/Imw10PB4y3SocxQcIQvL5uDxxw\n\tdrUMK9Zn5thng9kGYxeiqRTZXU3VoRCnXeyGMHSg=","Date":"Mon, 16 Dec 2024 17:03:53 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, Jacopo Mondi ,\n\tNaushir Patuck ,\n\tDavid Plowman ","Subject":"Re: [PATCH v5 5/8] ipa: raspberry: Port to the new AEGC controls","Message-ID":"<20241216150353.GA2131@pendragon.ideasonboard.com>","References":"<20241216043954.3506855-1-paul.elder@ideasonboard.com>\n\t<20241216043954.3506855-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241216043954.3506855-6-paul.elder@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32809,"web_url":"https://patchwork.libcamera.org/comment/32809/","msgid":"","date":"2024-12-17T11:43:09","subject":"Re: [PATCH v5 5/8] ipa: raspberry: Port to the new AEGC controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Paul\n\nThanks for the patch!\n\nMaybe s/raspberry/rpi/ in the subject line?\n\nBefore getting into the details, perhaps a few remarks would help\neveryone to understand where I'm coming from. We have a lot of users,\nand mostly I'm just a bit worried about the many Python users could\nget quite confused by some of these changes. So I'm interested in\nmitigations, such as:\n- limiting behaviour changes\n- providing obvious warnings when things are not behaving as before\n- maybe maintaining \"legacy\" controls for a period, maybe with warnings\n- as a last resort I can hide changes in our Python layers, though the\nthought of (effectively) \"reverting\" some parts of this patch up there\nsounds quite unappealing!\n\nOn Mon, 16 Dec 2024 at 04:40, Paul Elder wrote:\n>\n> From: Jacopo Mondi \n>\n> The newly introduced controls to drive the AEGC algorithm allow\n> to control the computation of the exposure time and analogue gain\n> separately.\n>\n> The RPi AEGC implementation already computes the shutter and gain\n> values separately but does not expose separate functions to control\n> them.\n>\n> Augment the AgcAlgorithm interface to allow pausing/resuming the shutter\n> and gain automatic computations separately and plumb them to the newly\n> introduced controls.\n>\n> Add safety checks to ignore ExposureTime and AnalogueGain values if the\n> algorithms are not paused, and report the correct AeState value by\n> checking if both algorithms have been paused or if they have converged.\n>\n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Paul Elder \n> Reviewed-by: Paul Elder \n>\n> ---\n> No change in v5\n>\n> Changes in v4:\n> - s/shutter/exposure/\n>\n> Changes in v3:\n> - recovered from 2-year-old bitrot\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 74 +++++++++++++++++++---\n> src/ipa/rpi/controller/agc_algorithm.h | 8 ++-\n> src/ipa/rpi/controller/rpi/agc.cpp | 52 +++++++++++++--\n> src/ipa/rpi/controller/rpi/agc.h | 8 ++-\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 24 ++++++-\n> src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n> 6 files changed, 150 insertions(+), 24 deletions(-)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 5fce17e67bd6..8924c3a52c26 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -55,8 +55,13 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>\n> /* List of controls handled by the Raspberry Pi IPA */\n> const ControlInfoMap::Map ipaControls{\n> - { &controls::AeEnable, ControlInfo(false, true) },\n\nThis feels like a common operation, perhaps almost the single most\ncommon one in practice. Is there an argument for keeping it?\n\n> + { &controls::ExposureTimeMode,\n> + ControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> + static_cast(controls::ExposureTimeModeManual)) },\n\nPreviously we had 0 = off (therefore manual), 1 = on (therefore auto),\nso this looks to be the opposite way round. Quite a lot of Python\nusers try to avoid doing the\n\"libcamera.controls.ExposureTimeModeEnum.Auto/Manual\" thing because\nthey can't remember it, so these folks could get caught out. Could we\navoid that just by swapping these round?\n\n> { &controls::ExposureTime, ControlInfo(0, 66666) },\n> + { &controls::AnalogueGainMode,\n> + ControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> + static_cast(controls::AnalogueGainModeManual)) },\n\nAs above.\n\n> { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> @@ -764,21 +769,22 @@ void IpaBase::applyControls(const ControlList &controls)\n> << \" = \" << ctrl.second.toString();\n>\n> switch (ctrl.first) {\n> - case controls::AE_ENABLE: {\n> + case controls::EXPOSURE_TIME_MODE: {\n> RPiController::AgcAlgorithm *agc = dynamic_cast(\n> controller_.getAlgorithm(\"agc\"));\n> if (!agc) {\n> LOG(IPARPI, Warning)\n> - << \"Could not set AE_ENABLE - no AGC algorithm\";\n> + << \"Could not set EXPOSURE_TIME_MODE - no AGC algorithm\";\n> break;\n> }\n>\n> - if (ctrl.second.get() == false)\n> - agc->disableAuto();\n> + if (ctrl.second.get() == controls::ExposureTimeModeManual)\n> + agc->disableAutoExposure();\n> else\n> - agc->enableAuto();\n> + agc->enableAutoExposure();\n>\n> - libcameraMetadata_.set(controls::AeEnable, ctrl.second.get());\n> + libcameraMetadata_.set(controls::ExposureTimeMode,\n> + ctrl.second.get());\n\nI'm wondering here whether we have introduced that race condition\nwhere you can't reliably update the ExposureTimeMode and the\nExposureTime in the same set of controls. (The unordered nature of\ncontrols means it could be random whether the exposure time gets\nignored.)\n\nBeing able to set manual mode and an exposure time atomically seems\nlike an obvious thing we (indeed everyone) should support.\n\nThe other case - setting an exposure time and re-enabling auto mode -\nsounds plausible too, though less obvious. I'm not totally sure how\nour implementation could support that, do we need to worry?\n\n> break;\n> }\n>\n> @@ -791,6 +797,13 @@ void IpaBase::applyControls(const ControlList &controls)\n> break;\n> }\n>\n> + /*\n> + * Ignore manual exposure time when the auto exposure\n> + * algorithm is running.\n> + */\n> + if (agc->autoExposureEnabled())\n> + break;\n> +\n\nMight it be preferable if setting the exposure time *automatically*\nputs you into manual mode? Not sure. This would feel slightly more\nfamiliar to our existing users, and also mirrors the behaviour of\nautofocus (where setting lens position puts you into manual mode), so\nit has a degree of consistency/precedent.\n\n> /* The control provides units of microseconds. */\n> agc->setFixedExposureTime(0, ctrl.second.get() * 1.0us);\n>\n> @@ -798,6 +811,25 @@ void IpaBase::applyControls(const ControlList &controls)\n> break;\n> }\n>\n> + case controls::ANALOGUE_GAIN_MODE: {\n> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> + controller_.getAlgorithm(\"agc\"));\n> + if (!agc) {\n> + LOG(IPARPI, Warning)\n> + << \"Could not set ANALOGUE_GAIN_MODE - no AGC algorithm\";\n> + break;\n> + }\n> +\n> + if (ctrl.second.get() == controls::AnalogueGainModeManual)\n> + agc->disableAutoGain();\n> + else\n> + agc->enableAutoGain();\n> +\n> + libcameraMetadata_.set(controls::AnalogueGainMode,\n> + ctrl.second.get());\n> + break;\n> + }\n> +\n\nSame as above re. control race condition.\n\n> case controls::ANALOGUE_GAIN: {\n> RPiController::AgcAlgorithm *agc = dynamic_cast(\n> controller_.getAlgorithm(\"agc\"));\n> @@ -807,6 +839,13 @@ void IpaBase::applyControls(const ControlList &controls)\n> break;\n> }\n>\n> + /*\n> + * Ignore manual analogue gain value when the auto gain\n> + * algorithm is running.\n> + */\n> + if (agc->autoGainEnabled())\n> + break;\n> +\n\nAlso same as before.\n\n> agc->setFixedAnalogueGain(0, ctrl.second.get());\n>\n> libcameraMetadata_.set(controls::AnalogueGain,\n> @@ -863,6 +902,13 @@ void IpaBase::applyControls(const ControlList &controls)\n> break;\n> }\n>\n> + /*\n> + * Ignore AE_EXPOSURE_MODE if the shutter or the gain\n> + * are in auto mode.\n> + */\n> + if (agc->autoExposureEnabled() || agc->autoGainEnabled())\n> + break;\n> +\n\nI wasn't convinced here. We want to set exposure mode if both are\nauto, right? Suddenly feeling a bit muddled...\n\nI feel a bit funny about not allowing this to update if it will have\nno (immediate) effect. All other \"auto parameters\" (constraint mode,\nmetering mode, ev, flicker...) can be updated in manual mode, and this\nfeels probably right to me. What do other folks think about this?\n\nJust as an aside, I also feel a bit funny about the exposure time and\nanalogue gain being different in this respect. That is, with this\npatch you can't update them while in auto mode, when perhaps you\nshould be able to - taking only effect when you go into manual mode.\nSome of the confusion here is because of our implementation - we\nconflate the \"enable\" and the \"value\" into just the \"value\", whereas\nthe controls for this are now split. I wonder if we should carefully\ndefine the desired behaviour for all this, and then rationalise the\nimplementation. Feeling a bit uncertain about all this...!\n\nAnyway, I think that's probably everything for now. Thanks again!\n\nDavid\n\n> int32_t idx = ctrl.second.get();\n> if (ExposureModeTable.count(idx)) {\n> agc->setExposureMode(ExposureModeTable.at(idx));\n> @@ -1323,9 +1369,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> }\n>\n> AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked(\"agc.prepare_status\");\n> - if (agcPrepareStatus) {\n> - libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);\n> + if (agcPrepareStatus)\n> libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);\n> +\n> + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> + controller_.getAlgorithm(\"agc\"));\n> + if (agc) {\n> + if (!agc->autoExposureEnabled() && !agc->autoGainEnabled())\n> + libcameraMetadata_.set(controls::AeState, controls::AeStateIdle);\n> + else if (agcPrepareStatus)\n> + libcameraMetadata_.set(controls::AeState,\n> + agcPrepareStatus->locked\n> + ? controls::AeStateConverged\n> + : controls::AeStateSearching);\n> }\n>\n> LuxStatus *luxStatus = rpiMetadata.getLocked(\"lux.status\");\n> diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h\n> index c97828577602..fdaa10e6c176 100644\n> --- a/src/ipa/rpi/controller/agc_algorithm.h\n> +++ b/src/ipa/rpi/controller/agc_algorithm.h\n> @@ -30,8 +30,12 @@ public:\n> virtual void setMeteringMode(std::string const &meteringModeName) = 0;\n> virtual void setExposureMode(std::string const &exposureModeName) = 0;\n> virtual void setConstraintMode(std::string const &contraintModeName) = 0;\n> - virtual void enableAuto() = 0;\n> - virtual void disableAuto() = 0;\n> + virtual void enableAutoExposure() = 0;\n> + virtual void disableAutoExposure() = 0;\n> + virtual bool autoExposureEnabled() const = 0;\n> + virtual void enableAutoGain() = 0;\n> + virtual void disableAutoGain() = 0;\n> + virtual bool autoGainEnabled() const = 0;\n> virtual void setActiveChannels(const std::vector &activeChannels) = 0;\n> };\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index c48fdf156591..02bfdb4a5e22 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const\n> return 0;\n> }\n>\n> -void Agc::disableAuto()\n> +void Agc::disableAutoExposure()\n> {\n> - LOG(RPiAgc, Debug) << \"disableAuto\";\n> + LOG(RPiAgc, Debug) << \"disableAutoExposure\";\n>\n> /* All channels are enabled/disabled together. */\n> for (auto &data : channelData_)\n> - data.channel.disableAuto();\n> + data.channel.disableAutoExposure();\n> }\n>\n> -void Agc::enableAuto()\n> +void Agc::enableAutoExposure()\n> {\n> - LOG(RPiAgc, Debug) << \"enableAuto\";\n> + LOG(RPiAgc, Debug) << \"enableAutoExposure\";\n>\n> /* All channels are enabled/disabled together. */\n> for (auto &data : channelData_)\n> - data.channel.enableAuto();\n> + data.channel.enableAutoExposure();\n> +}\n> +\n> +bool Agc::autoExposureEnabled() const\n> +{\n> + LOG(RPiAgc, Debug) << \"autoExposureEnabled\";\n> +\n> + /*\n> + * We always have at least one channel, and since all channels are\n> + * enabled and disabled together we can simply check the first one.\n> + */\n> + return channelData_[0].channel.autoExposureEnabled();\n> +}\n> +\n> +void Agc::disableAutoGain()\n> +{\n> + LOG(RPiAgc, Debug) << \"disableAutoGain\";\n> +\n> + /* All channels are enabled/disabled together. */\n> + for (auto &data : channelData_)\n> + data.channel.disableAutoGain();\n> +}\n> +\n> +void Agc::enableAutoGain()\n> +{\n> + LOG(RPiAgc, Debug) << \"enableAutoGain\";\n> +\n> + /* All channels are enabled/disabled together. */\n> + for (auto &data : channelData_)\n> + data.channel.enableAutoGain();\n> +}\n> +\n> +bool Agc::autoGainEnabled() const\n> +{\n> + LOG(RPiAgc, Debug) << \"autoGainEnabled\";\n> +\n> + /*\n> + * We always have at least one channel, and since all channels are\n> + * enabled and disabled together we can simply check the first one.\n> + */\n> + return channelData_[0].channel.autoGainEnabled();\n> }\n>\n> unsigned int Agc::getConvergenceFrames() const\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index 3aca000bb4c2..c3a940bf697a 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -40,8 +40,12 @@ public:\n> void setMeteringMode(std::string const &meteringModeName) override;\n> void setExposureMode(std::string const &exposureModeName) override;\n> void setConstraintMode(std::string const &contraintModeName) override;\n> - void enableAuto() override;\n> - void disableAuto() override;\n> + void enableAutoExposure() override;\n> + void disableAutoExposure() override;\n> + bool autoExposureEnabled() const override;\n> + void enableAutoGain() override;\n> + void disableAutoGain() override;\n> + bool autoGainEnabled() const override;\n> void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;\n> void prepare(Metadata *imageMetadata) override;\n> void process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index 79c459735dfd..e79184b7ac74 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -319,18 +319,36 @@ int AgcChannel::read(const libcamera::YamlObject ¶ms,\n> return 0;\n> }\n>\n> -void AgcChannel::disableAuto()\n> +void AgcChannel::disableAutoExposure()\n> {\n> fixedExposureTime_ = status_.exposureTime;\n> - fixedAnalogueGain_ = status_.analogueGain;\n> }\n>\n> -void AgcChannel::enableAuto()\n> +void AgcChannel::enableAutoExposure()\n> {\n> fixedExposureTime_ = 0s;\n> +}\n> +\n> +bool AgcChannel::autoExposureEnabled() const\n> +{\n> + return fixedExposureTime_ == 0s;\n> +}\n> +\n> +void AgcChannel::disableAutoGain()\n> +{\n> + fixedAnalogueGain_ = status_.analogueGain;\n> +}\n> +\n> +void AgcChannel::enableAutoGain()\n> +{\n> fixedAnalogueGain_ = 0;\n> }\n>\n> +bool AgcChannel::autoGainEnabled() const\n> +{\n> + return fixedAnalogueGain_ == 0;\n> +}\n> +\n> unsigned int AgcChannel::getConvergenceFrames() const\n> {\n> /*\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> index 734e5efd3266..fa697e6fa57d 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> @@ -96,8 +96,12 @@ public:\n> void setMeteringMode(std::string const &meteringModeName);\n> void setExposureMode(std::string const &exposureModeName);\n> void setConstraintMode(std::string const &contraintModeName);\n> - void enableAuto();\n> - void disableAuto();\n> + void enableAutoExposure();\n> + void disableAutoExposure();\n> + bool autoExposureEnabled() const;\n> + void enableAutoGain();\n> + void disableAutoGain();\n> + bool autoGainEnabled() const;\n> void switchMode(CameraMode const &cameraMode, Metadata *metadata);\n> void prepare(Metadata *imageMetadata);\n> void process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,\n> --\n> 2.39.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 71457C32F6\n\tfor ;\n\tTue, 17 Dec 2024 11:43:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAF4667F0D;\n\tTue, 17 Dec 2024 12:43:22 +0100 (CET)","from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com\n\t[IPv6:2607:f8b0:4864:20::82c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3E4967F0D\n\tfor ;\n\tTue, 17 Dec 2024 12:43:20 +0100 (CET)","by mail-qt1-x82c.google.com with SMTP id\n\td75a77b69052e-467a17055e6so53638551cf.3\n\tfor ;\n\tTue, 17 Dec 2024 03:43:20 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"nCizN2ic\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1734435800; x=1735040600;\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=AOC3Hdm2fTxe2lsMVJqcY2YCv08udTLgDKELQ2NAazw=;\n\tb=nCizN2ic3qMbs8/XEKFl3BmJMCyGWEjPNrLVMy5dJ0vchN2p4Y12KIdivW8VmepG5F\n\ttBw2i9YBw9ffEVNapf1zPGdvqesp/3vi4kAgelVX4cxYVwFqfyqyJYWgeEfK/zIUTy0I\n\tpXMo8L6dbPBd/ZC0SpAuPmnwXn2G28HHZfPCH0xRy3pztEfVSefge9mSG0dJu9Qf8v5s\n\tHaKhchPbr6hsTHak3xYdN1UB/01gQI0S4QA3Z6G/uvdGF3BIpmWxZZzp5xZC3WrePaoQ\n\tOcp1s2LoMzEW9hIXc0TIwoy8LujCMJQsnVKT4e5FxKBVJDo32tJaMfpuQ4Fz8iWoeu17\n\tMDKA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1734435800; x=1735040600;\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=AOC3Hdm2fTxe2lsMVJqcY2YCv08udTLgDKELQ2NAazw=;\n\tb=qilxGjEJnQ7yOHr5sGyF5zzN77L14w8bVmbH+oMTI7ra/vy/0C2kxHSzGAiju26W5/\n\tYxNzFV2Yd4NnwI4fw0lBqp/tl0qX/+W37wLoigCGVX1pFPoPaDux9S2n18KGVQR0vyap\n\tqWj+aGYQ65t+zWLohYNGobdqyXMUAi4SUDSXfv6SP7OaNrO6ggvlua+OY92Nlls0/sJf\n\tqeYcFQ3iFYCQjAKfBptLycOh6qNJTYW8ZNhecARoRSnT1cBcvRi026F5vrZBpxAK3ojD\n\tW5Obl2bDvfgGdxe7Dga747NeXNZdwVUJEylTCD8exQUpGBdGTc0QrAX2304HcSj8vsGI\n\twnoQ==","X-Gm-Message-State":"AOJu0YzFdID3QhmWTBHkhhHxlN4nWOVaIUtrxXWBwHJG5oOIFWmRf2oo\n\tPH3s4QYpvsHzwKfrwf7OQVq09RKBZ/Zd2iGWkrdemVFz5ETwxBkh3DyyEMv338ovp++dfM7sFuw\n\twU3mwMpv7ZfFV3g2YnQgThNoXcCR4R7FLasz0Rg==","X-Gm-Gg":"ASbGncuzouOtgOPRJ2oVamgGgBIMjM/jLe52VjPpmLW3d6tH5vz5BrtyA3so3pidmS6\n\te9ZPYZAcVcERWpPzOk64k9rTggt7xfGyC2IlTdNZWRchpeccN7QFxU8pVttg8wfst7Cqp","X-Google-Smtp-Source":"AGHT+IEaiCYom8D9GOyGhIJWtIASWghCWohZzLnZ1OBi2krPmk0KYNsrdVJhHMZl7oHZRMbZYIcD720G2yMzOCZMvUo=","X-Received":"by 2002:a05:622a:48f:b0:467:8703:a740 with SMTP id\n\td75a77b69052e-467a5755691mr311166421cf.18.1734435799651;\n\tTue, 17 Dec 2024 03:43:19 -0800 (PST)","MIME-Version":"1.0","References":"<20241216043954.3506855-1-paul.elder@ideasonboard.com>\n\t<20241216043954.3506855-6-paul.elder@ideasonboard.com>","In-Reply-To":"<20241216043954.3506855-6-paul.elder@ideasonboard.com>","From":"David Plowman ","Date":"Tue, 17 Dec 2024 11:43:09 +0000","Message-ID":"","Subject":"Re: [PATCH v5 5/8] ipa: raspberry: Port to the new AEGC controls","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]