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

[Dan Scally]

Hi Jacopo

On 23/04/2024 11:19, Jacopo Mondi wrote:
> 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 ? :)


Will do!

>
>>>> + * 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 ?


Yes if that makes sense to do; I'm never sure when Fatal is the right choice.

>
>>>> +	}
>>>> +
>>>> +	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 ?


Perhaps the terminology is wrong...in the derived classes I think the combination of shutter time 
and gain that was applied for a specific frame is called the "effectiveExposureValue" [1] - perhaps 
we should use that phrase to refer to the shutter time modified by gain? Or "effective exposure time"?


[1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/algorithms/agc.cpp#n267

>
> 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.
>
>
Fair enough; thanks.
>>
>>>> + * 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


Yep that was the aim

>
>
>>>> + * 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
>>>>