[libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus into AgcStatus and AgcPrepareStatus
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Aug 29 16:01:01 CEST 2023
Hi David
On Fri, Jul 28, 2023 at 02:37:00PM +0100, David Plowman via libcamera-devel wrote:
> The Agc::process() function returns an AgcStatus object in the
> metadata as before, but Agc::prepare() is changed to return the values
> it computes in a separate AgcPrepareStatus object (under the new tag
> "agc.prepare_status").
>
> The "digitalGain" and "locked" fields are moved from AgcStatus to
> AgcPrepareStatus.
>
> This will be useful going forward as we can be more flexible about the
> order in which prepare() and process() are called, without them
> trampling on each other's results.
I presume this will be useful when dealing with multiple AGC channels ?
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
> ---
> src/ipa/rpi/common/ipa_base.cpp | 8 ++++----
> src/ipa/rpi/controller/agc_status.h | 9 +++++++--
> src/ipa/rpi/controller/rpi/agc.cpp | 20 +++++++++++---------
> src/ipa/rpi/controller/rpi/agc.h | 2 +-
> src/ipa/rpi/vc4/vc4.cpp | 6 +++---
> 5 files changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index f40f2e71..6ae84cc6 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -1151,10 +1151,10 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
> libcameraMetadata_.set(controls::LensPosition, *deviceStatus->lensPosition);
> }
>
> - AgcStatus *agcStatus = rpiMetadata.getLocked<AgcStatus>("agc.status");
> - if (agcStatus) {
> - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);
> - libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);
> + AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
> + if (agcPrepareStatus) {
> + libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);
> + libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);
> }
>
> LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");
> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h
> index 6c112e76..597eddd7 100644
> --- a/src/ipa/rpi/controller/agc_status.h
> +++ b/src/ipa/rpi/controller/agc_status.h
> @@ -11,8 +11,10 @@
> #include <libcamera/base/utils.h>
>
> /*
> - * The AGC algorithm should post the following structure into the image's
> - * "agc.status" metadata.
> + * The AGC algorithm process method should post an AgcStatus into the image
> + * metadata under the tag "agc.status".
> + * The AGC algorithm prepare method should post an AgcPrepareStatus instead
> + * under "agc.prepare_status".
> */
>
> /*
> @@ -34,6 +36,9 @@ struct AgcStatus {
> int floatingRegionEnable;
> libcamera::utils::Duration fixedShutter;
> double fixedAnalogueGain;
> +};
> +
> +struct AgcPrepareStatus {
> double digitalGain;
> int locked;
> };
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index 6087fc60..7b02972a 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -419,11 +419,13 @@ void Agc::prepare(Metadata *imageMetadata)
> {
> Duration totalExposureValue = status_.totalExposureValue;
> AgcStatus delayedStatus;
> + AgcPrepareStatus prepareStatus;
>
> if (!imageMetadata->get("agc.delayed_status", delayedStatus))
> totalExposureValue = delayedStatus.totalExposureValue;
>
> - status_.digitalGain = 1.0;
> + prepareStatus.digitalGain = 1.0;
> + prepareStatus.locked = false;
>
> if (status_.totalExposureValue) {
> /* Process has run, so we have meaningful values. */
> @@ -432,23 +434,23 @@ void Agc::prepare(Metadata *imageMetadata)
> Duration actualExposure = deviceStatus.shutterSpeed *
> deviceStatus.analogueGain;
> if (actualExposure) {
> - status_.digitalGain = totalExposureValue / actualExposure;
> + double digitalGain = totalExposureValue / actualExposure;
> LOG(RPiAgc, Debug) << "Want total exposure " << totalExposureValue;
> /*
> * Never ask for a gain < 1.0, and also impose
> * some upper limit. Make it customisable?
> */
> - status_.digitalGain = std::max(1.0, std::min(status_.digitalGain, 4.0));
> + prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, 4.0));
> LOG(RPiAgc, Debug) << "Actual exposure " << actualExposure;
> - LOG(RPiAgc, Debug) << "Use digitalGain " << status_.digitalGain;
> + LOG(RPiAgc, Debug) << "Use digitalGain " << prepareStatus.digitalGain;
> LOG(RPiAgc, Debug) << "Effective exposure "
> - << actualExposure * status_.digitalGain;
> + << actualExposure * prepareStatus.digitalGain;
> /* Decide whether AEC/AGC has converged. */
> - updateLockStatus(deviceStatus);
> + prepareStatus.locked = updateLockStatus(deviceStatus);
> }
> } else
> LOG(RPiAgc, Warning) << name() << ": no device metadata";
> - imageMetadata->set("agc.status", status_);
> + imageMetadata->set("agc.prepare_status", prepareStatus);
> }
> }
>
> @@ -486,7 +488,7 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
> writeAndFinish(imageMetadata, desaturate);
> }
>
> -void Agc::updateLockStatus(DeviceStatus const &deviceStatus)
> +bool Agc::updateLockStatus(DeviceStatus const &deviceStatus)
> {
> const double errorFactor = 0.10; /* make these customisable? */
> const int maxLockCount = 5;
> @@ -522,7 +524,7 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)
> lastTargetExposure_ = status_.targetExposureValue;
>
> LOG(RPiAgc, Debug) << "Lock count updated to " << lockCount_;
> - status_.locked = lockCount_ == maxLockCount;
> + return lockCount_ == maxLockCount;
> }
>
> void Agc::housekeepConfig()
> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> index b7122de3..aaf77c8f 100644
> --- a/src/ipa/rpi/controller/rpi/agc.h
> +++ b/src/ipa/rpi/controller/rpi/agc.h
> @@ -85,7 +85,7 @@ public:
> void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
>
> private:
> - void updateLockStatus(DeviceStatus const &deviceStatus);
> + bool updateLockStatus(DeviceStatus const &deviceStatus);
> AgcConfig config_;
> void housekeepConfig();
> void fetchCurrentExposure(Metadata *imageMetadata);
> diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp
> index 3eea40a6..1de0d3cc 100644
> --- a/src/ipa/rpi/vc4/vc4.cpp
> +++ b/src/ipa/rpi/vc4/vc4.cpp
> @@ -60,7 +60,7 @@ private:
> bool validateIspControls();
>
> void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
> - void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
> + void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls);
> void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);
> void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);
> void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);
> @@ -142,7 +142,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,
> if (ccmStatus)
> applyCCM(ccmStatus, ctrls);
>
> - AgcStatus *dgStatus = rpiMetadata.getLocked<AgcStatus>("agc.status");
> + AgcPrepareStatus *dgStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
> if (dgStatus)
> applyDG(dgStatus, ctrls);
>
> @@ -284,7 +284,7 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> static_cast<int32_t>(awbStatus->gainB * 1000));
> }
>
> -void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls)
> {
> ctrls.set(V4L2_CID_DIGITAL_GAIN,
> static_cast<int32_t>(dgStatus->digitalGain * 1000));
> --
> 2.30.2
>
More information about the libcamera-devel
mailing list