[libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and simplify the brightness loop

[Kieran Bingham]

On 29/09/2021 10:02, Jean-Michel Hautbois wrote:
> On 28/09/2021 18:26, Kieran Bingham wrote:
>> On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:
>>> Now that we know how exactly the AWB statistics are formatted, use a
>>> simplified loop in processBrightness() to parse the green values and get the
>>> histogram.
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>> ---
>>>  src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------
>>>  src/ipa/ipu3/algorithms/agc.h   |  2 ++
>>>  2 files changed, 23 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>>> index 5ff50f4a..ebbc5676 100644
>>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>>> @@ -6,6 +6,7 @@
>>>   */
>>>  
>>>  #include "agc.h"
>>> +#include "awb.h"
>>>  
>>>  #include <algorithm>
>>>  #include <chrono>
>>> @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;
>>>  static constexpr uint32_t knumHistogramBins = 256;
>>>  static constexpr double kEvGainTarget = 0.5;
>>>  
>>> -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
>>> -static constexpr uint8_t kCellSize = 8;
>>> -
>>>  Agc::Agc()
>>>  	: frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>>>  	  maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
>>> @@ -60,6 +58,10 @@ Agc::Agc()
>>>  int Agc::configure([[maybe_unused]] IPAContext &context,
>>
>> looks like context is no longer [[maybe_unused]].
>>
>>
>>>  		   const IPAConfigInfo &configInfo)
>>>  {
>>> +	const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;
>>> +	/* The grid is aligned to the next multiple of 4 */
>>> +	stride_ = (grid.width + 3) / 4 * 4;
>>
>> This is familiar...
>>
>> How about putting a stride into context.configuration.grid.stride and
>> calculating it once during calculateBdsGrid()?
>>
> 
> Then I need to pass the grid to the processBrightness(), again (and in
> AWB too) while we could get rid of it. Or cache the value in configure() ?

I think it's fine, and better to cache it in configure.

--
Kieran