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

[Milan Zamazal]

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)?

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

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