[03/10] ipa: libipa: Add ExposureModeHelper

19789 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>
---
 src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++
 src/ipa/libipa/exposure_mode_helper.h   |  61 +++++
 src/ipa/libipa/meson.build              |   2 +
 3 files changed, 370 insertions(+)
 create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp
 create mode 100644 src/ipa/libipa/exposure_mode_helper.h

Hi Dan, Paul

On Fri, Mar 22, 2024 at 01:14:44PM +0000, 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>
> ---
>  src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++
>  src/ipa/libipa/exposure_mode_helper.h   |  61 +++++
>  src/ipa/libipa/meson.build              |   2 +
>  3 files changed, 370 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..9e01f908
> --- /dev/null
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -0,0 +1,307 @@
> +/* 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

      exposure_mode_helper.cpp

> + */
> +#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
> + *
> + * Exposure modes contain a list of shutter and gain values to determine how to
> + * split the supplied exposure time into shutter and gain. As this function is
> + * expected to be replicated in various IPAs, this helper class factors it
> + * away.
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(ExposureModeHelper)
> +
> +namespace ipa {
> +
> +/**
> + * \class ExposureModeHelper
> + * \brief Class for splitting exposure time into shutter and gain
> + */
> +
> +/**
> + * \brief Initialize an ExposureModeHelper instance
> + */
> +ExposureModeHelper::ExposureModeHelper()
> +	: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)
> +{
> +}
> +
> +ExposureModeHelper::~ExposureModeHelper()
> +{
> +}

You probably don't need this and can rely on the compiler generated
one

> +
> +/**
> + * \brief Initialize an ExposureModeHelper instance
> + * \param[in] shutter The list of shutter values
> + * \param[in] gain The list of gain values
> + *
> + * When splitting an exposure time into shutter and gain, the shutter will be
> + * increased first before increasing the gain. This is done in stages, where
> + * each stage is an index into both lists. Both lists consequently need to be
> + * the same length.

Is this expected to be called one time only ? At what time ? when the
library is loaded and the configuration file is parsed ? Is it worth
mentioning it ?

> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +int ExposureModeHelper::init(std::vector<utils::Duration> &shutter, std::vector<double> &gain)
> +{
> +	if (shutter.size() != gain.size()) {
> +		LOG(ExposureModeHelper, Error)
> +			<< "Invalid exposure mode:"
> +			<< " expected size of 'shutter' and 'gain to be equal,"
> +			<< " got " << shutter.size() << " and " << gain.size()
> +			<< " respectively";
> +		return -EINVAL;
> +	}
> +
> +	std::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));
> +	std::copy(gain.begin(), gain.end(), std::back_inserter(gains_));
> +
> +	/*
> +	 * Initialize the max shutter and gain if they aren't initialized yet.
> +	 * This is to protect against the event that configure() is not called
> +	 * before splitExposure().
> +	 */

If init() has to be called one time only, can you record it with an
initialized_ flag and make sure that
1) init cannot be called twice
2) configure() can be called after init() only

> +	if (!maxShutter_) {

so you can initialize the max values unconditionally

> +		if (shutters_.size() > 0)

Can this happen ? If not you can

	if (shutter.size() != gain.size() ||
            shutter.size() == 0) {
		LOG(ExposureModeHelper, Error)
			<< "Failed to initialize ExposureModeHelper. "
			<< " Number of shutter times:"  << shutter.size()
			<< " Number of gain values:"  << gain.size()
		return -EINVAL;
	}

At the beginning of the function

> +			maxShutter_ = shutter.back();
> +	}
> +
> +	if (!maxGain_) {
> +		if (gains_.size() > 0)
> +			maxGain_ = gain.back();
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Configure the ExposureModeHelper

    * \brief Configure the ExposureModeHelper limits ?

> + * \param[in] minShutter The minimum shutter time
> + * \param[in] maxShutter The maximum shutter time
> + * \param[in] minGain The minimum gain
> + * \param[in] maxGain The maximum gain
> + *
> + * Note that the ExposureModeHelper needs to be reconfigured when
> + * FrameDurationLimits is passed, and not just at IPA configuration time.

I would rather

 * The ExposureModeHelper limits define the min/max shutter times and
 * gain values value the helpers class can select. The limits need to
 * be initialized when the IPA is configured and everytime an
 * application applies a constraint to the selectable values range, in
 * example when FrameDurationLimits is passed in.

> + *
> + * This function configures the maximum shutter and maximum gain.
> + */
> +void ExposureModeHelper::configure(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)
> +{
> +	return std::clamp(shutter, minShutter_, maxShutter_);
> +}
> +
> +double ExposureModeHelper::clampGain(double gain)
> +{
> +	return std::clamp(gain, minGain_, maxGain_);
> +}
> +
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure,
> +				  utils::Duration shutter, bool shutterFixed,
> +				  double gain, bool gainFixed)
> +{
> +	shutter = clampShutter(shutter);
> +	gain = clampGain(gain);
> +
> +	/* Not sure why you'd want to do this... */
> +	if (shutterFixed && gainFixed)
> +		return { shutter, gain, 1 };
> +
> +	/* Initial shutter and gain settings are sufficient */
> +	if (shutter * gain >= exposure) {
> +		/* Both shutter and gain cannot go lower */
> +		if (shutter == minShutter_ && gain == minGain_)
> +			return { shutter, gain, 1 };
> +
> +		/* Shutter cannot go lower */
> +		if (shutter == minShutter_ || shutterFixed)
> +			return { shutter,
> +				 gainFixed ? gain : clampGain(exposure / shutter),
> +				 1 };
> +
> +		/* Gain cannot go lower */
> +		if (gain == minGain_ || gainFixed)
> +			return {
> +				shutterFixed ? shutter : clampShutter(exposure / gain),
> +				gain,
> +				1
> +			};
> +
> +		/* Both can go lower */
> +		return { clampShutter(exposure / minGain_),
> +			 exposure / clampShutter(exposure / minGain_),
> +			 1 };

I trust your calculations here :)

> +	}
> +
> +	unsigned int stage;
> +	utils::Duration stageShutter;
> +	double stageGain;
> +	double lastStageGain;
> +
> +	/* We've already done stage 0 above so we start at 1 */
> +	for (stage = 1; stage < gains_.size(); stage++) {
> +		stageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);
> +		stageGain = gainFixed ? gain : clampGain(gains_[stage]);
> +		lastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);
> +
> +		/*
> +		 * If the product of the new stage shutter and the old stage
> +		 * gain is sufficient and we can change the shutter, reduce it.
> +		 */
> +		if (!shutterFixed && stageShutter * lastStageGain >= exposure)
> +			return { clampShutter(exposure / lastStageGain), lastStageGain, 1 };
> +
> +		/*
> +		 * The new stage shutter with old stage gain were insufficient,
> +		 * so try the new stage shutter with new stage gain. If it is
> +		 * sufficient and we can change the shutter, reduce it.
> +		 */
> +		if (!shutterFixed && stageShutter * stageGain >= exposure)
> +			return { clampShutter(exposure / stageGain), stageGain, 1 };
> +
> +		/*
> +		 * Same as above, but we can't change the shutter, so change
> +		 * the gain instead.
> +		 *
> +		 * Note that at least one of !shutterFixed and !gainFixed is
> +		 * guaranteed.
> +		 */
> +		if (!gainFixed && stageShutter * stageGain >= exposure)
> +			return { stageShutter, clampGain(exposure / stageShutter), 1 };
> +	}
> +
> +	/* From here on we're going to try to max out shutter then gain */
> +	shutter = shutterFixed ? shutter : maxShutter_;
> +	gain = gainFixed ? gain : maxGain_;
> +
> +	/*
> +	 * We probably don't want to use the actual maximum analogue gain (as
> +	 * it'll be unreasonably high), so we'll at least try to max out the
> +	 * shutter, which is expected to be a bit more reasonable, as it is
> +	 * limited by FrameDurationLimits and/or the sensor configuration.
> +	 */
> +	if (!shutterFixed && shutter * stageGain >= exposure)
> +		return { clampShutter(exposure / stageGain), stageGain, 1 };
> +
> +	/*
> +	 * If that's still not enough exposure, or if shutter is fixed, then
> +	 * we'll max out the analogue gain before using digital gain.
> +	 */
> +	if (!gainFixed && shutter * gain >= exposure)
> +		return { shutter, clampGain(exposure / shutter), 1 };
> +
> +	/*
> +	 * We're out of shutter time and analogue gain; send the rest of the
> +	 * exposure time to digital gain.
> +	 */
> +	return { shutter, gain, exposure / (shutter * gain) };

This really seems helpful!

> +}
> +
> +/**
> + * \brief Split exposure time into shutter and gain
> + * \param[in] exposure Exposure time
> + * \return Tuple of shutter time, analogue gain, and digital gain
> + */
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure)
> +{
> +	ASSERT(maxShutter_);
> +	ASSERT(maxGain_);
> +	utils::Duration shutter;
> +	double gain;
> +
> +	if (shutters_.size()) {

Wait, if you have no shutter times

> +		shutter = shutters_.at(0);
> +		gain = gains_.at(0);
> +	} else {
> +		shutter = maxShutter_;

How can you have a maxShutter ?

Or is it allowed to call configure() without init() ?

> +		gain = maxGain_;
> +	}
> +
> +	return splitExposure(exposure, shutter, false, gain, false);
> +}
> +
> +/**
> + * \brief Split exposure time into shutter and gain, with fixed shutter
> + * \param[in] exposure Exposure time
> + * \param[in] fixedShutter Fixed shutter time
> + *
> + * Same as the base splitExposure, but with a fixed shutter (aka "shutter priority").
> + *
> + * \return Tuple of shutter time, analogue gain, and digital gain
> + */
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)
> +{
> +	ASSERT(maxGain_);
> +	double gain = gains_.size() ? gains_.at(0) : maxGain_;
> +
> +	return splitExposure(exposure, fixedShutter, true, gain, false);
> +}
> +
> +/**
> + * \brief Split exposure time into shutter and gain, with fixed gain
> + * \param[in] exposure Exposure time
> + * \param[in] fixedGain Fixed gain
> + *
> + * Same as the base splitExposure, but with a fixed gain (aka "gain priority").
> + *
> + * \return Tuple of shutter time, analogue gain, and digital gain
> + */
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)
> +{
> +	ASSERT(maxShutter_);
> +	utils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;
> +
> +	return splitExposure(exposure, shutter, false, fixedGain, true);
> +}
> +
> +/**
> + * \fn ExposureModeHelper::minShutter()
> + * \brief Retrieve the configured minimum shutter time
> + */
> +
> +/**
> + * \fn ExposureModeHelper::maxShutter()
> + * \brief Retrieve the configured maximum shutter time
> + */
> +
> +/**
> + * \fn ExposureModeHelper::minGain()
> + * \brief Retrieve the configured minimum gain
> + */
> +
> +/**
> + * \fn ExposureModeHelper::maxGain()
> + * \brief Retrieve the configured maximum gain
> + */
> +
> +} /* 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..d576c952
> --- /dev/null
> +++ b/src/ipa/libipa/exposure_mode_helper.h
> @@ -0,0 +1,61 @@
> +/* 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 <algorithm>

Included by .cpp

> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/utils.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class ExposureModeHelper
> +{
> +public:
> +	ExposureModeHelper();
> +	~ExposureModeHelper();
> +
> +	int init(std::vector<utils::Duration> &shutters, std::vector<double> &gains);
> +	void configure(utils::Duration minShutter, utils::Duration maxShutter,
> +		       double minGain, double maxGain);
> +
> +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure);
> +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> +								  utils::Duration fixedShutter);
> +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> +								  double fixedGain);
> +
> +	utils::Duration minShutter() { return minShutter_; };
> +	utils::Duration maxShutter() { return maxShutter_; };
> +	double minGain() { return minGain_; };
> +	double maxGain() { return maxGain_; };
> +
> +private:
> +	utils::Duration clampShutter(utils::Duration shutter);
> +	double clampGain(double gain);
> +
> +	std::tuple<utils::Duration, double, double>
> +	splitExposure(utils::Duration exposure,
> +		      utils::Duration shutter, bool shutterFixed,
> +		      double gain, bool gainFixed);
> +
> +	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',

This seems a really helpful addition overall !

> --
> 2.34.1
>

Hi Paul, hi Daniel,

Thanks for the patch. 

Jacopo already did a thorough review. I can only add little bits


On Fri, Mar 22, 2024 at 01:14:44PM +0000, 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>
> ---
>  src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++
>  src/ipa/libipa/exposure_mode_helper.h   |  61 +++++
>  src/ipa/libipa/meson.build              |   2 +
>  3 files changed, 370 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..9e01f908
> --- /dev/null
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -0,0 +1,307 @@
> +/* 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
> + */
> +#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
> + *
> + * Exposure modes contain a list of shutter and gain values to determine how to
> + * split the supplied exposure time into shutter and gain. As this function is
> + * expected to be replicated in various IPAs, this helper class factors it
> + * away.
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(ExposureModeHelper)
> +
> +namespace ipa {
> +
> +/**
> + * \class ExposureModeHelper
> + * \brief Class for splitting exposure time into shutter and gain
> + */
> +
> +/**
> + * \brief Initialize an ExposureModeHelper instance
> + */
> +ExposureModeHelper::ExposureModeHelper()
> +	: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)
> +{
> +}
> +
> +ExposureModeHelper::~ExposureModeHelper()
> +{
> +}
> +
> +/**
> + * \brief Initialize an ExposureModeHelper instance
> + * \param[in] shutter The list of shutter values
> + * \param[in] gain The list of gain values
> + *
> + * When splitting an exposure time into shutter and gain, the shutter will be
> + * increased first before increasing the gain. This is done in stages, where
> + * each stage is an index into both lists. Both lists consequently need to be
> + * the same length.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +int ExposureModeHelper::init(std::vector<utils::Duration> &shutter, std::vector<double> &gain)
> +{
> +	if (shutter.size() != gain.size()) {
> +		LOG(ExposureModeHelper, Error)
> +			<< "Invalid exposure mode:"
> +			<< " expected size of 'shutter' and 'gain to be equal,"
> +			<< " got " << shutter.size() << " and " << gain.size()
> +			<< " respectively";
> +		return -EINVAL;
> +	}
> +
> +	std::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));
> +	std::copy(gain.begin(), gain.end(), std::back_inserter(gains_));
> +
> +	/*
> +	 * Initialize the max shutter and gain if they aren't initialized yet.
> +	 * This is to protect against the event that configure() is not called
> +	 * before splitExposure().
> +	 */
> +	if (!maxShutter_) {
> +		if (shutters_.size() > 0)
> +			maxShutter_ = shutter.back();
> +	}
> +
> +	if (!maxGain_) {
> +		if (gains_.size() > 0)
> +			maxGain_ = gain.back();
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Configure the ExposureModeHelper
> + * \param[in] minShutter The minimum shutter time
> + * \param[in] maxShutter The maximum shutter time
> + * \param[in] minGain The minimum gain
> + * \param[in] maxGain The maximum gain
> + *
> + * Note that the ExposureModeHelper needs to be reconfigured when
> + * FrameDurationLimits is passed, and not just at IPA configuration time.
> + *
> + * This function configures the maximum shutter and maximum gain.
> + */
> +void ExposureModeHelper::configure(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)
> +{
> +	return std::clamp(shutter, minShutter_, maxShutter_);
> +}
> +
> +double ExposureModeHelper::clampGain(double gain)
> +{
> +	return std::clamp(gain, minGain_, maxGain_);
> +}
> +
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure,
> +				  utils::Duration shutter, bool shutterFixed,
> +				  double gain, bool gainFixed)
> +{
> +	shutter = clampShutter(shutter);
> +	gain = clampGain(gain);
> +
> +	/* Not sure why you'd want to do this... */
> +	if (shutterFixed && gainFixed)
> +		return { shutter, gain, 1 };
> +
> +	/* Initial shutter and gain settings are sufficient */
> +	if (shutter * gain >= exposure) {
> +		/* Both shutter and gain cannot go lower */
> +		if (shutter == minShutter_ && gain == minGain_)
> +			return { shutter, gain, 1 };
> +
> +		/* Shutter cannot go lower */
> +		if (shutter == minShutter_ || shutterFixed)
> +			return { shutter,
> +				 gainFixed ? gain : clampGain(exposure / shutter),
> +				 1 };
> +
> +		/* Gain cannot go lower */
> +		if (gain == minGain_ || gainFixed)
> +			return {
> +				shutterFixed ? shutter : clampShutter(exposure / gain),
> +				gain,
> +				1
> +			};
> +
> +		/* Both can go lower */
> +		return { clampShutter(exposure / minGain_),
> +			 exposure / clampShutter(exposure / minGain_),
> +			 1 };

Isn't this missing the clampGain()? Might be easier to read if the two
calls to clampShutter would be reduced to one and a variable.

> +	}
> +
> +	unsigned int stage;
> +	utils::Duration stageShutter;
> +	double stageGain;
> +	double lastStageGain;
> +
> +	/* We've already done stage 0 above so we start at 1 */
> +	for (stage = 1; stage < gains_.size(); stage++) {
> +		stageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);
> +		stageGain = gainFixed ? gain : clampGain(gains_[stage]);
> +		lastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);
> +
> +		/*
> +		 * If the product of the new stage shutter and the old stage
> +		 * gain is sufficient and we can change the shutter, reduce it.
> +		 */
> +		if (!shutterFixed && stageShutter * lastStageGain >= exposure)
> +			return { clampShutter(exposure / lastStageGain), lastStageGain, 1 };
> +
> +		/*
> +		 * The new stage shutter with old stage gain were insufficient,
> +		 * so try the new stage shutter with new stage gain. If it is
> +		 * sufficient and we can change the shutter, reduce it.
> +		 */
> +		if (!shutterFixed && stageShutter * stageGain >= exposure)
> +			return { clampShutter(exposure / stageGain), stageGain, 1 };
> +
> +		/*
> +		 * Same as above, but we can't change the shutter, so change
> +		 * the gain instead.
> +		 *
> +		 * Note that at least one of !shutterFixed and !gainFixed is
> +		 * guaranteed.
> +		 */
> +		if (!gainFixed && stageShutter * stageGain >= exposure)
> +			return { stageShutter, clampGain(exposure / stageShutter), 1 };
> +	}
> +
> +	/* From here on we're going to try to max out shutter then gain */
> +	shutter = shutterFixed ? shutter : maxShutter_;
> +	gain = gainFixed ? gain : maxGain_;
> +
> +	/*
> +	 * We probably don't want to use the actual maximum analogue gain (as
> +	 * it'll be unreasonably high), so we'll at least try to max out the
> +	 * shutter, which is expected to be a bit more reasonable, as it is
> +	 * limited by FrameDurationLimits and/or the sensor configuration.
> +	 */
> +	if (!shutterFixed && shutter * stageGain >= exposure)
> +		return { clampShutter(exposure / stageGain), stageGain, 1 };
> +
> +	/*
> +	 * If that's still not enough exposure, or if shutter is fixed, then
> +	 * we'll max out the analogue gain before using digital gain.
> +	 */
> +	if (!gainFixed && shutter * gain >= exposure)
> +		return { shutter, clampGain(exposure / shutter), 1 };
> +
> +	/*
> +	 * We're out of shutter time and analogue gain; send the rest of the
> +	 * exposure time to digital gain.
> +	 */
> +	return { shutter, gain, exposure / (shutter * gain) };
> +}

I wonder how this would work with flicker mitigation. But that's
propably out of scope.

Cheers,
Stefan

> +
> +/**
> + * \brief Split exposure time into shutter and gain
> + * \param[in] exposure Exposure time
> + * \return Tuple of shutter time, analogue gain, and digital gain
> + */
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure)
> +{
> +	ASSERT(maxShutter_);
> +	ASSERT(maxGain_);
> +	utils::Duration shutter;
> +	double gain;
> +
> +	if (shutters_.size()) {
> +		shutter = shutters_.at(0);
> +		gain = gains_.at(0);
> +	} else {
> +		shutter = maxShutter_;
> +		gain = maxGain_;
> +	}
> +
> +	return splitExposure(exposure, shutter, false, gain, false);
> +}
> +
> +/**
> + * \brief Split exposure time into shutter and gain, with fixed shutter
> + * \param[in] exposure Exposure time
> + * \param[in] fixedShutter Fixed shutter time
> + *
> + * Same as the base splitExposure, but with a fixed shutter (aka "shutter priority").
> + *
> + * \return Tuple of shutter time, analogue gain, and digital gain
> + */
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)
> +{
> +	ASSERT(maxGain_);
> +	double gain = gains_.size() ? gains_.at(0) : maxGain_;
> +
> +	return splitExposure(exposure, fixedShutter, true, gain, false);
> +}
> +
> +/**
> + * \brief Split exposure time into shutter and gain, with fixed gain
> + * \param[in] exposure Exposure time
> + * \param[in] fixedGain Fixed gain
> + *
> + * Same as the base splitExposure, but with a fixed gain (aka "gain priority").
> + *
> + * \return Tuple of shutter time, analogue gain, and digital gain
> + */
> +std::tuple<utils::Duration, double, double>
> +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)
> +{
> +	ASSERT(maxShutter_);
> +	utils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;
> +
> +	return splitExposure(exposure, shutter, false, fixedGain, true);
> +}
> +
> +/**
> + * \fn ExposureModeHelper::minShutter()
> + * \brief Retrieve the configured minimum shutter time
> + */
> +
> +/**
> + * \fn ExposureModeHelper::maxShutter()
> + * \brief Retrieve the configured maximum shutter time
> + */
> +
> +/**
> + * \fn ExposureModeHelper::minGain()
> + * \brief Retrieve the configured minimum gain
> + */
> +
> +/**
> + * \fn ExposureModeHelper::maxGain()
> + * \brief Retrieve the configured maximum gain
> + */
> +
> +} /* 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..d576c952
> --- /dev/null
> +++ b/src/ipa/libipa/exposure_mode_helper.h
> @@ -0,0 +1,61 @@
> +/* 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 <algorithm>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/utils.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class ExposureModeHelper
> +{
> +public:
> +	ExposureModeHelper();
> +	~ExposureModeHelper();
> +
> +	int init(std::vector<utils::Duration> &shutters, std::vector<double> &gains);
> +	void configure(utils::Duration minShutter, utils::Duration maxShutter,
> +		       double minGain, double maxGain);
> +
> +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure);
> +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> +								  utils::Duration fixedShutter);
> +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> +								  double fixedGain);
> +
> +	utils::Duration minShutter() { return minShutter_; };
> +	utils::Duration maxShutter() { return maxShutter_; };
> +	double minGain() { return minGain_; };
> +	double maxGain() { return maxGain_; };
> +
> +private:
> +	utils::Duration clampShutter(utils::Duration shutter);
> +	double clampGain(double gain);
> +
> +	std::tuple<utils::Duration, double, double>
> +	splitExposure(utils::Duration exposure,
> +		      utils::Duration shutter, bool shutterFixed,
> +		      double gain, bool gainFixed);
> +
> +	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
>

Quoting Stefan Klug (2024-03-26 08:58:18)
> Hi Paul, hi Daniel,
> 
> Thanks for the patch. 
> 
> Jacopo already did a thorough review. I can only add little bits
> 
> 
> On Fri, Mar 22, 2024 at 01:14:44PM +0000, 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>
> > ---
> >  src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++
> >  src/ipa/libipa/exposure_mode_helper.h   |  61 +++++
> >  src/ipa/libipa/meson.build              |   2 +
> >  3 files changed, 370 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..9e01f908
> > --- /dev/null
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -0,0 +1,307 @@
> > +/* 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
> > + */
> > +#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
> > + *
> > + * Exposure modes contain a list of shutter and gain values to determine how to
> > + * split the supplied exposure time into shutter and gain. As this function is
> > + * expected to be replicated in various IPAs, this helper class factors it
> > + * away.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(ExposureModeHelper)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class ExposureModeHelper
> > + * \brief Class for splitting exposure time into shutter and gain
> > + */
> > +
> > +/**
> > + * \brief Initialize an ExposureModeHelper instance
> > + */
> > +ExposureModeHelper::ExposureModeHelper()
> > +     : minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)
> > +{
> > +}
> > +
> > +ExposureModeHelper::~ExposureModeHelper()
> > +{
> > +}
> > +
> > +/**
> > + * \brief Initialize an ExposureModeHelper instance
> > + * \param[in] shutter The list of shutter values
> > + * \param[in] gain The list of gain values
> > + *
> > + * When splitting an exposure time into shutter and gain, the shutter will be
> > + * increased first before increasing the gain. This is done in stages, where
> > + * each stage is an index into both lists. Both lists consequently need to be
> > + * the same length.
> > + *
> > + * \return Zero on success, negative error code otherwise
> > + */
> > +int ExposureModeHelper::init(std::vector<utils::Duration> &shutter, std::vector<double> &gain)
> > +{
> > +     if (shutter.size() != gain.size()) {
> > +             LOG(ExposureModeHelper, Error)
> > +                     << "Invalid exposure mode:"
> > +                     << " expected size of 'shutter' and 'gain to be equal,"
> > +                     << " got " << shutter.size() << " and " << gain.size()
> > +                     << " respectively";
> > +             return -EINVAL;
> > +     }
> > +
> > +     std::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));
> > +     std::copy(gain.begin(), gain.end(), std::back_inserter(gains_));
> > +
> > +     /*
> > +      * Initialize the max shutter and gain if they aren't initialized yet.
> > +      * This is to protect against the event that configure() is not called
> > +      * before splitExposure().
> > +      */
> > +     if (!maxShutter_) {
> > +             if (shutters_.size() > 0)
> > +                     maxShutter_ = shutter.back();
> > +     }
> > +
> > +     if (!maxGain_) {
> > +             if (gains_.size() > 0)
> > +                     maxGain_ = gain.back();
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * \brief Configure the ExposureModeHelper
> > + * \param[in] minShutter The minimum shutter time
> > + * \param[in] maxShutter The maximum shutter time
> > + * \param[in] minGain The minimum gain
> > + * \param[in] maxGain The maximum gain
> > + *
> > + * Note that the ExposureModeHelper needs to be reconfigured when
> > + * FrameDurationLimits is passed, and not just at IPA configuration time.
> > + *
> > + * This function configures the maximum shutter and maximum gain.
> > + */
> > +void ExposureModeHelper::configure(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)
> > +{
> > +     return std::clamp(shutter, minShutter_, maxShutter_);
> > +}
> > +
> > +double ExposureModeHelper::clampGain(double gain)
> > +{
> > +     return std::clamp(gain, minGain_, maxGain_);
> > +}
> > +
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure,
> > +                               utils::Duration shutter, bool shutterFixed,
> > +                               double gain, bool gainFixed)
> > +{
> > +     shutter = clampShutter(shutter);
> > +     gain = clampGain(gain);
> > +
> > +     /* Not sure why you'd want to do this... */
> > +     if (shutterFixed && gainFixed)
> > +             return { shutter, gain, 1 };
> > +
> > +     /* Initial shutter and gain settings are sufficient */
> > +     if (shutter * gain >= exposure) {
> > +             /* Both shutter and gain cannot go lower */
> > +             if (shutter == minShutter_ && gain == minGain_)
> > +                     return { shutter, gain, 1 };
> > +
> > +             /* Shutter cannot go lower */
> > +             if (shutter == minShutter_ || shutterFixed)
> > +                     return { shutter,
> > +                              gainFixed ? gain : clampGain(exposure / shutter),
> > +                              1 };
> > +
> > +             /* Gain cannot go lower */
> > +             if (gain == minGain_ || gainFixed)
> > +                     return {
> > +                             shutterFixed ? shutter : clampShutter(exposure / gain),
> > +                             gain,
> > +                             1
> > +                     };
> > +
> > +             /* Both can go lower */
> > +             return { clampShutter(exposure / minGain_),
> > +                      exposure / clampShutter(exposure / minGain_),
> > +                      1 };
> 
> Isn't this missing the clampGain()? Might be easier to read if the two
> calls to clampShutter would be reduced to one and a variable.
> 
> > +     }
> > +
> > +     unsigned int stage;
> > +     utils::Duration stageShutter;
> > +     double stageGain;
> > +     double lastStageGain;
> > +
> > +     /* We've already done stage 0 above so we start at 1 */
> > +     for (stage = 1; stage < gains_.size(); stage++) {
> > +             stageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);
> > +             stageGain = gainFixed ? gain : clampGain(gains_[stage]);
> > +             lastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);
> > +
> > +             /*
> > +              * If the product of the new stage shutter and the old stage
> > +              * gain is sufficient and we can change the shutter, reduce it.
> > +              */
> > +             if (!shutterFixed && stageShutter * lastStageGain >= exposure)
> > +                     return { clampShutter(exposure / lastStageGain), lastStageGain, 1 };
> > +
> > +             /*
> > +              * The new stage shutter with old stage gain were insufficient,
> > +              * so try the new stage shutter with new stage gain. If it is
> > +              * sufficient and we can change the shutter, reduce it.
> > +              */
> > +             if (!shutterFixed && stageShutter * stageGain >= exposure)
> > +                     return { clampShutter(exposure / stageGain), stageGain, 1 };
> > +
> > +             /*
> > +              * Same as above, but we can't change the shutter, so change
> > +              * the gain instead.
> > +              *
> > +              * Note that at least one of !shutterFixed and !gainFixed is
> > +              * guaranteed.
> > +              */
> > +             if (!gainFixed && stageShutter * stageGain >= exposure)
> > +                     return { stageShutter, clampGain(exposure / stageShutter), 1 };
> > +     }
> > +
> > +     /* From here on we're going to try to max out shutter then gain */
> > +     shutter = shutterFixed ? shutter : maxShutter_;
> > +     gain = gainFixed ? gain : maxGain_;
> > +
> > +     /*
> > +      * We probably don't want to use the actual maximum analogue gain (as
> > +      * it'll be unreasonably high), so we'll at least try to max out the
> > +      * shutter, which is expected to be a bit more reasonable, as it is
> > +      * limited by FrameDurationLimits and/or the sensor configuration.
> > +      */
> > +     if (!shutterFixed && shutter * stageGain >= exposure)
> > +             return { clampShutter(exposure / stageGain), stageGain, 1 };
> > +
> > +     /*
> > +      * If that's still not enough exposure, or if shutter is fixed, then
> > +      * we'll max out the analogue gain before using digital gain.
> > +      */
> > +     if (!gainFixed && shutter * gain >= exposure)
> > +             return { shutter, clampGain(exposure / shutter), 1 };
> > +
> > +     /*
> > +      * We're out of shutter time and analogue gain; send the rest of the
> > +      * exposure time to digital gain.
> > +      */
> > +     return { shutter, gain, exposure / (shutter * gain) };
> > +}
> 
> I wonder how this would work with flicker mitigation. But that's
> propably out of scope.

Tackling flicker avoidance centrally here definitely makes sense, but as
neither IPU3 nor RKISP1 even attempt this yet - I think it's fine to do
so on top.

--
Kieran


> 
> Cheers,
> Stefan
> 
> > +
> > +/**
> > + * \brief Split exposure time into shutter and gain
> > + * \param[in] exposure Exposure time
> > + * \return Tuple of shutter time, analogue gain, and digital gain
> > + */
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure)
> > +{
> > +     ASSERT(maxShutter_);
> > +     ASSERT(maxGain_);
> > +     utils::Duration shutter;
> > +     double gain;
> > +
> > +     if (shutters_.size()) {
> > +             shutter = shutters_.at(0);
> > +             gain = gains_.at(0);
> > +     } else {
> > +             shutter = maxShutter_;
> > +             gain = maxGain_;
> > +     }
> > +
> > +     return splitExposure(exposure, shutter, false, gain, false);
> > +}
> > +
> > +/**
> > + * \brief Split exposure time into shutter and gain, with fixed shutter
> > + * \param[in] exposure Exposure time
> > + * \param[in] fixedShutter Fixed shutter time
> > + *
> > + * Same as the base splitExposure, but with a fixed shutter (aka "shutter priority").
> > + *
> > + * \return Tuple of shutter time, analogue gain, and digital gain
> > + */
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)
> > +{
> > +     ASSERT(maxGain_);
> > +     double gain = gains_.size() ? gains_.at(0) : maxGain_;
> > +
> > +     return splitExposure(exposure, fixedShutter, true, gain, false);
> > +}
> > +
> > +/**
> > + * \brief Split exposure time into shutter and gain, with fixed gain
> > + * \param[in] exposure Exposure time
> > + * \param[in] fixedGain Fixed gain
> > + *
> > + * Same as the base splitExposure, but with a fixed gain (aka "gain priority").
> > + *
> > + * \return Tuple of shutter time, analogue gain, and digital gain
> > + */
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)
> > +{
> > +     ASSERT(maxShutter_);
> > +     utils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;
> > +
> > +     return splitExposure(exposure, shutter, false, fixedGain, true);
> > +}
> > +
> > +/**
> > + * \fn ExposureModeHelper::minShutter()
> > + * \brief Retrieve the configured minimum shutter time
> > + */
> > +
> > +/**
> > + * \fn ExposureModeHelper::maxShutter()
> > + * \brief Retrieve the configured maximum shutter time
> > + */
> > +
> > +/**
> > + * \fn ExposureModeHelper::minGain()
> > + * \brief Retrieve the configured minimum gain
> > + */
> > +
> > +/**
> > + * \fn ExposureModeHelper::maxGain()
> > + * \brief Retrieve the configured maximum gain
> > + */
> > +
> > +} /* 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..d576c952
> > --- /dev/null
> > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > @@ -0,0 +1,61 @@
> > +/* 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 <algorithm>
> > +#include <tuple>
> > +#include <vector>
> > +
> > +#include <libcamera/base/utils.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +class ExposureModeHelper
> > +{
> > +public:
> > +     ExposureModeHelper();
> > +     ~ExposureModeHelper();
> > +
> > +     int init(std::vector<utils::Duration> &shutters, std::vector<double> &gains);
> > +     void configure(utils::Duration minShutter, utils::Duration maxShutter,
> > +                    double minGain, double maxGain);
> > +
> > +     std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure);
> > +     std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> > +                                                               utils::Duration fixedShutter);
> > +     std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> > +                                                               double fixedGain);
> > +
> > +     utils::Duration minShutter() { return minShutter_; };
> > +     utils::Duration maxShutter() { return maxShutter_; };
> > +     double minGain() { return minGain_; };
> > +     double maxGain() { return maxGain_; };
> > +
> > +private:
> > +     utils::Duration clampShutter(utils::Duration shutter);
> > +     double clampGain(double gain);
> > +
> > +     std::tuple<utils::Duration, double, double>
> > +     splitExposure(utils::Duration exposure,
> > +                   utils::Duration shutter, bool shutterFixed,
> > +                   double gain, bool gainFixed);
> > +
> > +     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 Mon, Mar 25, 2024 at 05:48:07PM +0100, Jacopo Mondi wrote:
> On Fri, Mar 22, 2024 at 01:14:44PM +0000, 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>
> > ---
> >  src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++
> >  src/ipa/libipa/exposure_mode_helper.h   |  61 +++++
> >  src/ipa/libipa/meson.build              |   2 +
> >  3 files changed, 370 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..9e01f908
> > --- /dev/null
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -0,0 +1,307 @@
> > +/* 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
> 
>       exposure_mode_helper.cpp

Should we drop the file name from the top-level headers project-wide ?

> > + */
> > +#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
> > + *
> > + * Exposure modes contain a list of shutter and gain values to determine how to
> > + * split the supplied exposure time into shutter and gain.

"exposure time" and "shutter" are the same. You probably meant
"exposure" instead of "exposure time". Same below where applicable.

On a side note, I think it would be clearer to transition to
"integration time" instead of "exposure time" or "shutter". This will
avoid confusing "exposure" and "exposure time", as well as free the term
"shutter" for mechanical shutters if we ever need to support them.

Setting shutter and gain ranges per mode is one possible implementation,
not something that libcamera dictates globally. Could you refactor this
text accordingly ? Something along the lines of "there's a need for AEGC
algorithms to split exposure in shutter and gain, multiple IPA modules
do so based on <explain common technique>, provide a helper to avoid
duplicating the code".

> > As this function is
> > + * expected to be replicated in various IPAs, this helper class factors it
> > + * away.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(ExposureModeHelper)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class ExposureModeHelper
> > + * \brief Class for splitting exposure time into shutter and gain

This is where you need to explain how the class operates. After reading
this block, the reader should understand what the class is used for and
if it matches its needs. The theory of operation of exposure modes and
how they dictate how to split exposure in shutter and gain belong here.

> > + */
> > +
> > +/**
> > + * \brief Initialize an ExposureModeHelper instance

s/Initialize/Construct/

> > + */
> > +ExposureModeHelper::ExposureModeHelper()
> > +	: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)
> > +{
> > +}
> > +
> > +ExposureModeHelper::~ExposureModeHelper()
> > +{
> > +}
> 
> You probably don't need this and can rely on the compiler generated
> one
> 
> > +
> > +/**
> > + * \brief Initialize an ExposureModeHelper instance
> > + * \param[in] shutter The list of shutter values
> > + * \param[in] gain The list of gain values

Is this for the analog gain, the digital gain, or both ? Make it
explicit here and where it matters. Also explain in the class
description that it handles both gains, and why there's no need to
provide configuration parameters (and limits) for both.

> > + *
> > + * When splitting an exposure time into shutter and gain, the shutter will be
> > + * increased first before increasing the gain. This is done in stages, where
> > + * each stage is an index into both lists. Both lists consequently need to be
> > + * the same length.
> 
> Is this expected to be called one time only ? At what time ? when the
> library is loaded and the configuration file is parsed ? Is it worth
> mentioning it ?

It's worth explaining it. Also, reading the documentation, I'm afraid I
don't understand what parameters to pass to this function.

> > + *
> > + * \return Zero on success, negative error code otherwise
> > + */
> > +int ExposureModeHelper::init(std::vector<utils::Duration> &shutter, std::vector<double> &gain)

The parameters are mutable, which I don't think is what you intended.
Also, nothing in the function strictly requires shutter and gain to be
vectors. Use spans to relax the data storage requirements in the caller.

> > +{
> > +	if (shutter.size() != gain.size()) {
> > +		LOG(ExposureModeHelper, Error)
> > +			<< "Invalid exposure mode:"
> > +			<< " expected size of 'shutter' and 'gain to be equal,"
> > +			<< " got " << shutter.size() << " and " << gain.size()
> > +			<< " respectively";
> > +		return -EINVAL;
> > +	}

Try to design the API in a way that minimizes misuse. If the function
needs the same number of shutter and gain values, you can for instance
store them in a vector of pairs (std::pair or a custom structure), not a
pair of vectors.

> > +
> > +	std::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));
> > +	std::copy(gain.begin(), gain.end(), std::back_inserter(gains_));

Calling the function multiple times will add values to shutters_ and
gains_, not replace them. Is that by design ?

> > +
> > +	/*
> > +	 * Initialize the max shutter and gain if they aren't initialized yet.
> > +	 * This is to protect against the event that configure() is not called
> > +	 * before splitExposure().
> > +	 */
> 
> If init() has to be called one time only, can you record it with an
> initialized_ flag and make sure that
> 1) init cannot be called twice
> 2) configure() can be called after init() only

It seems difficult to use the class correctly, we need to aim for
impossible to use incorrectly instead :-)

> > +	if (!maxShutter_) {
> 
> so you can initialize the max values unconditionally
> 
> > +		if (shutters_.size() > 0)
> 
> Can this happen ? If not you can
> 
> 	if (shutter.size() != gain.size() ||
>             shutter.size() == 0) {
> 		LOG(ExposureModeHelper, Error)
> 			<< "Failed to initialize ExposureModeHelper. "
> 			<< " Number of shutter times:"  << shutter.size()
> 			<< " Number of gain values:"  << gain.size()
> 		return -EINVAL;
> 	}
> 
> At the beginning of the function
> 
> > +			maxShutter_ = shutter.back();
> > +	}
> > +
> > +	if (!maxGain_) {
> > +		if (gains_.size() > 0)
> > +			maxGain_ = gain.back();
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \brief Configure the ExposureModeHelper
> 
>     * \brief Configure the ExposureModeHelper limits ?

And name the function accordingly. configure() doesn't tell me much.
setShutterGainLimits() is a much better name. You can then document it
as

 * \brief Set the shutter and gain limits

When your \brief can't tell much more than the function name without
becoming a non-brief text, you know you have a good name.

> > + * \param[in] minShutter The minimum shutter time
> > + * \param[in] maxShutter The maximum shutter time
> > + * \param[in] minGain The minimum gain
> > + * \param[in] maxGain The maximum gain
> > + *
> > + * Note that the ExposureModeHelper needs to be reconfigured when
> > + * FrameDurationLimits is passed, and not just at IPA configuration time.
> 
> I would rather
> 
>  * The ExposureModeHelper limits define the min/max shutter times and
>  * gain values value the helpers class can select. The limits need to
>  * be initialized when the IPA is configured and everytime an
>  * application applies a constraint to the selectable values range, in
>  * example when FrameDurationLimits is passed in.

That's information specific to the caller. Keep the class generic, tell
if setting the limits is required before using the class, if the limits
can change (and if so when), and what the limits are used for.

> > + *
> > + * This function configures the maximum shutter and maximum gain.
> > + */
> > +void ExposureModeHelper::configure(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)
> > +{
> > +	return std::clamp(shutter, minShutter_, maxShutter_);
> > +}
> > +
> > +double ExposureModeHelper::clampGain(double gain)
> > +{
> > +	return std::clamp(gain, minGain_, maxGain_);
> > +}
> > +
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure,
> > +				  utils::Duration shutter, bool shutterFixed,
> > +				  double gain, bool gainFixed)
> > +{
> > +	shutter = clampShutter(shutter);
> > +	gain = clampGain(gain);
> > +
> > +	/* Not sure why you'd want to do this... */
> > +	if (shutterFixed && gainFixed)
> > +		return { shutter, gain, 1 };

No need to use digital gain here ?

> > +
> > +	/* Initial shutter and gain settings are sufficient */
> > +	if (shutter * gain >= exposure) {
> > +		/* Both shutter and gain cannot go lower */
> > +		if (shutter == minShutter_ && gain == minGain_)
> > +			return { shutter, gain, 1 };
> > +
> > +		/* Shutter cannot go lower */
> > +		if (shutter == minShutter_ || shutterFixed)
> > +			return { shutter,
> > +				 gainFixed ? gain : clampGain(exposure / shutter),
> > +				 1 };
> > +
> > +		/* Gain cannot go lower */
> > +		if (gain == minGain_ || gainFixed)
> > +			return {
> > +				shutterFixed ? shutter : clampShutter(exposure / gain),
> > +				gain,
> > +				1
> > +			};
> > +
> > +		/* Both can go lower */
> > +		return { clampShutter(exposure / minGain_),
> > +			 exposure / clampShutter(exposure / minGain_),
> > +			 1 };
> 
> I trust your calculations here :)
> 
> > +	}
> > +
> > +	unsigned int stage;
> > +	utils::Duration stageShutter;
> > +	double stageGain;
> > +	double lastStageGain;
> > +
> > +	/* We've already done stage 0 above so we start at 1 */
> > +	for (stage = 1; stage < gains_.size(); stage++) {
> > +		stageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);
> > +		stageGain = gainFixed ? gain : clampGain(gains_[stage]);
> > +		lastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);
> > +
> > +		/*
> > +		 * If the product of the new stage shutter and the old stage
> > +		 * gain is sufficient and we can change the shutter, reduce it.
> > +		 */
> > +		if (!shutterFixed && stageShutter * lastStageGain >= exposure)
> > +			return { clampShutter(exposure / lastStageGain), lastStageGain, 1 };
> > +
> > +		/*
> > +		 * The new stage shutter with old stage gain were insufficient,
> > +		 * so try the new stage shutter with new stage gain. If it is
> > +		 * sufficient and we can change the shutter, reduce it.
> > +		 */
> > +		if (!shutterFixed && stageShutter * stageGain >= exposure)
> > +			return { clampShutter(exposure / stageGain), stageGain, 1 };
> > +
> > +		/*
> > +		 * Same as above, but we can't change the shutter, so change
> > +		 * the gain instead.
> > +		 *
> > +		 * Note that at least one of !shutterFixed and !gainFixed is
> > +		 * guaranteed.
> > +		 */
> > +		if (!gainFixed && stageShutter * stageGain >= exposure)
> > +			return { stageShutter, clampGain(exposure / stageShutter), 1 };
> > +	}
> > +
> > +	/* From here on we're going to try to max out shutter then gain */
> > +	shutter = shutterFixed ? shutter : maxShutter_;
> > +	gain = gainFixed ? gain : maxGain_;
> > +
> > +	/*
> > +	 * We probably don't want to use the actual maximum analogue gain (as
> > +	 * it'll be unreasonably high), so we'll at least try to max out the
> > +	 * shutter, which is expected to be a bit more reasonable, as it is
> > +	 * limited by FrameDurationLimits and/or the sensor configuration.
> > +	 */
> > +	if (!shutterFixed && shutter * stageGain >= exposure)
> > +		return { clampShutter(exposure / stageGain), stageGain, 1 };
> > +
> > +	/*
> > +	 * If that's still not enough exposure, or if shutter is fixed, then
> > +	 * we'll max out the analogue gain before using digital gain.
> > +	 */
> > +	if (!gainFixed && shutter * gain >= exposure)
> > +		return { shutter, clampGain(exposure / shutter), 1 };
> > +
> > +	/*
> > +	 * We're out of shutter time and analogue gain; send the rest of the
> > +	 * exposure time to digital gain.
> > +	 */
> > +	return { shutter, gain, exposure / (shutter * gain) };
> 
> This really seems helpful!
> 
> > +}
> > +
> > +/**
> > + * \brief Split exposure time into shutter and gain
> > + * \param[in] exposure Exposure time
> > + * \return Tuple of shutter time, analogue gain, and digital gain
> > + */
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure)
> > +{
> > +	ASSERT(maxShutter_);
> > +	ASSERT(maxGain_);
> > +	utils::Duration shutter;
> > +	double gain;
> > +
> > +	if (shutters_.size()) {
> 
> Wait, if you have no shutter times
> 
> > +		shutter = shutters_.at(0);
> > +		gain = gains_.at(0);
> > +	} else {
> > +		shutter = maxShutter_;
> 
> How can you have a maxShutter ?
> 
> Or is it allowed to call configure() without init() ?
> 
> > +		gain = maxGain_;
> > +	}
> > +
> > +	return splitExposure(exposure, shutter, false, gain, false);
> > +}
> > +
> > +/**
> > + * \brief Split exposure time into shutter and gain, with fixed shutter
> > + * \param[in] exposure Exposure time
> > + * \param[in] fixedShutter Fixed shutter time
> > + *
> > + * Same as the base splitExposure, but with a fixed shutter (aka "shutter priority").

This can be achieved by setting the min and max shutter limits to the
fixed shutter value. What is the reason for having two different ways to
do the same ?

> > + *
> > + * \return Tuple of shutter time, analogue gain, and digital gain
> > + */
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)
> > +{
> > +	ASSERT(maxGain_);
> > +	double gain = gains_.size() ? gains_.at(0) : maxGain_;
> > +
> > +	return splitExposure(exposure, fixedShutter, true, gain, false);
> > +}
> > +
> > +/**
> > + * \brief Split exposure time into shutter and gain, with fixed gain
> > + * \param[in] exposure Exposure time
> > + * \param[in] fixedGain Fixed gain
> > + *
> > + * Same as the base splitExposure, but with a fixed gain (aka "gain priority").
> > + *
> > + * \return Tuple of shutter time, analogue gain, and digital gain
> > + */
> > +std::tuple<utils::Duration, double, double>
> > +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)
> > +{
> > +	ASSERT(maxShutter_);
> > +	utils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;
> > +
> > +	return splitExposure(exposure, shutter, false, fixedGain, true);
> > +}
> > +
> > +/**
> > + * \fn ExposureModeHelper::minShutter()
> > + * \brief Retrieve the configured minimum shutter time

Missing \return. Same below. Update the text to mention limits if you
rename configure() to setShutterGainLimits(). Reference the
setShutterGainLimits() function explicitly.

> > + */
> > +
> > +/**
> > + * \fn ExposureModeHelper::maxShutter()
> > + * \brief Retrieve the configured maximum shutter time
> > + */
> > +
> > +/**
> > + * \fn ExposureModeHelper::minGain()
> > + * \brief Retrieve the configured minimum gain
> > + */
> > +
> > +/**
> > + * \fn ExposureModeHelper::maxGain()
> > + * \brief Retrieve the configured maximum gain
> > + */
> > +
> > +} /* 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..d576c952
> > --- /dev/null
> > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > @@ -0,0 +1,61 @@
> > +/* 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 <algorithm>
> 
> Included by .cpp
> 
> > +#include <tuple>
> > +#include <vector>
> > +
> > +#include <libcamera/base/utils.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +class ExposureModeHelper
> > +{
> > +public:
> > +	ExposureModeHelper();
> > +	~ExposureModeHelper();
> > +
> > +	int init(std::vector<utils::Duration> &shutters, std::vector<double> &gains);
> > +	void configure(utils::Duration minShutter, utils::Duration maxShutter,
> > +		       double minGain, double maxGain);
> > +
> > +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure);

This function and all the public functions below are const.

> > +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> > +								  utils::Duration fixedShutter);
> > +	std::tuple<utils::Duration, double, double> splitExposure(utils::Duration exposure,
> > +								  double fixedGain);
> > +
> > +	utils::Duration minShutter() { return minShutter_; };

Extra semicolumn after }. Same below.

> > +	utils::Duration maxShutter() { return maxShutter_; };
> > +	double minGain() { return minGain_; };
> > +	double maxGain() { return maxGain_; };
> > +
> > +private:
> > +	utils::Duration clampShutter(utils::Duration shutter);
> > +	double clampGain(double gain);
> > +
> > +	std::tuple<utils::Duration, double, double>
> > +	splitExposure(utils::Duration exposure,
> > +		      utils::Duration shutter, bool shutterFixed,
> > +		      double gain, bool gainFixed);
> > +
> > +	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',
> 
> This seems a really helpful addition overall !