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

[Hans de Goede]

Hi,

On 4/20/24 12:42 PM, Laurent Pinchart wrote:
> Hi Milan,
> 
> On Tue, Apr 16, 2024 at 05:31:38PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
>>
>>> 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.
>>
>> Hi Laurent,
>>
>> to make sure we understand each other correctly: Can we agree that "be moved
>> before debayering" above means primarily separating the level adjustments
>> computations from debayering code and building them (together with other stuff)
>> on top of libipa/algorithm.h?  Rather than first applying the corrections on the
>> data and only then perform debayering, which luxury we cannot afford, at least
>> on CPU?  I.e. the first step could be something like
>> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/6180d853005bb956b3d2469576b139ff523b76ad ?
> 
> What I meant is subtracting the black level before applying the colour
> gains and performing the CFA interpolation (a.k.a. debayering). I'm fine
> with the black level subtraction and CFA interpolation being performed
> in a single pass by a single function for performance reason, but the
> order needs to be swapped.

If we apply the per color lookup table (which does black-level compensation +
color-gain + gamma-curve in 1 step) to the raw bayer data then we either need to
do this on some extra copy before debayering or we need to do it multiple times
per pixel before averaging the surrounding pixels, neither one of which is ideal.

I guess we could make the memcpy to temp buffer behavior done to deal with
uncached mem mandatory (so always do this) and then first apply the rgb lookups
to a just copied raw line before debayering, then we still only need to do
this once per color. And this could also help wrt cache behavior of the
lookups since we then linearly travel through 1 line touching only the
1 line (in temp buffer) + read 2 lookup tables. So then there will be less
cache contention then when using the lookups during debayering when
were are reading 3 lines + writing 1 line + accessing all 3 lookup tables
(so I guess this might even speed things up especially on the imx8).

I don't expect this to make much difference for the resulting image though,
OTOH it will make some difference and testing has shown that the overhead
of using the extra memcpy buffers in cases where this is not necessary is
small. So I guess we could do this.

Milan can you extract what I have in mind from the above and is this
something you can work on, or shall I take a shot at this when I can schedule
some time for this?

Regards,

Hans