[libcamera-devel,10/12] ipa: ipu3: awb: Introduce Black Level Correction

13898 diff mbox series

The pixels output by the camera normally include a black level, because
sensors do not always report a signal level of '0' for black. Pixels at
or below this level should be considered black and to achieve that, we
need to substract an offset to all the pixels. This can be taken into
account by reading the lowest value of a special region on sensors which
is not exposed to the lens. This provides a substracting factor to be
able to adjust the expected black levels in the resultant images.

For a camera outputting 10-bit pixel values (in the range 0 to 1023) a
typical black level might be 64. It is a fixed value, obtained by
capturing a raw frame with minimum exposure and gain fixed to 1.0 while
covering the sensor (the darker the better). We consider it good enough
as a very first approximation, until we measure it during a tuning
process and include it in a configuration file

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

On Thu, Sep 23, 2021 at 10:16:23AM +0200, Jean-Michel Hautbois wrote:
> The pixels output by the camera normally include a black level, because
> sensors do not always report a signal level of '0' for black. Pixels at
> or below this level should be considered black and to achieve that, we
> need to substract an offset to all the pixels. This can be taken into
> account by reading the lowest value of a special region on sensors which
> is not exposed to the lens. This provides a substracting factor to be
> able to adjust the expected black levels in the resultant images.
> 
> For a camera outputting 10-bit pixel values (in the range 0 to 1023) a
> typical black level might be 64. It is a fixed value, obtained by
> capturing a raw frame with minimum exposure and gain fixed to 1.0 while
> covering the sensor (the darker the better). We consider it good enough
> as a very first approximation, until we measure it during a tuning
> process and include it in a configuration file
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index a5391653..3013870b 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -388,9 +388,17 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
>  	/* The CCM matrix may change when color temperature will be used */
>  	params->acc_param.ccm = imguCssCcmDefault;
>  
> +	/* The Optical Black Level correction values */

I think I'd like to see a clear todo highlighting that these values are
ad-hoc:

	/*
	 * \todo The correction values should come from sensor specific
	 * tuning processes. This is a first approximation
	 */

> +	params->obgrid_param.gr = 64;
> +	params->obgrid_param.r = 64;
> +	params->obgrid_param.b = 64;
> +	params->obgrid_param.gb = 64;
> +
>  	params->use.acc_awb = 1;
>  	params->use.acc_bnr = 1;
>  	params->use.acc_ccm = 1;

I'd be tempted by a blank line here to split out the obgrid to make it
clear that it's a separate component.

With those:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +	params->use.obgrid = 1;
> +	params->use.obgrid_param = 1;
>  }
>  
>  } /* namespace ipa::ipu3::algorithms */
> -- 
> 2.30.2
>

On Tue, Sep 28, 2021 at 05:10:56PM +0100, Kieran Bingham wrote:
> On Thu, Sep 23, 2021 at 10:16:23AM +0200, Jean-Michel Hautbois wrote:
> > The pixels output by the camera normally include a black level, because
> > sensors do not always report a signal level of '0' for black. Pixels at
> > or below this level should be considered black and to achieve that, we
> > need to substract an offset to all the pixels. This can be taken into
> > account by reading the lowest value of a special region on sensors which
> > is not exposed to the lens. This provides a substracting factor to be

s/the lens/light/

> > able to adjust the expected black levels in the resultant images.
> > 
> > For a camera outputting 10-bit pixel values (in the range 0 to 1023) a
> > typical black level might be 64. It is a fixed value, obtained by
> > capturing a raw frame with minimum exposure and gain fixed to 1.0 while
> > covering the sensor (the darker the better). We consider it good enough
> > as a very first approximation, until we measure it during a tuning
> > process and include it in a configuration file
> > 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > index a5391653..3013870b 100644
> > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > @@ -388,9 +388,17 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
> >  	/* The CCM matrix may change when color temperature will be used */
> >  	params->acc_param.ccm = imguCssCcmDefault;
> >  
> > +	/* The Optical Black Level correction values */
> 
> I think I'd like to see a clear todo highlighting that these values are
> ad-hoc:
> 
> 	/*
> 	 * \todo The correction values should come from sensor specific
> 	 * tuning processes. This is a first approximation
> 	 */

Indeed, this is really a hack, and should be documented as such.

> > +	params->obgrid_param.gr = 64;
> > +	params->obgrid_param.r = 64;
> > +	params->obgrid_param.b = 64;
> > +	params->obgrid_param.gb = 64;
> > +
> >  	params->use.acc_awb = 1;
> >  	params->use.acc_bnr = 1;
> >  	params->use.acc_ccm = 1;
> 
> I'd be tempted by a blank line here to split out the obgrid to make it
> clear that it's a separate component.

Why do we set the optical black level compensation parameters in AWB
though ? Shouldn't it be a separate algorithm ?

> With those:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +	params->use.obgrid = 1;
> > +	params->use.obgrid_param = 1;
> >  }
> >  
> >  } /* namespace ipa::ipu3::algorithms */