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

19895 diff mbox series

From: Paul Elder <paul.elder@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@ideasonboard.com>
Signed-off-by: Daniel Scally <dan.scally@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

Hi Dan,

On Wed, Apr 17, 2024 at 02:15:31PM +0100, Daniel Scally wrote:
> From: Paul Elder <paul.elder@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@ideasonboard.com>
> Signed-off-by: Daniel Scally <dan.scally@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@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.
> + * 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
> + * algorithm's .init() call).
> + *
> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
> + * analogue and digital gain.
> + *
> + * 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;
> +	}
> +
> +	for (auto stage : stages) {
> +		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
> + *
> + * 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*

s/these/this/

> + * 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.
> + */
> +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,
> +					     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
> + * digital gain by iterating through sets of shutter and gain limits. At each
> + * 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

typo "splitExpothe"?

> + * 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.
> + *
> + * When a combination of shutter and gain _stage_ limits are found that are
> + * 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
> + * 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) };
> +		}

This is really nice and clean but if I'm not mistaken you're ignoring
fixed gain and fixed shutter...


Paul

> +	}
> +
> +	/*
> +	 * 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()
> + * \return The minShutter_ value
> + */
> +
> +/**
> + * \fn ExposureModeHelper::maxShutter()
> + * \brief Retrieve the configured maximum shutter time set through
> + *        setShutterGainLimits()
> + * \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@ideasonboard.com>
> + *
> + * exposure_mode_helper.h - Helper class that performs computations relating to exposure
> + */
> +
> +#pragma once
> +
> +#include <tuple>
> +#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
>

Hi Paul

On 18/04/2024 12:35, Paul Elder wrote:
> Hi Dan,
>
> On Wed, Apr 17, 2024 at 02:15:31PM +0100, Daniel Scally wrote:
>> From: Paul Elder <paul.elder@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@ideasonboard.com>
>> Signed-off-by: Daniel Scally <dan.scally@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@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.
>> + * 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
>> + * algorithm's .init() call).
>> + *
>> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
>> + * analogue and digital gain.
>> + *
>> + * 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;
>> +	}
>> +
>> +	for (auto stage : stages) {
>> +		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
>> + *
>> + * 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*
> s/these/this/
>
>> + * 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.
>> + */
>> +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,
>> +					     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
>> + * digital gain by iterating through sets of shutter and gain limits. At each
>> + * 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
> typo "splitExpothe"?
Argh, I've actually got a whole section duplicated there!
>
>> + * 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.
>> + *
>> + * When a combination of shutter and gain _stage_ limits are found that are
>> + * 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
>> + * 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) };
>> +		}
> This is really nice and clean but if I'm not mistaken you're ignoring
> fixed gain and fixed shutter...
That should just be accounted for by the fact that clampShutter() and clampGain() will fix the value 
at that fixed shutter/gain - at least that's the idea. I tested by comparing the original 
implementation to this one, including with fixed shutter/gain/both and the calculated shutter time 
and gains matched.
>
> Paul
>
>> +	}
>> +
>> +	/*
>> +	 * 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()
>> + * \return The minShutter_ value
>> + */
>> +
>> +/**
>> + * \fn ExposureModeHelper::maxShutter()
>> + * \brief Retrieve the configured maximum shutter time set through
>> + *        setShutterGainLimits()
>> + * \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@ideasonboard.com>
>> + *
>> + * exposure_mode_helper.h - Helper class that performs computations relating to exposure
>> + */
>> +
>> +#pragma once
>> +
>> +#include <tuple>
>> +#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
>>

On Thu, Apr 18, 2024 at 12:48:13PM +0100, Dan Scally wrote:
> Hi Paul
> 
> On 18/04/2024 12:35, Paul Elder wrote:
> > Hi Dan,
> > 
> > On Wed, Apr 17, 2024 at 02:15:31PM +0100, Daniel Scally wrote:
> > > From: Paul Elder <paul.elder@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@ideasonboard.com>
> > > Signed-off-by: Daniel Scally <dan.scally@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@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.
> > > + * 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
> > > + * algorithm's .init() call).
> > > + *
> > > + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
> > > + * analogue and digital gain.
> > > + *
> > > + * 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;
> > > +	}
> > > +
> > > +	for (auto stage : stages) {
> > > +		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
> > > + *
> > > + * 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*
> > s/these/this/
> > 
> > > + * 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.
> > > + */
> > > +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,
> > > +					     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
> > > + * digital gain by iterating through sets of shutter and gain limits. At each
> > > + * 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
> > typo "splitExpothe"?
> Argh, I've actually got a whole section duplicated there!
> > 
> > > + * 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.
> > > + *
> > > + * When a combination of shutter and gain _stage_ limits are found that are
> > > + * 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
> > > + * 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) };
> > > +		}
> > This is really nice and clean but if I'm not mistaken you're ignoring
> > fixed gain and fixed shutter...
> That should just be accounted for by the fact that clampShutter() and
> clampGain() will fix the value at that fixed shutter/gain - at least that's
> the idea. I tested by comparing the original implementation to this one,
> including with fixed shutter/gain/both and the calculated shutter time and
> gains matched.

Ah, indeed you're right. That's efficient.

It feels weird rb-ing a patch that I authored but you've changed it
enough imo...

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> > 
> > > +	}
> > > +
> > > +	/*
> > > +	 * 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()
> > > + * \return The minShutter_ value
> > > + */
> > > +
> > > +/**
> > > + * \fn ExposureModeHelper::maxShutter()
> > > + * \brief Retrieve the configured maximum shutter time set through
> > > + *        setShutterGainLimits()
> > > + * \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@ideasonboard.com>
> > > + *
> > > + * exposure_mode_helper.h - Helper class that performs computations relating to exposure
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <tuple>
> > > +#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
> > >

Hi Dan

On Wed, Apr 17, 2024 at 02:15:31PM +0100, Daniel Scally wrote:
> From: Paul Elder <paul.elder@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@ideasonboard.com>
> Signed-off-by: Daniel Scally <dan.scally@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@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 ?

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

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

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

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

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

> + * digital gain by iterating through sets of shutter and gain limits. At each

does this iterate 'limits' or the stages populated at init() time ?

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

'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

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

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

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@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@ideasonboard.com>
>> Signed-off-by: Daniel Scally <dan.scally@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@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

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

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

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



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

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

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@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@ideasonboard.com>
> > > Signed-off-by: Daniel Scally <dan.scally@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@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@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
> > >

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@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@ideasonboard.com>
>>>> Signed-off-by: Daniel Scally <dan.scally@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@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@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
>>>>

Hi


2024. április 17., szerda 15:15 keltezéssel, Daniel Scally <dan.scally@ideasonboard.com> írta:

> From: Paul Elder <paul.elder@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@ideasonboard.com>
> Signed-off-by: Daniel Scally <dan.scally@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
> 
> [...]
> +/**
> + * \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
> + * algorithm's .init() call).
> + *
> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
> + * analogue and digital gain.
> + *
> + * 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)

Any reason for not using span here?


> +{
> +	/* We only need to check shutters_, as gains_ is filled alongside it */
> +	if (!shutters_.empty()) {

Any reason for not doing what init() does in the constructor?


> +		LOG(ExposureModeHelper, Warning)
> +			<< "Initialization attempted multiple times";
> +		return;
> +	}
> +
> +	for (auto stage : stages) {

You can use something like `const auto &[s, g] : stages`.


> +		shutters_.push_back(stage.first);
> +		gains_.push_back(stage.second);
> +	}
> +}
> [...]


Regards,
Barnabás Pőcze

Hi Barnabas

On 23/04/2024 15:29, Barnabás Pőcze wrote:
> Hi
>
>
> 2024. április 17., szerda 15:15 keltezéssel, Daniel Scally <dan.scally@ideasonboard.com> írta:
>
>> From: Paul Elder <paul.elder@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@ideasonboard.com>
>> Signed-off-by: Daniel Scally <dan.scally@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
>>
>> [...]
>> +/**
>> + * \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
>> + * algorithm's .init() call).
>> + *
>> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
>> + * analogue and digital gain.
>> + *
>> + * 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)
> Any reason for not using span here?


The shutter time and gain vectors are initially separate, and need combining like so:


+ std::vector<std::pair<utils::Duration, double>> stages; + for (unsigned int i = 0; i < 
shutters.size(); i++) { + stages.push_back({ + std::chrono::microseconds(shutters[i]), + gains[i] + 
}); + } + + std::shared_ptr<ExposureModeHelper> helper = + std::make_shared<ExposureModeHelper>(); + 
helper->init(stages); So given I have to build a new vector with the paired stages anyway I couldn't 
see value in then using a Span for the init() instead of just passing the vector...but quite 
possibly I am just not understanding why it's the better choice here.

>
>
>> +{
>> +	/* We only need to check shutters_, as gains_ is filled alongside it */
>> +	if (!shutters_.empty()) {
> Any reason for not doing what init() does in the constructor?


No; v3 does that.

>
>
>> +		LOG(ExposureModeHelper, Warning)
>> +			<< "Initialization attempted multiple times";
>> +		return;
>> +	}
>> +
>> +	for (auto stage : stages) {
> You can use something like `const auto &[s, g] : stages`.


thanks

>
>
>> +		shutters_.push_back(stage.first);
>> +		gains_.push_back(stage.second);
>> +	}
>> +}
>> [...]
>
> Regards,
> Barnabás Pőcze

Hi


2024. április 23., kedd 16:41 keltezéssel, Dan Scally <dan.scally@ideasonboard.com> írta:

> [...]
> >> [...]
> >> +/**
> >> + * \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
> >> + * algorithm's .init() call).
> >> + *
> >> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
> >> + * analogue and digital gain.
> >> + *
> >> + * 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)
> > Any reason for not using span here?
> 
> 
> The shutter time and gain vectors are initially separate, and need combining like so:
> 
> 
>  std::vector<std::pair<utils::Duration, double>> stages;
>  for (unsigned int i = 0; i < shutters.size(); i++) {
>    stages.push_back({
>      std::chrono::microseconds(shutters[i]),
>      gains[i]
>    });
>  }
> 
>  std::shared_ptr<ExposureModeHelper> helper =
>    std::make_shared<ExposureModeHelper>();
>  helper->init(stages);
> 
> So given I have to build a new vector with the paired stages anyway I couldn't
> see value in then using a Span for the init() instead of just passing the vector...but quite
> possibly I am just not understanding why it's the better choice here.

Note that in the code above, there is a copy when calling init(), using a span avoids that.


> [...]


Regards,
Barnabás Pőcze

Hi

On Tue, Apr 23, 2024 at 03:01:34PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2024. április 23., kedd 16:41 keltezéssel, Dan Scally <dan.scally@ideasonboard.com> írta:
>
> > [...]
> > >> [...]
> > >> +/**
> > >> + * \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
> > >> + * algorithm's .init() call).
> > >> + *
> > >> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
> > >> + * analogue and digital gain.
> > >> + *
> > >> + * 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)
> > > Any reason for not using span here?
> >
> >
> > The shutter time and gain vectors are initially separate, and need combining like so:
> >
> >
> >  std::vector<std::pair<utils::Duration, double>> stages;
> >  for (unsigned int i = 0; i < shutters.size(); i++) {
> >    stages.push_back({
> >      std::chrono::microseconds(shutters[i]),
> >      gains[i]
> >    });
> >  }
> >
> >  std::shared_ptr<ExposureModeHelper> helper =
> >    std::make_shared<ExposureModeHelper>();
> >  helper->init(stages);
> >
> > So given I have to build a new vector with the paired stages anyway I couldn't
> > see value in then using a Span for the init() instead of just passing the vector...but quite
> > possibly I am just not understanding why it's the better choice here.
>
> Note that in the code above, there is a copy when calling init(), using a span avoids that.
>

Or just by passing the vector by reference and not value.
The pairs' items will be copied inside the helper anyway.

>
> > [...]
>
>
> Regards,
> Barnabás Pőcze

On Wed, Apr 24, 2024 at 06:59:24PM +0200, Jacopo Mondi wrote:
> On Tue, Apr 23, 2024 at 03:01:34PM +0000, Barnabás Pőcze wrote:
> > 2024. április 23., kedd 16:41 keltezéssel, Dan Scally írta:
> >
> > > [...]
> > > >> [...]
> > > >> +/**
> > > >> + * \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
> > > >> + * algorithm's .init() call).
> > > >> + *
> > > >> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both
> > > >> + * analogue and digital gain.
> > > >> + *
> > > >> + * 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)
> > > > Any reason for not using span here?
> > >
> > >
> > > The shutter time and gain vectors are initially separate, and need combining like so:
> > >
> > >
> > >  std::vector<std::pair<utils::Duration, double>> stages;
> > >  for (unsigned int i = 0; i < shutters.size(); i++) {
> > >    stages.push_back({
> > >      std::chrono::microseconds(shutters[i]),
> > >      gains[i]
> > >    });
> > >  }
> > >
> > >  std::shared_ptr<ExposureModeHelper> helper =
> > >    std::make_shared<ExposureModeHelper>();
> > >  helper->init(stages);
> > >
> > > So given I have to build a new vector with the paired stages anyway I couldn't
> > > see value in then using a Span for the init() instead of just passing the vector...but quite
> > > possibly I am just not understanding why it's the better choice here.
> >
> > Note that in the code above, there is a copy when calling init(), using a span avoids that.
> 
> Or just by passing the vector by reference and not value.
> The pairs' items will be copied inside the helper anyway.

If we foresee that the caller won't need to retain access to the vector,
passing an rvalue reference would be more efficient and avoid a copy.

> > > [...]