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

[Laurent Pinchart]

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.

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_; }

-- 
Regards,

Laurent Pinchart