[libcamera-devel,v4,05/14] ipa: ipu3: agc: Compute the gain for each frame

14575 diff mbox series

Now that we have the real exposure applied at each frame, remove the
early return based on a frame counter and compute the gain for each
frame.

Instead of that, introduce a number of startup frames during which the
filter speed is 1.0, meaning we apply instantly the exposure value
calculated and not a slower filtered one.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

Hi JM,

Thank you for the patch.

On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:
> Now that we have the real exposure applied at each frame, remove the
> early return based on a frame counter and compute the gain for each
> frame.
>
> Instead of that, introduce a number of startup frames during which the
> filter speed is 1.0, meaning we apply instantly the exposure value
> calculated and not a slower filtered one.


To me, it feels a 'because...' is coming at the end of the commit 
message. Perhaps, I don't have enough context, but I assume we want to 
converge fast while startup hence we use speed == 1? If you could add 
the explaination a bit, it would make more complete commit message for 
naive reviewers like me ;-)

>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------
>   1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 83aa3676..74bce7bb 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {
>   
>   LOG_DEFINE_CATEGORY(IPU3Agc)
>   
> -/* Number of frames to wait before calculating stats on minimum exposure */
> -static constexpr uint32_t kInitialFrameMinAECount = 4;
> -/* Number of frames to wait between new gain/shutter time estimations */
> -static constexpr uint32_t kFrameSkipCount = 6;
> -
>   /* Limits for analogue gain values */
>   static constexpr double kMinAnalogueGain = 1.0;
>   static constexpr double kMaxAnalogueGain = 8.0;
> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;
>    */
>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
>   
> +/* Number of frames to wait before calculating stats on minimum exposure */
> +static constexpr uint32_t kNumStartupFrames = 10;
> +
>   Agc::Agc()
> -	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> -	  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
> -	  currentExposure_(0s), prevExposureValue_(0s)
> +	: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),
> +	  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),
> +	  prevExposureValue_(0s)
>   {
>   }
>   
> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>   void Agc::filterExposure()
>   {
>   	double speed = 0.2;
> +
> +	/* Adapt instantly if we are in startup phase */
> +	if (frameCount_ < kNumStartupFrames)
> +		speed = 1.0;
> +
>   	if (filteredExposure_ == 0s) {
>   		/* DG stands for digital gain.*/
>   		filteredExposure_ = currentExposure_;
> @@ -185,13 +188,6 @@ void Agc::filterExposure()
>    */
>   void Agc::computeExposure(IPAFrameContext &frameContext)
>   {
> -	/* Algorithm initialization should wait for first valid frames */
> -	/* \todo - have a number of frames given by DelayedControls ?
> -	 * - implement a function for IIR */
> -	if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
> -		return;
> -
> -	lastFrame_ = frameCount_;
>   
>   	/* Are we correctly exposed ? */
>   	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {

Hi Umang

On 12/11/2021 09:37, Umang Jain wrote:
> Hi JM,
> 
> Thank you for the patch.
> 
> On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:
>> Now that we have the real exposure applied at each frame, remove the
>> early return based on a frame counter and compute the gain for each
>> frame.
>>
>> Instead of that, introduce a number of startup frames during which the
>> filter speed is 1.0, meaning we apply instantly the exposure value
>> calculated and not a slower filtered one.
> 
> 
> To me, it feels a 'because...' is coming at the end of the commit 
> message. Perhaps, I don't have enough context, but I assume we want to 
> converge fast while startup hence we use speed == 1? If you could add 
> the explaination a bit, it would make more complete commit message for 
> naive reviewers like me ;-)

Would it help ?
'''
Instead of that, introduce a number of startup frames during which the
filter speed is 1.0, meaning we apply instantly the exposure value
calculated and not a slower filtered one. This is used to have a faster
convergence, and those frames may ultimately be dropped.
'''

> 
>>
>> Signed-off-by: Jean-Michel Hautbois 
>> <jeanmichel.hautbois@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------
>>   1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp 
>> b/src/ipa/ipu3/algorithms/agc.cpp
>> index 83aa3676..74bce7bb 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {
>>   LOG_DEFINE_CATEGORY(IPU3Agc)
>> -/* Number of frames to wait before calculating stats on minimum 
>> exposure */
>> -static constexpr uint32_t kInitialFrameMinAECount = 4;
>> -/* Number of frames to wait between new gain/shutter time estimations */
>> -static constexpr uint32_t kFrameSkipCount = 6;
>> -
>>   /* Limits for analogue gain values */
>>   static constexpr double kMinAnalogueGain = 1.0;
>>   static constexpr double kMaxAnalogueGain = 8.0;
>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;
>>    */
>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
>> +/* Number of frames to wait before calculating stats on minimum 
>> exposure */
>> +static constexpr uint32_t kNumStartupFrames = 10;
>> +
>>   Agc::Agc()
>> -    : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>> -      minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
>> -      currentExposure_(0s), prevExposureValue_(0s)
>> +    : frameCount_(0), iqMean_(0.0), lineDuration_(0s), 
>> minExposureLines_(0),
>> +      maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),
>> +      prevExposureValue_(0s)
>>   {
>>   }
>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const 
>> ipu3_uapi_stats_3a *stats,
>>   void Agc::filterExposure()
>>   {
>>       double speed = 0.2;
>> +
>> +    /* Adapt instantly if we are in startup phase */
>> +    if (frameCount_ < kNumStartupFrames)
>> +        speed = 1.0;
>> +
>>       if (filteredExposure_ == 0s) {
>>           /* DG stands for digital gain.*/
>>           filteredExposure_ = currentExposure_;
>> @@ -185,13 +188,6 @@ void Agc::filterExposure()
>>    */
>>   void Agc::computeExposure(IPAFrameContext &frameContext)
>>   {
>> -    /* Algorithm initialization should wait for first valid frames */
>> -    /* \todo - have a number of frames given by DelayedControls ?
>> -     * - implement a function for IIR */
>> -    if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - 
>> lastFrame_ < kFrameSkipCount))
>> -        return;
>> -
>> -    lastFrame_ = frameCount_;
>>       /* Are we correctly exposed ? */
>>       if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {

Hi JM,

On 11/12/21 2:17 PM, Jean-Michel Hautbois wrote:
> Hi Umang
>
> On 12/11/2021 09:37, Umang Jain wrote:
>> Hi JM,
>>
>> Thank you for the patch.
>>
>> On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:
>>> Now that we have the real exposure applied at each frame, remove the
>>> early return based on a frame counter and compute the gain for each
>>> frame.
>>>
>>> Instead of that, introduce a number of startup frames during which the
>>> filter speed is 1.0, meaning we apply instantly the exposure value
>>> calculated and not a slower filtered one.
>>
>>
>> To me, it feels a 'because...' is coming at the end of the commit 
>> message. Perhaps, I don't have enough context, but I assume we want 
>> to converge fast while startup hence we use speed == 1? If you could 
>> add the explaination a bit, it would make more complete commit 
>> message for naive reviewers like me ;-)
>
> Would it help ?
> '''
> Instead of that, introduce a number of startup frames during which the
> filter speed is 1.0, meaning we apply instantly the exposure value
> calculated and not a slower filtered one. This is used to have a faster
> convergence, and those frames may ultimately be dropped.
> '''


Yes, this would help. Thank you.

>
>>
>>>
>>> Signed-off-by: Jean-Michel Hautbois 
>>> <jeanmichel.hautbois@ideasonboard.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>
>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>>
>>> ---
>>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------
>>>   1 file changed, 11 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp 
>>> b/src/ipa/ipu3/algorithms/agc.cpp
>>> index 83aa3676..74bce7bb 100644
>>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {
>>>   LOG_DEFINE_CATEGORY(IPU3Agc)
>>> -/* Number of frames to wait before calculating stats on minimum 
>>> exposure */
>>> -static constexpr uint32_t kInitialFrameMinAECount = 4;
>>> -/* Number of frames to wait between new gain/shutter time 
>>> estimations */
>>> -static constexpr uint32_t kFrameSkipCount = 6;
>>> -
>>>   /* Limits for analogue gain values */
>>>   static constexpr double kMinAnalogueGain = 1.0;
>>>   static constexpr double kMaxAnalogueGain = 8.0;
>>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;
>>>    */
>>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
>>> +/* Number of frames to wait before calculating stats on minimum 
>>> exposure */
>>> +static constexpr uint32_t kNumStartupFrames = 10;
>>> +
>>>   Agc::Agc()
>>> -    : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>>> -      minExposureLines_(0), maxExposureLines_(0), 
>>> filteredExposure_(0s),
>>> -      currentExposure_(0s), prevExposureValue_(0s)
>>> +    : frameCount_(0), iqMean_(0.0), lineDuration_(0s), 
>>> minExposureLines_(0),
>>> +      maxExposureLines_(0), filteredExposure_(0s), 
>>> currentExposure_(0s),
>>> +      prevExposureValue_(0s)
>>>   {
>>>   }
>>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const 
>>> ipu3_uapi_stats_3a *stats,
>>>   void Agc::filterExposure()
>>>   {
>>>       double speed = 0.2;
>>> +
>>> +    /* Adapt instantly if we are in startup phase */
>>> +    if (frameCount_ < kNumStartupFrames)
>>> +        speed = 1.0;
>>> +
>>>       if (filteredExposure_ == 0s) {
>>>           /* DG stands for digital gain.*/
>>>           filteredExposure_ = currentExposure_;
>>> @@ -185,13 +188,6 @@ void Agc::filterExposure()
>>>    */
>>>   void Agc::computeExposure(IPAFrameContext &frameContext)
>>>   {
>>> -    /* Algorithm initialization should wait for first valid frames */
>>> -    /* \todo - have a number of frames given by DelayedControls ?
>>> -     * - implement a function for IIR */
>>> -    if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - 
>>> lastFrame_ < kFrameSkipCount))
>>> -        return;
>>> -
>>> -    lastFrame_ = frameCount_;
>>>       /* Are we correctly exposed ? */
>>>       if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {

Quoting Umang Jain (2021-11-12 08:48:44)
> Hi JM,
> 
> On 11/12/21 2:17 PM, Jean-Michel Hautbois wrote:
> > Hi Umang
> >
> > On 12/11/2021 09:37, Umang Jain wrote:
> >> Hi JM,
> >>
> >> Thank you for the patch.
> >>
> >> On 11/11/21 7:39 PM, Jean-Michel Hautbois wrote:
> >>> Now that we have the real exposure applied at each frame, remove the
> >>> early return based on a frame counter and compute the gain for each
> >>> frame.
> >>>
> >>> Instead of that, introduce a number of startup frames during which the
> >>> filter speed is 1.0, meaning we apply instantly the exposure value
> >>> calculated and not a slower filtered one.
> >>
> >>
> >> To me, it feels a 'because...' is coming at the end of the commit 
> >> message. Perhaps, I don't have enough context, but I assume we want 
> >> to converge fast while startup hence we use speed == 1? If you could 
> >> add the explaination a bit, it would make more complete commit 
> >> message for naive reviewers like me ;-)
> >
> > Would it help ?
> > '''
> > Instead of that, introduce a number of startup frames during which the
> > filter speed is 1.0, meaning we apply instantly the exposure value
> > calculated and not a slower filtered one. This is used to have a faster
> > convergence, and those frames may ultimately be dropped.

That sounds like it might already exist in the code ... perhaps:

"""
and those frames may be dropped in a future development to hide the
convergance process from the viewer.
"""

> > '''
> 
> 
> Yes, this would help. Thank you.
> 
> >
> >>
> >>>
> >>> Signed-off-by: Jean-Michel Hautbois 
> >>> <jeanmichel.hautbois@ideasonboard.com>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >>
> >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> >>
> >>> ---
> >>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------
> >>>   1 file changed, 11 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp 
> >>> b/src/ipa/ipu3/algorithms/agc.cpp
> >>> index 83aa3676..74bce7bb 100644
> >>> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {
> >>>   LOG_DEFINE_CATEGORY(IPU3Agc)
> >>> -/* Number of frames to wait before calculating stats on minimum 
> >>> exposure */
> >>> -static constexpr uint32_t kInitialFrameMinAECount = 4;
> >>> -/* Number of frames to wait between new gain/shutter time 
> >>> estimations */
> >>> -static constexpr uint32_t kFrameSkipCount = 6;
> >>> -
> >>>   /* Limits for analogue gain values */
> >>>   static constexpr double kMinAnalogueGain = 1.0;
> >>>   static constexpr double kMaxAnalogueGain = 8.0;
> >>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;
> >>>    */
> >>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
> >>> +/* Number of frames to wait before calculating stats on minimum 
> >>> exposure */
> >>> +static constexpr uint32_t kNumStartupFrames = 10;
> >>> +
> >>>   Agc::Agc()
> >>> -    : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> >>> -      minExposureLines_(0), maxExposureLines_(0), 
> >>> filteredExposure_(0s),
> >>> -      currentExposure_(0s), prevExposureValue_(0s)
> >>> +    : frameCount_(0), iqMean_(0.0), lineDuration_(0s), 
> >>> minExposureLines_(0),
> >>> +      maxExposureLines_(0), filteredExposure_(0s), 
> >>> currentExposure_(0s),
> >>> +      prevExposureValue_(0s)
> >>>   {
> >>>   }
> >>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const 
> >>> ipu3_uapi_stats_3a *stats,
> >>>   void Agc::filterExposure()
> >>>   {
> >>>       double speed = 0.2;
> >>> +
> >>> +    /* Adapt instantly if we are in startup phase */
> >>> +    if (frameCount_ < kNumStartupFrames)
> >>> +        speed = 1.0;
> >>> +
> >>>       if (filteredExposure_ == 0s) {
> >>>           /* DG stands for digital gain.*/
> >>>           filteredExposure_ = currentExposure_;
> >>> @@ -185,13 +188,6 @@ void Agc::filterExposure()
> >>>    */
> >>>   void Agc::computeExposure(IPAFrameContext &frameContext)
> >>>   {
> >>> -    /* Algorithm initialization should wait for first valid frames */
> >>> -    /* \todo - have a number of frames given by DelayedControls ?
> >>> -     * - implement a function for IIR */
> >>> -    if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - 
> >>> lastFrame_ < kFrameSkipCount))
> >>> -        return;
> >>> -
> >>> -    lastFrame_ = frameCount_;
> >>>       /* Are we correctly exposed ? */
> >>>       if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {

Hi Jean-Michel,

On Thu, Nov 11, 2021 at 03:09:19PM +0100, Jean-Michel Hautbois wrote:
> Now that we have the real exposure applied at each frame, remove the
> early return based on a frame counter and compute the gain for each
> frame.

This is parsed as "this patch removes the early return" and "this patch
computes the gain for each frame".

> 
> Instead of that, introduce a number of startup frames during which the

And this "instead" reads like it contradicts the two points above :/

I think dropping "instead of that" or adding a bit more, like "instead
of the early return" would make it easier to read.

> filter speed is 1.0, meaning we apply instantly the exposure value
> calculated and not a slower filtered one.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

With those clarifications (and the elaboration that you sent to Umang),

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 83aa3676..74bce7bb 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {
>  
>  LOG_DEFINE_CATEGORY(IPU3Agc)
>  
> -/* Number of frames to wait before calculating stats on minimum exposure */
> -static constexpr uint32_t kInitialFrameMinAECount = 4;
> -/* Number of frames to wait between new gain/shutter time estimations */
> -static constexpr uint32_t kFrameSkipCount = 6;
> -
>  /* Limits for analogue gain values */
>  static constexpr double kMinAnalogueGain = 1.0;
>  static constexpr double kMaxAnalogueGain = 8.0;
> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;
>   */
>  static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
>  
> +/* Number of frames to wait before calculating stats on minimum exposure */
> +static constexpr uint32_t kNumStartupFrames = 10;
> +
>  Agc::Agc()
> -	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> -	  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
> -	  currentExposure_(0s), prevExposureValue_(0s)
> +	: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),
> +	  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),
> +	  prevExposureValue_(0s)
>  {
>  }
>  
> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>  void Agc::filterExposure()
>  {
>  	double speed = 0.2;
> +
> +	/* Adapt instantly if we are in startup phase */
> +	if (frameCount_ < kNumStartupFrames)
> +		speed = 1.0;
> +
>  	if (filteredExposure_ == 0s) {
>  		/* DG stands for digital gain.*/
>  		filteredExposure_ = currentExposure_;
> @@ -185,13 +188,6 @@ void Agc::filterExposure()
>   */
>  void Agc::computeExposure(IPAFrameContext &frameContext)
>  {
> -	/* Algorithm initialization should wait for first valid frames */
> -	/* \todo - have a number of frames given by DelayedControls ?
> -	 * - implement a function for IIR */
> -	if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
> -		return;
> -
> -	lastFrame_ = frameCount_;
>  
>  	/* Are we correctly exposed ? */
>  	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> -- 
> 2.32.0
>

Hi Paul,

On 13/11/2021 00:05, Paul Elder wrote:
> Hi Jean-Michel,
> 
> On Thu, Nov 11, 2021 at 03:09:19PM +0100, Jean-Michel Hautbois wrote:
>> Now that we have the real exposure applied at each frame, remove the
>> early return based on a frame counter and compute the gain for each
>> frame.
> 
> This is parsed as "this patch removes the early return" and "this patch
> computes the gain for each frame".
> 
>>
>> Instead of that, introduce a number of startup frames during which the
> 
> And this "instead" reads like it contradicts the two points above :/
> 
> I think dropping "instead of that" or adding a bit more, like "instead
> of the early return" would make it easier to read.

I will remove "Instead of that", thanks !

> 
>> filter speed is 1.0, meaning we apply instantly the exposure value
>> calculated and not a slower filtered one.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> With those clarifications (and the elaboration that you sent to Umang),
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 26 +++++++++++---------------
>>   1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 83aa3676..74bce7bb 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -45,11 +45,6 @@ namespace ipa::ipu3::algorithms {
>>   
>>   LOG_DEFINE_CATEGORY(IPU3Agc)
>>   
>> -/* Number of frames to wait before calculating stats on minimum exposure */
>> -static constexpr uint32_t kInitialFrameMinAECount = 4;
>> -/* Number of frames to wait between new gain/shutter time estimations */
>> -static constexpr uint32_t kFrameSkipCount = 6;
>> -
>>   /* Limits for analogue gain values */
>>   static constexpr double kMinAnalogueGain = 1.0;
>>   static constexpr double kMaxAnalogueGain = 8.0;
>> @@ -69,10 +64,13 @@ static constexpr double kEvGainTarget = 0.5;
>>    */
>>   static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100;
>>   
>> +/* Number of frames to wait before calculating stats on minimum exposure */
>> +static constexpr uint32_t kNumStartupFrames = 10;
>> +
>>   Agc::Agc()
>> -	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>> -	  minExposureLines_(0), maxExposureLines_(0), filteredExposure_(0s),
>> -	  currentExposure_(0s), prevExposureValue_(0s)
>> +	: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minExposureLines_(0),
>> +	  maxExposureLines_(0), filteredExposure_(0s), currentExposure_(0s),
>> +	  prevExposureValue_(0s)
>>   {
>>   }
>>   
>> @@ -159,6 +157,11 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>>   void Agc::filterExposure()
>>   {
>>   	double speed = 0.2;
>> +
>> +	/* Adapt instantly if we are in startup phase */
>> +	if (frameCount_ < kNumStartupFrames)
>> +		speed = 1.0;
>> +
>>   	if (filteredExposure_ == 0s) {
>>   		/* DG stands for digital gain.*/
>>   		filteredExposure_ = currentExposure_;
>> @@ -185,13 +188,6 @@ void Agc::filterExposure()
>>    */
>>   void Agc::computeExposure(IPAFrameContext &frameContext)
>>   {
>> -	/* Algorithm initialization should wait for first valid frames */
>> -	/* \todo - have a number of frames given by DelayedControls ?
>> -	 * - implement a function for IIR */
>> -	if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
>> -		return;
>> -
>> -	lastFrame_ = frameCount_;
>>   
>>   	/* Are we correctly exposed ? */
>>   	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
>> -- 
>> 2.32.0
>>