[PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper

[Jacopo Mondi]

Hi Dan

On Tue, Apr 23, 2024 at 10:16:31AM +0100, Dan Scally wrote:
> Hi Jacopo
>
> On 19/04/2024 15:10, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Wed, Apr 17, 2024 at 02:15:31PM +0100, Daniel Scally wrote:
> > > From: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > Add a helper for managing exposure modes and splitting exposure times
> > > into shutter and gain values.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > > ---
> > > Changes in v2:
> > >
> > > 	- Expanded the documentation
> > > 	- Dropped the overloads for fixed shutter / gain - the same
> > > 	  functionality is instead done by setting min and max shutter and gain
> > > 	  to the same value
> > > 	- Changed ::init() to consume a vector of pairs instead of two separate
> > > 	  vectors
> > > 	- Reworked splitExposure()
> > >
> > >   src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++
> > >   src/ipa/libipa/exposure_mode_helper.h   |  53 +++++
> > >   src/ipa/libipa/meson.build              |   2 +
> > >   3 files changed, 312 insertions(+)
> > >   create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp
> > >   create mode 100644 src/ipa/libipa/exposure_mode_helper.h
> > >
> > > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > > new file mode 100644
> > > index 00000000..6463de18
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > > @@ -0,0 +1,257 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> > > + *
> > > + * exposure_mode_helper.cpp - Helper class that performs computations relating to exposure
> > > + */
> > > +#include "exposure_mode_helper.h"
> > > +
> > > +#include <algorithm>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +/**
> > > + * \file exposure_mode_helper.h
> > > + * \brief Helper class that performs computations relating to exposure
> > > + *
> > > + * AEGC algorithms have a need to split exposure between shutter and gain.
> > s/shutter/shutter time ?
> >
> > This and in the other occurences
> >
> > also, "gain" which gain ? Analogue, digital or a combination of the
> > two ?
>
>
> Both
>

Maybe specify it then ? :)

> >
> > > + * Multiple implementations do so based on paired sets of shutter and gain
> > > + * limits; provide a helper to avoid duplicating the code.
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(ExposureModeHelper)
> > > +
> > > +namespace ipa {
> > > +
> > > +/**
> > > + * \class ExposureModeHelper
> > > + * \brief Class for splitting exposure into shutter and gain
> > > + *
> > > + * The ExposureModeHelper class provides a standard interface through which an
> > > + * AEGC algorithm can divide exposure between shutter and gain. It is configured
> > > + * with a set of shutter and gain pairs and works by initially fixing gain at
> > > + * 1.0 and increasing shutter up to the shutter value from the first pair in the
> > > + * set in an attempt to meet the required exposure value.
> > > + *
> > > + * If the required exposure is not achievable by the first shutter value alone
> > > + * it ramps gain up to the value from the first pair in the set. If the required
> > > + * exposure is still not met it then allows shutter to ramp up to the shutter
> > > + * value from the second pair in the set, and continues in this vein until
> > > + * either the required exposure value is met, or else the hardware's shutter or
> > > + * gain limits are reached.
> > > + *
> > > + * This method allows users to strike a balance between a well-exposed image and
> > > + * an acceptable frame-rate, as opposed to simply maximising shutter followed by
> > > + * gain. The same helpers can be used to perform the latter operation if needed
> > > + * by passing an empty set of pairs to the initialisation function.
> > > + *
> > > + * The gain values may exceed a camera sensor's analogue gain limits if either
> > > + * it or the IPA is also capable of digital gain. The configure() function must
> > > + * be called with the hardware's limits to inform the helper of those
> > > + * constraints. Any gain that is needed will be applied as analogue gain first
> > > + * until the hardware's limit is reached, following which digital gain will be
> > > + * used.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct an ExposureModeHelper instance
> > > + */
> > > +ExposureModeHelper::ExposureModeHelper()
> > > +	: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Initialize an ExposureModeHelper instance
> > > + * \param[in] stages The vector of paired shutter time and gain limits
> > > + *
> > > + * This function is expected to be called a single time once the algorithm that
> > > + * is using these helpers has built the necessary list of shutter and gain pairs
> > > + * to use (archetypically by parsing them from a tuning file during the
> > Is archetypically intentional ? Isn't "typically" enough ?
>
>
> It's intentional yes; "archetypically" is like "in the first implementation"
> whereas "typically" is like "in most implementations"...but I can switch to
> typically so it's less confusing, either works.
>

as long as it's intentional and makes sense to a native speaker, I'm
fine!

> >
> > > + * algorithm's .init() call).
> > > + *
> > > + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
> > > + * analogue and digital gain.
> > > + *
> > Shutter -time- ?
> >
> > And yes, now gain is better explained.
> >
> > > + * The vector of stages may be empty. In that case, the helper will simply use
> > > + * the runtime limits set through setShutterGainLimits() instead.
> > > + */
> > > +void ExposureModeHelper::init(const std::vector<std::pair<utils::Duration, double>> stages)
> > > +{
> > > +	/* We only need to check shutters_, as gains_ is filled alongside it */
> > > +	if (!shutters_.empty()) {
> > > +		LOG(ExposureModeHelper, Warning)
> > > +			<< "Initialization attempted multiple times";
> > > +		return;
> > As we do this in a function instead of a constructor, we can return a
> > value. Is this useful for users of this class ?
>
>
> I don't think there's any need to return anything in this function if that's what you mean
>

But this is an error condition which means the class user is not
respecting the intended API usage. Can we make the above Fatal ?

> >
> > > +	}
> > > +
> > > +	for (auto stage : stages) {
> > auto const &
> >
> > > +		shutters_.push_back(stage.first);
> > > +		gains_.push_back(stage.second);
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * \brief Set the shutter and gain limits
> > > + * \param[in] minShutter The minimum shutter time supported
> > > + * \param[in] maxShutter The maximum shutter time supported
> > > + * \param[in] minGain The minimum analogue gain supported
> > > + * \param[in] maxGain The maximum analogue gain supported
> > Ah so this is only analogue ? but the steps are the total gain ?
>
>
> Correct
>
> >
> > > + *
> > > + * This function configures the shutter and analogue gain limits that need to be
> > > + * adhered to as the helper divides up exposure. Note that these function *must*
> > > + * be called whenever those limits change and before splitExposure() is used.
> > > + *
> > > + * If the algorithm using the helpers needs to indicate that either shutter,
> > > + * analogue gain or both should be fixed it can do so by setting both the minima
> > > + * and maxima to the same value.
> > Are minima and maxima intentional ? Isn't this minimum and maximum ?
>
>
> Minima/maxima is the plural, meaning to refer to both "minShutter" and "minGain" for example
>
> >
> > > + */
> > > +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,
> > Or just setLimits()
> >
> > > +					     utils::Duration maxShutter,
> > > +					     double minGain,
> > > +					     double maxGain)
> > > +{
> > > +	minShutter_ = minShutter;
> > > +	maxShutter_ = maxShutter;
> > > +	minGain_ = minGain;
> > > +	maxGain_ = maxGain;
> > > +}
> > > +
> > > +utils::Duration ExposureModeHelper::clampShutter(utils::Duration shutter) const
> > > +{
> > > +	return std::clamp(shutter, minShutter_, maxShutter_);
> > > +}
> > > +
> > > +double ExposureModeHelper::clampGain(double gain) const
> > > +{
> > > +	return std::clamp(gain, minGain_, maxGain_);
> > > +}
> > > +
> > > +/**
> > > + * \brief Split exposure time into shutter and gain
> > > + * \param[in] exposure Exposure time
> > > + *
> > > + * This function divides a given exposure value into shutter time, analogue and
> > exposure 'value' or exposure 'time' as in the function's brief ?
>
>
> It's Exposure time rather than value, if by Exposure value you mean:
> https://en.wikipedia.org/wiki/Exposure_value
>

The 'Exposure' here is a combination of the shutter time multiplied by
a gain and to me it represents an absolute value ? I just find
confusing that an "Exposure time" is divided in "shutter time" and
"gain". But indeed it might be only me, maybe check what others think ?

My main point however was: in \brief it is reported as "exposure time"
and in the extended description it's reported as "exposure value". I
would try to use one of the two consistently throughout the
documentation.


>
>
> >
> > > + * digital gain by iterating through sets of shutter and gain limits. At each
> > does this iterate 'limits' or the stages populated at init() time ?
>
>
> The limits parsed from tuning data
>
> >
> > > + * stage the current stage's shutter limit is multiplied by the previous stage's
> > > + * gain limit (or 1.0 initially) to see if the combination of the two can meet
> > > + * the required exposure value. If they cannot then splitExpothe current stage's shutter
> > Exposure definitely seems to be a value then (and this matches my
> > understanding)
>
>
> I just mean the value of the exposure parameter; the input is the exposure
> time needed to attain the target
>
> >
> > 'splitExposthe' ?
> >
> >
> > > + * limit is multiplied by the same stage's gain limit to see if that combination
> > > + * can meet the required exposure value. If they cannot then the function moves
> > > + * to consider the next stage.
> > I think something went wrong with formatting here
> >
> > > + *
> > > + * When a combination of shutter and gain _stage_ limits are found that are
> > why '_stage_' ? and why limits ?
> >
> > Can't it be simply:
> >
> >   * When a combination of shutter time and gain are found to be sufficient
>
>
> I was trying to make it clear that it's working on the limits from the sets
> parsed from tuning data rather than the runtime limits set through
> setShutterGainLimits.
>
> >
> > > + * sufficient to meet the required exposure value, the function attempts to
> > > + * reduce shutter time as much as possible whilst fixing gain and still meeting
> > > + * the exposure value. If a _runtime_ limit prevents shutter time from being
> > Again, why _runtime_ ?
>
>
> And here trying to emphasis that it's the limits from setShutterGainLimits
> rather than those parsed from Yaml
>

_this_ gets rendered as italic in Doxygen, so it's fine if you want to
emphasis it


> >
> > > + * lowered enough to meet the exposure value with gain fixed at the stage limit,
> > > + * gain is also lowered to compensate.
> > > + *
> > > + * Once the shutter time and gain values are ascertained, gain is assigned as
> > > + * analogue gain as much as possible, with digital gain only in use if the
> > > + * maximum analogue gain runtime limit is unable to accomodate the exposure
> > > + * value.
> > > + *
> > > + * If no combination of shutter and gain limits is found that meets the required
> > > + * exposure value, the helper falls-back to simply maximising the shutter time
> > > + * first, followed by analogue gain, followed by digital gain.
> > > + *
> > > + * @return Tuple of shutter time, analogue gain, and digital gain
> > > + */
> > > +std::tuple<utils::Duration, double, double>
> > > +ExposureModeHelper::splitExposure(utils::Duration exposure) const
> > > +{
> > > +	ASSERT(maxShutter_);
> > > +	ASSERT(maxGain_);
> > > +
> > > +	bool gainFixed = minGain_ == maxGain_;
> > > +	bool shutterFixed = minShutter_ == maxShutter_;
> > > +
> > > +	/*
> > > +	 * There's no point entering the loop if we cannot change either gain
> > > +	 * nor shutter anyway.
> > > +	 */
> > > +	if (shutterFixed && gainFixed)
> > > +		return { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };
> > > +
> > > +	utils::Duration shutter;
> > > +	double stageGain;
> > > +	double gain;
> > > +
> > > +	for (unsigned int stage = 0; stage < gains_.size(); stage++) {
> > > +		double lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);
> > > +		utils::Duration stageShutter = clampShutter(shutters_[stage]);
> > > +		stageGain = clampGain(gains_[stage]);
> > > +
> > > +		/*
> > > +		 * We perform the clamping on both shutter and gain in case the
> > > +		 * helper has had limits set that prevent those values being
> > > +		 * lowered beyond a certain minimum...this can happen at runtime
> > > +		 * for various reasons and so would not be known when the stage
> > > +		 * limits are initialised.
> > > +		 */
> > > +
> > > +		if (stageShutter * lastStageGain >= exposure) {
> > > +			shutter = clampShutter(exposure / clampGain(lastStageGain));
> > > +			gain = clampGain(exposure / shutter);
> > > +
> > > +			return { shutter, gain, exposure / (shutter * gain) };
> > > +		}
> > > +
> > > +		if (stageShutter * stageGain >= exposure) {
> > > +			shutter = clampShutter(exposure / clampGain(stageGain));
> > > +			gain = clampGain(exposure / shutter);
> > > +
> > > +			return { shutter, gain, exposure / (shutter * gain) };
> > > +		}
> > > +	}
> > > +
> > > +	/*
> > > +	 * From here on all we can do is max out the shutter, followed by the
> > > +	 * analogue gain. If we still haven't achieved the target we send the
> > > +	 * rest of the exposure time to digital gain. If we were given no stages
> > > +	 * to use then set stageGain to 1.0 so that shutter is maxed before gain
> > > +	 * touched at all.
> > > +	 */
> > > +	if (gains_.empty())
> > > +		stageGain = 1.0;
> > > +
> > > +	shutter = clampShutter(exposure / clampGain(stageGain));
> > > +	gain = clampGain(exposure / shutter);
> > > +
> > > +	return { shutter, gain, exposure / (shutter * gain) };
> > > +}
> > > +
> > > +/**
> > > + * \fn ExposureModeHelper::minShutter()
> > > + * \brief Retrieve the configured minimum shutter time limit set through
> > > + *        setShutterGainLimits()
> > No alignement in briefs
> >
> > > + * \return The minShutter_ value
> > > + */
> > > +
> > > +/**
> > > + * \fn ExposureModeHelper::maxShutter()
> > > + * \brief Retrieve the configured maximum shutter time set through
> > > + *        setShutterGainLimits()
> > ditto
> >
> > > + * \return The maxShutter_ value
> > > + */
> > > +
> > > +/**
> > > + * \fn ExposureModeHelper::minGain()
> > > + * \brief Retrieve the configured minimum gain set through
> > > + *        setShutterGainLimits()
> > > + * \return The minGain_ value
> > > + */
> > > +
> > > +/**
> > > + * \fn ExposureModeHelper::maxGain()
> > > + * \brief Retrieve the configured maximum gain set through
> > > + *        setShutterGainLimits()
> > > + * \return The maxGain_ value
> > > + */
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> > > new file mode 100644
> > > index 00000000..1f672135
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > > @@ -0,0 +1,53 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> > > + *
> > > + * exposure_mode_helper.h - Helper class that performs computations relating to exposure
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <tuple>
> > Missing <pair>
> >
> > > +#include <vector>
> > > +
> > > +#include <libcamera/base/utils.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa {
> > > +
> > > +class ExposureModeHelper
> > > +{
> > > +public:
> > > +	ExposureModeHelper();
> > > +	~ExposureModeHelper() = default;
> > > +
> > > +	void init(const std::vector<std::pair<utils::Duration, double>> stages);
> > > +	void setShutterGainLimits(utils::Duration minShutter,
> > > +				  utils::Duration maxShutter,
> > > +				  double minGain, double maxGain);
> > > +
> > > +	std::tuple<utils::Duration, double, double>
> > > +	splitExposure(utils::Duration exposure) const;
> > > +
> > > +	utils::Duration minShutter() const { return minShutter_; }
> > > +	utils::Duration maxShutter() const { return maxShutter_; }
> > > +	double minGain() const { return minGain_; }
> > > +	double maxGain() const { return maxGain_; }
> > > +
> > > +private:
> > > +	utils::Duration clampShutter(utils::Duration shutter) const;
> > > +	double clampGain(double gain) const;
> > > +
> > > +	std::vector<utils::Duration> shutters_;
> > > +	std::vector<double> gains_;
> > > +
> > > +	utils::Duration minShutter_;
> > > +	utils::Duration maxShutter_;
> > > +	double minGain_;
> > > +	double maxGain_;
> > > +};
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > index 016b8e0e..37fbd177 100644
> > > --- a/src/ipa/libipa/meson.build
> > > +++ b/src/ipa/libipa/meson.build
> > > @@ -3,6 +3,7 @@
> > >   libipa_headers = files([
> > >       'algorithm.h',
> > >       'camera_sensor_helper.h',
> > > +    'exposure_mode_helper.h',
> > >       'fc_queue.h',
> > >       'histogram.h',
> > >       'module.h',
> > > @@ -11,6 +12,7 @@ libipa_headers = files([
> > >   libipa_sources = files([
> > >       'algorithm.cpp',
> > >       'camera_sensor_helper.cpp',
> > > +    'exposure_mode_helper.cpp',
> > >       'fc_queue.cpp',
> > >       'histogram.cpp',
> > >       'module.cpp',
> > > --
> > > 2.34.1
> > >