[PATCH v2 23/25] libtuning: Make blacklevel optional

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Fri, Jun 28, 2024 at 12:47:16PM +0200, Stefan Klug wrote:
> Not every sensor provides blacklevel information through the kernel
> interface.

Actually, unless I'm mistaken, none do.

Raspberry Pi is the only platform to report the black levels through the
libcamera API, and they come from the tuning file. There's a bit of a
circular dependency below :-)

The question is where black levels should come from. They are typically
the result of two phenomena:

- Dark currents and other physical effects that add charge to pixels in
  the absence of light. Those can depend on the integration time and the
  sensor die temperature, and their contribution to pixel values depend
  on the sensor gains.

- Pedestal added by the sensor to pixel values. They are typically
  fixed, sometimes programmable (in which case we could control them),
  and should be reported in datasheets (but documentation is not always
  available).

Offsetting the dark currents is the most difficult part. This can be
done in multiple ways, including

- Automatic BLC in the sensor based on dark lines. That's the easiest
  option, we only need to enable it and there's nothing else to do.

- Calculating black levels in the sensor based on dark lines, reporting
  them (through embedded data or I2C polling) to the host, and
  subtracting them in the ISP. This is more work, but does not require
  any tuning, so it's still not too bad.

- Reading the dark lines from the sensor, calculating the black levels
  in the ISP (or worst case in software) and subtracting them in the
  ISP. A bit more complicated than the previous option, and requires the
  ability to separate dark lines from visible image on the host side,
  which is not necessarily common.

- Calculating black levels dynamically from tuning data, based on the
  integration time, and temperature and the gains. This requires
  measurements and is the only option involving tuning.

The data pedestal needs to be subtracted regardless of how we handle the
dark currents, and is likely the biggest contribution to the overall
black level. If the value is fixed, it could be added to the sensor
database in libcamera. If it's programmable, then the driver should
report it (which would leave the question of what to set it to, but I
think that can be handled separately).

I think we can handle the dark currents separately, and later. For the
pedestal, let's add it to the sensor database to start with.

> We should therefore not require that information to be in the
> raw images for the tuning process. It can be provided via the tuning config.

I think that's not what it's for. The reason we need it for tuning is
that it should be subtracted from the raw image before we tune LSC
(assuming the ISP subtract the black level before it performs LSC).
Providing the value in the config file doesn't seem right.

> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  utils/tuning/libtuning/image.py | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/tuning/libtuning/image.py b/utils/tuning/libtuning/image.py
> index c8911a0ff125..0b9c78fcd9c2 100644
> --- a/utils/tuning/libtuning/image.py
> +++ b/utils/tuning/libtuning/image.py
> @@ -25,6 +25,8 @@ class Image:
>          self.color = -1
>          self.lux = -1
>          self.macbeth = None
> +        self.blacklevel = 0
> +        self.blacklevel_16 = 0
>  
>          try:
>              self._load_metadata_exif()
> @@ -74,8 +76,12 @@ class Image:
>          self.againQ8 = metadata[f'Exif.{photo}.ISOSpeedRatings'].value * 256 / 100
>          self.againQ8_norm = self.againQ8 / 256
>          self.camName = metadata['Exif.Image.Model'].value
> -        self.blacklevel = int(metadata[f'Exif.{subimage}.BlackLevel'].value[0])
> -        self.blacklevel_16 = self.blacklevel << (16 - self.sigbits)
> +        if f'Exif.{subimage}.BlackLevel' in metadata:
> +            self.blacklevel = int(metadata[f'Exif.{subimage}.BlackLevel'].value[0])
> +            self.blacklevel_16 = self.blacklevel << (16 - self.sigbits)
> +        else:
> +            logger.warning("Image doesn't contain blacklevel info")
> +
>  
>          # Channel order depending on bayer pattern
>          # The key is the order given by exif, where 0 is R, 1 is G, and 2 is B

-- 
Regards,

Laurent Pinchart