[{"id":38524,"web_url":"https://patchwork.libcamera.org/comment/38524/","msgid":"<20260408003407.GO1268443@killaraus.ideasonboard.com>","date":"2026-04-08T00:34:07","subject":"Re: [PATCH 03/13] libcamera: software_isp: Fix black level handling\n\tin CPU ISP","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Apr 07, 2026 at 11:01:06PM +0100, Kieran Bingham wrote:\n> From: Milan Zamazal \n> \n> The black level handling in CPU ISP has two flaws:\n> \n> - The black level is applied after white balance rather than before.\n> \n> - It doesn't handle black levels with different values for individual\n> colour channels.\n> \n> The flaws are in both CCM and non-CCM cases. The wrong black level and\n> white balance application order is well visible when the white balance\n> gains are significantly different from 1.0. Then the output differs\n> significantly from GPU ISP output, which uses the correct order.\n> \n> This patch changes the computations of the lookup tables in a way that\n> fixes both the problems.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 60 +++++++++++++++---------------\n> 1 file changed, 30 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index dd0fff8714144417467bac16a1055d1185782d14..5356d6bbec11c30fa0b05659d44a91e69e2b79d0 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -879,15 +879,12 @@ void DebayerCpuThread::process4(uint32_t frame, const uint8_t *src, uint8_t *dst\n> \n> void DebayerCpu::updateGammaTable(const DebayerParams ¶ms)\n> {\n> -\tconst RGB blackLevel = params.blackLevel;\n> -\t/* Take let's say the green channel black level */\n> -\tconst unsigned int blackIndex = blackLevel[1] * gammaTable_.size();\n> \tconst float gamma = params.gamma;\n> \tconst float contrastExp = params.contrastExp;\n> \n> -\tconst float divisor = gammaTable_.size() - blackIndex - 1.0;\n> -\tfor (unsigned int i = blackIndex; i < gammaTable_.size(); i++) {\n> -\t\tfloat normalized = (i - blackIndex) / divisor;\n> +\tconst float divisor = gammaTable_.size() - 1.0;\n> +\tfor (unsigned int i = 0; i < gammaTable_.size(); i++) {\n\n\nI think this could now be written as\n\n\tfor (auto [i, value] : utils::enumerate(gammaTable_)) {\n\n> +\t\tfloat normalized = i / divisor;\n> \t\t/* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */\n> \t\t/* Apply simple S-curve */\n> \t\tif (normalized < 0.5)\n> @@ -897,14 +894,6 @@ void DebayerCpu::updateGammaTable(const DebayerParams ¶ms)\n> \t\tgammaTable_[i] = UINT8_MAX *\n> \t\t\t\t std::pow(normalized, gamma);\n\nand here,\n\n\t\tvalue = UINT8_MAX * std::pow(normalized, gamma);\n\nif you think that's clearer.\n\n> \t}\n> -\t/*\n> -\t * Due to CCM operations, the table lookup may reach indices below the black\n> -\t * level. Let's set the table values below black level to the minimum\n> -\t * non-black value to prevent problems when the minimum value is\n> -\t * significantly non-zero (for example, when the image should be all grey).\n> -\t */\n> -\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex,\n> -\t\t gammaTable_[blackIndex]);\n> }\n> \n> void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> @@ -916,11 +905,13 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> \tif (gammaUpdateNeeded)\n> \t\tupdateGammaTable(params);\n> \n> +\t/* Processing order: black level -> gains -> gamma */\n> \tauto matrixChanged = [](const Matrix &m1, const Matrix &m2) -> bool {\n> \t\treturn !std::equal(m1.data().begin(), m1.data().end(), m2.data().begin());\n> \t};\n> \tconst unsigned int gammaTableSize = gammaTable_.size();\n> -\tconst double div = static_cast(kRGBLookupSize) / gammaTableSize;\n> +\tRGB blackIndex = params.blackLevel * kRGBLookupSize;\n> +\n> \tif (ccmEnabled_) {\n> \t\tif (gammaUpdateNeeded ||\n> \t\t matrixChanged(params.combinedMatrix, params_.combinedMatrix)) {\n> @@ -930,17 +921,21 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> \t\t\tconst unsigned int redIndex = swapRedBlueGains_ ? 2 : 0;\n> \t\t\tconst unsigned int greenIndex = 1;\n> \t\t\tconst unsigned int blueIndex = swapRedBlueGains_ ? 0 : 2;\n> +\t\t\tconst RGB div =\n> +\t\t\t\t(RGB(kRGBLookupSize) - blackIndex).max(1.0) /\n> +\t\t\t\tkRGBLookupSize;\n> \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> -\t\t\t\tred[i].r = std::round(i * params.combinedMatrix[redIndex][0]);\n> -\t\t\t\tred[i].g = std::round(i * params.combinedMatrix[greenIndex][0]);\n> -\t\t\t\tred[i].b = std::round(i * params.combinedMatrix[blueIndex][0]);\n> -\t\t\t\tgreen[i].r = std::round(i * params.combinedMatrix[redIndex][1]);\n> -\t\t\t\tgreen[i].g = std::round(i * params.combinedMatrix[greenIndex][1]);\n> -\t\t\t\tgreen[i].b = std::round(i * params.combinedMatrix[blueIndex][1]);\n> -\t\t\t\tblue[i].r = std::round(i * params.combinedMatrix[redIndex][2]);\n> -\t\t\t\tblue[i].g = std::round(i * params.combinedMatrix[greenIndex][2]);\n> -\t\t\t\tblue[i].b = std::round(i * params.combinedMatrix[blueIndex][2]);\n> -\t\t\t\tgammaLut_[i] = gammaTable_[i / div];\n> +\t\t\t\tconst RGB rgb = ((RGB(i) - blackIndex) / div).max(0.0);\n> +\t\t\t\tred[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][0]);\n> +\t\t\t\tred[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][0]);\n> +\t\t\t\tred[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][0]);\n> +\t\t\t\tgreen[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][1]);\n> +\t\t\t\tgreen[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][1]);\n> +\t\t\t\tgreen[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][1]);\n> +\t\t\t\tblue[i].r = std::round(rgb.r() * params.combinedMatrix[redIndex][2]);\n> +\t\t\t\tblue[i].g = std::round(rgb.g() * params.combinedMatrix[greenIndex][2]);\n> +\t\t\t\tblue[i].b = std::round(rgb.b() * params.combinedMatrix[blueIndex][2]);\n> +\t\t\t\tgammaLut_[i] = gammaTable_[i * gammaTableSize / kRGBLookupSize];\n> \t\t\t}\n> \t\t}\n> \t} else {\n> @@ -949,12 +944,17 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms)\n> \t\t\tauto &red = swapRedBlueGains_ ? blue_ : red_;\n> \t\t\tauto &green = green_;\n> \t\t\tauto &blue = swapRedBlueGains_ ? red_ : blue_;\n> +\t\t\tconst RGB div =\n> +\t\t\t\t(RGB(kRGBLookupSize) - blackIndex).max(1.0) /\n> +\t\t\t\tgammaTableSize;\n> \t\t\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> -\t\t\t\t/* Apply gamma after gain! */\n> -\t\t\t\tconst RGB lutGains = (gains * i / div).min(gammaTableSize - 1);\n> -\t\t\t\tred[i] = gammaTable_[static_cast(lutGains.r())];\n> -\t\t\t\tgreen[i] = gammaTable_[static_cast(lutGains.g())];\n> -\t\t\t\tblue[i] = gammaTable_[static_cast(lutGains.b())];\n> +\t\t\t\tconst RGB lutGains =\n> +\t\t\t\t\t(gains * (RGB(i) - blackIndex) / div)\n> +\t\t\t\t\t\t.min(gammaTableSize - 1)\n> +\t\t\t\t\t\t.max(0.0);\n> +\t\t\t\tred[i] = gammaTable_[lutGains.r()];\n> +\t\t\t\tgreen[i] = gammaTable_[lutGains.g()];\n> +\t\t\t\tblue[i] = gammaTable_[lutGains.b()];\n> \t\t\t}\n> \t\t}\n> \t}\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 7F8BBBDCBD\n\tfor ;\n\tWed, 8 Apr 2026 00:34:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEFAA62DA6;\n\tWed, 8 Apr 2026 02:34:10 +0200 (CEST)","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 49C9E62CEB\n\tfor ;\n\tWed, 8 Apr 2026 02:34:09 +0200 (CEST)","from killaraus.ideasonboard.com\n\t(2001-14ba-703d-e500--2a1.rev.dnainternet.fi\n\t[IPv6:2001:14ba:703d:e500::2a1])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 6285F1121; \n\tWed, 8 Apr 2026 02:32:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uLf1Lpeh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1775608361;\n\tbh=wMuaJlelEx/9trtaVc9udD/TNvSRK78lnQCBXC2g5EE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uLf1LpehSJ3oYfVQTHOicYIInukEG2zeGJxrOQ0j98eOyTWnaLEoHPpfMCpySppAd\n\tEeYkeBgaScN7Ew9Z/HCLafvWygcVf+/e2+tj9MC/IASPL4kYU89m8uKuOPVnOrR1mF\n\tfeOE7WmJLwdqAILmk7E4YSnahAYqiyIsMqNXhiOM=","Date":"Wed, 8 Apr 2026 03:34:07 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ","Subject":"Re: [PATCH 03/13] libcamera: software_isp: Fix black level handling\n\tin CPU ISP","Message-ID":"<20260408003407.GO1268443@killaraus.ideasonboard.com>","References":"<20260407-kbingham-awb-split-v1-0-a39af3f4dc20@ideasonboard.com>\n\t<20260407-kbingham-awb-split-v1-3-a39af3f4dc20@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260407-kbingham-awb-split-v1-3-a39af3f4dc20@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]