[PATCH v5 5/8] ipa: raspberry: Port to the new AEGC controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 16 16:03:53 CET 2024
Hi Paul,
Thank you for the patch.
As this patch touches the Rapsberry Pi IPA, Naush, David, could you
review it ?
On Mon, Dec 16, 2024 at 01:39:51PM +0900, Paul Elder wrote:
> From: Jacopo Mondi <jacopo at jmondi.org>
>
> The newly introduced controls to drive the AEGC algorithm allow
> to control the computation of the exposure time and analogue gain
> separately.
>
> The RPi AEGC implementation already computes the shutter and gain
> values separately but does not expose separate functions to control
> them.
>
> Augment the AgcAlgorithm interface to allow pausing/resuming the shutter
> and gain automatic computations separately and plumb them to the newly
> introduced controls.
>
> Add safety checks to ignore ExposureTime and AnalogueGain values if the
> algorithms are not paused, and report the correct AeState value by
> checking if both algorithms have been paused or if they have converged.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> No change in v5
>
> Changes in v4:
> - s/shutter/exposure/
>
> Changes in v3:
> - recovered from 2-year-old bitrot
> ---
> src/ipa/rpi/common/ipa_base.cpp | 74 +++++++++++++++++++---
> src/ipa/rpi/controller/agc_algorithm.h | 8 ++-
> src/ipa/rpi/controller/rpi/agc.cpp | 52 +++++++++++++--
> src/ipa/rpi/controller/rpi/agc.h | 8 ++-
> src/ipa/rpi/controller/rpi/agc_channel.cpp | 24 ++++++-
> src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-
> 6 files changed, 150 insertions(+), 24 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 5fce17e67bd6..8924c3a52c26 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -55,8 +55,13 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>
> /* List of controls handled by the Raspberry Pi IPA */
> const ControlInfoMap::Map ipaControls{
> - { &controls::AeEnable, ControlInfo(false, true) },
> + { &controls::ExposureTimeMode,
> + ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> + static_cast<int32_t>(controls::ExposureTimeModeManual)) },
> { &controls::ExposureTime, ControlInfo(0, 66666) },
> + { &controls::AnalogueGainMode,
> + ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> + static_cast<int32_t>(controls::AnalogueGainModeManual)) },
> { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> @@ -764,21 +769,22 @@ void IpaBase::applyControls(const ControlList &controls)
> << " = " << ctrl.second.toString();
>
> switch (ctrl.first) {
> - case controls::AE_ENABLE: {
> + case controls::EXPOSURE_TIME_MODE: {
> RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> controller_.getAlgorithm("agc"));
> if (!agc) {
> LOG(IPARPI, Warning)
> - << "Could not set AE_ENABLE - no AGC algorithm";
> + << "Could not set EXPOSURE_TIME_MODE - no AGC algorithm";
> break;
> }
>
> - if (ctrl.second.get<bool>() == false)
> - agc->disableAuto();
> + if (ctrl.second.get<int32_t>() == controls::ExposureTimeModeManual)
> + agc->disableAutoExposure();
> else
> - agc->enableAuto();
> + agc->enableAutoExposure();
>
> - libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
> + libcameraMetadata_.set(controls::ExposureTimeMode,
> + ctrl.second.get<int32_t>());
> break;
> }
>
> @@ -791,6 +797,13 @@ void IpaBase::applyControls(const ControlList &controls)
> break;
> }
>
> + /*
> + * Ignore manual exposure time when the auto exposure
> + * algorithm is running.
> + */
> + if (agc->autoExposureEnabled())
> + break;
> +
> /* The control provides units of microseconds. */
> agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);
>
> @@ -798,6 +811,25 @@ void IpaBase::applyControls(const ControlList &controls)
> break;
> }
>
> + case controls::ANALOGUE_GAIN_MODE: {
> + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> + controller_.getAlgorithm("agc"));
> + if (!agc) {
> + LOG(IPARPI, Warning)
> + << "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
> + break;
> + }
> +
> + if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeManual)
> + agc->disableAutoGain();
> + else
> + agc->enableAutoGain();
> +
> + libcameraMetadata_.set(controls::AnalogueGainMode,
> + ctrl.second.get<int32_t>());
> + break;
> + }
> +
> case controls::ANALOGUE_GAIN: {
> RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> controller_.getAlgorithm("agc"));
> @@ -807,6 +839,13 @@ void IpaBase::applyControls(const ControlList &controls)
> break;
> }
>
> + /*
> + * Ignore manual analogue gain value when the auto gain
> + * algorithm is running.
> + */
> + if (agc->autoGainEnabled())
> + break;
> +
> agc->setFixedAnalogueGain(0, ctrl.second.get<float>());
>
> libcameraMetadata_.set(controls::AnalogueGain,
> @@ -863,6 +902,13 @@ void IpaBase::applyControls(const ControlList &controls)
> break;
> }
>
> + /*
> + * Ignore AE_EXPOSURE_MODE if the shutter or the gain
> + * are in auto mode.
> + */
> + if (agc->autoExposureEnabled() || agc->autoGainEnabled())
> + break;
> +
> int32_t idx = ctrl.second.get<int32_t>();
> if (ExposureModeTable.count(idx)) {
> agc->setExposureMode(ExposureModeTable.at(idx));
> @@ -1323,9 +1369,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
> }
>
> AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
> - if (agcPrepareStatus) {
> - libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);
> + if (agcPrepareStatus)
> libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);
> +
> + RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> + controller_.getAlgorithm("agc"));
> + if (agc) {
> + if (!agc->autoExposureEnabled() && !agc->autoGainEnabled())
> + libcameraMetadata_.set(controls::AeState, controls::AeStateIdle);
> + else if (agcPrepareStatus)
> + libcameraMetadata_.set(controls::AeState,
> + agcPrepareStatus->locked
> + ? controls::AeStateConverged
> + : controls::AeStateSearching);
> }
>
> LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");
> diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h
> index c97828577602..fdaa10e6c176 100644
> --- a/src/ipa/rpi/controller/agc_algorithm.h
> +++ b/src/ipa/rpi/controller/agc_algorithm.h
> @@ -30,8 +30,12 @@ public:
> virtual void setMeteringMode(std::string const &meteringModeName) = 0;
> virtual void setExposureMode(std::string const &exposureModeName) = 0;
> virtual void setConstraintMode(std::string const &contraintModeName) = 0;
> - virtual void enableAuto() = 0;
> - virtual void disableAuto() = 0;
> + virtual void enableAutoExposure() = 0;
> + virtual void disableAutoExposure() = 0;
> + virtual bool autoExposureEnabled() const = 0;
> + virtual void enableAutoGain() = 0;
> + virtual void disableAutoGain() = 0;
> + virtual bool autoGainEnabled() const = 0;
> virtual void setActiveChannels(const std::vector<unsigned int> &activeChannels) = 0;
> };
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index c48fdf156591..02bfdb4a5e22 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const
> return 0;
> }
>
> -void Agc::disableAuto()
> +void Agc::disableAutoExposure()
> {
> - LOG(RPiAgc, Debug) << "disableAuto";
> + LOG(RPiAgc, Debug) << "disableAutoExposure";
>
> /* All channels are enabled/disabled together. */
> for (auto &data : channelData_)
> - data.channel.disableAuto();
> + data.channel.disableAutoExposure();
> }
>
> -void Agc::enableAuto()
> +void Agc::enableAutoExposure()
> {
> - LOG(RPiAgc, Debug) << "enableAuto";
> + LOG(RPiAgc, Debug) << "enableAutoExposure";
>
> /* All channels are enabled/disabled together. */
> for (auto &data : channelData_)
> - data.channel.enableAuto();
> + data.channel.enableAutoExposure();
> +}
> +
> +bool Agc::autoExposureEnabled() const
> +{
> + LOG(RPiAgc, Debug) << "autoExposureEnabled";
> +
> + /*
> + * We always have at least one channel, and since all channels are
> + * enabled and disabled together we can simply check the first one.
> + */
> + return channelData_[0].channel.autoExposureEnabled();
> +}
> +
> +void Agc::disableAutoGain()
> +{
> + LOG(RPiAgc, Debug) << "disableAutoGain";
> +
> + /* All channels are enabled/disabled together. */
> + for (auto &data : channelData_)
> + data.channel.disableAutoGain();
> +}
> +
> +void Agc::enableAutoGain()
> +{
> + LOG(RPiAgc, Debug) << "enableAutoGain";
> +
> + /* All channels are enabled/disabled together. */
> + for (auto &data : channelData_)
> + data.channel.enableAutoGain();
> +}
> +
> +bool Agc::autoGainEnabled() const
> +{
> + LOG(RPiAgc, Debug) << "autoGainEnabled";
> +
> + /*
> + * We always have at least one channel, and since all channels are
> + * enabled and disabled together we can simply check the first one.
> + */
> + return channelData_[0].channel.autoGainEnabled();
> }
>
> unsigned int Agc::getConvergenceFrames() const
> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> index 3aca000bb4c2..c3a940bf697a 100644
> --- a/src/ipa/rpi/controller/rpi/agc.h
> +++ b/src/ipa/rpi/controller/rpi/agc.h
> @@ -40,8 +40,12 @@ public:
> void setMeteringMode(std::string const &meteringModeName) override;
> void setExposureMode(std::string const &exposureModeName) override;
> void setConstraintMode(std::string const &contraintModeName) override;
> - void enableAuto() override;
> - void disableAuto() override;
> + void enableAutoExposure() override;
> + void disableAutoExposure() override;
> + bool autoExposureEnabled() const override;
> + void enableAutoGain() override;
> + void disableAutoGain() override;
> + bool autoGainEnabled() const override;
> void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
> void prepare(Metadata *imageMetadata) override;
> void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index 79c459735dfd..e79184b7ac74 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -319,18 +319,36 @@ int AgcChannel::read(const libcamera::YamlObject ¶ms,
> return 0;
> }
>
> -void AgcChannel::disableAuto()
> +void AgcChannel::disableAutoExposure()
> {
> fixedExposureTime_ = status_.exposureTime;
> - fixedAnalogueGain_ = status_.analogueGain;
> }
>
> -void AgcChannel::enableAuto()
> +void AgcChannel::enableAutoExposure()
> {
> fixedExposureTime_ = 0s;
> +}
> +
> +bool AgcChannel::autoExposureEnabled() const
> +{
> + return fixedExposureTime_ == 0s;
> +}
> +
> +void AgcChannel::disableAutoGain()
> +{
> + fixedAnalogueGain_ = status_.analogueGain;
> +}
> +
> +void AgcChannel::enableAutoGain()
> +{
> fixedAnalogueGain_ = 0;
> }
>
> +bool AgcChannel::autoGainEnabled() const
> +{
> + return fixedAnalogueGain_ == 0;
> +}
> +
> unsigned int AgcChannel::getConvergenceFrames() const
> {
> /*
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> index 734e5efd3266..fa697e6fa57d 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> @@ -96,8 +96,12 @@ public:
> void setMeteringMode(std::string const &meteringModeName);
> void setExposureMode(std::string const &exposureModeName);
> void setConstraintMode(std::string const &contraintModeName);
> - void enableAuto();
> - void disableAuto();
> + void enableAutoExposure();
> + void disableAutoExposure();
> + bool autoExposureEnabled() const;
> + void enableAutoGain();
> + void disableAutoGain();
> + bool autoGainEnabled() const;
> void switchMode(CameraMode const &cameraMode, Metadata *metadata);
> void prepare(Metadata *imageMetadata);
> void process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list