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

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> 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.

I agree with this approach.  I added your comment to the TODO file.

>> > 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.