[{"id":33040,"web_url":"https://patchwork.libcamera.org/comment/33040/","msgid":"<jw7ssonzor62yx75k7pnniyylpqjfwurm7a6vigqjhu7wyx4sv@mfwruyh7e6vp>","date":"2025-01-13T11:18:14","subject":"Re: [PATCH v7 05/12] ipa: raspberry: Port to the new AEGC controls","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch. \n\nOn Fri, Jan 10, 2025 at 05:57:30PM -0600, Paul Elder wrote:\n> From: Jacopo Mondi <jacopo@jmondi.org>\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 <jacopo@jmondi.org>\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> \n> ---\n> No change in v7\n> \n> No change in v6\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 6ff1e22b1..d6721c49c 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<int32_t>(controls::ExposureTimeModeAuto),\n> +\t\t      static_cast<int32_t>(controls::ExposureTimeModeManual)) },\n>  \t{ &controls::ExposureTime, ControlInfo(0, 66666) },\n> +\t{ &controls::AnalogueGainMode,\n> +\t  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),\n> +\t\t      static_cast<int32_t>(controls::AnalogueGainModeManual)) },\n\nI think we should set the default value to Auto for ExposureTimeMode and\nAnalogueGainMode.\n\n>  \t{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n>  \t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>  \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> @@ -756,21 +761,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<RPiController::AgcAlgorithm *>(\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<bool>() == false)\n> -\t\t\t\tagc->disableAuto();\n> +\t\t\tif (ctrl.second.get<int32_t>() == 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<bool>());\n> +\t\t\tlibcameraMetadata_.set(controls::ExposureTimeMode,\n> +\t\t\t\t\t       ctrl.second.get<int32_t>());\n>  \t\t\tbreak;\n>  \t\t}\n>  \n> @@ -783,6 +789,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\nI believe this is still racy, as to my knowledge there is no defined\norder in which the controls are applied to the algorithm. I think that\nwas postponed to be done in later patches, but maybe it's worth a todo?\n\nEither way:\nReviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> \n\nCheers,\nStefan\n\n>  \t\t\t/* The control provides units of microseconds. */\n>  \t\t\tagc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);\n>  \n> @@ -790,6 +803,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<RPiController::AgcAlgorithm *>(\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<int32_t>() == 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<int32_t>());\n> +\t\t\tbreak;\n> +\t\t}\n> +\n>  \t\tcase controls::ANALOGUE_GAIN: {\n>  \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(\n>  \t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n> @@ -799,6 +831,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<float>());\n>  \n>  \t\t\tlibcameraMetadata_.set(controls::AnalogueGain,\n> @@ -855,6 +894,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<int32_t>();\n>  \t\t\tif (ExposureModeTable.count(idx)) {\n>  \t\t\t\tagc->setExposureMode(ExposureModeTable.at(idx));\n> @@ -1334,9 +1380,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n>  \t}\n>  \n>  \tAgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>(\"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<RPiController::AgcAlgorithm *>(\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<LuxStatus>(\"lux.status\");\n> diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h\n> index c97828577..fdaa10e6c 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<unsigned int> &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 c48fdf156..02bfdb4a5 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 3aca000bb..c3a940bf6 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 79c459735..e79184b7a 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 &params,\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 734e5efd3..fa697e6fa 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,\n> -- \n> 2.39.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 38528C32F6\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 13 Jan 2025 11:18:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75A24684D9;\n\tMon, 13 Jan 2025 12:18:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D510607D6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 13 Jan 2025 12:18:17 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3c15:8eea:8742:2e5a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B0DCF63F;\n\tMon, 13 Jan 2025 12:17:20 +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=\"SgXE+A8i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736767040;\n\tbh=4220Z9nLqeqQb+81ZgpnX/KBoyaqro3A1a+CMufHo+M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SgXE+A8iL4KefXcdJI6zmjEgPu+bm4xZDrk+R1sPuJmTF0JqP8vvtqavaPQTaUWBb\n\tXkYkbrjyqdSFj/yL37h7VJ8Vv2dsB73JVQp9v2rlXj8kLGTiPsJt5qdFmdISQ4gYyp\n\tYFlLQQxuPXV4OHVVRnBwjIqZEjVGD3C01EI3HO3c=","Date":"Mon, 13 Jan 2025 12:18:14 +0100","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, Jacopo Mondi <jacopo@jmondi.org>, \n\tlaurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH v7 05/12] ipa: raspberry: Port to the new AEGC controls","Message-ID":"<jw7ssonzor62yx75k7pnniyylpqjfwurm7a6vigqjhu7wyx4sv@mfwruyh7e6vp>","References":"<20250110235737.1524733-1-paul.elder@ideasonboard.com>\n\t<20250110235737.1524733-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250110235737.1524733-6-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]