[libcamera-devel] [PATCH 04/13] ipa: ipu3: awb: Change minimal green threshold value

[Laurent Pinchart]

On Thu, Oct 14, 2021 at 11:50:22AM +0100, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-13 16:41:16)
> > When the zones are used for the grey world algorithm, we need a minimal
> > number of zones to make it relevant, and we want the green values to be
> > at least 32 today (with a maximum of 255).
> > 
> > This value is a bit high, as we now are correcting the black levels, and
> > a sudden change of colors can be visible for the end user when the gains
> > are applied.
> > 
> > Lower the value to 16.

If I understand the patch correctly, the most relevant part here is the
minimal green value for a zone to be considered. I would then write the
above as

When zones are used for the grey world algorithm, they are only
considered if their average green value is at least 32/255 to exclude
zones that are too dark and don't provide relevant colour information
(on the opposite side of the spectrum, saturated regions are excluded by
the ImgU statistics engine).

The algorithm requires a minimal number of zones that meet this criteria
in order to run. Now that we correct the black level, the 32/255 minimal
value is a bit high and prevents the algorithm for running in low-light
conditions. Lower the value to 16/255 to fix it.

> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/awb.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > index 30693923..b18874a3 100644
> > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > @@ -17,7 +17,7 @@ namespace ipa::ipu3::algorithms {
> >  
> >  LOG_DEFINE_CATEGORY(IPU3Awb)
> 
> Do you document the meaning of this constant, and what it is used for /
> effects of changing it in the later documentation series?
> 
> If not, could you add something please?
> 
> (Either in this patch, or later)

Agreed.

It could also make sense to capture the commit message or parts of it in
the documentation.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> Acked-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > -static constexpr uint32_t kMinGreenLevelInZone = 32;
> > +static constexpr uint32_t kMinGreenLevelInZone = 16;
> >  
> >  /**
> >   * \struct Accumulator

-- 
Regards,

Laurent Pinchart