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

[Dan Scally]

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 at ideasonboard.com> írta:
>
>> From: Paul Elder <paul.elder at ideasonboard.com>
>>
>> Add a helper for managing exposure modes and splitting exposure times
>> into shutter and gain values.
>>
>> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
>> ---
>> Changes in v2:
>>
>> 	- Expanded the documentation
>> 	- Dropped the overloads for fixed shutter / gain - the same
>> 	  functionality is instead done by setting min and max shutter and gain
>> 	  to the same value
>> 	- Changed ::init() to consume a vector of pairs instead of two separate
>> 	  vectors
>> 	- Reworked splitExposure()
>>
>>   src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++
>>   src/ipa/libipa/exposure_mode_helper.h   |  53 +++++
>>   src/ipa/libipa/meson.build              |   2 +
>>   3 files changed, 312 insertions(+)
>>   create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp
>>   create mode 100644 src/ipa/libipa/exposure_mode_helper.h
>>
>> [...]
>> +/**
>> + * \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