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

[Laurent Pinchart]

Hello,

On Thu, Mar 28, 2024 at 03:47:32PM +0100, Milan Zamazal wrote:
> David Plowman writes:
> 
> > Hi everyone
> >
> > Thanks for cc'ing me on this question. Always happy to try and answer!
> 
> Hi David,
> 
> thank you for your answer and explanations.

Likewise, thank you.

> > On Thu, 28 Mar 2024 at 10:11, Milan Zamazal wrote:
> >> Laurent Pinchart writes:
> >> > 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.
> 
> Yes.
> 
> > 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.
> 
> We don't have lens shading or color correction, once we have support for these
> there is no reason not to have proper black level correction as well.  As for
> white balance, we currently use probably the most primitive method possible
> (green/red and green/blue ratios) and we don't subtract black level even with
> this patch (which should get fixed later).  The patch subtracts black level when
> correcting exposure though, which is also influenced by this.  Considering all
> the imperfections at the moment, the actual effects on white balance and
> exposure may not be that big to care very much.  But even then ...
> 
> > 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.
> 
> ... the thing is that not applying any black level correction, even when not
> considering white balance and exposure deformations, often makes the image
> looking noticeably flat as there are no blacks.  I'd say better imperfect blacks
> than no blacks, similarly to having imperfect rather than no white balance
> correction (yes, comparing apples and oranges technically, but both have their
> impacts on the user satisfaction with the output).
> 
> >> > 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.
> 
> Yes, as for contrast, we quickly rejected the idea of handling it automatically
> and decided to implement the proper user adjustable settings later.
> 
> > 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!
> 
> Exactly.  This is why I'm advocating for still considering the patch rather than
> dropping it.  At least in my setup the auto-determined black value doesn't seem
> to be way off and serves its purpose even without access to exact tuning data.

As for other part of this series, I'm fine starting with this initial
implementation and improving it, but I think the black level should
eventually be moved before debayering, and ideally the colour gains as
well. I understand the need for optimizations to lower the CPU
consumption, but at the same time I don't feel comfortable building up
on top of an implementation that may work a bit more by chance than by
correctness, as that's not very maintainable.

> > Anyway, I hope that provides a little bit of a comparison, at least
> > from the point of view of a different implementation. Good luck!
> >
> >> 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.

-- 
Regards,

Laurent Pinchart