[libcamera-devel] [PATCH v2 5/5] ipa: rpi: agc: Use channel constraints in the AGC algorithm
David Plowman
david.plowman at raspberrypi.com
Mon Sep 11 17:22:53 CEST 2023
Hi Jacopo
Thanks for the review!
On Wed, 30 Aug 2023 at 09:28, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi David
>
> On Wed, Aug 23, 2023 at 01:09:15PM +0100, David Plowman via libcamera-devel wrote:
> > Whenever we run Agc::process(), we store the most recent total
> > exposure requested for each channel.
> >
> > With these values we can apply the channel constraints after
> > time-filtering the requested total exposure, but before working out
> > how much digital gain is needed.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > src/ipa/rpi/controller/rpi/agc.cpp | 21 ++++--
> > src/ipa/rpi/controller/rpi/agc.h | 1 +
> > src/ipa/rpi/controller/rpi/agc_channel.cpp | 75 +++++++++++++++++++---
> > src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-
> > 4 files changed, 88 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> > index 7e627bba..3c730d4c 100644
> > --- a/src/ipa/rpi/controller/rpi/agc.cpp
> > +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> > @@ -40,6 +40,7 @@ int Agc::read(const libcamera::YamlObject ¶ms)
> > */
> > if (!params.contains("channels")) {
> > LOG(RPiAgc, Debug) << "Single channel only";
> > + channelTotalExposures_.resize(1, 0s);
> > channelData_.emplace_back();
> > return channelData_.back().channel.read(params, getHardwareConfig());
> > }
> > @@ -59,6 +60,8 @@ int Agc::read(const libcamera::YamlObject ¶ms)
> > return -1;
> > }
> >
> > + channelTotalExposures_.resize(channelData_.size(), 0s);
> > +
> > return 0;
> > }
> >
> > @@ -234,15 +237,21 @@ static void getChannelIndex(Metadata *metadata, const char *message, unsigned in
> > LOG(RPiAgc, Debug) << message;
> > }
> >
> > -static void setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)
> > +static libcamera::utils::Duration
> > +setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)
>
> This function does a few different things.. but hey, you maintain this
> code so...
>
> > {
> > std::unique_lock<RPiController::Metadata> lock(*metadata);
> > AgcStatus *status = metadata->getLocked<AgcStatus>("agc.status");
> > - if (status)
> > + libcamera::utils::Duration dur = 0s;
> > +
> > + if (status) {
> > status->channel = channelIndex;
> > - else
> > + dur = status->totalExposureValue;
> > + } else
> > /* This does happen at startup, otherwise it would be a Warning or Error. */
> > LOG(RPiAgc, Debug) << message;
> > +
> > + return dur;
> > }
> >
> > void Agc::prepare(Metadata *imageMetadata)
> > @@ -306,8 +315,10 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
> > /* Can also happen when new channels start. */
> > LOG(RPiAgc, Debug) << "process: channel " << channelIndex << " not seen yet";
> >
> > - channelData.channel.process(stats, &deviceStatus, imageMetadata);
> > - setChannelIndex(imageMetadata, "process: no AGC status found", channelIndex);
> > + channelData.channel.process(stats, &deviceStatus, imageMetadata, channelTotalExposures_);
> > + auto dur = setChannelIndex(imageMetadata, "process: no AGC status found", channelIndex);
> > + if (dur)
> > + channelTotalExposures_[channelIndex] = dur;
> >
> > /* And onto the next channel for the next call. */
> > index_ = (index_ + 1) % activeChannels_.size();
> > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> > index 2eed2bab..e301c5cb 100644
> > --- a/src/ipa/rpi/controller/rpi/agc.h
> > +++ b/src/ipa/rpi/controller/rpi/agc.h
> > @@ -54,6 +54,7 @@ private:
> > std::vector<AgcChannelData> channelData_;
> > std::vector<unsigned int> activeChannels_;
> > unsigned int index_; /* index into the activeChannels_ */
> > + AgcChannelTotalExposures channelTotalExposures_;
> > };
> >
> > } /* namespace RPiController */
> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > index 44198c2c..54fd08e8 100644
> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > @@ -175,6 +175,11 @@ readConstraintModes(std::map<std::string, AgcConstraintMode> &constraintModes,
> >
> > int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)
> > {
> > + auto channelValue = params["channel"].get<unsigned int>();
> > + if (!channelValue)
> > + return -EINVAL;
>
> malformed config needs a message ?
Yes, agree.
>
> > + channel = *channelValue;
> > +
>
> Should this be part of the previous patch ?
Indeed, will move it.
>
> > std::string boundString = params["bound"].get<std::string>("");
> > transform(boundString.begin(), boundString.end(),
> > boundString.begin(), ::toupper);
> > @@ -184,10 +189,10 @@ int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)
> > }
> > bound = boundString == "UPPER" ? Bound::UPPER : Bound::LOWER;
> >
> > - auto value = params["factor"].get<double>();
> > - if (!value)
> > + auto factorValue = params["factor"].get<double>();
> > + if (!factorValue)
> > return -EINVAL;
> > - factor = *value;
> > + factor = *factorValue;
>
> Maybe squash in the previous one ?
Yes.
>
> >
> > return 0;
> > }
> > @@ -231,6 +236,7 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)
> > ret = readChannelConstraints(channelConstraints, params["channel_constraints"]);
> > if (ret)
> > return ret;
> > + LOG(RPiAgc, Info) << "Read " << channelConstraints.size() << " channel constraints";
>
> ditto
Agree. Or perhaps I should remove it altogether, none of the others
print out how many "things" they read...
>
> > }
> >
> > ret = yTarget.read(params["y_target"]);
> > @@ -489,7 +495,9 @@ void AgcChannel::prepare(Metadata *imageMetadata)
> > }
> > }
> >
> > -void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata)
> > +void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus,
> > + Metadata *imageMetadata,
> > + const AgcChannelTotalExposures &channelTotalExposures)
> > {
> > frameCount_++;
> > /*
> > @@ -508,12 +516,17 @@ void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus,
> > computeTargetExposure(gain);
> > /* The results have to be filtered so as not to change too rapidly. */
> > filterExposure();
> > + /*
> > + * We may be asked to limit the exposure using other channels. If another channel
> > + * determines our upper bound we may want to know this later.
> > + */
> > + bool channelBound = applyChannelConstraints(channelTotalExposures);
> > /*
> > * Some of the exposure has to be applied as digital gain, so work out
> > - * what that is. This function also tells us whether it's decided to
> > - * "desaturate" the image more quickly.
> > + * what that is. It also tells us whether it's trying to desaturate the image
> > + * more quickly, which can only happen when another channel is not limiting us.
> > */
> > - bool desaturate = applyDigitalGain(gain, targetY);
> > + bool desaturate = applyDigitalGain(gain, targetY, channelBound);
> > /*
> > * The last thing is to divide up the exposure value into a shutter time
> > * and analogue gain, according to the current exposure mode.
> > @@ -792,7 +805,49 @@ void AgcChannel::computeTargetExposure(double gain)
> > LOG(RPiAgc, Debug) << "Target totalExposure " << target_.totalExposure;
> > }
> >
> > -bool AgcChannel::applyDigitalGain(double gain, double targetY)
> > +bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures)
> > +{
> > + bool channelBound = false;
> > + LOG(RPiAgc, Debug)
> > + << "Total exposure before channel constraints " << filtered_.totalExposure;
> > +
> > + for (const auto &constraint : config_.channelConstraints) {
>
> What's the logic here ? We are operating on one channel, how many
> constraints can we have for a single channel ? Why are we iterating
> config_.channelConstraints, shouldn't we consider only constraints for
> this channel ?
So these are constraints where other AGC channels constrain this
channel. For example, this might be channel 2, and we might have one
constraint that says "don't be more than 8x the exposure of channel 0"
and another that says "you must less than half the exposure of channel
1".
Obviously I don't have any specific requirement for anything as
complex as that myself, but it's infrastructure that folks can use if
they are very advanced!
>
> > + LOG(RPiAgc, Debug)
> > + << "Check constraint: channel " << constraint.channel << " bound "
> > + << (constraint.bound == AgcChannelConstraint::Bound::UPPER ? "UPPER" : "LOWER")
> > + << " factor " << constraint.factor;
> > + if (constraint.channel >= channelTotalExposures.size() ||
> > + !channelTotalExposures[constraint.channel]) {
> > + LOG(RPiAgc, Debug) << "no such channel - skipped";
> > + continue;
> > + }
> > +
> > + if (channelTotalExposures[constraint.channel] == 0s) {
>
> isn't this "!channelTotalExposures[constraint.channel]" tested in the
> previous if () ?
You are right - well spotted!!
>
>
> > + LOG(RPiAgc, Debug) << "zero exposure - skipped";
> > + continue;
> > + }
> > +
> > + libcamera::utils::Duration limitExposure =
> > + channelTotalExposures[constraint.channel] * constraint.factor;
> > + LOG(RPiAgc, Debug) << "Limit exposure " << limitExposure;
> > + if ((constraint.bound == AgcChannelConstraint::Bound::UPPER &&
> > + filtered_.totalExposure > limitExposure) ||
> > + (constraint.bound == AgcChannelConstraint::Bound::LOWER &&
> > + filtered_.totalExposure < limitExposure)) {
> > + filtered_.totalExposure = limitExposure;
> > + LOG(RPiAgc, Debug) << "Applies";
>
> What applies ? :)
"This constraint", of course! :)
But I'll improve the message!
>
> > + channelBound = true;
> > + } else
> > + LOG(RPiAgc, Debug) << "Does not apply";
>
> and what doesn't apply ? :)
As above. Will improve the message.
Thanks again for all the reviewing, it's very helpful.
David
>
> > + }
> > +
> > + LOG(RPiAgc, Debug)
> > + << "Total exposure after channel constraints " << filtered_.totalExposure;
> > +
> > + return channelBound;
> > +}
> > +
> > +bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound)
> > {
> > double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 });
> > ASSERT(minColourGain != 0.0);
> > @@ -812,8 +867,8 @@ bool AgcChannel::applyDigitalGain(double gain, double targetY)
> > * quickly (and we then approach the correct value more quickly from
> > * below).
> > */
> > - bool desaturate = targetY > config_.fastReduceThreshold &&
> > - gain < sqrt(targetY);
> > + bool desaturate = !channelBound &&
> > + targetY > config_.fastReduceThreshold && gain < sqrt(targetY);
> > if (desaturate)
> > dg /= config_.fastReduceThreshold;
> > LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate;
> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> > index 446125ef..c3f701eb 100644
> > --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> > @@ -19,6 +19,8 @@
> >
> > namespace RPiController {
> >
> > +using AgcChannelTotalExposures = std::vector<libcamera::utils::Duration>;
> > +
> > struct AgcMeteringMode {
> > std::vector<double> weights;
> > int read(const libcamera::YamlObject ¶ms);
> > @@ -93,7 +95,8 @@ public:
> > void disableAuto();
> > void switchMode(CameraMode const &cameraMode, Metadata *metadata);
> > void prepare(Metadata *imageMetadata);
> > - void process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata);
> > + void process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata,
> > + const AgcChannelTotalExposures &channelTotalExposures);
> >
> > private:
> > bool updateLockStatus(DeviceStatus const &deviceStatus);
> > @@ -105,7 +108,8 @@ private:
> > double &gain, double &targetY);
> > void computeTargetExposure(double gain);
> > void filterExposure();
> > - bool applyDigitalGain(double gain, double targetY);
> > + bool applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures);
> > + bool applyDigitalGain(double gain, double targetY, bool channelBound);
> > void divideUpExposure();
> > void writeAndFinish(Metadata *imageMetadata, bool desaturate);
> > libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);
> > --
> > 2.30.2
> >
More information about the libcamera-devel
mailing list