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

[Milan Zamazal]

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.

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

[...]

> +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 */.

[...]

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

> +		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);
> +		stats_->processLine0(y_end, linePointers);
> +		(this->*debayer0_)(dst, linePointers);
> +		src += inputConfig_.stride;
> +		dst += outputConfig_.stride;
> +
> +		shiftLinePointers(linePointers, src);
> +		/* next line may point outside of src, use prev. */
> +		linePointers[2] = linePointers[0];

At least this deserves a more elaborate comment; honestly, I'm lost what's
happening here exactly.

> +		(this->*debayer1_)(dst, linePointers);
> +		src += inputConfig_.stride;
> +		dst += outputConfig_.stride;
> +	}
> +}
> +
> +void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)
> +{
> +	const unsigned int y_end = window_.y + window_.height;
> +	/*
> +	 * This holds pointers to [0] 2-lines-up [1] 1-line-up [2] current-line
> +	 * [3] 1-line-down [4] 2-lines-down.
> +	 */
> +	const uint8_t *linePointers[5];
> +
> +	/* 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 */
> +	linePointers[1] = src - 2 * inputConfig_.stride;
> +	linePointers[2] = src - inputConfig_.stride;
> +	linePointers[3] = src;
> +	linePointers[4] = src + inputConfig_.stride;
> +
> +	setupInputMemcpy(linePointers);
> +
> +	for (unsigned int y = window_.y; y < y_end; y += 4) {
> +		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;
> +
> +		shiftLinePointers(linePointers, src);
> +		memcpyNextLine(linePointers);
> +		stats_->processLine2(y, linePointers);
> +		(this->*debayer2_)(dst, linePointers);
> +		src += inputConfig_.stride;
> +		dst += outputConfig_.stride;
> +
> +		shiftLinePointers(linePointers, src);
> +		memcpyNextLine(linePointers);
> +		(this->*debayer3_)(dst, linePointers);
> +		src += inputConfig_.stride;
> +		dst += outputConfig_.stride;
> +	}
> +}
> +

[...]

> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> new file mode 100644
> index 00000000..98782838
> --- /dev/null
> +++ b/src/libcamera/software_isp/debayer_cpu.h

[...]

> +	int configure(const StreamConfiguration &inputCfg,
> +		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs);
> +	Size patternSize(PixelFormat inputFormat);
> +	std::vector<PixelFormat> formats(PixelFormat input);
> +	std::tuple<unsigned int, unsigned int>
> +	strideAndFrameSize(const PixelFormat &outputFormat, const Size &size);
> +	void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params);
> +	SizeRange sizes(PixelFormat inputFormat, const Size &inputSize);
> +
> +	/**
> +	 * \brief Get the file descriptor for the statistics.
> +	 *
> +	 * \return the file descriptor pointing to the statistics.
> +	 */
> +	const SharedFD &getStatsFD() { return stats_->getStatsFD(); }
> +
> +	/**
> +	 * \brief Get the output frame size.
> +	 *
> +	 * \return The output frame size.
> +	 */
> +	unsigned int frameSize() { return outputConfig_.frameSize; }
> +
> +private:
> +	/**
> +	 * \brief Called to debayer 1 line of Bayer input data to output format
> +	 * \param[out] dst Pointer to the start of the output line to write
> +	 * \param[in] src The input data
> +	 *
> +	 * Input data is an array of (patternSize_.height + 1) src
> +	 * pointers each pointing to a line in the Bayer source. The middle
> +	 * element of the array will point to the actual line being processed.
> +	 * Earlier element(s) will point to the previous line(s) and later
> +	 * element(s) to the next line(s).
> +	 *
> +	 * These functions take an array of src pointers, rather then

then -> than

[...]