[libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC mean-based algorithm

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-10-26 07:13:17)
> Hi Kieran,
> 
> On 25/10/2021 23:26, Kieran Bingham wrote:
> > Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)
> >> The AGC class was not documented while developing. Extend that to
> >> reference the origins of the implementation, and improve the
> >> descriptions on how the algorithm operates internally.
> >>
> >> While at it, rename the functions which have bad names.
> > 
> > Aha, these are just renames. It threw me off thinking there were bigger
> > changes in here, but a rename is fine.
> > 
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >> ---
> >> v3:
> >> - rename processBrightness to measureBrightness
> >> - rename lockExposureGain to computeExposure
> >> - reword some comments
> >> ---
> >>   src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
> >>   src/ipa/ipu3/algorithms/agc.h   |  6 +--
> >>   2 files changed, 75 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index 6c151232..b145e522 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -2,7 +2,7 @@
> >>   /*
> >>    * Copyright (C) 2021, Ideas On Board
> >>    *
> >> - * ipu3_agc.cpp - AGC/AEC control algorithm
> >> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm
> >>    */
> >>   
> >>   #include "agc.h"
> >> @@ -17,17 +17,37 @@
> >>   
> >>   #include "libipa/histogram.h"
> >>   
> >> +/**
> >> + * \file agc.h
> >> + */
> >> +
> >>   namespace libcamera {
> >>   
> >>   using namespace std::literals::chrono_literals;
> >>   
> >>   namespace ipa::ipu3::algorithms {
> >>   
> >> +/**
> >> + * \class Agc
> >> + * \brief A mean-based auto-exposure algorithm
> >> + *
> >> + * This algorithm calculates a shutter time and an analogue gain so that the
> > 
> > Is it an analogue gain? or just a gain? Does the algorithm care if it is
> > analogue?
> > 
> > Perhaps it does, as it's targetted to the Sensor controls.
> 
> Quoting you and Laurent in v1:
> 
> Kieran:
> Surely the algorithm itself doesn't care if this is an analogue gain or 
> a digital gain.
> 
> Laurent:
> Analogue gain is applied in the sensor, before the ADC, while digital
> gain can be applied anywhere after the ADC. Most sensors will have a
> digital gain, which is usually unused, and ISPs also have digital gains.
> It's important to split gain between analogue and digital correctly as
> they have different implications (in terms of noise for instance), so
> the algorithm needs to care.
> 
> Kieran:
> The IPU3 will apply it as an analogue gain - but that's independent 
> isn't it ?
> 
> Laurent:
> If you meant the IPU3 IPA, yes, it applies the gain as analogue gain. If
> you meant the IPU3 hardware, no, it has no analogue gain.
> 
> Kieran:
> Presumably if we weren't able to apply it fully as analogue - the 
> remainder could be applied digitally to meet the desired gain requested 
>   by the algorithm?
> 
> Laurent:
> That's usually what a basic implementation will do, there could be
> reason to do something more complex.
> 
> Me:
> We apply analogue gains right now and should take care of digital gain 
> later :-).

Ok, lets leave this all as is then. Thanks for the refresh.

(perhaps in the future we'll have a way to maintain discussion history
alongside progressive patch versions...)

--
Kieran