[2/3] ipa: libipa: Adjust for flicker in ExposureModeHelper
diff mbox series

Message ID 20250117143410.20363-3-dan.scally@ideasonboard.com
State New
Headers show
Series
  • Add flicker mitigation controls to AgcMeanLuminance
Related show

Commit Message

Daniel Scally Jan. 17, 2025, 2:34 p.m. UTC
Update the ExposureModeHelper class to compensate for flickering
light sources in the ::splitExposure() function. The adjustment
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@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(-)

Comments

Laurent Pinchart Jan. 21, 2025, 4:27 p.m. UTC | #1
Hi Dan,

Thank you for the patch.

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()

> 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@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 ?

>  {
>  	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 ? Should we define what controls take
precedence in that case ? For instance, when manual exposure time is
enabled, should the flicker period be ignored ?

> +
>  	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_; }
Daniel Scally Jan. 21, 2025, 4:30 p.m. UTC | #2
Hi Laurent

On 21/01/2025 16:27, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> 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@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.

>
>> +
>>   	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_; }
Laurent Pinchart Jan. 21, 2025, 5:17 p.m. UTC | #3
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@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_; }
Daniel Scally Jan. 21, 2025, 9:29 p.m. UTC | #4
Hi Laurent

On 21/01/2025 17:17, Laurent Pinchart wrote:
> 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@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.
>
> 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_; }
Laurent Pinchart Jan. 24, 2025, 12:35 a.m. UTC | #5
On Wed, Jan 22, 2025 at 11:31:29PM -0500, Paul Elder wrote:
> On Wed, Jan 22, 2025 at 03:22:11PM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 22, 2025 at 01:15:43PM +0000, Daniel Scally wrote:
> > > On 22/01/2025 13:09, Laurent Pinchart wrote:
> > > > 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@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.
> 
> I would also be more comfortable with a clamp.
> 
> Also, do we not have to worry about cameras that have really high
> framerate?

It depends what you mean by "very high". At the moment we assume that
libcamera can run all IPA algorithms per frame. That limits the frame
rate. For high frames rates we'll have to reconsider that, and also
likely implement a batch API in V4L2.

> > > >>>>> 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.
> > 
> > With the AEGC series that we merged recently, the exposure time mode can
> > be set to manual and the gain mode to automatic.
> > 
> > > > Paul, what do you think ?
> 
> Ah, I see the problem, I forgot to plumb those sub-controls into the
> exposure mode helper.
> 
> I don't think this has to be addressed in this series though, and we can
> do it on top (otherwise that fix and this series will start
> conflicting).

Fine with me. Could you please coordinate with Dan to implement this in
the near future ?

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

Patch
diff mbox series

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