[PATCH v4 08/18] libcamera: software_isp: Add DebayerCpu class

[Hans de Goede]

Hi Milan,

Thnak you for all the reviews. For reviews where
I have not responded I have applied all suggested changes
for the next version.

On 3/4/24 5:58 PM, Milan Zamazal wrote:
> Hans de Goede <hdegoede at redhat.com> writes:
> 
>> Add CPU based debayering implementation. This initial implementation
>> only supports debayering packed 10 bits per pixel bayer data in
>> the 4 standard bayer orders.
>>
>> Doxygen documentation by Dennis Bonke.
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel at ucw.cz>
>> Reviewed-by: Pavel Machek <pavel at ucw.cz>
>> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
>> Co-developed-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>> Co-developed-by: Pavel Machek <pavel at ucw.cz>
>> Signed-off-by: Pavel Machek <pavel at ucw.cz>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> 
> This patch is quite difficult to understand and I wonder what happens if this
> piece of code needs to be modified a year or two later.  My primary problem with
> it is tracking all the line pointers and where they point to at each given
> moment.  I think more verbosity (code comments) is desirable unless other
> reviewers think it's crystal clear.

I tried to explain how this works already in the new comments in this
v4 series. I think it might be best for someone less familiar with
the code to add extra comments since to me the working is already clear.

Maybe you can prepare a follow-up patch to add some extra comments
in places where you believe that this is necessary ?


> 
>> ---
>> Changes in v3:
>> - Move debayer_cpu.h to src/libcamera/software_isp/
>> - Move documentation to .cpp file
>> - Document how/why an array of src pointers is passed to
>>   the debayer functions
>> ---
>>  src/libcamera/software_isp/debayer_cpu.cpp | 619 +++++++++++++++++++++
>>  src/libcamera/software_isp/debayer_cpu.h   | 143 +++++
>>  src/libcamera/software_isp/meson.build     |   1 +
>>  3 files changed, 763 insertions(+)
>>  create mode 100644 src/libcamera/software_isp/debayer_cpu.cpp
>>  create mode 100644 src/libcamera/software_isp/debayer_cpu.h
>>
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> new file mode 100644
>> index 00000000..53e90776
>> --- /dev/null
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> 
> [...]
> 
>> +/**
>> + * \brief Constructs a DebayerCpu object.
>> + * \param[in] stats Pointer to the stats object to use.
>> + */
>> +DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>> +	: stats_(std::move(stats)), gamma_correction_(1.0)
>> +{
>> +#ifdef __x86_64__
>> +	enableInputMemcpy_ = false;
>> +#else
>> +	enableInputMemcpy_ = true;
>> +#endif
>> +	/* Initialize gamma to 1.0 curve */
>> +	for (unsigned int i = 0; i < kGammaLookupSize; i++)
>> +		gamma_[i] = i / 4;
> 
> What is this division by 4?  Is it related to kGammaLookupSize being 1024?
> If yes then it should derive from that constant.

Good point, I will replace this with a a calculation using kGammaLookupSize
for the next version.


> 
> [...]
> 
>> +void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])
>> +{
>> +	const int width_in_bytes = window_.width * 5 / 4;
>> +	const uint8_t *prev = (const uint8_t *)src[0];
>> +	const uint8_t *curr = (const uint8_t *)src[1];
>> +	const uint8_t *next = (const uint8_t *)src[2];
>> +
>> +	for (int x = 0; x < width_in_bytes; x++) {
>> +		/* Even pixel */
>> +		RGGB_BGR888(2, 1, 1)
>> +		/* Odd pixel RGGB -> GRBG*/
> 
> Missing space before */.

Ack, will fix.

> [...]
> 
>> +void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)
>> +{
>> +	unsigned int y_end = window_.y + window_.height;
>> +	/* Holds [0] previous- [1] current- [2] next-line */
>> +	const uint8_t *linePointers[3];
>> +
>> +	/* Adjust src to top left corner of the window */
>> +	src += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;
>> +
>> +	/* [x] becomes [x - 1] after initial shiftLinePointers() call */
>> +	if (window_.y) {
>> +		linePointers[1] = src - inputConfig_.stride; /* previous-line */
>> +		linePointers[2] = src;
>> +	} else {
>> +		/* window_.y == 0, use the next line as prev line */
> 
> Why is this needed here and not in process4?

This comes from:

https://github.com/jwrdegoede/libcamera/commit/df717903fd99b371efde20314991df2e08e4c22b

Which was later squashed into the main commit adding the DebayerCpu class.

If you look at: DebayerCpu::sizes() then you see that normally
it uses:

	unsigned int border_height = pattern_size.height;

And there is a special case for bayer patterns with a pattern height of 2
(which are actually all standard bayer patterns):

	/* No need for top/bottom border with a pattern height of 2 */
	if (pattern_size.height == 2)
		border_height = 0;

So for the pattern-height = 4 case there is a top and bottom border of
4 extra pixels and doing e.g. "src - 2 * srcStride" still points to
valid data because the initial src value starts at line 4 or more
of the raw src data.

The code used to do the same thing for pattern-height = 2, but testing on a HP
laptop with a hi556 sensor showed that that sensor (or at least the current
driver for that sensor) can produce a max raw-bayer data height of 722,
so we could not do 1280x720 there after adding a 2 byte border to both
the top and bottom which in turn confused the webrtc code in firefox.

Since I already introduced the line-pointers for the sliding window
with memcpy-ed buffers, the top/bottom padding was no longer strictly
necessary. So I decided to remove it to make things work with the hi556
sensor:

https://github.com/jwrdegoede/libcamera/commit/df717903fd99b371efde20314991df2e08e4c22b

>> +		linePointers[1] = src + inputConfig_.stride;
>> +		linePointers[2] = src;
>> +		/* Last 2 lines also need special handling */
>> +		y_end -= 2;
>> +	}
>> +
>> +	setupInputMemcpy(linePointers);
>> +
>> +	for (unsigned int y = window_.y; y < y_end; y += 2) {
>> +		shiftLinePointers(linePointers, src);
>> +		memcpyNextLine(linePointers);
>> +		stats_->processLine0(y, linePointers);
>> +		(this->*debayer0_)(dst, linePointers);
>> +		src += inputConfig_.stride;
>> +		dst += outputConfig_.stride;
>> +
>> +		shiftLinePointers(linePointers, src);
>> +		memcpyNextLine(linePointers);
>> +		(this->*debayer1_)(dst, linePointers);
>> +		src += inputConfig_.stride;
>> +		dst += outputConfig_.stride;
>> +	}
>> +
>> +	if (window_.y == 0) {
>> +		shiftLinePointers(linePointers, src);
>> +		memcpyNextLine(linePointers);

Without memcpy-ing (1) we have the following linePointer values
(with lines[] being a virtual array which contains pointers
to the start of each line):

linePointers[0] = lines[window_.height - 3];
linePointers[1] = lines[window_.height - 2];
linePointers[2] = lines[window_.height - 1];

Note that linePointers[2] now points to the very last line of
the input buffer. Since there is no bottom border we cannot
go further then this!

>> +		stats_->processLine0(y_end, linePointers);
>> +		(this->*debayer0_)(dst, linePointers);
>> +		src += inputConfig_.stride;
>> +		dst += outputConfig_.stride;
>> +
>> +		shiftLinePointers(linePointers, src);
>> +		/* next line may point outside of srclinePointers[2], use prev. */
>> +		linePointers[2] = linePointers[0];
> 
> At least this deserves a more elaborate comment; honestly, I'm lost what's
> happening here exactly.

After the shiftLinePointers() we now have:

linePointers[0] = lines[window_.height - 2];
linePointers[1] = lines[window_.height - 1];
linePointers[2] = lines[window_.height];

Note linePointers[2] now points outside of the valid area of our
src buffer!

So we need to fixup linePointers[2]. And it needs to point to
a line matching the expected bayer pattern (which has different
colors in odd/even lines) so to keep linePointers[2] pointing
to an even line we do:

		linePointers[2] = linePointers[0];

resulting in:

linePointers[0] = lines[window_.height - 2];
linePointers[1] = lines[window_.height - 1];
linePointers[2] = lines[window_.height - 2];

So instead of giving the debayer code a pointer to the line above and
below the current line for interpolating, we are giving it the line
above the current line twice (since there is no line below to give it)
since the line above and below have the same bayer order this will still
result in correct output.

This really is just a small trick to avoid the need for a border / for
extra rows at the top/bottom of the src buffer.


1) With memcpy-ing the linePointers each point to one of the 3 lineBuffers_
holding the contents of these lines.

Also note that we don't need to do the memcpy for debayering the last line
since we are simply making linePointers[2] point to the lineBuffers_[] entry
which already contains a copy of lines[window_.height - 2]

Regards,

Hans