| Message ID | 20230728133700.3713-4-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 250565b5e8848c80443422d9e44c0693aa381c81 |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for your work. On Fri, 28 Jul 2023 at 14:37, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> 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. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > 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 >
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@raspberrypi.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@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 >
Hi Jacopo Thanks for reviewing this series! On Tue, 29 Aug 2023 at 15:01, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > 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 ? Yes, exactly. The problem is because we generate new AE values for each channel in turn, in a round-robin fashion. And this channel is not the same channel as the frame that's just arrived. So it becomes important not to muddle together information that actually relates to different channels. > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> I'll probably repost this set with your review tags, so thanks for adding them! David > > 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 > >
Ah, I see it's already been merged. All good then! David On Mon, 11 Sept 2023 at 10:21, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi Jacopo > > Thanks for reviewing this series! > > On Tue, 29 Aug 2023 at 15:01, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > 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 ? > > Yes, exactly. The problem is because we generate new AE values for > each channel in turn, in a round-robin fashion. And this channel is > not the same channel as the frame that's just arrived. So it becomes > important not to muddle together information that actually relates to > different channels. > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > I'll probably repost this set with your review tags, so thanks for adding them! > > David > > > > > 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 > > >
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));
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. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- 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(-)