[libcamera-devel] [PATCH] ipa: ipu3: agc: Remove the threshold for the histogram calculation

[Jean-Michel Hautbois]

Hi Laurent,

On 16/11/2021 15:59, Laurent Pinchart wrote:
> Hello,
> 
> On Tue, Nov 16, 2021 at 02:41:14PM +0000, Kieran Bingham wrote:
>> Quoting Jean-Michel Hautbois (2021-11-16 08:09:56)
>>> Until f8f07f94 (ipa: ipu3: agc: Improve gain calculation, 2021-11-04)
> 
> s/Until/Until commit/
> 
> I'd drop the date, and expand the commit ID to 12 characters as usual.

OK, this is what git log --pretty=reference returned, I will use 
--abbrev instead.

> 
>>> the gain to apply on the exposure value was only using the histogram.
>>> Now that the global brightness of the frame is estimated too, we don't
>>> need to remove part of the saturated pixels from the equation anymore.
> 
> I'm not sure to understand why the above commit changed the situation,
> but I also don't understand why we needed to take saturation into
> account here in the first place, so the change looks good to me overall.
> 
> Commit f8f07f94 also added a histogram empty check. Do we still need it
> ?

I don't think we need it anymore... as I can't see why the histogram 
could be empty now. I may remove it too in v2 then.

> 
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>
>> This is still working quite well here. No perceived regression as far
>> as I can tell, and it's still coping well with backlights and skylights.
>>
>> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> ---
>>>   src/ipa/ipu3/algorithms/agc.cpp | 24 ++++++++----------------
>>>   1 file changed, 8 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>>> index 4e424857..f67a79d3 100644
>>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>>> @@ -58,12 +58,6 @@ static constexpr uint32_t knumHistogramBins = 256;
>>>   /* Target value to reach for the top 2% of the histogram */
>>>   static constexpr double kEvGainTarget = 0.5;
>>>   
>>> -/*
>>> - * Maximum ratio of saturated pixels in a cell for the cell to be considered
>>> - * non-saturated and counted by the AGC algorithm.
>>> - */
>>> -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;
>>>   
>>> @@ -133,16 +127,14 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>>>                                          &stats->awb_raw_buffer.meta_data[cellPosition]
>>>                                  );
>>>   
>>> -                       if (cell->sat_ratio <= kMinCellsPerZoneRatio) {
>>> -                               uint8_t gr = cell->Gr_avg;
>>> -                               uint8_t gb = cell->Gb_avg;
>>> -                               /*
>>> -                                * Store the average green value to estimate the
>>> -                                * brightness. Even the overexposed pixels are
>>> -                                * taken into account.
>>> -                                */
>>> -                               hist[(gr + gb) / 2]++;
>>> -                       }
>>> +                       uint8_t gr = cell->Gr_avg;
>>> +                       uint8_t gb = cell->Gb_avg;
>>> +                       /*
>>> +                        * Store the average green value to estimate the
>>> +                        * brightness. Even the overexposed pixels are
>>> +                        * taken into account.
>>> +                        */
>>> +                       hist[(gr + gb) / 2]++;
>>>                  }
>>>          }
>>>   
>