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

[Laurent Pinchart]

Hi Dan,

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.

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