[libcamera-devel] [PATCH 3/3] ipa: rpi: agc: Split AgcStatus into AgcStatus and AgcPrepareStatus
David Plowman
david.plowman at raspberrypi.com
Mon Sep 11 11:25:37 CEST 2023
Ah, I see it's already been merged. All good then!
David
On Mon, 11 Sept 2023 at 10:21, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi Jacopo
>
> Thanks for reviewing this series!
>
> On Tue, 29 Aug 2023 at 15:01, Jacopo Mondi
> <jacopo.mondi at 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 at raspberrypi.com>
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at 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
> > >
More information about the libcamera-devel
mailing list