[{"id":29617,"web_url":"https://patchwork.libcamera.org/comment/29617/","msgid":"<171658962832.1040123.10492191765783818786@ping.linuxembedded.co.uk>","date":"2024-05-24T22:27:08","subject":"Re: [PATCH v3 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-05-23 12:26:54)\n> Constructing the color mapping tables is related to stats rather than\n> debayering, where they are applied. Let's move the corresponding code\n> to stats processing.\n> \n> This is a preliminary step towards building this functionality on top of\n> libipa/algorithm.h, which should follow.\n> \n> Signed-off-by: Milan Zamazal \n> Reviewed-by: Andrei Konovalov \n\nI haven't investigated/identified why yet - but I bisected the startup\noverflow/colour issue to this patch.\n\nThere's also a CI failure with blackLevel_ and gammaCorrection_ being\nleft unused that got caught.\n\n\n> ---\n> .../internal/software_isp/debayer_params.h | 18 +++----\n> src/ipa/simple/soft_simple.cpp | 51 +++++++++++++++----\n> src/libcamera/software_isp/debayer.cpp | 29 +++++------\n> src/libcamera/software_isp/debayer_cpu.cpp | 41 +++------------\n> src/libcamera/software_isp/debayer_cpu.h | 9 ++--\n> src/libcamera/software_isp/software_isp.cpp | 4 +-\n> 6 files changed, 75 insertions(+), 77 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 ce1b5945..463d24b3 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, Red Hat Inc.\n> + * Copyright (C) 2023, 2024 Red Hat Inc.\n> *\n> * Authors:\n> * Hans de Goede \n> @@ -10,20 +10,20 @@\n> \n> #pragma once\n> \n> +#include \n> +#include \n> +\n> namespace libcamera {\n> \n> struct DebayerParams {\n> static constexpr unsigned int kGain10 = 256;\n> + static constexpr unsigned int kRGBLookupSize = 256;\n> \n> - unsigned int gainR;\n> - unsigned int gainG;\n> - unsigned int gainB;\n> + using ColorLookupTable = std::array;\n> \n> - float gamma;\n> - /**\n> - * \\brief Level of the black point, 0..255, 0 is no correction.\n> - */\n> - unsigned int blackLevel;\n> + ColorLookupTable red;\n> + ColorLookupTable green;\n> + ColorLookupTable blue;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index 722aac83..1b0655fe 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -5,6 +5,7 @@\n> * Simple Software Image Processing Algorithm module\n> */\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -84,6 +85,10 @@ private:\n> ControlInfoMap sensorInfoMap_;\n> BlackLevel blackLevel_;\n> \n> + static constexpr unsigned int kGammaLookupSize = 1024;\n> + std::array gammaTable_;\n> + int lastBlackLevel_ = -1;\n> +\n> int32_t exposureMin_, exposureMax_;\n> int32_t exposure_;\n> double againMin_, againMax_, againMinStep_;\n> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> if (ignoreUpdates_ > 0)\n> blackLevel_.update(histogram);\n> const uint8_t blackLevel = blackLevel_.get();\n> - params_->blackLevel = blackLevel;\n> \n> /*\n> * Calculate red and blue gains for AWB.\n> @@ -265,12 +269,41 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n> const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n> \n> - params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> - params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> -\n> + /* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */\n> + const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> + const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> /* Green gain and gamma values are fixed */\n> - params_->gainG = 256;\n> - params_->gamma = 0.5;\n> + constexpr unsigned int gainG = 256;\n> + /* gamma == 1.0 means no correction */\n> + constexpr float gamma = 0.5;\n> +\n> + /* Update the gamma table if needed */\n> + if (blackLevel != lastBlackLevel_) {\n> + const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n> + std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n> + const float divisor = kGammaLookupSize - blackIndex - 1.0;\n> + for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> + gammaTable_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, gamma);\n> +\n> + lastBlackLevel_ = blackLevel;\n> + }\n> +\n> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> + constexpr unsigned int div =\n> + DebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n> + kGammaLookupSize;\n> + unsigned int idx;\n> +\n> + /* Apply gamma after gain! */\n> + idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n> + params_->red[i] = gammaTable_[idx];\n> +\n> + idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n> + params_->green[i] = gammaTable_[idx];\n> +\n> + idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n> + params_->blue[i] = gammaTable_[idx];\n> + }\n> \n> setIspParams.emit();\n> \n> @@ -291,7 +324,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> */\n> const unsigned int blackLevelHistIdx =\n> - params_->blackLevel / (256 / SwIspStats::kYHistogramSize);\n> + blackLevel / (256 / SwIspStats::kYHistogramSize);\n> const unsigned int histogramSize =\n> SwIspStats::kYHistogramSize - blackLevelHistIdx;\n> const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;\n> @@ -339,8 +372,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \n> LOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> << \" exp \" << exposure_ << \" again \" << again_\n> - << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB\n> - << \" black level \" << params_->blackLevel;\n> + << \" gain R/B \" << gainR << \"/\" << gainB\n> + << \" black level \" << static_cast(blackLevel);\n> }\n> \n> void IPASoftSimple::updateExposure(double exposureMSV)\n> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n> index efe75ea8..3f3969f7 100644\n> --- a/src/libcamera/software_isp/debayer.cpp\n> +++ b/src/libcamera/software_isp/debayer.cpp\n> @@ -1,7 +1,7 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> * Copyright (C) 2023, Linaro Ltd\n> - * Copyright (C) 2023, Red Hat Inc.\n> + * Copyright (C) 2023, 2024 Red Hat Inc.\n> *\n> * Authors:\n> * Hans de Goede \n> @@ -24,29 +24,28 @@ namespace libcamera {\n> */\n> \n> /**\n> - * \\var DebayerParams::gainR\n> - * \\brief Red gain\n> - *\n> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n> + * \\var DebayerParams::kRGBLookupSize\n> + * \\brief Size of a color lookup table\n> */\n> \n> /**\n> - * \\var DebayerParams::gainG\n> - * \\brief Green gain\n> - *\n> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n> + * \\typedef DebayerParams::ColorLookupTable\n> + * \\brief Type of the lookup tables for red, green, blue values\n> */\n> \n> /**\n> - * \\var DebayerParams::gainB\n> - * \\brief Blue gain\n> - *\n> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n> + * \\var DebayerParams::red\n> + * \\brief Lookup table for red color, mapping input values to output values\n> + */\n> +\n> +/**\n> + * \\var DebayerParams::green\n> + * \\brief Lookup table for green color, mapping input values to output values\n> */\n> \n> /**\n> - * \\var DebayerParams::gamma\n> - * \\brief Gamma correction, 1.0 is no correction\n> + * \\var DebayerParams::blue\n> + * \\brief Lookup table for blue color, mapping input values to output values\n> */\n> \n> /**\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 8254bbe9..f3099a62 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -11,7 +11,6 @@\n> \n> #include \"debayer_cpu.h\"\n> \n> -#include \n> #include \n> #include \n> \n> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> */\n> enableInputMemcpy_ = true;\n> \n> - /* Initialize gamma to 1.0 curve */\n> - for (unsigned int i = 0; i < kGammaLookupSize; i++)\n> - gamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n> + /* Initialize color lookup tables */\n> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n> + red_[i] = green_[i] = blue_[i] = i;\n> \n> for (unsigned int i = 0; i < kMaxLineBuffers; i++)\n> lineBuffers_[i] = nullptr;\n> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> }\n> \n> - /* Apply DebayerParams */\n> - if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {\n> - const unsigned int blackIndex =\n> - params.blackLevel * kGammaLookupSize / 256;\n> - std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);\n> - const float divisor = kGammaLookupSize - blackIndex - 1.0;\n> - for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> - gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);\n> -\n> - gammaCorrection_ = params.gamma;\n> - blackLevel_ = params.blackLevel;\n\n\nThe CI also identifies that this series leaves blackLevel_ and\ngammaCorrection_ unused.\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/59127177\n\n--\nKieran\n\n\n> - }\n> -\n> - if (swapRedBlueGains_)\n> - std::swap(params.gainR, params.gainB);\n> -\n> - for (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> - constexpr unsigned int div =\n> - kRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n> - unsigned int idx;\n> -\n> - /* Apply gamma after gain! */\n> - idx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n> - red_[i] = gamma_[idx];\n> -\n> - idx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n> - green_[i] = gamma_[idx];\n> -\n> - idx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n> - blue_[i] = gamma_[idx];\n> - }\n> + green_ = params.green;\n> + red_ = swapRedBlueGains_ ? params.blue : params.red;\n> + blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> \n> /* Copy metadata from the input buffer */\n> FrameMetadata &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 de216fe3..768a4643 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -122,15 +122,12 @@ private:\n> void process2(const uint8_t *src, uint8_t *dst);\n> void process4(const uint8_t *src, uint8_t *dst);\n> \n> - static constexpr unsigned int kGammaLookupSize = 1024;\n> - static constexpr unsigned int kRGBLookupSize = 256;\n> /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> static constexpr unsigned int kMaxLineBuffers = 5;\n> \n> - std::array gamma_;\n> - std::array red_;\n> - std::array green_;\n> - std::array blue_;\n> + DebayerParams::ColorLookupTable red_;\n> + DebayerParams::ColorLookupTable green_;\n> + DebayerParams::ColorLookupTable blue_;\n> debayerFn debayer0_;\n> debayerFn debayer1_;\n> debayerFn debayer2_;\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index c9b6be56..1003d835 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -63,9 +63,7 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> * handler\n> */\n> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> - : debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,\n> - DebayerParams::kGain10, 0.5f, 0 },\n> - dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> + : dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> {\n> if (!dmaHeap_.isValid()) {\n> LOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n> -- \n> 2.42.0\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 72258BDE6B\n\tfor ;\n\tFri, 24 May 2024 22:27:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD2D6634AC;\n\tSat, 25 May 2024 00:27:33 +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 E937363486\n\tfor ;\n\tSat, 25 May 2024 00:27:12 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3965F2B3;\n\tSat, 25 May 2024 00:26:57 +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=\"X6wYjnQF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716589617;\n\tbh=HY+1UirFjnSzOvG0PUmlrZ7VgOoCdFJ8KAz7gQSmrYc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=X6wYjnQF8cB9OGoy1hM9XIjIcGSAWrhD2IHu0wdlJF51kzu38uXBWmV0Nw+SNZh8h\n\tK2TdC6N37rrnBtMmXjdv51iEQryXmukxiI2tleUZLdrsYcT7Vt3rRRVevhv1HYWHyq\n\tpanAuLrwkgVyPYxC9yQwHUBgE7bpoRTeHUNJo79U=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240523112656.559150-4-mzamazal@redhat.com>","References":"<20240523112656.559150-1-mzamazal@redhat.com>\n\t<20240523112656.559150-4-mzamazal@redhat.com>","Subject":"Re: [PATCH v3 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tAndrei Konovalov ,\n\tLaurent Pinchart ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Fri, 24 May 2024 23:27:08 +0100","Message-ID":"<171658962832.1040123.10492191765783818786@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29626,"web_url":"https://patchwork.libcamera.org/comment/29626/","msgid":"<87zfsb2mkw.fsf@redhat.com>","date":"2024-05-27T17:26:07","subject":"Re: [PATCH v3 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Kieran,\n\nthank you for testing.\n\nKieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-05-23 12:26:54)\n>> Constructing the color mapping tables is related to stats rather than\n>> debayering, where they are applied. Let's move the corresponding code\n>\n>> to stats processing.\n>> \n>> This is a preliminary step towards building this functionality on top of\n>> libipa/algorithm.h, which should follow.\n>> \n>> Signed-off-by: Milan Zamazal \n>> Reviewed-by: Andrei Konovalov \n>\n> I haven't investigated/identified why yet - but I bisected the startup\n> overflow/colour issue to this patch.\n\nI cannot reproduce that nice effect but looking at the commit (thank you\nfor identifying it) and given that I get black frames at the beginning,\nI guess it's uninitialized debayerParams_. My fault, I'll fix it.\n\n> There's also a CI failure with blackLevel_ and gammaCorrection_ being\n> left unused that got caught.\n\nAh, right. I wonder why gcc doesn't report this (only clang does).\n\n>> ---\n>> .../internal/software_isp/debayer_params.h | 18 +++----\n>> src/ipa/simple/soft_simple.cpp | 51 +++++++++++++++----\n>> src/libcamera/software_isp/debayer.cpp | 29 +++++------\n>> src/libcamera/software_isp/debayer_cpu.cpp | 41 +++------------\n>> src/libcamera/software_isp/debayer_cpu.h | 9 ++--\n>> src/libcamera/software_isp/software_isp.cpp | 4 +-\n>> 6 files changed, 75 insertions(+), 77 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 ce1b5945..463d24b3 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, Red Hat Inc.\n>> + * Copyright (C) 2023, 2024 Red Hat Inc.\n>> *\n>> * Authors:\n>> * Hans de Goede \n>> @@ -10,20 +10,20 @@\n>> \n>> #pragma once\n>> \n>> +#include \n>> +#include \n>> +\n>> namespace libcamera {\n>> \n>> struct DebayerParams {\n>> static constexpr unsigned int kGain10 = 256;\n>> + static constexpr unsigned int kRGBLookupSize = 256;\n>> \n>> - unsigned int gainR;\n>> - unsigned int gainG;\n>> - unsigned int gainB;\n>> + using ColorLookupTable = std::array;\n>> \n>> - float gamma;\n>> - /**\n>> - * \\brief Level of the black point, 0..255, 0 is no correction.\n>> - */\n>> - unsigned int blackLevel;\n>> + ColorLookupTable red;\n>> + ColorLookupTable green;\n>> + ColorLookupTable blue;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index 722aac83..1b0655fe 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -5,6 +5,7 @@\n>> * Simple Software Image Processing Algorithm module\n>> */\n>> \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -84,6 +85,10 @@ private:\n>> ControlInfoMap sensorInfoMap_;\n>> BlackLevel blackLevel_;\n>> \n>> + static constexpr unsigned int kGammaLookupSize = 1024;\n>> + std::array gammaTable_;\n>> + int lastBlackLevel_ = -1;\n>> +\n>> int32_t exposureMin_, exposureMax_;\n>> int32_t exposure_;\n>> double againMin_, againMax_, againMinStep_;\n>> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> if (ignoreUpdates_ > 0)\n>> blackLevel_.update(histogram);\n>> const uint8_t blackLevel = blackLevel_.get();\n>> - params_->blackLevel = blackLevel;\n>> \n>> /*\n>> * Calculate red and blue gains for AWB.\n>> @@ -265,12 +269,41 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n>> const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n>> \n>> - params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> - params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> -\n>> + /* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */\n>> + const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> + const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> /* Green gain and gamma values are fixed */\n>> - params_->gainG = 256;\n>> - params_->gamma = 0.5;\n>> + constexpr unsigned int gainG = 256;\n>> + /* gamma == 1.0 means no correction */\n>> + constexpr float gamma = 0.5;\n>> +\n>> + /* Update the gamma table if needed */\n>> + if (blackLevel != lastBlackLevel_) {\n>> + const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n>> + std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n>> + const float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> + for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>> + gammaTable_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, gamma);\n>> +\n>> + lastBlackLevel_ = blackLevel;\n>> + }\n>> +\n>> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> + constexpr unsigned int div =\n>> + DebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n>> + kGammaLookupSize;\n>> + unsigned int idx;\n>> +\n>> + /* Apply gamma after gain! */\n>> + idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n>> + params_->red[i] = gammaTable_[idx];\n>> +\n>> + idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n>> + params_->green[i] = gammaTable_[idx];\n>> +\n>> + idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n>> + params_->blue[i] = gammaTable_[idx];\n>> + }\n>> \n>> setIspParams.emit();\n>> \n>> @@ -291,7 +324,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>> */\n>> const unsigned int blackLevelHistIdx =\n>> - params_->blackLevel / (256 / SwIspStats::kYHistogramSize);\n>> + blackLevel / (256 / SwIspStats::kYHistogramSize);\n>> const unsigned int histogramSize =\n>> SwIspStats::kYHistogramSize - blackLevelHistIdx;\n>> const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;\n>> @@ -339,8 +372,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> \n>> LOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n>> << \" exp \" << exposure_ << \" again \" << again_\n>> - << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB\n>> - << \" black level \" << params_->blackLevel;\n>> + << \" gain R/B \" << gainR << \"/\" << gainB\n>> + << \" black level \" << static_cast(blackLevel);\n>> }\n>> \n>> void IPASoftSimple::updateExposure(double exposureMSV)\n>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp\n>> index efe75ea8..3f3969f7 100644\n>> --- a/src/libcamera/software_isp/debayer.cpp\n>> +++ b/src/libcamera/software_isp/debayer.cpp\n>> @@ -1,7 +1,7 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> * Copyright (C) 2023, Linaro Ltd\n>> - * Copyright (C) 2023, Red Hat Inc.\n>> + * Copyright (C) 2023, 2024 Red Hat Inc.\n>> *\n>> * Authors:\n>> * Hans de Goede \n>> @@ -24,29 +24,28 @@ namespace libcamera {\n>> */\n>> \n>> /**\n>> - * \\var DebayerParams::gainR\n>> - * \\brief Red gain\n>> - *\n>> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n>> + * \\var DebayerParams::kRGBLookupSize\n>> + * \\brief Size of a color lookup table\n>> */\n>> \n>> /**\n>> - * \\var DebayerParams::gainG\n>> - * \\brief Green gain\n>> - *\n>> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n>> + * \\typedef DebayerParams::ColorLookupTable\n>> + * \\brief Type of the lookup tables for red, green, blue values\n>> */\n>> \n>> /**\n>> - * \\var DebayerParams::gainB\n>> - * \\brief Blue gain\n>> - *\n>> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.\n>> + * \\var DebayerParams::red\n>> + * \\brief Lookup table for red color, mapping input values to output values\n>> + */\n>> +\n>> +/**\n>> + * \\var DebayerParams::green\n>> + * \\brief Lookup table for green color, mapping input values to output values\n>> */\n>> \n>> /**\n>> - * \\var DebayerParams::gamma\n>> - * \\brief Gamma correction, 1.0 is no correction\n>> + * \\var DebayerParams::blue\n>> + * \\brief Lookup table for blue color, mapping input values to output values\n>> */\n>> \n>> /**\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 8254bbe9..f3099a62 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -11,7 +11,6 @@\n>> \n>> #include \"debayer_cpu.h\"\n>> \n>> -#include \n>> #include \n>> #include \n>> \n>> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> */\n>> enableInputMemcpy_ = true;\n>> \n>> - /* Initialize gamma to 1.0 curve */\n>> - for (unsigned int i = 0; i < kGammaLookupSize; i++)\n>> - gamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n>> + /* Initialize color lookup tables */\n>> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n>> + red_[i] = green_[i] = blue_[i] = i;\n>> \n>> for (unsigned int i = 0; i < kMaxLineBuffers; i++)\n>> lineBuffers_[i] = nullptr;\n>> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> }\n>> \n>> - /* Apply DebayerParams */\n>> - if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {\n>> - const unsigned int blackIndex =\n>> - params.blackLevel * kGammaLookupSize / 256;\n>> - std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);\n>> - const float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> - for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>> - gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);\n>> -\n>> - gammaCorrection_ = params.gamma;\n>> - blackLevel_ = params.blackLevel;\n>\n>\n> The CI also identifies that this series leaves blackLevel_ and\n> gammaCorrection_ unused.\n>\n> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59127177\n>\n> --\n> Kieran\n>\n>\n>> - }\n>> -\n>> - if (swapRedBlueGains_)\n>> - std::swap(params.gainR, params.gainB);\n>> -\n>> - for (unsigned int i = 0; i < kRGBLookupSize; i++) {\n>> - constexpr unsigned int div =\n>> - kRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n>> - unsigned int idx;\n>> -\n>> - /* Apply gamma after gain! */\n>> - idx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n>> - red_[i] = gamma_[idx];\n>> -\n>> - idx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n>> - green_[i] = gamma_[idx];\n>> -\n>> - idx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n>> - blue_[i] = gamma_[idx];\n>> - }\n>> + green_ = params.green;\n>> + red_ = swapRedBlueGains_ ? params.blue : params.red;\n>> + blue_ = swapRedBlueGains_ ? params.red : params.blue;\n>> \n>> /* Copy metadata from the input buffer */\n>> FrameMetadata &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 de216fe3..768a4643 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> @@ -122,15 +122,12 @@ private:\n>> void process2(const uint8_t *src, uint8_t *dst);\n>> void process4(const uint8_t *src, uint8_t *dst);\n>> \n>> - static constexpr unsigned int kGammaLookupSize = 1024;\n>> - static constexpr unsigned int kRGBLookupSize = 256;\n>> /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n>> static constexpr unsigned int kMaxLineBuffers = 5;\n>> \n>> - std::array gamma_;\n>> - std::array red_;\n>> - std::array green_;\n>> - std::array blue_;\n>> + DebayerParams::ColorLookupTable red_;\n>> + DebayerParams::ColorLookupTable green_;\n>> + DebayerParams::ColorLookupTable blue_;\n>> debayerFn debayer0_;\n>> debayerFn debayer1_;\n>> debayerFn debayer2_;\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index c9b6be56..1003d835 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -63,9 +63,7 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n>> * handler\n>> */\n>> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>> - : debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,\n>> - DebayerParams::kGain10, 0.5f, 0 },\n>> - dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n>> + : dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n>> {\n>> if (!dmaHeap_.isValid()) {\n>> LOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n>> -- \n>> 2.42.0\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 ADEA0BD87C\n\tfor ;\n\tMon, 27 May 2024 17:26:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0D76634B1;\n\tMon, 27 May 2024 19:26:17 +0200 (CEST)","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 E6FC9634AD\n\tfor ;\n\tMon, 27 May 2024 19:26:15 +0200 (CEST)","from mail-ed1-f72.google.com (mail-ed1-f72.google.com\n\t[209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-375-vF6KJwUbPr6euUV_yZT6yg-1; Mon, 27 May 2024 13:26:10 -0400","by mail-ed1-f72.google.com with SMTP id\n\t4fb4d7f45d1cf-579c69260bbso469942a12.0\n\tfor ;\n\tMon, 27 May 2024 10:26:10 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-578524bbb52sm5998746a12.84.2024.05.27.10.26.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 27 May 2024 10:26:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Z2Vkodto\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1716830772;\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=JcOKwDHhOjZJEYPjHt2Hvk1TVRhnUkH95s2UoqIdD5g=;\n\tb=Z2VkodtoUXg3WL82GMJhbz38+5yYWspNNgDh1ywTwXIBYGmqvxQ76fqxfEMFS4MNjlDQco\n\tkTXVyrvvYILHnitOeyfgsrnwFsl0LUlNAtZWI4yCz4OMq7Vsnl8c/+V0pB6Y/VBQ2DJZIv\n\tH2IId0II1BV/jWHclTmViB9O2UQLKdM=","X-MC-Unique":"vF6KJwUbPr6euUV_yZT6yg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1716830769; x=1717435569;\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=JcOKwDHhOjZJEYPjHt2Hvk1TVRhnUkH95s2UoqIdD5g=;\n\tb=BPCsp40zFx7G41n9f4IH115p9BeGYvHRO3JTpUc64oSQ1X1OLcJWZ/TtBrHOIAduTw\n\tEZal2ZcaxA4vd1fQAPycvZyNLcBWFvuAJ2C0RnjmN2E6tfNqvPQaZLMf9hmZzTroCWYD\n\tpNxaRe775xpRDNhAKUa9lVfFu0oaggB102Nc69Fht7R/ns/XWyadMUuudcgYlFbQ83Vx\n\t4PTmxxIxvqOHz18Z6/CeSQykTu4pqsXDP4SEmrVsZ3ctvbAAlKiFexAyYk7v6U49ix9E\n\tHKdVfnMEuX86oLVErqba7/1eGgGSL2Y+hZosCHIDfcIs9VSDHBx8Mzrgf+0Fn9FRiSRR\n\thaPg==","X-Gm-Message-State":"AOJu0YxwbBLdtUDjB1mnAPWBooXiRCtA0knQunFamaGK6xBPR1UTUqYc\n\tvqaaVDoOiQOzf9KiWtX078hRKNoDr9C5aXb1JLCSUXan4mfbZvp9ALv09rV+hsbWbXLpKnvQlDw\n\tePLQ6l8/3CEzvxzIfO54gx9E0wcT25VkZbWkrqx9UcYlDP9jkSZ23QHdYoM3A8W1elo8vNtQ=","X-Received":["by 2002:a50:9986:0:b0:572:a073:a39a with SMTP id\n\t4fb4d7f45d1cf-578518fecdbmr10002342a12.5.1716830769121; \n\tMon, 27 May 2024 10:26:09 -0700 (PDT)","by 2002:a50:9986:0:b0:572:a073:a39a with SMTP id\n\t4fb4d7f45d1cf-578518fecdbmr10002325a12.5.1716830768564; \n\tMon, 27 May 2024 10:26:08 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH+n/u1zfByJc0oXaK5s0WnIp+pJ2Nas1mGK3LJdwyXwSLVzpPs/r27NJs7drlLZB8Y8NsdRQ==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Andrei Konovalov\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v3 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","In-Reply-To":"<171658962832.1040123.10492191765783818786@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Fri, 24 May 2024 23:27:08 +0100\")","References":"<20240523112656.559150-1-mzamazal@redhat.com>\n\t<20240523112656.559150-4-mzamazal@redhat.com>\n\t<171658962832.1040123.10492191765783818786@ping.linuxembedded.co.uk>","Date":"Mon, 27 May 2024 19:26:07 +0200","Message-ID":"<87zfsb2mkw.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","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\" "}}]