[{"id":29085,"web_url":"https://patchwork.libcamera.org/comment/29085/","msgid":"<20240327141225.GF4721@pendragon.ideasonboard.com>","date":"2024-03-27T14:12:25","subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan and Hans,\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:35:55PM +0100, Milan Zamazal wrote:\n> From: Hans de Goede \n> \n> Add CPU based debayering implementation. This initial implementation\n> only supports debayering packed 10 bits per pixel bayer data in\n> the 4 standard bayer orders.\n> \n> Doxygen documentation by Dennis Bonke.\n> \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Pavel Machek \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Co-developed-by: Andrey Konovalov \n> Signed-off-by: Andrey Konovalov \n> Co-developed-by: Pavel Machek \n> Signed-off-by: Pavel Machek \n> Signed-off-by: Hans de Goede \n> Reviewed-by: Milan Zamazal \n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 626 +++++++++++++++++++++\n> src/libcamera/software_isp/debayer_cpu.h | 143 +++++\n> src/libcamera/software_isp/meson.build | 1 +\n> 3 files changed, 770 insertions(+)\n> create mode 100644 src/libcamera/software_isp/debayer_cpu.cpp\n> create mode 100644 src/libcamera/software_isp/debayer_cpu.h\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> new file mode 100644\n> index 00000000..f932362c\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -0,0 +1,626 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Linaro Ltd\n> + * Copyright (C) 2023, Red Hat Inc.\n> + *\n> + * Authors:\n> + * Hans de Goede \n> + *\n> + * debayer_cpu.cpp - CPU based debayering class\n> + */\n> +\n> +#include \"debayer_cpu.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/bayer_format.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class DebayerCpu\n> + * \\brief Class for debayering on the CPU\n> + *\n> + * Implementation for CPU based debayering\n> + */\n> +\n> +/**\n> + * \\brief Constructs a DebayerCpu object.\n> + * \\param[in] stats Pointer to the stats object to use.\n\nNo period at the end of \\brief or \\param.\n\n> + */\n> +DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> +\t: stats_(std::move(stats)), gamma_correction_(1.0)\n\ngammaCorrection_\n\n> +{\n> +#ifdef __x86_64__\n> +\tenableInputMemcpy_ = false;\n> +#else\n> +\tenableInputMemcpy_ = true;\n> +#endif\n\nThis is very much *not* nice :-( It needs to at least be explained\nclearly somewhere.\n\n> +\t/* Initialize gamma to 1.0 curve */\n> +\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> +\t\tgamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n> +\n> +\tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n> +\t\tlineBuffers_[i] = nullptr;\n> +}\n> +\n> +DebayerCpu::~DebayerCpu()\n> +{\n> +\tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n> +\t\tfree(lineBuffers_[i]);\n> +}\n> +\n> +// RGR\n> +// GBG\n> +// RGR\n\nC-style comments.\n\n> +#define BGGR_BGR888(p, n, div) \\\n> +\t*dst++ = blue_[curr[x] / (div)]; \\\n> +\t*dst++ = green_[(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div))]; \\\n> +\t*dst++ = red_[(prev[x - p] + prev[x + n] + next[x - p] + next[x + n]) / (4 * (div))]; \\\n> +\tx++;\n> +\n> +// GBG\n> +// RGR\n> +// GBG\n> +#define GRBG_BGR888(p, n, div) \\\n> +\t*dst++ = blue_[(prev[x] + next[x]) / (2 * (div))]; \\\n> +\t*dst++ = green_[curr[x] / (div)]; \\\n> +\t*dst++ = red_[(curr[x - p] + curr[x + n]) / (2 * (div))]; \\\n> +\tx++;\n> +\n> +// GRG\n> +// BGB\n> +// GRG\n> +#define GBRG_BGR888(p, n, div) \\\n> +\t*dst++ = blue_[(curr[x - p] + curr[x + n]) / (2 * (div))]; \\\n> +\t*dst++ = green_[curr[x] / (div)]; \\\n> +\t*dst++ = red_[(prev[x] + next[x]) / (2 * (div))]; \\\n> +\tx++;\n> +\n> +// BGB\n> +// GRG\n> +// BGB\n> +#define RGGB_BGR888(p, n, div) \\\n> +\t*dst++ = blue_[(prev[x - p] + prev[x + n] + next[x - p] + next[x + n]) / (4 * (div))]; \\\n> +\t*dst++ = green_[(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div))]; \\\n> +\t*dst++ = red_[curr[x] / (div)]; \\\n> +\tx++;\n> +\n> +void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> +{\n> +\tconst int width_in_bytes = window_.width * 5 / 4;\n\nsnakeCase here too, ane where applicable everywhere else.\n\n> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n\nIs the cast needed ? If so it should be a C++ cast.\n\n> +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> +\tconst uint8_t *next = (const uint8_t *)src[2];\n> +\n> +\t/*\n> +\t * For the first pixel getting a pixel from the previous column uses\n> +\t * x - 2 to skip the 5th byte with least-significant bits for 4 pixels.\n> +\t * Same for last pixel (uses x + 2) and looking at the next column.\n> +\t */\n> +\tfor (int x = 0; x < width_in_bytes;) {\n> +\t\t/* First pixel */\n> +\t\tBGGR_BGR888(2, 1, 1)\n> +\t\t/* Second pixel BGGR -> GBRG */\n> +\t\tGBRG_BGR888(1, 1, 1)\n> +\t\t/* Same thing for third and fourth pixels */\n> +\t\tBGGR_BGR888(1, 1, 1)\n> +\t\tGBRG_BGR888(1, 2, 1)\n> +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> +\t\tx++;\n> +\t}\n> +}\n> +\n> +void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> +{\n> +\tconst int width_in_bytes = window_.width * 5 / 4;\n> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> +\tconst uint8_t *next = (const uint8_t *)src[2];\n> +\n> +\tfor (int x = 0; x < width_in_bytes;) {\n> +\t\t/* First pixel */\n> +\t\tGRBG_BGR888(2, 1, 1)\n> +\t\t/* Second pixel GRBG -> RGGB */\n> +\t\tRGGB_BGR888(1, 1, 1)\n> +\t\t/* Same thing for third and fourth pixels */\n> +\t\tGRBG_BGR888(1, 1, 1)\n> +\t\tRGGB_BGR888(1, 2, 1)\n> +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> +\t\tx++;\n> +\t}\n> +}\n> +\n> +void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n> +{\n> +\tconst int width_in_bytes = window_.width * 5 / 4;\n> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> +\tconst uint8_t *next = (const uint8_t *)src[2];\n> +\n> +\tfor (int x = 0; x < width_in_bytes;) {\n> +\t\t/* Even pixel */\n> +\t\tGBRG_BGR888(2, 1, 1)\n> +\t\t/* Odd pixel GBGR -> BGGR */\n> +\t\tBGGR_BGR888(1, 1, 1)\n> +\t\t/* Same thing for next 2 pixels */\n> +\t\tGBRG_BGR888(1, 1, 1)\n> +\t\tBGGR_BGR888(1, 2, 1)\n> +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> +\t\tx++;\n> +\t}\n> +}\n> +\n> +void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n> +{\n> +\tconst int width_in_bytes = window_.width * 5 / 4;\n> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> +\tconst uint8_t *next = (const uint8_t *)src[2];\n> +\n> +\tfor (int x = 0; x < width_in_bytes;) {\n> +\t\t/* Even pixel */\n> +\t\tRGGB_BGR888(2, 1, 1)\n> +\t\t/* Odd pixel RGGB -> GRBG */\n> +\t\tGRBG_BGR888(1, 1, 1)\n> +\t\t/* Same thing for next 2 pixels */\n> +\t\tRGGB_BGR888(1, 1, 1)\n> +\t\tGRBG_BGR888(1, 2, 1)\n> +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> +\t\tx++;\n> +\t}\n> +}\n> +\n> +static bool isStandardBayerOrder(BayerFormat::Order order)\n> +{\n> +\treturn order == BayerFormat::BGGR || order == BayerFormat::GBRG ||\n> +\t order == BayerFormat::GRBG || order == BayerFormat::RGGB;\n> +}\n> +\n> +/*\n> + * Setup the Debayer object according to the passed in parameters.\n> + * Return 0 on success, a negative errno value on failure\n> + * (unsupported parameters).\n> + */\n> +int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config)\n> +{\n> +\tBayerFormat bayerFormat =\n> +\t\tBayerFormat::fromPixelFormat(inputFormat);\n> +\n> +\tif (bayerFormat.bitDepth == 10 &&\n> +\t bayerFormat.packing == BayerFormat::Packing::CSI2 &&\n> +\t isStandardBayerOrder(bayerFormat.order)) {\n> +\t\tconfig.bpp = 10;\n> +\t\tconfig.patternSize.width = 4; /* 5 bytes per *4* pixels */\n> +\t\tconfig.patternSize.height = 2;\n> +\t\tconfig.outputFormats = std::vector({ formats::RGB888 });\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tLOG(Debayer, Info)\n\nShouldn't this be an Error mesage ? Same below.\n\n> +\t\t<< \"Unsupported input format \" << inputFormat.toString();\n> +\treturn -EINVAL;\n> +}\n> +\n> +int DebayerCpu::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)\n> +{\n> +\tif (outputFormat == formats::RGB888) {\n> +\t\tconfig.bpp = 24;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tLOG(Debayer, Info)\n> +\t\t<< \"Unsupported output format \" << outputFormat.toString();\n> +\treturn -EINVAL;\n> +}\n> +\n> +/* TODO: this ignores outputFormat since there is only 1 supported outputFormat for now */\n> +int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, [[maybe_unused]] PixelFormat outputFormat)\n> +{\n> +\tBayerFormat bayerFormat =\n> +\t\tBayerFormat::fromPixelFormat(inputFormat);\n> +\n> +\tif (bayerFormat.bitDepth == 10 &&\n> +\t bayerFormat.packing == BayerFormat::Packing::CSI2) {\n> +\t\tswitch (bayerFormat.order) {\n> +\t\tcase BayerFormat::BGGR:\n> +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_BGBG_BGR888;\n> +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_GRGR_BGR888;\n> +\t\t\treturn 0;\n> +\t\tcase BayerFormat::GBRG:\n> +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_GBGB_BGR888;\n> +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_RGRG_BGR888;\n> +\t\t\treturn 0;\n> +\t\tcase BayerFormat::GRBG:\n> +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_GRGR_BGR888;\n> +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_BGBG_BGR888;\n> +\t\t\treturn 0;\n> +\t\tcase BayerFormat::RGGB:\n> +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_RGRG_BGR888;\n> +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_GBGB_BGR888;\n> +\t\t\treturn 0;\n> +\t\tdefault:\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tLOG(Debayer, Error) << \"Unsupported input output format combination\";\n> +\treturn -EINVAL;\n> +}\n> +\n> +int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> +\t\t\t const std::vector> &outputCfgs)\n> +{\n> +\tif (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tif (stats_->configure(inputCfg) != 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tconst Size &stats_pattern_size = stats_->patternSize();\n> +\tif (inputConfig_.patternSize.width != stats_pattern_size.width ||\n> +\t inputConfig_.patternSize.height != stats_pattern_size.height) {\n> +\t\tLOG(Debayer, Error)\n> +\t\t\t<< \"mismatching stats and debayer pattern sizes for \"\n> +\t\t\t<< inputCfg.pixelFormat.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tinputConfig_.stride = inputCfg.stride;\n> +\n> +\tif (outputCfgs.size() != 1) {\n> +\t\tLOG(Debayer, Error)\n> +\t\t\t<< \"Unsupported number of output streams: \"\n> +\t\t\t<< outputCfgs.size();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst StreamConfiguration &outputCfg = outputCfgs[0];\n> +\tSizeRange outSizeRange = sizes(inputCfg.pixelFormat, inputCfg.size);\n> +\tstd::tie(outputConfig_.stride, outputConfig_.frameSize) =\n> +\t\tstrideAndFrameSize(outputCfg.pixelFormat, outputCfg.size);\n> +\n> +\tif (!outSizeRange.contains(outputCfg.size) || outputConfig_.stride != outputCfg.stride) {\n> +\t\tLOG(Debayer, Error)\n> +\t\t\t<< \"Invalid output size/stride: \"\n> +\t\t\t<< \"\\n \" << outputCfg.size << \" (\" << outSizeRange << \")\"\n> +\t\t\t<< \"\\n \" << outputCfg.stride << \" (\" << outputConfig_.stride << \")\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)\n> +\t\treturn -EINVAL;\n> +\n> +\twindow_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &\n> +\t\t ~(inputConfig_.patternSize.width - 1);\n> +\twindow_.y = ((inputCfg.size.height - outputCfg.size.height) / 2) &\n> +\t\t ~(inputConfig_.patternSize.height - 1);\n> +\twindow_.width = outputCfg.size.width;\n> +\twindow_.height = outputCfg.size.height;\n> +\n> +\t/* Don't pass x,y since process() already adjusts src before passing it */\n> +\tstats_->setWindow(Rectangle(window_.size()));\n> +\n> +\t/* pad with patternSize.Width on both left and right side */\n> +\tlineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;\n> +\tlineBufferLength_ = window_.width * inputConfig_.bpp / 8 +\n> +\t\t\t 2 * lineBufferPadding_;\n> +\tfor (unsigned int i = 0;\n> +\t i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;\n> +\t i++) {\n> +\t\tfree(lineBuffers_[i]);\n> +\t\tlineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);\n> +\t\tif (!lineBuffers_[i])\n> +\t\t\treturn -ENOMEM;\n> +\t}\n> +\n> +\tmeasuredFrames_ = 0;\n> +\tframeProcessTime_ = 0;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * Get width and height at which the bayer-pattern repeats.\n> + * Return pattern-size or an empty Size for an unsupported inputFormat.\n> + */\n> +Size DebayerCpu::patternSize(PixelFormat inputFormat)\n> +{\n> +\tDebayerCpu::DebayerInputConfig config;\n> +\n> +\tif (getInputConfig(inputFormat, config) != 0)\n> +\t\treturn {};\n> +\n> +\treturn config.patternSize;\n> +}\n> +\n> +std::vector DebayerCpu::formats(PixelFormat inputFormat)\n> +{\n> +\tDebayerCpu::DebayerInputConfig config;\n> +\n> +\tif (getInputConfig(inputFormat, config) != 0)\n> +\t\treturn std::vector();\n> +\n> +\treturn config.outputFormats;\n> +}\n> +\n> +std::tuple\n> +DebayerCpu::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)\n> +{\n> +\tDebayerCpu::DebayerOutputConfig config;\n> +\n> +\tif (getOutputConfig(outputFormat, config) != 0)\n> +\t\treturn std::make_tuple(0, 0);\n> +\n> +\t/* round up to multiple of 8 for 64 bits alignment */\n> +\tunsigned int stride = (size.width * config.bpp / 8 + 7) & ~7;\n> +\n> +\treturn std::make_tuple(stride, stride * size.height);\n> +}\n> +\n> +void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])\n> +{\n> +\tconst unsigned int patternHeight = inputConfig_.patternSize.height;\n> +\n> +\tif (!enableInputMemcpy_)\n> +\t\treturn;\n> +\n> +\tfor (unsigned int i = 0; i < patternHeight; i++) {\n> +\t\tmemcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,\n> +\t\t lineBufferLength_);\n> +\t\tlinePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;\n> +\t}\n> +\n> +\t/* Point lineBufferIndex_ to first unused lineBuffer */\n> +\tlineBufferIndex_ = patternHeight;\n> +}\n> +\n> +void DebayerCpu::shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src)\n> +{\n> +\tconst unsigned int patternHeight = inputConfig_.patternSize.height;\n> +\n> +\tfor (unsigned int i = 0; i < patternHeight; i++)\n> +\t\tlinePointers[i] = linePointers[i + 1];\n> +\n> +\tlinePointers[patternHeight] = src +\n> +\t\t\t\t (patternHeight / 2) * (int)inputConfig_.stride;\n> +}\n> +\n> +void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])\n> +{\n> +\tconst unsigned int patternHeight = inputConfig_.patternSize.height;\n> +\n> +\tif (!enableInputMemcpy_)\n> +\t\treturn;\n> +\n> +\tmemcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,\n> +\t lineBufferLength_);\n> +\tlinePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;\n> +\n> +\tlineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);\n> +}\n> +\n> +void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> +{\n> +\tunsigned int y_end = window_.y + window_.height;\n> +\t/* Holds [0] previous- [1] current- [2] next-line */\n> +\tconst uint8_t *linePointers[3];\n> +\n> +\t/* Adjust src to top left corner of the window */\n> +\tsrc += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;\n> +\n> +\t/* [x] becomes [x - 1] after initial shiftLinePointers() call */\n> +\tif (window_.y) {\n> +\t\tlinePointers[1] = src - inputConfig_.stride; /* previous-line */\n> +\t\tlinePointers[2] = src;\n> +\t} else {\n> +\t\t/* window_.y == 0, use the next line as prev line */\n> +\t\tlinePointers[1] = src + inputConfig_.stride;\n> +\t\tlinePointers[2] = src;\n> +\t\t/* Last 2 lines also need special handling */\n> +\t\ty_end -= 2;\n> +\t}\n> +\n> +\tsetupInputMemcpy(linePointers);\n> +\n> +\tfor (unsigned int y = window_.y; y < y_end; y += 2) {\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine0(y, linePointers);\n> +\t\t(this->*debayer0_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\t(this->*debayer1_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\t}\n> +\n> +\tif (window_.y == 0) {\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine0(y_end, linePointers);\n> +\t\t(this->*debayer0_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\t/* next line may point outside of src, use prev. */\n> +\t\tlinePointers[2] = linePointers[0];\n> +\t\t(this->*debayer1_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\t}\n> +}\n> +\n> +void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> +{\n> +\tconst unsigned int y_end = window_.y + window_.height;\n> +\t/*\n> +\t * This holds pointers to [0] 2-lines-up [1] 1-line-up [2] current-line\n> +\t * [3] 1-line-down [4] 2-lines-down.\n> +\t */\n> +\tconst uint8_t *linePointers[5];\n> +\n> +\t/* Adjust src to top left corner of the window */\n> +\tsrc += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;\n> +\n> +\t/* [x] becomes [x - 1] after initial shiftLinePointers() call */\n> +\tlinePointers[1] = src - 2 * inputConfig_.stride;\n> +\tlinePointers[2] = src - inputConfig_.stride;\n> +\tlinePointers[3] = src;\n> +\tlinePointers[4] = src + inputConfig_.stride;\n> +\n> +\tsetupInputMemcpy(linePointers);\n> +\n> +\tfor (unsigned int y = window_.y; y < y_end; y += 4) {\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine0(y, linePointers);\n> +\t\t(this->*debayer0_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\t(this->*debayer1_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine2(y, linePointers);\n> +\t\t(this->*debayer2_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\t(this->*debayer3_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\t}\n> +}\n> +\n> +static inline int64_t timeDiff(timespec &after, timespec &before)\n> +{\n> +\treturn (after.tv_sec - before.tv_sec) * 1000000000LL +\n> +\t (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> +}\n> +\n> +void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> +{\n> +\ttimespec frameStartTime;\n> +\n> +\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> +\t\tframeStartTime = {};\n> +\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> +\t}\n> +\n> +\t/* Apply DebayerParams */\n> +\tif (params.gamma != gamma_correction_) {\n> +\t\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> +\t\t\tgamma_[i] = UINT8_MAX * powf(i / (kGammaLookupSize - 1.0), params.gamma);\n> +\n> +\t\tgamma_correction_ = params.gamma;\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> +\t\tconstexpr unsigned int div =\n> +\t\t\tkRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n> +\t\tunsigned int idx;\n> +\n> +\t\t/* Apply gamma after gain! */\n> +\t\tidx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n> +\t\tred_[i] = gamma_[idx];\n> +\n> +\t\tidx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n> +\t\tgreen_[i] = gamma_[idx];\n> +\n> +\t\tidx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n> +\t\tblue_[i] = gamma_[idx];\n> +\t}\n> +\n> +\t/* Copy metadata from the input buffer */\n> +\tFrameMetadata &metadata = output->_d()->metadata();\n> +\tmetadata.status = input->metadata().status;\n> +\tmetadata.sequence = input->metadata().sequence;\n> +\tmetadata.timestamp = input->metadata().timestamp;\n> +\n> +\tMappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> +\tMappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n> +\tif (!in.isValid() || !out.isValid()) {\n> +\t\tLOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n> +\t\tmetadata.status = FrameMetadata::FrameError;\n> +\t\treturn;\n> +\t}\n> +\n> +\tstats_->startFrame();\n> +\n> +\tif (inputConfig_.patternSize.height == 2)\n> +\t\tprocess2(in.planes()[0].data(), out.planes()[0].data());\n> +\telse\n> +\t\tprocess4(in.planes()[0].data(), out.planes()[0].data());\n> +\n> +\tmetadata.planes()[0].bytesused = out.planes()[0].size();\n> +\n> +\t/* Measure before emitting signals */\n> +\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> +\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> +\t\ttimespec frameEndTime = {};\n> +\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> +\t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> +\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> +\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> +\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n> +\t\t\tLOG(Debayer, Info)\n> +\t\t\t\t<< \"Processed \" << measuredFrames\n> +\t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> +\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> +\t\t\t\t<< \" us/frame\";\n> +\t\t}\n> +\t}\n\nIs this debugging leftovers, or should it be cleaned up ? It seems quite\na bit of a hack. If we want to enable measurements, we shouldn't\nhardcode them to frames 30 to 60.\n\n> +\n> +\tstats_->finishFrame();\n> +\toutputBufferReady.emit(output);\n> +\tinputBufferReady.emit(input);\n> +}\n> +\n> +SizeRange DebayerCpu::sizes(PixelFormat inputFormat, const Size &inputSize)\n> +{\n> +\tSize pattern_size = patternSize(inputFormat);\n> +\tunsigned int border_height = pattern_size.height;\n> +\n> +\tif (pattern_size.isNull())\n> +\t\treturn {};\n> +\n> +\t/* No need for top/bottom border with a pattern height of 2 */\n> +\tif (pattern_size.height == 2)\n> +\t\tborder_height = 0;\n> +\n> +\t/*\n> +\t * For debayer interpolation a border is kept around the entire image\n> +\t * and the minimum output size is pattern-height x pattern-width.\n> +\t */\n> +\tif (inputSize.width < (3 * pattern_size.width) ||\n> +\t inputSize.height < (2 * border_height + pattern_size.height)) {\n> +\t\tLOG(Debayer, Warning)\n> +\t\t\t<< \"Input format size too small: \" << inputSize.toString();\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn SizeRange(Size(pattern_size.width, pattern_size.height),\n> +\t\t\t Size((inputSize.width - 2 * pattern_size.width) & ~(pattern_size.width - 1),\n> +\t\t\t (inputSize.height - 2 * border_height) & ~(pattern_size.height - 1)),\n> +\t\t\t pattern_size.width, pattern_size.height);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> new file mode 100644\n> index 00000000..8a51ed85\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -0,0 +1,143 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Linaro Ltd\n> + * Copyright (C) 2023, Red Hat Inc.\n> + *\n> + * Authors:\n> + * Hans de Goede \n> + *\n> + * debayer_cpu.h - CPU based debayering header\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"debayer.h\"\n> +#include \"swstats_cpu.h\"\n> +\n> +namespace libcamera {\n> +\n> +class DebayerCpu : public Debayer, public Object\n> +{\n> +public:\n> +\tDebayerCpu(std::unique_ptr stats);\n> +\t~DebayerCpu();\n> +\n> +\tint configure(const StreamConfiguration &inputCfg,\n> +\t\t const std::vector> &outputCfgs);\n> +\tSize patternSize(PixelFormat inputFormat);\n> +\tstd::vector formats(PixelFormat input);\n> +\tstd::tuple\n> +\tstrideAndFrameSize(const PixelFormat &outputFormat, const Size &size);\n> +\tvoid process(FrameBuffer *input, FrameBuffer *output, DebayerParams params);\n> +\tSizeRange sizes(PixelFormat inputFormat, const Size &inputSize);\n> +\n> +\t/**\n> +\t * \\brief Get the file descriptor for the statistics.\n> +\t *\n> +\t * \\return the file descriptor pointing to the statistics.\n> +\t */\n> +\tconst SharedFD &getStatsFD() { return stats_->getStatsFD(); }\n\nThis, plus the fact that this class handles colour gains and gamma,\nmakes me thing we have either a naming issue, or an architecture issue.\n\n> +\n> +\t/**\n> +\t * \\brief Get the output frame size.\n> +\t *\n> +\t * \\return The output frame size.\n> +\t */\n> +\tunsigned int frameSize() { return outputConfig_.frameSize; }\n> +\n> +private:\n> +\t/**\n> +\t * \\brief Called to debayer 1 line of Bayer input data to output format\n> +\t * \\param[out] dst Pointer to the start of the output line to write\n> +\t * \\param[in] src The input data\n> +\t *\n> +\t * Input data is an array of (patternSize_.height + 1) src\n> +\t * pointers each pointing to a line in the Bayer source. The middle\n> +\t * element of the array will point to the actual line being processed.\n> +\t * Earlier element(s) will point to the previous line(s) and later\n> +\t * element(s) to the next line(s).\n> +\t *\n> +\t * These functions take an array of src pointers, rather than\n> +\t * a single src pointer + a stride for the source, so that when the src\n> +\t * is slow uncached memory it can be copied to faster memory before\n> +\t * debayering. Debayering a standard 2x2 Bayer pattern requires access\n> +\t * to the previous and next src lines for interpolating the missing\n> +\t * colors. To allow copying the src lines only once 3 temporary buffers\n> +\t * each holding a single line are used, re-using the oldest buffer for\n> +\t * the next line and the pointers are swizzled so that:\n> +\t * src[0] = previous-line, src[1] = currrent-line, src[2] = next-line.\n> +\t * This way the 3 pointers passed to the debayer functions form\n> +\t * a sliding window over the src avoiding the need to copy each\n> +\t * line more than once.\n> +\t *\n> +\t * Similarly for bayer patterns which repeat every 4 lines, 5 src\n> +\t * pointers are passed holding: src[0] = 2-lines-up, src[1] = 1-line-up\n> +\t * src[2] = current-line, src[3] = 1-line-down, src[4] = 2-lines-down.\n> +\t */\n> +\tusing debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);\n> +\n> +\t/* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */\n> +\tvoid debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> +\tvoid debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> +\tvoid debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);\n> +\tvoid debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> +\n> +\tstruct DebayerInputConfig {\n> +\t\tSize patternSize;\n> +\t\tunsigned int bpp; /* Memory used per pixel, not precision */\n> +\t\tunsigned int stride;\n> +\t\tstd::vector outputFormats;\n> +\t};\n> +\n> +\tstruct DebayerOutputConfig {\n> +\t\tunsigned int bpp; /* Memory used per pixel, not precision */\n> +\t\tunsigned int stride;\n> +\t\tunsigned int frameSize;\n> +\t};\n> +\n> +\tint getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);\n> +\tint getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);\n> +\tint setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);\n> +\tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n> +\tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n> +\tvoid memcpyNextLine(const uint8_t *linePointers[]);\n> +\tvoid process2(const uint8_t *src, uint8_t *dst);\n> +\tvoid process4(const uint8_t *src, uint8_t *dst);\n> +\n> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> +\tstatic constexpr unsigned int kRGBLookupSize = 256;\n> +\t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> +\tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> +\n> +\tstd::array gamma_;\n> +\tstd::array red_;\n> +\tstd::array green_;\n> +\tstd::array blue_;\n> +\tdebayerFn debayer0_;\n> +\tdebayerFn debayer1_;\n> +\tdebayerFn debayer2_;\n> +\tdebayerFn debayer3_;\n> +\tRectangle window_;\n> +\tDebayerInputConfig inputConfig_;\n> +\tDebayerOutputConfig outputConfig_;\n> +\tstd::unique_ptr stats_;\n> +\tuint8_t *lineBuffers_[kMaxLineBuffers];\n> +\tunsigned int lineBufferLength_;\n> +\tunsigned int lineBufferPadding_;\n> +\tunsigned int lineBufferIndex_;\n> +\tbool enableInputMemcpy_;\n> +\tfloat gamma_correction_;\n> +\tunsigned int measuredFrames_;\n> +\tint64_t frameProcessTime_;\n> +\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n> +\tstatic constexpr unsigned int kFramesToSkip = 30;\n> +\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build\n> index 62095f61..71b46539 100644\n> --- a/src/libcamera/software_isp/meson.build\n> +++ b/src/libcamera/software_isp/meson.build\n> @@ -9,5 +9,6 @@ endif\n> \n> libcamera_sources += files([\n> 'debayer.cpp',\n> + 'debayer_cpu.cpp',\n> 'swstats_cpu.cpp',\n> ])","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9FE0CC0DA4\n\tfor ;\n\tWed, 27 Mar 2024 14:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF3BA63331;\n\tWed, 27 Mar 2024 15:12:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E56A361C35\n\tfor ;\n\tWed, 27 Mar 2024 15:12:35 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B95BD1890;\n\tWed, 27 Mar 2024 15:12:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dz75Z+P3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711548723;\n\tbh=4UlTJWdbfVw9qS40AEA5WSWtsxs08FYci06J/0gqW/0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dz75Z+P3fbQGu3p13wbpvGxZMIj4z6vkrSK6O6dCAmdm6luc8BYbL77YY9ZdUb4iR\n\tk2621dKmxElwT8PZaNvHuzl/lAQdaV1LJM6mnSuiEWQ+Axp+KqKjvg/829w6wZN0YG\n\tWTxG2S6MP5GDMcCmigsdUCT4YDF4S9OliwwpiDdk=","Date":"Wed, 27 Mar 2024 16:12:25 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Hans de Goede ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ,\n\tDennis Bonke ,\n\tAndrey Konovalov ","Subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","Message-ID":"<20240327141225.GF4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-9-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-9-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29103,"web_url":"https://patchwork.libcamera.org/comment/29103/","msgid":"<20240327180132.GK4721@pendragon.ideasonboard.com>","date":"2024-03-27T18:01:32","subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Another comment.\n\nOn Wed, Mar 27, 2024 at 04:12:25PM +0200, Laurent Pinchart wrote:\n> Hi Milan and Hans,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 19, 2024 at 01:35:55PM +0100, Milan Zamazal wrote:\n> > From: Hans de Goede \n> > \n> > Add CPU based debayering implementation. This initial implementation\n> > only supports debayering packed 10 bits per pixel bayer data in\n> > the 4 standard bayer orders.\n> > \n> > Doxygen documentation by Dennis Bonke.\n> > \n> > Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> > Tested-by: Pavel Machek \n> > Reviewed-by: Pavel Machek \n> > Co-developed-by: Dennis Bonke \n> > Signed-off-by: Dennis Bonke \n> > Co-developed-by: Andrey Konovalov \n> > Signed-off-by: Andrey Konovalov \n> > Co-developed-by: Pavel Machek \n> > Signed-off-by: Pavel Machek \n> > Signed-off-by: Hans de Goede \n> > Reviewed-by: Milan Zamazal \n> > ---\n> > src/libcamera/software_isp/debayer_cpu.cpp | 626 +++++++++++++++++++++\n> > src/libcamera/software_isp/debayer_cpu.h | 143 +++++\n> > src/libcamera/software_isp/meson.build | 1 +\n> > 3 files changed, 770 insertions(+)\n> > create mode 100644 src/libcamera/software_isp/debayer_cpu.cpp\n> > create mode 100644 src/libcamera/software_isp/debayer_cpu.h\n> > \n> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> > new file mode 100644\n> > index 00000000..f932362c\n> > --- /dev/null\n> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> > @@ -0,0 +1,626 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Linaro Ltd\n> > + * Copyright (C) 2023, Red Hat Inc.\n> > + *\n> > + * Authors:\n> > + * Hans de Goede \n> > + *\n> > + * debayer_cpu.cpp - CPU based debayering class\n> > + */\n> > +\n> > +#include \"debayer_cpu.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/bayer_format.h\"\n> > +#include \"libcamera/internal/framebuffer.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class DebayerCpu\n> > + * \\brief Class for debayering on the CPU\n> > + *\n> > + * Implementation for CPU based debayering\n> > + */\n> > +\n> > +/**\n> > + * \\brief Constructs a DebayerCpu object.\n> > + * \\param[in] stats Pointer to the stats object to use.\n> \n> No period at the end of \\brief or \\param.\n> \n> > + */\n> > +DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> > +\t: stats_(std::move(stats)), gamma_correction_(1.0)\n> \n> gammaCorrection_\n> \n> > +{\n> > +#ifdef __x86_64__\n> > +\tenableInputMemcpy_ = false;\n> > +#else\n> > +\tenableInputMemcpy_ = true;\n> > +#endif\n> \n> This is very much *not* nice :-( It needs to at least be explained\n> clearly somewhere.\n> \n> > +\t/* Initialize gamma to 1.0 curve */\n> > +\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> > +\t\tgamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n> > +\n> > +\tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n> > +\t\tlineBuffers_[i] = nullptr;\n> > +}\n> > +\n> > +DebayerCpu::~DebayerCpu()\n> > +{\n> > +\tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n> > +\t\tfree(lineBuffers_[i]);\n> > +}\n> > +\n> > +// RGR\n> > +// GBG\n> > +// RGR\n> \n> C-style comments.\n> \n> > +#define BGGR_BGR888(p, n, div) \\\n> > +\t*dst++ = blue_[curr[x] / (div)]; \\\n> > +\t*dst++ = green_[(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div))]; \\\n> > +\t*dst++ = red_[(prev[x - p] + prev[x + n] + next[x - p] + next[x + n]) / (4 * (div))]; \\\n> > +\tx++;\n> > +\n> > +// GBG\n> > +// RGR\n> > +// GBG\n> > +#define GRBG_BGR888(p, n, div) \\\n> > +\t*dst++ = blue_[(prev[x] + next[x]) / (2 * (div))]; \\\n> > +\t*dst++ = green_[curr[x] / (div)]; \\\n> > +\t*dst++ = red_[(curr[x - p] + curr[x + n]) / (2 * (div))]; \\\n> > +\tx++;\n> > +\n> > +// GRG\n> > +// BGB\n> > +// GRG\n> > +#define GBRG_BGR888(p, n, div) \\\n> > +\t*dst++ = blue_[(curr[x - p] + curr[x + n]) / (2 * (div))]; \\\n> > +\t*dst++ = green_[curr[x] / (div)]; \\\n> > +\t*dst++ = red_[(prev[x] + next[x]) / (2 * (div))]; \\\n> > +\tx++;\n> > +\n> > +// BGB\n> > +// GRG\n> > +// BGB\n> > +#define RGGB_BGR888(p, n, div) \\\n> > +\t*dst++ = blue_[(prev[x - p] + prev[x + n] + next[x - p] + next[x + n]) / (4 * (div))]; \\\n> > +\t*dst++ = green_[(prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div))]; \\\n> > +\t*dst++ = red_[curr[x] / (div)]; \\\n> > +\tx++;\n> > +\n> > +void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> > +{\n> > +\tconst int width_in_bytes = window_.width * 5 / 4;\n> \n> snakeCase here too, ane where applicable everywhere else.\n> \n> > +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> \n> Is the cast needed ? If so it should be a C++ cast.\n> \n> > +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> > +\tconst uint8_t *next = (const uint8_t *)src[2];\n> > +\n> > +\t/*\n> > +\t * For the first pixel getting a pixel from the previous column uses\n> > +\t * x - 2 to skip the 5th byte with least-significant bits for 4 pixels.\n> > +\t * Same for last pixel (uses x + 2) and looking at the next column.\n> > +\t */\n> > +\tfor (int x = 0; x < width_in_bytes;) {\n> > +\t\t/* First pixel */\n> > +\t\tBGGR_BGR888(2, 1, 1)\n> > +\t\t/* Second pixel BGGR -> GBRG */\n> > +\t\tGBRG_BGR888(1, 1, 1)\n> > +\t\t/* Same thing for third and fourth pixels */\n> > +\t\tBGGR_BGR888(1, 1, 1)\n> > +\t\tGBRG_BGR888(1, 2, 1)\n> > +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> > +\t\tx++;\n> > +\t}\n> > +}\n> > +\n> > +void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])\n> > +{\n> > +\tconst int width_in_bytes = window_.width * 5 / 4;\n> > +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> > +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> > +\tconst uint8_t *next = (const uint8_t *)src[2];\n> > +\n> > +\tfor (int x = 0; x < width_in_bytes;) {\n> > +\t\t/* First pixel */\n> > +\t\tGRBG_BGR888(2, 1, 1)\n> > +\t\t/* Second pixel GRBG -> RGGB */\n> > +\t\tRGGB_BGR888(1, 1, 1)\n> > +\t\t/* Same thing for third and fourth pixels */\n> > +\t\tGRBG_BGR888(1, 1, 1)\n> > +\t\tRGGB_BGR888(1, 2, 1)\n> > +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> > +\t\tx++;\n> > +\t}\n> > +}\n> > +\n> > +void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])\n> > +{\n> > +\tconst int width_in_bytes = window_.width * 5 / 4;\n> > +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> > +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> > +\tconst uint8_t *next = (const uint8_t *)src[2];\n> > +\n> > +\tfor (int x = 0; x < width_in_bytes;) {\n> > +\t\t/* Even pixel */\n> > +\t\tGBRG_BGR888(2, 1, 1)\n> > +\t\t/* Odd pixel GBGR -> BGGR */\n> > +\t\tBGGR_BGR888(1, 1, 1)\n> > +\t\t/* Same thing for next 2 pixels */\n> > +\t\tGBRG_BGR888(1, 1, 1)\n> > +\t\tBGGR_BGR888(1, 2, 1)\n> > +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> > +\t\tx++;\n> > +\t}\n> > +}\n> > +\n> > +void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n> > +{\n> > +\tconst int width_in_bytes = window_.width * 5 / 4;\n> > +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> > +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> > +\tconst uint8_t *next = (const uint8_t *)src[2];\n> > +\n> > +\tfor (int x = 0; x < width_in_bytes;) {\n> > +\t\t/* Even pixel */\n> > +\t\tRGGB_BGR888(2, 1, 1)\n> > +\t\t/* Odd pixel RGGB -> GRBG */\n> > +\t\tGRBG_BGR888(1, 1, 1)\n> > +\t\t/* Same thing for next 2 pixels */\n> > +\t\tRGGB_BGR888(1, 1, 1)\n> > +\t\tGRBG_BGR888(1, 2, 1)\n> > +\t\t/* Skip 5th src byte with 4 x 2 least-significant-bits */\n> > +\t\tx++;\n> > +\t}\n> > +}\n> > +\n> > +static bool isStandardBayerOrder(BayerFormat::Order order)\n> > +{\n> > +\treturn order == BayerFormat::BGGR || order == BayerFormat::GBRG ||\n> > +\t order == BayerFormat::GRBG || order == BayerFormat::RGGB;\n> > +}\n> > +\n> > +/*\n> > + * Setup the Debayer object according to the passed in parameters.\n> > + * Return 0 on success, a negative errno value on failure\n> > + * (unsupported parameters).\n> > + */\n> > +int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config)\n> > +{\n> > +\tBayerFormat bayerFormat =\n> > +\t\tBayerFormat::fromPixelFormat(inputFormat);\n> > +\n> > +\tif (bayerFormat.bitDepth == 10 &&\n> > +\t bayerFormat.packing == BayerFormat::Packing::CSI2 &&\n> > +\t isStandardBayerOrder(bayerFormat.order)) {\n> > +\t\tconfig.bpp = 10;\n> > +\t\tconfig.patternSize.width = 4; /* 5 bytes per *4* pixels */\n> > +\t\tconfig.patternSize.height = 2;\n> > +\t\tconfig.outputFormats = std::vector({ formats::RGB888 });\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tLOG(Debayer, Info)\n> \n> Shouldn't this be an Error mesage ? Same below.\n> \n> > +\t\t<< \"Unsupported input format \" << inputFormat.toString();\n> > +\treturn -EINVAL;\n> > +}\n> > +\n> > +int DebayerCpu::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)\n> > +{\n> > +\tif (outputFormat == formats::RGB888) {\n> > +\t\tconfig.bpp = 24;\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tLOG(Debayer, Info)\n> > +\t\t<< \"Unsupported output format \" << outputFormat.toString();\n> > +\treturn -EINVAL;\n> > +}\n> > +\n> > +/* TODO: this ignores outputFormat since there is only 1 supported outputFormat for now */\n\n/* \\todo This ignores outputFormat since there is only 1 supported outputFormat for now */\n\n> > +int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, [[maybe_unused]] PixelFormat outputFormat)\n> > +{\n> > +\tBayerFormat bayerFormat =\n> > +\t\tBayerFormat::fromPixelFormat(inputFormat);\n> > +\n> > +\tif (bayerFormat.bitDepth == 10 &&\n> > +\t bayerFormat.packing == BayerFormat::Packing::CSI2) {\n> > +\t\tswitch (bayerFormat.order) {\n> > +\t\tcase BayerFormat::BGGR:\n> > +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_BGBG_BGR888;\n> > +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_GRGR_BGR888;\n> > +\t\t\treturn 0;\n> > +\t\tcase BayerFormat::GBRG:\n> > +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_GBGB_BGR888;\n> > +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_RGRG_BGR888;\n> > +\t\t\treturn 0;\n> > +\t\tcase BayerFormat::GRBG:\n> > +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_GRGR_BGR888;\n> > +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_BGBG_BGR888;\n> > +\t\t\treturn 0;\n> > +\t\tcase BayerFormat::RGGB:\n> > +\t\t\tdebayer0_ = &DebayerCpu::debayer10P_RGRG_BGR888;\n> > +\t\t\tdebayer1_ = &DebayerCpu::debayer10P_GBGB_BGR888;\n> > +\t\t\treturn 0;\n> > +\t\tdefault:\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tLOG(Debayer, Error) << \"Unsupported input output format combination\";\n> > +\treturn -EINVAL;\n> > +}\n> > +\n> > +int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> > +\t\t\t const std::vector> &outputCfgs)\n> > +{\n> > +\tif (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tif (stats_->configure(inputCfg) != 0)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tconst Size &stats_pattern_size = stats_->patternSize();\n> > +\tif (inputConfig_.patternSize.width != stats_pattern_size.width ||\n> > +\t inputConfig_.patternSize.height != stats_pattern_size.height) {\n> > +\t\tLOG(Debayer, Error)\n> > +\t\t\t<< \"mismatching stats and debayer pattern sizes for \"\n> > +\t\t\t<< inputCfg.pixelFormat.toString();\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tinputConfig_.stride = inputCfg.stride;\n> > +\n> > +\tif (outputCfgs.size() != 1) {\n> > +\t\tLOG(Debayer, Error)\n> > +\t\t\t<< \"Unsupported number of output streams: \"\n> > +\t\t\t<< outputCfgs.size();\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tconst StreamConfiguration &outputCfg = outputCfgs[0];\n> > +\tSizeRange outSizeRange = sizes(inputCfg.pixelFormat, inputCfg.size);\n> > +\tstd::tie(outputConfig_.stride, outputConfig_.frameSize) =\n> > +\t\tstrideAndFrameSize(outputCfg.pixelFormat, outputCfg.size);\n> > +\n> > +\tif (!outSizeRange.contains(outputCfg.size) || outputConfig_.stride != outputCfg.stride) {\n> > +\t\tLOG(Debayer, Error)\n> > +\t\t\t<< \"Invalid output size/stride: \"\n> > +\t\t\t<< \"\\n \" << outputCfg.size << \" (\" << outSizeRange << \")\"\n> > +\t\t\t<< \"\\n \" << outputCfg.stride << \" (\" << outputConfig_.stride << \")\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tif (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\twindow_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &\n> > +\t\t ~(inputConfig_.patternSize.width - 1);\n> > +\twindow_.y = ((inputCfg.size.height - outputCfg.size.height) / 2) &\n> > +\t\t ~(inputConfig_.patternSize.height - 1);\n> > +\twindow_.width = outputCfg.size.width;\n> > +\twindow_.height = outputCfg.size.height;\n> > +\n> > +\t/* Don't pass x,y since process() already adjusts src before passing it */\n> > +\tstats_->setWindow(Rectangle(window_.size()));\n> > +\n> > +\t/* pad with patternSize.Width on both left and right side */\n> > +\tlineBufferPadding_ = inputConfig_.patternSize.width * inputConfig_.bpp / 8;\n> > +\tlineBufferLength_ = window_.width * inputConfig_.bpp / 8 +\n> > +\t\t\t 2 * lineBufferPadding_;\n> > +\tfor (unsigned int i = 0;\n> > +\t i < (inputConfig_.patternSize.height + 1) && enableInputMemcpy_;\n> > +\t i++) {\n> > +\t\tfree(lineBuffers_[i]);\n> > +\t\tlineBuffers_[i] = (uint8_t *)malloc(lineBufferLength_);\n> > +\t\tif (!lineBuffers_[i])\n> > +\t\t\treturn -ENOMEM;\n> > +\t}\n> > +\n> > +\tmeasuredFrames_ = 0;\n> > +\tframeProcessTime_ = 0;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/*\n> > + * Get width and height at which the bayer-pattern repeats.\n> > + * Return pattern-size or an empty Size for an unsupported inputFormat.\n> > + */\n> > +Size DebayerCpu::patternSize(PixelFormat inputFormat)\n> > +{\n> > +\tDebayerCpu::DebayerInputConfig config;\n> > +\n> > +\tif (getInputConfig(inputFormat, config) != 0)\n> > +\t\treturn {};\n> > +\n> > +\treturn config.patternSize;\n> > +}\n> > +\n> > +std::vector DebayerCpu::formats(PixelFormat inputFormat)\n> > +{\n> > +\tDebayerCpu::DebayerInputConfig config;\n> > +\n> > +\tif (getInputConfig(inputFormat, config) != 0)\n> > +\t\treturn std::vector();\n> > +\n> > +\treturn config.outputFormats;\n> > +}\n> > +\n> > +std::tuple\n> > +DebayerCpu::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)\n> > +{\n> > +\tDebayerCpu::DebayerOutputConfig config;\n> > +\n> > +\tif (getOutputConfig(outputFormat, config) != 0)\n> > +\t\treturn std::make_tuple(0, 0);\n> > +\n> > +\t/* round up to multiple of 8 for 64 bits alignment */\n> > +\tunsigned int stride = (size.width * config.bpp / 8 + 7) & ~7;\n> > +\n> > +\treturn std::make_tuple(stride, stride * size.height);\n> > +}\n> > +\n> > +void DebayerCpu::setupInputMemcpy(const uint8_t *linePointers[])\n> > +{\n> > +\tconst unsigned int patternHeight = inputConfig_.patternSize.height;\n> > +\n> > +\tif (!enableInputMemcpy_)\n> > +\t\treturn;\n> > +\n> > +\tfor (unsigned int i = 0; i < patternHeight; i++) {\n> > +\t\tmemcpy(lineBuffers_[i], linePointers[i + 1] - lineBufferPadding_,\n> > +\t\t lineBufferLength_);\n> > +\t\tlinePointers[i + 1] = lineBuffers_[i] + lineBufferPadding_;\n> > +\t}\n> > +\n> > +\t/* Point lineBufferIndex_ to first unused lineBuffer */\n> > +\tlineBufferIndex_ = patternHeight;\n> > +}\n> > +\n> > +void DebayerCpu::shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src)\n> > +{\n> > +\tconst unsigned int patternHeight = inputConfig_.patternSize.height;\n> > +\n> > +\tfor (unsigned int i = 0; i < patternHeight; i++)\n> > +\t\tlinePointers[i] = linePointers[i + 1];\n> > +\n> > +\tlinePointers[patternHeight] = src +\n> > +\t\t\t\t (patternHeight / 2) * (int)inputConfig_.stride;\n> > +}\n> > +\n> > +void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])\n> > +{\n> > +\tconst unsigned int patternHeight = inputConfig_.patternSize.height;\n> > +\n> > +\tif (!enableInputMemcpy_)\n> > +\t\treturn;\n> > +\n> > +\tmemcpy(lineBuffers_[lineBufferIndex_], linePointers[patternHeight] - lineBufferPadding_,\n> > +\t lineBufferLength_);\n> > +\tlinePointers[patternHeight] = lineBuffers_[lineBufferIndex_] + lineBufferPadding_;\n> > +\n> > +\tlineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);\n> > +}\n> > +\n> > +void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> > +{\n> > +\tunsigned int y_end = window_.y + window_.height;\n> > +\t/* Holds [0] previous- [1] current- [2] next-line */\n> > +\tconst uint8_t *linePointers[3];\n> > +\n> > +\t/* Adjust src to top left corner of the window */\n> > +\tsrc += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;\n> > +\n> > +\t/* [x] becomes [x - 1] after initial shiftLinePointers() call */\n> > +\tif (window_.y) {\n> > +\t\tlinePointers[1] = src - inputConfig_.stride; /* previous-line */\n> > +\t\tlinePointers[2] = src;\n> > +\t} else {\n> > +\t\t/* window_.y == 0, use the next line as prev line */\n> > +\t\tlinePointers[1] = src + inputConfig_.stride;\n> > +\t\tlinePointers[2] = src;\n> > +\t\t/* Last 2 lines also need special handling */\n> > +\t\ty_end -= 2;\n> > +\t}\n> > +\n> > +\tsetupInputMemcpy(linePointers);\n> > +\n> > +\tfor (unsigned int y = window_.y; y < y_end; y += 2) {\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\tmemcpyNextLine(linePointers);\n> > +\t\tstats_->processLine0(y, linePointers);\n> > +\t\t(this->*debayer0_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\tmemcpyNextLine(linePointers);\n> > +\t\t(this->*debayer1_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\t}\n> > +\n> > +\tif (window_.y == 0) {\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\tmemcpyNextLine(linePointers);\n> > +\t\tstats_->processLine0(y_end, linePointers);\n> > +\t\t(this->*debayer0_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\t/* next line may point outside of src, use prev. */\n> > +\t\tlinePointers[2] = linePointers[0];\n> > +\t\t(this->*debayer1_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\t}\n> > +}\n> > +\n> > +void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> > +{\n> > +\tconst unsigned int y_end = window_.y + window_.height;\n> > +\t/*\n> > +\t * This holds pointers to [0] 2-lines-up [1] 1-line-up [2] current-line\n> > +\t * [3] 1-line-down [4] 2-lines-down.\n> > +\t */\n> > +\tconst uint8_t *linePointers[5];\n> > +\n> > +\t/* Adjust src to top left corner of the window */\n> > +\tsrc += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;\n> > +\n> > +\t/* [x] becomes [x - 1] after initial shiftLinePointers() call */\n> > +\tlinePointers[1] = src - 2 * inputConfig_.stride;\n> > +\tlinePointers[2] = src - inputConfig_.stride;\n> > +\tlinePointers[3] = src;\n> > +\tlinePointers[4] = src + inputConfig_.stride;\n> > +\n> > +\tsetupInputMemcpy(linePointers);\n> > +\n> > +\tfor (unsigned int y = window_.y; y < y_end; y += 4) {\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\tmemcpyNextLine(linePointers);\n> > +\t\tstats_->processLine0(y, linePointers);\n> > +\t\t(this->*debayer0_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\tmemcpyNextLine(linePointers);\n> > +\t\t(this->*debayer1_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\tmemcpyNextLine(linePointers);\n> > +\t\tstats_->processLine2(y, linePointers);\n> > +\t\t(this->*debayer2_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\n> > +\t\tshiftLinePointers(linePointers, src);\n> > +\t\tmemcpyNextLine(linePointers);\n> > +\t\t(this->*debayer3_)(dst, linePointers);\n> > +\t\tsrc += inputConfig_.stride;\n> > +\t\tdst += outputConfig_.stride;\n> > +\t}\n> > +}\n> > +\n> > +static inline int64_t timeDiff(timespec &after, timespec &before)\n> > +{\n> > +\treturn (after.tv_sec - before.tv_sec) * 1000000000LL +\n> > +\t (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> > +}\n> > +\n> > +void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> > +{\n> > +\ttimespec frameStartTime;\n> > +\n> > +\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> > +\t\tframeStartTime = {};\n> > +\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> > +\t}\n> > +\n> > +\t/* Apply DebayerParams */\n> > +\tif (params.gamma != gamma_correction_) {\n> > +\t\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> > +\t\t\tgamma_[i] = UINT8_MAX * powf(i / (kGammaLookupSize - 1.0), params.gamma);\n> > +\n> > +\t\tgamma_correction_ = params.gamma;\n> > +\t}\n> > +\n> > +\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> > +\t\tconstexpr unsigned int div =\n> > +\t\t\tkRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n> > +\t\tunsigned int idx;\n> > +\n> > +\t\t/* Apply gamma after gain! */\n> > +\t\tidx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n> > +\t\tred_[i] = gamma_[idx];\n> > +\n> > +\t\tidx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n> > +\t\tgreen_[i] = gamma_[idx];\n> > +\n> > +\t\tidx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n> > +\t\tblue_[i] = gamma_[idx];\n> > +\t}\n> > +\n> > +\t/* Copy metadata from the input buffer */\n> > +\tFrameMetadata &metadata = output->_d()->metadata();\n> > +\tmetadata.status = input->metadata().status;\n> > +\tmetadata.sequence = input->metadata().sequence;\n> > +\tmetadata.timestamp = input->metadata().timestamp;\n> > +\n> > +\tMappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> > +\tMappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n> > +\tif (!in.isValid() || !out.isValid()) {\n> > +\t\tLOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n> > +\t\tmetadata.status = FrameMetadata::FrameError;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tstats_->startFrame();\n> > +\n> > +\tif (inputConfig_.patternSize.height == 2)\n> > +\t\tprocess2(in.planes()[0].data(), out.planes()[0].data());\n> > +\telse\n> > +\t\tprocess4(in.planes()[0].data(), out.planes()[0].data());\n> > +\n> > +\tmetadata.planes()[0].bytesused = out.planes()[0].size();\n> > +\n> > +\t/* Measure before emitting signals */\n> > +\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> > +\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> > +\t\ttimespec frameEndTime = {};\n> > +\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> > +\t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> > +\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> > +\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> > +\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n> > +\t\t\tLOG(Debayer, Info)\n> > +\t\t\t\t<< \"Processed \" << measuredFrames\n> > +\t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> > +\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> > +\t\t\t\t<< \" us/frame\";\n> > +\t\t}\n> > +\t}\n> \n> Is this debugging leftovers, or should it be cleaned up ? It seems quite\n> a bit of a hack. If we want to enable measurements, we shouldn't\n> hardcode them to frames 30 to 60.\n> \n> > +\n> > +\tstats_->finishFrame();\n> > +\toutputBufferReady.emit(output);\n> > +\tinputBufferReady.emit(input);\n> > +}\n> > +\n> > +SizeRange DebayerCpu::sizes(PixelFormat inputFormat, const Size &inputSize)\n> > +{\n> > +\tSize pattern_size = patternSize(inputFormat);\n> > +\tunsigned int border_height = pattern_size.height;\n> > +\n> > +\tif (pattern_size.isNull())\n> > +\t\treturn {};\n> > +\n> > +\t/* No need for top/bottom border with a pattern height of 2 */\n> > +\tif (pattern_size.height == 2)\n> > +\t\tborder_height = 0;\n> > +\n> > +\t/*\n> > +\t * For debayer interpolation a border is kept around the entire image\n> > +\t * and the minimum output size is pattern-height x pattern-width.\n> > +\t */\n> > +\tif (inputSize.width < (3 * pattern_size.width) ||\n> > +\t inputSize.height < (2 * border_height + pattern_size.height)) {\n> > +\t\tLOG(Debayer, Warning)\n> > +\t\t\t<< \"Input format size too small: \" << inputSize.toString();\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\treturn SizeRange(Size(pattern_size.width, pattern_size.height),\n> > +\t\t\t Size((inputSize.width - 2 * pattern_size.width) & ~(pattern_size.width - 1),\n> > +\t\t\t (inputSize.height - 2 * border_height) & ~(pattern_size.height - 1)),\n> > +\t\t\t pattern_size.width, pattern_size.height);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> > new file mode 100644\n> > index 00000000..8a51ed85\n> > --- /dev/null\n> > +++ b/src/libcamera/software_isp/debayer_cpu.h\n> > @@ -0,0 +1,143 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Linaro Ltd\n> > + * Copyright (C) 2023, Red Hat Inc.\n> > + *\n> > + * Authors:\n> > + * Hans de Goede \n> > + *\n> > + * debayer_cpu.h - CPU based debayering header\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"debayer.h\"\n> > +#include \"swstats_cpu.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class DebayerCpu : public Debayer, public Object\n> > +{\n> > +public:\n> > +\tDebayerCpu(std::unique_ptr stats);\n> > +\t~DebayerCpu();\n> > +\n> > +\tint configure(const StreamConfiguration &inputCfg,\n> > +\t\t const std::vector> &outputCfgs);\n> > +\tSize patternSize(PixelFormat inputFormat);\n> > +\tstd::vector formats(PixelFormat input);\n> > +\tstd::tuple\n> > +\tstrideAndFrameSize(const PixelFormat &outputFormat, const Size &size);\n> > +\tvoid process(FrameBuffer *input, FrameBuffer *output, DebayerParams params);\n> > +\tSizeRange sizes(PixelFormat inputFormat, const Size &inputSize);\n> > +\n> > +\t/**\n> > +\t * \\brief Get the file descriptor for the statistics.\n> > +\t *\n> > +\t * \\return the file descriptor pointing to the statistics.\n> > +\t */\n> > +\tconst SharedFD &getStatsFD() { return stats_->getStatsFD(); }\n> \n> This, plus the fact that this class handles colour gains and gamma,\n> makes me thing we have either a naming issue, or an architecture issue.\n> \n> > +\n> > +\t/**\n> > +\t * \\brief Get the output frame size.\n> > +\t *\n> > +\t * \\return The output frame size.\n> > +\t */\n> > +\tunsigned int frameSize() { return outputConfig_.frameSize; }\n> > +\n> > +private:\n> > +\t/**\n> > +\t * \\brief Called to debayer 1 line of Bayer input data to output format\n> > +\t * \\param[out] dst Pointer to the start of the output line to write\n> > +\t * \\param[in] src The input data\n> > +\t *\n> > +\t * Input data is an array of (patternSize_.height + 1) src\n> > +\t * pointers each pointing to a line in the Bayer source. The middle\n> > +\t * element of the array will point to the actual line being processed.\n> > +\t * Earlier element(s) will point to the previous line(s) and later\n> > +\t * element(s) to the next line(s).\n> > +\t *\n> > +\t * These functions take an array of src pointers, rather than\n> > +\t * a single src pointer + a stride for the source, so that when the src\n> > +\t * is slow uncached memory it can be copied to faster memory before\n> > +\t * debayering. Debayering a standard 2x2 Bayer pattern requires access\n> > +\t * to the previous and next src lines for interpolating the missing\n> > +\t * colors. To allow copying the src lines only once 3 temporary buffers\n> > +\t * each holding a single line are used, re-using the oldest buffer for\n> > +\t * the next line and the pointers are swizzled so that:\n> > +\t * src[0] = previous-line, src[1] = currrent-line, src[2] = next-line.\n> > +\t * This way the 3 pointers passed to the debayer functions form\n> > +\t * a sliding window over the src avoiding the need to copy each\n> > +\t * line more than once.\n> > +\t *\n> > +\t * Similarly for bayer patterns which repeat every 4 lines, 5 src\n> > +\t * pointers are passed holding: src[0] = 2-lines-up, src[1] = 1-line-up\n> > +\t * src[2] = current-line, src[3] = 1-line-down, src[4] = 2-lines-down.\n> > +\t */\n> > +\tusing debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);\n> > +\n> > +\t/* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */\n> > +\tvoid debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> > +\tvoid debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);\n> > +\tvoid debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);\n> > +\tvoid debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);\n> > +\n> > +\tstruct DebayerInputConfig {\n> > +\t\tSize patternSize;\n> > +\t\tunsigned int bpp; /* Memory used per pixel, not precision */\n> > +\t\tunsigned int stride;\n> > +\t\tstd::vector outputFormats;\n> > +\t};\n> > +\n> > +\tstruct DebayerOutputConfig {\n> > +\t\tunsigned int bpp; /* Memory used per pixel, not precision */\n> > +\t\tunsigned int stride;\n> > +\t\tunsigned int frameSize;\n> > +\t};\n> > +\n> > +\tint getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);\n> > +\tint getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);\n> > +\tint setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);\n> > +\tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n> > +\tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n> > +\tvoid memcpyNextLine(const uint8_t *linePointers[]);\n> > +\tvoid process2(const uint8_t *src, uint8_t *dst);\n> > +\tvoid process4(const uint8_t *src, uint8_t *dst);\n> > +\n> > +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> > +\tstatic constexpr unsigned int kRGBLookupSize = 256;\n> > +\t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> > +\tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> > +\n> > +\tstd::array gamma_;\n> > +\tstd::array red_;\n> > +\tstd::array green_;\n> > +\tstd::array blue_;\n> > +\tdebayerFn debayer0_;\n> > +\tdebayerFn debayer1_;\n> > +\tdebayerFn debayer2_;\n> > +\tdebayerFn debayer3_;\n> > +\tRectangle window_;\n> > +\tDebayerInputConfig inputConfig_;\n> > +\tDebayerOutputConfig outputConfig_;\n> > +\tstd::unique_ptr stats_;\n> > +\tuint8_t *lineBuffers_[kMaxLineBuffers];\n> > +\tunsigned int lineBufferLength_;\n> > +\tunsigned int lineBufferPadding_;\n> > +\tunsigned int lineBufferIndex_;\n> > +\tbool enableInputMemcpy_;\n> > +\tfloat gamma_correction_;\n> > +\tunsigned int measuredFrames_;\n> > +\tint64_t frameProcessTime_;\n> > +\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n> > +\tstatic constexpr unsigned int kFramesToSkip = 30;\n> > +\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build\n> > index 62095f61..71b46539 100644\n> > --- a/src/libcamera/software_isp/meson.build\n> > +++ b/src/libcamera/software_isp/meson.build\n> > @@ -9,5 +9,6 @@ endif\n> > \n> > libcamera_sources += files([\n> > 'debayer.cpp',\n> > + 'debayer_cpu.cpp',\n> > 'swstats_cpu.cpp',\n> > ])","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9BE78BD160\n\tfor ;\n\tWed, 27 Mar 2024 18:01:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C41C63340;\n\tWed, 27 Mar 2024 19:01:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DF1661C41\n\tfor ;\n\tWed, 27 Mar 2024 19:01:42 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02AC82CFB;\n\tWed, 27 Mar 2024 19:01:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V0vFP6sM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711562469;\n\tbh=loxMUpYup/3IxlNmYy4VEkzWREiqJOo2igXW9mllpfI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V0vFP6sMqeNV8Epf+lLVA/F61R2e7a0ppf/ImFrkDFEwz0u3vLxRYNKv9g/HShhyW\n\tWe4ox31tLUuXUNPPMM9pW8sDrfMM1Z/0VKNO9b0Ye/TpkxlvHKdMvZ/Sex9v9Poz2W\n\tf1BbGeiPoX+98lkI5Z4AXxVqPl1dLDQ8Sj7CrXCw=","Date":"Wed, 27 Mar 2024 20:01:32 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Hans de Goede ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ,\n\tDennis Bonke ,\n\tAndrey Konovalov ","Subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","Message-ID":"<20240327180132.GK4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-9-mzamazal@redhat.com>\n\t<20240327141225.GF4721@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240327141225.GF4721@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29104,"web_url":"https://patchwork.libcamera.org/comment/29104/","msgid":"<53f37db2-d407-4c38-a4b5-ebe7a05ed4a8@redhat.com>","date":"2024-03-27T18:08:02","subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Laurent,\n\nOn 3/27/24 3:12 PM, Laurent Pinchart wrote:\n> Hi Milan and Hans,\n> \n> Thank you for the patch.\n\nSame as with the previous review: Thank you for the review.\n\nMilan is going to prepare a v7 of this series, so I'm going to \nall the trivial remarks and only comment on the remaining remarks.\n\n> On Tue, Mar 19, 2024 at 01:35:55PM +0100, Milan Zamazal wrote:\n>> From: Hans de Goede \n\n\n\n>> +{\n>> +#ifdef __x86_64__\n>> +\tenableInputMemcpy_ = false;\n>> +#else\n>> +\tenableInputMemcpy_ = true;\n>> +#endif\n> \n> This is very much *not* nice :-( It needs to at least be explained\n> clearly somewhere.\n\nYeah this really needs to be a parameter to the SoftwareISP\nconstructor because some non x86 designs may also have\nfully cached buffers, so they can also skip the extra\nmemcpy step.\n\nMy suggestion would be to introduce a struct SoftwareIspParams\nwith only a bool enableInputMemcpy_ in there for now and\nthen in the simplepipeline handler instead of having a bool\nflag to indicate if the SoftwareISP should be list for a specific\nmedia-controller driver-name, have a pointer to this struct and\nhave that pointer being non NULL indicate the swisp should\nbe used.\n\nFor now we would then have 2 SoftwareIspParams structs\nthis pointer can point to (uncached and cached) but in the future\nwe may very well have 1 SoftwareIspParams struct per supported\nmediactl driver.\n\nActually since the IPU6 kernel side has not been merged yet,\nfor now we can just replace this with a hardcoded:\n\n\tenableInputMemcpy_ = true;\n\nMilan, lets do that for the v7 submission.\n\nThen I can add some mechanism to config this as a follow up\npatch when IPU6 has actually landed in the kernel and thus\nwe can enable it in libcamera upstream.\n\n\n\n>> +void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> +{\n>> +\tconst int width_in_bytes = window_.width * 5 / 4;\n> \n> snakeCase here too, ane where applicable everywhere else.\n> \n>> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> \n> Is the cast needed ? If so it should be a C++ cast.\n\nFor the set of debayer10P_* functions introduced as the first\nsupported bayer fmt these casts are not necessary and can\nbe dropped.\n\nIn \"[PATCH v6 14/18] libcamera: debayer_cpu: Add support for 8, 10\nand 12 bpp unpacked bayer input\" there are more casts and these\nare actually necessary and will need to be replaced with\nstatic c++ style casts.\n\n>> +\tLOG(Debayer, Info)\n> \n> Shouldn't this be an Error mesage ? Same below.\n\nThese functions are called when the simple pipeline handler\nis discovering supported formats so on CSI receivers or sensors\nsupporting multiple formats this happen for one of the fmts\nis sorta expected behavior.\n\nI don't feel strongly about this though, so if you want this\nto be upgraded to an Error we can do that.\n\n\n\n\n> \n>> +\t\t<< \"Unsupported input format \" << inputFormat.toString();\n>> +\treturn -EINVAL;\n>> +}\n\n\n\n>> +\t/* Measure before emitting signals */\n>> +\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>> +\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>> +\t\ttimespec frameEndTime = {};\n>> +\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>> +\t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>> +\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>> +\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>> +\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n>> +\t\t\tLOG(Debayer, Info)\n>> +\t\t\t\t<< \"Processed \" << measuredFrames\n>> +\t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>> +\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n>> +\t\t\t\t<< \" us/frame\";\n>> +\t\t}\n>> +\t}\n> \n> Is this debugging leftovers, or should it be cleaned up ? It seems quite\n> a bit of a hack. If we want to enable measurements, we shouldn't\n> hardcode them to frames 30 to 60.\n\nThis is meant to be kept around, also see:\n\n[PATCH v6 16/18] libcamera: Add \"Software ISP benchmarking\" documentation\n\nthe SoftwareISP is sensitive to performance regressions so this is\nintended as a simple builtin benchmark to check for those.\n\nSo that means this falls under the \"or should it be cleaned up\" part\nof your question.\n\nYou indicate you don't like the hardcoded frames 30 - 60. The idea is\nto warm up the caches a bit and then measure a bunch of frames, which\nis just what this does. What alternative approach are you suggesting?\n\n>> +\t/**\n>> +\t * \\brief Get the file descriptor for the statistics.\n>> +\t *\n>> +\t * \\return the file descriptor pointing to the statistics.\n>> +\t */\n>> +\tconst SharedFD &getStatsFD() { return stats_->getStatsFD(); }\n> \n> This,\n\nNote the statistics pass-through stuff is sort of a necessary evil\nsince we want one main loop going over the data line by line and\ndoing both debayering as well as stats while the line is still\nhot in the l2 cache. And things like the process2() and process4()\nloops are highly CPU debayering specific so I don't think we should\nmove those out of the CpuDebayer code.\n\n> plus the fact that this class handles colour gains and gamma,\n> makes me thing we have either a naming issue, or an architecture issue.\n\nI agree that this does a bit more then debayering, although\nthe debayering really is the main thing it does.\n\nI guess the calculation of the rgb lookup tables which do the\ncolor gains and gamma could be moved outside of this class,\nthat might even be beneficial for GPU based debayering assuming\nthat that is going to use rgb lookup tables too (it could\nimplement actual color gains + gamma correction in some different\nway).\n\nI think this falls under the lets wait until we have a GPU\nbased SoftISP MVP/POC and then do some refactoring to see which\nbits should go where.\n\nRegards,\n\nHans","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 73795C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 18:08:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2328C63340;\n\tWed, 27 Mar 2024 19:08:12 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 844EE61C41\n\tfor ;\n\tWed, 27 Mar 2024 19:08:10 +0100 (CET)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-267-Oc3m_3ZAOO20c2zSCmspFw-1; Wed, 27 Mar 2024 14:08:05 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a4e05cee8faso5455666b.1\n\tfor ;\n\tWed, 27 Mar 2024 11:08:05 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\tj9-20020a17090623e900b00a46e56c8764sm5668156ejg.114.2024.03.27.11.08.02\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 27 Mar 2024 11:08:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"OE5URXq7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1711562889;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=7Ayc+WMAwFSkDmIswqvge7St8YTU/Jm1WIPOpDdmEs4=;\n\tb=OE5URXq7rgDFd/hVNTf/ey9iPEMiasNZ2RjxAptUYjUnQhfKQQPFkDT+d5r8Kfu+Bg4usE\n\tru94uOPNRoIGYjNvo8yTCuEdH82pQYWkngpY9kwYfAIkQyNrSt4fZb79k8lWhiarXAMRSx\n\tQrUcXk4ee9eWRSYHa8lITTyQgBnHh3M=","X-MC-Unique":"Oc3m_3ZAOO20c2zSCmspFw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711562884; x=1712167684;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=7Ayc+WMAwFSkDmIswqvge7St8YTU/Jm1WIPOpDdmEs4=;\n\tb=flayn+rRI1El6f7vLyfkk7hKmGNpBriwMfHFU51qAMqKTF79pwgcqV4HZg22j6Cpxy\n\tteAKcQWAagHC2RP4sRitS3t3yUJJ9YnfeFwHukY+ay+lgUVHzsOYaeyYu7BqALUVfz8F\n\tqWvMI5Q0Pa3PIeMfC4ERovOWTXEzPtRiEUN3/F1EVhm8Vq3Gq7Ucz1f+q0tL7DStUWSr\n\t+MRk/mcB9tv90F1j6nxbIhUQMHhGOBB+DXhHW6r77lpmQ76ZaB9ioCCvfubR56pb5BTc\n\t4WMyyO+SSWxr+r9FS5YByd0z/W6txCWig5FWUciBrfHogcwOBaq4KY2NLkzyN/luJHc5\n\tiT/A==","X-Gm-Message-State":"AOJu0YyhCF6KZk9uVFx54c1anLTOdvwymTKowkyCaST8BBfvjlN8QUbK\n\tqLre8lAvuKeEHwZWdkuVHgda2ScvAx2WJvhSBbLR31xW8SQhWfRtOEr9tjHtKb3WnNPY/Q2Pdcz\n\tFluB1FCkOG8+wFR+ebOfp9L4yfbzpClqsNCZrs9umm7scMef40rMAOt2zji/VkA83nA75ob4=","X-Received":["by 2002:a17:906:c0d3:b0:a46:d6f7:e8f5 with SMTP id\n\tbn19-20020a170906c0d300b00a46d6f7e8f5mr171805ejb.18.1711562884130; \n\tWed, 27 Mar 2024 11:08:04 -0700 (PDT)","by 2002:a17:906:c0d3:b0:a46:d6f7:e8f5 with SMTP id\n\tbn19-20020a170906c0d300b00a46d6f7e8f5mr171786ejb.18.1711562883682; \n\tWed, 27 Mar 2024 11:08:03 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHNc6dW39yQ3EZAoyei8siiOxZzftSwN/96a3V93L5oMPB9O0PFSCeR8mQHchE4o7CECbFckw==","Message-ID":"<53f37db2-d407-4c38-a4b5-ebe7a05ed4a8@redhat.com>","Date":"Wed, 27 Mar 2024 19:08:02 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","To":"Laurent Pinchart ,\n\tMilan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ,\n\tDennis Bonke ,\n\tAndrey Konovalov ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-9-mzamazal@redhat.com>\n\t<20240327141225.GF4721@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240327141225.GF4721@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29113,"web_url":"https://patchwork.libcamera.org/comment/29113/","msgid":"<20240327193729.GS4721@pendragon.ideasonboard.com>","date":"2024-03-27T19:37:29","subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Wed, Mar 27, 2024 at 07:08:02PM +0100, Hans de Goede wrote:\n> On 3/27/24 3:12 PM, Laurent Pinchart wrote:\n> > Hi Milan and Hans,\n> > \n> > Thank you for the patch.\n> \n> Same as with the previous review: Thank you for the review.\n> \n> Milan is going to prepare a v7 of this series, so I'm going to \n> all the trivial remarks and only comment on the remaining remarks.\n> \n> > On Tue, Mar 19, 2024 at 01:35:55PM +0100, Milan Zamazal wrote:\n> >> From: Hans de Goede \n> \n> \n> \n> >> +{\n> >> +#ifdef __x86_64__\n> >> +\tenableInputMemcpy_ = false;\n> >> +#else\n> >> +\tenableInputMemcpy_ = true;\n> >> +#endif\n> > \n> > This is very much *not* nice :-( It needs to at least be explained\n> > clearly somewhere.\n> \n> Yeah this really needs to be a parameter to the SoftwareISP\n> constructor because some non x86 designs may also have\n> fully cached buffers, so they can also skip the extra\n> memcpy step.\n> \n> My suggestion would be to introduce a struct SoftwareIspParams\n> with only a bool enableInputMemcpy_ in there for now and\n> then in the simplepipeline handler instead of having a bool\n> flag to indicate if the SoftwareISP should be list for a specific\n> media-controller driver-name, have a pointer to this struct and\n> have that pointer being non NULL indicate the swisp should\n> be used.\n> \n> For now we would then have 2 SoftwareIspParams structs\n> this pointer can point to (uncached and cached) but in the future\n> we may very well have 1 SoftwareIspParams struct per supported\n> mediactl driver.\n> \n> Actually since the IPU6 kernel side has not been merged yet,\n> for now we can just replace this with a hardcoded:\n> \n> \tenableInputMemcpy_ = true;\n> \n> Milan, lets do that for the v7 submission.\n\nCould you please also add a comment that explains why we want the memcpy\nto be configurable ?\n\n> Then I can add some mechanism to config this as a follow up\n> patch when IPU6 has actually landed in the kernel and thus\n> we can enable it in libcamera upstream.\n> \n> \n> \n> >> +void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])\n> >> +{\n> >> +\tconst int width_in_bytes = window_.width * 5 / 4;\n> > \n> > snakeCase here too, ane where applicable everywhere else.\n> > \n> >> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> > \n> > Is the cast needed ? If so it should be a C++ cast.\n> \n> For the set of debayer10P_* functions introduced as the first\n> supported bayer fmt these casts are not necessary and can\n> be dropped.\n> \n> In \"[PATCH v6 14/18] libcamera: debayer_cpu: Add support for 8, 10\n> and 12 bpp unpacked bayer input\" there are more casts and these\n> are actually necessary and will need to be replaced with\n> static c++ style casts.\n\nRight, I hadn't seen that yet when reviewing this patch.\n\n> >> +\tLOG(Debayer, Info)\n> > \n> > Shouldn't this be an Error mesage ? Same below.\n> \n> These functions are called when the simple pipeline handler\n> is discovering supported formats so on CSI receivers or sensors\n> supporting multiple formats this happen for one of the fmts\n> is sorta expected behavior.\n> \n> I don't feel strongly about this though, so if you want this\n> to be upgraded to an Error we can do that.\n\nIf it's an expected behaviour, I would make this a debug message then.\n\n> >> +\t\t<< \"Unsupported input format \" << inputFormat.toString();\n> >> +\treturn -EINVAL;\n> >> +}\n> \n> \n> \n> >> +\t/* Measure before emitting signals */\n> >> +\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> >> +\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> >> +\t\ttimespec frameEndTime = {};\n> >> +\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> >> +\t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> >> +\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> >> +\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> >> +\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n> >> +\t\t\tLOG(Debayer, Info)\n> >> +\t\t\t\t<< \"Processed \" << measuredFrames\n> >> +\t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> >> +\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> >> +\t\t\t\t<< \" us/frame\";\n> >> +\t\t}\n> >> +\t}\n> > \n> > Is this debugging leftovers, or should it be cleaned up ? It seems quite\n> > a bit of a hack. If we want to enable measurements, we shouldn't\n> > hardcode them to frames 30 to 60.\n> \n> This is meant to be kept around, also see:\n> \n> [PATCH v6 16/18] libcamera: Add \"Software ISP benchmarking\" documentation\n> \n> the SoftwareISP is sensitive to performance regressions so this is\n> intended as a simple builtin benchmark to check for those.\n> \n> So that means this falls under the \"or should it be cleaned up\" part\n> of your question.\n> \n> You indicate you don't like the hardcoded frames 30 - 60. The idea is\n> to warm up the caches a bit and then measure a bunch of frames, which\n> is just what this does. What alternative approach are you suggesting?\n\nI wonder if there would be a way to control at runtime when/how to\nperform those measurements. Maybe that's a bit overkill. In any case, it\nwould be something to address later.\n\n> >> +\t/**\n> >> +\t * \\brief Get the file descriptor for the statistics.\n> >> +\t *\n> >> +\t * \\return the file descriptor pointing to the statistics.\n> >> +\t */\n> >> +\tconst SharedFD &getStatsFD() { return stats_->getStatsFD(); }\n> > \n> > This,\n> \n> Note the statistics pass-through stuff is sort of a necessary evil\n> since we want one main loop going over the data line by line and\n> doing both debayering as well as stats while the line is still\n> hot in the l2 cache. And things like the process2() and process4()\n> loops are highly CPU debayering specific so I don't think we should\n> move those out of the CpuDebayer code.\n\nYes, that I understood from the review. \"necessary evil\" is indeed the\nright term :-) I expect it will take quite some design skills to balance\nthe need for performances and the need for a maintainable architecture.\n\n> > plus the fact that this class handles colour gains and gamma,\n> > makes me thing we have either a naming issue, or an architecture issue.\n> \n> I agree that this does a bit more then debayering, although\n> the debayering really is the main thing it does.\n> \n> I guess the calculation of the rgb lookup tables which do the\n> color gains and gamma could be moved outside of this class,\n> that might even be beneficial for GPU based debayering assuming\n> that that is going to use rgb lookup tables too (it could\n> implement actual color gains + gamma correction in some different\n> way).\n> \n> I think this falls under the lets wait until we have a GPU\n> based SoftISP MVP/POC and then do some refactoring to see which\n> bits should go where.\n\nAs long as we record this in the todo file (with a short summary of the\nabove), that's fine with me.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E8839C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 19:37:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE6C963339;\n\tWed, 27 Mar 2024 20:37:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 25C9C61C41\n\tfor ;\n\tWed, 27 Mar 2024 20:37:38 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23656899;\n\tWed, 27 Mar 2024 20:37:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G8efUuPY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711568225;\n\tbh=9v8LHtuuerVVhkTC2gfg1oy+iXRDmHBLDMnMNhhyGCA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G8efUuPY7dol7JC3oiasPUkibnLB6ZNfFnLZO8T40G+6/TLnVDR0KwKIn0C6OXQGY\n\t8mciX+RxEMIlUfSm+eZt4lOo8eNsUOydQAveZ60u3w8D7vubFdGb2W68Ifcd0fyGJ+\n\to7/73Y8pg+m+cXZVFNvP9S5joPII6ytoH+gs0oFk=","Date":"Wed, 27 Mar 2024 21:37:29 +0200","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ,\n\tDennis Bonke ,\n\tAndrey Konovalov ","Subject":"Re: [PATCH v6 08/18] libcamera: software_isp: Add DebayerCpu class","Message-ID":"<20240327193729.GS4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-9-mzamazal@redhat.com>\n\t<20240327141225.GF4721@pendragon.ideasonboard.com>\n\t<53f37db2-d407-4c38-a4b5-ebe7a05ed4a8@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<53f37db2-d407-4c38-a4b5-ebe7a05ed4a8@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]