[libcamera-devel] [PATCH v1 7/7] ipa: ipu3: Introduce Black Level Correction

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Aug 23, 2021 at 02:49:37PM +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.

That's a bit misleading, I read it as a need to clamp the value of
pixels below a certain threshold to 0, while what we need it subtracting
an offset from all 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 scaling factor to be able to

Scaling is a multiplication.

> 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, but the correct value would be found in
> the sensor manaufacturer’s datasheet.

s/manaufacturer/manufacturer/

except that it's not found in datasheets, especially given that it
varies based on the temperature (among other things).

64 isn't a "typical" value. It's a fixed value (how did you obtain it
btw ?) that... we consider 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 at 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 294871b1..ad3784d3 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -366,9 +366,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 */

This deserves a todo comment to explain this is an initial value that
needs to be moved to a configuration file.

> +	params->obgrid_param.gr = 64;
> +	params->obgrid_param.r = 63;
> +	params->obgrid_param.b = 63;
> +	params->obgrid_param.gb = 64;

Why 64/63 ?

Does the unit of the value depend on the bpp of the sensor (that is, the
value will be subtracted from the pixel value without being scaled
first), or is it expressed in a fixed unit (for instance 10, 12 or
16bpp, in which case it will be scaled by the ImgU before subtraction) ?

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

-- 
Regards,

Laurent Pinchart