[libcamera-devel] [PATCH v2 5/5] ipa: rpi: agc: Use channel constraints in the AGC algorithm
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Aug 30 10:28:20 CEST 2023
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 ?
> + channel = *channelValue;
> +
Should this be part of the previous patch ?
> 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 ?
>
> 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
> }
>
> 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 ?
> + 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 () ?
> + 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 ? :)
> + channelBound = true;
> + } else
> + LOG(RPiAgc, Debug) << "Does not apply";
and what doesn't apply ? :)
> + }
> +
> + 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