[{"id":32344,"web_url":"https://patchwork.libcamera.org/comment/32344/","msgid":"<3d3b7d90-20c9-4f2d-9fb6-39abdd0e0cc0@collabora.com>","date":"2024-11-24T15:37:30","subject":"Re: [PATCH 9/9] libcamera: software_isp: Apply CCM in debayering","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Hi, awesome to see this series! I currently giving this a try - found \ntwo issues so far:\n\n 1. Currently things crash because of the wrong gamma clamp, see below.\n 2. With that fixed I get a Red<->Blue channel switch. Haven't found the\n line yet, but it also happens without this patch, so probably\n happens earlier in the series.\n\nOn 20.11.24 19:01, Milan Zamazal wrote:\n> This patch applies color correction matrix (CCM) in debayering if the\n> CCM is specified. Not using CCM must still be supported for performance\n> reasons.\n>\n> The CCM is applied as follows:\n>\n> [r1 r2 r3]\n> [r g b] * [g1 g2 g3]\n> [b1 b2 b3]\n>\n> The CCM matrix (the right side of the multiplication) is constant during\n> single frame processing, while the input pixel (the left side) changes.\n> Because each of the color channels is only 8-bit in software ISP, we can\n> make 9 lookup tables with 256 input values for multiplications of each\n> of the r_i, g_i, b_i values. This way we don't have to multiply each\n> pixel, we can use table lookups and additions instead. Gamma (which is\n> non-linear and thus cannot be a part of the 9 lookup tables values) is\n> applied on the final values rounded to integers using another lookup\n> table.\n>\n> We use int16_t to store the precomputed multiplications. This seems to\n> be noticeably (>10%) faster than `float' for the price of slightly less\n> accuracy and it covers the range of values that sane CCMs produce. The\n> selection and structure of data is performance critical, for example\n> using bytes would add significant (>10%) speedup but would be too short\n> to cover the value range.\n>\n> The color lookup tables are changed to unions to be able to serve as\n> both simple lookup tables or CCM lookup tables. This is arguable but it\n> keeps the code easier to understand if nothing else. It is important to\n> make the unions on the whole lookup tables rather than their values\n> because the latter might cause a noticeable slowdown on simple lookup\n> due to the sheer fact that the table is not compact. Using unions makes\n> the initialization of the tables dubious because it is done before the\n> decision about using CCM is made but the initial values are dubious\n> anyway.\n>\n> The tables are copied (as before), which is not elegant but also not a\n> big problem. There are patches posted that use shared buffers for\n> parameters passing in software ISP (see software ISP TODO #5) and they\n> can be adjusted for the new parameter format.\n>\n> With this patch, the reported per-frame slowdown when applying CCM is\n> about 45% on Debix Model A and about 85% on TI AM69 SK.\n>\n> Signed-off-by: Milan Zamazal\n> ---\n> .../internal/software_isp/debayer_params.h | 20 ++++++-\n> src/ipa/simple/algorithms/lut.cpp | 58 ++++++++++++++-----\n> src/ipa/simple/algorithms/lut.h | 1 +\n> src/libcamera/software_isp/debayer.cpp | 53 ++++++++++++++++-\n> src/libcamera/software_isp/debayer_cpu.cpp | 34 ++++++++---\n> src/libcamera/software_isp/debayer_cpu.h | 7 ++-\n> src/libcamera/software_isp/software_isp.cpp | 6 +-\n> 7 files changed, 145 insertions(+), 34 deletions(-)\n>\n> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n> index 7d8fdd481..531db6968 100644\n> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> @@ -18,11 +18,25 @@ namespace libcamera {\n> struct DebayerParams {\n> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n> \n> +\tstruct CcmRow {\n> +\t\tint16_t c1;\n> +\t\tint16_t c2;\n> +\t\tint16_t c3;\n> +\t};\n> +\n> \tusing ColorLookupTable = std::array;\n> +\tusing CcmLookupTable = std::array;\n> +\tusing GammaLookupTable = std::array;\n> +\n> +\tunion LookupTable {\n> +\t\tColorLookupTable simple;\n> +\t\tCcmLookupTable ccm;\n> +\t};\n> \n> -\tColorLookupTable red;\n> -\tColorLookupTable green;\n> -\tColorLookupTable blue;\n> +\tLookupTable red;\n> +\tLookupTable green;\n> +\tLookupTable blue;\n> +\tGammaLookupTable gammaLut;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> index 8fca96b0a..259976e08 100644\n> --- a/src/ipa/simple/algorithms/lut.cpp\n> +++ b/src/ipa/simple/algorithms/lut.cpp\n> @@ -44,6 +44,11 @@ void Lut::updateGammaTable(IPAContext &context)\n> \tcontext.activeState.gamma.blackLevel = blackLevel;\n> }\n> \n> +int16_t Lut::ccmValue(unsigned int i, float ccm) const\n> +{\n> +\treturn std::round(i * ccm);\n> +}\n> +\n> void Lut::prepare(IPAContext &context,\n> \t\t [[maybe_unused]] const uint32_t frame,\n> \t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> @@ -55,27 +60,48 @@ void Lut::prepare(IPAContext &context,\n> \t * observed, it's not permanently prone to minor fluctuations or\n> \t * rounding errors.\n> \t */\n> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n> +\tconst bool gammaUpdateNeeded =\n> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level;\n> +\tif (gammaUpdateNeeded)\n> \t\tupdateGammaTable(context);\n> \n> \tauto &gains = context.activeState.awb.gains;\n> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n> \tconst unsigned int gammaTableSize = gammaTable.size();\n> -\n> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> -\t\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n> -\t\t\t\t gammaTableSize;\n> -\t\t/* Apply gamma after gain! */\n> -\t\tunsigned int idx;\n> -\t\tidx = std::min({ static_cast(i * gains.red / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->red[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.green / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->green[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.blue / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->blue[i] = gammaTable[idx];\n> +\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n> +\t\t\t gammaTableSize;\n> +\n> +\tif (!context.activeState.ccm.enabled) {\n> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\t\t/* Apply gamma after gain! */\n> +\t\t\tunsigned int idx;\n> +\t\t\tidx = std::min({ static_cast(i * gains.red / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->red.simple[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.green / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->green.simple[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.blue / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->blue.simple[i] = gammaTable[idx];\n> +\t\t}\n> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n> +\t\tauto &ccm = context.activeState.ccm.ccm;\n> +\t\tauto &red = params->red.ccm;\n> +\t\tauto &green = params->green.ccm;\n> +\t\tauto &blue = params->blue.ccm;\n> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\t\tred[i].c1 = ccmValue(i, ccm[0][0]);\n> +\t\t\tred[i].c2 = ccmValue(i, ccm[0][1]);\n> +\t\t\tred[i].c3 = ccmValue(i, ccm[0][2]);\n> +\t\t\tgreen[i].c1 = ccmValue(i, ccm[1][0]);\n> +\t\t\tgreen[i].c2 = ccmValue(i, ccm[1][1]);\n> +\t\t\tgreen[i].c3 = ccmValue(i, ccm[1][2]);\n> +\t\t\tblue[i].c1 = ccmValue(i, ccm[2][0]);\n> +\t\t\tblue[i].c2 = ccmValue(i, ccm[2][1]);\n> +\t\t\tblue[i].c3 = ccmValue(i, ccm[2][2]);\n> +\t\t\tparams->gammaLut[i] = gammaTable[i / div];\n> +\t\t}\n> \t}\n> }\n> \n> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> index b635987d0..8d50a23fc 100644\n> --- a/src/ipa/simple/algorithms/lut.h\n> +++ b/src/ipa/simple/algorithms/lut.h\n> @@ -27,6 +27,7 @@ public:\n> \n> private:\n> \tvoid updateGammaTable(IPAContext &context);\n> +\tint16_t ccmValue(unsigned int i, float ccm) const;\n> };\n> \n> } /* namespace ipa::soft::algorithms */\n> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n> index f0b832619..97106b22b 100644\n> --- a/src/libcamera/software_isp/debayer.cpp\n> +++ b/src/libcamera/software_isp/debayer.cpp\n> @@ -23,11 +23,56 @@ namespace libcamera {\n> * \\brief Size of a color lookup table\n> */\n> \n> +/**\n> + * \\struct DebayerParams::CcmRow\n> + * \\brief Type of a single row of a color correction matrix\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmRow::c1\n> + * \\brief First column of a CCM row\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmRow::c2\n> + * \\brief Second column of a CCM row\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmRow::c3\n> + * \\brief Third column of a CCM row\n> + */\n> +\n> /**\n> * \\typedef DebayerParams::ColorLookupTable\n> + * \\brief Type of the simple lookup tables for red, green, blue values\n> + */\n> +\n> +/**\n> + * \\typedef DebayerParams::CcmLookupTable\n> + * \\brief Type of the CCM lookup tables for red, green, blue values\n> + */\n> +\n> +/**\n> + * \\typedef DebayerParams::GammaLookupTable\n> + * \\brief Type of the gamma lookup tables for CCM\n> + */\n> +\n> +/**\n> + * \\union DebayerParams::LookupTable\n> * \\brief Type of the lookup tables for red, green, blue values\n> */\n> \n> +/**\n> + * \\var DebayerParams::LookupTable::simple\n> + * \\brief Simple lookup table for red, green, blue values\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::LookupTable::ccm\n> + * \\brief CCM lookup table for red, green, blue values\n> + */\n> +\n> /**\n> * \\var DebayerParams::red\n> * \\brief Lookup table for red color, mapping input values to output values\n> @@ -43,6 +88,11 @@ namespace libcamera {\n> * \\brief Lookup table for blue color, mapping input values to output values\n> */\n> \n> +/**\n> + * \\var DebayerParams::gammaLut\n> + * \\brief Gamma lookup table used with color correction matrix\n> + */\n> +\n> /**\n> * \\class Debayer\n> * \\brief Base debayering class\n> @@ -57,10 +107,11 @@ Debayer::~Debayer()\n> }\n> \n> /**\n> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, ccmEnabled)\n> * \\brief Configure the debayer object according to the passed in parameters\n> * \\param[in] inputCfg The input configuration\n> * \\param[in] outputCfgs The output configurations\n> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n> *\n> * \\return 0 on success, a negative errno on failure\n> */\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 52baf43dc..d8976f183 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -11,6 +11,7 @@\n> \n> #include \"debayer_cpu.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -50,8 +51,12 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> \tenableInputMemcpy_ = true;\n> \n> \t/* Initialize color lookup tables */\n> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n> -\t\tred_[i] = green_[i] = blue_[i] = i;\n> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\tred_.simple[i] = green_.simple[i] = blue_.simple[i] = i;\n> +\t\tred_.ccm[i].c1 = red_.ccm[i].c2 = red_.ccm[i].c3 = 0;\n> +\t\tgreen_.ccm[i].c1 = green_.ccm[i].c2 = green_.ccm[i].c3 = 0;\n> +\t\tblue_.ccm[i].c1 = blue_.ccm[i].c2 = blue_.ccm[i].c3 = 0;\n> +\t}\n> }\n> \n> DebayerCpu::~DebayerCpu() = default;\n> @@ -61,12 +66,24 @@ DebayerCpu::~DebayerCpu() = default;\n> \tconst pixel_t *curr = (const pixel_t *)src[1] + xShift_; \\\n> \tconst pixel_t *next = (const pixel_t *)src[2] + xShift_;\n> \n> -#define STORE_PIXEL(b, g, r) \\\n> -\t*dst++ = blue_[r]; \\\n> -\t*dst++ = green_[g]; \\\n> -\t*dst++ = red_[b]; \\\n> -\tif constexpr (addAlphaByte) \\\n> -\t\t*dst++ = 255; \\\n> +#define GAMMA(value) \\\n> +\t*dst++ = gammaLut_[std::clamp(value, 0, 1023)]\n\nThis should be 255 instead 1023, correct?\n\n> +\n> +#define STORE_PIXEL(b, g, r) \\\n> +\tif constexpr (ccmEnabled) { \\\n> +\t\tDebayerParams::CcmRow &red = red_.ccm[r]; \\\n> +\t\tDebayerParams::CcmRow &green = green_.ccm[g]; \\\n> +\t\tDebayerParams::CcmRow &blue = blue_.ccm[b]; \\\n> +\t\tGAMMA(red.c3 + green.c3 + blue.c3); \\\n> +\t\tGAMMA(red.c2 + green.c2 + blue.c2); \\\n> +\t\tGAMMA(red.c1 + green.c1 + blue.c1); \\\n> +\t} else { \\\n> +\t\t*dst++ = blue_.simple[b]; \\\n> +\t\t*dst++ = green_.simple[g]; \\\n> +\t\t*dst++ = red_.simple[r]; \\\n> +\t} \\\n> +\tif constexpr (addAlphaByte) \\\n> +\t\t*dst++ = 255; \\\n> \tx++;\n> \n> /*\n> @@ -764,6 +781,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \tgreen_ = params.green;\n> \tred_ = swapRedBlueGains_ ? params.blue : params.red;\n> \tblue_ = swapRedBlueGains_ ? params.red : params.blue;\n> +\tgammaLut_ = params.gammaLut;\n> \n> \t/* Copy metadata from the input buffer */\n> \tFrameMetadata &metadata = output->_d()->metadata();\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index b2ec8f1bd..35c51e4fd 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -137,9 +137,10 @@ private:\n> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> \n> -\tDebayerParams::ColorLookupTable red_;\n> -\tDebayerParams::ColorLookupTable green_;\n> -\tDebayerParams::ColorLookupTable blue_;\n> +\tDebayerParams::LookupTable red_;\n> +\tDebayerParams::LookupTable green_;\n> +\tDebayerParams::LookupTable blue_;\n> +\tDebayerParams::GammaLookupTable gammaLut_;\n> \tdebayerFn debayer0_;\n> \tdebayerFn debayer1_;\n> \tdebayerFn debayer2_;\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index 0e1d3a457..08eca192e 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -79,9 +79,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> \tfor (unsigned int i = 0; i < 256; i++)\n> \t\tgammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5);\n> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> -\t\tdebayerParams_.red[i] = gammaTable[i];\n> -\t\tdebayerParams_.green[i] = gammaTable[i];\n> -\t\tdebayerParams_.blue[i] = gammaTable[i];\n> +\t\tdebayerParams_.red.simple[i] = gammaTable[i];\n> +\t\tdebayerParams_.green.simple[i] = gammaTable[i];\n> +\t\tdebayerParams_.blue.simple[i] = gammaTable[i];\n> \t}\n> \n> \tif (!dmaHeap_.isValid()) {","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 2CFEAC3273\n\tfor ;\n\tSun, 24 Nov 2024 15:37:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2875165FED;\n\tSun, 24 Nov 2024 16:37:41 +0100 (CET)","from sender4-op-o12.zoho.com (sender4-op-o12.zoho.com\n\t[136.143.188.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65B2465FC6\n\tfor ;\n\tSun, 24 Nov 2024 16:37:38 +0100 (CET)","by mx.zohomail.com with SMTPS id 1732462652355780.2655589339699;\n\tSun, 24 Nov 2024 07:37:32 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"O2E0rFoJ\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1732462655; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=STerMxSQMhJGujLYYV1pvxIDE1/cT/ECLqRWhfmz06EaP/lDBwuJccwmrCi2sjIbvIINd/KgEE2GkRmRDEZAiY34rHSLN/hGDGX2y7iBZ56u3qatdrUHl6NLfujvx1HMYgfdQHYWa+pGK0zzL7vwC7ZiA8E2g11xOuAerb+eDXs=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1732462655;\n\th=Content-Type:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=EOFK0xNbbStTUjhyOoQx0qKRizaxKlir46/4zPLlkjA=; \n\tb=bPh46zh6sOEP7rihPESaZtbBPtbsECFLPwkAJe96Cjl2lq06dxUuKz4QCqqGkJr692F4onoWl97LJWgTqqnMHbXtuw0Z004jg2ciFvHgtyklhFwIHAAAXBhTlcAj7/hnAjMVxYKxcZGAKfkFHRTY5XbrxUOiKX46ApGm1XPzyMw=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1732462655;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Content-Type:Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Message-Id:Reply-To:Cc;\n\tbh=EOFK0xNbbStTUjhyOoQx0qKRizaxKlir46/4zPLlkjA=;\n\tb=O2E0rFoJLTZSkuiznl2ZRIMn91NTUpbKhw1nJ/TnmKmtJYtaE60OrpBl4nwiihWu\n\tmwv0OT3DrRrjvTU58YwtrUzJ8DC2B9kqPs/vLvqSiJiBe5qC/n/tmyQIOfy8UO72Ufv\n\tC9tsDaKstP2q0qnXfCaeR69dMoDpvbgZ9qjU/TqE=","Content-Type":"multipart/alternative;\n\tboundary=\"------------9SopqU0b5WWM58rlIrPocXox\"","Message-ID":"<3d3b7d90-20c9-4f2d-9fb6-39abdd0e0cc0@collabora.com>","Date":"Sun, 24 Nov 2024 16:37:30 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 9/9] libcamera: software_isp: Apply CCM in debayering","To":"libcamera-devel@lists.libcamera.org","References":"<20241120180104.1221846-1-mzamazal@redhat.com>\n\t<20241120180104.1221846-10-mzamazal@redhat.com>","Content-Language":"en-US","From":"Robert Mader ","In-Reply-To":"<20241120180104.1221846-10-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":32346,"web_url":"https://patchwork.libcamera.org/comment/32346/","msgid":"","date":"2024-11-24T16:13:16","subject":"Re: [PATCH 9/9] libcamera: software_isp: Apply CCM in debayering","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"There's a red <-> blue color swap when the ccm is active, see below:\n\nOn 20.11.24 19:01, Milan Zamazal wrote:\n> This patch applies color correction matrix (CCM) in debayering if the\n> CCM is specified. Not using CCM must still be supported for performance\n> reasons.\n>\n> The CCM is applied as follows:\n>\n> [r1 r2 r3]\n> [r g b] * [g1 g2 g3]\n> [b1 b2 b3]\n>\n> The CCM matrix (the right side of the multiplication) is constant during\n> single frame processing, while the input pixel (the left side) changes.\n> Because each of the color channels is only 8-bit in software ISP, we can\n> make 9 lookup tables with 256 input values for multiplications of each\n> of the r_i, g_i, b_i values. This way we don't have to multiply each\n> pixel, we can use table lookups and additions instead. Gamma (which is\n> non-linear and thus cannot be a part of the 9 lookup tables values) is\n> applied on the final values rounded to integers using another lookup\n> table.\n>\n> We use int16_t to store the precomputed multiplications. This seems to\n> be noticeably (>10%) faster than `float' for the price of slightly less\n> accuracy and it covers the range of values that sane CCMs produce. The\n> selection and structure of data is performance critical, for example\n> using bytes would add significant (>10%) speedup but would be too short\n> to cover the value range.\n>\n> The color lookup tables are changed to unions to be able to serve as\n> both simple lookup tables or CCM lookup tables. This is arguable but it\n> keeps the code easier to understand if nothing else. It is important to\n> make the unions on the whole lookup tables rather than their values\n> because the latter might cause a noticeable slowdown on simple lookup\n> due to the sheer fact that the table is not compact. Using unions makes\n> the initialization of the tables dubious because it is done before the\n> decision about using CCM is made but the initial values are dubious\n> anyway.\n>\n> The tables are copied (as before), which is not elegant but also not a\n> big problem. There are patches posted that use shared buffers for\n> parameters passing in software ISP (see software ISP TODO #5) and they\n> can be adjusted for the new parameter format.\n>\n> With this patch, the reported per-frame slowdown when applying CCM is\n> about 45% on Debix Model A and about 85% on TI AM69 SK.\n>\n> Signed-off-by: Milan Zamazal\n> ---\n> .../internal/software_isp/debayer_params.h | 20 ++++++-\n> src/ipa/simple/algorithms/lut.cpp | 58 ++++++++++++++-----\n> src/ipa/simple/algorithms/lut.h | 1 +\n> src/libcamera/software_isp/debayer.cpp | 53 ++++++++++++++++-\n> src/libcamera/software_isp/debayer_cpu.cpp | 34 ++++++++---\n> src/libcamera/software_isp/debayer_cpu.h | 7 ++-\n> src/libcamera/software_isp/software_isp.cpp | 6 +-\n> 7 files changed, 145 insertions(+), 34 deletions(-)\n>\n> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n> index 7d8fdd481..531db6968 100644\n> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> @@ -18,11 +18,25 @@ namespace libcamera {\n> struct DebayerParams {\n> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n> \n> +\tstruct CcmRow {\n> +\t\tint16_t c1;\n> +\t\tint16_t c2;\n> +\t\tint16_t c3;\n> +\t};\n> +\n> \tusing ColorLookupTable = std::array;\n> +\tusing CcmLookupTable = std::array;\n> +\tusing GammaLookupTable = std::array;\n> +\n> +\tunion LookupTable {\n> +\t\tColorLookupTable simple;\n> +\t\tCcmLookupTable ccm;\n> +\t};\n> \n> -\tColorLookupTable red;\n> -\tColorLookupTable green;\n> -\tColorLookupTable blue;\n> +\tLookupTable red;\n> +\tLookupTable green;\n> +\tLookupTable blue;\n> +\tGammaLookupTable gammaLut;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> index 8fca96b0a..259976e08 100644\n> --- a/src/ipa/simple/algorithms/lut.cpp\n> +++ b/src/ipa/simple/algorithms/lut.cpp\n> @@ -44,6 +44,11 @@ void Lut::updateGammaTable(IPAContext &context)\n> \tcontext.activeState.gamma.blackLevel = blackLevel;\n> }\n> \n> +int16_t Lut::ccmValue(unsigned int i, float ccm) const\n> +{\n> +\treturn std::round(i * ccm);\n> +}\n> +\n> void Lut::prepare(IPAContext &context,\n> \t\t [[maybe_unused]] const uint32_t frame,\n> \t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> @@ -55,27 +60,48 @@ void Lut::prepare(IPAContext &context,\n> \t * observed, it's not permanently prone to minor fluctuations or\n> \t * rounding errors.\n> \t */\n> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n> +\tconst bool gammaUpdateNeeded =\n> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level;\n> +\tif (gammaUpdateNeeded)\n> \t\tupdateGammaTable(context);\n> \n> \tauto &gains = context.activeState.awb.gains;\n> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n> \tconst unsigned int gammaTableSize = gammaTable.size();\n> -\n> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> -\t\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n> -\t\t\t\t gammaTableSize;\n> -\t\t/* Apply gamma after gain! */\n> -\t\tunsigned int idx;\n> -\t\tidx = std::min({ static_cast(i * gains.red / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->red[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.green / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->green[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.blue / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->blue[i] = gammaTable[idx];\n> +\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n> +\t\t\t gammaTableSize;\n> +\n> +\tif (!context.activeState.ccm.enabled) {\n> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\t\t/* Apply gamma after gain! */\n> +\t\t\tunsigned int idx;\n> +\t\t\tidx = std::min({ static_cast(i * gains.red / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->red.simple[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.green / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->green.simple[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.blue / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->blue.simple[i] = gammaTable[idx];\n> +\t\t}\n> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n> +\t\tauto &ccm = context.activeState.ccm.ccm;\n> +\t\tauto &red = params->red.ccm;\n> +\t\tauto &green = params->green.ccm;\n> +\t\tauto &blue = params->blue.ccm;\n> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\t\tred[i].c1 = ccmValue(i, ccm[0][0]);\n> +\t\t\tred[i].c2 = ccmValue(i, ccm[0][1]);\n> +\t\t\tred[i].c3 = ccmValue(i, ccm[0][2]);\n> +\t\t\tgreen[i].c1 = ccmValue(i, ccm[1][0]);\n> +\t\t\tgreen[i].c2 = ccmValue(i, ccm[1][1]);\n> +\t\t\tgreen[i].c3 = ccmValue(i, ccm[1][2]);\n> +\t\t\tblue[i].c1 = ccmValue(i, ccm[2][0]);\n> +\t\t\tblue[i].c2 = ccmValue(i, ccm[2][1]);\n> +\t\t\tblue[i].c3 = ccmValue(i, ccm[2][2]);\n> +\t\t\tparams->gammaLut[i] = gammaTable[i / div];\n> +\t\t}\n> \t}\n> }\n> \n> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n> index b635987d0..8d50a23fc 100644\n> --- a/src/ipa/simple/algorithms/lut.h\n> +++ b/src/ipa/simple/algorithms/lut.h\n> @@ -27,6 +27,7 @@ public:\n> \n> private:\n> \tvoid updateGammaTable(IPAContext &context);\n> +\tint16_t ccmValue(unsigned int i, float ccm) const;\n> };\n> \n> } /* namespace ipa::soft::algorithms */\n> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n> index f0b832619..97106b22b 100644\n> --- a/src/libcamera/software_isp/debayer.cpp\n> +++ b/src/libcamera/software_isp/debayer.cpp\n> @@ -23,11 +23,56 @@ namespace libcamera {\n> * \\brief Size of a color lookup table\n> */\n> \n> +/**\n> + * \\struct DebayerParams::CcmRow\n> + * \\brief Type of a single row of a color correction matrix\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmRow::c1\n> + * \\brief First column of a CCM row\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmRow::c2\n> + * \\brief Second column of a CCM row\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmRow::c3\n> + * \\brief Third column of a CCM row\n> + */\n> +\n> /**\n> * \\typedef DebayerParams::ColorLookupTable\n> + * \\brief Type of the simple lookup tables for red, green, blue values\n> + */\n> +\n> +/**\n> + * \\typedef DebayerParams::CcmLookupTable\n> + * \\brief Type of the CCM lookup tables for red, green, blue values\n> + */\n> +\n> +/**\n> + * \\typedef DebayerParams::GammaLookupTable\n> + * \\brief Type of the gamma lookup tables for CCM\n> + */\n> +\n> +/**\n> + * \\union DebayerParams::LookupTable\n> * \\brief Type of the lookup tables for red, green, blue values\n> */\n> \n> +/**\n> + * \\var DebayerParams::LookupTable::simple\n> + * \\brief Simple lookup table for red, green, blue values\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::LookupTable::ccm\n> + * \\brief CCM lookup table for red, green, blue values\n> + */\n> +\n> /**\n> * \\var DebayerParams::red\n> * \\brief Lookup table for red color, mapping input values to output values\n> @@ -43,6 +88,11 @@ namespace libcamera {\n> * \\brief Lookup table for blue color, mapping input values to output values\n> */\n> \n> +/**\n> + * \\var DebayerParams::gammaLut\n> + * \\brief Gamma lookup table used with color correction matrix\n> + */\n> +\n> /**\n> * \\class Debayer\n> * \\brief Base debayering class\n> @@ -57,10 +107,11 @@ Debayer::~Debayer()\n> }\n> \n> /**\n> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, ccmEnabled)\n> * \\brief Configure the debayer object according to the passed in parameters\n> * \\param[in] inputCfg The input configuration\n> * \\param[in] outputCfgs The output configurations\n> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n> *\n> * \\return 0 on success, a negative errno on failure\n> */\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 52baf43dc..d8976f183 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -11,6 +11,7 @@\n> \n> #include \"debayer_cpu.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -50,8 +51,12 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> \tenableInputMemcpy_ = true;\n> \n> \t/* Initialize color lookup tables */\n> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n> -\t\tred_[i] = green_[i] = blue_[i] = i;\n> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\tred_.simple[i] = green_.simple[i] = blue_.simple[i] = i;\n> +\t\tred_.ccm[i].c1 = red_.ccm[i].c2 = red_.ccm[i].c3 = 0;\n> +\t\tgreen_.ccm[i].c1 = green_.ccm[i].c2 = green_.ccm[i].c3 = 0;\n> +\t\tblue_.ccm[i].c1 = blue_.ccm[i].c2 = blue_.ccm[i].c3 = 0;\n> +\t}\n> }\n> \n> DebayerCpu::~DebayerCpu() = default;\n> @@ -61,12 +66,24 @@ DebayerCpu::~DebayerCpu() = default;\n> \tconst pixel_t *curr = (const pixel_t *)src[1] + xShift_; \\\n> \tconst pixel_t *next = (const pixel_t *)src[2] + xShift_;\n> \n> -#define STORE_PIXEL(b, g, r) \\\n> -\t*dst++ = blue_[r]; \\\n> -\t*dst++ = green_[g]; \\\n> -\t*dst++ = red_[b]; \\\n> -\tif constexpr (addAlphaByte) \\\n> -\t\t*dst++ = 255; \\\n> +#define GAMMA(value) \\\n> +\t*dst++ = gammaLut_[std::clamp(value, 0, 1023)]\n> +\n> +#define STORE_PIXEL(b, g, r) \\\n> +\tif constexpr (ccmEnabled) { \\\n> +\t\tDebayerParams::CcmRow &red = red_.ccm[r]; \\\n> +\t\tDebayerParams::CcmRow &green = green_.ccm[g]; \\\n> +\t\tDebayerParams::CcmRow &blue = blue_.ccm[b]; \\\n> +\t\tGAMMA(red.c3 + green.c3 + blue.c3); \\\n> +\t\tGAMMA(red.c2 + green.c2 + blue.c2); \\\n> +\t\tGAMMA(red.c1 + green.c1 + blue.c1); \\\n\nThis apparently should be:\n\n+        GAMMA(red.c1 + green.c1 + blue.c1);           \\\n+        GAMMA(red.c2 + green.c2 + blue.c2);           \\\n+        GAMMA(red.c3 + green.c3 + blue.c3);           \\\n\n> +\t} else { \\\n> +\t\t*dst++ = blue_.simple[b]; \\\n> +\t\t*dst++ = green_.simple[g]; \\\n> +\t\t*dst++ = red_.simple[r]; \\\n> +\t} \\\n> +\tif constexpr (addAlphaByte) \\\n> +\t\t*dst++ = 255; \\\n> \tx++;\n> \n> /*\n> @@ -764,6 +781,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \tgreen_ = params.green;\n> \tred_ = swapRedBlueGains_ ? params.blue : params.red;\n> \tblue_ = swapRedBlueGains_ ? params.red : params.blue;\n> +\tgammaLut_ = params.gammaLut;\n> \n> \t/* Copy metadata from the input buffer */\n> \tFrameMetadata &metadata = output->_d()->metadata();\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index b2ec8f1bd..35c51e4fd 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -137,9 +137,10 @@ private:\n> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> \n> -\tDebayerParams::ColorLookupTable red_;\n> -\tDebayerParams::ColorLookupTable green_;\n> -\tDebayerParams::ColorLookupTable blue_;\n> +\tDebayerParams::LookupTable red_;\n> +\tDebayerParams::LookupTable green_;\n> +\tDebayerParams::LookupTable blue_;\n> +\tDebayerParams::GammaLookupTable gammaLut_;\n> \tdebayerFn debayer0_;\n> \tdebayerFn debayer1_;\n> \tdebayerFn debayer2_;\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index 0e1d3a457..08eca192e 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -79,9 +79,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> \tfor (unsigned int i = 0; i < 256; i++)\n> \t\tgammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5);\n> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> -\t\tdebayerParams_.red[i] = gammaTable[i];\n> -\t\tdebayerParams_.green[i] = gammaTable[i];\n> -\t\tdebayerParams_.blue[i] = gammaTable[i];\n> +\t\tdebayerParams_.red.simple[i] = gammaTable[i];\n> +\t\tdebayerParams_.green.simple[i] = gammaTable[i];\n> +\t\tdebayerParams_.blue.simple[i] = gammaTable[i];\n> \t}\n> \n> \tif (!dmaHeap_.isValid()) {","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 B572AC3273\n\tfor ;\n\tSun, 24 Nov 2024 16:13:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E94A365FF5;\n\tSun, 24 Nov 2024 17:13:23 +0100 (CET)","from sender4-op-o12.zoho.com (sender4-op-o12.zoho.com\n\t[136.143.188.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E70C665FC6\n\tfor ;\n\tSun, 24 Nov 2024 17:13:21 +0100 (CET)","by mx.zohomail.com with SMTPS id 1732464798982224.65690692377984; \n\tSun, 24 Nov 2024 08:13:18 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"BT9DP8ce\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1732464799; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=C9Xh0iv50pgGnvscuYMMVBbdPyUkh29KUu9eWjE/p0WAY2ZCyYn1Mhp/f+PGCHpDvNd6coGDlkiAf7CJeVNIand2a9FdZTStQdoTAphKwHH0WwwjQLymFSSNSjwmhdRNdc5PSHXms/5LgupA4aPesnONBeBOr6o1ukvZH+mP9oY=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1732464799;\n\th=Content-Type:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=L4SgOP4IRVRJLQgk5S1iJCikUlPrjHqkwGhRUgKfijo=; \n\tb=licg3c0rviYqvQyg5mJxVMXGWhYJ+lYPkJMChsjLFhLSqXagkK5NymVIC5P09CoPQ1bkb3yf01OJvIbpXmlSfR8WgmIjCYXqKxWiinKSVPFR/0LCZWOoNLvFQK7ORih/+8sSRrjxtfUL9H3hwKf6ET8J/VJeuLePqob4HgNtwqw=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1732464799;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Content-Type:Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Message-Id:Reply-To:Cc;\n\tbh=L4SgOP4IRVRJLQgk5S1iJCikUlPrjHqkwGhRUgKfijo=;\n\tb=BT9DP8cebwYRQGvBY56BLwtPxqJ0BP1HgP0nTjtbxQFi97sP8YdBML2YwQ1Vq5tB\n\ts8R6kDQONJbRAMhSGDKbVlAUA1jAuXMoJfkVOhZC71BuhOTKgVqKDxf3dhgSM+livfZ\n\tNqXZEfOFnZjAJ0JGxgozOYC7a0SCHMwi8KJMmbrA=","Content-Type":"multipart/alternative;\n\tboundary=\"------------9jDMzgatsK5wyJyQXngc7uZX\"","Message-ID":"","Date":"Sun, 24 Nov 2024 17:13:16 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 9/9] libcamera: software_isp: Apply CCM in debayering","To":"libcamera-devel@lists.libcamera.org","References":"<20241120180104.1221846-1-mzamazal@redhat.com>\n\t<20241120180104.1221846-10-mzamazal@redhat.com>","Content-Language":"en-US","From":"Robert Mader ","In-Reply-To":"<20241120180104.1221846-10-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":32362,"web_url":"https://patchwork.libcamera.org/comment/32362/","msgid":"<87h67v5qjt.fsf@redhat.com>","date":"2024-11-25T14:36:06","subject":"Re: [PATCH 9/9] libcamera: software_isp: Apply CCM in debayering","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Robert,\n\nthank you for testing.\n\nRobert Mader writes:\n\n> Hi, awesome to see this series! I currently giving this a try - found two issues so far:\n>\n> 1. Currently things crash because of the wrong gamma clamp, see below.\n> 2. With that fixed I get a Red<->Blue channel switch. Haven't found the\n> line yet, but it also happens without this patch, so probably\n> happens earlier in the series.\n>\n> On 20.11.24 19:01, Milan Zamazal wrote:\n>> This patch applies color correction matrix (CCM) in debayering if the\n>> CCM is specified. Not using CCM must still be supported for performance\n>> reasons.\n>>\n>> The CCM is applied as follows:\n>>\n>> [r1 r2 r3]\n>> [r g b] * [g1 g2 g3]\n>> [b1 b2 b3]\n>>\n>> The CCM matrix (the right side of the multiplication) is constant during\n>> single frame processing, while the input pixel (the left side) changes.\n>> Because each of the color channels is only 8-bit in software ISP, we can\n>> make 9 lookup tables with 256 input values for multiplications of each\n>> of the r_i, g_i, b_i values. This way we don't have to multiply each\n>> pixel, we can use table lookups and additions instead. Gamma (which is\n>> non-linear and thus cannot be a part of the 9 lookup tables values) is\n>> applied on the final values rounded to integers using another lookup\n>> table.\n>>\n>> We use int16_t to store the precomputed multiplications. This seems to\n>> be noticeably (>10%) faster than `float' for the price of slightly less\n>> accuracy and it covers the range of values that sane CCMs produce. The\n>> selection and structure of data is performance critical, for example\n>> using bytes would add significant (>10%) speedup but would be too short\n>> to cover the value range.\n>>\n>> The color lookup tables are changed to unions to be able to serve as\n>> both simple lookup tables or CCM lookup tables. This is arguable but it\n>> keeps the code easier to understand if nothing else. It is important to\n>> make the unions on the whole lookup tables rather than their values\n>> because the latter might cause a noticeable slowdown on simple lookup\n>> due to the sheer fact that the table is not compact. Using unions makes\n>> the initialization of the tables dubious because it is done before the\n>> decision about using CCM is made but the initial values are dubious\n>> anyway.\n>>\n>> The tables are copied (as before), which is not elegant but also not a\n>> big problem. There are patches posted that use shared buffers for\n>> parameters passing in software ISP (see software ISP TODO #5) and they\n>> can be adjusted for the new parameter format.\n>>\n>> With this patch, the reported per-frame slowdown when applying CCM is\n>> about 45% on Debix Model A and about 85% on TI AM69 SK.\n>>\n>> Signed-off-by: Milan Zamazal\n>> ---\n>> .../internal/software_isp/debayer_params.h | 20 ++++++-\n>> src/ipa/simple/algorithms/lut.cpp | 58 ++++++++++++++-----\n>> src/ipa/simple/algorithms/lut.h | 1 +\n>> src/libcamera/software_isp/debayer.cpp | 53 ++++++++++++++++-\n>> src/libcamera/software_isp/debayer_cpu.cpp | 34 ++++++++---\n>> src/libcamera/software_isp/debayer_cpu.h | 7 ++-\n>> src/libcamera/software_isp/software_isp.cpp | 6 +-\n>> 7 files changed, 145 insertions(+), 34 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n>> index 7d8fdd481..531db6968 100644\n>> --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> @@ -18,11 +18,25 @@ namespace libcamera {\n>> struct DebayerParams {\n>> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> +\tstruct CcmRow {\n>> +\t\tint16_t c1;\n>> +\t\tint16_t c2;\n>> +\t\tint16_t c3;\n>> +\t};\n>> +\n>> \tusing ColorLookupTable = std::array;\n>> +\tusing CcmLookupTable = std::array;\n>> +\tusing GammaLookupTable = std::array;\n>> +\n>> +\tunion LookupTable {\n>> +\t\tColorLookupTable simple;\n>> +\t\tCcmLookupTable ccm;\n>> +\t};\n>> -\tColorLookupTable red;\n>> -\tColorLookupTable green;\n>> -\tColorLookupTable blue;\n>> +\tLookupTable red;\n>> +\tLookupTable green;\n>> +\tLookupTable blue;\n>> +\tGammaLookupTable gammaLut;\n>> };\n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n>> index 8fca96b0a..259976e08 100644\n>> --- a/src/ipa/simple/algorithms/lut.cpp\n>> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> @@ -44,6 +44,11 @@ void Lut::updateGammaTable(IPAContext &context)\n>> \tcontext.activeState.gamma.blackLevel = blackLevel;\n>> }\n>> +int16_t Lut::ccmValue(unsigned int i, float ccm) const\n>> +{\n>> +\treturn std::round(i * ccm);\n>> +}\n>> +\n>> void Lut::prepare(IPAContext &context,\n>> \t\t [[maybe_unused]] const uint32_t frame,\n>> \t\t [[maybe_unused]] IPAFrameContext &frameContext,\n>> @@ -55,27 +60,48 @@ void Lut::prepare(IPAContext &context,\n>> \t * observed, it's not permanently prone to minor fluctuations or\n>> \t * rounding errors.\n>> \t */\n>> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n>> +\tconst bool gammaUpdateNeeded =\n>> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level;\n>> +\tif (gammaUpdateNeeded)\n>> \t\tupdateGammaTable(context);\n>> \tauto &gains = context.activeState.awb.gains;\n>> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n>> \tconst unsigned int gammaTableSize = gammaTable.size();\n>> -\n>> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> -\t\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n>> -\t\t\t\t gammaTableSize;\n>> -\t\t/* Apply gamma after gain! */\n>> -\t\tunsigned int idx;\n>> -\t\tidx = std::min({ static_cast(i * gains.red / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->red[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.green / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->green[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.blue / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->blue[i] = gammaTable[idx];\n>> +\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n>> +\t\t\t gammaTableSize;\n>> +\n>> +\tif (!context.activeState.ccm.enabled) {\n>> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\t\t/* Apply gamma after gain! */\n>> +\t\t\tunsigned int idx;\n>> +\t\t\tidx = std::min({ static_cast(i * gains.red / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->red.simple[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.green / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->green.simple[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.blue / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->blue.simple[i] = gammaTable[idx];\n>> +\t\t}\n>> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n>> +\t\tauto &ccm = context.activeState.ccm.ccm;\n>> +\t\tauto &red = params->red.ccm;\n>> +\t\tauto &green = params->green.ccm;\n>> +\t\tauto &blue = params->blue.ccm;\n>> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\t\tred[i].c1 = ccmValue(i, ccm[0][0]);\n>> +\t\t\tred[i].c2 = ccmValue(i, ccm[0][1]);\n>> +\t\t\tred[i].c3 = ccmValue(i, ccm[0][2]);\n>> +\t\t\tgreen[i].c1 = ccmValue(i, ccm[1][0]);\n>> +\t\t\tgreen[i].c2 = ccmValue(i, ccm[1][1]);\n>> +\t\t\tgreen[i].c3 = ccmValue(i, ccm[1][2]);\n>> +\t\t\tblue[i].c1 = ccmValue(i, ccm[2][0]);\n>> +\t\t\tblue[i].c2 = ccmValue(i, ccm[2][1]);\n>> +\t\t\tblue[i].c3 = ccmValue(i, ccm[2][2]);\n>> +\t\t\tparams->gammaLut[i] = gammaTable[i / div];\n>> +\t\t}\n>> \t}\n>> }\n>> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n>> index b635987d0..8d50a23fc 100644\n>> --- a/src/ipa/simple/algorithms/lut.h\n>> +++ b/src/ipa/simple/algorithms/lut.h\n>> @@ -27,6 +27,7 @@ public:\n>> private:\n>> \tvoid updateGammaTable(IPAContext &context);\n>> +\tint16_t ccmValue(unsigned int i, float ccm) const;\n>> };\n>> } /* namespace ipa::soft::algorithms */\n>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n>> index f0b832619..97106b22b 100644\n>> --- a/src/libcamera/software_isp/debayer.cpp\n>> +++ b/src/libcamera/software_isp/debayer.cpp\n>> @@ -23,11 +23,56 @@ namespace libcamera {\n>> * \\brief Size of a color lookup table\n>> */\n>> +/**\n>> + * \\struct DebayerParams::CcmRow\n>> + * \\brief Type of a single row of a color correction matrix\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmRow::c1\n>> + * \\brief First column of a CCM row\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmRow::c2\n>> + * \\brief Second column of a CCM row\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmRow::c3\n>> + * \\brief Third column of a CCM row\n>> + */\n>> +\n>> /**\n>> * \\typedef DebayerParams::ColorLookupTable\n>> + * \\brief Type of the simple lookup tables for red, green, blue values\n>> + */\n>> +\n>> +/**\n>> + * \\typedef DebayerParams::CcmLookupTable\n>> + * \\brief Type of the CCM lookup tables for red, green, blue values\n>> + */\n>> +\n>> +/**\n>> + * \\typedef DebayerParams::GammaLookupTable\n>> + * \\brief Type of the gamma lookup tables for CCM\n>> + */\n>> +\n>> +/**\n>> + * \\union DebayerParams::LookupTable\n>> * \\brief Type of the lookup tables for red, green, blue values\n>> */\n>> +/**\n>> + * \\var DebayerParams::LookupTable::simple\n>> + * \\brief Simple lookup table for red, green, blue values\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::LookupTable::ccm\n>> + * \\brief CCM lookup table for red, green, blue values\n>> + */\n>> +\n>> /**\n>> * \\var DebayerParams::red\n>> * \\brief Lookup table for red color, mapping input values to output values\n>> @@ -43,6 +88,11 @@ namespace libcamera {\n>> * \\brief Lookup table for blue color, mapping input values to output values\n>> */\n>> +/**\n>> + * \\var DebayerParams::gammaLut\n>> + * \\brief Gamma lookup table used with color correction matrix\n>> + */\n>> +\n>> /**\n>> * \\class Debayer\n>> * \\brief Base debayering class\n>> @@ -57,10 +107,11 @@ Debayer::~Debayer()\n>> }\n>> /**\n>> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n>> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, ccmEnabled)\n>> * \\brief Configure the debayer object according to the passed in parameters\n>> * \\param[in] inputCfg The input configuration\n>> * \\param[in] outputCfgs The output configurations\n>> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n>> *\n>> * \\return 0 on success, a negative errno on failure\n>> */\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 52baf43dc..d8976f183 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -11,6 +11,7 @@\n>> #include \"debayer_cpu.h\"\n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -50,8 +51,12 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> \tenableInputMemcpy_ = true;\n>> \t/* Initialize color lookup tables */\n>> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n>> -\t\tred_[i] = green_[i] = blue_[i] = i;\n>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\tred_.simple[i] = green_.simple[i] = blue_.simple[i] = i;\n>> +\t\tred_.ccm[i].c1 = red_.ccm[i].c2 = red_.ccm[i].c3 = 0;\n>> +\t\tgreen_.ccm[i].c1 = green_.ccm[i].c2 = green_.ccm[i].c3 = 0;\n>> +\t\tblue_.ccm[i].c1 = blue_.ccm[i].c2 = blue_.ccm[i].c3 = 0;\n>> +\t}\n>> }\n>> DebayerCpu::~DebayerCpu() = default;\n>> @@ -61,12 +66,24 @@ DebayerCpu::~DebayerCpu() = default;\n>> \tconst pixel_t *curr = (const pixel_t *)src[1] + xShift_; \\\n>> \tconst pixel_t *next = (const pixel_t *)src[2] + xShift_;\n>> -#define STORE_PIXEL(b, g, r) \\\n>> -\t*dst++ = blue_[r]; \\\n>> -\t*dst++ = green_[g]; \\\n>> -\t*dst++ = red_[b]; \\\n>> -\tif constexpr (addAlphaByte) \\\n>> -\t\t*dst++ = 255; \\\n>> +#define GAMMA(value) \\\n>> +\t*dst++ = gammaLut_[std::clamp(value, 0, 1023)]\n>\n> This should be 255 instead 1023, correct?\n\nIndeed, thank you for finding this.\n\nI'll change the upper boundary to\n\n static_cast(gammaLut_.size()) - 1\n\nin v2.\n\n>> +\n>> +#define STORE_PIXEL(b, g, r) \\\n>> +\tif constexpr (ccmEnabled) { \\\n>> +\t\tDebayerParams::CcmRow &red = red_.ccm[r]; \\\n>> +\t\tDebayerParams::CcmRow &green = green_.ccm[g]; \\\n>> +\t\tDebayerParams::CcmRow &blue = blue_.ccm[b]; \\\n>> +\t\tGAMMA(red.c3 + green.c3 + blue.c3); \\\n>> +\t\tGAMMA(red.c2 + green.c2 + blue.c2); \\\n>> +\t\tGAMMA(red.c1 + green.c1 + blue.c1); \\\n>> +\t} else { \\\n>> +\t\t*dst++ = blue_.simple[b]; \\\n>> +\t\t*dst++ = green_.simple[g]; \\\n>> +\t\t*dst++ = red_.simple[r]; \\\n>> +\t} \\\n>> +\tif constexpr (addAlphaByte) \\\n>> +\t\t*dst++ = 255; \\\n>> \tx++;\n>> /*\n>> @@ -764,6 +781,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>> \tgreen_ = params.green;\n>> \tred_ = swapRedBlueGains_ ? params.blue : params.red;\n>> \tblue_ = swapRedBlueGains_ ? params.red : params.blue;\n>> +\tgammaLut_ = params.gammaLut;\n>> \t/* Copy metadata from the input buffer */\n>> \tFrameMetadata &metadata = output->_d()->metadata();\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n>> index b2ec8f1bd..35c51e4fd 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> @@ -137,9 +137,10 @@ private:\n>> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n>> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n>> -\tDebayerParams::ColorLookupTable red_;\n>> -\tDebayerParams::ColorLookupTable green_;\n>> -\tDebayerParams::ColorLookupTable blue_;\n>> +\tDebayerParams::LookupTable red_;\n>> +\tDebayerParams::LookupTable green_;\n>> +\tDebayerParams::LookupTable blue_;\n>> +\tDebayerParams::GammaLookupTable gammaLut_;\n>> \tdebayerFn debayer0_;\n>> \tdebayerFn debayer1_;\n>> \tdebayerFn debayer2_;\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index 0e1d3a457..08eca192e 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -79,9 +79,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>> \tfor (unsigned int i = 0; i < 256; i++)\n>> \t\tgammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5);\n>> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> -\t\tdebayerParams_.red[i] = gammaTable[i];\n>> -\t\tdebayerParams_.green[i] = gammaTable[i];\n>> -\t\tdebayerParams_.blue[i] = gammaTable[i];\n>> +\t\tdebayerParams_.red.simple[i] = gammaTable[i];\n>> +\t\tdebayerParams_.green.simple[i] = gammaTable[i];\n>> +\t\tdebayerParams_.blue.simple[i] = gammaTable[i];\n>> \t}\n>> \tif (!dmaHeap_.isValid()) {\n\nRobert Mader writes:\n\n> There's a red <-> blue color swap when the ccm is active, see below:\n>\n> On 20.11.24 19:01, Milan Zamazal wrote:\n>> This patch applies color correction matrix (CCM) in debayering if the\n>> CCM is specified. Not using CCM must still be supported for performance\n>> reasons.\n>>\n>> The CCM is applied as follows:\n>>\n>> [r1 r2 r3]\n>> [r g b] * [g1 g2 g3]\n>> [b1 b2 b3]\n>>\n>> The CCM matrix (the right side of the multiplication) is constant during\n>> single frame processing, while the input pixel (the left side) changes.\n>> Because each of the color channels is only 8-bit in software ISP, we can\n>> make 9 lookup tables with 256 input values for multiplications of each\n>> of the r_i, g_i, b_i values. This way we don't have to multiply each\n>> pixel, we can use table lookups and additions instead. Gamma (which is\n>> non-linear and thus cannot be a part of the 9 lookup tables values) is\n>> applied on the final values rounded to integers using another lookup\n>> table.\n>>\n>> We use int16_t to store the precomputed multiplications. This seems to\n>> be noticeably (>10%) faster than `float' for the price of slightly less\n>> accuracy and it covers the range of values that sane CCMs produce. The\n>> selection and structure of data is performance critical, for example\n>> using bytes would add significant (>10%) speedup but would be too short\n>> to cover the value range.\n>>\n>> The color lookup tables are changed to unions to be able to serve as\n>> both simple lookup tables or CCM lookup tables. This is arguable but it\n>> keeps the code easier to understand if nothing else. It is important to\n>> make the unions on the whole lookup tables rather than their values\n>> because the latter might cause a noticeable slowdown on simple lookup\n>> due to the sheer fact that the table is not compact. Using unions makes\n>> the initialization of the tables dubious because it is done before the\n>> decision about using CCM is made but the initial values are dubious\n>> anyway.\n>>\n>> The tables are copied (as before), which is not elegant but also not a\n>> big problem. There are patches posted that use shared buffers for\n>> parameters passing in software ISP (see software ISP TODO #5) and they\n>> can be adjusted for the new parameter format.\n>>\n>> With this patch, the reported per-frame slowdown when applying CCM is\n>> about 45% on Debix Model A and about 85% on TI AM69 SK.\n>>\n>> Signed-off-by: Milan Zamazal\n>> ---\n>> .../internal/software_isp/debayer_params.h | 20 ++++++-\n>> src/ipa/simple/algorithms/lut.cpp | 58 ++++++++++++++-----\n>> src/ipa/simple/algorithms/lut.h | 1 +\n>> src/libcamera/software_isp/debayer.cpp | 53 ++++++++++++++++-\n>> src/libcamera/software_isp/debayer_cpu.cpp | 34 ++++++++---\n>> src/libcamera/software_isp/debayer_cpu.h | 7 ++-\n>> src/libcamera/software_isp/software_isp.cpp | 6 +-\n>> 7 files changed, 145 insertions(+), 34 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n>> index 7d8fdd481..531db6968 100644\n>> --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> @@ -18,11 +18,25 @@ namespace libcamera {\n>> struct DebayerParams {\n>> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> +\tstruct CcmRow {\n>> +\t\tint16_t c1;\n>> +\t\tint16_t c2;\n>> +\t\tint16_t c3;\n>> +\t};\n>> +\n>> \tusing ColorLookupTable = std::array;\n>> +\tusing CcmLookupTable = std::array;\n>> +\tusing GammaLookupTable = std::array;\n>> +\n>> +\tunion LookupTable {\n>> +\t\tColorLookupTable simple;\n>> +\t\tCcmLookupTable ccm;\n>> +\t};\n>> -\tColorLookupTable red;\n>> -\tColorLookupTable green;\n>> -\tColorLookupTable blue;\n>> +\tLookupTable red;\n>> +\tLookupTable green;\n>> +\tLookupTable blue;\n>> +\tGammaLookupTable gammaLut;\n>> };\n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n>> index 8fca96b0a..259976e08 100644\n>> --- a/src/ipa/simple/algorithms/lut.cpp\n>> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> @@ -44,6 +44,11 @@ void Lut::updateGammaTable(IPAContext &context)\n>> \tcontext.activeState.gamma.blackLevel = blackLevel;\n>> }\n>> +int16_t Lut::ccmValue(unsigned int i, float ccm) const\n>> +{\n>> +\treturn std::round(i * ccm);\n>> +}\n>> +\n>> void Lut::prepare(IPAContext &context,\n>> \t\t [[maybe_unused]] const uint32_t frame,\n>> \t\t [[maybe_unused]] IPAFrameContext &frameContext,\n>> @@ -55,27 +60,48 @@ void Lut::prepare(IPAContext &context,\n>> \t * observed, it's not permanently prone to minor fluctuations or\n>> \t * rounding errors.\n>> \t */\n>> -\tif (context.activeState.gamma.blackLevel != context.activeState.blc.level)\n>> +\tconst bool gammaUpdateNeeded =\n>> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level;\n>> +\tif (gammaUpdateNeeded)\n>> \t\tupdateGammaTable(context);\n>> \tauto &gains = context.activeState.awb.gains;\n>> \tauto &gammaTable = context.activeState.gamma.gammaTable;\n>> \tconst unsigned int gammaTableSize = gammaTable.size();\n>> -\n>> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> -\t\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n>> -\t\t\t\t gammaTableSize;\n>> -\t\t/* Apply gamma after gain! */\n>> -\t\tunsigned int idx;\n>> -\t\tidx = std::min({ static_cast(i * gains.red / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->red[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.green / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->green[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.blue / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->blue[i] = gammaTable[idx];\n>> +\tconst double div = static_cast(DebayerParams::kRGBLookupSize) /\n>> +\t\t\t gammaTableSize;\n>> +\n>> +\tif (!context.activeState.ccm.enabled) {\n>> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\t\t/* Apply gamma after gain! */\n>> +\t\t\tunsigned int idx;\n>> +\t\t\tidx = std::min({ static_cast(i * gains.red / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->red.simple[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.green / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->green.simple[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.blue / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->blue.simple[i] = gammaTable[idx];\n>> +\t\t}\n>> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n>> +\t\tauto &ccm = context.activeState.ccm.ccm;\n>> +\t\tauto &red = params->red.ccm;\n>> +\t\tauto &green = params->green.ccm;\n>> +\t\tauto &blue = params->blue.ccm;\n>> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\t\tred[i].c1 = ccmValue(i, ccm[0][0]);\n>> +\t\t\tred[i].c2 = ccmValue(i, ccm[0][1]);\n>> +\t\t\tred[i].c3 = ccmValue(i, ccm[0][2]);\n>> +\t\t\tgreen[i].c1 = ccmValue(i, ccm[1][0]);\n>> +\t\t\tgreen[i].c2 = ccmValue(i, ccm[1][1]);\n>> +\t\t\tgreen[i].c3 = ccmValue(i, ccm[1][2]);\n>> +\t\t\tblue[i].c1 = ccmValue(i, ccm[2][0]);\n>> +\t\t\tblue[i].c2 = ccmValue(i, ccm[2][1]);\n>> +\t\t\tblue[i].c3 = ccmValue(i, ccm[2][2]);\n>> +\t\t\tparams->gammaLut[i] = gammaTable[i / div];\n>> +\t\t}\n>> \t}\n>> }\n>> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h\n>> index b635987d0..8d50a23fc 100644\n>> --- a/src/ipa/simple/algorithms/lut.h\n>> +++ b/src/ipa/simple/algorithms/lut.h\n>> @@ -27,6 +27,7 @@ public:\n>> private:\n>> \tvoid updateGammaTable(IPAContext &context);\n>> +\tint16_t ccmValue(unsigned int i, float ccm) const;\n>> };\n>> } /* namespace ipa::soft::algorithms */\n>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n>> index f0b832619..97106b22b 100644\n>> --- a/src/libcamera/software_isp/debayer.cpp\n>> +++ b/src/libcamera/software_isp/debayer.cpp\n>> @@ -23,11 +23,56 @@ namespace libcamera {\n>> * \\brief Size of a color lookup table\n>> */\n>> +/**\n>> + * \\struct DebayerParams::CcmRow\n>> + * \\brief Type of a single row of a color correction matrix\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmRow::c1\n>> + * \\brief First column of a CCM row\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmRow::c2\n>> + * \\brief Second column of a CCM row\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmRow::c3\n>> + * \\brief Third column of a CCM row\n>> + */\n>> +\n>> /**\n>> * \\typedef DebayerParams::ColorLookupTable\n>> + * \\brief Type of the simple lookup tables for red, green, blue values\n>> + */\n>> +\n>> +/**\n>> + * \\typedef DebayerParams::CcmLookupTable\n>> + * \\brief Type of the CCM lookup tables for red, green, blue values\n>> + */\n>> +\n>> +/**\n>> + * \\typedef DebayerParams::GammaLookupTable\n>> + * \\brief Type of the gamma lookup tables for CCM\n>> + */\n>> +\n>> +/**\n>> + * \\union DebayerParams::LookupTable\n>> * \\brief Type of the lookup tables for red, green, blue values\n>> */\n>> +/**\n>> + * \\var DebayerParams::LookupTable::simple\n>> + * \\brief Simple lookup table for red, green, blue values\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::LookupTable::ccm\n>> + * \\brief CCM lookup table for red, green, blue values\n>> + */\n>> +\n>> /**\n>> * \\var DebayerParams::red\n>> * \\brief Lookup table for red color, mapping input values to output values\n>> @@ -43,6 +88,11 @@ namespace libcamera {\n>> * \\brief Lookup table for blue color, mapping input values to output values\n>> */\n>> +/**\n>> + * \\var DebayerParams::gammaLut\n>> + * \\brief Gamma lookup table used with color correction matrix\n>> + */\n>> +\n>> /**\n>> * \\class Debayer\n>> * \\brief Base debayering class\n>> @@ -57,10 +107,11 @@ Debayer::~Debayer()\n>> }\n>> /**\n>> - * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs)\n>> + * \\fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector> &outputCfgs, ccmEnabled)\n>> * \\brief Configure the debayer object according to the passed in parameters\n>> * \\param[in] inputCfg The input configuration\n>> * \\param[in] outputCfgs The output configurations\n>> + * \\param[in] ccmEnabled Whether a color correction matrix is applied\n>> *\n>> * \\return 0 on success, a negative errno on failure\n>> */\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 52baf43dc..d8976f183 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -11,6 +11,7 @@\n>> #include \"debayer_cpu.h\"\n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -50,8 +51,12 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> \tenableInputMemcpy_ = true;\n>> \t/* Initialize color lookup tables */\n>> -\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n>> -\t\tred_[i] = green_[i] = blue_[i] = i;\n>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\tred_.simple[i] = green_.simple[i] = blue_.simple[i] = i;\n>> +\t\tred_.ccm[i].c1 = red_.ccm[i].c2 = red_.ccm[i].c3 = 0;\n>> +\t\tgreen_.ccm[i].c1 = green_.ccm[i].c2 = green_.ccm[i].c3 = 0;\n>> +\t\tblue_.ccm[i].c1 = blue_.ccm[i].c2 = blue_.ccm[i].c3 = 0;\n>> +\t}\n>> }\n>> DebayerCpu::~DebayerCpu() = default;\n>> @@ -61,12 +66,24 @@ DebayerCpu::~DebayerCpu() = default;\n>> \tconst pixel_t *curr = (const pixel_t *)src[1] + xShift_; \\\n>> \tconst pixel_t *next = (const pixel_t *)src[2] + xShift_;\n>> -#define STORE_PIXEL(b, g, r) \\\n>> -\t*dst++ = blue_[r]; \\\n>> -\t*dst++ = green_[g]; \\\n>> -\t*dst++ = red_[b]; \\\n>> -\tif constexpr (addAlphaByte) \\\n>> -\t\t*dst++ = 255; \\\n>> +#define GAMMA(value) \\\n>> +\t*dst++ = gammaLut_[std::clamp(value, 0, 1023)]\n>> +\n>> +#define STORE_PIXEL(b, g, r) \\\n>> +\tif constexpr (ccmEnabled) { \\\n>> +\t\tDebayerParams::CcmRow &red = red_.ccm[r]; \\\n>> +\t\tDebayerParams::CcmRow &green = green_.ccm[g]; \\\n>> +\t\tDebayerParams::CcmRow &blue = blue_.ccm[b]; \\\n>> +\t\tGAMMA(red.c3 + green.c3 + blue.c3); \\\n>> +\t\tGAMMA(red.c2 + green.c2 + blue.c2); \\\n>> +\t\tGAMMA(red.c1 + green.c1 + blue.c1); \\\n>\n> This apparently should be:\n>\n> +        GAMMA(red.c1 + green.c1 + blue.c1);           \\\n> +        GAMMA(red.c2 + green.c2 + blue.c2);           \\\n> +        GAMMA(red.c3 + green.c3 + blue.c3);           \\\n\nNot exactly. If red is red and blue is blue then the order is correct\nbut the case where \"red\" and \"blue\" are swapped (swapRedBlueGains_) is\nhandled incorrectly for the CCM case. I'll fix it in v2 too. Thank you\nfor catching this.\n\n>> +\t} else { \\\n>> +\t\t*dst++ = blue_.simple[b]; \\\n>> +\t\t*dst++ = green_.simple[g]; \\\n>> +\t\t*dst++ = red_.simple[r]; \\\n>> +\t} \\\n>> +\tif constexpr (addAlphaByte) \\\n>> +\t\t*dst++ = 255; \\\n>> \tx++;\n>> /*\n>> @@ -764,6 +781,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>> \tgreen_ = params.green;\n>> \tred_ = swapRedBlueGains_ ? params.blue : params.red;\n>> \tblue_ = swapRedBlueGains_ ? params.red : params.blue;\n>> +\tgammaLut_ = params.gammaLut;\n>> \t/* Copy metadata from the input buffer */\n>> \tFrameMetadata &metadata = output->_d()->metadata();\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n>> index b2ec8f1bd..35c51e4fd 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> @@ -137,9 +137,10 @@ private:\n>> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n>> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n>> -\tDebayerParams::ColorLookupTable red_;\n>> -\tDebayerParams::ColorLookupTable green_;\n>> -\tDebayerParams::ColorLookupTable blue_;\n>> +\tDebayerParams::LookupTable red_;\n>> +\tDebayerParams::LookupTable green_;\n>> +\tDebayerParams::LookupTable blue_;\n>> +\tDebayerParams::GammaLookupTable gammaLut_;\n>> \tdebayerFn debayer0_;\n>> \tdebayerFn debayer1_;\n>> \tdebayerFn debayer2_;\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index 0e1d3a457..08eca192e 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -79,9 +79,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>> \tfor (unsigned int i = 0; i < 256; i++)\n>> \t\tgammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5);\n>> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> -\t\tdebayerParams_.red[i] = gammaTable[i];\n>> -\t\tdebayerParams_.green[i] = gammaTable[i];\n>> -\t\tdebayerParams_.blue[i] = gammaTable[i];\n>> +\t\tdebayerParams_.red.simple[i] = gammaTable[i];\n>> +\t\tdebayerParams_.green.simple[i] = gammaTable[i];\n>> +\t\tdebayerParams_.blue.simple[i] = gammaTable[i];\n>> \t}\n>> \tif (!dmaHeap_.isValid()) {","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 7E1BEC32A3\n\tfor ;\n\tMon, 25 Nov 2024 14:36:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF16165F5A;\n\tMon, 25 Nov 2024 15:36:15 +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 6521765F53\n\tfor ;\n\tMon, 25 Nov 2024 15:36:13 +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-693-xENK4XhfMqSr0IBRDxHREg-1; Mon, 25 Nov 2024 09:36:10 -0500","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-aa529e707f4so182206266b.0\n\tfor ;\n\tMon, 25 Nov 2024 06:36:10 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-aa53a146fd0sm287684966b.143.2024.11.25.06.36.06\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 25 Nov 2024 06:36:07 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"GxjwrZe9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1732545372;\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=Ymmk04njH63+m8LbDVWPOuKZ19if2N1fEjWFIXSN5Q0=;\n\tb=GxjwrZe9cOCwGVthm6+GvWTMqtYaWndJJfnfgKL99ABfysNqjCB1CJFhQ0j3hsb2yz//N9\n\tK+KiR1rQSzgMx6Fl0gsHpoe0tJB/npop2z9Et5Oci9M5ijB72b50hNG2l+Y/bNJJmR/EgL\n\tbyUehebcTzMNkJS7LAjs6HNa9ZMVRg4=","X-MC-Unique":"xENK4XhfMqSr0IBRDxHREg-1","X-Mimecast-MFC-AGG-ID":"xENK4XhfMqSr0IBRDxHREg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732545369; x=1733150169;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=aS5kYHZ8IwpFzAEAO1teMvYcKQFBzE02qvxoPvrX4UU=;\n\tb=s0InIGma3YOp8fJWv2YXEdR3jsScIfatSo9/Am0pOc/HFt0qJtCbpkg9qXqi9lIBb5\n\ttgOhcX4Tpx+QYgJfLn8vwOOMm+T+zIXrtY4aUCO+WhYbYwucLQ/XyfrGgkfBCHvgXN/w\n\tVgmoS5hKIVhkKGk94abobDwE7YBo7L6adplKdRrFK7TJZLK8nTpctHBGDMGjxxeMNiRr\n\t5iXAgaJZ/QwZhBGJK36ATnTIT6Ws98wIZ4E1WCZFtST/OeATC9nkHLdxnFtpB2+Khf7q\n\tJluMSOUp78z3fOPsP8PNtlQBh2FUHFwQhw6EjVzq6/YtVQvbBjPbEuSWoyIE/rrVzFSt\n\tYy8Q==","X-Gm-Message-State":"AOJu0YxfoHEVGXEpu7eDE+wzv6I7XAXUhpvp87vSYabLAOYURfRkuvQE\n\t+hV0DV+aTqrPKsgFTOs5yVgDvyp3KP0uP3Hwm7h1c5Rtt6PyDrlH5V8bLUFW8zvVefwDSv4mBg6\n\tsL8PYXPc6L23hD3Y/XDjx0pl+3vuJbcbJNVVvEZVQ9A6cI7Y/GUIlCua40o8fdGFPNRL2btos8m\n\tA0n3JFYowPJpDBgbE2wqzoUfqxMUZbpiM0h2CuP8tTRtWVn1RXdnM097w=","X-Gm-Gg":"ASbGncuqzMLkcTu6DWTdtr2WRHchs2M4HQLfIsI+jYdfAGYrtSxOOZWcZhtOkdOx6qp\n\tuudyf5mL8svGWhSJL0X05R/Cwvx+rDVo2KqHwvXNWxf8Vtt83PzOSLT5nGT6SJxcdKNrlbjxUjq\n\tY0FSlh7POs7uJAmdxT+MlwYMy2IXvMOEYmQynoP/hhsqVQASoXbdihyDOlYOgvqVG7mzq37pYmZ\n\t1lxQ2P3JTbIKx5Gw8sBlXNUYikMfWT4b+4MkZi1gQN7gh1Ea5Gu8EstuTcX/qUXpudzUOc=","X-Received":["by 2002:a17:907:77c3:b0:aa5:459c:bef2 with SMTP id\n\ta640c23a62f3a-aa5459cbf9dmr533686966b.30.1732545368539; \n\tMon, 25 Nov 2024 06:36:08 -0800 (PST)","by 2002:a17:907:77c3:b0:aa5:459c:bef2 with SMTP id\n\ta640c23a62f3a-aa5459cbf9dmr533682666b.30.1732545367855; \n\tMon, 25 Nov 2024 06:36:07 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IF3J455dV0ZV7dfd+47VNqJAsZofpMj6XVk0Xq1aPnez01ldTdUJyORSsPGQffg50ohruE1fw==","From":"Milan Zamazal ","To":"Robert Mader ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 9/9] libcamera: software_isp: Apply CCM in debayering","In-Reply-To":"<3d3b7d90-20c9-4f2d-9fb6-39abdd0e0cc0@collabora.com> (Robert\n\tMader's message of \"Sun, 24 Nov 2024 16:37:30 +0100\")","References":"<20241120180104.1221846-1-mzamazal@redhat.com>\n\t<20241120180104.1221846-10-mzamazal@redhat.com>\n\t<3d3b7d90-20c9-4f2d-9fb6-39abdd0e0cc0@collabora.com>","Date":"Mon, 25 Nov 2024 15:36:06 +0100","Message-ID":"<87h67v5qjt.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"42Xprl5bdjQPjrg7oLmALYh4IBcS8zRp5dUaiKe2Vu0_1732545369","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}}]