[{"id":29642,"web_url":"https://patchwork.libcamera.org/comment/29642/","msgid":"<20240529002036.GB19014@pendragon.ideasonboard.com>","date":"2024-05-29T00:20:36","subject":"Re: [PATCH v4 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","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. This is a nice change, I think the resulting\ncode is easier to understand.\n\nOn Tue, May 28, 2024 at 06:11:24PM +0200, Milan Zamazal wrote:\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\nYou're also changing how gamma is handled, and that's not explained\nhere.\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> .../internal/software_isp/debayer_params.h | 19 +++----\n> src/ipa/simple/soft_simple.cpp | 50 +++++++++++++++----\n> src/libcamera/software_isp/debayer.cpp | 34 +++++++------\n> src/libcamera/software_isp/debayer_cpu.cpp | 43 +++-------------\n> src/libcamera/software_isp/debayer_cpu.h | 11 ++--\n> src/libcamera/software_isp/software_isp.cpp | 15 ++++--\n> 6 files changed, 92 insertions(+), 80 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..69fed1e7 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,21 @@\n> \n> #pragma once\n> \n> +#include \n> +#include \n> +\n> namespace libcamera {\n> \n> struct DebayerParams {\n> \tstatic constexpr unsigned int kGain10 = 256;\n> +\tstatic constexpr unsigned int kRGBLookupSize = 256;\n> +\tstatic constexpr float kGamma = 0.5;\n\nWith this patch the gamma value isn't an ISP parameter anything, so I\ndon't think this field belongs here. As far as I understand, the only\nreason why you need it is to initialize debayerParams_ in\nSoftwareIsp::SoftwareIsp(). Is that needed, or does the IPA module\nprovide ISP parameters for the first frame ?\n\nIf you need to initialize the parameters for the first frame on the\npipeline handler side for the time being, I would hardcode the 0.5 value\nthere and move the kGamma member from this structure to the IPA module.\n\n> \n> -\tunsigned int gainR;\n> -\tunsigned int gainG;\n> -\tunsigned int gainB;\n> +\tusing ColorLookupTable = std::array;\n> \n> -\tfloat gamma;\n> -\t/**\n> -\t * \\brief Level of the black point, 0..255, 0 is no correction.\n> -\t */\n> -\tunsigned int blackLevel;\n> +\tColorLookupTable red;\n> +\tColorLookupTable green;\n> +\tColorLookupTable 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..2c9fe98e 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\n#include \n\nSee Documentation/coding-style.rst\n\n> #include \n> #include \n> #include \n> @@ -84,6 +85,10 @@ private:\n> \tControlInfoMap sensorInfoMap_;\n> \tBlackLevel blackLevel_;\n> \n> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> +\tstd::array gammaTable_;\n> +\tint lastBlackLevel_ = -1;\n> +\n> \tint32_t exposureMin_, exposureMax_;\n> \tint32_t exposure_;\n> \tdouble againMin_, againMax_, againMinStep_;\n> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \tif (ignoreUpdates_ > 0)\n> \t\tblackLevel_.update(histogram);\n> \tconst uint8_t blackLevel = blackLevel_.get();\n> -\tparams_->blackLevel = blackLevel;\n> \n> \t/*\n> \t * Calculate red and blue gains for AWB.\n> @@ -265,12 +269,40 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n> \tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n> \n> -\tparams_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> -\tparams_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> -\n> +\t/* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */\n> +\tconst unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> +\tconst unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> \t/* Green gain and gamma values are fixed */\n> -\tparams_->gainG = 256;\n> -\tparams_->gamma = 0.5;\n> +\tconstexpr unsigned int gainG = 256;\n> +\n> +\t/* Update the gamma table if needed */\n> +\tif (blackLevel != lastBlackLevel_) {\n> +\t\tconst unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n> +\t\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n> +\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> +\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> +\t\t\tgammaTable_[i] = UINT8_MAX *\n> +\t\t\t\t\t powf((i - blackIndex) / divisor, DebayerParams::kGamma);\n\ns/powf/std::pow/\n\n> +\n> +\t\tlastBlackLevel_ = blackLevel;\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\tconstexpr unsigned int div =\n> +\t\t\tDebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n> +\t\t\tkGammaLookupSize;\n> +\t\tunsigned int idx;\n> +\n> +\t\t/* Apply gamma after gain! */\n> +\t\tidx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n> +\t\tparams_->red[i] = gammaTable_[idx];\n> +\n> +\t\tidx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n> +\t\tparams_->green[i] = gammaTable_[idx];\n> +\n> +\t\tidx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n> +\t\tparams_->blue[i] = gammaTable_[idx];\n> +\t}\n> \n> \tsetIspParams.emit();\n> \n> @@ -291,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> \t */\n> \tconst unsigned int blackLevelHistIdx =\n> -\t\tparams_->blackLevel / (256 / SwIspStats::kYHistogramSize);\n> +\t\tblackLevel / (256 / SwIspStats::kYHistogramSize);\n> \tconst unsigned int histogramSize =\n> \t\tSwIspStats::kYHistogramSize - blackLevelHistIdx;\n> \tconst unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;\n> @@ -339,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \n> \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> \t\t\t << \" exp \" << exposure_ << \" again \" << again_\n> -\t\t\t << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB\n> -\t\t\t << \" black level \" << params_->blackLevel;\n> +\t\t\t << \" gain R/B \" << gainR << \"/\" << gainB\n> +\t\t\t << \" 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..028bf27e 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,33 @@ 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> + * \\var DebayerParams::kGamma\n> + * \\brief Gamma correction, 1.0 is no correction\n> */\n> \n> /**\n> - * \\var DebayerParams::gainB\n> - * \\brief Blue 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::gamma\n> - * \\brief Gamma correction, 1.0 is no correction\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::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..c038eed4 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> @@ -35,7 +34,7 @@ namespace libcamera {\n> * \\param[in] stats Pointer to the stats object to use\n> */\n> DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> -\t: stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0)\n> +\t: stats_(std::move(stats))\n> {\n> \t/*\n> \t * Reading from uncached buffers may be very slow.\n> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> \t */\n> \tenableInputMemcpy_ = true;\n> \n> -\t/* Initialize gamma to 1.0 curve */\n> -\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> -\t\tgamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n> +\t/* Initialize color lookup tables */\n> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n> +\t\tred_[i] = green_[i] = blue_[i] = i;\n> \n> \tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n> \t\tlineBuffers_[i] = nullptr;\n> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> \t}\n> \n> -\t/* Apply DebayerParams */\n> -\tif (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {\n> -\t\tconst unsigned int blackIndex =\n> -\t\t\tparams.blackLevel * kGammaLookupSize / 256;\n> -\t\tstd::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);\n> -\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> -\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> -\t\t\tgamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);\n> -\n> -\t\tgammaCorrection_ = params.gamma;\n> -\t\tblackLevel_ = params.blackLevel;\n> -\t}\n> -\n> -\tif (swapRedBlueGains_)\n> -\t\tstd::swap(params.gainR, params.gainB);\n> -\n> -\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> -\t\tconstexpr unsigned int div =\n> -\t\t\tkRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n> -\t\tunsigned int idx;\n> -\n> -\t\t/* Apply gamma after gain! */\n> -\t\tidx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n> -\t\tred_[i] = gamma_[idx];\n> -\n> -\t\tidx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n> -\t\tgreen_[i] = gamma_[idx];\n> -\n> -\t\tidx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n> -\t\tblue_[i] = gamma_[idx];\n> -\t}\n> +\tgreen_ = params.green;\n> +\tred_ = swapRedBlueGains_ ? params.blue : params.red;\n> +\tblue_ = swapRedBlueGains_ ? params.red : params.blue;\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 de216fe3..be7dcdca 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> \tvoid process2(const uint8_t *src, uint8_t *dst);\n> \tvoid process4(const uint8_t *src, uint8_t *dst);\n> \n> -\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> -\tstatic constexpr unsigned int kRGBLookupSize = 256;\n> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> \n> -\tstd::array gamma_;\n> -\tstd::array red_;\n> -\tstd::array green_;\n> -\tstd::array blue_;\n> +\tDebayerParams::ColorLookupTable red_;\n> +\tDebayerParams::ColorLookupTable green_;\n> +\tDebayerParams::ColorLookupTable blue_;\n> \tdebayerFn debayer0_;\n> \tdebayerFn debayer1_;\n> \tdebayerFn debayer2_;\n> @@ -146,8 +143,6 @@ private:\n> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> \tbool enableInputMemcpy_;\n> \tbool swapRedBlueGains_;\n> -\tfloat gammaCorrection_;\n> -\tunsigned int blackLevel_;\n> \tunsigned int measuredFrames_;\n> \tint64_t frameProcessTime_;\n> \t/* Skip 30 frames for things to stabilize then measure 30 frames */\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index c9b6be56..84558c4e 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \"libcamera/internal/software_isp/software_isp.h\"\n> \n> +#include \n\n#include \n\n> #include \n> #include \n> #include \n> @@ -18,6 +19,7 @@\n> #include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> \n> #include \"debayer_cpu.h\"\n> \n> @@ -63,10 +65,17 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> * handler\n> */\n> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> -\t: debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,\n> -\t\t\t DebayerParams::kGain10, 0.5f, 0 },\n> -\t dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> +\t: dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> {\n> +\tstd::array gammaTable;\n> +\tfor (unsigned int i = 0; i < 256; i++)\n> +\t\tgammaTable[i] = powf(i / 256.0, DebayerParams::kGamma);\n\ns/powf/std::pow/\n\n> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> +\t\tdebayerParams_.red[i] = gammaTable[i];\n> +\t\tdebayerParams_.green[i] = gammaTable[i];\n> +\t\tdebayerParams_.blue[i] = gammaTable[i];\n> +\t}\n> +\n> \tif (!dmaHeap_.isValid()) {\n> \t\tLOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n> \t\treturn;","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 E5F0FBD87C\n\tfor ;\n\tWed, 29 May 2024 00:20:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B5FE634B0;\n\tWed, 29 May 2024 02:20:50 +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 A19926349B\n\tfor ;\n\tWed, 29 May 2024 02:20:48 +0200 (CEST)","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 21FDF9CA;\n\tWed, 29 May 2024 02:20:45 +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=\"pb5ZNHDa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716942045;\n\tbh=vmIRFaN7V4oBwrrtkUjEZ1Ef8tEcWqiJMgkh/L/MDHI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pb5ZNHDarrHr8sEHmYhtCehjFfKYct9CXAdW1IQ1ulcbQr4Y+csrYvbgxpp+MtIuq\n\tUBhm5ZIhxYOnfq0+5cpT/FBiomfkWS995u5uKdSRtJ8aDA6k+dUho6vzfbZo4Hb47T\n\tPzE/Licsfocj0TQ2tkUF48eoKmd8cRWA6Lp/xixk=","Date":"Wed, 29 May 2024 03:20:36 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tAndrei Konovalov ","Subject":"Re: [PATCH v4 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","Message-ID":"<20240529002036.GB19014@pendragon.ideasonboard.com>","References":"<20240528161126.35119-1-mzamazal@redhat.com>\n\t<20240528161126.35119-4-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240528161126.35119-4-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":29677,"web_url":"https://patchwork.libcamera.org/comment/29677/","msgid":"<87zfs7owmo.fsf@redhat.com>","date":"2024-05-30T20:47:11","subject":"Re: [PATCH v4 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 Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch. This is a nice change, I think the resulting\n> code is easier to understand.\n>\n> On Tue, May 28, 2024 at 06:11:24PM +0200, Milan Zamazal wrote:\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> You're also changing how gamma is handled, and that's not explained\n> here.\n\nI'll add the explanation.\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>> .../internal/software_isp/debayer_params.h | 19 +++----\n>> src/ipa/simple/soft_simple.cpp | 50 +++++++++++++++----\n>> src/libcamera/software_isp/debayer.cpp | 34 +++++++------\n>> src/libcamera/software_isp/debayer_cpu.cpp | 43 +++-------------\n>> src/libcamera/software_isp/debayer_cpu.h | 11 ++--\n>> src/libcamera/software_isp/software_isp.cpp | 15 ++++--\n>> 6 files changed, 92 insertions(+), 80 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..69fed1e7 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,21 @@\n>> \n>> #pragma once\n>> \n>> +#include \n>> +#include \n>> +\n>> namespace libcamera {\n>> \n>> struct DebayerParams {\n>> \tstatic constexpr unsigned int kGain10 = 256;\n>> +\tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> +\tstatic constexpr float kGamma = 0.5;\n>\n> With this patch the gamma value isn't an ISP parameter anything, so I\n> don't think this field belongs here. As far as I understand, the only\n> reason why you need it is to initialize debayerParams_ in\n> SoftwareIsp::SoftwareIsp(). \n\nYes.\n\n> Is that needed, or does the IPA module provide ISP parameters for the\n> first frame ?\n\nIt's needed for the first two frames, i.e. until stats processing starts\nproviding its own parameters.\n\n> If you need to initialize the parameters for the first frame on the\n> pipeline handler side for the time being, I would hardcode the 0.5 value\n> there and move the kGamma member from this structure to the IPA module.\n\nOK.\n\n>> -\tunsigned int gainR;\n>> -\tunsigned int gainG;\n>> -\tunsigned int gainB;\n>> +\tusing ColorLookupTable = std::array;\n>> \n>> -\tfloat gamma;\n>> -\t/**\n>> -\t * \\brief Level of the black point, 0..255, 0 is no correction.\n>> -\t */\n>> -\tunsigned int blackLevel;\n>> +\tColorLookupTable red;\n>> +\tColorLookupTable green;\n>> +\tColorLookupTable 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..2c9fe98e 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>\n> #include \n>\n> See Documentation/coding-style.rst\n\nI see, tricky :-). Could checkstyle.py catch this or is it more\ncomplicated (I can see math.h is used in multiple places, including\ndocumentation)?\n\n>> #include \n>> #include \n>> #include \n>> @@ -84,6 +85,10 @@ private:\n>> \tControlInfoMap sensorInfoMap_;\n>> \tBlackLevel blackLevel_;\n>> \n>> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>> +\tstd::array gammaTable_;\n>> +\tint lastBlackLevel_ = -1;\n>> +\n>> \tint32_t exposureMin_, exposureMax_;\n>> \tint32_t exposure_;\n>> \tdouble againMin_, againMax_, againMinStep_;\n>> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> \tif (ignoreUpdates_ > 0)\n>> \t\tblackLevel_.update(histogram);\n>> \tconst uint8_t blackLevel = blackLevel_.get();\n>> -\tparams_->blackLevel = blackLevel;\n>> \n>> \t/*\n>> \t * Calculate red and blue gains for AWB.\n>> @@ -265,12 +269,40 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> \tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n>> \tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n>> \n>> -\tparams_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> -\tparams_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> -\n>> +\t/* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */\n>> +\tconst unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> +\tconst unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> \t/* Green gain and gamma values are fixed */\n>> -\tparams_->gainG = 256;\n>> -\tparams_->gamma = 0.5;\n>> +\tconstexpr unsigned int gainG = 256;\n>> +\n>> +\t/* Update the gamma table if needed */\n>> +\tif (blackLevel != lastBlackLevel_) {\n>> +\t\tconst unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n>> +\t\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n>> +\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> +\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>> +\t\t\tgammaTable_[i] = UINT8_MAX *\n>> +\t\t\t\t\t powf((i - blackIndex) / divisor, DebayerParams::kGamma);\n>\n> s/powf/std::pow/\n>\n>> +\n>> +\t\tlastBlackLevel_ = blackLevel;\n>> +\t}\n>> +\n>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\tconstexpr unsigned int div =\n>> +\t\t\tDebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n>> +\t\t\tkGammaLookupSize;\n>> +\t\tunsigned int idx;\n>> +\n>> +\t\t/* Apply gamma after gain! */\n>> +\t\tidx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n>> +\t\tparams_->red[i] = gammaTable_[idx];\n>> +\n>> +\t\tidx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n>> +\t\tparams_->green[i] = gammaTable_[idx];\n>> +\n>> +\t\tidx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n>> +\t\tparams_->blue[i] = gammaTable_[idx];\n>> +\t}\n>> \n>> \tsetIspParams.emit();\n>> \n>> @@ -291,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>> \t */\n>> \tconst unsigned int blackLevelHistIdx =\n>> -\t\tparams_->blackLevel / (256 / SwIspStats::kYHistogramSize);\n>> +\t\tblackLevel / (256 / SwIspStats::kYHistogramSize);\n>> \tconst unsigned int histogramSize =\n>> \t\tSwIspStats::kYHistogramSize - blackLevelHistIdx;\n>> \tconst unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;\n>> @@ -339,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> \n>> \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n>> \t\t\t << \" exp \" << exposure_ << \" again \" << again_\n>> -\t\t\t << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB\n>> -\t\t\t << \" black level \" << params_->blackLevel;\n>> +\t\t\t << \" gain R/B \" << gainR << \"/\" << gainB\n>> +\t\t\t << \" 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..028bf27e 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,33 @@ 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>> + * \\var DebayerParams::kGamma\n>> + * \\brief Gamma correction, 1.0 is no correction\n>> */\n>> \n>> /**\n>> - * \\var DebayerParams::gainB\n>> - * \\brief Blue 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::gamma\n>> - * \\brief Gamma correction, 1.0 is no correction\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::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..c038eed4 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>> @@ -35,7 +34,7 @@ namespace libcamera {\n>> * \\param[in] stats Pointer to the stats object to use\n>> */\n>> DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> -\t: stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0)\n>> +\t: stats_(std::move(stats))\n>> {\n>> \t/*\n>> \t * Reading from uncached buffers may be very slow.\n>> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> \t */\n>> \tenableInputMemcpy_ = true;\n>> \n>> -\t/* Initialize gamma to 1.0 curve */\n>> -\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n>> -\t\tgamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n>> +\t/* Initialize color lookup tables */\n>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n>> +\t\tred_[i] = green_[i] = blue_[i] = i;\n>> \n>> \tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n>> \t\tlineBuffers_[i] = nullptr;\n>> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> \t}\n>> \n>> -\t/* Apply DebayerParams */\n>> -\tif (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {\n>> -\t\tconst unsigned int blackIndex =\n>> -\t\t\tparams.blackLevel * kGammaLookupSize / 256;\n>> -\t\tstd::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);\n>> -\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> -\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>> -\t\t\tgamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);\n>> -\n>> -\t\tgammaCorrection_ = params.gamma;\n>> -\t\tblackLevel_ = params.blackLevel;\n>> -\t}\n>> -\n>> -\tif (swapRedBlueGains_)\n>> -\t\tstd::swap(params.gainR, params.gainB);\n>> -\n>> -\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n>> -\t\tconstexpr unsigned int div =\n>> -\t\t\tkRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n>> -\t\tunsigned int idx;\n>> -\n>> -\t\t/* Apply gamma after gain! */\n>> -\t\tidx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n>> -\t\tred_[i] = gamma_[idx];\n>> -\n>> -\t\tidx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n>> -\t\tgreen_[i] = gamma_[idx];\n>> -\n>> -\t\tidx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n>> -\t\tblue_[i] = gamma_[idx];\n>> -\t}\n>> +\tgreen_ = params.green;\n>> +\tred_ = swapRedBlueGains_ ? params.blue : params.red;\n>> +\tblue_ = swapRedBlueGains_ ? params.red : params.blue;\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 de216fe3..be7dcdca 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>> \tvoid process2(const uint8_t *src, uint8_t *dst);\n>> \tvoid process4(const uint8_t *src, uint8_t *dst);\n>> \n>> -\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>> -\tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n>> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n>> \n>> -\tstd::array gamma_;\n>> -\tstd::array red_;\n>> -\tstd::array green_;\n>> -\tstd::array blue_;\n>> +\tDebayerParams::ColorLookupTable red_;\n>> +\tDebayerParams::ColorLookupTable green_;\n>> +\tDebayerParams::ColorLookupTable blue_;\n>> \tdebayerFn debayer0_;\n>> \tdebayerFn debayer1_;\n>> \tdebayerFn debayer2_;\n>> @@ -146,8 +143,6 @@ private:\n>> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n>> \tbool enableInputMemcpy_;\n>> \tbool swapRedBlueGains_;\n>> -\tfloat gammaCorrection_;\n>> -\tunsigned int blackLevel_;\n>> \tunsigned int measuredFrames_;\n>> \tint64_t frameProcessTime_;\n>> \t/* Skip 30 frames for things to stabilize then measure 30 frames */\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index c9b6be56..84558c4e 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -7,6 +7,7 @@\n>> \n>> #include \"libcamera/internal/software_isp/software_isp.h\"\n>> \n>> +#include \n>\n> #include \n>\n>> #include \n>> #include \n>> #include \n>> @@ -18,6 +19,7 @@\n>> #include \"libcamera/internal/framebuffer.h\"\n>> #include \"libcamera/internal/ipa_manager.h\"\n>> #include \"libcamera/internal/mapped_framebuffer.h\"\n>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> \n>> #include \"debayer_cpu.h\"\n>> \n>> @@ -63,10 +65,17 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n>> * handler\n>> */\n>> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>> -\t: debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,\n>> -\t\t\t DebayerParams::kGain10, 0.5f, 0 },\n>> -\t dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n>> +\t: dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n>> {\n>> +\tstd::array gammaTable;\n>> +\tfor (unsigned int i = 0; i < 256; i++)\n>> +\t\tgammaTable[i] = powf(i / 256.0, DebayerParams::kGamma);\n>\n> s/powf/std::pow/\n\nAnd it should be std::array here, I'll fix both.\n\n>> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> +\t\tdebayerParams_.red[i] = gammaTable[i];\n>> +\t\tdebayerParams_.green[i] = gammaTable[i];\n>> +\t\tdebayerParams_.blue[i] = gammaTable[i];\n>> +\t}\n>> +\n>> \tif (!dmaHeap_.isValid()) {\n>> \t\tLOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n>> \t\treturn;","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 42CB7BD87C\n\tfor ;\n\tThu, 30 May 2024 20:47:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68482634B6;\n\tThu, 30 May 2024 22:47:19 +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 B099061A43\n\tfor ;\n\tThu, 30 May 2024 22:47:17 +0200 (CEST)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-231-6r9HlpmROFqayVzTY6SAEQ-1; Thu, 30 May 2024 16:47:15 -0400","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-35dd0cc1a7bso114233f8f.1\n\tfor ;\n\tThu, 30 May 2024 13:47:15 -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\tffacd0b85a97d-35dd064998bsm290741f8f.91.2024.05.30.13.47.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 30 May 2024 13:47:12 -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=\"CDg9T3MW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1717102036;\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=EaQBX1m5MiTBG0v7nXgWHtdTB1WeikQ3pvdbRv7Qypw=;\n\tb=CDg9T3MWm00M3C58acGacz1Pjk5uCuwMfY2vF2AA42Oaoe4UQSKNNt7jyz25b2m4wT9Fcp\n\tz59PUoNLP3+hqu6TjxBFLcFjPNl+N+vcLnlpaqyEQFgu+LiwL0jHdYqyQ7h8PPirKJxduD\n\t233mZt1oXbFrucC+0Hg0KFNW3LOj5+0=","X-MC-Unique":"6r9HlpmROFqayVzTY6SAEQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1717102033; x=1717706833;\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=EaQBX1m5MiTBG0v7nXgWHtdTB1WeikQ3pvdbRv7Qypw=;\n\tb=lROTsM4ZXQ+E3I86pBUvapgKcBUtYfaIKSOmT4qhG+dOCSpfBwxNO6yQnPVu1jkALQ\n\tipR+7k3Vd8k2Bt5w6IVA72SbPbhs65iZQh56Siu2HJqb8kLVFFX3o868QftH9ilylh+D\n\t74M8niOsYUCrWdrXW6BpKHOI+DfaxXV2tnBaF4LpuUAD2ZQT12TpMPYNfqBXyXdcXdXo\n\t1WxT4bABcW6NCd6odLKa3GyWwFY+bPZt4zEIDnhceQX1Gln2s5loqMmwwt87Z/L4AbO+\n\t+gb6ogEchQyMM7U1YmX+dYticHVsH+WQIrt6BWPtjagUxfiL+zhMBTU8ulXFAt4lA8GX\n\tcecw==","X-Gm-Message-State":"AOJu0YxZ2TkifcUDBOzvrBJ/syV549njXeFHdg0tLim2mgfadNbgpOA8\n\thug80FSltohU+roHRKKhdr6F7kWpbqjff4W47iqiNYiUl4BwivalCi7/SRPEjIENmwy3iblFaNe\n\tU2K7UNB9lq7b72ugkXLVT64HeGJk4S7JDVz2IJnaZFFgv1n0DtTOr+kZxPSUhfB2OzeK6CXyChF\n\tpsVDc=","X-Received":["by 2002:a5d:45c7:0:b0:354:f66f:9292 with SMTP id\n\tffacd0b85a97d-35dc0099526mr2023463f8f.28.1717102033479; \n\tThu, 30 May 2024 13:47:13 -0700 (PDT)","by 2002:a5d:45c7:0:b0:354:f66f:9292 with SMTP id\n\tffacd0b85a97d-35dc0099526mr2023446f8f.28.1717102032842; \n\tThu, 30 May 2024 13:47:12 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHEqjexNX746DFAoSvLCtzzIWmrxNWnmiF/PCnIXQ2zhDYBXnUQhaCS0YzlWL1EQrFAHSlEwA==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Andrei Konovalov\n\t","Subject":"Re: [PATCH v4 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","In-Reply-To":"<20240529002036.GB19014@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 29 May 2024 03:20:36 +0300\")","References":"<20240528161126.35119-1-mzamazal@redhat.com>\n\t<20240528161126.35119-4-mzamazal@redhat.com>\n\t<20240529002036.GB19014@pendragon.ideasonboard.com>","Date":"Thu, 30 May 2024 22:47:11 +0200","Message-ID":"<87zfs7owmo.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\" "}},{"id":29679,"web_url":"https://patchwork.libcamera.org/comment/29679/","msgid":"<20240530210155.GB23937@pendragon.ideasonboard.com>","date":"2024-05-30T21:01:55","subject":"Re: [PATCH v4 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Thu, May 30, 2024 at 10:47:11PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> \n> > Hi Milan,\n> >\n> > Thank you for the patch. This is a nice change, I think the resulting\n> > code is easier to understand.\n> >\n> > On Tue, May 28, 2024 at 06:11:24PM +0200, Milan Zamazal wrote:\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> > You're also changing how gamma is handled, and that's not explained\n> > here.\n> \n> I'll add the explanation.\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> >> .../internal/software_isp/debayer_params.h | 19 +++----\n> >> src/ipa/simple/soft_simple.cpp | 50 +++++++++++++++----\n> >> src/libcamera/software_isp/debayer.cpp | 34 +++++++------\n> >> src/libcamera/software_isp/debayer_cpu.cpp | 43 +++-------------\n> >> src/libcamera/software_isp/debayer_cpu.h | 11 ++--\n> >> src/libcamera/software_isp/software_isp.cpp | 15 ++++--\n> >> 6 files changed, 92 insertions(+), 80 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..69fed1e7 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,21 @@\n> >> \n> >> #pragma once\n> >> \n> >> +#include \n> >> +#include \n> >> +\n> >> namespace libcamera {\n> >> \n> >> struct DebayerParams {\n> >> \tstatic constexpr unsigned int kGain10 = 256;\n> >> +\tstatic constexpr unsigned int kRGBLookupSize = 256;\n> >> +\tstatic constexpr float kGamma = 0.5;\n> >\n> > With this patch the gamma value isn't an ISP parameter anything, so I\n> > don't think this field belongs here. As far as I understand, the only\n> > reason why you need it is to initialize debayerParams_ in\n> > SoftwareIsp::SoftwareIsp(). \n> \n> Yes.\n> \n> > Is that needed, or does the IPA module provide ISP parameters for the\n> > first frame ?\n> \n> It's needed for the first two frames, i.e. until stats processing starts\n> providing its own parameters.\n\nCould you record that in a comment ? I think it's important information\nfor the readers. It would also be worth trying to fix that and get the\nIPA to produce initial parameters for the first frames at some point.\nThere's no urgency, recording a todo item is fine for now (if you think\nit's a good idea).\n\n> > If you need to initialize the parameters for the first frame on the\n> > pipeline handler side for the time being, I would hardcode the 0.5 value\n> > there and move the kGamma member from this structure to the IPA module.\n> \n> OK.\n> \n> >> -\tunsigned int gainR;\n> >> -\tunsigned int gainG;\n> >> -\tunsigned int gainB;\n> >> +\tusing ColorLookupTable = std::array;\n> >> \n> >> -\tfloat gamma;\n> >> -\t/**\n> >> -\t * \\brief Level of the black point, 0..255, 0 is no correction.\n> >> -\t */\n> >> -\tunsigned int blackLevel;\n> >> +\tColorLookupTable red;\n> >> +\tColorLookupTable green;\n> >> +\tColorLookupTable 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..2c9fe98e 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> >\n> > #include \n> >\n> > See Documentation/coding-style.rst\n> \n> I see, tricky :-). Could checkstyle.py catch this or is it more\n> complicated (I can see math.h is used in multiple places, including\n> documentation)?\n\nI would have sworn checkstyle.py would catch this already. I think it\nshould be added. We already have a IncludeChecker class, it should be\neasy to extend it to cover this.\n\n> >> #include \n> >> #include \n> >> #include \n> >> @@ -84,6 +85,10 @@ private:\n> >> \tControlInfoMap sensorInfoMap_;\n> >> \tBlackLevel blackLevel_;\n> >> \n> >> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> >> +\tstd::array gammaTable_;\n> >> +\tint lastBlackLevel_ = -1;\n> >> +\n> >> \tint32_t exposureMin_, exposureMax_;\n> >> \tint32_t exposure_;\n> >> \tdouble againMin_, againMax_, againMinStep_;\n> >> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> >> \tif (ignoreUpdates_ > 0)\n> >> \t\tblackLevel_.update(histogram);\n> >> \tconst uint8_t blackLevel = blackLevel_.get();\n> >> -\tparams_->blackLevel = blackLevel;\n> >> \n> >> \t/*\n> >> \t * Calculate red and blue gains for AWB.\n> >> @@ -265,12 +269,40 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> >> \tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n> >> \tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n> >> \n> >> -\tparams_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> >> -\tparams_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> >> -\n> >> +\t/* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */\n> >> +\tconst unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n> >> +\tconst unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n> >> \t/* Green gain and gamma values are fixed */\n> >> -\tparams_->gainG = 256;\n> >> -\tparams_->gamma = 0.5;\n> >> +\tconstexpr unsigned int gainG = 256;\n> >> +\n> >> +\t/* Update the gamma table if needed */\n> >> +\tif (blackLevel != lastBlackLevel_) {\n> >> +\t\tconst unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n> >> +\t\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n> >> +\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> >> +\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> >> +\t\t\tgammaTable_[i] = UINT8_MAX *\n> >> +\t\t\t\t\t powf((i - blackIndex) / divisor, DebayerParams::kGamma);\n> >\n> > s/powf/std::pow/\n> >\n> >> +\n> >> +\t\tlastBlackLevel_ = blackLevel;\n> >> +\t}\n> >> +\n> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> +\t\tconstexpr unsigned int div =\n> >> +\t\t\tDebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n> >> +\t\t\tkGammaLookupSize;\n> >> +\t\tunsigned int idx;\n> >> +\n> >> +\t\t/* Apply gamma after gain! */\n> >> +\t\tidx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n> >> +\t\tparams_->red[i] = gammaTable_[idx];\n> >> +\n> >> +\t\tidx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n> >> +\t\tparams_->green[i] = gammaTable_[idx];\n> >> +\n> >> +\t\tidx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n> >> +\t\tparams_->blue[i] = gammaTable_[idx];\n> >> +\t}\n> >> \n> >> \tsetIspParams.emit();\n> >> \n> >> @@ -291,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> >> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> >> \t */\n> >> \tconst unsigned int blackLevelHistIdx =\n> >> -\t\tparams_->blackLevel / (256 / SwIspStats::kYHistogramSize);\n> >> +\t\tblackLevel / (256 / SwIspStats::kYHistogramSize);\n> >> \tconst unsigned int histogramSize =\n> >> \t\tSwIspStats::kYHistogramSize - blackLevelHistIdx;\n> >> \tconst unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;\n> >> @@ -339,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> >> \n> >> \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> >> \t\t\t << \" exp \" << exposure_ << \" again \" << again_\n> >> -\t\t\t << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB\n> >> -\t\t\t << \" black level \" << params_->blackLevel;\n> >> +\t\t\t << \" gain R/B \" << gainR << \"/\" << gainB\n> >> +\t\t\t << \" 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..028bf27e 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,33 @@ 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> >> + * \\var DebayerParams::kGamma\n> >> + * \\brief Gamma correction, 1.0 is no correction\n> >> */\n> >> \n> >> /**\n> >> - * \\var DebayerParams::gainB\n> >> - * \\brief Blue 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::gamma\n> >> - * \\brief Gamma correction, 1.0 is no correction\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::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..c038eed4 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> >> @@ -35,7 +34,7 @@ namespace libcamera {\n> >> * \\param[in] stats Pointer to the stats object to use\n> >> */\n> >> DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> >> -\t: stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0)\n> >> +\t: stats_(std::move(stats))\n> >> {\n> >> \t/*\n> >> \t * Reading from uncached buffers may be very slow.\n> >> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> >> \t */\n> >> \tenableInputMemcpy_ = true;\n> >> \n> >> -\t/* Initialize gamma to 1.0 curve */\n> >> -\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> >> -\t\tgamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n> >> +\t/* Initialize color lookup tables */\n> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n> >> +\t\tred_[i] = green_[i] = blue_[i] = i;\n> >> \n> >> \tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n> >> \t\tlineBuffers_[i] = nullptr;\n> >> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> >> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> >> \t}\n> >> \n> >> -\t/* Apply DebayerParams */\n> >> -\tif (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {\n> >> -\t\tconst unsigned int blackIndex =\n> >> -\t\t\tparams.blackLevel * kGammaLookupSize / 256;\n> >> -\t\tstd::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);\n> >> -\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n> >> -\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n> >> -\t\t\tgamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);\n> >> -\n> >> -\t\tgammaCorrection_ = params.gamma;\n> >> -\t\tblackLevel_ = params.blackLevel;\n> >> -\t}\n> >> -\n> >> -\tif (swapRedBlueGains_)\n> >> -\t\tstd::swap(params.gainR, params.gainB);\n> >> -\n> >> -\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> >> -\t\tconstexpr unsigned int div =\n> >> -\t\t\tkRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n> >> -\t\tunsigned int idx;\n> >> -\n> >> -\t\t/* Apply gamma after gain! */\n> >> -\t\tidx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n> >> -\t\tred_[i] = gamma_[idx];\n> >> -\n> >> -\t\tidx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n> >> -\t\tgreen_[i] = gamma_[idx];\n> >> -\n> >> -\t\tidx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n> >> -\t\tblue_[i] = gamma_[idx];\n> >> -\t}\n> >> +\tgreen_ = params.green;\n> >> +\tred_ = swapRedBlueGains_ ? params.blue : params.red;\n> >> +\tblue_ = swapRedBlueGains_ ? params.red : params.blue;\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 de216fe3..be7dcdca 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> >> \tvoid process2(const uint8_t *src, uint8_t *dst);\n> >> \tvoid process4(const uint8_t *src, uint8_t *dst);\n> >> \n> >> -\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> >> -\tstatic constexpr unsigned int kRGBLookupSize = 256;\n> >> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> >> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> >> \n> >> -\tstd::array gamma_;\n> >> -\tstd::array red_;\n> >> -\tstd::array green_;\n> >> -\tstd::array blue_;\n> >> +\tDebayerParams::ColorLookupTable red_;\n> >> +\tDebayerParams::ColorLookupTable green_;\n> >> +\tDebayerParams::ColorLookupTable blue_;\n> >> \tdebayerFn debayer0_;\n> >> \tdebayerFn debayer1_;\n> >> \tdebayerFn debayer2_;\n> >> @@ -146,8 +143,6 @@ private:\n> >> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> >> \tbool enableInputMemcpy_;\n> >> \tbool swapRedBlueGains_;\n> >> -\tfloat gammaCorrection_;\n> >> -\tunsigned int blackLevel_;\n> >> \tunsigned int measuredFrames_;\n> >> \tint64_t frameProcessTime_;\n> >> \t/* Skip 30 frames for things to stabilize then measure 30 frames */\n> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> >> index c9b6be56..84558c4e 100644\n> >> --- a/src/libcamera/software_isp/software_isp.cpp\n> >> +++ b/src/libcamera/software_isp/software_isp.cpp\n> >> @@ -7,6 +7,7 @@\n> >> \n> >> #include \"libcamera/internal/software_isp/software_isp.h\"\n> >> \n> >> +#include \n> >\n> > #include \n> >\n> >> #include \n> >> #include \n> >> #include \n> >> @@ -18,6 +19,7 @@\n> >> #include \"libcamera/internal/framebuffer.h\"\n> >> #include \"libcamera/internal/ipa_manager.h\"\n> >> #include \"libcamera/internal/mapped_framebuffer.h\"\n> >> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> >> \n> >> #include \"debayer_cpu.h\"\n> >> \n> >> @@ -63,10 +65,17 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> >> * handler\n> >> */\n> >> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> >> -\t: debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,\n> >> -\t\t\t DebayerParams::kGain10, 0.5f, 0 },\n> >> -\t dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> >> +\t: dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> >> {\n> >> +\tstd::array gammaTable;\n> >> +\tfor (unsigned int i = 0; i < 256; i++)\n> >> +\t\tgammaTable[i] = powf(i / 256.0, DebayerParams::kGamma);\n> >\n> > s/powf/std::pow/\n> \n> And it should be std::array here, I'll fix both.\n> \n> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> +\t\tdebayerParams_.red[i] = gammaTable[i];\n> >> +\t\tdebayerParams_.green[i] = gammaTable[i];\n> >> +\t\tdebayerParams_.blue[i] = gammaTable[i];\n> >> +\t}\n> >> +\n> >> \tif (!dmaHeap_.isValid()) {\n> >> \t\tLOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n> >> \t\treturn;","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 50AA1BDE6B\n\tfor ;\n\tThu, 30 May 2024 21:02:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B7BA634B8;\n\tThu, 30 May 2024 23:02:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D84561A43\n\tfor ;\n\tThu, 30 May 2024 23:02:09 +0200 (CEST)","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 479292F62;\n\tThu, 30 May 2024 23:02:04 +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=\"A6aNGH/a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717102924;\n\tbh=Zir4DULscT5hGB4VE7UV5sl5/wLx2yLAqjGu6axis1c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A6aNGH/aQ1zxsdjenvB5uymkx0iYfEt+39E41K1uUpjieEU/YtZ0BBjwGhDWsuusl\n\tCALwHymLT1jjuvKQVQNe9dP3FzneERtbcgZ/eEhQd0MbuxZwumG+8rLTMRpIMKghQp\n\tHLwRi1WNr7S0JHXi78DVRLd0EsGjdoK7w0TNn45s=","Date":"Fri, 31 May 2024 00:01:55 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tAndrei Konovalov ","Subject":"Re: [PATCH v4 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","Message-ID":"<20240530210155.GB23937@pendragon.ideasonboard.com>","References":"<20240528161126.35119-1-mzamazal@redhat.com>\n\t<20240528161126.35119-4-mzamazal@redhat.com>\n\t<20240529002036.GB19014@pendragon.ideasonboard.com>\n\t<87zfs7owmo.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87zfs7owmo.fsf@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":29698,"web_url":"https://patchwork.libcamera.org/comment/29698/","msgid":"<87frtyqhdc.fsf@redhat.com>","date":"2024-05-31T12:46:07","subject":"Re: [PATCH v4 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 Laurent,\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> On Thu, May 30, 2024 at 10:47:11PM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>> \n>> > Hi Milan,\n>> >\n>> > Thank you for the patch. This is a nice change, I think the resulting\n>> > code is easier to understand.\n>> >\n>> > On Tue, May 28, 2024 at 06:11:24PM +0200, Milan Zamazal wrote:\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>> > You're also changing how gamma is handled, and that's not explained\n>> > here.\n>> \n>> I'll add the explanation.\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>> >> .../internal/software_isp/debayer_params.h | 19 +++----\n>> >> src/ipa/simple/soft_simple.cpp | 50 +++++++++++++++----\n>> >> src/libcamera/software_isp/debayer.cpp | 34 +++++++------\n>> >> src/libcamera/software_isp/debayer_cpu.cpp | 43 +++-------------\n>> >> src/libcamera/software_isp/debayer_cpu.h | 11 ++--\n>> >> src/libcamera/software_isp/software_isp.cpp | 15 ++++--\n>> >> 6 files changed, 92 insertions(+), 80 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..69fed1e7 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,21 @@\n>> >> \n>> >> #pragma once\n>> >> \n>> >> +#include \n>> >> +#include \n>> >> +\n>> >> namespace libcamera {\n>> >> \n>> >> struct DebayerParams {\n>> >> \tstatic constexpr unsigned int kGain10 = 256;\n>> >> +\tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> >> +\tstatic constexpr float kGamma = 0.5;\n>> >\n>> > With this patch the gamma value isn't an ISP parameter anything, so I\n>> > don't think this field belongs here. As far as I understand, the only\n>> > reason why you need it is to initialize debayerParams_ in\n>> > SoftwareIsp::SoftwareIsp(). \n>> \n>> Yes.\n>> \n>> > Is that needed, or does the IPA module provide ISP parameters for the\n>> > first frame ?\n>> \n>> It's needed for the first two frames, i.e. until stats processing starts\n>> providing its own parameters.\n>\n> Could you record that in a comment ? I think it's important information\n> for the readers. \n\nYes, done.\n\n> It would also be worth trying to fix that and get the IPA to produce\n> initial parameters for the first frames at some point. There's no\n> urgency, recording a todo item is fine for now (if you think it's a\n> good idea).\n\nYes, I'd definitely like to have this improved in the future refactoring\nseries. The current state is confusing.\n\n>> > If you need to initialize the parameters for the first frame on the\n>> > pipeline handler side for the time being, I would hardcode the 0.5 value\n>> > there and move the kGamma member from this structure to the IPA module.\n>> \n>> OK.\n>> \n>> >> -\tunsigned int gainR;\n>> >> -\tunsigned int gainG;\n>> >> -\tunsigned int gainB;\n>> >> +\tusing ColorLookupTable = std::array;\n>> >> \n>> >> -\tfloat gamma;\n>> >> -\t/**\n>> >> -\t * \\brief Level of the black point, 0..255, 0 is no correction.\n>> >> -\t */\n>> >> -\tunsigned int blackLevel;\n>> >> +\tColorLookupTable red;\n>> >> +\tColorLookupTable green;\n>> >> +\tColorLookupTable 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..2c9fe98e 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>> >\n>> > #include \n>> >\n>> > See Documentation/coding-style.rst\n>> \n>> I see, tricky :-). Could checkstyle.py catch this or is it more\n>> complicated (I can see math.h is used in multiple places, including\n>> documentation)?\n>\n> I would have sworn checkstyle.py would catch this already. I think it\n> should be added. We already have a IncludeChecker class, it should be\n> easy to extend it to cover this.\n>\n>> >> #include \n>> >> #include \n>> >> #include \n>> >> @@ -84,6 +85,10 @@ private:\n>> >> \tControlInfoMap sensorInfoMap_;\n>> >> \tBlackLevel blackLevel_;\n>> >> \n>> >> +\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>> >> +\tstd::array gammaTable_;\n>> >> +\tint lastBlackLevel_ = -1;\n>> >> +\n>> >> \tint32_t exposureMin_, exposureMax_;\n>> >> \tint32_t exposure_;\n>> >> \tdouble againMin_, againMax_, againMinStep_;\n>> >> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> >> \tif (ignoreUpdates_ > 0)\n>> >> \t\tblackLevel_.update(histogram);\n>> >> \tconst uint8_t blackLevel = blackLevel_.get();\n>> >> -\tparams_->blackLevel = blackLevel;\n>> >> \n>> >> \t/*\n>> >> \t * Calculate red and blue gains for AWB.\n>> >> @@ -265,12 +269,40 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> >> \tconst uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);\n>> >> \tconst uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);\n>> >> \n>> >> -\tparams_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> >> -\tparams_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> >> -\n>> >> +\t/* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */\n>> >> +\tconst unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;\n>> >> +\tconst unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;\n>> >> \t/* Green gain and gamma values are fixed */\n>> >> -\tparams_->gainG = 256;\n>> >> -\tparams_->gamma = 0.5;\n>> >> +\tconstexpr unsigned int gainG = 256;\n>> >> +\n>> >> +\t/* Update the gamma table if needed */\n>> >> +\tif (blackLevel != lastBlackLevel_) {\n>> >> +\t\tconst unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;\n>> >> +\t\tstd::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);\n>> >> +\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> >> +\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>> >> +\t\t\tgammaTable_[i] = UINT8_MAX *\n>> >> +\t\t\t\t\t powf((i - blackIndex) / divisor, DebayerParams::kGamma);\n>> >\n>> > s/powf/std::pow/\n>> >\n>> >> +\n>> >> +\t\tlastBlackLevel_ = blackLevel;\n>> >> +\t}\n>> >> +\n>> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> >> +\t\tconstexpr unsigned int div =\n>> >> +\t\t\tDebayerParams::kRGBLookupSize * DebayerParams::kGain10 /\n>> >> +\t\t\tkGammaLookupSize;\n>> >> +\t\tunsigned int idx;\n>> >> +\n>> >> +\t\t/* Apply gamma after gain! */\n>> >> +\t\tidx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });\n>> >> +\t\tparams_->red[i] = gammaTable_[idx];\n>> >> +\n>> >> +\t\tidx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });\n>> >> +\t\tparams_->green[i] = gammaTable_[idx];\n>> >> +\n>> >> +\t\tidx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });\n>> >> +\t\tparams_->blue[i] = gammaTable_[idx];\n>> >> +\t}\n>> >> \n>> >> \tsetIspParams.emit();\n>> >> \n>> >> @@ -291,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> >> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>> >> \t */\n>> >> \tconst unsigned int blackLevelHistIdx =\n>> >> -\t\tparams_->blackLevel / (256 / SwIspStats::kYHistogramSize);\n>> >> +\t\tblackLevel / (256 / SwIspStats::kYHistogramSize);\n>> >> \tconst unsigned int histogramSize =\n>> >> \t\tSwIspStats::kYHistogramSize - blackLevelHistIdx;\n>> >> \tconst unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;\n>> >> @@ -339,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> >> \n>> >> \tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n>> >> \t\t\t << \" exp \" << exposure_ << \" again \" << again_\n>> >> -\t\t\t << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB\n>> >> -\t\t\t << \" black level \" << params_->blackLevel;\n>> >> +\t\t\t << \" gain R/B \" << gainR << \"/\" << gainB\n>> >> +\t\t\t << \" 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..028bf27e 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,33 @@ 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>> >> + * \\var DebayerParams::kGamma\n>> >> + * \\brief Gamma correction, 1.0 is no correction\n>> >> */\n>> >> \n>> >> /**\n>> >> - * \\var DebayerParams::gainB\n>> >> - * \\brief Blue 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::gamma\n>> >> - * \\brief Gamma correction, 1.0 is no correction\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::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..c038eed4 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>> >> @@ -35,7 +34,7 @@ namespace libcamera {\n>> >> * \\param[in] stats Pointer to the stats object to use\n>> >> */\n>> >> DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> >> -\t: stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0)\n>> >> +\t: stats_(std::move(stats))\n>> >> {\n>> >> \t/*\n>> >> \t * Reading from uncached buffers may be very slow.\n>> >> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> >> \t */\n>> >> \tenableInputMemcpy_ = true;\n>> >> \n>> >> -\t/* Initialize gamma to 1.0 curve */\n>> >> -\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n>> >> -\t\tgamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);\n>> >> +\t/* Initialize color lookup tables */\n>> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)\n>> >> +\t\tred_[i] = green_[i] = blue_[i] = i;\n>> >> \n>> >> \tfor (unsigned int i = 0; i < kMaxLineBuffers; i++)\n>> >> \t\tlineBuffers_[i] = nullptr;\n>> >> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>> >> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> >> \t}\n>> >> \n>> >> -\t/* Apply DebayerParams */\n>> >> -\tif (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {\n>> >> -\t\tconst unsigned int blackIndex =\n>> >> -\t\t\tparams.blackLevel * kGammaLookupSize / 256;\n>> >> -\t\tstd::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);\n>> >> -\t\tconst float divisor = kGammaLookupSize - blackIndex - 1.0;\n>> >> -\t\tfor (unsigned int i = blackIndex; i < kGammaLookupSize; i++)\n>> >> -\t\t\tgamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);\n>> >> -\n>> >> -\t\tgammaCorrection_ = params.gamma;\n>> >> -\t\tblackLevel_ = params.blackLevel;\n>> >> -\t}\n>> >> -\n>> >> -\tif (swapRedBlueGains_)\n>> >> -\t\tstd::swap(params.gainR, params.gainB);\n>> >> -\n>> >> -\tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n>> >> -\t\tconstexpr unsigned int div =\n>> >> -\t\t\tkRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n>> >> -\t\tunsigned int idx;\n>> >> -\n>> >> -\t\t/* Apply gamma after gain! */\n>> >> -\t\tidx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });\n>> >> -\t\tred_[i] = gamma_[idx];\n>> >> -\n>> >> -\t\tidx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });\n>> >> -\t\tgreen_[i] = gamma_[idx];\n>> >> -\n>> >> -\t\tidx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });\n>> >> -\t\tblue_[i] = gamma_[idx];\n>> >> -\t}\n>> >> +\tgreen_ = params.green;\n>> >> +\tred_ = swapRedBlueGains_ ? params.blue : params.red;\n>> >> +\tblue_ = swapRedBlueGains_ ? params.red : params.blue;\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 de216fe3..be7dcdca 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>> >> \tvoid process2(const uint8_t *src, uint8_t *dst);\n>> >> \tvoid process4(const uint8_t *src, uint8_t *dst);\n>> >> \n>> >> -\tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>> >> -\tstatic constexpr unsigned int kRGBLookupSize = 256;\n>> >> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n>> >> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n>> >> \n>> >> -\tstd::array gamma_;\n>> >> -\tstd::array red_;\n>> >> -\tstd::array green_;\n>> >> -\tstd::array blue_;\n>> >> +\tDebayerParams::ColorLookupTable red_;\n>> >> +\tDebayerParams::ColorLookupTable green_;\n>> >> +\tDebayerParams::ColorLookupTable blue_;\n>> >> \tdebayerFn debayer0_;\n>> >> \tdebayerFn debayer1_;\n>> >> \tdebayerFn debayer2_;\n>> >> @@ -146,8 +143,6 @@ private:\n>> >> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n>> >> \tbool enableInputMemcpy_;\n>> >> \tbool swapRedBlueGains_;\n>> >> -\tfloat gammaCorrection_;\n>> >> -\tunsigned int blackLevel_;\n>> >> \tunsigned int measuredFrames_;\n>> >> \tint64_t frameProcessTime_;\n>> >> \t/* Skip 30 frames for things to stabilize then measure 30 frames */\n>> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> >> index c9b6be56..84558c4e 100644\n>> >> --- a/src/libcamera/software_isp/software_isp.cpp\n>> >> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> >> @@ -7,6 +7,7 @@\n>> >> \n>> >> #include \"libcamera/internal/software_isp/software_isp.h\"\n>> >> \n>> >> +#include \n>> >\n>> > #include \n>> >\n>> >> #include \n>> >> #include \n>> >> #include \n>> >> @@ -18,6 +19,7 @@\n>> >> #include \"libcamera/internal/framebuffer.h\"\n>> >> #include \"libcamera/internal/ipa_manager.h\"\n>> >> #include \"libcamera/internal/mapped_framebuffer.h\"\n>> >> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> >> \n>> >> #include \"debayer_cpu.h\"\n>> >> \n>> >> @@ -63,10 +65,17 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n>> >> * handler\n>> >> */\n>> >> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n>> >> -\t: debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,\n>> >> -\t\t\t DebayerParams::kGain10, 0.5f, 0 },\n>> >> -\t dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n>> >> +\t: dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n>> >> {\n>> >> +\tstd::array gammaTable;\n>> >> +\tfor (unsigned int i = 0; i < 256; i++)\n>> >> +\t\tgammaTable[i] = powf(i / 256.0, DebayerParams::kGamma);\n>> >\n>> > s/powf/std::pow/\n>> \n>> And it should be std::array here, I'll fix both.\n>> \n>> >> +\tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> >> +\t\tdebayerParams_.red[i] = gammaTable[i];\n>> >> +\t\tdebayerParams_.green[i] = gammaTable[i];\n>> >> +\t\tdebayerParams_.blue[i] = gammaTable[i];\n>> >> +\t}\n>> >> +\n>> >> \tif (!dmaHeap_.isValid()) {\n>> >> \t\tLOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n>> >> \t\treturn;","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 21249BD87C\n\tfor ;\n\tFri, 31 May 2024 12:46:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43F5D634B8;\n\tFri, 31 May 2024 14:46:15 +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 6AD94634B2\n\tfor ;\n\tFri, 31 May 2024 14:46:13 +0200 (CEST)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-691-1mhAWaqmN-q7u45wwAYG3g-1; Fri, 31 May 2024 08:46:11 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-35dcfb8ba61so856818f8f.2\n\tfor ;\n\tFri, 31 May 2024 05:46: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\tffacd0b85a97d-35dd04c9d78sm1796977f8f.27.2024.05.31.05.46.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 31 May 2024 05:46: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=\"Z0rjAHYo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1717159572;\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=93eV+E6Rx8vGrvHZ/6ZgPj84bj4r29OUtcNQXIw/5fE=;\n\tb=Z0rjAHYo7wTFy/csZfrDZiFj/gL2ya2vVraUcTwaVqSl7m29G3BpQ/2NwHMvHSY7uvNufE\n\tCh9qtSTDvWcCgk4MiqskWuKf4CdeBDwLXnnwanhEBzVcXS+/E62FiisaebhM1sn0Qop2Vz\n\t99tp3bRd2ljIgIvq+m6T4xJN4z+q77k=","X-MC-Unique":"1mhAWaqmN-q7u45wwAYG3g-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1717159569; x=1717764369;\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=93eV+E6Rx8vGrvHZ/6ZgPj84bj4r29OUtcNQXIw/5fE=;\n\tb=N8r59sIafa2K4yzi4IU8lQch0PYJEVs8fx+gK1CpFB9h++JLMBbFUkAprI7BuffSd1\n\tE7Q/rpLX0U3rIt1LFSFhy+6uBW4xccDAGM/Avt/IzH5cdFnXgXO+gH0XC15FABStF7py\n\t0J7+Vbmb2yyNCXaioh43NHcH3J5OXOqGkAT+ANrJV5XQyUaHSvZsyxYnRutPdu9F6PS0\n\tSxPidtkpYSyl0p13hNShn1uqboU4FZ/Ky/srd234CEYHK30hLNTIWDET6Hmfw61MNaxz\n\tbiSgHSxA0ehe8etEQO/Uhj72xkfnxC0Aa+USGvgweRfpkKFZ6GLcYcT8a4wlIWHo+sfI\n\tWcCA==","X-Gm-Message-State":"AOJu0YwVSNm9vJLupmCVxEsTMT5BoqTv1B9qq4KsJVptGMI88GztZeKz\n\t8cEhVsBbEzg7nMmXLqso1jrRBbx/DvlqtwOh3pfmmdZYIvJovEy1ia/R9dIPi3EaAlyvzsbVyWd\n\t5NVwJYZQwhiVywLiTMJzHLnCkTlzncYxzW+vuz8A/ATBc2gZm03ZGeVydj3u+Oz2sZVPT6ElC3Q\n\tgK4ds=","X-Received":["by 2002:a5d:42ca:0:b0:354:f304:cd36 with SMTP id\n\tffacd0b85a97d-35e0f34f5bdmr1396420f8f.70.1717159569334; \n\tFri, 31 May 2024 05:46:09 -0700 (PDT)","by 2002:a5d:42ca:0:b0:354:f304:cd36 with SMTP id\n\tffacd0b85a97d-35e0f34f5bdmr1396401f8f.70.1717159568855; \n\tFri, 31 May 2024 05:46:08 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFwPmxNKqzAP5L4IrQ1jB5iWPxfKJoconInbr6FTF3j95TOaIe/nW4SaYuAhvECgSdy4o0Fpg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Andrei Konovalov\n\t","Subject":"Re: [PATCH v4 3/5] libcamera: software_isp: Move color mappings out\n\tof debayering","In-Reply-To":"<20240530210155.GB23937@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Fri, 31 May 2024 00:01:55 +0300\")","References":"<20240528161126.35119-1-mzamazal@redhat.com>\n\t<20240528161126.35119-4-mzamazal@redhat.com>\n\t<20240529002036.GB19014@pendragon.ideasonboard.com>\n\t<87zfs7owmo.fsf@redhat.com>\n\t<20240530210155.GB23937@pendragon.ideasonboard.com>","Date":"Fri, 31 May 2024 14:46:07 +0200","Message-ID":"<87frtyqhdc.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\" "}}]