[libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add SwIspLinaro implementation of SoftwareIsp

[Bryan O'Donoghue]

On 04/12/2023 01:10, Andrey Konovalov via libcamera-devel wrote:
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> ---
>   include/libcamera/internal/meson.build        |   1 +
>   .../internal/software_isp/meson.build         |   6 +
>   .../internal/software_isp/statistics-linaro.h |  17 +
>   .../internal/software_isp/swisp_linaro.h      | 109 ++++
>   src/libcamera/meson.build                     |   1 +
>   src/libcamera/software_isp/meson.build        |   5 +
>   src/libcamera/software_isp/swisp_linaro.cpp   | 539 ++++++++++++++++++
>   7 files changed, 678 insertions(+)
>   create mode 100644 include/libcamera/internal/software_isp/meson.build
>   create mode 100644 include/libcamera/internal/software_isp/statistics-linaro.h
>   create mode 100644 include/libcamera/internal/software_isp/swisp_linaro.h
>   create mode 100644 src/libcamera/software_isp/meson.build
>   create mode 100644 src/libcamera/software_isp/swisp_linaro.cpp
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index b780777c..eeae801c 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build

> +void SwIspLinaro::IspWorker::debayerRaw10P(uint8_t *dst, const uint8_t *src)
> +{
> +	/* for brightness values in the 0 to 255 range: */
> +	static const unsigned int BRIGHT_LVL = 200U << 8;
> +	static const unsigned int TOO_BRIGHT_LVL = 240U << 8;
> +
> +	static const unsigned int RED_Y_MUL = 77;	/* 0.30 * 256 */
> +	static const unsigned int GREEN_Y_MUL = 150;	/* 0.59 * 256 */
> +	static const unsigned int BLUE_Y_MUL = 29;	/* 0.11 * 256 */

These seem like magic numbers that deserve a little bit more explanation.

Why is blue 0.11 and not 0.10 for example ?

Same comment with the brightness value - why these numbers ? Does no 
harm to add in some of the thinking behind why you've choosen these 
particular values.

> +
> +int SwIspLinaro::queueBuffers(FrameBuffer *input,
> +			      const std::map<unsigned int, FrameBuffer *> &outputs)
> +{
> +	unsigned int mask = 0;
> +
> +	/*
> +	 * Validate the outputs as a sanity check: at least one output is
> +	 * required, all outputs must reference a valid stream and no two
> +	 * outputs can reference the same stream.
> +	 */
> +	if (outputs.empty())
> +		return -EINVAL;
> +
> +	for (auto [index, buffer] : outputs) {
> +		if (!buffer)
> +			return -EINVAL;
> +		if (index >= 1) /* only single stream atm */
> +			return -EINVAL;

Single stream per sensor ? Or single stream per instance of the class, 
which is ~ the same thing... ?

Anyway maybe make that clearer in the comment.

General comment on the debayer part is more comments please. The logic 
of the algorithm deserves a bit more explanation.

---
bod