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

[David Plowman]

Hi everyone

Thanks for cc'ing me on this question. Always happy to try and answer!

On Thu, 28 Mar 2024 at 10:11, Milan Zamazal <mzamazal at redhat.com> wrote:
>
> Hi Laurent,
>
> thank you for the review.
>
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
>
> > Hi Milan,
> >
> > (CC'ing David)
> >
> > Thank you for the patch.
> >
> > On Tue, Mar 19, 2024 at 01:36:04PM +0100, Milan Zamazal wrote:
> >> 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,
> >
> > s/on fly/on the fly/
> >
> >> 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.
> >
> > I don't think this is quite right. Strictly speaking, the black level
> > compensation is about subtracting the black level as produced by the
> > sensor. This requires tuning, and shouldn't be done on-the-fly.
>
> A general agreement from those who tried the patch was that it makes a
> noticeable improvement.  As explained in my comments above, this is a fallback
> mechanism that we can use cheaply and immediately, until or unless tuning file
> for software ISP is available.  So far, it seems to be beneficial rather than
> harmful.
>
> Unless we have something better or perfect soon, which doesn't seem to be the
> case to me, why not to use this simple and useful (I believe) fallback
> mechanism?  Do you have a better suggestion what could be done right now
> regarding this issue (not considering black level at all is a real issue)?

This is of course partly a technical issue - what's the right thing to
do? - and particularly a pragmatic issue - whether to do something
imperfect right now or not.

To tackle the technical question, black level is normally removed
fairly early in the pipeline, because not doing so will make typical
operations in the Bayer pipe go wrong. For example:

* Lens shading (vignetting correction). I don't know whether you have
this feature currently, but maybe it will be required at some point to
improve image quality? Doing this without first removing the black
level will make all the colours go wrong.

* White balance. This too will go wrong if you haven't subtracted the
black level. White balance is normally applied before demosaic because
demosaic typically works better with white-balanced pixels. Obviously
it depends what your demosaic algorithm does, but I wonder if there's
a future in which you have higher quality versions of some algorithms
for certain use cases (e.g. stills capture) where this could become an
issue.

* Colour correction. Like white balance, colour correction matrices
will not do the right thing if black level has not been removed.

You can probably guess that I belong more to the "calibrate the camera
properly" view of the world because doing that fixes problems and
makes them go away _forever_. But I'm not familiar with the background
here, so I can appreciate that interim solutions may have a place.

>
> > On the other hand, looking at the histogram to try and stretch it is
> > called contrast adjustment. There's nothing wrong in doing so, but it's
> > usually done towards the output of the processing pipeline, and is
> > associated with manual adjustments of the contrast and brightness. See
> > src/ipa/rpi/controller/rpi/contrast.cpp for instance.
>
> Contrast adjustment is a different issue than the one this patch tries to
> address and less important now.  I plan to work on it and similar functionality
> later.
>
> > David may be able to comment further as to why BLC and contrast are
> > quite different.

We treat contrast adjustment as a more cosmetic feature of an image
which users can adjust according to their own personal taste. They can
have more of it or less of it, as they wish, and it doesn't really
affect the other aspects of image, such as colours or image quality.

Black level is quite different. There's a known value that you should
use. There's basically no choice. Speaking for the Pi, I would be
highly reluctant to couple these two things together - you certainly
wouldn't want contrast adjustment to start changing the colours, for
example!

Anyway, I hope that provides a little bit of a comparison, at least
from the point of view of a different implementation. Good luck!

David

>
> I think I understand the difference but feel free to comment if you think I'm
> missing something.
>
> > As this patch may need more work, I propose not including it in v7 to
> > avoid delay merging the rest of the implementation.
>
> Please consider my comments above.  I'm open to suggestions how to do better at
> the moment.  Less comfortable with nothing should be done about black level
> until we have a perfect solution.
>
> Thanks,
> Milan
>