[PATCH 2/3] ipa: libipa: Adjust for flicker in ExposureModeHelper

[Dan Scally]

Hi Laurent

On 22/01/2025 13:09, Laurent Pinchart wrote:
> Hi Dan,
>
> On Tue, Jan 21, 2025 at 09:29:01PM +0000, Daniel Scally wrote:
>> On 21/01/2025 17:17, Laurent Pinchart wrote:
>>> On Tue, Jan 21, 2025 at 04:30:19PM +0000, Daniel Scally wrote:
>>>> On 21/01/2025 16:27, Laurent Pinchart wrote:
>>>>> On Fri, Jan 17, 2025 at 02:34:09PM +0000, Daniel Scally wrote:
>>>>>> Update the ExposureModeHelper class to compensate for flickering
>>>>>> light sources in the ::splitExposure() function. The adjustment
>>>>> ExposureModeHelper::splitExposure() or just splitExposure()
>>>> The former :)
>>>>
>>>>>> simply caps exposure time at a multiple of the given flicker period
>>>>>> and compensates for any loss in the effective exposure value by
>>>>>> increasing analogue and then digital gain.
>>>>>>
>>>>>> Initially in the one call-site for this function, a flicker period
>>>>>> of 0us is passed, making this a no-op.
>>>>>>
>>>>>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
>>>>>> ---
>>>>>>     src/ipa/libipa/agc_mean_luminance.cpp   |  2 +-
>>>>>>     src/ipa/libipa/exposure_mode_helper.cpp | 20 +++++++++++++++++++-
>>>>>>     src/ipa/libipa/exposure_mode_helper.h   |  2 +-
>>>>>>     3 files changed, 21 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>>>>>> index 02555a44..b5e6afe3 100644
>>>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>>>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>>>>>> @@ -560,7 +560,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>>>>>>     	newExposureValue = filterExposure(newExposureValue);
>>>>>>     
>>>>>>     	frameCount_++;
>>>>>> -	return exposureModeHelper->splitExposure(newExposureValue);
>>>>>> +	return exposureModeHelper->splitExposure(newExposureValue, 0us);
>>>>>>     }
>>>>>>     
>>>>>>     /**
>>>>>> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
>>>>>> index f235316d..038aa33c 100644
>>>>>> --- a/src/ipa/libipa/exposure_mode_helper.cpp
>>>>>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
>>>>>> @@ -121,6 +121,7 @@ double ExposureModeHelper::clampGain(double gain) const
>>>>>>     /**
>>>>>>      * \brief Split exposure into exposure time and gain
>>>>>>      * \param[in] exposure Exposure value
>>>>>> + * \param[in] flickerPeriod The period of a flickering light source
>>>>>>      *
>>>>>>      * This function divides a given exposure into exposure time, analogue and
>>>>>>      * digital gain by iterating through stages of exposure time and gain limits.
>>>>>> @@ -147,10 +148,15 @@ double ExposureModeHelper::clampGain(double gain) const
>>>>>>      * required exposure, the helper falls-back to simply maximising the exposure
>>>>>>      * time first, followed by analogue gain, followed by digital gain.
>>>>>>      *
>>>>>> + * Once the exposure time has been determined from the modes, an adjustment is
>>>>>> + * made to compensate for a flickering light source by fixing the exposure time
>>>>>> + * to an exact multiple of the flicker period. Any effective exposure value that
>>>>>> + * is lost is added back via analogue and digital gain.
>>>>>> + *
>>>>>>      * \return Tuple of exposure time, analogue gain, and digital gain
>>>>>>      */
>>>>>>     std::tuple<utils::Duration, double, double>
>>>>>> -ExposureModeHelper::splitExposure(utils::Duration exposure) const
>>>>>> +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const
>>>>> We could avoid magic values by passing a std::optional<utils::Duration>.
>>>>> What do you think ?
>>>> Good idea
>>>>
>>>>>>     {
>>>>>>     	ASSERT(maxExposureTime_);
>>>>>>     	ASSERT(maxGain_);
>>>>>> @@ -205,6 +211,18 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
>>>>>>     	 * exposure time is maxed before gain is touched at all.
>>>>>>     	 */
>>>>>>     	exposureTime = clampExposureTime(exposure / clampGain(stageGain));
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Finally, if we have been given a flicker period we need to reduce the
>>>>>> +	 * exposure time to be a multiple of that period (if possible). The
>>>>>> +	 * effect of this should be to hide the flicker.
>>>>>> +	 */
>>>>>> +	if (flickerPeriod > 0us && flickerPeriod < exposureTime) {
>>>>>> +		unsigned int flickerPeriods = exposureTime / flickerPeriod;
>>>>>> +		if (flickerPeriods)
>>>>>> +			exposureTime = flickerPeriods * flickerPeriod;
>>>>>> +	}
>>>>> This means that the exposure time could become lower than
>>>>> minExposureTime_. Is that an issue ?
>>>> I doubt it given likely exposure times and flicker periods, but I can
>>>> extend it to guarantee that doesn't happen.
>>>>
>>>>> Should we define what controls take
>>>>> precedence in that case ? For instance, when manual exposure time is
>>>>> enabled, should the flicker period be ignored ?
>>>> Yes, and is. This only takes effect in automatic exposure mode.
>>> Is it ? If exposure time is manual and gain is automatic, the above code
>>> still gets run as far as I can tell.
>> This calculation still runs as is, but the rkisp1 and mali-c55 IPAs both take exposure time from a
>> different field in the IPA context if they are running in manual mode than the field this value is
>> set to, and the IPU3 IPA has no handling of controls for AGC at all at the moment.
> That doesn't seem right. It means the helpers will split exposure in
> exposure time and gain without knowing that one of the two will be
> overridden. It's not an issue with this patch, but I think it needs to
> be fixed.
Sorry - I should have been clearer - analogue and digital gain are treated the same way. Either all 
three sensor controls / ISP parameters are set from the manual values passed in through libcamera 
controls, or all three values are set from the automatic values calculated here. I don't think there 
would be a situation in which only one of the values calculated here was overridden.
>
> Paul, what do you think ?
>
>>> The interactions between the flicker and AE controls also need to be
>>> documented in the definition of the appropriate controls.
>>>
>>>>>> +
>>>>>>     	gain = clampGain(exposure / exposureTime);
>>>>>>     
>>>>>>     	return { exposureTime, gain, exposure / (exposureTime * gain) };
>>>>>> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
>>>>>> index c5be1b67..a5fcc366 100644
>>>>>> --- a/src/ipa/libipa/exposure_mode_helper.h
>>>>>> +++ b/src/ipa/libipa/exposure_mode_helper.h
>>>>>> @@ -28,7 +28,7 @@ public:
>>>>>>     		       double minGain, double maxGain);
>>>>>>     
>>>>>>     	std::tuple<utils::Duration, double, double>
>>>>>> -	splitExposure(utils::Duration exposure) const;
>>>>>> +	splitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const;
>>>>>>     
>>>>>>     	utils::Duration minExposureTime() const { return minExposureTime_; }
>>>>>>     	utils::Duration maxExposureTime() const { return maxExposureTime_; }