[PATCH v5 17/18] libcamera: software_isp: Apply black level compensation

[Kieran Bingham]

Quoting Milan Zamazal (2024-03-12 13:48:30)
> Hans de Goede <hdegoede at redhat.com> writes:
> 
> > From: Milan Zamazal <mzamazal at redhat.com>
> >
> > Black may not be represented as 0 pixel value for given hardware, it may be
> > higher.  If this is not compensated then various problems may occur such as low
> > contrast or suboptimal exposure.
> >
> > The black pixel value can be either retrieved from a tuning file for the given
> > hardware, or automatically on fly.  The former is the right and correct method,
> > while the latter can be used when a tuning file is not available for the given
> > hardware.  Since there is currently no support for tuning files in software ISP,
> > the automatic, hardware independent way, is always used.  Support for tuning
> > files should be added in future but it will require more work than this patch.
> >
> > The patch looks at the image histogram and assumes that black starts when pixel
> > values start occurring on the left.  A certain amount of the darkest pixels is
> > ignored; it doesn't matter whether they represent various kinds of noise or are
> > real, they are better to omit in any case to make the image looking better.  It
> > also doesn't matter whether the darkest pixels occur around the supposed black
> > level or are spread between 0 and the black level, the difference is not
> > important.
> >
> > An arbitrary threshold of 2% darkest pixels is applied; there is no magic about
> > that value.
> >
> > The patch assumes that the black values for different colors are the same and
> > doesn't attempt any other non-primitive enhancements.  It cannot completely
> > replace tuning files and simplicity, while providing visible benefit, is its
> > goal.  Anything more sophisticated is left for future patches.
> >
> > A possible cheap enhancement, if needed, could be setting exposure + gain to
> > minimum values temporarily, before setting the black level.  In theory, the
> > black level should be fixed but it may not be reached in all images.  For this
> > reason, the patch updates black level only if the observed value is lower than
> > the current one; it should be never increased.
> >
> > The purpose of the patch is to compensate for hardware properties.  General
> > image contrast enhancements are out of scope of this patch.
> >
> > Stats are still gathered as an uncorrected histogram, to avoid any confusion and
> > to represent the raw image data.  Exposure must be determined after the black
> > level correction -- it has no influence on the sub-black area and must be
> > correct after applying the black level correction.  The granularity of the
> > histogram is increased from 16 to 64 to provide a better precision (there is no
> > theory behind either of those numbers).
> >
> > Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> > Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > ---
> 
> [...]
> 
> > +void BlackLevel::update(SwIspStats::histogram &yHistogram)
> > +{
> > +     // The constant is selected to be "good enough", not overly conservative or
> > +     // aggressive. There is no magic about the given value.

Nit here too... for block comments we use kernel styles not C++ style.

 /*
  * The constant is selected to be "good enough", not overly
  * conservative or aggressive. There is no magic about the given value.
  */

> > +     constexpr float ignoredPercentage_ = 0.02;
> > +     const unsigned int total =
> > +             std::accumulate(begin(yHistogram), end(yHistogram), 0);
> > +     const unsigned int pixelThreshold = ignoredPercentage_ * total;
> > +     const unsigned int currentBlackIdx =
> > +             blackLevel_ / (256 / SwIspStats::kYHistogramSize);
> 
> Should the newly introduced kRGBLookupSize constant be used instead of 256 here
> and in other places in this patch?
> 
> [...]
>