[{"id":33249,"web_url":"https://patchwork.libcamera.org/comment/33249/","msgid":"<20250203013122.GM12673@pendragon.ideasonboard.com>","date":"2025-02-03T01:31:22","subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Thu, Jan 30, 2025 at 07:14:47PM +0100, 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 can be represented either as unions,\n> accommodating tables for both the CCM and non-CCM cases, or as separate\n> tables for each of the cases, leaving the tables for the other case\n> unused. The latter is selected as a matter of preference.\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> Color gains from white balance are supposed not to be a part of the\n> specified CCM. They are applied on it using matrix multiplication,\n> which is simple and in correspondence with future additions in the form\n> of matrix multiplication, like saturation adjustment.\n> \n> With this patch, the reported per-frame slowdown when applying CCM is\n> about 45% on Debix Model A and about 75% on TI AM69 SK.\n> \n> Using std::clamp in debayering adds some performance penalty (a few\n> percent). The clamping is necessary to eliminate out of range values\n> possibly produced by the CCM. If it could be avoided by adjusting the\n> precomputed tables some way then performance could be improved a bit.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> .../internal/software_isp/debayer_params.h | 15 ++++-\n> src/ipa/simple/algorithms/lut.cpp | 65 ++++++++++++++-----\n> src/ipa/simple/algorithms/lut.h | 1 +\n> src/libcamera/software_isp/debayer.cpp | 52 ++++++++++++++-\n> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++--\n> src/libcamera/software_isp/debayer_cpu.h | 4 ++\n> 6 files changed, 145 insertions(+), 27 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 7d8fdd48..9a7dbe93 100644\n> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2023, 2024 Red Hat Inc.\n> + * Copyright (C) 2023-2025 Red Hat Inc.\n> *\n> * Authors:\n> * Hans de Goede \n> @@ -18,11 +18,24 @@ namespace libcamera {\n> struct DebayerParams {\n> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n> \n> +\tstruct CcmColumn {\n> +\t\tint16_t r;\n> +\t\tint16_t g;\n> +\t\tint16_t b;\n> +\t};\n> +\n> \tusing ColorLookupTable = std::array;\n> +\tusing CcmLookupTable = std::array;\n> +\tusing GammaLookupTable = std::array;\n> \n> \tColorLookupTable red;\n> \tColorLookupTable green;\n> \tColorLookupTable blue;\n> +\n> +\tCcmLookupTable redCcm;\n> +\tCcmLookupTable greenCcm;\n> +\tCcmLookupTable blueCcm;\n> +\tGammaLookupTable gammaLut;\n\nGiven that the ColorLookupTable and GammaLookupTable types are the same,\nwould it make sense to save a bit of memory by combining the combining\nthe colour and gamma tables ? I'm thinking about something like\n\n\tstruct ColorLookupTables {\n\t\tCcmLookupTable ccm;\n\t\tGammaLookupTable gamma;\n\t}\n\n\tColorLookupTables red;\n\tColorLookupTables green;\n\tColorLookupTables blue;\n\nThe three gamma tables would be identical when CCM is disabled. If that\ncauses performance issues (possibly because the three identical gamma\ntables would take more cache space), then we can keep them separate. I'm\ntempted to merge the ColorLookupTable and GammaLookupTable into a single\ntype.\n\nYou should also document somewhere which tables are used in which case\n( when CCM is disabled, combining gains and gamma, and Ccm\nand gammaLut when CCM is enabled, with Ccm covering CCM and\ngains, and gammaLut covering gamma). I think this would be easier to\ndocument with the ColorLookupTables type, as you can then document what\nthe ccm and gamma fields cover.\n\nEither way, with this addressed,\n\nReviewed-by: Laurent Pinchart \n\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> index e5e995c7..7719aecc 100644\n> --- a/src/ipa/simple/algorithms/lut.cpp\n> +++ b/src/ipa/simple/algorithms/lut.cpp\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2024, Red Hat Inc.\n> + * Copyright (C) 2024-2025, Red Hat Inc.\n> *\n> * Color lookup tables construction\n> */\n> @@ -80,6 +80,11 @@ void Lut::updateGammaTable(IPAContext &context)\n> \tcontext.activeState.gamma.contrast = contrast;\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> @@ -91,28 +96,52 @@ 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> -\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> +\tconst bool gammaUpdateNeeded =\n> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> +\t\tcontext.activeState.gamma.contrast != context.activeState.knobs.contrast;\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.r() / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->red[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->green[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.b() / 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.ccmEnabled) {\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.r() / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->red[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->green[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.b() / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->blue[i] = gammaTable[idx];\n> +\t\t}\n> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n> +\t\tMatrix gainCcm = { { gains.r(), 0, 0,\n> +\t\t\t\t\t\t 0, gains.g(), 0,\n> +\t\t\t\t\t\t 0, 0, gains.b() } };\n> +\t\tauto ccm = gainCcm * context.activeState.ccm.ccm;\n> +\t\tauto &red = params->redCcm;\n> +\t\tauto &green = params->greenCcm;\n> +\t\tauto &blue = params->blueCcm;\n> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\t\tred[i].r = ccmValue(i, ccm[0][0]);\n> +\t\t\tred[i].g = ccmValue(i, ccm[1][0]);\n> +\t\t\tred[i].b = ccmValue(i, ccm[2][0]);\n> +\t\t\tgreen[i].r = ccmValue(i, ccm[0][1]);\n> +\t\t\tgreen[i].g = ccmValue(i, ccm[1][1]);\n> +\t\t\tgreen[i].b = ccmValue(i, ccm[2][1]);\n> +\t\t\tblue[i].r = ccmValue(i, ccm[0][2]);\n> +\t\t\tblue[i].g = ccmValue(i, ccm[1][2]);\n> +\t\t\tblue[i].b = 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 889f864b..77324800 100644\n> --- a/src/ipa/simple/algorithms/lut.h\n> +++ b/src/ipa/simple/algorithms/lut.h\n> @@ -33,6 +33,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 45fe6960..4c84a556 100644\n> --- a/src/libcamera/software_isp/debayer.cpp\n> +++ b/src/libcamera/software_isp/debayer.cpp\n> @@ -23,9 +23,39 @@ namespace libcamera {\n> * \\brief Size of a color lookup table\n> */\n> \n> +/**\n> + * \\struct DebayerParams::CcmColumn\n> + * \\brief Type of a single column of a color correction matrix (CCM)\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmColumn::r\n> + * \\brief Red (first) component of a CCM column\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmColumn::g\n> + * \\brief Green (second) component of a CCM column\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmColumn::b\n> + * \\brief Blue (third) component of a CCM column\n> + */\n> +\n> /**\n> * \\typedef DebayerParams::ColorLookupTable\n> - * \\brief Type of the lookup tables for red, green, blue values\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> @@ -43,6 +73,26 @@ namespace libcamera {\n> * \\brief Lookup table for blue color, mapping input values to output values\n> */\n> \n> +/**\n> + * \\var DebayerParams::redCcm\n> + * \\brief CCM lookup table for red color, mapping input values to output values\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::greenCcm\n> + * \\brief CCM lookup table for green color, mapping input values to output values\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::blueCcm\n> + * \\brief CCM 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> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 0cd03a8f..5ddfdcf6 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> @@ -51,8 +52,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> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> \t\tred_[i] = green_[i] = blue_[i] = i;\n> +\t\tredCcm_[i] = { 1, 0, 0 };\n> +\t\tgreenCcm_[i] = { 0, 1, 0 };\n> +\t\tblueCcm_[i] = { 0, 0, 1 };\n> +\t}\n> }\n> \n> DebayerCpu::~DebayerCpu() = default;\n> @@ -62,12 +67,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_[b]; \\\n> -\t*dst++ = green_[g]; \\\n> -\t*dst++ = red_[r]; \\\n> -\tif constexpr (addAlphaByte) \\\n> -\t\t*dst++ = 255; \\\n> +#define GAMMA(value) \\\n> +\t*dst++ = gammaLut_[std::clamp(value, 0, static_cast(gammaLut_.size()) - 1)]\n> +\n> +#define STORE_PIXEL(b_, g_, r_) \\\n> +\tif constexpr (ccmEnabled) { \\\n> +\t\tconst DebayerParams::CcmColumn &blue = blueCcm_[b_]; \\\n> +\t\tconst DebayerParams::CcmColumn &green = greenCcm_[g_]; \\\n> +\t\tconst DebayerParams::CcmColumn &red = redCcm_[r_]; \\\n> +\t\tGAMMA(blue.r + blue.g + blue.b); \\\n> +\t\tGAMMA(green.r + green.g + green.b); \\\n> +\t\tGAMMA(red.r + red.g + red.b); \\\n> +\t} else { \\\n> +\t\t*dst++ = blue_[b_]; \\\n> +\t\t*dst++ = green_[g_]; \\\n> +\t\t*dst++ = red_[r_]; \\\n> +\t} \\\n> +\tif constexpr (addAlphaByte) \\\n> +\t\t*dst++ = 255; \\\n> \tx++;\n> \n> /*\n> @@ -757,6 +774,10 @@ 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> +\tgreenCcm_ = params.greenCcm;\n> +\tredCcm_ = swapRedBlueGains_ ? params.blueCcm : params.redCcm;\n> +\tblueCcm_ = swapRedBlueGains_ ? params.redCcm : params.blueCcm;\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 21c08a2d..9f73a2fd 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -141,6 +141,10 @@ private:\n> \tDebayerParams::ColorLookupTable red_;\n> \tDebayerParams::ColorLookupTable green_;\n> \tDebayerParams::ColorLookupTable blue_;\n> +\tDebayerParams::CcmLookupTable redCcm_;\n> +\tDebayerParams::CcmLookupTable greenCcm_;\n> +\tDebayerParams::CcmLookupTable blueCcm_;\n> +\tDebayerParams::GammaLookupTable gammaLut_;\n> \tdebayerFn debayer0_;\n> \tdebayerFn debayer1_;\n> \tdebayerFn debayer2_;","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 47466C0F1B\n\tfor ;\n\tMon, 3 Feb 2025 01:31:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E1AB68579;\n\tMon, 3 Feb 2025 02:31:28 +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 62C4261876\n\tfor ;\n\tMon, 3 Feb 2025 02:31:26 +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 0B076497;\n\tMon, 3 Feb 2025 02:30:14 +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=\"t9fOS9lU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738546215;\n\tbh=4udr00bAmLjqekKpIjdP5HR4kY07uwz1CFfD+W4w8Wk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t9fOS9lUGvvIqO7nvNr4q5RqQQxXbLVfKvUSOJR764vV/lRzCl6g2bzDGpEsqzPke\n\tq1styWGlR4BniggvAy2G0BcdYdhfF1euAmUrDJCpwkGcPbIWa6zc5hP3tHqzNRnqqD\n\td2QlcWRv7FAGVn4q8341pDvczxgrXSmgrtBHFLy4=","Date":"Mon, 3 Feb 2025 03:31:22 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tRobert Mader ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","Message-ID":"<20250203013122.GM12673@pendragon.ideasonboard.com>","References":"<20250130181449.130492-1-mzamazal@redhat.com>\n\t<20250130181449.130492-11-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250130181449.130492-11-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":33281,"web_url":"https://patchwork.libcamera.org/comment/33281/","msgid":"<85v7tqvl84.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-02-03T20:15:07","subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Milan Zamazal writes:\n\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\nI think this should actually be:\n\n [r1 g1 b1] [r]\n [r2 g2 b2] * [g]\n [r3 g3 b3] [b]\n\nUnless somebody corrects me, I'll change this in v6.\n\nI was unsure which is the correct version, I couldn't find it documented\nanywhere. I can't remember why I selected the original version\neventually. But the second one produces much better images with the\nmatrices I copied from another pipeline for my sensor and it's also used\nby matrices in RGB<->YUV conversion recipes so I guess that one is\nactually right. Of course, it's just about the format of the input\nmatrices, nothing else changes.\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 can be represented either as unions,\n> accommodating tables for both the CCM and non-CCM cases, or as separate\n> tables for each of the cases, leaving the tables for the other case\n> unused. The latter is selected as a matter of preference.\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> Color gains from white balance are supposed not to be a part of the\n> specified CCM. They are applied on it using matrix multiplication,\n> which is simple and in correspondence with future additions in the form\n> of matrix multiplication, like saturation adjustment.\n>\n> With this patch, the reported per-frame slowdown when applying CCM is\n> about 45% on Debix Model A and about 75% on TI AM69 SK.\n>\n> Using std::clamp in debayering adds some performance penalty (a few\n> percent). The clamping is necessary to eliminate out of range values\n> possibly produced by the CCM. If it could be avoided by adjusting the\n> precomputed tables some way then performance could be improved a bit.\n>\n> Signed-off-by: Milan Zamazal \n> ---\n> .../internal/software_isp/debayer_params.h | 15 ++++-\n> src/ipa/simple/algorithms/lut.cpp | 65 ++++++++++++++-----\n> src/ipa/simple/algorithms/lut.h | 1 +\n> src/libcamera/software_isp/debayer.cpp | 52 ++++++++++++++-\n> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++--\n> src/libcamera/software_isp/debayer_cpu.h | 4 ++\n> 6 files changed, 145 insertions(+), 27 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 7d8fdd48..9a7dbe93 100644\n> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2023, 2024 Red Hat Inc.\n> + * Copyright (C) 2023-2025 Red Hat Inc.\n> *\n> * Authors:\n> * Hans de Goede \n> @@ -18,11 +18,24 @@ namespace libcamera {\n> struct DebayerParams {\n> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n> \n> +\tstruct CcmColumn {\n> +\t\tint16_t r;\n> +\t\tint16_t g;\n> +\t\tint16_t b;\n> +\t};\n> +\n> \tusing ColorLookupTable = std::array;\n> +\tusing CcmLookupTable = std::array;\n> +\tusing GammaLookupTable = std::array;\n> \n> \tColorLookupTable red;\n> \tColorLookupTable green;\n> \tColorLookupTable blue;\n> +\n> +\tCcmLookupTable redCcm;\n> +\tCcmLookupTable greenCcm;\n> +\tCcmLookupTable blueCcm;\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 e5e995c7..7719aecc 100644\n> --- a/src/ipa/simple/algorithms/lut.cpp\n> +++ b/src/ipa/simple/algorithms/lut.cpp\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2024, Red Hat Inc.\n> + * Copyright (C) 2024-2025, Red Hat Inc.\n> *\n> * Color lookup tables construction\n> */\n> @@ -80,6 +80,11 @@ void Lut::updateGammaTable(IPAContext &context)\n> \tcontext.activeState.gamma.contrast = contrast;\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> @@ -91,28 +96,52 @@ 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> -\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> +\tconst bool gammaUpdateNeeded =\n> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> +\t\tcontext.activeState.gamma.contrast != context.activeState.knobs.contrast;\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.r() / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->red[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> -\t\t\t\t gammaTableSize - 1 });\n> -\t\tparams->green[i] = gammaTable[idx];\n> -\t\tidx = std::min({ static_cast(i * gains.b() / 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.ccmEnabled) {\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.r() / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->red[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->green[i] = gammaTable[idx];\n> +\t\t\tidx = std::min({ static_cast(i * gains.b() / div),\n> +\t\t\t\t\t gammaTableSize - 1 });\n> +\t\t\tparams->blue[i] = gammaTable[idx];\n> +\t\t}\n> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n> +\t\tMatrix gainCcm = { { gains.r(), 0, 0,\n> +\t\t\t\t\t\t 0, gains.g(), 0,\n> +\t\t\t\t\t\t 0, 0, gains.b() } };\n> +\t\tauto ccm = gainCcm * context.activeState.ccm.ccm;\n> +\t\tauto &red = params->redCcm;\n> +\t\tauto &green = params->greenCcm;\n> +\t\tauto &blue = params->blueCcm;\n> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\t\tred[i].r = ccmValue(i, ccm[0][0]);\n> +\t\t\tred[i].g = ccmValue(i, ccm[1][0]);\n> +\t\t\tred[i].b = ccmValue(i, ccm[2][0]);\n> +\t\t\tgreen[i].r = ccmValue(i, ccm[0][1]);\n> +\t\t\tgreen[i].g = ccmValue(i, ccm[1][1]);\n> +\t\t\tgreen[i].b = ccmValue(i, ccm[2][1]);\n> +\t\t\tblue[i].r = ccmValue(i, ccm[0][2]);\n> +\t\t\tblue[i].g = ccmValue(i, ccm[1][2]);\n> +\t\t\tblue[i].b = 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 889f864b..77324800 100644\n> --- a/src/ipa/simple/algorithms/lut.h\n> +++ b/src/ipa/simple/algorithms/lut.h\n> @@ -33,6 +33,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 45fe6960..4c84a556 100644\n> --- a/src/libcamera/software_isp/debayer.cpp\n> +++ b/src/libcamera/software_isp/debayer.cpp\n> @@ -23,9 +23,39 @@ namespace libcamera {\n> * \\brief Size of a color lookup table\n> */\n> \n> +/**\n> + * \\struct DebayerParams::CcmColumn\n> + * \\brief Type of a single column of a color correction matrix (CCM)\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmColumn::r\n> + * \\brief Red (first) component of a CCM column\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmColumn::g\n> + * \\brief Green (second) component of a CCM column\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::CcmColumn::b\n> + * \\brief Blue (third) component of a CCM column\n> + */\n> +\n> /**\n> * \\typedef DebayerParams::ColorLookupTable\n> - * \\brief Type of the lookup tables for red, green, blue values\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> @@ -43,6 +73,26 @@ namespace libcamera {\n> * \\brief Lookup table for blue color, mapping input values to output values\n> */\n> \n> +/**\n> + * \\var DebayerParams::redCcm\n> + * \\brief CCM lookup table for red color, mapping input values to output values\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::greenCcm\n> + * \\brief CCM lookup table for green color, mapping input values to output values\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::blueCcm\n> + * \\brief CCM 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> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 0cd03a8f..5ddfdcf6 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> @@ -51,8 +52,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> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> \t\tred_[i] = green_[i] = blue_[i] = i;\n> +\t\tredCcm_[i] = { 1, 0, 0 };\n> +\t\tgreenCcm_[i] = { 0, 1, 0 };\n> +\t\tblueCcm_[i] = { 0, 0, 1 };\n> +\t}\n> }\n> \n> DebayerCpu::~DebayerCpu() = default;\n> @@ -62,12 +67,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_[b]; \\\n> -\t*dst++ = green_[g]; \\\n> -\t*dst++ = red_[r]; \\\n> -\tif constexpr (addAlphaByte) \\\n> -\t\t*dst++ = 255; \\\n> +#define GAMMA(value) \\\n> +\t*dst++ = gammaLut_[std::clamp(value, 0, static_cast(gammaLut_.size()) - 1)]\n> +\n> +#define STORE_PIXEL(b_, g_, r_) \\\n> +\tif constexpr (ccmEnabled) { \\\n> +\t\tconst DebayerParams::CcmColumn &blue = blueCcm_[b_]; \\\n> +\t\tconst DebayerParams::CcmColumn &green = greenCcm_[g_]; \\\n> +\t\tconst DebayerParams::CcmColumn &red = redCcm_[r_]; \\\n> +\t\tGAMMA(blue.r + blue.g + blue.b); \\\n> +\t\tGAMMA(green.r + green.g + green.b); \\\n> +\t\tGAMMA(red.r + red.g + red.b); \\\n> +\t} else { \\\n> +\t\t*dst++ = blue_[b_]; \\\n> +\t\t*dst++ = green_[g_]; \\\n> +\t\t*dst++ = red_[r_]; \\\n> +\t} \\\n> +\tif constexpr (addAlphaByte) \\\n> +\t\t*dst++ = 255; \\\n> \tx++;\n> \n> /*\n> @@ -757,6 +774,10 @@ 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> +\tgreenCcm_ = params.greenCcm;\n> +\tredCcm_ = swapRedBlueGains_ ? params.blueCcm : params.redCcm;\n> +\tblueCcm_ = swapRedBlueGains_ ? params.redCcm : params.blueCcm;\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 21c08a2d..9f73a2fd 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -141,6 +141,10 @@ private:\n> \tDebayerParams::ColorLookupTable red_;\n> \tDebayerParams::ColorLookupTable green_;\n> \tDebayerParams::ColorLookupTable blue_;\n> +\tDebayerParams::CcmLookupTable redCcm_;\n> +\tDebayerParams::CcmLookupTable greenCcm_;\n> +\tDebayerParams::CcmLookupTable blueCcm_;\n> +\tDebayerParams::GammaLookupTable gammaLut_;\n> \tdebayerFn debayer0_;\n> \tdebayerFn debayer1_;\n> \tdebayerFn debayer2_;","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 E39A6C3260\n\tfor ;\n\tMon, 3 Feb 2025 20:15:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34ACE685A7;\n\tMon, 3 Feb 2025 21:15:17 +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 41C5661876\n\tfor ;\n\tMon, 3 Feb 2025 21:15:15 +0100 (CET)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-687-xND4-dfOM2ewLe7ui43V7w-1; Mon, 03 Feb 2025 15:15:12 -0500","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5da14484978so5435813a12.0\n\tfor ;\n\tMon, 03 Feb 2025 12:15:11 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5dc724c9cd7sm8275853a12.78.2025.02.03.12.15.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 03 Feb 2025 12:15:09 -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=\"C11HmG7d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738613713;\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\tin-reply-to:in-reply-to:references:references;\n\tbh=fNnVnihdXBP2V7p5jNWGxEMK0vD/Hcl4022ogVDaps4=;\n\tb=C11HmG7da/dEWi38WPzRkk6svuuHcziAsVIrePgW4qOBwjVll4zYt0dksQ271s6uVb9Rfu\n\t2NE1nmZHePYAoeesdmlcCb8OIW0PRthS9FbSU4ETq9aXxk2PUd0uSUha3TmXvLo4kM7GZb\n\tXOSHCNZAGjQjvUOdtzJAmG2l3baIdJU=","X-MC-Unique":"xND4-dfOM2ewLe7ui43V7w-1","X-Mimecast-MFC-AGG-ID":"xND4-dfOM2ewLe7ui43V7w","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738613711; x=1739218511;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=fNnVnihdXBP2V7p5jNWGxEMK0vD/Hcl4022ogVDaps4=;\n\tb=lalBhHHgnLT+rHh7ZmpGMGRg0H08ZEeJXJS6rQWJOXMGMffgVXsgV9WymmTNCkyHII\n\tJrWrbec3SDh/hGIc5KfH8I1Hq1fLVm1PjILUojNtazU1TcciKM6URBP/ZjJ1AJOEJdFC\n\t4yFXkbEKIww7eX9d4tw+rdgxl239Nyo/c2GnyO2BcYbfhXPtsDtojo0+GRd0Zq8K0mrw\n\tTURdpMSuwn7ozxuAaJiVTCPA2m/Ea93kjuCCTBAazRUSMpspR50klQqtizby9D9fPch0\n\tqk9F/F6iFJirTrLX07v56UUeZ5BdvamIn1VI+tnXPWYagJUPkLsjlRzOMj2BwUz3qY0U\n\t5vUw==","X-Gm-Message-State":"AOJu0Ywy/WZJkDBM0KZ66LKYM/nWZWiokyebZgXdtlkUzjjJVwkieIKQ\n\tEfIIxlbLshpPIU6sU7u8HYq6qjCPloyBCATXKEob65/YCNBATTG6hKQ9qMfkku7B6eI56715zt6\n\tfzuRM42LcbNxoH++OpbfOEblHC/98eq2V6o0LJfYtT4G/QBIzkP9LZDo+xj0mGO4VXZ+c/Ic=","X-Gm-Gg":"ASbGncsX+Y0h/CXLp3OCaqKeg4Z9bUKKGZ148QN66oFiiP+pC7K6YIs/LSgyUF1eGTA\n\tHXhXRjwrahMXpo1i/R62FkFCXXF78/rXqWQMARCn8/pA0RoDgSaCXo5mlZUBCrvxQVbdIfLNSYt\n\tKGf/lruNQT7MSqVYlCy2hunLjHmI71DRc5EwDw7+2PhSIcSiWOOi06AdS8iAwBcN+RmjQQg8qOv\n\tBsvAL8dh9GVT3mRhOoU1ofE689x/vMkKbdH8KwD1Tr1WJSjXS41/zwHKWB+fjMPKwS0IvRGxoSP\n\tKVbpPpTZrIcX0CcYaiSfcSOyUDzJ4YUNALR6X/RzB0F4sdySjkrDC4BwCQ==","X-Received":["by 2002:a05:6402:26cb:b0:5dc:882f:74a5 with SMTP id\n\t4fb4d7f45d1cf-5dc882f7635mr11948332a12.32.1738613710575; \n\tMon, 03 Feb 2025 12:15:10 -0800 (PST)","by 2002:a05:6402:26cb:b0:5dc:882f:74a5 with SMTP id\n\t4fb4d7f45d1cf-5dc882f7635mr11948307a12.32.1738613710090; \n\tMon, 03 Feb 2025 12:15:10 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IEFTKTppguoqc+pES+J4kfcs4ashhZK5Dm6kKoVnxfcy38i2OtJW2s/EoePgBkHAlYOGYaLRw==","From":"Milan Zamazal ","To":"libcamera-devel@lists.libcamera.org","Cc":"Robert Mader , Hans de Goede\n\t, Laurent Pinchart\n\t, Kieran Bingham\n\t","Subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","In-Reply-To":"<20250130181449.130492-11-mzamazal@redhat.com> (Milan Zamazal's\n\tmessage of \"Thu, 30 Jan 2025 19:14:47 +0100\")","References":"<20250130181449.130492-1-mzamazal@redhat.com>\n\t<20250130181449.130492-11-mzamazal@redhat.com>","Date":"Mon, 03 Feb 2025 21:15:07 +0100","Message-ID":"<85v7tqvl84.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"1wuMTvbUwQB372IN_Bc_c4iacZmsITKHda1BE8GsSKA_1738613711","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":33282,"web_url":"https://patchwork.libcamera.org/comment/33282/","msgid":"<85r04evf5l.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-02-03T22:26:14","subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Milan Zamazal writes:\n\n> Milan Zamazal writes:\n>\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> I think this should actually be:\n>\n> [r1 g1 b1] [r]\n> [r2 g2 b2] * [g]\n> [r3 g3 b3] [b]\n>\n> Unless somebody corrects me, I'll change this in v6.\n\nOr maybe not. There is a bug in applying the CCM color tables in\ndebayering, which confused me. If anybody knows which of the formats\nabove applies to the CCMs in the tuning files, please tell me.\n\n> I was unsure which is the correct version, I couldn't find it documented\n> anywhere. I can't remember why I selected the original version\n> eventually. But the second one produces much better images with the\n> matrices I copied from another pipeline for my sensor and it's also used\n> by matrices in RGB<->YUV conversion recipes so I guess that one is\n> actually right. Of course, it's just about the format of the input\n> matrices, nothing else changes.\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 can be represented either as unions,\n>> accommodating tables for both the CCM and non-CCM cases, or as separate\n>> tables for each of the cases, leaving the tables for the other case\n>> unused. The latter is selected as a matter of preference.\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>> Color gains from white balance are supposed not to be a part of the\n>> specified CCM. They are applied on it using matrix multiplication,\n>> which is simple and in correspondence with future additions in the form\n>> of matrix multiplication, like saturation adjustment.\n>>\n>> With this patch, the reported per-frame slowdown when applying CCM is\n>> about 45% on Debix Model A and about 75% on TI AM69 SK.\n>>\n>> Using std::clamp in debayering adds some performance penalty (a few\n>> percent). The clamping is necessary to eliminate out of range values\n>> possibly produced by the CCM. If it could be avoided by adjusting the\n>> precomputed tables some way then performance could be improved a bit.\n>>\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../internal/software_isp/debayer_params.h | 15 ++++-\n>> src/ipa/simple/algorithms/lut.cpp | 65 ++++++++++++++-----\n>> src/ipa/simple/algorithms/lut.h | 1 +\n>> src/libcamera/software_isp/debayer.cpp | 52 ++++++++++++++-\n>> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++--\n>> src/libcamera/software_isp/debayer_cpu.h | 4 ++\n>> 6 files changed, 145 insertions(+), 27 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 7d8fdd48..9a7dbe93 100644\n>> --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> @@ -1,6 +1,6 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> - * Copyright (C) 2023, 2024 Red Hat Inc.\n>> + * Copyright (C) 2023-2025 Red Hat Inc.\n>> *\n>> * Authors:\n>> * Hans de Goede \n>> @@ -18,11 +18,24 @@ namespace libcamera {\n>> struct DebayerParams {\n>> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> \n>> +\tstruct CcmColumn {\n>> +\t\tint16_t r;\n>> +\t\tint16_t g;\n>> +\t\tint16_t b;\n>> +\t};\n>> +\n>> \tusing ColorLookupTable = std::array;\n>> +\tusing CcmLookupTable = std::array;\n>> +\tusing GammaLookupTable = std::array;\n>> \n>> \tColorLookupTable red;\n>> \tColorLookupTable green;\n>> \tColorLookupTable blue;\n>> +\n>> +\tCcmLookupTable redCcm;\n>> +\tCcmLookupTable greenCcm;\n>> +\tCcmLookupTable blueCcm;\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 e5e995c7..7719aecc 100644\n>> --- a/src/ipa/simple/algorithms/lut.cpp\n>> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> @@ -1,6 +1,6 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> - * Copyright (C) 2024, Red Hat Inc.\n>> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> *\n>> * Color lookup tables construction\n>> */\n>> @@ -80,6 +80,11 @@ void Lut::updateGammaTable(IPAContext &context)\n>> \tcontext.activeState.gamma.contrast = contrast;\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>> @@ -91,28 +96,52 @@ 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>> -\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n>> +\tconst bool gammaUpdateNeeded =\n>> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level ||\n>> +\t\tcontext.activeState.gamma.contrast != context.activeState.knobs.contrast;\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.r() / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->red[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.g() / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->green[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.b() / 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.ccmEnabled) {\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.r() / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->red[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.g() / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->green[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.b() / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->blue[i] = gammaTable[idx];\n>> +\t\t}\n>> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n>> +\t\tMatrix gainCcm = { { gains.r(), 0, 0,\n>> +\t\t\t\t\t\t 0, gains.g(), 0,\n>> +\t\t\t\t\t\t 0, 0, gains.b() } };\n>> +\t\tauto ccm = gainCcm * context.activeState.ccm.ccm;\n>> +\t\tauto &red = params->redCcm;\n>> +\t\tauto &green = params->greenCcm;\n>> +\t\tauto &blue = params->blueCcm;\n>> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\t\tred[i].r = ccmValue(i, ccm[0][0]);\n>> +\t\t\tred[i].g = ccmValue(i, ccm[1][0]);\n>> +\t\t\tred[i].b = ccmValue(i, ccm[2][0]);\n>> +\t\t\tgreen[i].r = ccmValue(i, ccm[0][1]);\n>> +\t\t\tgreen[i].g = ccmValue(i, ccm[1][1]);\n>> +\t\t\tgreen[i].b = ccmValue(i, ccm[2][1]);\n>> +\t\t\tblue[i].r = ccmValue(i, ccm[0][2]);\n>> +\t\t\tblue[i].g = ccmValue(i, ccm[1][2]);\n>> +\t\t\tblue[i].b = 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 889f864b..77324800 100644\n>> --- a/src/ipa/simple/algorithms/lut.h\n>> +++ b/src/ipa/simple/algorithms/lut.h\n>> @@ -33,6 +33,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 45fe6960..4c84a556 100644\n>> --- a/src/libcamera/software_isp/debayer.cpp\n>> +++ b/src/libcamera/software_isp/debayer.cpp\n>> @@ -23,9 +23,39 @@ namespace libcamera {\n>> * \\brief Size of a color lookup table\n>> */\n>> \n>> +/**\n>> + * \\struct DebayerParams::CcmColumn\n>> + * \\brief Type of a single column of a color correction matrix (CCM)\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmColumn::r\n>> + * \\brief Red (first) component of a CCM column\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmColumn::g\n>> + * \\brief Green (second) component of a CCM column\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmColumn::b\n>> + * \\brief Blue (third) component of a CCM column\n>> + */\n>> +\n>> /**\n>> * \\typedef DebayerParams::ColorLookupTable\n>> - * \\brief Type of the lookup tables for red, green, blue values\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>> @@ -43,6 +73,26 @@ namespace libcamera {\n>> * \\brief Lookup table for blue color, mapping input values to output values\n>> */\n>> \n>> +/**\n>> + * \\var DebayerParams::redCcm\n>> + * \\brief CCM lookup table for red color, mapping input values to output values\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::greenCcm\n>> + * \\brief CCM lookup table for green color, mapping input values to output values\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::blueCcm\n>> + * \\brief CCM 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>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 0cd03a8f..5ddfdcf6 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>> @@ -51,8 +52,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>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> \t\tred_[i] = green_[i] = blue_[i] = i;\n>> +\t\tredCcm_[i] = { 1, 0, 0 };\n>> +\t\tgreenCcm_[i] = { 0, 1, 0 };\n>> +\t\tblueCcm_[i] = { 0, 0, 1 };\n>> +\t}\n>> }\n>> \n>> DebayerCpu::~DebayerCpu() = default;\n>> @@ -62,12 +67,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_[b]; \\\n>> -\t*dst++ = green_[g]; \\\n>> -\t*dst++ = red_[r]; \\\n>> -\tif constexpr (addAlphaByte) \\\n>> -\t\t*dst++ = 255; \\\n>> +#define GAMMA(value) \\\n>> +\t*dst++ = gammaLut_[std::clamp(value, 0, static_cast(gammaLut_.size()) - 1)]\n>> +\n>> +#define STORE_PIXEL(b_, g_, r_) \\\n>> +\tif constexpr (ccmEnabled) { \\\n>> +\t\tconst DebayerParams::CcmColumn &blue = blueCcm_[b_]; \\\n>> +\t\tconst DebayerParams::CcmColumn &green = greenCcm_[g_]; \\\n>> +\t\tconst DebayerParams::CcmColumn &red = redCcm_[r_]; \\\n>> +\t\tGAMMA(blue.r + blue.g + blue.b); \\\n>> +\t\tGAMMA(green.r + green.g + green.b); \\\n>> +\t\tGAMMA(red.r + red.g + red.b); \\\n>> +\t} else { \\\n>> +\t\t*dst++ = blue_[b_]; \\\n>> +\t\t*dst++ = green_[g_]; \\\n>> +\t\t*dst++ = red_[r_]; \\\n>> +\t} \\\n>> +\tif constexpr (addAlphaByte) \\\n>> +\t\t*dst++ = 255; \\\n>> \tx++;\n>> \n>> /*\n>> @@ -757,6 +774,10 @@ 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>> +\tgreenCcm_ = params.greenCcm;\n>> +\tredCcm_ = swapRedBlueGains_ ? params.blueCcm : params.redCcm;\n>> +\tblueCcm_ = swapRedBlueGains_ ? params.redCcm : params.blueCcm;\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 21c08a2d..9f73a2fd 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> @@ -141,6 +141,10 @@ private:\n>> \tDebayerParams::ColorLookupTable red_;\n>> \tDebayerParams::ColorLookupTable green_;\n>> \tDebayerParams::ColorLookupTable blue_;\n>> +\tDebayerParams::CcmLookupTable redCcm_;\n>> +\tDebayerParams::CcmLookupTable greenCcm_;\n>> +\tDebayerParams::CcmLookupTable blueCcm_;\n>> +\tDebayerParams::GammaLookupTable gammaLut_;\n>> \tdebayerFn debayer0_;\n>> \tdebayerFn debayer1_;\n>> \tdebayerFn debayer2_;","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 203FEC326B\n\tfor ;\n\tMon, 3 Feb 2025 22:26:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28E90685A7;\n\tMon, 3 Feb 2025 23:26:26 +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 A6CF161876\n\tfor ;\n\tMon, 3 Feb 2025 23:26:23 +0100 (CET)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-627-WUYXc3HcM76u0lj9xLLCxw-1; Mon, 03 Feb 2025 17:26:19 -0500","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-ab6d5363a4bso625989166b.3\n\tfor ;\n\tMon, 03 Feb 2025 14:26:18 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-ab6e49ff30bsm809498466b.88.2025.02.03.14.26.15\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 03 Feb 2025 14:26:15 -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=\"BSh4YCQ7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738621582;\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\tin-reply-to:in-reply-to:references:references;\n\tbh=1gSz/i/8Ew7Qv4PmP2qBgnUdguNH/VPCqS4DjwuRroA=;\n\tb=BSh4YCQ7sxMsVeXtWenZCezqj0Mp1rXznHPKFCaBjRLAJVPono6+Hfs+ehyvUabh72XxMh\n\tz/qxByKSXKNO8pUCEFNr7rLT+vE71IiM1YbwmAks9Wl9rCVNhBbG+vVJnMLgwt3Dk9UbDR\n\tf5rVqr2ckhyXTY+q5W6iz+LYOn9zlmk=","X-MC-Unique":"WUYXc3HcM76u0lj9xLLCxw-1","X-Mimecast-MFC-AGG-ID":"WUYXc3HcM76u0lj9xLLCxw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738621578; x=1739226378;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=1gSz/i/8Ew7Qv4PmP2qBgnUdguNH/VPCqS4DjwuRroA=;\n\tb=UK/9sLUxullGxPU4lH2BkkacreNw8tJFoKuuArxXJIpP9MQfQJMWPfPmbAqEM4rO2j\n\tyvkF6Fp77qfHOZdrgLhrYh5iakdhXR4ns4DTh9OliXYT09V4MKtKH6UfHKoHnLvxQjRv\n\tbHqEF7knaHTFmcY+kdux7cMk4/HSK8DaWk9OPXBGzWsHfXi1mzO1/yvXkrpZ6195B94i\n\t4o1Y/Ik8mMGoc4Flw0/4ChkWsz4gN9v44gT+rIjDzrZq0dUm7M9lXjJ6nkldJnm+GHUB\n\t0zr6qb+zX/QNVMqNa2KinyhNORlaSyZgRJopvlpcWh0lYLQx8IwlYSLmoa+8D7shUcyq\n\tr43A==","X-Gm-Message-State":"AOJu0YxxnhYSr+GPa3au1oGIGiALKzTchKRzMRmLdqkRJzdr/fejqFBJ\n\tzuBi42i18F6F1BCQADHJ1y/G0IehBmo5RTFxYUeb/MTOhm5YtgGUJGLXkGb+wBTan9J2GZVYzn3\n\tMC2x54IBBENLzrok2f5gmm9fwr5DDWy0y3+0Z7MYP3cj/MaToRrJq5VCbsUn5JvxtqLqiysY=","X-Gm-Gg":"ASbGncvFOZryqCUbw0K9LHP7wM7KqEzmaZf3DRPvCp7cvuaLFlQy44D57oRLmutCk8v\n\toF7qZQ+zW4kbEi8ziMlwNZ1EXCNGqKSrCsIWtd6TpIwawu/Hs1wq7ddJT4fnZprIgms2gNZKK7c\n\twMmnYNaX4lDcb8TPHJFPNA2wNSOB1p8POOHasRdpgYqrP0r3/1VmYSo+xDuoMz6WO0qCx1VN7Pq\n\tbiS2/GjNetsbUvxClX9zYq69+JANOOjYZgnkajU1GPfRvHMDPw+ad9B4HAuaHqbUvRY/CyQxLOv\n\ti5YXqCVkl280HKp2KpVUw6zOF6Vdv/hMNqNcrxmLv2TG2pdPScyrj+E+8g==","X-Received":["by 2002:a17:907:72cc:b0:ab6:ef33:ab7 with SMTP id\n\ta640c23a62f3a-ab6ef330aeemr2266362966b.46.1738621577505; \n\tMon, 03 Feb 2025 14:26:17 -0800 (PST)","by 2002:a17:907:72cc:b0:ab6:ef33:ab7 with SMTP id\n\ta640c23a62f3a-ab6ef330aeemr2266360666b.46.1738621576993; \n\tMon, 03 Feb 2025 14:26:16 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFkTYE1L8ur7Se8DUryWrdAp7Ex1Zof0R0sk2L7I6KYwWktvsXYuIGgQRzan4zWH0WT/hBlaw==","From":"Milan Zamazal ","To":"libcamera-devel@lists.libcamera.org","Cc":"Robert Mader , Hans de Goede\n\t, Laurent Pinchart\n\t, Kieran Bingham\n\t","Subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","In-Reply-To":"<85v7tqvl84.fsf@mzamazal-thinkpadp1gen3.tpbc.csb> (Milan\n\tZamazal's message of \"Mon, 03 Feb 2025 21:15:07 +0100\")","References":"<20250130181449.130492-1-mzamazal@redhat.com>\n\t<20250130181449.130492-11-mzamazal@redhat.com>\n\t<85v7tqvl84.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","Date":"Mon, 03 Feb 2025 23:26:14 +0100","Message-ID":"<85r04evf5l.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"oELMsX7iDbSVW9yC8Vvsc2pTGJH9AQtP9p1FtD5r6OU_1738621578","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":33284,"web_url":"https://patchwork.libcamera.org/comment/33284/","msgid":"","date":"2025-02-04T13:08:59","subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Milan,\n\nOn Mon, Feb 03, 2025 at 11:26:14PM +0100, Milan Zamazal wrote:\n> Milan Zamazal writes:\n> \n> > Milan Zamazal writes:\n> >\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> > I think this should actually be:\n> >\n> > [r1 g1 b1] [r]\n> > [r2 g2 b2] * [g]\n> > [r3 g3 b3] [b]\n\nThis is the one that is usually used. The color is represented as\ncolumn. But mathematically both are equivalent. You wrote the transposed\nnotation.\n\nout = M * c\n\nis equivalent to \n\nout^T = c^T * M^T\n\n> >\n> > Unless somebody corrects me, I'll change this in v6.\n> \n> Or maybe not. There is a bug in applying the CCM color tables in\n> debayering, which confused me. If anybody knows which of the formats\n> above applies to the CCMs in the tuning files, please tell me.\n\nAs noted above, the second form is used. In the tuning files they are\nwritten in row major order [r1, g1, b1, r2, g2, b2, r3, g3, b3]. A nice\ntrick to check that is that every row usually sums up to one. This is\nneeded so that the CCM does preserves grey colors.\n\nBest regards,\nStefan\n\n> \n> > I was unsure which is the correct version, I couldn't find it documented\n> > anywhere. I can't remember why I selected the original version\n> > eventually. But the second one produces much better images with the\n> > matrices I copied from another pipeline for my sensor and it's also used\n> > by matrices in RGB<->YUV conversion recipes so I guess that one is\n> > actually right. Of course, it's just about the format of the input\n> > matrices, nothing else changes.\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 can be represented either as unions,\n> >> accommodating tables for both the CCM and non-CCM cases, or as separate\n> >> tables for each of the cases, leaving the tables for the other case\n> >> unused. The latter is selected as a matter of preference.\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> >> Color gains from white balance are supposed not to be a part of the\n> >> specified CCM. They are applied on it using matrix multiplication,\n> >> which is simple and in correspondence with future additions in the form\n> >> of matrix multiplication, like saturation adjustment.\n> >>\n> >> With this patch, the reported per-frame slowdown when applying CCM is\n> >> about 45% on Debix Model A and about 75% on TI AM69 SK.\n> >>\n> >> Using std::clamp in debayering adds some performance penalty (a few\n> >> percent). The clamping is necessary to eliminate out of range values\n> >> possibly produced by the CCM. If it could be avoided by adjusting the\n> >> precomputed tables some way then performance could be improved a bit.\n> >>\n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> .../internal/software_isp/debayer_params.h | 15 ++++-\n> >> src/ipa/simple/algorithms/lut.cpp | 65 ++++++++++++++-----\n> >> src/ipa/simple/algorithms/lut.h | 1 +\n> >> src/libcamera/software_isp/debayer.cpp | 52 ++++++++++++++-\n> >> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++--\n> >> src/libcamera/software_isp/debayer_cpu.h | 4 ++\n> >> 6 files changed, 145 insertions(+), 27 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 7d8fdd48..9a7dbe93 100644\n> >> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> >> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> >> @@ -1,6 +1,6 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> - * Copyright (C) 2023, 2024 Red Hat Inc.\n> >> + * Copyright (C) 2023-2025 Red Hat Inc.\n> >> *\n> >> * Authors:\n> >> * Hans de Goede \n> >> @@ -18,11 +18,24 @@ namespace libcamera {\n> >> struct DebayerParams {\n> >> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n> >> \n> >> +\tstruct CcmColumn {\n> >> +\t\tint16_t r;\n> >> +\t\tint16_t g;\n> >> +\t\tint16_t b;\n> >> +\t};\n> >> +\n> >> \tusing ColorLookupTable = std::array;\n> >> +\tusing CcmLookupTable = std::array;\n> >> +\tusing GammaLookupTable = std::array;\n> >> \n> >> \tColorLookupTable red;\n> >> \tColorLookupTable green;\n> >> \tColorLookupTable blue;\n> >> +\n> >> +\tCcmLookupTable redCcm;\n> >> +\tCcmLookupTable greenCcm;\n> >> +\tCcmLookupTable blueCcm;\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 e5e995c7..7719aecc 100644\n> >> --- a/src/ipa/simple/algorithms/lut.cpp\n> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n> >> @@ -1,6 +1,6 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> - * Copyright (C) 2024, Red Hat Inc.\n> >> + * Copyright (C) 2024-2025, Red Hat Inc.\n> >> *\n> >> * Color lookup tables construction\n> >> */\n> >> @@ -80,6 +80,11 @@ void Lut::updateGammaTable(IPAContext &context)\n> >> \tcontext.activeState.gamma.contrast = contrast;\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> >> @@ -91,28 +96,52 @@ 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> >> -\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> >> +\tconst bool gammaUpdateNeeded =\n> >> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> >> +\t\tcontext.activeState.gamma.contrast != context.activeState.knobs.contrast;\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.r() / div),\n> >> -\t\t\t\t gammaTableSize - 1 });\n> >> -\t\tparams->red[i] = gammaTable[idx];\n> >> -\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> >> -\t\t\t\t gammaTableSize - 1 });\n> >> -\t\tparams->green[i] = gammaTable[idx];\n> >> -\t\tidx = std::min({ static_cast(i * gains.b() / 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.ccmEnabled) {\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.r() / div),\n> >> +\t\t\t\t\t gammaTableSize - 1 });\n> >> +\t\t\tparams->red[i] = gammaTable[idx];\n> >> +\t\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> >> +\t\t\t\t\t gammaTableSize - 1 });\n> >> +\t\t\tparams->green[i] = gammaTable[idx];\n> >> +\t\t\tidx = std::min({ static_cast(i * gains.b() / div),\n> >> +\t\t\t\t\t gammaTableSize - 1 });\n> >> +\t\t\tparams->blue[i] = gammaTable[idx];\n> >> +\t\t}\n> >> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n> >> +\t\tMatrix gainCcm = { { gains.r(), 0, 0,\n> >> +\t\t\t\t\t\t 0, gains.g(), 0,\n> >> +\t\t\t\t\t\t 0, 0, gains.b() } };\n> >> +\t\tauto ccm = gainCcm * context.activeState.ccm.ccm;\n> >> +\t\tauto &red = params->redCcm;\n> >> +\t\tauto &green = params->greenCcm;\n> >> +\t\tauto &blue = params->blueCcm;\n> >> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> +\t\t\tred[i].r = ccmValue(i, ccm[0][0]);\n> >> +\t\t\tred[i].g = ccmValue(i, ccm[1][0]);\n> >> +\t\t\tred[i].b = ccmValue(i, ccm[2][0]);\n> >> +\t\t\tgreen[i].r = ccmValue(i, ccm[0][1]);\n> >> +\t\t\tgreen[i].g = ccmValue(i, ccm[1][1]);\n> >> +\t\t\tgreen[i].b = ccmValue(i, ccm[2][1]);\n> >> +\t\t\tblue[i].r = ccmValue(i, ccm[0][2]);\n> >> +\t\t\tblue[i].g = ccmValue(i, ccm[1][2]);\n> >> +\t\t\tblue[i].b = 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 889f864b..77324800 100644\n> >> --- a/src/ipa/simple/algorithms/lut.h\n> >> +++ b/src/ipa/simple/algorithms/lut.h\n> >> @@ -33,6 +33,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 45fe6960..4c84a556 100644\n> >> --- a/src/libcamera/software_isp/debayer.cpp\n> >> +++ b/src/libcamera/software_isp/debayer.cpp\n> >> @@ -23,9 +23,39 @@ namespace libcamera {\n> >> * \\brief Size of a color lookup table\n> >> */\n> >> \n> >> +/**\n> >> + * \\struct DebayerParams::CcmColumn\n> >> + * \\brief Type of a single column of a color correction matrix (CCM)\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::CcmColumn::r\n> >> + * \\brief Red (first) component of a CCM column\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::CcmColumn::g\n> >> + * \\brief Green (second) component of a CCM column\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::CcmColumn::b\n> >> + * \\brief Blue (third) component of a CCM column\n> >> + */\n> >> +\n> >> /**\n> >> * \\typedef DebayerParams::ColorLookupTable\n> >> - * \\brief Type of the lookup tables for red, green, blue values\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> >> @@ -43,6 +73,26 @@ namespace libcamera {\n> >> * \\brief Lookup table for blue color, mapping input values to output values\n> >> */\n> >> \n> >> +/**\n> >> + * \\var DebayerParams::redCcm\n> >> + * \\brief CCM lookup table for red color, mapping input values to output values\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::greenCcm\n> >> + * \\brief CCM lookup table for green color, mapping input values to output values\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::blueCcm\n> >> + * \\brief CCM 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> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> >> index 0cd03a8f..5ddfdcf6 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> >> @@ -51,8 +52,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> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> \t\tred_[i] = green_[i] = blue_[i] = i;\n> >> +\t\tredCcm_[i] = { 1, 0, 0 };\n> >> +\t\tgreenCcm_[i] = { 0, 1, 0 };\n> >> +\t\tblueCcm_[i] = { 0, 0, 1 };\n> >> +\t}\n> >> }\n> >> \n> >> DebayerCpu::~DebayerCpu() = default;\n> >> @@ -62,12 +67,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_[b]; \\\n> >> -\t*dst++ = green_[g]; \\\n> >> -\t*dst++ = red_[r]; \\\n> >> -\tif constexpr (addAlphaByte) \\\n> >> -\t\t*dst++ = 255; \\\n> >> +#define GAMMA(value) \\\n> >> +\t*dst++ = gammaLut_[std::clamp(value, 0, static_cast(gammaLut_.size()) - 1)]\n> >> +\n> >> +#define STORE_PIXEL(b_, g_, r_) \\\n> >> +\tif constexpr (ccmEnabled) { \\\n> >> +\t\tconst DebayerParams::CcmColumn &blue = blueCcm_[b_]; \\\n> >> +\t\tconst DebayerParams::CcmColumn &green = greenCcm_[g_]; \\\n> >> +\t\tconst DebayerParams::CcmColumn &red = redCcm_[r_]; \\\n> >> +\t\tGAMMA(blue.r + blue.g + blue.b); \\\n> >> +\t\tGAMMA(green.r + green.g + green.b); \\\n> >> +\t\tGAMMA(red.r + red.g + red.b); \\\n> >> +\t} else { \\\n> >> +\t\t*dst++ = blue_[b_]; \\\n> >> +\t\t*dst++ = green_[g_]; \\\n> >> +\t\t*dst++ = red_[r_]; \\\n> >> +\t} \\\n> >> +\tif constexpr (addAlphaByte) \\\n> >> +\t\t*dst++ = 255; \\\n> >> \tx++;\n> >> \n> >> /*\n> >> @@ -757,6 +774,10 @@ 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> >> +\tgreenCcm_ = params.greenCcm;\n> >> +\tredCcm_ = swapRedBlueGains_ ? params.blueCcm : params.redCcm;\n> >> +\tblueCcm_ = swapRedBlueGains_ ? params.redCcm : params.blueCcm;\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 21c08a2d..9f73a2fd 100644\n> >> --- a/src/libcamera/software_isp/debayer_cpu.h\n> >> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> >> @@ -141,6 +141,10 @@ private:\n> >> \tDebayerParams::ColorLookupTable red_;\n> >> \tDebayerParams::ColorLookupTable green_;\n> >> \tDebayerParams::ColorLookupTable blue_;\n> >> +\tDebayerParams::CcmLookupTable redCcm_;\n> >> +\tDebayerParams::CcmLookupTable greenCcm_;\n> >> +\tDebayerParams::CcmLookupTable blueCcm_;\n> >> +\tDebayerParams::GammaLookupTable gammaLut_;\n> >> \tdebayerFn debayer0_;\n> >> \tdebayerFn debayer1_;\n> >> \tdebayerFn debayer2_;\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 33B41C326B\n\tfor ;\n\tTue, 4 Feb 2025 13:09:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4047E6858F;\n\tTue, 4 Feb 2025 14:09:05 +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 BD91F6053F\n\tfor ;\n\tTue, 4 Feb 2025 14:09:02 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:af53:7929:a212:d468])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 61385CDB;\n\tTue, 4 Feb 2025 14:07:50 +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=\"QzrpELiH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738674470;\n\tbh=TaIvYoIKKzohHADhLEbIOW22driVaEn8pAT93D6BZlU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QzrpELiHB3r3DOgQqXfhstrN19TNPnDQ3eT85g8WU5qFaJQLhJlFai7MqXyNlxgx/\n\ttTx2dkUg+6jpGTe7hWfLXBUVy0NmV9SLW0hY5S4nDXpf1yEw7vjatG8clNnLQmCH6v\n\thjpSZiLBdExgn22SF7V3xn5xUJboQT2jY18NiFpc=","Date":"Tue, 4 Feb 2025 14:08:59 +0100","From":"Stefan Klug ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, \n\tRobert Mader ,\n\tHans de Goede , \n\tLaurent Pinchart ,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","Message-ID":"","References":"<20250130181449.130492-1-mzamazal@redhat.com>\n\t<20250130181449.130492-11-mzamazal@redhat.com>\n\t<85v7tqvl84.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t<85r04evf5l.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85r04evf5l.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","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":33286,"web_url":"https://patchwork.libcamera.org/comment/33286/","msgid":"<851pwd7qug.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-02-04T13:59:51","subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Stefan,\n\nStefan Klug writes:\n\n> Hi Milan,\n>\n> On Mon, Feb 03, 2025 at 11:26:14PM +0100, Milan Zamazal wrote:\n>> Milan Zamazal writes:\n>> \n>> > Milan Zamazal writes:\n>> >\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>> > I think this should actually be:\n>> >\n>> > [r1 g1 b1] [r]\n>> > [r2 g2 b2] * [g]\n>> > [r3 g3 b3] [b]\n>\n> This is the one that is usually used. The color is represented as\n> column. But mathematically both are equivalent. You wrote the transposed\n> notation.\n>\n> out = M * c\n>\n> is equivalent to \n>\n> out^T = c^T * M^T\n>\n>> >\n>> > Unless somebody corrects me, I'll change this in v6.\n>> \n>> Or maybe not. There is a bug in applying the CCM color tables in\n>> debayering, which confused me. If anybody knows which of the formats\n>> above applies to the CCMs in the tuning files, please tell me.\n>\n> As noted above, the second form is used. In the tuning files they are\n> written in row major order [r1, g1, b1, r2, g2, b2, r3, g3, b3]. A nice\n> trick to check that is that every row usually sums up to one. This is\n> needed so that the CCM does preserves grey colors.\n\nThank you for clarification. I'll fix it in v6, together with two other\nmistakes I discovered while implementing a saturation control (which\nmade the issues much more prominent than my previous testing).\n\n> Best regards,\n> Stefan\n>\n>> \n>> > I was unsure which is the correct version, I couldn't find it documented\n>> > anywhere. I can't remember why I selected the original version\n>> > eventually. But the second one produces much better images with the\n>> > matrices I copied from another pipeline for my sensor and it's also used\n>> > by matrices in RGB<->YUV conversion recipes so I guess that one is\n>> > actually right. Of course, it's just about the format of the input\n>> > matrices, nothing else changes.\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 can be represented either as unions,\n>> >> accommodating tables for both the CCM and non-CCM cases, or as separate\n>> >> tables for each of the cases, leaving the tables for the other case\n>> >> unused. The latter is selected as a matter of preference.\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>> >> Color gains from white balance are supposed not to be a part of the\n>> >> specified CCM. They are applied on it using matrix multiplication,\n>> >> which is simple and in correspondence with future additions in the form\n>> >> of matrix multiplication, like saturation adjustment.\n>> >>\n>> >> With this patch, the reported per-frame slowdown when applying CCM is\n>> >> about 45% on Debix Model A and about 75% on TI AM69 SK.\n>> >>\n>> >> Using std::clamp in debayering adds some performance penalty (a few\n>> >> percent). The clamping is necessary to eliminate out of range values\n>> >> possibly produced by the CCM. If it could be avoided by adjusting the\n>> >> precomputed tables some way then performance could be improved a bit.\n>> >>\n>> >> Signed-off-by: Milan Zamazal \n>> >> ---\n>> >> .../internal/software_isp/debayer_params.h | 15 ++++-\n>> >> src/ipa/simple/algorithms/lut.cpp | 65 ++++++++++++++-----\n>> >> src/ipa/simple/algorithms/lut.h | 1 +\n>> >> src/libcamera/software_isp/debayer.cpp | 52 ++++++++++++++-\n>> >> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++--\n>> >> src/libcamera/software_isp/debayer_cpu.h | 4 ++\n>> >> 6 files changed, 145 insertions(+), 27 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 7d8fdd48..9a7dbe93 100644\n>> >> --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> >> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> >> @@ -1,6 +1,6 @@\n>> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> >> /*\n>> >> - * Copyright (C) 2023, 2024 Red Hat Inc.\n>> >> + * Copyright (C) 2023-2025 Red Hat Inc.\n>> >> *\n>> >> * Authors:\n>> >> * Hans de Goede \n>> >> @@ -18,11 +18,24 @@ namespace libcamera {\n>> >> struct DebayerParams {\n>> >> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> >> \n>> >> +\tstruct CcmColumn {\n>> >> +\t\tint16_t r;\n>> >> +\t\tint16_t g;\n>> >> +\t\tint16_t b;\n>> >> +\t};\n>> >> +\n>> >> \tusing ColorLookupTable = std::array;\n>> >> +\tusing CcmLookupTable = std::array;\n>> >> +\tusing GammaLookupTable = std::array;\n>> >> \n>> >> \tColorLookupTable red;\n>> >> \tColorLookupTable green;\n>> >> \tColorLookupTable blue;\n>> >> +\n>> >> +\tCcmLookupTable redCcm;\n>> >> +\tCcmLookupTable greenCcm;\n>> >> +\tCcmLookupTable blueCcm;\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 e5e995c7..7719aecc 100644\n>> >> --- a/src/ipa/simple/algorithms/lut.cpp\n>> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> >> @@ -1,6 +1,6 @@\n>> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> >> /*\n>> >> - * Copyright (C) 2024, Red Hat Inc.\n>> >> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> >> *\n>> >> * Color lookup tables construction\n>> >> */\n>> >> @@ -80,6 +80,11 @@ void Lut::updateGammaTable(IPAContext &context)\n>> >> \tcontext.activeState.gamma.contrast = contrast;\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>> >> @@ -91,28 +96,52 @@ 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>> >> -\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n>> >> +\tconst bool gammaUpdateNeeded =\n>> >> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level ||\n>> >> +\t\tcontext.activeState.gamma.contrast != context.activeState.knobs.contrast;\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.r() / div),\n>> >> -\t\t\t\t gammaTableSize - 1 });\n>> >> -\t\tparams->red[i] = gammaTable[idx];\n>> >> -\t\tidx = std::min({ static_cast(i * gains.g() / div),\n>> >> -\t\t\t\t gammaTableSize - 1 });\n>> >> -\t\tparams->green[i] = gammaTable[idx];\n>> >> -\t\tidx = std::min({ static_cast(i * gains.b() / 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.ccmEnabled) {\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.r() / div),\n>> >> +\t\t\t\t\t gammaTableSize - 1 });\n>> >> +\t\t\tparams->red[i] = gammaTable[idx];\n>> >> +\t\t\tidx = std::min({ static_cast(i * gains.g() / div),\n>> >> +\t\t\t\t\t gammaTableSize - 1 });\n>> >> +\t\t\tparams->green[i] = gammaTable[idx];\n>> >> +\t\t\tidx = std::min({ static_cast(i * gains.b() / div),\n>> >> +\t\t\t\t\t gammaTableSize - 1 });\n>> >> +\t\t\tparams->blue[i] = gammaTable[idx];\n>> >> +\t\t}\n>> >> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n>> >> +\t\tMatrix gainCcm = { { gains.r(), 0, 0,\n>> >> +\t\t\t\t\t\t 0, gains.g(), 0,\n>> >> +\t\t\t\t\t\t 0, 0, gains.b() } };\n>> >> +\t\tauto ccm = gainCcm * context.activeState.ccm.ccm;\n>> >> +\t\tauto &red = params->redCcm;\n>> >> +\t\tauto &green = params->greenCcm;\n>> >> +\t\tauto &blue = params->blueCcm;\n>> >> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> >> +\t\t\tred[i].r = ccmValue(i, ccm[0][0]);\n>> >> +\t\t\tred[i].g = ccmValue(i, ccm[1][0]);\n>> >> +\t\t\tred[i].b = ccmValue(i, ccm[2][0]);\n>> >> +\t\t\tgreen[i].r = ccmValue(i, ccm[0][1]);\n>> >> +\t\t\tgreen[i].g = ccmValue(i, ccm[1][1]);\n>> >> +\t\t\tgreen[i].b = ccmValue(i, ccm[2][1]);\n>> >> +\t\t\tblue[i].r = ccmValue(i, ccm[0][2]);\n>> >> +\t\t\tblue[i].g = ccmValue(i, ccm[1][2]);\n>> >> +\t\t\tblue[i].b = 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 889f864b..77324800 100644\n>> >> --- a/src/ipa/simple/algorithms/lut.h\n>> >> +++ b/src/ipa/simple/algorithms/lut.h\n>> >> @@ -33,6 +33,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 45fe6960..4c84a556 100644\n>> >> --- a/src/libcamera/software_isp/debayer.cpp\n>> >> +++ b/src/libcamera/software_isp/debayer.cpp\n>> >> @@ -23,9 +23,39 @@ namespace libcamera {\n>> >> * \\brief Size of a color lookup table\n>> >> */\n>> >> \n>> >> +/**\n>> >> + * \\struct DebayerParams::CcmColumn\n>> >> + * \\brief Type of a single column of a color correction matrix (CCM)\n>> >> + */\n>> >> +\n>> >> +/**\n>> >> + * \\var DebayerParams::CcmColumn::r\n>> >> + * \\brief Red (first) component of a CCM column\n>> >> + */\n>> >> +\n>> >> +/**\n>> >> + * \\var DebayerParams::CcmColumn::g\n>> >> + * \\brief Green (second) component of a CCM column\n>> >> + */\n>> >> +\n>> >> +/**\n>> >> + * \\var DebayerParams::CcmColumn::b\n>> >> + * \\brief Blue (third) component of a CCM column\n>> >> + */\n>> >> +\n>> >> /**\n>> >> * \\typedef DebayerParams::ColorLookupTable\n>> >> - * \\brief Type of the lookup tables for red, green, blue values\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>> >> @@ -43,6 +73,26 @@ namespace libcamera {\n>> >> * \\brief Lookup table for blue color, mapping input values to output values\n>> >> */\n>> >> \n>> >> +/**\n>> >> + * \\var DebayerParams::redCcm\n>> >> + * \\brief CCM lookup table for red color, mapping input values to output values\n>> >> + */\n>> >> +\n>> >> +/**\n>> >> + * \\var DebayerParams::greenCcm\n>> >> + * \\brief CCM lookup table for green color, mapping input values to output values\n>> >> + */\n>> >> +\n>> >> +/**\n>> >> + * \\var DebayerParams::blueCcm\n>> >> + * \\brief CCM 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>> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> >> index 0cd03a8f..5ddfdcf6 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>> >> @@ -51,8 +52,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>> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> >> \t\tred_[i] = green_[i] = blue_[i] = i;\n>> >> +\t\tredCcm_[i] = { 1, 0, 0 };\n>> >> +\t\tgreenCcm_[i] = { 0, 1, 0 };\n>> >> +\t\tblueCcm_[i] = { 0, 0, 1 };\n>> >> +\t}\n>> >> }\n>> >> \n>> >> DebayerCpu::~DebayerCpu() = default;\n>> >> @@ -62,12 +67,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_[b]; \\\n>> >> -\t*dst++ = green_[g]; \\\n>> >> -\t*dst++ = red_[r]; \\\n>> >> -\tif constexpr (addAlphaByte) \\\n>> >> -\t\t*dst++ = 255; \\\n>> >> +#define GAMMA(value) \\\n>> >> +\t*dst++ = gammaLut_[std::clamp(value, 0, static_cast(gammaLut_.size()) - 1)]\n>> >> +\n>> >> +#define STORE_PIXEL(b_, g_, r_) \\\n>> >> +\tif constexpr (ccmEnabled) { \\\n>> >> +\t\tconst DebayerParams::CcmColumn &blue = blueCcm_[b_]; \\\n>> >> +\t\tconst DebayerParams::CcmColumn &green = greenCcm_[g_]; \\\n>> >> +\t\tconst DebayerParams::CcmColumn &red = redCcm_[r_]; \\\n>> >> +\t\tGAMMA(blue.r + blue.g + blue.b); \\\n>> >> +\t\tGAMMA(green.r + green.g + green.b); \\\n>> >> +\t\tGAMMA(red.r + red.g + red.b); \\\n>> >> +\t} else { \\\n>> >> +\t\t*dst++ = blue_[b_]; \\\n>> >> +\t\t*dst++ = green_[g_]; \\\n>> >> +\t\t*dst++ = red_[r_]; \\\n>> >> +\t} \\\n>> >> +\tif constexpr (addAlphaByte) \\\n>> >> +\t\t*dst++ = 255; \\\n>> >> \tx++;\n>> >> \n>> >> /*\n>> >> @@ -757,6 +774,10 @@ 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>> >> +\tgreenCcm_ = params.greenCcm;\n>> >> +\tredCcm_ = swapRedBlueGains_ ? params.blueCcm : params.redCcm;\n>> >> +\tblueCcm_ = swapRedBlueGains_ ? params.redCcm : params.blueCcm;\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 21c08a2d..9f73a2fd 100644\n>> >> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> >> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> >> @@ -141,6 +141,10 @@ private:\n>> >> \tDebayerParams::ColorLookupTable red_;\n>> >> \tDebayerParams::ColorLookupTable green_;\n>> >> \tDebayerParams::ColorLookupTable blue_;\n>> >> +\tDebayerParams::CcmLookupTable redCcm_;\n>> >> +\tDebayerParams::CcmLookupTable greenCcm_;\n>> >> +\tDebayerParams::CcmLookupTable blueCcm_;\n>> >> +\tDebayerParams::GammaLookupTable gammaLut_;\n>> >> \tdebayerFn debayer0_;\n>> >> \tdebayerFn debayer1_;\n>> >> \tdebayerFn debayer2_;\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 39028C326B\n\tfor ;\n\tTue, 4 Feb 2025 14:00:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59CE26053F;\n\tTue, 4 Feb 2025 14:59:59 +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 917876053F\n\tfor ;\n\tTue, 4 Feb 2025 14:59:57 +0100 (CET)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-605-C1Gf-jNNNl27w_Yi-k5KhQ-1; Tue, 04 Feb 2025 08:59:54 -0500","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-ab6fe8d375eso359748266b.2\n\tfor ;\n\tTue, 04 Feb 2025 05:59:54 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-ab6e47cf48esm932799866b.50.2025.02.04.05.59.51\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 04 Feb 2025 05:59:52 -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=\"I++bwpdw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738677596;\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\tin-reply-to:in-reply-to:references:references;\n\tbh=Kw/7NUL44AU7iTrg/gV2jSe00C4757SJiRQgTxwFfL8=;\n\tb=I++bwpdwEa5jaKw0urHarXG+Nwvdo2lJ6oKKpcMQ4/i23pwmJxhifjCu3RPi9ub0zCT+Mr\n\tmdSGfyB5Ouncq+hODboOHVtSNyRO0Q7ne1vMZMHBs/UInXVBqgaNZt+macOWp3htsW3vEF\n\t/9EYVrgXiTr87UnPZggdCNHSPu2ZxKs=","X-MC-Unique":"C1Gf-jNNNl27w_Yi-k5KhQ-1","X-Mimecast-MFC-AGG-ID":"C1Gf-jNNNl27w_Yi-k5KhQ","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738677594; x=1739282394;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Kw/7NUL44AU7iTrg/gV2jSe00C4757SJiRQgTxwFfL8=;\n\tb=N9IzyD9GZrHjxrPUTH8DdBovB2kaqJlneQm312sOPlaBVGsPGnjh0woJ0Hbo07GCuj\n\tvKBgaJv5B4ck7n3KY8UbgJpMrdIiAb5A5v7uXmJv5j6WiNWDrElU0gJDQzx+Tsw6aUJD\n\tR6xXsw937YoKZ2EPrRgmKlrUp7ignpN3m42m4AsMV79d/9XioDQUsedhypHXCPanD86t\n\t0OBEuBF3cDKWFWyx7TFExwuRbrGufhRpYPhRBN6JIL+eFOSsUMAfjhHZEurV1wYIVgXB\n\tRQzNh3SrMXhZu8+loqtICJM2jkHJgBa8MjMgGUT64KDS1hWCkCj1lkMj3JUg7zwtPPVw\n\tkS+Q==","X-Gm-Message-State":"AOJu0YzYfl+yFY3Q6kGyAr/RUpRDn3wskU38kDNjfjON+PSxnmSW1Y7R\n\toGLyqyQeH1l6SiJR2Od6Kwi3vG3KYiW8reo6FSTdEBxP8/Ql3xagCrhhLhfGbQbuwoIYXnHgAXE\n\tC+5PEtmNYFaCn+5bk2HoE/ZSA2UE6LofnthBCYN9ToOratIzwR1ODWof125xhudZ5S6ouO58=","X-Gm-Gg":"ASbGncunUc516yEG7jG0C0Pa/unKlry6tml0pyPnM2Y4PQscmwyqsc5/e395cdg6n9E\n\tgc3UOLQl/ZnkA+VPP0tFwW4oYKfVg7vieqAESPlaKjVzkgu+LmNNcXHegug1ciEoJIoCHPGD7z/\n\tg0VtkwFjCdS68+w80AMI/ZHj4ZSpe/i9aOnkQ+6sEpCXRg4rEO4Gou2l6S+FQou7QkO4WQXn99/\n\tLI+GM+ZJuA9+ZWn6SNGgylv3Ppy5GTdtQg/RoOpGb02bSNAs0YgtcRpdsqizcJrFQYg8rO+AKHI\n\tAunRv3zz1hUsh7aYBvMqo1cobMOeqsF+mH0vMi7u1K2SzCyGBnlSNC33Tw==","X-Received":["by 2002:a17:907:971b:b0:ab6:597a:f5ee with SMTP id\n\ta640c23a62f3a-ab6cfcc5099mr2924550266b.12.1738677593462; \n\tTue, 04 Feb 2025 05:59:53 -0800 (PST)","by 2002:a17:907:971b:b0:ab6:597a:f5ee with SMTP id\n\ta640c23a62f3a-ab6cfcc5099mr2924545166b.12.1738677592831; \n\tTue, 04 Feb 2025 05:59:52 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGn+I199vAN1T1SmB4c7ngFjKFQz52umfZ9yuYz5dDowTrvupAaWajjirsqmseN3zWKqUQVzA==","From":"Milan Zamazal ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org, Robert Mader\n\t, Hans de Goede ,\n\tLaurent Pinchart , Kieran Bingham\n\t","Subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","In-Reply-To":"\n\t(Stefan Klug's message of \"Tue, 4 Feb 2025 14:08:59 +0100\")","References":"<20250130181449.130492-1-mzamazal@redhat.com>\n\t<20250130181449.130492-11-mzamazal@redhat.com>\n\t<85v7tqvl84.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t<85r04evf5l.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t","Date":"Tue, 04 Feb 2025 14:59:51 +0100","Message-ID":"<851pwd7qug.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"H-UfBxCiZGFFHapMBn4RASSzSNiYLVM2kpdfi2SLZM0_1738677594","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":33290,"web_url":"https://patchwork.libcamera.org/comment/33290/","msgid":"<85o6zh64aw.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-02-04T16:52:07","subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Thu, Jan 30, 2025 at 07:14:47PM +0100, 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 can be represented either as unions,\n>> accommodating tables for both the CCM and non-CCM cases, or as separate\n>> tables for each of the cases, leaving the tables for the other case\n>> unused. The latter is selected as a matter of preference.\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>> Color gains from white balance are supposed not to be a part of the\n>> specified CCM. They are applied on it using matrix multiplication,\n>> which is simple and in correspondence with future additions in the form\n>> of matrix multiplication, like saturation adjustment.\n>> \n>> With this patch, the reported per-frame slowdown when applying CCM is\n>> about 45% on Debix Model A and about 75% on TI AM69 SK.\n>> \n>> Using std::clamp in debayering adds some performance penalty (a few\n>> percent). The clamping is necessary to eliminate out of range values\n>> possibly produced by the CCM. If it could be avoided by adjusting the\n>> precomputed tables some way then performance could be improved a bit.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../internal/software_isp/debayer_params.h | 15 ++++-\n>> src/ipa/simple/algorithms/lut.cpp | 65 ++++++++++++++-----\n>> src/ipa/simple/algorithms/lut.h | 1 +\n>> src/libcamera/software_isp/debayer.cpp | 52 ++++++++++++++-\n>> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++--\n>> src/libcamera/software_isp/debayer_cpu.h | 4 ++\n>> 6 files changed, 145 insertions(+), 27 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 7d8fdd48..9a7dbe93 100644\n>> --- a/include/libcamera/internal/software_isp/debayer_params.h\n>> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n>> @@ -1,6 +1,6 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> - * Copyright (C) 2023, 2024 Red Hat Inc.\n>> + * Copyright (C) 2023-2025 Red Hat Inc.\n>> *\n>> * Authors:\n>> * Hans de Goede \n>> @@ -18,11 +18,24 @@ namespace libcamera {\n>> struct DebayerParams {\n>> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> \n>> +\tstruct CcmColumn {\n>> +\t\tint16_t r;\n>> +\t\tint16_t g;\n>> +\t\tint16_t b;\n>> +\t};\n>> +\n>> \tusing ColorLookupTable = std::array;\n>> +\tusing CcmLookupTable = std::array;\n>> +\tusing GammaLookupTable = std::array;\n>> \n>> \tColorLookupTable red;\n>> \tColorLookupTable green;\n>> \tColorLookupTable blue;\n>> +\n>> +\tCcmLookupTable redCcm;\n>> +\tCcmLookupTable greenCcm;\n>> +\tCcmLookupTable blueCcm;\n>> +\tGammaLookupTable gammaLut;\n>\n> Given that the ColorLookupTable and GammaLookupTable types are the same,\n> would it make sense to save a bit of memory by combining the combining\n> the colour and gamma tables ? I'm thinking about something like\n>\n> \tstruct ColorLookupTables {\n> \t\tCcmLookupTable ccm;\n> \t\tGammaLookupTable gamma;\n> \t}\n>\n> \tColorLookupTables red;\n> \tColorLookupTables green;\n> \tColorLookupTables blue;\n>\n> The three gamma tables would be identical when CCM is disabled. \n\nHm, this looks messy. Let's think a bit in term of a common type\n\n using LookupTable = std::array\n\nIf CCM is disabled, we need three LookupTable's for red, green, blue and\nthat's all.\n\nIf CCM is enabled, we need three CcmLookupTable's for red, green, blue\nand a single LookupTable for gamma.\n\nThis means in your proposal it should actually be\n\n struct ColorLookupTables {\n CcmLookupTable ccm;\n LookupTable colorOrGamma;\n }\n\nWith CCM disabled, `ccm' would be unused and colorOrGamma used. With\nCCM enabled, `ccm' would be used and colorOrGamma would be also used but\nit would be the same for all three colors. Such an arrangement makes\nlittle sense to me.\n\nIt's obvious the current version is also not crystal clear so I'll think\nwhether it could be done a bit better to make this patch more\nsatisfactory. The rest of the series is already settled down (with your\nother suggestions incorporated in v6). I'll also fix the bugs below in\nv6.\n\n> If that causes performance issues (possibly because the three\n> identical gamma tables would take more cache space), then we can keep\n> them separate. I'm tempted to merge the ColorLookupTable and\n> GammaLookupTable into a single type.\n>\n> You should also document somewhere which tables are used in which case\n> ( when CCM is disabled, combining gains and gamma, and Ccm\n> and gammaLut when CCM is enabled, with Ccm covering CCM and\n> gains, and gammaLut covering gamma). I think this would be easier to\n> document with the ColorLookupTables type, as you can then document what\n> the ccm and gamma fields cover.\n>\n> Either way, with this addressed,\n>\n> Reviewed-by: Laurent Pinchart \n>\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n>> index e5e995c7..7719aecc 100644\n>> --- a/src/ipa/simple/algorithms/lut.cpp\n>> +++ b/src/ipa/simple/algorithms/lut.cpp\n>> @@ -1,6 +1,6 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> - * Copyright (C) 2024, Red Hat Inc.\n>> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> *\n>> * Color lookup tables construction\n>> */\n>> @@ -80,6 +80,11 @@ void Lut::updateGammaTable(IPAContext &context)\n>> \tcontext.activeState.gamma.contrast = contrast;\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>> @@ -91,28 +96,52 @@ 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>> -\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n>> +\tconst bool gammaUpdateNeeded =\n>> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level ||\n>> +\t\tcontext.activeState.gamma.contrast != context.activeState.knobs.contrast;\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.r() / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->red[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.g() / div),\n>> -\t\t\t\t gammaTableSize - 1 });\n>> -\t\tparams->green[i] = gammaTable[idx];\n>> -\t\tidx = std::min({ static_cast(i * gains.b() / 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.ccmEnabled) {\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.r() / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->red[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.g() / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->green[i] = gammaTable[idx];\n>> +\t\t\tidx = std::min({ static_cast(i * gains.b() / div),\n>> +\t\t\t\t\t gammaTableSize - 1 });\n>> +\t\t\tparams->blue[i] = gammaTable[idx];\n>> +\t\t}\n>> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n>> +\t\tMatrix gainCcm = { { gains.r(), 0, 0,\n>> +\t\t\t\t\t\t 0, gains.g(), 0,\n>> +\t\t\t\t\t\t 0, 0, gains.b() } };\n>> +\t\tauto ccm = gainCcm * context.activeState.ccm.ccm;\n>> +\t\tauto &red = params->redCcm;\n>> +\t\tauto &green = params->greenCcm;\n>> +\t\tauto &blue = params->blueCcm;\n>> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\t\tred[i].r = ccmValue(i, ccm[0][0]);\n>> +\t\t\tred[i].g = ccmValue(i, ccm[1][0]);\n>> +\t\t\tred[i].b = ccmValue(i, ccm[2][0]);\n>> +\t\t\tgreen[i].r = ccmValue(i, ccm[0][1]);\n>> +\t\t\tgreen[i].g = ccmValue(i, ccm[1][1]);\n>> +\t\t\tgreen[i].b = ccmValue(i, ccm[2][1]);\n>> +\t\t\tblue[i].r = ccmValue(i, ccm[0][2]);\n>> +\t\t\tblue[i].g = ccmValue(i, ccm[1][2]);\n>> +\t\t\tblue[i].b = ccmValue(i, ccm[2][2]);\n\nThis needs to be transposed to conform with the CCM format in the tuning\nfiles as clarified by Stefan.\n\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 889f864b..77324800 100644\n>> --- a/src/ipa/simple/algorithms/lut.h\n>> +++ b/src/ipa/simple/algorithms/lut.h\n>> @@ -33,6 +33,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 45fe6960..4c84a556 100644\n>> --- a/src/libcamera/software_isp/debayer.cpp\n>> +++ b/src/libcamera/software_isp/debayer.cpp\n>> @@ -23,9 +23,39 @@ namespace libcamera {\n>> * \\brief Size of a color lookup table\n>> */\n>> \n>> +/**\n>> + * \\struct DebayerParams::CcmColumn\n>> + * \\brief Type of a single column of a color correction matrix (CCM)\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmColumn::r\n>> + * \\brief Red (first) component of a CCM column\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmColumn::g\n>> + * \\brief Green (second) component of a CCM column\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::CcmColumn::b\n>> + * \\brief Blue (third) component of a CCM column\n>> + */\n>> +\n>> /**\n>> * \\typedef DebayerParams::ColorLookupTable\n>> - * \\brief Type of the lookup tables for red, green, blue values\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>> @@ -43,6 +73,26 @@ namespace libcamera {\n>> * \\brief Lookup table for blue color, mapping input values to output values\n>> */\n>> \n>> +/**\n>> + * \\var DebayerParams::redCcm\n>> + * \\brief CCM lookup table for red color, mapping input values to output values\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::greenCcm\n>> + * \\brief CCM lookup table for green color, mapping input values to output values\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::blueCcm\n>> + * \\brief CCM 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>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 0cd03a8f..5ddfdcf6 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>> @@ -51,8 +52,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>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> \t\tred_[i] = green_[i] = blue_[i] = i;\n>> +\t\tredCcm_[i] = { 1, 0, 0 };\n>> +\t\tgreenCcm_[i] = { 0, 1, 0 };\n>> +\t\tblueCcm_[i] = { 0, 0, 1 };\n>> +\t}\n>> }\n>> \n>> DebayerCpu::~DebayerCpu() = default;\n>> @@ -62,12 +67,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_[b]; \\\n>> -\t*dst++ = green_[g]; \\\n>> -\t*dst++ = red_[r]; \\\n>> -\tif constexpr (addAlphaByte) \\\n>> -\t\t*dst++ = 255; \\\n>> +#define GAMMA(value) \\\n>> +\t*dst++ = gammaLut_[std::clamp(value, 0, static_cast(gammaLut_.size()) - 1)]\n>> +\n>> +#define STORE_PIXEL(b_, g_, r_) \\\n>> +\tif constexpr (ccmEnabled) { \\\n>> +\t\tconst DebayerParams::CcmColumn &blue = blueCcm_[b_]; \\\n>> +\t\tconst DebayerParams::CcmColumn &green = greenCcm_[g_]; \\\n>> +\t\tconst DebayerParams::CcmColumn &red = redCcm_[r_]; \\\n>> +\t\tGAMMA(blue.r + blue.g + blue.b); \\\n\nThis should be GAMMA(red.b + green.b + blue.b). Similarly for the other\ncolors. (It's embarrassing I haven't noticed this earlier but the wrong\nversion often produces believable outputs. I identified the problem\nwhen working on a saturation control when color casts in a supposedly\nmonochrome output were no longer plausible :-).)\n\nUnfortunately, the fixed version causes some slowdown (close to 10% in\nmy environment), apparently due to worse locality.\n\n>> +\t\tGAMMA(green.r + green.g + green.b); \\\n>> +\t\tGAMMA(red.r + red.g + red.b); \\\n>> +\t} else { \\\n>> +\t\t*dst++ = blue_[b_]; \\\n>> +\t\t*dst++ = green_[g_]; \\\n>> +\t\t*dst++ = red_[r_]; \\\n>> +\t} \\\n>> +\tif constexpr (addAlphaByte) \\\n>> +\t\t*dst++ = 255; \\\n>> \tx++;\n>> \n>> /*\n>> @@ -757,6 +774,10 @@ 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>> +\tgreenCcm_ = params.greenCcm;\n>> +\tredCcm_ = swapRedBlueGains_ ? params.blueCcm : params.redCcm;\n>> +\tblueCcm_ = swapRedBlueGains_ ? params.redCcm : params.blueCcm;\n\nAdditionally, `red' and 'blue' must be swapped in redCcm_ and blueCcm_\nitems when swapRedBlueGains_ is true. This became visible after fixing\nthe bug above.\n\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 21c08a2d..9f73a2fd 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> @@ -141,6 +141,10 @@ private:\n>> \tDebayerParams::ColorLookupTable red_;\n>> \tDebayerParams::ColorLookupTable green_;\n>> \tDebayerParams::ColorLookupTable blue_;\n>> +\tDebayerParams::CcmLookupTable redCcm_;\n>> +\tDebayerParams::CcmLookupTable greenCcm_;\n>> +\tDebayerParams::CcmLookupTable blueCcm_;\n>> +\tDebayerParams::GammaLookupTable gammaLut_;\n>> \tdebayerFn debayer0_;\n>> \tdebayerFn debayer1_;\n>> \tdebayerFn debayer2_;","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 31CD9C326B\n\tfor ;\n\tTue, 4 Feb 2025 16:52:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3DE20685B9;\n\tTue, 4 Feb 2025 17:52:17 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3784685A7\n\tfor ;\n\tTue, 4 Feb 2025 17:52:14 +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-596-MmPD5-4dP3CSvBNu8K4ckg-1; Tue, 04 Feb 2025 11:52:12 -0500","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-ab6eecc3221so391936166b.0\n\tfor ;\n\tTue, 04 Feb 2025 08:52:12 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5dc724aa1b1sm9462833a12.50.2025.02.04.08.52.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 04 Feb 2025 08:52:09 -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=\"bs/hNLX+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738687933;\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\tin-reply-to:in-reply-to:references:references;\n\tbh=za0RPIrmPUPyzZ1TdWn3cYLzONgRrGqJHq6+gPqVmuc=;\n\tb=bs/hNLX+LKM34dntec52lp3PLXB3WEvl00f7fRu46iq3RGWzeVdhScX69+ankGDg7SrE5W\n\tc2+DuDTUENIhcuf6zR9Clg3OO/ETwsC5YrPLJj5q444eXrZ9z5pS3F/mpNmMBETdTKeNDe\n\tGarCjYM1UEU1ED6SxRpOGTgoAVa8pd4=","X-MC-Unique":"MmPD5-4dP3CSvBNu8K4ckg-1","X-Mimecast-MFC-AGG-ID":"MmPD5-4dP3CSvBNu8K4ckg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738687931; x=1739292731;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=za0RPIrmPUPyzZ1TdWn3cYLzONgRrGqJHq6+gPqVmuc=;\n\tb=GOYqb2sYB+fUSQL0ttR8wAV8ghZntc4DY3H7idvIhZ+Nx1C2DOi61snnIwz3k/FitG\n\tMtisBE2W0pobNFhNhheenfD/Q6PNAq+2GoMhK2lTJMJnCp0mCUccrLnCb6BsS7yvIHys\n\tOBZQvZELNYA1wlhF8C8PVhcntGpAgMaSDocKJ2BG4m4KmFj8DyR2oW5koIz9IK98qbIu\n\tUgQF4d//S5GIh3nHQ9tLmStD+mglm9Zgu+uRYq0pdutRYSHSRWH+GKn2RMGR1Q8ds9Lt\n\tPeo4WtWdzAW66mE5JYWG8mv3GNo+L7Ll7Zulz5KqGqut5CEL3k4NXXlS4Y9Sz97JCwSi\n\tsA4Q==","X-Gm-Message-State":"AOJu0Ywvt760fS5A1F3YwVhxNSNnhm6S9Q6SEY7IO1P6iCroew0Zzffv\n\t+w+ulyCsrbSrydnTEGGuiOC/kWR1+PVWeKGbENPDxI8PzyJZUNxrTGJENwafkVVSIVi1fac0Tzf\n\tPY/ppTtv3xsVzjiGTjmzxrsIHlsglUOwTjxtXrPdwgXwLk/gJijluKqVR9ASqpIQ+uB7mimA=","X-Gm-Gg":"ASbGncvQxfDzp+qZ9NWBZDim1eu7jJCtHB0CO6h/SxmzVCWThMz6w2dQJn83y/A2Ep3\n\tv9wClakQvfNfGi95GQOcQHg70JEULHeGjpX7zhUjfYdg6tutBKTTJ7l/NMBioQMr1O7mpqMTRJw\n\tGMUelzze2qdetcFVVvhuSXdIuXy0lKd3HiYmeI9xbcsd9+9Z4sFY39NmVlf1xvMxz0whBuYQD2d\n\tQXTLi4jQYuet+zlekBXZVaPAtboRaLURLVl/iotZsi46OSnSKWvvFgkHithpE+hIRQahukmlY7T\n\t33xX9QkXNUtxdnOLn9kFZnCUHiCLamrdZ9X5CbIGggDtPnue72XzvDvDuA==","X-Received":["by 2002:a05:6402:1e93:b0:5da:d76:7b2e with SMTP id\n\t4fb4d7f45d1cf-5dc5e6d858bmr30675471a12.0.1738687930824; \n\tTue, 04 Feb 2025 08:52:10 -0800 (PST)","by 2002:a05:6402:1e93:b0:5da:d76:7b2e with SMTP id\n\t4fb4d7f45d1cf-5dc5e6d858bmr30675441a12.0.1738687930295; \n\tTue, 04 Feb 2025 08:52:10 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGWv9fhoHWcugPoEEIyv7ZsTHAzXxMNnJHgOCFbcMmsHPuNCjSflUAeowo2asQBc9mtBY0aXg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Robert Mader\n\t, Hans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","In-Reply-To":"<20250203013122.GM12673@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 3 Feb 2025 03:31:22 +0200\")","References":"<20250130181449.130492-1-mzamazal@redhat.com>\n\t<20250130181449.130492-11-mzamazal@redhat.com>\n\t<20250203013122.GM12673@pendragon.ideasonboard.com>","Date":"Tue, 04 Feb 2025 17:52:07 +0100","Message-ID":"<85o6zh64aw.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"q2CPVmWMp8zev6irkVXZHOfLDUZ6zl6PxcXFQuiqqGw_1738687931","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":33294,"web_url":"https://patchwork.libcamera.org/comment/33294/","msgid":"<20250204200254.GH22963@pendragon.ideasonboard.com>","date":"2025-02-04T20:02:54","subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Tue, Feb 04, 2025 at 05:52:07PM +0100, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Thu, Jan 30, 2025 at 07:14:47PM +0100, 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 can be represented either as unions,\n> >> accommodating tables for both the CCM and non-CCM cases, or as separate\n> >> tables for each of the cases, leaving the tables for the other case\n> >> unused. The latter is selected as a matter of preference.\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> >> Color gains from white balance are supposed not to be a part of the\n> >> specified CCM. They are applied on it using matrix multiplication,\n> >> which is simple and in correspondence with future additions in the form\n> >> of matrix multiplication, like saturation adjustment.\n> >> \n> >> With this patch, the reported per-frame slowdown when applying CCM is\n> >> about 45% on Debix Model A and about 75% on TI AM69 SK.\n> >> \n> >> Using std::clamp in debayering adds some performance penalty (a few\n> >> percent). The clamping is necessary to eliminate out of range values\n> >> possibly produced by the CCM. If it could be avoided by adjusting the\n> >> precomputed tables some way then performance could be improved a bit.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> .../internal/software_isp/debayer_params.h | 15 ++++-\n> >> src/ipa/simple/algorithms/lut.cpp | 65 ++++++++++++++-----\n> >> src/ipa/simple/algorithms/lut.h | 1 +\n> >> src/libcamera/software_isp/debayer.cpp | 52 ++++++++++++++-\n> >> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++--\n> >> src/libcamera/software_isp/debayer_cpu.h | 4 ++\n> >> 6 files changed, 145 insertions(+), 27 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 7d8fdd48..9a7dbe93 100644\n> >> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> >> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> >> @@ -1,6 +1,6 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> - * Copyright (C) 2023, 2024 Red Hat Inc.\n> >> + * Copyright (C) 2023-2025 Red Hat Inc.\n> >> *\n> >> * Authors:\n> >> * Hans de Goede \n> >> @@ -18,11 +18,24 @@ namespace libcamera {\n> >> struct DebayerParams {\n> >> \tstatic constexpr unsigned int kRGBLookupSize = 256;\n> >> \n> >> +\tstruct CcmColumn {\n> >> +\t\tint16_t r;\n> >> +\t\tint16_t g;\n> >> +\t\tint16_t b;\n> >> +\t};\n> >> +\n> >> \tusing ColorLookupTable = std::array;\n> >> +\tusing CcmLookupTable = std::array;\n> >> +\tusing GammaLookupTable = std::array;\n> >> \n> >> \tColorLookupTable red;\n> >> \tColorLookupTable green;\n> >> \tColorLookupTable blue;\n> >> +\n> >> +\tCcmLookupTable redCcm;\n> >> +\tCcmLookupTable greenCcm;\n> >> +\tCcmLookupTable blueCcm;\n> >> +\tGammaLookupTable gammaLut;\n> >\n> > Given that the ColorLookupTable and GammaLookupTable types are the same,\n> > would it make sense to save a bit of memory by combining the combining\n> > the colour and gamma tables ? I'm thinking about something like\n> >\n> > \tstruct ColorLookupTables {\n> > \t\tCcmLookupTable ccm;\n> > \t\tGammaLookupTable gamma;\n> > \t}\n> >\n> > \tColorLookupTables red;\n> > \tColorLookupTables green;\n> > \tColorLookupTables blue;\n> >\n> > The three gamma tables would be identical when CCM is disabled. \n> \n> Hm, this looks messy. Let's think a bit in term of a common type\n> \n> using LookupTable = std::array\n> \n> If CCM is disabled, we need three LookupTable's for red, green, blue and\n> that's all.\n> \n> If CCM is enabled, we need three CcmLookupTable's for red, green, blue\n> and a single LookupTable for gamma.\n> \n> This means in your proposal it should actually be\n> \n> struct ColorLookupTables {\n> CcmLookupTable ccm;\n> LookupTable colorOrGamma;\n> }\n> \n> With CCM disabled, `ccm' would be unused and colorOrGamma used. With\n> CCM enabled, `ccm' would be used and colorOrGamma would be also used but\n> it would be the same for all three colors. Such an arrangement makes\n> little sense to me.\n\nI agree it's not perfect.\n\n> It's obvious the current version is also not crystal clear so I'll think\n> whether it could be done a bit better to make this patch more\n> satisfactory.\n\nMaybe adding some extra comments would be enough. Documenting what\nprocessing steps are handled by each table in the CCM and !CCM case\nwould work for me.\n\n> The rest of the series is already settled down (with your\n> other suggestions incorporated in v6). I'll also fix the bugs below in\n> v6.\n> \n> > If that causes performance issues (possibly because the three\n> > identical gamma tables would take more cache space), then we can keep\n> > them separate. I'm tempted to merge the ColorLookupTable and\n> > GammaLookupTable into a single type.\n> >\n> > You should also document somewhere which tables are used in which case\n> > ( when CCM is disabled, combining gains and gamma, and Ccm\n> > and gammaLut when CCM is enabled, with Ccm covering CCM and\n> > gains, and gammaLut covering gamma). I think this would be easier to\n> > document with the ColorLookupTables type, as you can then document what\n> > the ccm and gamma fields cover.\n> >\n> > Either way, with this addressed,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp\n> >> index e5e995c7..7719aecc 100644\n> >> --- a/src/ipa/simple/algorithms/lut.cpp\n> >> +++ b/src/ipa/simple/algorithms/lut.cpp\n> >> @@ -1,6 +1,6 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> - * Copyright (C) 2024, Red Hat Inc.\n> >> + * Copyright (C) 2024-2025, Red Hat Inc.\n> >> *\n> >> * Color lookup tables construction\n> >> */\n> >> @@ -80,6 +80,11 @@ void Lut::updateGammaTable(IPAContext &context)\n> >> \tcontext.activeState.gamma.contrast = contrast;\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> >> @@ -91,28 +96,52 @@ 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> >> -\t context.activeState.gamma.contrast != context.activeState.knobs.contrast)\n> >> +\tconst bool gammaUpdateNeeded =\n> >> +\t\tcontext.activeState.gamma.blackLevel != context.activeState.blc.level ||\n> >> +\t\tcontext.activeState.gamma.contrast != context.activeState.knobs.contrast;\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.r() / div),\n> >> -\t\t\t\t gammaTableSize - 1 });\n> >> -\t\tparams->red[i] = gammaTable[idx];\n> >> -\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> >> -\t\t\t\t gammaTableSize - 1 });\n> >> -\t\tparams->green[i] = gammaTable[idx];\n> >> -\t\tidx = std::min({ static_cast(i * gains.b() / 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.ccmEnabled) {\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.r() / div),\n> >> +\t\t\t\t\t gammaTableSize - 1 });\n> >> +\t\t\tparams->red[i] = gammaTable[idx];\n> >> +\t\t\tidx = std::min({ static_cast(i * gains.g() / div),\n> >> +\t\t\t\t\t gammaTableSize - 1 });\n> >> +\t\t\tparams->green[i] = gammaTable[idx];\n> >> +\t\t\tidx = std::min({ static_cast(i * gains.b() / div),\n> >> +\t\t\t\t\t gammaTableSize - 1 });\n> >> +\t\t\tparams->blue[i] = gammaTable[idx];\n> >> +\t\t}\n> >> +\t} else if (context.activeState.ccm.changed || gammaUpdateNeeded) {\n> >> +\t\tMatrix gainCcm = { { gains.r(), 0, 0,\n> >> +\t\t\t\t\t\t 0, gains.g(), 0,\n> >> +\t\t\t\t\t\t 0, 0, gains.b() } };\n> >> +\t\tauto ccm = gainCcm * context.activeState.ccm.ccm;\n> >> +\t\tauto &red = params->redCcm;\n> >> +\t\tauto &green = params->greenCcm;\n> >> +\t\tauto &blue = params->blueCcm;\n> >> +\t\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> +\t\t\tred[i].r = ccmValue(i, ccm[0][0]);\n> >> +\t\t\tred[i].g = ccmValue(i, ccm[1][0]);\n> >> +\t\t\tred[i].b = ccmValue(i, ccm[2][0]);\n> >> +\t\t\tgreen[i].r = ccmValue(i, ccm[0][1]);\n> >> +\t\t\tgreen[i].g = ccmValue(i, ccm[1][1]);\n> >> +\t\t\tgreen[i].b = ccmValue(i, ccm[2][1]);\n> >> +\t\t\tblue[i].r = ccmValue(i, ccm[0][2]);\n> >> +\t\t\tblue[i].g = ccmValue(i, ccm[1][2]);\n> >> +\t\t\tblue[i].b = ccmValue(i, ccm[2][2]);\n> \n> This needs to be transposed to conform with the CCM format in the tuning\n> files as clarified by Stefan.\n> \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 889f864b..77324800 100644\n> >> --- a/src/ipa/simple/algorithms/lut.h\n> >> +++ b/src/ipa/simple/algorithms/lut.h\n> >> @@ -33,6 +33,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 45fe6960..4c84a556 100644\n> >> --- a/src/libcamera/software_isp/debayer.cpp\n> >> +++ b/src/libcamera/software_isp/debayer.cpp\n> >> @@ -23,9 +23,39 @@ namespace libcamera {\n> >> * \\brief Size of a color lookup table\n> >> */\n> >> \n> >> +/**\n> >> + * \\struct DebayerParams::CcmColumn\n> >> + * \\brief Type of a single column of a color correction matrix (CCM)\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::CcmColumn::r\n> >> + * \\brief Red (first) component of a CCM column\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::CcmColumn::g\n> >> + * \\brief Green (second) component of a CCM column\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::CcmColumn::b\n> >> + * \\brief Blue (third) component of a CCM column\n> >> + */\n> >> +\n> >> /**\n> >> * \\typedef DebayerParams::ColorLookupTable\n> >> - * \\brief Type of the lookup tables for red, green, blue values\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> >> @@ -43,6 +73,26 @@ namespace libcamera {\n> >> * \\brief Lookup table for blue color, mapping input values to output values\n> >> */\n> >> \n> >> +/**\n> >> + * \\var DebayerParams::redCcm\n> >> + * \\brief CCM lookup table for red color, mapping input values to output values\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::greenCcm\n> >> + * \\brief CCM lookup table for green color, mapping input values to output values\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var DebayerParams::blueCcm\n> >> + * \\brief CCM 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> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> >> index 0cd03a8f..5ddfdcf6 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> >> @@ -51,8 +52,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> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> \t\tred_[i] = green_[i] = blue_[i] = i;\n> >> +\t\tredCcm_[i] = { 1, 0, 0 };\n> >> +\t\tgreenCcm_[i] = { 0, 1, 0 };\n> >> +\t\tblueCcm_[i] = { 0, 0, 1 };\n> >> +\t}\n> >> }\n> >> \n> >> DebayerCpu::~DebayerCpu() = default;\n> >> @@ -62,12 +67,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_[b]; \\\n> >> -\t*dst++ = green_[g]; \\\n> >> -\t*dst++ = red_[r]; \\\n> >> -\tif constexpr (addAlphaByte) \\\n> >> -\t\t*dst++ = 255; \\\n> >> +#define GAMMA(value) \\\n> >> +\t*dst++ = gammaLut_[std::clamp(value, 0, static_cast(gammaLut_.size()) - 1)]\n> >> +\n> >> +#define STORE_PIXEL(b_, g_, r_) \\\n> >> +\tif constexpr (ccmEnabled) { \\\n> >> +\t\tconst DebayerParams::CcmColumn &blue = blueCcm_[b_]; \\\n> >> +\t\tconst DebayerParams::CcmColumn &green = greenCcm_[g_]; \\\n> >> +\t\tconst DebayerParams::CcmColumn &red = redCcm_[r_]; \\\n> >> +\t\tGAMMA(blue.r + blue.g + blue.b); \\\n> \n> This should be GAMMA(red.b + green.b + blue.b). Similarly for the other\n> colors. (It's embarrassing I haven't noticed this earlier but the wrong\n> version often produces believable outputs. I identified the problem\n> when working on a saturation control when color casts in a supposedly\n> monochrome output were no longer plausible :-).)\n> \n> Unfortunately, the fixed version causes some slowdown (close to 10% in\n> my environment), apparently due to worse locality.\n> \n> >> +\t\tGAMMA(green.r + green.g + green.b); \\\n> >> +\t\tGAMMA(red.r + red.g + red.b); \\\n> >> +\t} else { \\\n> >> +\t\t*dst++ = blue_[b_]; \\\n> >> +\t\t*dst++ = green_[g_]; \\\n> >> +\t\t*dst++ = red_[r_]; \\\n> >> +\t} \\\n> >> +\tif constexpr (addAlphaByte) \\\n> >> +\t\t*dst++ = 255; \\\n> >> \tx++;\n> >> \n> >> /*\n> >> @@ -757,6 +774,10 @@ 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> >> +\tgreenCcm_ = params.greenCcm;\n> >> +\tredCcm_ = swapRedBlueGains_ ? params.blueCcm : params.redCcm;\n> >> +\tblueCcm_ = swapRedBlueGains_ ? params.redCcm : params.blueCcm;\n> \n> Additionally, `red' and 'blue' must be swapped in redCcm_ and blueCcm_\n> items when swapRedBlueGains_ is true. This became visible after fixing\n> the bug above.\n> \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 21c08a2d..9f73a2fd 100644\n> >> --- a/src/libcamera/software_isp/debayer_cpu.h\n> >> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> >> @@ -141,6 +141,10 @@ private:\n> >> \tDebayerParams::ColorLookupTable red_;\n> >> \tDebayerParams::ColorLookupTable green_;\n> >> \tDebayerParams::ColorLookupTable blue_;\n> >> +\tDebayerParams::CcmLookupTable redCcm_;\n> >> +\tDebayerParams::CcmLookupTable greenCcm_;\n> >> +\tDebayerParams::CcmLookupTable blueCcm_;\n> >> +\tDebayerParams::GammaLookupTable gammaLut_;\n> >> \tdebayerFn debayer0_;\n> >> \tdebayerFn debayer1_;\n> >> \tdebayerFn debayer2_;","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 A323BC32F1\n\tfor ;\n\tTue, 4 Feb 2025 20:03:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91011685C4;\n\tTue, 4 Feb 2025 21:03:01 +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 B7106685A7\n\tfor ;\n\tTue, 4 Feb 2025 21:02:59 +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 0ED41227;\n\tTue, 4 Feb 2025 21:01:46 +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=\"fMj3Xgg+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738699307;\n\tbh=XBnbem3qcwZwE8BViVuBmyqlrGiLxZ9EQy2Fww74XTM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fMj3Xgg+5RGANDCtaOh0XIkJ3HLGeRoaHLkdycAB31jOYDbiSvZq9CXmr7WCKQJyS\n\t6Abh3XpQ6P0I9NAA96MfoM/oyeyCzNlwCwD/N0D6FTy8BkXNBLRQfjRk5qeN/0V0fx\n\tFPXXlSO/zikvnj5+xg9WiWyM3UF8yYSm7peH1nxY=","Date":"Tue, 4 Feb 2025 22:02:54 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tRobert Mader ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 10/10] libcamera: software_isp: Apply CCM in\n\tdebayering","Message-ID":"<20250204200254.GH22963@pendragon.ideasonboard.com>","References":"<20250130181449.130492-1-mzamazal@redhat.com>\n\t<20250130181449.130492-11-mzamazal@redhat.com>\n\t<20250203013122.GM12673@pendragon.ideasonboard.com>\n\t<85o6zh64aw.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85o6zh64aw.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","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\" "}}]