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

[Laurent Pinchart]

Hi Hans,

On Sun, Apr 21, 2024 at 02:16:06PM +0200, Hans de Goede wrote:
> On 4/20/24 12:42 PM, Laurent Pinchart wrote:
> > On Tue, Apr 16, 2024 at 05:31:38PM +0200, Milan Zamazal wrote:
> >> Laurent Pinchart 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.

It's not the full lookup table that needs to be applied before
debayering, it's only the black level subtraction and the colour gains.
The gamma curve needs to go after debayering, and additional histogram
stretching could go there too.

There's also a need to add a CCM matrix to the pipeline, multiplying the
RGB values obtained by debayering by a 3x3 matrix before applying the
gamma curve.

With that I think we would have a reasonable image quality already.
Additional processing, such as lens shading correction, defective pixel
correction and denoising, are out of scope for CPU processing in my
opinion. They could be implemented with a GPU soft ISP though.

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

Laurent Pinchart