[{"id":29108,"web_url":"https://patchwork.libcamera.org/comment/29108/","msgid":"<20240327185543.GN4721@pendragon.ideasonboard.com>","date":"2024-03-27T18:55:43","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\n(CC'ing David)\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:36:04PM +0100, Milan Zamazal wrote:\n> Black may not be represented as 0 pixel value for given hardware, it may be\n> higher. If this is not compensated then various problems may occur such as low\n> contrast or suboptimal exposure.\n> \n> The black pixel value can be either retrieved from a tuning file for the given\n> hardware, or automatically on fly. The former is the right and correct method,\n\ns/on fly/on the fly/\n\n> while the latter can be used when a tuning file is not available for the given\n> hardware. Since there is currently no support for tuning files in software ISP,\n> the automatic, hardware independent way, is always used. Support for tuning\n> files should be added in future but it will require more work than this patch.\n\nI don't think this is quite right. Strictly speaking, the black level\ncompensation is about subtracting the black level as produced by the\nsensor. This requires tuning, and shouldn't be done on-the-fly.\n\nOn the other hand, looking at the histogram to try and stretch it is\ncalled contrast adjustment. There's nothing wrong in doing so, but it's\nusually done towards the output of the processing pipeline, and is\nassociated with manual adjustments of the contrast and brightness. See\nsrc/ipa/rpi/controller/rpi/contrast.cpp for instance.\n\nDavid may be able to comment further as to why BLC and contrast are\nquite different.\n\nAs this patch may need more work, I propose not including it in v7 to\navoid delay merging the rest of the implementation.\n\n> The patch looks at the image histogram and assumes that black starts when pixel\n> values start occurring on the left. A certain amount of the darkest pixels is\n> ignored; it doesn't matter whether they represent various kinds of noise or are\n> real, they are better to omit in any case to make the image looking better. It\n> also doesn't matter whether the darkest pixels occur around the supposed black\n> level or are spread between 0 and the black level, the difference is not\n> important.\n> \n> An arbitrary threshold of 2% darkest pixels is applied; there is no magic about\n> that value.\n> \n> The patch assumes that the black values for different colors are the same and\n> doesn't attempt any other non-primitive enhancements. It cannot completely\n> replace tuning files and simplicity, while providing visible benefit, is its\n> goal. Anything more sophisticated is left for future patches.\n> \n> A possible cheap enhancement, if needed, could be setting exposure + gain to\n> minimum values temporarily, before setting the black level. In theory, the\n> black level should be fixed but it may not be reached in all images. For this\n> reason, the patch updates black level only if the observed value is lower than\n> the current one; it should be never increased.\n> \n> The purpose of the patch is to compensate for hardware properties. General\n> image contrast enhancements are out of scope of this patch.\n> \n> Stats are still gathered as an uncorrected histogram, to avoid any confusion and\n> to represent the raw image data. Exposure must be determined after the black\n> level correction -- it has no influence on the sub-black area and must be\n> correct after applying the black level correction. The granularity of the\n> histogram is increased from 16 to 64 to provide a better precision (there is no\n> theory behind either of those numbers).\n> \n> Reviewed-by: Hans de Goede \n> Signed-off-by: Milan Zamazal \n> Signed-off-by: Hans de Goede \n> ---\n> .../internal/software_isp/debayer_params.h | 4 +\n> .../internal/software_isp/swisp_stats.h | 10 ++-\n> src/ipa/simple/black_level.cpp | 88 +++++++++++++++++++\n> src/ipa/simple/black_level.h | 28 ++++++\n> src/ipa/simple/meson.build | 7 +-\n> src/ipa/simple/soft_simple.cpp | 28 ++++--\n> src/libcamera/software_isp/debayer_cpu.cpp | 13 ++-\n> src/libcamera/software_isp/debayer_cpu.h | 1 +\n> src/libcamera/software_isp/software_isp.cpp | 2 +-\n> 9 files changed, 164 insertions(+), 17 deletions(-)\n> create mode 100644 src/ipa/simple/black_level.cpp\n> create mode 100644 src/ipa/simple/black_level.h\n> \n> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\n> index 98965fa1..5e38e08b 100644\n> --- a/include/libcamera/internal/software_isp/debayer_params.h\n> +++ b/include/libcamera/internal/software_isp/debayer_params.h\n> @@ -43,6 +43,10 @@ struct DebayerParams {\n> \t * \\brief Gamma correction, 1.0 is no correction\n> \t */\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> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h\n> index afe42c9a..25cd5abd 100644\n> --- a/include/libcamera/internal/software_isp/swisp_stats.h\n> +++ b/include/libcamera/internal/software_isp/swisp_stats.h\n> @@ -7,6 +7,8 @@\n> \n> #pragma once\n> \n> +#include \n> +\n> namespace libcamera {\n> \n> /**\n> @@ -28,11 +30,15 @@ struct SwIspStats {\n> \t/**\n> \t * \\brief Number of bins in the yHistogram.\n> \t */\n> -\tstatic constexpr unsigned int kYHistogramSize = 16;\n> +\tstatic constexpr unsigned int kYHistogramSize = 64;\n> +\t/**\n> +\t * \\brief Type of the histogram.\n> +\t */\n> +\tusing histogram = std::array;\n> \t/**\n> \t * \\brief A histogram of luminance values.\n> \t */\n> -\tstd::array yHistogram;\n> +\thistogram yHistogram;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp\n> new file mode 100644\n> index 00000000..9d4aa800\n> --- /dev/null\n> +++ b/src/ipa/simple/black_level.cpp\n> @@ -0,0 +1,88 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * black_level.cpp - black level handling\n> + */\n> +\n> +#include \"black_level.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoftBL)\n> +\n> +/**\n> + * \\class BlackLevel\n> + * \\brief Object providing black point level for software ISP\n> + *\n> + * Black level can be provided in hardware tuning files or, if no tuning file is\n> + * available for the given hardware, guessed automatically, with less accuracy.\n> + * As tuning files are not yet implemented for software ISP, BlackLevel\n> + * currently provides only guessed black levels.\n> + *\n> + * This class serves for tracking black level as a property of the underlying\n> + * hardware, not as means of enhancing a particular scene or image.\n> + *\n> + * The class is supposed to be instantiated for the given camera stream.\n> + * The black level can be retrieved using BlackLevel::get() method. It is\n> + * initially 0 and may change when updated using BlackLevel::update() method.\n> + */\n> +\n> +BlackLevel::BlackLevel()\n> +\t: blackLevel_(255), blackLevelSet_(false)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Return the current black level\n> + *\n> + * \\return The black level, in the range from 0 (minimum) to 255 (maximum).\n> + * If the black level couldn't be determined yet, return 0.\n> + */\n> +unsigned int BlackLevel::get() const\n> +{\n> +\treturn blackLevelSet_ ? blackLevel_ : 0;\n> +}\n> +\n> +/**\n> + * \\brief Update black level from the provided histogram\n> + * \\param[in] yHistogram The histogram to be used for updating black level\n> + *\n> + * The black level is property of the given hardware, not image. It is updated\n> + * only if it has not been yet set or if it is lower than the lowest value seen\n> + * so far.\n> + */\n> +void BlackLevel::update(SwIspStats::histogram &yHistogram)\n> +{\n> +\t/*\n> +\t * The constant is selected to be \"good enough\", not overly conservative or\n> +\t * aggressive. There is no magic about the given value.\n> +\t */\n> +\tconstexpr float ignoredPercentage_ = 0.02;\n> +\tconst unsigned int total =\n> +\t\tstd::accumulate(begin(yHistogram), end(yHistogram), 0);\n> +\tconst unsigned int pixelThreshold = ignoredPercentage_ * total;\n> +\tconst unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;\n> +\tconst unsigned int currentBlackIdx = blackLevel_ / histogramRatio;\n> +\n> +\tfor (unsigned int i = 0, seen = 0;\n> +\t i < currentBlackIdx && i < SwIspStats::kYHistogramSize;\n> +\t i++) {\n> +\t\tseen += yHistogram[i];\n> +\t\tif (seen >= pixelThreshold) {\n> +\t\t\tblackLevel_ = i * histogramRatio;\n> +\t\t\tblackLevelSet_ = true;\n> +\t\t\tLOG(IPASoftBL, Debug)\n> +\t\t\t\t<< \"Auto-set black level: \"\n> +\t\t\t\t<< i << \"/\" << SwIspStats::kYHistogramSize\n> +\t\t\t\t<< \" (\" << 100 * (seen - yHistogram[i]) / total << \"% below, \"\n> +\t\t\t\t<< 100 * seen / total << \"% at or below)\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t};\n> +}\n> +} // namespace libcamera\n\n} /* namespace libcamera */\n\nSame below.\n\n> diff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h\n> new file mode 100644\n> index 00000000..b3785db0\n> --- /dev/null\n> +++ b/src/ipa/simple/black_level.h\n> @@ -0,0 +1,28 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * black_level.h - black level handling\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> +\n> +namespace libcamera {\n> +\n> +class BlackLevel\n\nWouldn't it be better to move to using the libipa Algorithm class before\nintroducing new algorithms ?\n\n> +{\n> +public:\n> +\tBlackLevel();\n> +\tunsigned int get() const;\n> +\tvoid update(std::array &yHistogram);\n> +\n> +private:\n> +\tunsigned int blackLevel_;\n> +\tbool blackLevelSet_;\n> +};\n> +\n> +} // namespace libcamera\n> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> index 3e863db7..44b5f1d7 100644\n> --- a/src/ipa/simple/meson.build\n> +++ b/src/ipa/simple/meson.build\n> @@ -2,8 +2,13 @@\n> \n> ipa_name = 'ipa_soft_simple'\n> \n> +soft_simple_sources = files([\n> + 'soft_simple.cpp',\n> + 'black_level.cpp',\n> +])\n> +\n> mod = shared_module(ipa_name,\n> - ['soft_simple.cpp', libcamera_generated_ipa_headers],\n> + [soft_simple_sources, libcamera_generated_ipa_headers],\n> name_prefix : '',\n> include_directories : [ipa_includes, libipa_includes],\n> dependencies : libcamera_private,\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index 312df4ba..ac027568 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -22,6 +22,8 @@\n> #include \"libcamera/internal/software_isp/debayer_params.h\"\n> #include \"libcamera/internal/software_isp/swisp_stats.h\"\n> \n> +#include \"black_level.h\"\n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPASoft)\n> @@ -33,7 +35,8 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface\n> public:\n> \tIPASoftSimple()\n> \t\t: params_(static_cast(MAP_FAILED)),\n> -\t\t stats_(static_cast(MAP_FAILED)), ignore_updates_(0)\n> +\t\t stats_(static_cast(MAP_FAILED)),\n> +\t\t blackLevel_(BlackLevel()), ignore_updates_(0)\n> \t{\n> \t}\n> \n> @@ -63,6 +66,7 @@ private:\n> \tSharedFD fdParams_;\n> \tDebayerParams *params_;\n> \tSwIspStats *stats_;\n> +\tBlackLevel blackLevel_;\n> \n> \tint32_t exposure_min_, exposure_max_;\n> \tint32_t again_min_, again_max_;\n> @@ -196,6 +200,10 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \tparams_->gainG = 256;\n> \tparams_->gamma = 0.5;\n> \n> +\tif (ignore_updates_ > 0)\n> +\t\tblackLevel_.update(stats_->yHistogram);\n> +\tparams_->blackLevel = blackLevel_.get();\n> +\n> \tsetIspParams.emit(0);\n> \n> \t/*\n> @@ -211,18 +219,19 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \t * Calculate Mean Sample Value (MSV) according to formula from:\n> \t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> \t */\n> -\tconstexpr unsigned int yHistValsPerBin =\n> -\t\tSwIspStats::kYHistogramSize / kExposureBinsCount;\n> -\tconstexpr unsigned int yHistValsPerBinMod =\n> -\t\tSwIspStats::kYHistogramSize /\n> -\t\t(SwIspStats::kYHistogramSize % kExposureBinsCount + 1);\n> +\tconst unsigned int blackLevelHistIdx =\n> +\t\tparams_->blackLevel / (256 / SwIspStats::kYHistogramSize);\n> +\tconst unsigned int histogramSize = SwIspStats::kYHistogramSize - blackLevelHistIdx;\n> +\tconst unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;\n> +\tconst unsigned int yHistValsPerBinMod =\n> +\t\thistogramSize / (histogramSize % kExposureBinsCount + 1);\n> \tint ExposureBins[kExposureBinsCount] = {};\n> \tunsigned int denom = 0;\n> \tunsigned int num = 0;\n> \n> -\tfor (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {\n> +\tfor (unsigned int i = 0; i < histogramSize; i++) {\n> \t\tunsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;\n> -\t\tExposureBins[idx] += stats_->yHistogram[i];\n> +\t\tExposureBins[idx] += stats_->yHistogram[blackLevelHistIdx + i];\n> \t}\n> \n> \tfor (unsigned int i = 0; i < kExposureBinsCount; i++) {\n> @@ -256,7 +265,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 << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB\n> +\t\t\t << \" black level \" << params_->blackLevel;\n> }\n> \n> void IPASoftSimple::updateExposure(double exposureMSV)\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index a1692693..3be3cdfe 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -35,7 +35,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)), gamma_correction_(1.0)\n> +\t: stats_(std::move(stats)), gamma_correction_(1.0), blackLevel_(0)\n> {\n> #ifdef __x86_64__\n> \tenableInputMemcpy_ = false;\n> @@ -683,11 +683,16 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> \t}\n> \n> \t/* Apply DebayerParams */\n> -\tif (params.gamma != gamma_correction_) {\n> -\t\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> -\t\t\tgamma_[i] = UINT8_MAX * powf(i / (kGammaLookupSize - 1.0), params.gamma);\n> +\tif (params.gamma != gamma_correction_ || 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\tgamma_correction_ = params.gamma;\n> +\t\tblackLevel_ = params.blackLevel;\n> \t}\n> \n> \tif (swapRedBlueGains_)\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 5f44fc65..ea02f909 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -147,6 +147,7 @@ private:\n> \tbool enableInputMemcpy_;\n> \tbool swapRedBlueGains_;\n> \tfloat gamma_correction_;\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 388b4496..9b49be41 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -64,7 +64,7 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> */\n> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls)\n> \t: debayer_(nullptr),\n> -\t debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },\n> +\t debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f, 0 },\n> \t dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> {\n> \tif (!dmaHeap_.isValid()) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DDCA9BD160\n\tfor ;\n\tWed, 27 Mar 2024 18:55:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 769426334D;\n\tWed, 27 Mar 2024 19:55:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6115261C35\n\tfor ;\n\tWed, 27 Mar 2024 19:55:52 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86828675;\n\tWed, 27 Mar 2024 19:55:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aiMqIF8F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711565719;\n\tbh=KRrwEOl6mjvkqVrAKc4pRiNWsVPyCi3Rkj3hkHE8fBM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aiMqIF8FhQ/dB/peJ2amGcDQS/D2I95GHf+1/LqtbzdPq9KL3KVbPQJGpgOkj3Ei0\n\tCxEylY7Sz4+678cX0Lp6Z2VoHWbtg1nV9+5nOeSlczeNfUsbE/gDxWJy89ywXsHf2C\n\tR+9M+tccIbGO5eA6OhyqoS8oTD3hlL5Aqf9I9xj8=","Date":"Wed, 27 Mar 2024 20:55:43 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDavid Plowman ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"<20240327185543.GN4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-18-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":29121,"web_url":"https://patchwork.libcamera.org/comment/29121/","msgid":"<87h6gqis2p.fsf@redhat.com>","date":"2024-03-28T10:11:42","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","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 the review. \n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> (CC'ing David)\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 19, 2024 at 01:36:04PM +0100, Milan Zamazal wrote:\n>> Black may not be represented as 0 pixel value for given hardware, it may be\n>> higher. If this is not compensated then various problems may occur such as low\n>> contrast or suboptimal exposure.\n>> \n>> The black pixel value can be either retrieved from a tuning file for the given\n>> hardware, or automatically on fly. The former is the right and correct method,\n>\n> s/on fly/on the fly/\n>\n>> while the latter can be used when a tuning file is not available for the given\n>> hardware. Since there is currently no support for tuning files in software ISP,\n>> the automatic, hardware independent way, is always used. Support for tuning\n>> files should be added in future but it will require more work than this patch.\n>\n> I don't think this is quite right. Strictly speaking, the black level\n> compensation is about subtracting the black level as produced by the\n> sensor. This requires tuning, and shouldn't be done on-the-fly.\n\nA general agreement from those who tried the patch was that it makes a\nnoticeable improvement. As explained in my comments above, this is a fallback\nmechanism that we can use cheaply and immediately, until or unless tuning file\nfor software ISP is available. So far, it seems to be beneficial rather than\nharmful.\n\nUnless we have something better or perfect soon, which doesn't seem to be the\ncase to me, why not to use this simple and useful (I believe) fallback\nmechanism? Do you have a better suggestion what could be done right now\nregarding this issue (not considering black level at all is a real issue)?\n\n> On the other hand, looking at the histogram to try and stretch it is\n> called contrast adjustment. There's nothing wrong in doing so, but it's\n> usually done towards the output of the processing pipeline, and is\n> associated with manual adjustments of the contrast and brightness. See\n> src/ipa/rpi/controller/rpi/contrast.cpp for instance.\n\nContrast adjustment is a different issue than the one this patch tries to\naddress and less important now. I plan to work on it and similar functionality\nlater.\n\n> David may be able to comment further as to why BLC and contrast are\n> quite different.\n\nI think I understand the difference but feel free to comment if you think I'm\nmissing something.\n\n> As this patch may need more work, I propose not including it in v7 to\n> avoid delay merging the rest of the implementation.\n\nPlease consider my comments above. I'm open to suggestions how to do better at\nthe moment. Less comfortable with nothing should be done about black level\nuntil we have a perfect solution.\n\nThanks,\nMilan","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 CADE1C0DA4\n\tfor ;\n\tThu, 28 Mar 2024 10:11:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4848163339;\n\tThu, 28 Mar 2024 11:11:50 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B48D61C39\n\tfor ;\n\tThu, 28 Mar 2024 11:11:48 +0100 (CET)","from mail-lj1-f197.google.com (mail-lj1-f197.google.com\n\t[209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-159-b9VP9mR1PKepnE1KtXeQ_g-1; Thu, 28 Mar 2024 06:11:45 -0400","by mail-lj1-f197.google.com with SMTP id\n\t38308e7fff4ca-2d6fee84ad1so6474571fa.0\n\tfor ;\n\tThu, 28 Mar 2024 03:11:45 -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\te10-20020a05600c4e4a00b00414112a6159sm1799084wmq.44.2024.03.28.03.11.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 28 Mar 2024 03:11:43 -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=\"Vivhch7/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1711620707;\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=dn/w1L9lN41mv09JJPGGd8DjgDE5706eAdid9h8QwP8=;\n\tb=Vivhch7/n3DcYby/dTZuMzdFX3PHKZO56/NxLs9MaCMsq1S6kCOA+xYvj9PWElkzFeHaNt\n\t4o0suWs5ZniIPTRflUv7rW2V30qhJMnprbfks0ibQxyIsD/jK9jjc9z1DrohT38DBcq+/u\n\t80y9RJfnEIcLhHtEhz3G4jNJDJZf8ew=","X-MC-Unique":"b9VP9mR1PKepnE1KtXeQ_g-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711620704; x=1712225504;\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=dn/w1L9lN41mv09JJPGGd8DjgDE5706eAdid9h8QwP8=;\n\tb=fbpa+JHWfbF1ulITn4cWRdI7HBNr30uYcXgVv6cGndkMKJuI1N+lZFhR3/5ntrPyo7\n\tTZIPgFdmJ2kEoGtrsU8AhadDQN+OQSFvGi5Cr7lcYQo4P5AS+ZvWugUa6pBORPmBUhZS\n\tvHLvrSKO79Bm9rPlusoQ/LGCho39uZzt70cX1WXAjcc9l2uXDJHjama00tPUL+F8JcZI\n\tTkKgAWz60iHhYhsygLhN8AbaeEXsSichXe0/lBOgKIOjJuIxKZGMTan6oWV2LkeCnoEz\n\teGIIsMWa4X+6W4Xf4B8BwtP9HaufshfkpHblaKsBlzWRpRof90diVCpDkvyHTbX63OPu\n\tEIXw==","X-Gm-Message-State":"AOJu0Yy7owagqLQDtqIbQV/RxXzmRfen5Y1rbfEMXw55ho3PCc3k1gHS\n\tshk0Rj584zg0KHg6F85ltBG5OE7aljkFQaCydtFT6o0L+Dk6sDzJ4yQS1PLcLuPVmsSTqObACCu\n\tq6+LiHvmfU8EemLysubrR40vA7Z+vw/t5ZfwCaW9EAwyccm4Y7ukX07/XDKDZyb6B+cdU5kk=","X-Received":["by 2002:a2e:9ec3:0:b0:2d4:78ac:1168 with SMTP id\n\th3-20020a2e9ec3000000b002d478ac1168mr1237729ljk.32.1711620704382; \n\tThu, 28 Mar 2024 03:11:44 -0700 (PDT)","by 2002:a2e:9ec3:0:b0:2d4:78ac:1168 with SMTP id\n\th3-20020a2e9ec3000000b002d478ac1168mr1237714ljk.32.1711620703814; \n\tThu, 28 Mar 2024 03:11:43 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHmNLHnJsP+pWjzGXCWwtmxakt/QL71kUXJd3Ff1E0m48VO5UOYJ7nunHrSIbL9iQty9a9Yyw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Andrey Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham , David Plowman\n\t","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","In-Reply-To":"<20240327185543.GN4721@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Mar 2024 20:55:43 +0200\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>","Date":"Thu, 28 Mar 2024 11:11:42 +0100","Message-ID":"<87h6gqis2p.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":29123,"web_url":"https://patchwork.libcamera.org/comment/29123/","msgid":"","date":"2024-03-28T11:51:35","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nThanks for cc'ing me on this question. Always happy to try and answer!\n\nOn Thu, 28 Mar 2024 at 10:11, Milan Zamazal wrote:\n>\n> Hi Laurent,\n>\n> thank you for the review.\n>\n> Laurent Pinchart writes:\n>\n> > Hi Milan,\n> >\n> > (CC'ing David)\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Mar 19, 2024 at 01:36:04PM +0100, Milan Zamazal wrote:\n> >> Black may not be represented as 0 pixel value for given hardware, it may be\n> >> higher. If this is not compensated then various problems may occur such as low\n> >> contrast or suboptimal exposure.\n> >>\n> >> The black pixel value can be either retrieved from a tuning file for the given\n> >> hardware, or automatically on fly. The former is the right and correct method,\n> >\n> > s/on fly/on the fly/\n> >\n> >> while the latter can be used when a tuning file is not available for the given\n> >> hardware. Since there is currently no support for tuning files in software ISP,\n> >> the automatic, hardware independent way, is always used. Support for tuning\n> >> files should be added in future but it will require more work than this patch.\n> >\n> > I don't think this is quite right. Strictly speaking, the black level\n> > compensation is about subtracting the black level as produced by the\n> > sensor. This requires tuning, and shouldn't be done on-the-fly.\n>\n> A general agreement from those who tried the patch was that it makes a\n> noticeable improvement. As explained in my comments above, this is a fallback\n> mechanism that we can use cheaply and immediately, until or unless tuning file\n> for software ISP is available. So far, it seems to be beneficial rather than\n> harmful.\n>\n> Unless we have something better or perfect soon, which doesn't seem to be the\n> case to me, why not to use this simple and useful (I believe) fallback\n> mechanism? Do you have a better suggestion what could be done right now\n> regarding this issue (not considering black level at all is a real issue)?\n\nThis is of course partly a technical issue - what's the right thing to\ndo? - and particularly a pragmatic issue - whether to do something\nimperfect right now or not.\n\nTo tackle the technical question, black level is normally removed\nfairly early in the pipeline, because not doing so will make typical\noperations in the Bayer pipe go wrong. For example:\n\n* Lens shading (vignetting correction). I don't know whether you have\nthis feature currently, but maybe it will be required at some point to\nimprove image quality? Doing this without first removing the black\nlevel will make all the colours go wrong.\n\n* White balance. This too will go wrong if you haven't subtracted the\nblack level. White balance is normally applied before demosaic because\ndemosaic typically works better with white-balanced pixels. Obviously\nit depends what your demosaic algorithm does, but I wonder if there's\na future in which you have higher quality versions of some algorithms\nfor certain use cases (e.g. stills capture) where this could become an\nissue.\n\n* Colour correction. Like white balance, colour correction matrices\nwill not do the right thing if black level has not been removed.\n\nYou can probably guess that I belong more to the \"calibrate the camera\nproperly\" view of the world because doing that fixes problems and\nmakes them go away _forever_. But I'm not familiar with the background\nhere, so I can appreciate that interim solutions may have a place.\n\n>\n> > On the other hand, looking at the histogram to try and stretch it is\n> > called contrast adjustment. There's nothing wrong in doing so, but it's\n> > usually done towards the output of the processing pipeline, and is\n> > associated with manual adjustments of the contrast and brightness. See\n> > src/ipa/rpi/controller/rpi/contrast.cpp for instance.\n>\n> Contrast adjustment is a different issue than the one this patch tries to\n> address and less important now. I plan to work on it and similar functionality\n> later.\n>\n> > David may be able to comment further as to why BLC and contrast are\n> > quite different.\n\nWe treat contrast adjustment as a more cosmetic feature of an image\nwhich users can adjust according to their own personal taste. They can\nhave more of it or less of it, as they wish, and it doesn't really\naffect the other aspects of image, such as colours or image quality.\n\nBlack level is quite different. There's a known value that you should\nuse. There's basically no choice. Speaking for the Pi, I would be\nhighly reluctant to couple these two things together - you certainly\nwouldn't want contrast adjustment to start changing the colours, for\nexample!\n\nAnyway, I hope that provides a little bit of a comparison, at least\nfrom the point of view of a different implementation. Good luck!\n\nDavid\n\n>\n> I think I understand the difference but feel free to comment if you think I'm\n> missing something.\n>\n> > As this patch may need more work, I propose not including it in v7 to\n> > avoid delay merging the rest of the implementation.\n>\n> Please consider my comments above. I'm open to suggestions how to do better at\n> the moment. Less comfortable with nothing should be done about black level\n> until we have a perfect solution.\n>\n> Thanks,\n> Milan\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 DA606C0DA4\n\tfor ;\n\tThu, 28 Mar 2024 11:51:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 144896333B;\n\tThu, 28 Mar 2024 12:51:50 +0100 (CET)","from mail-qv1-xf33.google.com (mail-qv1-xf33.google.com\n\t[IPv6:2607:f8b0:4864:20::f33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 054BB632EA\n\tfor ;\n\tThu, 28 Mar 2024 12:51:48 +0100 (CET)","by mail-qv1-xf33.google.com with SMTP id\n\t6a1803df08f44-69185f093f5so5322916d6.3\n\tfor ;\n\tThu, 28 Mar 2024 04:51:47 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"lB0fpdWS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1711626707; x=1712231507;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=6ICNAd2oFbfgdhjgM5P43zoU8bs1c980s3+YbNhbNQo=;\n\tb=lB0fpdWSpzqzYZECzQQJ3L999ddiK2uayc6v2Qtzu61fEKA75/vfBpVohTE+4rsQwP\n\t4Knfns9JCAG765BGgJevHZO4gEYiA2G2ueMw4RMnWoysGS6c/vmuN6mITsC7lYMju6uk\n\tFiX7e2R8LWNFnxEX5N4pKp6okBKjtLS/j3V91bRWUJk6XXIeZv/oeilZtWBqiD9fxkp4\n\tkZph0hnjiCm7Q1xQyQK/CyKTeUYNwv3ESVsUJD10L8KoutJSoMgX2RaPUIyHMxgj5D9M\n\tKPrSHytOoSXMeJ1jujOETnM9SsW04RU9us1m27SJraXB5foz/p+OTM3Nl3Pnc+KNvQae\n\toqHA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711626707; x=1712231507;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=6ICNAd2oFbfgdhjgM5P43zoU8bs1c980s3+YbNhbNQo=;\n\tb=PKDMC+pW5skBBn6zP2Nv9PqxwWU/iPZfhzrd+ey6aBhI02xrWhcvhOWh8ATQMAyYqF\n\t5hYSBDG1o6V9pZAL/0XPVbQSPWorAQqeoeHjmDRxbm51RSdu1PVuDAwTVt7bh8Nv9oHz\n\t7Mkccj8f/LJ/RZj+7iXd+NUcrT+DqF7tmMIFDYfhZl24p7pjuW6MKTwjLevKjfkwSWwk\n\twx627StgvFA5kD4lYfr2ceLKBtwZFWOA81EIgj+waD/d7ZkVkMFaIIBmhCOUnRIhsJS5\n\tQFOmAPiy9NEJQjUJD++0cOPGlJbJ+tY5dPzlfgm02vRIKv2Lgl+7A/ust6cmqwI884b9\n\t5HzQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCV1mBPjBNtSwO45JALbXHkUOxnh9H8dsKrEXlgUohhBgLqoOCgCmnBkCsBl10Ekrf7VoO8bnTV3POMqSYwTcU+4Auf++cL7ytjsE6YquTkyyJxa2w==","X-Gm-Message-State":"AOJu0YyFrJoCmcJSYT11tmlXoJqqtYMg9FASPRRfFjk9zNbB+jPv16U5\n\tp3Nxopl0M1xwYErVE0GhdS7H0m1uhhNE2nV2sdOw/GmM7y3N24musCVJo1bWFB+oxzDAM8+atrl\n\tZng6tvPc/J6vu3wYwODtB1Ouhd9HC6FwlQDoTdw==","X-Google-Smtp-Source":"AGHT+IG8ThaeYTXyGFltdPhrEmyUSN5tN39DzRkIsDLwjeLg0EKZnW7CESmaQNoV/BcjJfEs6dmUrdBHekqj4HWbOX0=","X-Received":"by 2002:a0c:aa5d:0:b0:696:6712:66a0 with SMTP id\n\te29-20020a0caa5d000000b00696671266a0mr2083086qvb.61.1711626706705;\n\tThu, 28 Mar 2024 04:51:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>","In-Reply-To":"<87h6gqis2p.fsf@redhat.com>","From":"David Plowman ","Date":"Thu, 28 Mar 2024 11:51:35 +0000","Message-ID":"","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","To":"Milan Zamazal ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, \n\tAndrey Konovalov , \n\t\"Bryan O'Donoghue\" ,\n\tMaxime Ripard , \n\tPavel Machek , Hans de Goede , \n\tKieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","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":29124,"web_url":"https://patchwork.libcamera.org/comment/29124/","msgid":"<87le62h0qj.fsf@redhat.com>","date":"2024-03-28T14:47:32","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"David Plowman writes:\n\n> Hi everyone\n>\n> Thanks for cc'ing me on this question. Always happy to try and answer!\n\nHi David,\n\nthank you for your answer and explanations.\n\n> On Thu, 28 Mar 2024 at 10:11, Milan Zamazal wrote:\n>>\n>> Hi Laurent,\n>>\n>> thank you for the review.\n>>\n>> Laurent Pinchart writes:\n>>\n>> > Hi Milan,\n>> >\n>> > (CC'ing David)\n>> >\n>> > Thank you for the patch.\n>> >\n>> > On Tue, Mar 19, 2024 at 01:36:04PM +0100, Milan Zamazal wrote:\n>> >> Black may not be represented as 0 pixel value for given hardware, it may be\n>> >> higher. If this is not compensated then various problems may occur such as low\n>> >> contrast or suboptimal exposure.\n>> >>\n>> >> The black pixel value can be either retrieved from a tuning file for the given\n>> >> hardware, or automatically on fly. The former is the right and correct method,\n>> >\n>> > s/on fly/on the fly/\n>> >\n>> >> while the latter can be used when a tuning file is not available for the given\n>> >> hardware. Since there is currently no support for tuning files in software ISP,\n>> >> the automatic, hardware independent way, is always used. Support for tuning\n>> >> files should be added in future but it will require more work than this patch.\n>> >\n>> > I don't think this is quite right. Strictly speaking, the black level\n>> > compensation is about subtracting the black level as produced by the\n>> > sensor. This requires tuning, and shouldn't be done on-the-fly.\n>>\n>> A general agreement from those who tried the patch was that it makes a\n>> noticeable improvement. As explained in my comments above, this is a fallback\n>> mechanism that we can use cheaply and immediately, until or unless tuning file\n>> for software ISP is available. So far, it seems to be beneficial rather than\n>> harmful.\n>>\n>> Unless we have something better or perfect soon, which doesn't seem to be the\n>> case to me, why not to use this simple and useful (I believe) fallback\n>> mechanism? Do you have a better suggestion what could be done right now\n>> regarding this issue (not considering black level at all is a real issue)?\n>\n> This is of course partly a technical issue - what's the right thing to\n> do? - and particularly a pragmatic issue - whether to do something\n> imperfect right now or not.\n\nYes.\n\n> To tackle the technical question, black level is normally removed\n> fairly early in the pipeline, because not doing so will make typical\n> operations in the Bayer pipe go wrong. For example:\n>\n> * Lens shading (vignetting correction). I don't know whether you have\n> this feature currently, but maybe it will be required at some point to\n> improve image quality? Doing this without first removing the black\n> level will make all the colours go wrong.\n>\n> * White balance. This too will go wrong if you haven't subtracted the\n> black level. White balance is normally applied before demosaic because\n> demosaic typically works better with white-balanced pixels. Obviously\n> it depends what your demosaic algorithm does, but I wonder if there's\n> a future in which you have higher quality versions of some algorithms\n> for certain use cases (e.g. stills capture) where this could become an\n> issue.\n>\n> * Colour correction. Like white balance, colour correction matrices\n> will not do the right thing if black level has not been removed.\n\nWe don't have lens shading or color correction, once we have support for these\nthere is no reason not to have proper black level correction as well. As for\nwhite balance, we currently use probably the most primitive method possible\n(green/red and green/blue ratios) and we don't subtract black level even with\nthis patch (which should get fixed later). The patch subtracts black level when\ncorrecting exposure though, which is also influenced by this. Considering all\nthe imperfections at the moment, the actual effects on white balance and\nexposure may not be that big to care very much. But even then ...\n\n> You can probably guess that I belong more to the \"calibrate the camera\n> properly\" view of the world because doing that fixes problems and\n> makes them go away _forever_. But I'm not familiar with the background\n> here, so I can appreciate that interim solutions may have a place.\n\n... the thing is that not applying any black level correction, even when not\nconsidering white balance and exposure deformations, often makes the image\nlooking noticeably flat as there are no blacks. I'd say better imperfect blacks\nthan no blacks, similarly to having imperfect rather than no white balance\ncorrection (yes, comparing apples and oranges technically, but both have their\nimpacts on the user satisfaction with the output).\n\n>> > On the other hand, looking at the histogram to try and stretch it is\n>> > called contrast adjustment. There's nothing wrong in doing so, but it's\n>> > usually done towards the output of the processing pipeline, and is\n>> > associated with manual adjustments of the contrast and brightness. See\n>> > src/ipa/rpi/controller/rpi/contrast.cpp for instance.\n>>\n>> Contrast adjustment is a different issue than the one this patch tries to\n>> address and less important now. I plan to work on it and similar functionality\n>> later.\n>>\n>> > David may be able to comment further as to why BLC and contrast are\n>> > quite different.\n>\n> We treat contrast adjustment as a more cosmetic feature of an image\n> which users can adjust according to their own personal taste. They can\n> have more of it or less of it, as they wish, and it doesn't really\n> affect the other aspects of image, such as colours or image quality.\n\nYes, as for contrast, we quickly rejected the idea of handling it automatically\nand decided to implement the proper user adjustable settings later.\n\n> Black level is quite different. There's a known value that you should\n> use. There's basically no choice. Speaking for the Pi, I would be\n> highly reluctant to couple these two things together - you certainly\n> wouldn't want contrast adjustment to start changing the colours, for\n> example!\n\nExactly. This is why I'm advocating for still considering the patch rather than\ndropping it. At least in my setup the auto-determined black value doesn't seem\nto be way off and serves its purpose even without access to exact tuning data.\n\n> Anyway, I hope that provides a little bit of a comparison, at least\n> from the point of view of a different implementation. Good luck!\n\nThanks,\nMilan\n\n> David\n>\n>>\n>> I think I understand the difference but feel free to comment if you think I'm\n>> missing something.\n>>\n>> > As this patch may need more work, I propose not including it in v7 to\n>> > avoid delay merging the rest of the implementation.\n>>\n>> Please consider my comments above. I'm open to suggestions how to do better at\n>> the moment. Less comfortable with nothing should be done about black level\n>> until we have a perfect solution.\n>>\n>> Thanks,\n>> Milan\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 AEE39BDCBD\n\tfor ;\n\tThu, 28 Mar 2024 14:47:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 888B56333B;\n\tThu, 28 Mar 2024 15:47:40 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C64E0632EA\n\tfor ;\n\tThu, 28 Mar 2024 15:47:38 +0100 (CET)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-582-mIPBhyH0PFqHhXPzUAFxMg-1; Thu, 28 Mar 2024 10:47:35 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-414042da713so10965075e9.0\n\tfor ;\n\tThu, 28 Mar 2024 07:47:34 -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\tr5-20020a056000014500b00341dc343e21sm1909678wrx.65.2024.03.28.07.47.32\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 28 Mar 2024 07:47:32 -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=\"U5Yd55Rf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1711637257;\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=nZn4qrpwmOkLG+Z7qGTS+ulWJrvkFbO11c908Inha6M=;\n\tb=U5Yd55Rfnx3CfIT1E+mKLw/sgrwtf3v0iJrFKBlFlzMAYKqFenoB0fIzT7XEfBQQVj+rFB\n\tEgOl9PBDNN5cGyaRem4FG8C1EZxEbXIPzgRhZi4+IeTmGA8T8D6gvatsuvo0dlPbr+muW7\n\tZyHaSJdiJr+SY5Y2djT37Dkwn3MsjRc=","X-MC-Unique":"mIPBhyH0PFqHhXPzUAFxMg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711637254; x=1712242054;\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=nZn4qrpwmOkLG+Z7qGTS+ulWJrvkFbO11c908Inha6M=;\n\tb=EJaDWjJwoAWUoOxm4BydDU66HxhRSKj2Nc7680BvFBn1DIrT5nwdHBHEqAEszQeQ4s\n\tst1EDZ5tWQrDOW5eSvS9WnhzuYQI354osoAh8l2MirqIvEVyRpg8xyGVMJmAuJ/Lcubq\n\txjBYgOsuRYrN2oKeTA9+v9J1wgBq6OF60AcKA5k19fmIz6n4LiB7OBNEE0FGWwFVgcTW\n\t0+aC7VK+AvyDwaiMrlLTKimZhcHLEaCSAjrjxi1Bolnz86espvMVwySbl58e90gCJp8d\n\tkIb0O9jQSpItLWCq5fc3gjsWm7iHWFEHIEKm6qCv/US3vOqzPAtCZu++9gTsut20a4kI\n\t5aFQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXjO0OB+u1JwEogrsaasVPksdMXR806bciTtne5oLSm4JQxBqF4AILMz7CXd8bz7vSQ8+3nkx5nn+dfB41MjlsScUbf+FrGhV+7T6Sx8WSizuyGWA==","X-Gm-Message-State":"AOJu0YwxzsiOB4LCuBwH/mKXm9rPxD3N4uzVbIJ5kjCIIzkAlYF/61W5\n\tzc5ezzUrv6lTYQppLsIwYqZaAafjpmVs/zao672J19f1qmTTMm/ZpCE+TeDblG2pD74fVwaUnTj\n\tEjVa3x8oAoSB5atPUtLfomOFAMwkq6dEFe2hDGvgDifY/PaDQHoMA38ohpamluOH6aR1OSA4=","X-Received":["by 2002:a5d:6a92:0:b0:341:c9bc:6340 with SMTP id\n\ts18-20020a5d6a92000000b00341c9bc6340mr2003367wru.12.1711637253937; \n\tThu, 28 Mar 2024 07:47:33 -0700 (PDT)","by 2002:a5d:6a92:0:b0:341:c9bc:6340 with SMTP id\n\ts18-20020a5d6a92000000b00341c9bc6340mr2003353wru.12.1711637253559; \n\tThu, 28 Mar 2024 07:47:33 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHNz97DYTGAiczVVL6PPb+LCyN5SNObv3mOYf1xBFADOqCK4ezFTa1DO8HrTANz5omzlG8WJQ==","From":"Milan Zamazal ","To":"David Plowman ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Andrey Konovalov\n\t, \"Bryan O'Donoghue\"\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","In-Reply-To":"\n\t(David Plowman's message of \"Thu, 28 Mar 2024 11:51:35 +0000\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t","Date":"Thu, 28 Mar 2024 15:47:32 +0100","Message-ID":"<87le62h0qj.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":29136,"web_url":"https://patchwork.libcamera.org/comment/29136/","msgid":"<87r0fnh7fx.fsf@redhat.com>","date":"2024-04-02T19:48:34","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> As this patch may need more work, I propose not including it in v7 to\n> avoid delay merging the rest of the implementation.\n\nI think David and I have provided sufficient additional information to make the\ndecision. I'll keep the patch in v7 but it will be the last patch in the series\nagain so it's easy to either take or omit it.\n\n[...]\n\n>> +class BlackLevel\n>\n> Wouldn't it be better to move to using the libipa Algorithm class before\n> introducing new algorithms ?\n\nI suppose we will need this sooner or later but it will need a non-trivial\namount of work to integrate software ISP with this in a meaningful way and to\ndecide how to de-wire exposure, white balance, and possibly black level from the\ncurrent code, without wasting the precious CPU cycles.\n\nRegards,\nMilan","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 515FCC0DA4\n\tfor ;\n\tTue, 2 Apr 2024 19:48:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CD1C63334;\n\tTue, 2 Apr 2024 21:48:41 +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 312066308D\n\tfor ;\n\tTue, 2 Apr 2024 21:48:40 +0200 (CEST)","from mail-lj1-f198.google.com (mail-lj1-f198.google.com\n\t[209.85.208.198]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-275-13fXFNnwPuebABcICTDk_g-1; Tue, 02 Apr 2024 15:48:37 -0400","by mail-lj1-f198.google.com with SMTP id\n\t38308e7fff4ca-2d82715d3e2so11855321fa.2\n\tfor ;\n\tTue, 02 Apr 2024 12:48:37 -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\tcn20-20020a0564020cb400b0056dfc8d12fasm211001edb.21.2024.04.02.12.48.34\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Apr 2024 12:48:34 -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=\"PXOr/JIm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712087319;\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=lVJP9E5mLYsH00SFr7xa+rJPo/jbkjpTlK8nhZ0TMyY=;\n\tb=PXOr/JImD0+ydOsA4Vop+VUIqTfwtb18j9KcVJclTBUxhgRmTCQ2/Tm9le0D3BHJhFHYko\n\tDvfIsWZQzi00JxMyXHdzCjIG6bbgPrTCalrsw1OJJCGtUGqqJIpN8dz8uqX8u231JcRuHf\n\t5HpNwAwiRh9bfttAIjsymrjRPCEYUzg=","X-MC-Unique":"13fXFNnwPuebABcICTDk_g-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712087316; x=1712692116;\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=lVJP9E5mLYsH00SFr7xa+rJPo/jbkjpTlK8nhZ0TMyY=;\n\tb=A8JCzoWW0tQGkQKhJNdUJWm9NIN65FVTwX3AbXKRPDVX+Ne5yybR/AJHzfDovTgEkK\n\tgiHdLRShgc3b0PORmPiZw8t5QPx8GTY5N/MEYCFyffl6bTy17q1bJsGCU/7lPTOfvivG\n\tvf7TcjuM8ory21J9j9De2cHZRpx8p+DFvfWPcVZpzKSxywwU+lqylPfTkpQuqhuvcvms\n\tqNrKmlP7oQVCLFpf98HeuCg/Ome+vcKuPOL4WRH+jfLjHKbqMtK7X2XBbzMmiwFxgGVJ\n\tCYScuyqg3MG0Q5XS50gDKax3J7O8k+fxA9SGUzyut4l3blTLpf/1S0G/0eRyVUWiBaYi\n\tYlDQ==","X-Gm-Message-State":"AOJu0YyWnNILPTMZMDBsIwCHYQLnztmEwsdl1dPqsbrN7Xkwv4WbbgRf\n\tG0kbUH9Snx2CykyuFgO5AGCvYGB12OnSR6yJRNRBDGrFJQOb+0Gf16UE3qwXvMLZvToD+LX7UgN\n\tJpuDg2GWe91u15jLgDCwWJib1pGKLj3z9oJqy9mgi3t+/HTIUDJiOe3c06KoMrS7VEW6niQ0=","X-Received":["by 2002:a05:6512:3091:b0:516:9f13:883 with SMTP id\n\tz17-20020a056512309100b005169f130883mr8162641lfd.24.1712087316029; \n\tTue, 02 Apr 2024 12:48:36 -0700 (PDT)","by 2002:a05:6512:3091:b0:516:9f13:883 with SMTP id\n\tz17-20020a056512309100b005169f130883mr8162629lfd.24.1712087315525; \n\tTue, 02 Apr 2024 12:48:35 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHjHn0TIx+qCqdrnGwPyxQZaYDOGxiHR5HcLEPmJMmDPz/z3GxEvqMnLXCcrh18oNGfs8jeHg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Andrey Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham , David Plowman\n\t","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","In-Reply-To":"<20240327185543.GN4721@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Mar 2024 20:55:43 +0200\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>","Date":"Tue, 02 Apr 2024 21:48:34 +0200","Message-ID":"<87r0fnh7fx.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":29143,"web_url":"https://patchwork.libcamera.org/comment/29143/","msgid":"<20240402213157.GF16740@pendragon.ideasonboard.com>","date":"2024-04-02T21:31:57","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Tue, Apr 02, 2024 at 09:48:34PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> \n> > As this patch may need more work, I propose not including it in v7 to\n> > avoid delay merging the rest of the implementation.\n> \n> I think David and I have provided sufficient additional information to make the\n> decision. I'll keep the patch in v7 but it will be the last patch in the series\n> again so it's easy to either take or omit it.\n\nThank you.\n\n> [...]\n> \n> >> +class BlackLevel\n> >\n> > Wouldn't it be better to move to using the libipa Algorithm class before\n> > introducing new algorithms ?\n> \n> I suppose we will need this sooner or later but it will need a non-trivial\n> amount of work to integrate software ISP with this in a meaningful way and to\n> decide how to de-wire exposure, white balance, and possibly black level from the\n> current code, without wasting the precious CPU cycles.\n\nThe CPU-intensive part is the image processing itself, the IPA side\nshould hopefully not be an issue, even if we complexify it a little bit\nby using the more modular architecture that the Algorithm class\nprovides.","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 02DB2C0DA4\n\tfor ;\n\tTue, 2 Apr 2024 21:32:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDDF96336C;\n\tTue, 2 Apr 2024 23:32:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A38D6334D\n\tfor ;\n\tTue, 2 Apr 2024 23:32:08 +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 F24EA564;\n\tTue, 2 Apr 2024 23:31:30 +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=\"hxrsaZH+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712093491;\n\tbh=7+o1HhDRkz22vTvxrxzouFc+OVIdq/WhZoh8xyHLvfE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hxrsaZH+E422L/xrYKIhTRwbkN3Y+gAUIsSNc/TMa+ajQbZBP1E45phgWlqXTLYAC\n\tmF/5kli+FoaPegukvAXDp7rGxiuItwE9wW5C6pHindtiBkANQoPHjN+KAAJdgigsiM\n\tw9fuM1uaSxULAXb+n9LJ3+pxXKkFvrf7Fx7aoz7k=","Date":"Wed, 3 Apr 2024 00:31:57 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDavid Plowman ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"<20240402213157.GF16740@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87r0fnh7fx.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87r0fnh7fx.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":29144,"web_url":"https://patchwork.libcamera.org/comment/29144/","msgid":"<20240402220753.GG16740@pendragon.ideasonboard.com>","date":"2024-04-02T22:07:53","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Mar 28, 2024 at 03:47:32PM +0100, Milan Zamazal wrote:\n> David Plowman writes:\n> \n> > Hi everyone\n> >\n> > Thanks for cc'ing me on this question. Always happy to try and answer!\n> \n> Hi David,\n> \n> thank you for your answer and explanations.\n\nLikewise, thank you.\n\n> > On Thu, 28 Mar 2024 at 10:11, Milan Zamazal wrote:\n> >> Laurent Pinchart writes:\n> >> > On Tue, Mar 19, 2024 at 01:36:04PM +0100, Milan Zamazal wrote:\n> >> >> Black may not be represented as 0 pixel value for given hardware, it may be\n> >> >> higher. If this is not compensated then various problems may occur such as low\n> >> >> contrast or suboptimal exposure.\n> >> >>\n> >> >> The black pixel value can be either retrieved from a tuning file for the given\n> >> >> hardware, or automatically on fly. The former is the right and correct method,\n> >> >\n> >> > s/on fly/on the fly/\n> >> >\n> >> >> while the latter can be used when a tuning file is not available for the given\n> >> >> hardware. Since there is currently no support for tuning files in software ISP,\n> >> >> the automatic, hardware independent way, is always used. Support for tuning\n> >> >> files should be added in future but it will require more work than this patch.\n> >> >\n> >> > I don't think this is quite right. Strictly speaking, the black level\n> >> > compensation is about subtracting the black level as produced by the\n> >> > sensor. This requires tuning, and shouldn't be done on-the-fly.\n> >>\n> >> A general agreement from those who tried the patch was that it makes a\n> >> noticeable improvement. As explained in my comments above, this is a fallback\n> >> mechanism that we can use cheaply and immediately, until or unless tuning file\n> >> for software ISP is available. So far, it seems to be beneficial rather than\n> >> harmful.\n> >>\n> >> Unless we have something better or perfect soon, which doesn't seem to be the\n> >> case to me, why not to use this simple and useful (I believe) fallback\n> >> mechanism? Do you have a better suggestion what could be done right now\n> >> regarding this issue (not considering black level at all is a real issue)?\n> >\n> > This is of course partly a technical issue - what's the right thing to\n> > do? - and particularly a pragmatic issue - whether to do something\n> > imperfect right now or not.\n> \n> Yes.\n> \n> > To tackle the technical question, black level is normally removed\n> > fairly early in the pipeline, because not doing so will make typical\n> > operations in the Bayer pipe go wrong. For example:\n> >\n> > * Lens shading (vignetting correction). I don't know whether you have\n> > this feature currently, but maybe it will be required at some point to\n> > improve image quality? Doing this without first removing the black\n> > level will make all the colours go wrong.\n> >\n> > * White balance. This too will go wrong if you haven't subtracted the\n> > black level. White balance is normally applied before demosaic because\n> > demosaic typically works better with white-balanced pixels. Obviously\n> > it depends what your demosaic algorithm does, but I wonder if there's\n> > a future in which you have higher quality versions of some algorithms\n> > for certain use cases (e.g. stills capture) where this could become an\n> > issue.\n> >\n> > * Colour correction. Like white balance, colour correction matrices\n> > will not do the right thing if black level has not been removed.\n> \n> We don't have lens shading or color correction, once we have support for these\n> there is no reason not to have proper black level correction as well. As for\n> white balance, we currently use probably the most primitive method possible\n> (green/red and green/blue ratios) and we don't subtract black level even with\n> this patch (which should get fixed later). The patch subtracts black level when\n> correcting exposure though, which is also influenced by this. Considering all\n> the imperfections at the moment, the actual effects on white balance and\n> exposure may not be that big to care very much. But even then ...\n> \n> > You can probably guess that I belong more to the \"calibrate the camera\n> > properly\" view of the world because doing that fixes problems and\n> > makes them go away _forever_. But I'm not familiar with the background\n> > here, so I can appreciate that interim solutions may have a place.\n> \n> ... the thing is that not applying any black level correction, even when not\n> considering white balance and exposure deformations, often makes the image\n> looking noticeably flat as there are no blacks. I'd say better imperfect blacks\n> than no blacks, similarly to having imperfect rather than no white balance\n> correction (yes, comparing apples and oranges technically, but both have their\n> impacts on the user satisfaction with the output).\n> \n> >> > On the other hand, looking at the histogram to try and stretch it is\n> >> > called contrast adjustment. There's nothing wrong in doing so, but it's\n> >> > usually done towards the output of the processing pipeline, and is\n> >> > associated with manual adjustments of the contrast and brightness. See\n> >> > src/ipa/rpi/controller/rpi/contrast.cpp for instance.\n> >>\n> >> Contrast adjustment is a different issue than the one this patch tries to\n> >> address and less important now. I plan to work on it and similar functionality\n> >> later.\n> >>\n> >> > David may be able to comment further as to why BLC and contrast are\n> >> > quite different.\n> >\n> > We treat contrast adjustment as a more cosmetic feature of an image\n> > which users can adjust according to their own personal taste. They can\n> > have more of it or less of it, as they wish, and it doesn't really\n> > affect the other aspects of image, such as colours or image quality.\n> \n> Yes, as for contrast, we quickly rejected the idea of handling it automatically\n> and decided to implement the proper user adjustable settings later.\n> \n> > Black level is quite different. There's a known value that you should\n> > use. There's basically no choice. Speaking for the Pi, I would be\n> > highly reluctant to couple these two things together - you certainly\n> > wouldn't want contrast adjustment to start changing the colours, for\n> > example!\n> \n> Exactly. This is why I'm advocating for still considering the patch rather than\n> dropping it. At least in my setup the auto-determined black value doesn't seem\n> to be way off and serves its purpose even without access to exact tuning data.\n\nAs for other part of this series, I'm fine starting with this initial\nimplementation and improving it, but I think the black level should\neventually be moved before debayering, and ideally the colour gains as\nwell. I understand the need for optimizations to lower the CPU\nconsumption, but at the same time I don't feel comfortable building up\non top of an implementation that may work a bit more by chance than by\ncorrectness, as that's not very maintainable.\n\n> > Anyway, I hope that provides a little bit of a comparison, at least\n> > from the point of view of a different implementation. Good luck!\n> >\n> >> I think I understand the difference but feel free to comment if you think I'm\n> >> missing something.\n> >>\n> >> > As this patch may need more work, I propose not including it in v7 to\n> >> > avoid delay merging the rest of the implementation.\n> >>\n> >> Please consider my comments above. I'm open to suggestions how to do better at\n> >> the moment. Less comfortable with nothing should be done about black level\n> >> until we have a perfect solution.","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 D147FBD16B\n\tfor ;\n\tTue, 2 Apr 2024 22:08:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAEAB6336D;\n\tWed, 3 Apr 2024 00:08:07 +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 D2B1E63334\n\tfor ;\n\tWed, 3 Apr 2024 00:08:05 +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 A451B512;\n\tWed, 3 Apr 2024 00:07:28 +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=\"I0ggusMn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712095648;\n\tbh=flvEquOCBtwAGU6mELbl993adicIl1GcBvxg0qnnzPc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I0ggusMnY1rTV+kd4sTqr1NgJFC5EoVEG7LcQzdt9gNl9Sxn/8ndXk/rPQsp/RIPG\n\tGcd577EhWoMM5PY7TCLP3Nlpl9j3D4PmVzi0gyZtkNbTfAIJm0jfpWpKhJegOZLWaZ\n\tZmu71v7/3O2fyL5tf5pprF31RgEqjeSqlMngj+nA=","Date":"Wed, 3 Apr 2024 01:07:53 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"<20240402220753.GG16740@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87le62h0qj.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":29148,"web_url":"https://patchwork.libcamera.org/comment/29148/","msgid":"<87le5ueowz.fsf@redhat.com>","date":"2024-04-03T10:11:40","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> Hello,\n>\n> On Thu, Mar 28, 2024 at 03:47:32PM +0100, Milan Zamazal wrote:\n>> David Plowman writes:\n>> \n>> > Hi everyone\n>> >\n>> > Thanks for cc'ing me on this question. Always happy to try and answer!\n>> \n>> Hi David,\n>> \n>> thank you for your answer and explanations.\n>\n> Likewise, thank you.\n>\n>> > On Thu, 28 Mar 2024 at 10:11, Milan Zamazal wrote:\n>> >> Laurent Pinchart writes:\n>> >> > On Tue, Mar 19, 2024 at 01:36:04PM +0100, Milan Zamazal wrote:\n>> >> >> Black may not be represented as 0 pixel value for given hardware, it may be\n>> >> >> higher. If this is not compensated then various problems may occur such as low\n>> >> >> contrast or suboptimal exposure.\n>> >> >>\n>> >> >> The black pixel value can be either retrieved from a tuning file for the given\n>> >> >> hardware, or automatically on fly. The former is the right and correct method,\n>> >> >\n>> >> > s/on fly/on the fly/\n>> >> >\n>> >> >> while the latter can be used when a tuning file is not available for the given\n>> >> >> hardware. Since there is currently no support for tuning files in software ISP,\n>> >> >> the automatic, hardware independent way, is always used. Support for tuning\n>> >> >> files should be added in future but it will require more work than this patch.\n>> >> >\n>> >> > I don't think this is quite right. Strictly speaking, the black level\n>> >> > compensation is about subtracting the black level as produced by the\n>> >> > sensor. This requires tuning, and shouldn't be done on-the-fly.\n>> >>\n>> >> A general agreement from those who tried the patch was that it makes a\n>> >> noticeable improvement. As explained in my comments above, this is a fallback\n>> >> mechanism that we can use cheaply and immediately, until or unless tuning file\n>> >> for software ISP is available. So far, it seems to be beneficial rather than\n>> >> harmful.\n>> >>\n>> >> Unless we have something better or perfect soon, which doesn't seem to be the\n>> >> case to me, why not to use this simple and useful (I believe) fallback\n>> >> mechanism? Do you have a better suggestion what could be done right now\n>> >> regarding this issue (not considering black level at all is a real issue)?\n>> >\n>> > This is of course partly a technical issue - what's the right thing to\n>> > do? - and particularly a pragmatic issue - whether to do something\n>> > imperfect right now or not.\n>> \n>> Yes.\n>> \n>> > To tackle the technical question, black level is normally removed\n>> > fairly early in the pipeline, because not doing so will make typical\n>> > operations in the Bayer pipe go wrong. For example:\n>> >\n>> > * Lens shading (vignetting correction). I don't know whether you have\n>> > this feature currently, but maybe it will be required at some point to\n>> > improve image quality? Doing this without first removing the black\n>> > level will make all the colours go wrong.\n>> >\n>> > * White balance. This too will go wrong if you haven't subtracted the\n>> > black level. White balance is normally applied before demosaic because\n>> > demosaic typically works better with white-balanced pixels. Obviously\n>> > it depends what your demosaic algorithm does, but I wonder if there's\n>> > a future in which you have higher quality versions of some algorithms\n>> > for certain use cases (e.g. stills capture) where this could become an\n>> > issue.\n>> >\n>> > * Colour correction. Like white balance, colour correction matrices\n>> > will not do the right thing if black level has not been removed.\n>> \n>> We don't have lens shading or color correction, once we have support for these\n>> there is no reason not to have proper black level correction as well. As for\n>> white balance, we currently use probably the most primitive method possible\n>> (green/red and green/blue ratios) and we don't subtract black level even with\n>> this patch (which should get fixed later). The patch subtracts black level when\n>> correcting exposure though, which is also influenced by this. Considering all\n>> the imperfections at the moment, the actual effects on white balance and\n>> exposure may not be that big to care very much. But even then ...\n>> \n>> > You can probably guess that I belong more to the \"calibrate the camera\n>> > properly\" view of the world because doing that fixes problems and\n>> > makes them go away _forever_. But I'm not familiar with the background\n>> > here, so I can appreciate that interim solutions may have a place.\n>> \n>> ... the thing is that not applying any black level correction, even when not\n>> considering white balance and exposure deformations, often makes the image\n>> looking noticeably flat as there are no blacks. I'd say better imperfect blacks\n>> than no blacks, similarly to having imperfect rather than no white balance\n>> correction (yes, comparing apples and oranges technically, but both have their\n>> impacts on the user satisfaction with the output).\n>> \n>> >> > On the other hand, looking at the histogram to try and stretch it is\n>> >> > called contrast adjustment. There's nothing wrong in doing so, but it's\n>> >> > usually done towards the output of the processing pipeline, and is\n>> >> > associated with manual adjustments of the contrast and brightness. See\n>> >> > src/ipa/rpi/controller/rpi/contrast.cpp for instance.\n>> >>\n>> >> Contrast adjustment is a different issue than the one this patch tries to\n>> >> address and less important now. I plan to work on it and similar functionality\n>> >> later.\n>> >>\n>> >> > David may be able to comment further as to why BLC and contrast are\n>> >> > quite different.\n>> >\n>> > We treat contrast adjustment as a more cosmetic feature of an image\n>> > which users can adjust according to their own personal taste. They can\n>> > have more of it or less of it, as they wish, and it doesn't really\n>> > affect the other aspects of image, such as colours or image quality.\n>> \n>> Yes, as for contrast, we quickly rejected the idea of handling it automatically\n>> and decided to implement the proper user adjustable settings later.\n>> \n>> > Black level is quite different. There's a known value that you should\n>> > use. There's basically no choice. Speaking for the Pi, I would be\n>> > highly reluctant to couple these two things together - you certainly\n>> > wouldn't want contrast adjustment to start changing the colours, for\n>> > example!\n>> \n>> Exactly. This is why I'm advocating for still considering the patch rather than\n>> dropping it. At least in my setup the auto-determined black value doesn't seem\n>> to be way off and serves its purpose even without access to exact tuning data.\n>\n> As for other part of this series, I'm fine starting with this initial\n> implementation and improving it, but I think the black level should\n> eventually be moved before debayering, and ideally the colour gains as\n> well. I understand the need for optimizations to lower the CPU\n> consumption, but at the same time I don't feel comfortable building up\n> on top of an implementation that may work a bit more by chance than by\n> correctness, as that's not very maintainable.\n\nI agree with this approach. I added your comment to the TODO file.\n\n>> > Anyway, I hope that provides a little bit of a comparison, at least\n>> > from the point of view of a different implementation. Good luck!\n>> >\n>> >> I think I understand the difference but feel free to comment if you think I'm\n>> >> missing something.\n>> >>\n>> >> > As this patch may need more work, I propose not including it in v7 to\n>> >> > avoid delay merging the rest of the implementation.\n>> >>\n>> >> Please consider my comments above. I'm open to suggestions how to do better at\n>> >> the moment. Less comfortable with nothing should be done about black level\n>> >> until we have a perfect solution.","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 B1832BD16B\n\tfor ;\n\tWed, 3 Apr 2024 10:11:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 161CC6336D;\n\tWed, 3 Apr 2024 12:11:49 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8BBC62CA0\n\tfor ;\n\tWed, 3 Apr 2024 12:11:46 +0200 (CEST)","from mail-lf1-f71.google.com (mail-lf1-f71.google.com\n\t[209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-50-Yj7rj0zbOVKzoptYOYD7AQ-1; Wed, 03 Apr 2024 06:11:44 -0400","by mail-lf1-f71.google.com with SMTP id\n\t2adb3069b0e04-515ab5c3738so5750453e87.2\n\tfor ;\n\tWed, 03 Apr 2024 03:11:44 -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\tfe8-20020a056402390800b0056c522d014esm7680347edb.57.2024.04.03.03.11.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 03 Apr 2024 03:11:41 -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=\"g39rb7Ow\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712139105;\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=kN7Z5bIlXs2qMFle8erjhArf2qGV3HiH9qaXMvGavkY=;\n\tb=g39rb7Owphqoeo+YjIA0VmVJQ4oTrsD1WSAIRtfs3X323qoJfupFeMd7MRYM2czUe99r3z\n\tu4U/Ebp+mENIDghuTroTt1YH7UFTmjhbDDYCt7iPGe7z9QFkm5gk0F7ReyZ4WlZo9zcE8G\n\tgb+h6FvWiXAmta2eD+6vLI8Wha+1HOE=","X-MC-Unique":"Yj7rj0zbOVKzoptYOYD7AQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712139103; x=1712743903;\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=kN7Z5bIlXs2qMFle8erjhArf2qGV3HiH9qaXMvGavkY=;\n\tb=vsUK0fpcUVejlM5hOZeWii1k3b63riwDqzu1JMaOUQi69HPxlATlwcVjyf8k4sklK8\n\toMALS/weyy5nD+neZyWuGmHVrdXhsdTnhqgrMrjByj7gCz7QM3ZfynU6BAsJ90coNBmY\n\tJsRgy9NmAAErbB5OEU/bAdYAO6jMGMjtEVW9YS9UdpBer5ukINRGmclWyjL0ShAL3ecQ\n\t7eIdDhMDdToZTrVj5VDjWOJWdHpWiWoS47PU+nLEJZ2OizP3ycV0/x38wdTNp+CjCPA8\n\tHIH9SNKqI6cnAODbmG3ag4QCGxqRN9cOK+tkF6E+YBzXpsTffafCAn9WHivGd6m8OzXa\n\t2mdg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCU2zg8N+pAWSCUw4y9NJFFgmR3jUxSPYBmL6F6GPCVhb41ZQ63OezSLWpwjsm3uHaGiGfdfFZKY17EX0iCp7wdq5h+DGzQIkrPhrrKyMXRU7tRy5A==","X-Gm-Message-State":"AOJu0Ywr4C5BKngwAwKTe12kWS5v3+0917zfySG1bPtjHaizfm/LhlbR\n\tL1TX8TBK7LHuyvCPvvaA0JhK7AEBMUabPVGK34rmaHah4B1pXagTQBaxjJBB4crVu7oFVL/xjmX\n\tEwhtyrWGmMPCdAkE4ByJwOIgkRfL3opFvVy9m/gsE16A9wQirW/j1YLw87lijBsYqA5Eu5AE=","X-Received":["by 2002:a19:5f53:0:b0:515:d1c3:fe3f with SMTP id\n\ta19-20020a195f53000000b00515d1c3fe3fmr1454965lfj.39.1712139102851; \n\tWed, 03 Apr 2024 03:11:42 -0700 (PDT)","by 2002:a19:5f53:0:b0:515:d1c3:fe3f with SMTP id\n\ta19-20020a195f53000000b00515d1c3fe3fmr1454942lfj.39.1712139102301; \n\tWed, 03 Apr 2024 03:11:42 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFyt6J/XKBUoWr9yfJ0HIrJWnPX30ToChGiXkPqx8rmVqM2V1GdGnUwl4UqZ4eLnKTK62CUyQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org, Andrey Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","In-Reply-To":"<20240402220753.GG16740@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 3 Apr 2024 01:07:53 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>","Date":"Wed, 03 Apr 2024 12:11:40 +0200","Message-ID":"<87le5ueowz.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":29236,"web_url":"https://patchwork.libcamera.org/comment/29236/","msgid":"<87plupjpd1.fsf@redhat.com>","date":"2024-04-16T15:31:38","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> As for other part of this series, I'm fine starting with this initial\n> implementation and improving it, but I think the black level should\n> eventually be moved before debayering, and ideally the colour gains as\n> well. I understand the need for optimizations to lower the CPU\n> consumption, but at the same time I don't feel comfortable building up\n> on top of an implementation that may work a bit more by chance than by\n> correctness, as that's not very maintainable.\n\nHi Laurent,\n\nto make sure we understand each other correctly: Can we agree that \"be moved\nbefore debayering\" above means primarily separating the level adjustments\ncomputations from debayering code and building them (together with other stuff)\non top of libipa/algorithm.h? Rather than first applying the corrections on the\ndata and only then perform debayering, which luxury we cannot afford, at least\non CPU? I.e. the first step could be something like\nhttps://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/6180d853005bb956b3d2469576b139ff523b76ad ?\n\nRegards,\nMilan","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 B46D2C3213\n\tfor ;\n\tTue, 16 Apr 2024 15:31:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C78963352;\n\tTue, 16 Apr 2024 17:31:46 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C295E61B75\n\tfor ;\n\tTue, 16 Apr 2024 17:31:44 +0200 (CEST)","from mail-ed1-f69.google.com (mail-ed1-f69.google.com\n\t[209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-8-1ARXCZ0EMb-7HVpj9uRZIg-1; Tue, 16 Apr 2024 11:31:41 -0400","by mail-ed1-f69.google.com with SMTP id\n\t4fb4d7f45d1cf-56e58297218so1882517a12.0\n\tfor ;\n\tTue, 16 Apr 2024 08:31:41 -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\tes20-20020a056402381400b0056fe4dff5e3sm6180963edb.60.2024.04.16.08.31.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 16 Apr 2024 08:31:39 -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=\"K+oSGJzy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1713281503;\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=HggjTHf9NUBTWOkkhAck2fjSjgAlg3YOkxs87fMUWJE=;\n\tb=K+oSGJzypvDNCl4iY0CCa57wGm72tj72IcyCUZdw1R7CEC4cFbreoI5PaJQyp5RlLsCX46\n\tkCE5aXavWpHFAcUgPuhpBJSCpvfBqPW/SCQrFnNJqDzR/m7yTBJ2iWQzEkYCw6uSCc6QtS\n\tDE+ZEKQNiUz6L94z68JLP3YRed1Wfqo=","X-MC-Unique":"1ARXCZ0EMb-7HVpj9uRZIg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1713281501; x=1713886301;\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=HggjTHf9NUBTWOkkhAck2fjSjgAlg3YOkxs87fMUWJE=;\n\tb=FIatiIqXIev0jLJaryyjlhRRutcEfahnOlCPsCJ+NcG/o8yT7xA7rkzVkYUYfQ3ozq\n\t+0AYwzTXvLNjUPesr0Wh8wbJDBfRN+sw08SQOunJsdg1SJ6srLM8PoGD+BpdBfBfo8dA\n\tdS6g1Aks0K6n7xWMPYkRFPGclYyqfR6qtnE+iGREWjDqMNyHE2nKeJpDs3RvMWUQDXnW\n\tesrvL3TtHfJFse721urEZckyLFL6RQKqK2+ppL4Iygp9x4qo18qMUqw+l+ee9Yr9MsSA\n\tBNm4J29uUxqcHWHz9Mpn2cUO2EGhrvb2QRbYTF4U0mBeSOFum4N3CZjHQAJHApJTq1BX\n\tXQVQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVwP+d0HSzHnacE/Rb5ZaiPAmihprGnYvv4iV1l9tv9F4a1ts6oOkjk8lqvAT3dMZOl6B/mdoZdB+G6+AIRpmc8OHVuNIJdHXtgfUVke6v7WHx+Pg==","X-Gm-Message-State":"AOJu0YxpoxVX87u3LxReRfP6FI+vXVxuur9/kuU/PbyaI/VvMMsYrVsU\n\tTRKvQcsTfTYuacUEHtOMHYh1XmXF/0k8TBo0YwCj8Ej+OBvb78dvZSPUOJB4TeAH/8fgeFjGtE2\n\tu8e9qRX2eIurRb823lf3WSvOkYY5DItf9tBYfKtOL5cYbO4VHMVo0JQrosmJBfl2rxDEzDtA=","X-Received":["by 2002:a50:ccd2:0:b0:570:1ea8:cd20 with SMTP id\n\tb18-20020a50ccd2000000b005701ea8cd20mr5868021edj.3.1713281500761; \n\tTue, 16 Apr 2024 08:31:40 -0700 (PDT)","by 2002:a50:ccd2:0:b0:570:1ea8:cd20 with SMTP id\n\tb18-20020a50ccd2000000b005701ea8cd20mr5868005edj.3.1713281500370; \n\tTue, 16 Apr 2024 08:31:40 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IG/8QdoBPKUQ/aHKeXKbr5XVd8AHBYyT80BAOVaHdWASvPgj4I5hsg6VDmE2TkZqXpQ8Ssnyw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org, Andrey Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","In-Reply-To":"<20240402220753.GG16740@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 3 Apr 2024 01:07:53 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>","Date":"Tue, 16 Apr 2024 17:31:38 +0200","Message-ID":"<87plupjpd1.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":29282,"web_url":"https://patchwork.libcamera.org/comment/29282/","msgid":"<20240420104247.GE6414@pendragon.ideasonboard.com>","date":"2024-04-20T10:42:47","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Tue, Apr 16, 2024 at 05:31:38PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> \n> > As for other part of this series, I'm fine starting with this initial\n> > implementation and improving it, but I think the black level should\n> > eventually be moved before debayering, and ideally the colour gains as\n> > well. I understand the need for optimizations to lower the CPU\n> > consumption, but at the same time I don't feel comfortable building up\n> > on top of an implementation that may work a bit more by chance than by\n> > correctness, as that's not very maintainable.\n> \n> Hi Laurent,\n> \n> to make sure we understand each other correctly: Can we agree that \"be moved\n> before debayering\" above means primarily separating the level adjustments\n> computations from debayering code and building them (together with other stuff)\n> on top of libipa/algorithm.h? Rather than first applying the corrections on the\n> data and only then perform debayering, which luxury we cannot afford, at least\n> on CPU? I.e. the first step could be something like\n> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/6180d853005bb956b3d2469576b139ff523b76ad ?\n\nWhat I meant is subtracting the black level before applying the colour\ngains and performing the CFA interpolation (a.k.a. debayering). I'm fine\nwith the black level subtraction and CFA interpolation being performed\nin a single pass by a single function for performance reason, but the\norder needs to be swapped.","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 D3CBCBE08B\n\tfor ;\n\tSat, 20 Apr 2024 10:42:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 73F0B633F3;\n\tSat, 20 Apr 2024 12:42:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F1D161B43\n\tfor ;\n\tSat, 20 Apr 2024 12:42:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 315941A2;\n\tSat, 20 Apr 2024 12:42:06 +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=\"n/D62cWq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713609726;\n\tbh=tCVzHv+fWOcLJoged+t+ngdfX3PPshEcueirRiIhX50=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n/D62cWqr16pB1FaZqKcKTzbidPnjrqt47XsUePQdMa5CyebBeRwTk0lkgAajFYUa\n\t2hYMuh2ppELg6okLACGznHENSFPFOSB4+NZLtHip1L3zzTGjX8H2jZ/78TlY67Z7TX\n\tK/dvhcCf//JVTRdYl+FYG/KjEhpziQv1ztIXm8kY=","Date":"Sat, 20 Apr 2024 13:42:47 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"<20240420104247.GE6414@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87plupjpd1.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":29283,"web_url":"https://patchwork.libcamera.org/comment/29283/","msgid":"","date":"2024-04-21T12:16:06","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 4/20/24 12:42 PM, Laurent Pinchart wrote:\n> Hi Milan,\n> \n> On Tue, Apr 16, 2024 at 05:31:38PM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>>\n>>> As for other part of this series, I'm fine starting with this initial\n>>> implementation and improving it, but I think the black level should\n>>> eventually be moved before debayering, and ideally the colour gains as\n>>> well. I understand the need for optimizations to lower the CPU\n>>> consumption, but at the same time I don't feel comfortable building up\n>>> on top of an implementation that may work a bit more by chance than by\n>>> correctness, as that's not very maintainable.\n>>\n>> Hi Laurent,\n>>\n>> to make sure we understand each other correctly: Can we agree that \"be moved\n>> before debayering\" above means primarily separating the level adjustments\n>> computations from debayering code and building them (together with other stuff)\n>> on top of libipa/algorithm.h? Rather than first applying the corrections on the\n>> data and only then perform debayering, which luxury we cannot afford, at least\n>> on CPU? I.e. the first step could be something like\n>> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/6180d853005bb956b3d2469576b139ff523b76ad ?\n> \n> What I meant is subtracting the black level before applying the colour\n> gains and performing the CFA interpolation (a.k.a. debayering). I'm fine\n> with the black level subtraction and CFA interpolation being performed\n> in a single pass by a single function for performance reason, but the\n> order needs to be swapped.\n\nIf we apply the per color lookup table (which does black-level compensation +\ncolor-gain + gamma-curve in 1 step) to the raw bayer data then we either need to\ndo this on some extra copy before debayering or we need to do it multiple times\nper pixel before averaging the surrounding pixels, neither one of which is ideal.\n\nI guess we could make the memcpy to temp buffer behavior done to deal with\nuncached mem mandatory (so always do this) and then first apply the rgb lookups\nto a just copied raw line before debayering, then we still only need to do\nthis once per color. And this could also help wrt cache behavior of the\nlookups since we then linearly travel through 1 line touching only the\n1 line (in temp buffer) + read 2 lookup tables. So then there will be less\ncache contention then when using the lookups during debayering when\nwere are reading 3 lines + writing 1 line + accessing all 3 lookup tables\n(so I guess this might even speed things up especially on the imx8).\n\nI don't expect this to make much difference for the resulting image though,\nOTOH it will make some difference and testing has shown that the overhead\nof using the extra memcpy buffers in cases where this is not necessary is\nsmall. So I guess we could do this.\n\nMilan can you extract what I have in mind from the above and is this\nsomething you can work on, or shall I take a shot at this when I can schedule\nsome time for this?\n\nRegards,\n\nHans","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 D3D2BC3200\n\tfor ;\n\tSun, 21 Apr 2024 12:16:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B22C8633F4;\n\tSun, 21 Apr 2024 14:16:13 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB57C61B3D\n\tfor ;\n\tSun, 21 Apr 2024 14:16:11 +0200 (CEST)","from mail-ed1-f71.google.com (mail-ed1-f71.google.com\n\t[209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-612-uZvY9IoLMxCCSWdHO1_VoA-1; Sun, 21 Apr 2024 08:16:08 -0400","by mail-ed1-f71.google.com with SMTP id\n\t4fb4d7f45d1cf-56c1b105949so2828616a12.3\n\tfor ;\n\tSun, 21 Apr 2024 05:16:08 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\te6-20020a056402148600b005702c757af2sm4334788edv.30.2024.04.21.05.16.06\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 21 Apr 2024 05:16:07 -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=\"MAQfJFTe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1713701770;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=u62TUOyy2OYGdILLAVv8BkMvlUhBnr/Rb9JHIdTFljk=;\n\tb=MAQfJFTeeqfNa7r3whykqOcDFtABLlW0xBr54ytTk4XyvXOLhengSrujHUUE9gUBhaR5t1\n\t9L7JrjJeFvb/txcYQvWp91fnLjMppUCgVnboZ2Wxtu8yCZXwx4Oo5TCFqBt3ep3pa3e4jo\n\tDIxKwD4mLLtg7623HWFRkVS7/HKiAJM=","X-MC-Unique":"uZvY9IoLMxCCSWdHO1_VoA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1713701768; x=1714306568;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=u62TUOyy2OYGdILLAVv8BkMvlUhBnr/Rb9JHIdTFljk=;\n\tb=pV+METeC68m2bKgxdtMkSWYpOCLAHBgVdZn/dppVtqVAoOXZ3sBM2ZgtHQB6O/Q4gw\n\tf6VSlXA7Dw5S9gEFdPgbKl7vZZcZKvbnJmaMO4l67+j5/cty+eK3nWsTOqq/JhgHUiNQ\n\t/UrjetP6sIn76EyUcQRIH82CuF2pdmL2QvVxPceoXhdKwLDhYgz3ocPYzPfbUvtVfEj7\n\teC5KYA6ihAX+6y9m99+aEmMMiJWwLvKoL67hnLEgycq6fuddCVCjLP3kL9QpdOjv+uf4\n\tM7Ig8AirCWONjrsnbk7IaCVGRBMq+zmqy/lLBFaV0dKRIqOq9cyaDtu61dDJGFxIG8Fo\n\t8r1Q==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVxlivvznn61UGPGwPltuHNdcuJoDoMytQ5G8fODqdtQB5Gu/TEVU1UTXN2Z4lVuyW0o0VGor+W9m5nEAR4Z+V4ryxqoQR0mItkhJF7uqm0y3QrLg==","X-Gm-Message-State":"AOJu0YzjA0sN6fKOZA9G4vrZwfIJP9ldBvtIggBvSPVSFWHLDU9uPuzN\n\trQFUnbcpxg6Tj9/eTGPSOJbHTxlvvaesKRJ0OFTD6EL+LkF4RVzOtaPPj/Y7DSkF5PevA2XoP4F\n\tbxZ6at9cAiY/AGzEIui+37h057SlMdmb/E6PjYKP5+ry8qwpPQvCqb/y4BqjJmrss0iIM/QY=","X-Received":["by 2002:a50:baad:0:b0:56e:323b:d7e7 with SMTP id\n\tx42-20020a50baad000000b0056e323bd7e7mr4984459ede.34.1713701767816; \n\tSun, 21 Apr 2024 05:16:07 -0700 (PDT)","by 2002:a50:baad:0:b0:56e:323b:d7e7 with SMTP id\n\tx42-20020a50baad000000b0056e323bd7e7mr4984448ede.34.1713701767491; \n\tSun, 21 Apr 2024 05:16:07 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IE3/Ye2wuxhAbf1TgWdv2qY+e89eXnvnmZ2Tl1Mh/3is6k0AfR+DvbmWpbcOeRw7j+t9BgtrQ==","Message-ID":"","Date":"Sun, 21 Apr 2024 14:16:06 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","To":"Laurent Pinchart ,\n\tMilan Zamazal ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240420104247.GE6414@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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":29284,"web_url":"https://patchwork.libcamera.org/comment/29284/","msgid":"<20240421122239.GB19147@pendragon.ideasonboard.com>","date":"2024-04-21T12:22:39","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Sun, Apr 21, 2024 at 02:16:06PM +0200, Hans de Goede wrote:\n> On 4/20/24 12:42 PM, Laurent Pinchart wrote:\n> > On Tue, Apr 16, 2024 at 05:31:38PM +0200, Milan Zamazal wrote:\n> >> Laurent Pinchart writes:\n> >>\n> >>> As for other part of this series, I'm fine starting with this initial\n> >>> implementation and improving it, but I think the black level should\n> >>> eventually be moved before debayering, and ideally the colour gains as\n> >>> well. I understand the need for optimizations to lower the CPU\n> >>> consumption, but at the same time I don't feel comfortable building up\n> >>> on top of an implementation that may work a bit more by chance than by\n> >>> correctness, as that's not very maintainable.\n> >>\n> >> Hi Laurent,\n> >>\n> >> to make sure we understand each other correctly: Can we agree that \"be moved\n> >> before debayering\" above means primarily separating the level adjustments\n> >> computations from debayering code and building them (together with other stuff)\n> >> on top of libipa/algorithm.h? Rather than first applying the corrections on the\n> >> data and only then perform debayering, which luxury we cannot afford, at least\n> >> on CPU? I.e. the first step could be something like\n> >> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/6180d853005bb956b3d2469576b139ff523b76ad ?\n> > \n> > What I meant is subtracting the black level before applying the colour\n> > gains and performing the CFA interpolation (a.k.a. debayering). I'm fine\n> > with the black level subtraction and CFA interpolation being performed\n> > in a single pass by a single function for performance reason, but the\n> > order needs to be swapped.\n> \n> If we apply the per color lookup table (which does black-level compensation +\n> color-gain + gamma-curve in 1 step) to the raw bayer data then we either need to\n> do this on some extra copy before debayering or we need to do it multiple times\n> per pixel before averaging the surrounding pixels, neither one of which is ideal.\n\nIt's not the full lookup table that needs to be applied before\ndebayering, it's only the black level subtraction and the colour gains.\nThe gamma curve needs to go after debayering, and additional histogram\nstretching could go there too.\n\nThere's also a need to add a CCM matrix to the pipeline, multiplying the\nRGB values obtained by debayering by a 3x3 matrix before applying the\ngamma curve.\n\nWith that I think we would have a reasonable image quality already.\nAdditional processing, such as lens shading correction, defective pixel\ncorrection and denoising, are out of scope for CPU processing in my\nopinion. They could be implemented with a GPU soft ISP though.\n\n> I guess we could make the memcpy to temp buffer behavior done to deal with\n> uncached mem mandatory (so always do this) and then first apply the rgb lookups\n> to a just copied raw line before debayering, then we still only need to do\n> this once per color. And this could also help wrt cache behavior of the\n> lookups since we then linearly travel through 1 line touching only the\n> 1 line (in temp buffer) + read 2 lookup tables. So then there will be less\n> cache contention then when using the lookups during debayering when\n> were are reading 3 lines + writing 1 line + accessing all 3 lookup tables\n> (so I guess this might even speed things up especially on the imx8).\n> \n> I don't expect this to make much difference for the resulting image though,\n> OTOH it will make some difference and testing has shown that the overhead\n> of using the extra memcpy buffers in cases where this is not necessary is\n> small. So I guess we could do this.\n> \n> Milan can you extract what I have in mind from the above and is this\n> something you can work on, or shall I take a shot at this when I can schedule\n> some time for this?","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 8E759BE08B\n\tfor ;\n\tSun, 21 Apr 2024 12:22:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AF9A633F5;\n\tSun, 21 Apr 2024 14:22:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5597D61B3D\n\tfor ;\n\tSun, 21 Apr 2024 14:22:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 412ADE1;\n\tSun, 21 Apr 2024 14:21: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=\"YxYzSfJk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713702117;\n\tbh=I5iYI0PLY7oJ4oXZDi/k9ekbCSJvobQQ8ZhuRb4VnHw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YxYzSfJkV9U7ADOvX/Ruc3o1BtQ3u/+yces+8agw4wC5l/5Qqrl4/KCtuhQ6X33Sf\n\t4VMCymcgk3P2XKw2AZml6ycs96Bp4EkmtYtZCwQt0JYl9O0xZTaGeELtn1AYVmFgJ2\n\tLXy2JGwmCJJKEWWRubdZJ5SodnDuX+OmU92FB++0=","Date":"Sun, 21 Apr 2024 15:22:39 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"Milan Zamazal ,\n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"<20240421122239.GB19147@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":29285,"web_url":"https://patchwork.libcamera.org/comment/29285/","msgid":"","date":"2024-04-21T14:47:30","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Laurent,\n\nOn 4/21/24 2:22 PM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> On Sun, Apr 21, 2024 at 02:16:06PM +0200, Hans de Goede wrote:\n>> On 4/20/24 12:42 PM, Laurent Pinchart wrote:\n>>> On Tue, Apr 16, 2024 at 05:31:38PM +0200, Milan Zamazal wrote:\n>>>> Laurent Pinchart writes:\n>>>>\n>>>>> As for other part of this series, I'm fine starting with this initial\n>>>>> implementation and improving it, but I think the black level should\n>>>>> eventually be moved before debayering, and ideally the colour gains as\n>>>>> well. I understand the need for optimizations to lower the CPU\n>>>>> consumption, but at the same time I don't feel comfortable building up\n>>>>> on top of an implementation that may work a bit more by chance than by\n>>>>> correctness, as that's not very maintainable.\n>>>>\n>>>> Hi Laurent,\n>>>>\n>>>> to make sure we understand each other correctly: Can we agree that \"be moved\n>>>> before debayering\" above means primarily separating the level adjustments\n>>>> computations from debayering code and building them (together with other stuff)\n>>>> on top of libipa/algorithm.h? Rather than first applying the corrections on the\n>>>> data and only then perform debayering, which luxury we cannot afford, at least\n>>>> on CPU? I.e. the first step could be something like\n>>>> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/6180d853005bb956b3d2469576b139ff523b76ad ?\n>>>\n>>> What I meant is subtracting the black level before applying the colour\n>>> gains and performing the CFA interpolation (a.k.a. debayering). I'm fine\n>>> with the black level subtraction and CFA interpolation being performed\n>>> in a single pass by a single function for performance reason, but the\n>>> order needs to be swapped.\n>>\n>> If we apply the per color lookup table (which does black-level compensation +\n>> color-gain + gamma-curve in 1 step) to the raw bayer data then we either need to\n>> do this on some extra copy before debayering or we need to do it multiple times\n>> per pixel before averaging the surrounding pixels, neither one of which is ideal.\n> \n> It's not the full lookup table that needs to be applied before\n> debayering, it's only the black level subtraction and the colour gains.\n\nThe lookup table also gives us clamping for free. If we do actual substract\n+ multiply we need to clamp and that means adding 2 conditional jumps\nper pixel and testing has shown that that really really hurts on modern\nprocessors.\n\n> The gamma curve needs to go after debayering, and additional histogram\n> stretching could go there too.\n> \n> There's also a need to add a CCM matrix to the pipeline, multiplying the\n> RGB values obtained by debayering by a 3x3 matrix before applying the\n> gamma curve.\n\nI have looked into CCM already, but IMHO that is too heavy to do on\nthe CPU. We might get away with that at the big Intel Cores, but at\na significant CPU usage and battery cost.\n\nAs for gamma correction I think that we need to live with that being applied\nbefore debayering too (our debayering is a straight averaging of surrounding\npixels, so the impact of doing this before / after should be small).\n\n> With that I think we would have a reasonable image quality already.\n\nI agree that a CCM is the biggest missing item. But I think we can still\nget reasonable quality without that and the CPU SoftwareISP really should\nbe a fallback for when all else fails.\n\nI would rather see us focus on working on a GPU based SoftwareISP and then\nwe can add CCM and proper post debay gamma correction there.\n\n> Additional processing, such as lens shading correction, defective pixel\n> correction and denoising, are out of scope for CPU processing in my\n> opinion. They could be implemented with a GPU soft ISP though.\n\nAck.\n\nRegards,\n\nHans","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 29EC3C3200\n\tfor ;\n\tSun, 21 Apr 2024 14:47:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2610633F5;\n\tSun, 21 Apr 2024 16:47:39 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28D4161B3D\n\tfor ;\n\tSun, 21 Apr 2024 16:47:36 +0200 (CEST)","from mail-lf1-f71.google.com (mail-lf1-f71.google.com\n\t[209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-96-YRUnrtahNpiJnTEztpl3NQ-1; Sun, 21 Apr 2024 10:47:34 -0400","by mail-lf1-f71.google.com with SMTP id\n\t2adb3069b0e04-51aaf264aa9so2312893e87.0\n\tfor ;\n\tSun, 21 Apr 2024 07:47:33 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\tm18-20020a1709060d9200b00a5561456fa8sm4590787eji.21.2024.04.21.07.47.30\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 21 Apr 2024 07:47:31 -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=\"IP16A8Oc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1713710855;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=UmsEMzxTct6jAjwFvM7YVq1MVN8Mun6WBKEkKxn3vxo=;\n\tb=IP16A8Oc6627Ic3WXgMxCzzEhuRFmfoRf4KmZ7elNkYTIy4hhKx4g6OHcLZOrCWJ575I38\n\tkgzDWVqccYKzq3Gj+BbxV5qC+SrHK01T1houCkHfA/Ol/59o1up1lMsGmg4rRKc19lC8l7\n\tw8KryZI3FVLk/SA2bl6NOUoqVn2Nrc8=","X-MC-Unique":"YRUnrtahNpiJnTEztpl3NQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1713710852; x=1714315652;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=UmsEMzxTct6jAjwFvM7YVq1MVN8Mun6WBKEkKxn3vxo=;\n\tb=iu3PvtuMeqPavnZ0l+uNLPE8rGjxJjj3ET1Vk45MAaFXlnAoxaKADviQ9Fh+GbJvr6\n\tC3gCug6FKN42cMSXD2GZMAuSFaYyiiBTXvWcxOho8zaXkxtV+y2vuu3/v+u0MqFUCFpT\n\tp/78Q18w+cs5q4OQVnfOnyDz2bAAq+3Esgh1gHqydpbWnaxQSC4cjUBWTiS8kO9XIxfv\n\tsZZ1NhmtnefnWjFPJ24ccalpaYOaiBSXv7Cw4KgA4/4z4HYOTZrkmd6LVdkUUxLi+bBs\n\tEz7oCjvMlxIG/GdFw4yn8eAWErQZevCw/zVOW2odXihMkGzMc3uZyL49O6/rppdAWbtc\n\t3S4A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUNj+NAdST/e0GxPBk9IlRBwhfWc142sdOydEp5F/WpkKVgW05tgU4dalr3jq73/PY9ewobOxKQz5xYUu6UIY3rN4aVDqjUrcOmF5wLUQs1B5eJZA==","X-Gm-Message-State":"AOJu0YwQisYo9X9zWTX5wqoHpuMse4Xxgp11887E2Ke3bJbkeIXy2Bz1\n\tnpG8X+1mCr8qKk6Pk0bCEzifwDXS+0S1mWxHByOo/A1p8A+YSQqVtZRrhN7T4WmMXJ0UH1echyG\n\tRhgFTeY6DannppH9Ma6MlZkHgrpho8YKvxGleijjH938nARYoM32pz/moR4hIYlKzJG2V4B4=","X-Received":["by 2002:a19:5e11:0:b0:519:7b00:d371 with SMTP id\n\ts17-20020a195e11000000b005197b00d371mr4409923lfb.7.1713710852515; \n\tSun, 21 Apr 2024 07:47:32 -0700 (PDT)","by 2002:a19:5e11:0:b0:519:7b00:d371 with SMTP id\n\ts17-20020a195e11000000b005197b00d371mr4409912lfb.7.1713710852103; \n\tSun, 21 Apr 2024 07:47:32 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IG61A7I3CDOQK1ZkYukeYswCqmq9cSp36TySiwmvM3uQda1INlFHuKEWjD8OAtUHgnU1ALtGw==","Message-ID":"","Date":"Sun, 21 Apr 2024 16:47:30 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","To":"Laurent Pinchart ","Cc":"Milan Zamazal ,\n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240421122239.GB19147@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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":29286,"web_url":"https://patchwork.libcamera.org/comment/29286/","msgid":"<20240421145923.GC29222@pendragon.ideasonboard.com>","date":"2024-04-21T14:59:23","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Sun, Apr 21, 2024 at 04:47:30PM +0200, Hans de Goede wrote:\n> On 4/21/24 2:22 PM, Laurent Pinchart wrote:\n> > On Sun, Apr 21, 2024 at 02:16:06PM +0200, Hans de Goede wrote:\n> >> On 4/20/24 12:42 PM, Laurent Pinchart wrote:\n> >>> On Tue, Apr 16, 2024 at 05:31:38PM +0200, Milan Zamazal wrote:\n> >>>> Laurent Pinchart writes:\n> >>>>\n> >>>>> As for other part of this series, I'm fine starting with this initial\n> >>>>> implementation and improving it, but I think the black level should\n> >>>>> eventually be moved before debayering, and ideally the colour gains as\n> >>>>> well. I understand the need for optimizations to lower the CPU\n> >>>>> consumption, but at the same time I don't feel comfortable building up\n> >>>>> on top of an implementation that may work a bit more by chance than by\n> >>>>> correctness, as that's not very maintainable.\n> >>>>\n> >>>> Hi Laurent,\n> >>>>\n> >>>> to make sure we understand each other correctly: Can we agree that \"be moved\n> >>>> before debayering\" above means primarily separating the level adjustments\n> >>>> computations from debayering code and building them (together with other stuff)\n> >>>> on top of libipa/algorithm.h? Rather than first applying the corrections on the\n> >>>> data and only then perform debayering, which luxury we cannot afford, at least\n> >>>> on CPU? I.e. the first step could be something like\n> >>>> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/6180d853005bb956b3d2469576b139ff523b76ad ?\n> >>>\n> >>> What I meant is subtracting the black level before applying the colour\n> >>> gains and performing the CFA interpolation (a.k.a. debayering). I'm fine\n> >>> with the black level subtraction and CFA interpolation being performed\n> >>> in a single pass by a single function for performance reason, but the\n> >>> order needs to be swapped.\n> >>\n> >> If we apply the per color lookup table (which does black-level compensation +\n> >> color-gain + gamma-curve in 1 step) to the raw bayer data then we either need to\n> >> do this on some extra copy before debayering or we need to do it multiple times\n> >> per pixel before averaging the surrounding pixels, neither one of which is ideal.\n> > \n> > It's not the full lookup table that needs to be applied before\n> > debayering, it's only the black level subtraction and the colour gains.\n> \n> The lookup table also gives us clamping for free. If we do actual substract\n> + multiply we need to clamp and that means adding 2 conditional jumps\n> per pixel and testing has shown that that really really hurts on modern\n> processors.\n\nCould this be implemented with a LUT separate from the gamma LUT ?\n\n> > The gamma curve needs to go after debayering, and additional histogram\n> > stretching could go there too.\n> > \n> > There's also a need to add a CCM matrix to the pipeline, multiplying the\n> > RGB values obtained by debayering by a 3x3 matrix before applying the\n> > gamma curve.\n> \n> I have looked into CCM already, but IMHO that is too heavy to do on\n> the CPU. We might get away with that at the big Intel Cores, but at\n> a significant CPU usage and battery cost.\n\nWould SIMD help ?\n\n> As for gamma correction I think that we need to live with that being applied\n> before debayering too (our debayering is a straight averaging of surrounding\n> pixels, so the impact of doing this before / after should be small).\n\nAs long as we have no CCM that may be an option.\n\n> > With that I think we would have a reasonable image quality already.\n> \n> I agree that a CCM is the biggest missing item. But I think we can still\n> get reasonable quality without that and the CPU SoftwareISP really should\n> be a fallback for when all else fails.\n\nI think the CPU-based ISP is also a useful platform to experiment with\nISP algorithms, even if it consumes more CPU. When we'll have GPU\noffload support, when do you expect we'll fall back to the CPU ?\n\n> I would rather see us focus on working on a GPU based SoftwareISP and then\n> we can add CCM and proper post debay gamma correction there.\n\nI would certainly welcome GPU support :-)\n\n> > Additional processing, such as lens shading correction, defective pixel\n> > correction and denoising, are out of scope for CPU processing in my\n> > opinion. They could be implemented with a GPU soft ISP though.\n> \n> Ack.","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 E7134BE08B\n\tfor ;\n\tSun, 21 Apr 2024 14:59:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF6D3633F9;\n\tSun, 21 Apr 2024 16:59:32 +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 59552633F4\n\tfor ;\n\tSun, 21 Apr 2024 16:59:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32B5ADB7;\n\tSun, 21 Apr 2024 16:58:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WEsEQE8U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713711521;\n\tbh=1+aITNtJNaKMY0CRJ1CIoOHfjCgLcSVSC/nQfCHHfUk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WEsEQE8UT16r6u08NAqlPl5Tw+jAEkydYMRNPKh2fYwgznlyFcHMMElfI+j0NoflC\n\thvHwyhrmrtF8jWns2wWF2U3mMII16J+kMjlqFlKojhFjt0cDDzQhk0ndOnoO9SS47/\n\t8/hEUU5q0w53LN2mhMWlGOp8G8oURAdpBLAmVKjM=","Date":"Sun, 21 Apr 2024 17:59:23 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"Milan Zamazal ,\n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"<20240421145923.GC29222@pendragon.ideasonboard.com>","References":"<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":29287,"web_url":"https://patchwork.libcamera.org/comment/29287/","msgid":"","date":"2024-04-21T20:29:58","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":49,"url":"https://patchwork.libcamera.org/api/people/49/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"Hi!\n\n\n> > > The gamma curve needs to go after debayering, and additional histogram\n> > > stretching could go there too.\n> > > \n> > > There's also a need to add a CCM matrix to the pipeline, multiplying the\n> > > RGB values obtained by debayering by a 3x3 matrix before applying the\n> > > gamma curve.\n> > \n> > I have looked into CCM already, but IMHO that is too heavy to do on\n> > the CPU. We might get away with that at the big Intel Cores, but at\n> > a significant CPU usage and battery cost.\n> \n> Would SIMD help ?\n\nI tried to do debayering using SIMD, and good news is that gcc now has\nsupport so playing with SIMD is easy. Bad news was I was not able to\nget speedups... Neither on x86-64 nor on arm64.\n\n(But for matrix multiplication, it might make sense... not\nsure). Anyway SIMD is _not_ easy.\n\nBad news with GPU is that there's quite some overhead with moving data\nto and from GPU on my platforms.\n\nBest regards,\n\t\t\t\t\t\t\t\tPavel","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 9001CC3200\n\tfor ;\n\tSun, 21 Apr 2024 20:30:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6BF7633F7;\n\tSun, 21 Apr 2024 22:30:01 +0200 (CEST)","from jabberwock.ucw.cz (jabberwock.ucw.cz [46.255.230.98])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67200633F5\n\tfor ;\n\tSun, 21 Apr 2024 22:29:59 +0200 (CEST)","by jabberwock.ucw.cz (Postfix, from userid 1017)\n\tid B801E1C0080; Sun, 21 Apr 2024 22:29:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ucw.cz header.i=@ucw.cz header.b=\"beUh8EXO\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1;\n\tt=1713731398;\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=CbIlee+z1IwN51Prcu/iwEiLkQ3+MRb8jJ+2yKzsW5o=;\n\tb=beUh8EXOGSI6rDRXvfSWHcwLPQBLx0i1VCljvuzABbx+JK03vzmm5WcerYKDuyd6z+qnrN\n\tWwNHpI53wascSt3+jmDFza+6SF35KEJL5UPqjddL35hqc/yZUGhsOc0F4vldI+zY+xzNlg\n\t1p7rExKyyA9nfzSdwmp3/ZGFkvNRFd4=","Date":"Sun, 21 Apr 2024 22:29:58 +0200","From":"Pavel Machek ","To":"Laurent Pinchart ","Cc":"Hans de Goede , Milan Zamazal ,\n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"","References":"<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"PrxSr5d0hYDL/tZS\"","Content-Disposition":"inline","In-Reply-To":"<20240421145923.GC29222@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29288,"web_url":"https://patchwork.libcamera.org/comment/29288/","msgid":"","date":"2024-04-21T21:33:05","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":49,"url":"https://patchwork.libcamera.org/api/people/49/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"Hi!\n\n> If we apply the per color lookup table (which does black-level compensation +\n> color-gain + gamma-curve in 1 step) to the raw bayer data then we either need to\n> do this on some extra copy before debayering or we need to do it multiple times\n> per pixel before averaging the surrounding pixels, neither one of which is ideal.\n> \n> I guess we could make the memcpy to temp buffer behavior done to deal with\n> uncached mem mandatory (so always do this) and then first apply the rgb lookups\n> to a just copied raw line before debayering, then we still only need to do\n> this once per color. And this could also help wrt cache behavior of\n> the\n\nI believe that doing extra memcpy will hurt (but black-level during\nmemcpy will be free)\n\nOTOH doing the pass at convenient time when data is already in L1\ncache should be free, too.\n\nAnyway, point to consider. If we are talking about correcting gammas\netc, we'll probably need to switch processing to 16-bit values. Those\nsensors are pretty non-linear...\n\nhttps://blog.brixit.nl/fixing-the-megapixels-sensor-linearization/\n\nAnyway there's ton of work, and it would be good to get the basics\nmerged, first. For example, for digital photography, it is important\nto do statistics and AAA algorithms, but not debayer...\n\nBest regards,\n\t\t\t\t\t\t\t\tPavel","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 3D6AABE08B\n\tfor ;\n\tSun, 21 Apr 2024 21:33:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 05751633FA;\n\tSun, 21 Apr 2024 23:33:09 +0200 (CEST)","from jabberwock.ucw.cz (jabberwock.ucw.cz [46.255.230.98])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E26D633F5\n\tfor ;\n\tSun, 21 Apr 2024 23:33:07 +0200 (CEST)","by jabberwock.ucw.cz (Postfix, from userid 1017)\n\tid 7269C1C0084; Sun, 21 Apr 2024 23:33:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ucw.cz header.i=@ucw.cz header.b=\"pLRW+Aev\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1;\n\tt=1713735186;\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=x3OhSJVAPTx2qB1zEQBRdSGosgh3B3uLqV3IM/G53fY=;\n\tb=pLRW+AeveuRHaJ0rFgHmNq2/Q5Gs+988sbnJjS1fpuf02Hn/BmZKxV8OczQoJNGVK0TFqe\n\tLB7fdvPVsdWPnaaZSZ3jYMb7dL92HxdmNiLWDn8ZX5Bqr88mUPairWwUgvHFjmvqJB/vyX\n\t4x1kkpZ5M6HLE7hD35S9SsIZBB72kNA=","Date":"Sun, 21 Apr 2024 23:33:05 +0200","From":"Pavel Machek ","To":"Hans de Goede ","Cc":"Laurent Pinchart ,\n\tMilan Zamazal ,\n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Message-ID":"","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-18-mzamazal@redhat.com>\n\t<20240327185543.GN4721@pendragon.ideasonboard.com>\n\t<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"4SY8XdaUxOW+C8ru\"","Content-Disposition":"inline","In-Reply-To":"","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":29294,"web_url":"https://patchwork.libcamera.org/comment/29294/","msgid":"<87ttjty707.fsf@redhat.com>","date":"2024-04-22T11:24:56","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi,\n\nthank you all for your comments and clarifications. Let me summarize the\ndiscussion, with my notes merged in, before I get lost in it:\n\n- Since our current black level and color gain operations are linear, we average\n only the same color pixels and the lookup table is applied after averaging the\n pixels, the current debayering implementation is correct. It doesn't matter\n whether we do \"average pixels -> subtract black -> multiply by color gain ->\n apply gamma\" or \"subtract black -> multiply by color gain -> average pixels ->\n apply gamma\".\n\n- This may no longer hold if we change the operations, for example:\n\n- The optimization suggested by Hans (preprocessing while copying lines from the\n input buffer). It may or may not have a significant impact on the performance\n and may or may not have a significant impact on the image quality. I think we\n need some experiments here. Performance on the CPU is generally challenging,\n so if we can demonstrate a performance benefit, we should consider including\n it (most likely configurable), if it doesn't turn the implementation into a\n complete mess.\n\n- Or if we add support for non-linear gains, as mentioned by Pavel.\n\n- Laurent suggested adding a CCM matrix between debayering and gamma to achieve\n a reasonable image quality. According to Hans, this is a heavy operation on\n CPUs. Laurent still sees CPU processing as useful for experiments. I think\n we can make this simply configurable (if it gets implemented).\n\n- If we split the processing to pre-bayer and post-bayer parts, we should\n probably work with uint16_t or float's, which may have impact on performance.\n\n- Pavel couldn't get a better performance by using SIMD CPU instructions for\n debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n CPU is hard to use and may differ on architectures, so the question is whether\n it's worth to invest into it.\n\n- GPU processing should make many of these things easier and more things\n possible.\n\n- Do we already know how to map the buffers to GPU efficiently?\n\nMy conclusions are:\n\n- The current CPU debayering computation itself is fine at the moment and we\n shouldn't change it without a good reason. It can be replaced in future if\n needed, once we have a better understanding of what and how we can\n realistically achieve. General cleanups, like moving table computations\n elsewhere, would be still useful already now.\n\n- We can experiment with whatever mentioned above to get the better\n understanding, but:\n\n- GPU processing is clearly a priority.\n\n- We have also other items in the TODO file! (I already work on some of them.)\n\n- We should probably change the e-mail thread subject.\n\nThanks,\nMilan","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 5BFACBE08B\n\tfor ;\n\tMon, 22 Apr 2024 11:25:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51E9E63407;\n\tMon, 22 Apr 2024 13:25:04 +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 A448A633FA\n\tfor ;\n\tMon, 22 Apr 2024 13:25:02 +0200 (CEST)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-155-Nnfo5IF5Pk2Ec6tY0hHLpg-1; Mon, 22 Apr 2024 07:24:59 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a526a77445bso297274966b.2\n\tfor ;\n\tMon, 22 Apr 2024 04:24:59 -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\tx21-20020a1709060a5500b00a521891f8cbsm5666713ejf.224.2024.04.22.04.24.57\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 22 Apr 2024 04:24:57 -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=\"ef7breS3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1713785101;\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=Ok+OP7p6emy3/jLUD0Fbr2TjY8hh4hDDMbDrGdgMdQI=;\n\tb=ef7breS3utmrfgwDQ1epl1SqYAIIpTi7B6yu3EW8j35lkomOiyDhHU6R408mO+0WIXDSPP\n\tOeZ7AGAkRwQ/wSbJWQeZmyjED5izaWw0OY8je/5V4r6UK4H+dnLRDP1/orcRgN+TKoP1rs\n\tAsMlFnaw76tEml4dKpEffeaXC5xoyvw=","X-MC-Unique":"Nnfo5IF5Pk2Ec6tY0hHLpg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1713785099; x=1714389899;\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=Ok+OP7p6emy3/jLUD0Fbr2TjY8hh4hDDMbDrGdgMdQI=;\n\tb=WQHrTZ9D6LSLN0u8Ft4A5YKUrkhRpgqh0GBBUxTt4Mn/FumheHqtfhBQqAWx7PHSBE\n\t5ubu3F0F2zVH2by+c1REheR6DN+SgEii0vZW2gSEz32TEAIyKWDYszlbbIaQPUADt374\n\tPL7YZ98xWRMX1/+Fc2nZADdlLFSmvh+yc/2JpE0y2G0vHQjpBqNkUE28nQ/pNy31Xti4\n\tGaVCv6u2sqCTCl1IVHj5hpL+SOlsvtapu1IijDx5Fs29wHM3M02ZvRskhO9MqSi5ZDbj\n\te86lehrguZygve+3O7b7ydNUX8ByYunJDUDEnZ2sk0eQSQ/Tl6fVPWBSHueHST4upvm8\n\toV3Q==","X-Gm-Message-State":"AOJu0YzgQqQjLeicqa1GsCVTKyhXm6wsvYmbQDJkoIB+m7Zmr0z+obFz\n\tFEWGrcs3EyOmwpVC0ZV+kHUM3g8QtyUXqJDy0RX+IEOSofS31YKiTj5MBshGiJjbDT6QKsnme7z\n\tDHzo78QM9X21eUbtjutGyqYlzj49JRaJcG+UizfWStzuXw8aZQ17kBp7dDhg8Y4DdnxuKhb8=","X-Received":["by 2002:a17:906:1f04:b0:a52:3645:22b6 with SMTP id\n\tw4-20020a1709061f0400b00a52364522b6mr6249983ejj.1.1713785098801; \n\tMon, 22 Apr 2024 04:24:58 -0700 (PDT)","by 2002:a17:906:1f04:b0:a52:3645:22b6 with SMTP id\n\tw4-20020a1709061f0400b00a52364522b6mr6249963ejj.1.1713785098330; \n\tMon, 22 Apr 2024 04:24:58 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IErPbXLf0lDgJzxnvbkDsZGW3yH9wHgxZPNSytbLPMkxn0fhLtpx89Jr21kFNFJ7PwoXmfCFQ==","From":"Milan Zamazal ","To":"libcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart , Hans de Goede\n\t, David Plowman , \n\tPavel Machek , Andrey Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Kieran\n\tBingham ","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","In-Reply-To":" (Pavel Machek's message of \"Sun,\n\t21 Apr 2024 22:29:58 +0200\")","References":"<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t","Date":"Mon, 22 Apr 2024 13:24:56 +0200","Message-ID":"<87ttjty707.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":29297,"web_url":"https://patchwork.libcamera.org/comment/29297/","msgid":"<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>","date":"2024-04-22T11:38:58","subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 4/22/24 1:24 PM, Milan Zamazal wrote:\n> Hi,\n> \n> thank you all for your comments and clarifications. Let me summarize the\n> discussion, with my notes merged in, before I get lost in it:\n> \n> - Since our current black level and color gain operations are linear, we average\n> only the same color pixels and the lookup table is applied after averaging the\n> pixels, the current debayering implementation is correct. It doesn't matter\n> whether we do \"average pixels -> subtract black -> multiply by color gain ->\n> apply gamma\" or \"subtract black -> multiply by color gain -> average pixels ->\n> apply gamma\".\n\nThis is not entirely true, because of the clamping involved in black-level\nsubtraction (and the same for clamping at the top for gains), e.g.\n\nLets say that for the red component we average 2 pixels with a blacklevel\ncompensation of 8 for all colors and the value of the 2 pixels is:\n6 and 10, then if we average first and then do blacklevel compensation the\nresult of blacklevel correction is 2 pixels with a value of 0 and 2,\nwhich averages to 1. Where as if we first average we get an average of 8\nand then after blacklevel compensation we end up with 0.\n\n\n\n>\n> - This may no longer hold if we change the operations, for example:\n> \n> - The optimization suggested by Hans (preprocessing while copying lines from the\n> input buffer). It may or may not have a significant impact on the performance\n> and may or may not have a significant impact on the image quality. I think we\n> need some experiments here. Performance on the CPU is generally challenging,\n> so if we can demonstrate a performance benefit, we should consider including\n> it (most likely configurable), if it doesn't turn the implementation into a\n> complete mess.\n\nI think that if we want to move to applying blacklevel + gain before\ndebayering that we then should do this unconditionally, which means also\nunconditionally doing the memcpy to a temporary line buffer.\n\n> \n> - Or if we add support for non-linear gains, as mentioned by Pavel.\n> \n> - Laurent suggested adding a CCM matrix between debayering and gamma to achieve\n> a reasonable image quality. According to Hans, this is a heavy operation on\n> CPUs. Laurent still sees CPU processing as useful for experiments. I think\n> we can make this simply configurable (if it gets implemented).\n\nI agree that adding a CCM matrix as an optional step sounds interesting\nand this should indeed be configurable. This can be done as an optional\n(gaurded by a single if per line) post-debayer step run on the output buffer.\n\n> - If we split the processing to pre-bayer and post-bayer parts, we should\n> probably work with uint16_t or float's, which may have impact on performance.\n> \n> - Pavel couldn't get a better performance by using SIMD CPU instructions for\n> debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n> CPU is hard to use and may differ on architectures, so the question is whether\n> it's worth to invest into it.\n> \n> - GPU processing should make many of these things easier and more things\n> possible.\n> \n> - Do we already know how to map the buffers to GPU efficiently?\n> \n> My conclusions are:\n> \n> - The current CPU debayering computation itself is fine at the moment and we\n> shouldn't change it without a good reason. It can be replaced in future if\n> needed, once we have a better understanding of what and how we can\n> realistically achieve. General cleanups, like moving table computations\n> elsewhere, would be still useful already now.\n> \n> - We can experiment with whatever mentioned above to get the better\n> understanding, but:\n> \n> - GPU processing is clearly a priority.\n> \n> - We have also other items in the TODO file! (I already work on some of them.)\n> \n> - We should probably change the e-mail thread subject.\n\nI agree with all of your conclusions :)\n\nRegards,\n\nHans","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 76F6EBE08B\n\tfor ;\n\tMon, 22 Apr 2024 11:39:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1834163407;\n\tMon, 22 Apr 2024 13:39:07 +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 33792633FD\n\tfor ;\n\tMon, 22 Apr 2024 13:39:04 +0200 (CEST)","from mail-lf1-f72.google.com (mail-lf1-f72.google.com\n\t[209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-341-5T6-2HzGNJ6ZrnMdefmgMA-1; Mon, 22 Apr 2024 07:39:01 -0400","by mail-lf1-f72.google.com with SMTP id\n\t2adb3069b0e04-516d6c879c5so2509699e87.3\n\tfor ;\n\tMon, 22 Apr 2024 04:39:01 -0700 (PDT)","from [10.40.98.157] ([78.108.130.194])\n\tby smtp.gmail.com with ESMTPSA id\n\tbi10-20020a170906a24a00b00a54c12de34dsm5617206ejb.188.2024.04.22.04.38.59\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 22 Apr 2024 04:38:59 -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=\"GGL4v6dV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1713785943;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=5TyarYXyu2OcS852vavki4WUeKVCflpb90CG0H8qwzE=;\n\tb=GGL4v6dVRs/VAeE5uT5uccH0bn5wgQsq5Mbw8Gt/Nx/+Yvg07fn4acivUhH3yF6Yzv15MG\n\tM0PHYibOIFXpe97QgLEkeWgn2DuJor50R4wxWEEYsOtcImiRQdTyY1G1JPULRJYNdeKtKR\n\tZtWR4Iy7fbRw5Wvsib4wdqRRbDfuS5c=","X-MC-Unique":"5T6-2HzGNJ6ZrnMdefmgMA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1713785940; x=1714390740;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=5TyarYXyu2OcS852vavki4WUeKVCflpb90CG0H8qwzE=;\n\tb=VqVRs6ntR4FFERW0s2C1oW4JVw15V4cyEU3smyqHT4Y3a4uRV9aJ1ktUuqVY2zq0ad\n\tvMJF1FYEIeZkfKsG+J9Q9+7LjmfHKPUBNEZyjaS7EC90SylQH+it0K/kCnJDHQr1tE2O\n\tV7aq2Nyy9ROGUxNPfdoK7C/LMZPLE/TSREAxCknOxza+QCDU6ifHtB/Y4CNFhj5VQyet\n\tbbChgzbEFPnIpQfMomJElawDybNhhDPgL8jgTA0wyY6gp8XzvrbL8N3CmRT1e2yORUEC\n\tQyu4VGRSlh5OGNezOegc+Nrh8ldkoqOK60sxJNqgVzSSwXHXVcTIMt4Hkr7A5b5s5pzc\n\tBUdw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCW7UBraVao1T+JrQAPO3clzP9zFrEausNJOtzEX6AJCV28B/LgqlJFKvTrmobPjQcm4lSFHxTP6PYygP/F2n+i1rLcxbZja5BYWnjijosz3bx/uag==","X-Gm-Message-State":"AOJu0Yz22qubctfR/P8qe5VtM1HC4CdhmRKqRxuHAH40bBOHxhl27Vj5\n\toTiZbTHOgdV9Exo532UV+Xs+05JkscWKAc09FyWjFC+KRJbB72RKf6i0LnVhVkMDm9fJ2QK//QO\n\t7gz0iBVBch8cAonTkIoIyQHMkN/zrjDpmsTi37Xi5syK9VDk5JvuZPf0HosWdQq47yshsIQI=","X-Received":["by 2002:ac2:5a44:0:b0:51a:bb5c:56e5 with SMTP id\n\tr4-20020ac25a44000000b0051abb5c56e5mr6423315lfn.2.1713785940133; \n\tMon, 22 Apr 2024 04:39:00 -0700 (PDT)","by 2002:ac2:5a44:0:b0:51a:bb5c:56e5 with SMTP id\n\tr4-20020ac25a44000000b0051abb5c56e5mr6423291lfn.2.1713785939722; \n\tMon, 22 Apr 2024 04:38:59 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IED8nhlo836/B6RRXGEj/aKCCZFMDq+ZYmw1TOAgfgLbvP6W6Uw1grRbpCQY/PcTlum1kLAiQ==","Message-ID":"<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>","Date":"Mon, 22 Apr 2024 13:38:58 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart ,\n\tDavid Plowman ,\n\tPavel Machek , \n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","References":"<87h6gqis2p.fsf@redhat.com>\n\t\n\t<87le62h0qj.fsf@redhat.com>\n\t<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t <87ttjty707.fsf@redhat.com>","From":"Hans de Goede ","In-Reply-To":"<87ttjty707.fsf@redhat.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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":29298,"web_url":"https://patchwork.libcamera.org/comment/29298/","msgid":"<20240422121246.GA2438@pendragon.ideasonboard.com>","date":"2024-04-22T12:12:46","subject":"Soft ISP TODO list (was: Re: [PATCH v6 17/18] libcamera:\n\tsoftware_isp: Apply black level compensation)","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Apr 22, 2024 at 01:38:58PM +0200, Hans de Goede wrote:\n> On 4/22/24 1:24 PM, Milan Zamazal wrote:\n> > Hi,\n> > \n> > thank you all for your comments and clarifications. Let me summarize the\n> > discussion, with my notes merged in, before I get lost in it:\n> > \n> > - Since our current black level and color gain operations are linear, we average\n> > only the same color pixels and the lookup table is applied after averaging the\n> > pixels, the current debayering implementation is correct. It doesn't matter\n> > whether we do \"average pixels -> subtract black -> multiply by color gain ->\n> > apply gamma\" or \"subtract black -> multiply by color gain -> average pixels ->\n> > apply gamma\".\n> \n> This is not entirely true, because of the clamping involved in black-level\n> subtraction (and the same for clamping at the top for gains), e.g.\n> \n> Lets say that for the red component we average 2 pixels with a blacklevel\n> compensation of 8 for all colors and the value of the 2 pixels is:\n> 6 and 10, then if we average first and then do blacklevel compensation the\n> result of blacklevel correction is 2 pixels with a value of 0 and 2,\n> which averages to 1. Where as if we first average we get an average of 8\n> and then after blacklevel compensation we end up with 0.\n> \n> > - This may no longer hold if we change the operations, for example:\n> > \n> > - The optimization suggested by Hans (preprocessing while copying lines from the\n> > input buffer). It may or may not have a significant impact on the performance\n> > and may or may not have a significant impact on the image quality. I think we\n> > need some experiments here. Performance on the CPU is generally challenging,\n> > so if we can demonstrate a performance benefit, we should consider including\n> > it (most likely configurable), if it doesn't turn the implementation into a\n> > complete mess.\n> \n> I think that if we want to move to applying blacklevel + gain before\n> debayering that we then should do this unconditionally, which means also\n> unconditionally doing the memcpy to a temporary line buffer.\n\nIf we can move it before debayering with acceptable performance, then I\nwouldn't make it configurable.\n\n> > - Or if we add support for non-linear gains, as mentioned by Pavel.\n\nSensors are mostly linear these days. That is, until they reach\nsaturation of course. I don't think the soft ISP needs a linearization\nLUT.\n\n> > - Laurent suggested adding a CCM matrix between debayering and gamma to achieve\n> > a reasonable image quality. According to Hans, this is a heavy operation on\n> > CPUs. Laurent still sees CPU processing as useful for experiments. I think\n> > we can make this simply configurable (if it gets implemented).\n> \n> I agree that adding a CCM matrix as an optional step sounds interesting\n> and this should indeed be configurable. This can be done as an optional\n> (gaurded by a single if per line) post-debayer step run on the output buffer.\n\nGiven that the debayering currently produces an RGB value, do you think\nan additional multiplication by a 3x3 matrix (ideally accelerated by\nSIMD) would be that costly ?\n\n> > - If we split the processing to pre-bayer and post-bayer parts, we should\n> > probably work with uint16_t or float's, which may have impact on performance.\n> > \n> > - Pavel couldn't get a better performance by using SIMD CPU instructions for\n> > debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n> > CPU is hard to use and may differ on architectures, so the question is whether\n> > it's worth to invest into it.\n\nGood question :-)\n\n> > - GPU processing should make many of these things easier and more things\n> > possible.\n> > \n> > - Do we already know how to map the buffers to GPU efficiently?\n\nI haven't looked into it yet personally.\n\n> > My conclusions are:\n> > \n> > - The current CPU debayering computation itself is fine at the moment and we\n> > shouldn't change it without a good reason. It can be replaced in future if\n> > needed, once we have a better understanding of what and how we can\n> > realistically achieve. General cleanups, like moving table computations\n> > elsewhere, would be still useful already now.\n> > \n> > - We can experiment with whatever mentioned above to get the better\n> > understanding, but:\n> > \n> > - GPU processing is clearly a priority.\n> > \n> > - We have also other items in the TODO file! (I already work on some of them.)\n\nOn this topic, I'm having a look at how we could use the soft ISP with\nthe vimc pipeline handler.\n\n> > - We should probably change the e-mail thread subject.\n\nDone :-)\n\n> I agree with all of your conclusions :)","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 71A62C3200\n\tfor ;\n\tMon, 22 Apr 2024 12:12:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62A5E6340C;\n\tMon, 22 Apr 2024 14:12:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66C47633FF\n\tfor ;\n\tMon, 22 Apr 2024 14:12:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9AF596B3;\n\tMon, 22 Apr 2024 14:12: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=\"pRlZNrar\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713787924;\n\tbh=dX6DACmTb9wEs3tyOe+ybc1Cll43PSf+cp/Ye95ypm0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pRlZNrarnQ3uI72yckLzXFaA43EWu/jOoclpgvTVt12F24d3q0BC58Q8qVUAjRo3Q\n\tQ61SE/siZr3uA9WBFb9XTEXYbf49R6vpcVemJMrQas2WUc+y2V5wIk90Ix0IJDtYzi\n\tXwDXGpbhh4gCgoSmMsA/vXH/Gr7HB9WkPAIYAbdI=","Date":"Mon, 22 Apr 2024 15:12:46 +0300","From":"Laurent Pinchart ","To":"Hans de Goede ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tPavel Machek , \n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","Subject":"Soft ISP TODO list (was: Re: [PATCH v6 17/18] libcamera:\n\tsoftware_isp: Apply black level compensation)","Message-ID":"<20240422121246.GA2438@pendragon.ideasonboard.com>","References":"<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t <87ttjty707.fsf@redhat.com>\n\t<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@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":29299,"web_url":"https://patchwork.libcamera.org/comment/29299/","msgid":"<1bfa8a24-a10b-4536-8112-b19ec5f11cfd@redhat.com>","date":"2024-04-22T12:42:24","subject":"Re: Soft ISP TODO list","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 4/22/24 2:12 PM, Laurent Pinchart wrote:\n> Hello,\n> \n> On Mon, Apr 22, 2024 at 01:38:58PM +0200, Hans de Goede wrote:\n>> On 4/22/24 1:24 PM, Milan Zamazal wrote:\n>>> Hi,\n>>>\n>>> thank you all for your comments and clarifications. Let me summarize the\n>>> discussion, with my notes merged in, before I get lost in it:\n>>>\n>>> - Since our current black level and color gain operations are linear, we average\n>>> only the same color pixels and the lookup table is applied after averaging the\n>>> pixels, the current debayering implementation is correct. It doesn't matter\n>>> whether we do \"average pixels -> subtract black -> multiply by color gain ->\n>>> apply gamma\" or \"subtract black -> multiply by color gain -> average pixels ->\n>>> apply gamma\".\n>>\n>> This is not entirely true, because of the clamping involved in black-level\n>> subtraction (and the same for clamping at the top for gains), e.g.\n>>\n>> Lets say that for the red component we average 2 pixels with a blacklevel\n>> compensation of 8 for all colors and the value of the 2 pixels is:\n>> 6 and 10, then if we average first and then do blacklevel compensation the\n>> result of blacklevel correction is 2 pixels with a value of 0 and 2,\n>> which averages to 1. Where as if we first average we get an average of 8\n>> and then after blacklevel compensation we end up with 0.\n>>\n>>> - This may no longer hold if we change the operations, for example:\n>>>\n>>> - The optimization suggested by Hans (preprocessing while copying lines from the\n>>> input buffer). It may or may not have a significant impact on the performance\n>>> and may or may not have a significant impact on the image quality. I think we\n>>> need some experiments here. Performance on the CPU is generally challenging,\n>>> so if we can demonstrate a performance benefit, we should consider including\n>>> it (most likely configurable), if it doesn't turn the implementation into a\n>>> complete mess.\n>>\n>> I think that if we want to move to applying blacklevel + gain before\n>> debayering that we then should do this unconditionally, which means also\n>> unconditionally doing the memcpy to a temporary line buffer.\n> \n> If we can move it before debayering with acceptable performance, then I\n> wouldn't make it configurable.\n> \n>>> - Or if we add support for non-linear gains, as mentioned by Pavel.\n> \n> Sensors are mostly linear these days. That is, until they reach\n> saturation of course. I don't think the soft ISP needs a linearization\n> LUT.\n> \n>>> - Laurent suggested adding a CCM matrix between debayering and gamma to achieve\n>>> a reasonable image quality. According to Hans, this is a heavy operation on\n>>> CPUs. Laurent still sees CPU processing as useful for experiments. I think\n>>> we can make this simply configurable (if it gets implemented).\n>>\n>> I agree that adding a CCM matrix as an optional step sounds interesting\n>> and this should indeed be configurable. This can be done as an optional\n>> (gaurded by a single if per line) post-debayer step run on the output buffer.\n> \n> Given that the debayering currently produces an RGB value, do you think\n> an additional multiplication by a 3x3 matrix (ideally accelerated by\n> SIMD) would be that costly ?\n\nCurrently we do max 4 additions + 1 divide per pixel. this adds * 9 *\nmultiplications + 6 adds per pixel. With FHD at 30 FPS that is\n~560 million multiplications per second!\n\nAnd many sensors do way higher resolutions/fps.\n\nMaybe with SIMD we can do some of these in parallel and it certainly\nis interesting to try at least on some beefy x86 laptops (or upcoming\nbeefy ARM laptops) but this is significantly going to hurt performance.\n\nI'm not saying this is not interesting to experiment with, but I have\ndoubts this is something which we will enable by default anywhere.\n\nRegards,\n\nHans\n\n\n\n\n\n\n> \n>>> - If we split the processing to pre-bayer and post-bayer parts, we should\n>>> probably work with uint16_t or float's, which may have impact on performance.\n>>>\n>>> - Pavel couldn't get a better performance by using SIMD CPU instructions for\n>>> debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n>>> CPU is hard to use and may differ on architectures, so the question is whether\n>>> it's worth to invest into it.\n> \n> Good question :-)\n> \n>>> - GPU processing should make many of these things easier and more things\n>>> possible.\n>>>\n>>> - Do we already know how to map the buffers to GPU efficiently?\n> \n> I haven't looked into it yet personally.\n> \n>>> My conclusions are:\n>>>\n>>> - The current CPU debayering computation itself is fine at the moment and we\n>>> shouldn't change it without a good reason. It can be replaced in future if\n>>> needed, once we have a better understanding of what and how we can\n>>> realistically achieve. General cleanups, like moving table computations\n>>> elsewhere, would be still useful already now.\n>>>\n>>> - We can experiment with whatever mentioned above to get the better\n>>> understanding, but:\n>>>\n>>> - GPU processing is clearly a priority.\n>>>\n>>> - We have also other items in the TODO file! (I already work on some of them.)\n> \n> On this topic, I'm having a look at how we could use the soft ISP with\n> the vimc pipeline handler.\n> \n>>> - We should probably change the e-mail thread subject.\n> \n> Done :-)\n> \n>> I agree with all of your conclusions :)\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 9BF4BBE08B\n\tfor ;\n\tMon, 22 Apr 2024 12:42:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A6D26340F;\n\tMon, 22 Apr 2024 14:42:31 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7ADEA63407\n\tfor ;\n\tMon, 22 Apr 2024 14:42:29 +0200 (CEST)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-550-FmM5JuGENYmJ78RQefNQkQ-1; Mon, 22 Apr 2024 08:42:27 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a5238ca77adso623506466b.1\n\tfor ;\n\tMon, 22 Apr 2024 05:42:26 -0700 (PDT)","from [10.40.98.157] ([78.108.130.194])\n\tby smtp.gmail.com with ESMTPSA id\n\tbt15-20020a170906b14f00b00a51eed4f0d7sm5727401ejb.130.2024.04.22.05.42.25\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 22 Apr 2024 05:42:25 -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=\"IjPBt1nK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1713789748;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=FUgQDt+V+Aj6MTYaGQc/SVLlpPOp+007+9dxLEKOQwA=;\n\tb=IjPBt1nKyczF1GefGI5N4N/kfyH7vxTOfwIJC06hCtFD2fE97FqO9xzC2Xx7/pUBEwpjBX\n\tjDe61C0q0mzoEEvv9HoatGnNsLUJLV450L8TNKuf2tYTRRlSXmuBuYTpYiTWV/UECBMGLm\n\trPf7cTq3WyTanLKePj8Bfe2lghDeY8E=","X-MC-Unique":"FmM5JuGENYmJ78RQefNQkQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1713789746; x=1714394546;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=FUgQDt+V+Aj6MTYaGQc/SVLlpPOp+007+9dxLEKOQwA=;\n\tb=Z7gp/J9PvUGiASbhtrbck7O6IlgzvCpt5aeIWWrwfYaIdaes48BKHevWzGh/jEDnVJ\n\trdjDdSsEesmiPucebmzT1gPU3z40NIfM5aGpZmqs9cXvXvWRNIWGn+mvWYvnC1re77Ml\n\tk+8aTjnHPLCHOaE9v1LzcbIAsgv+5FrB9gPsL33Psv37NuBChta+SjI7jFokY/RUh1u3\n\tRCqw5KqLwXiwPRNXuIyp2sJyis/GINDnVl3rzr9Rx3BgPGhK0DiXza2Ch6fLtffB3V8L\n\tgPCHGKXeWnPkaAgF1PQmId1tGj7Evp8WuTlWOcKs6jbLh80kv6HIB0AMFN7XStIT1nut\n\t6olg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXUBAZwrtZ8aKzx+OZmqyE8jea5IaqrydPB1Up3iIk6qNaoa8GMp4/k6ymU+6nfztR2xe3mJByWig6RR9jpXBUs658UPB9vMzaW02SYM5lpEcYgzQ==","X-Gm-Message-State":"AOJu0YwK8p/fD6dILSp1InyLIzxB6OszSX/LpmFNmyrxCg90FFETD8VY\n\tZcib0svIyjrMAkmjy3aL/GNGYu9hbUJf2qtj9kzde2ILqwF9AEQ01Zmyh80s8cSRkcYtJHOWoEM\n\tprgazH82OioeFPVzBDWEfNfeOIoleoDpFOt4oiVMpqQmLQM5L6lBeTLN3rw3taskZwXeC7g0=","X-Received":["by 2002:a17:906:b190:b0:a52:182b:dee9 with SMTP id\n\tw16-20020a170906b19000b00a52182bdee9mr7725346ejy.35.1713789745933; \n\tMon, 22 Apr 2024 05:42:25 -0700 (PDT)","by 2002:a17:906:b190:b0:a52:182b:dee9 with SMTP id\n\tw16-20020a170906b19000b00a52182bdee9mr7725329ejy.35.1713789745572; \n\tMon, 22 Apr 2024 05:42:25 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEXFjkddORxWrhL542w9TXwBCW2E1/BXDMjeD3ZIliBofoutz4hTGl9lWo8+XXDMtuMQJwCUQ==","Message-ID":"<1bfa8a24-a10b-4536-8112-b19ec5f11cfd@redhat.com>","Date":"Mon, 22 Apr 2024 14:42:24 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: Soft ISP TODO list","To":"Laurent Pinchart ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tPavel Machek , \n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","References":"<20240402220753.GG16740@pendragon.ideasonboard.com>\n\t<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t <87ttjty707.fsf@redhat.com>\n\t<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>\n\t<20240422121246.GA2438@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240422121246.GA2438@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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":29302,"web_url":"https://patchwork.libcamera.org/comment/29302/","msgid":"","date":"2024-04-22T19:28:08","subject":"Re: Soft ISP TODO list (was: Re: [PATCH v6 17/18] libcamera:\n\tsoftware_isp: Apply black level compensation)","submitter":{"id":49,"url":"https://patchwork.libcamera.org/api/people/49/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"Hi!\n\n> > > - Or if we add support for non-linear gains, as mentioned by Pavel.\n> \n> Sensors are mostly linear these days. That is, until they reach\n> saturation of course. I don't think the soft ISP needs a linearization\n> LUT.\n\nThat's not what Martijn measured:\nhttps://blog.brixit.nl/fixing-the-megapixels-sensor-linearization/\nLibrem 5 is reasonably modern.\n\nBasically it is linear in 0..70% range, and then it is linear again in\n90%..100% range, but at different slope. Take a look, it is fun article.\n\n> > > - If we split the processing to pre-bayer and post-bayer parts, we should\n> > > probably work with uint16_t or float's, which may have impact on performance.\n> > > \n> > > - Pavel couldn't get a better performance by using SIMD CPU instructions for\n> > > debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n> > > CPU is hard to use and may differ on architectures, so the question is whether\n> > > it's worth to invest into it.\n> \n> Good question :-)\n\nOh, so good news is you write SIMD code once with gcc intristics, and\ngcc does its magic. You don't have to know assembly for that, but it\ncertainly helps to look at the assembly if it looks reasonable.\n\n(Debayering needs too much of byte shuffling to work well with SIMD\nextensions. Matrix multiply could be better, but I'm not sure if 3x3\nmatrix is big enough to get advantage).\n\nBest regards,\n\t\t\t\t\t\t\t\tPavel","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 39072C3200\n\tfor ;\n\tMon, 22 Apr 2024 19:28:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E771963417;\n\tMon, 22 Apr 2024 21:28:12 +0200 (CEST)","from jabberwock.ucw.cz (jabberwock.ucw.cz [46.255.230.98])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BAB1963412\n\tfor ;\n\tMon, 22 Apr 2024 21:28:09 +0200 (CEST)","by jabberwock.ucw.cz (Postfix, from userid 1017)\n\tid 4A9221C0080; Mon, 22 Apr 2024 21:28:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ucw.cz header.i=@ucw.cz header.b=\"R06G9ISj\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1;\n\tt=1713814089;\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=d5wxHvRzQXI9uX6hCbk7hkFLWdTW90VKLWduq08bYb8=;\n\tb=R06G9ISjwQf3JzelA9JbuBvcL3aKjMLHgJo03K+RF38OzLO2aoWw2DYCxqPHET4AMy2odW\n\t78rNMNRIMbu8AtiUsQRR6EiOG/09RlV8ItL3jlV/UrKuEjG0SB7qyjUCfKnNC/S9PfbacI\n\tWSF+UhBAoNTwZl7i9dNYY0ovnikRQw4=","Date":"Mon, 22 Apr 2024 21:28:08 +0200","From":"Pavel Machek ","To":"Laurent Pinchart ","Cc":"Hans de Goede , Milan Zamazal ,\n\tlibcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","Subject":"Re: Soft ISP TODO list (was: Re: [PATCH v6 17/18] libcamera:\n\tsoftware_isp: Apply black level compensation)","Message-ID":"","References":"<87plupjpd1.fsf@redhat.com>\n\t<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t <87ttjty707.fsf@redhat.com>\n\t<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>\n\t<20240422121246.GA2438@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"V6iNWIQwvV80qUhV\"","Content-Disposition":"inline","In-Reply-To":"<20240422121246.GA2438@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29303,"web_url":"https://patchwork.libcamera.org/comment/29303/","msgid":"<20240422194749.GE2438@pendragon.ideasonboard.com>","date":"2024-04-22T19:47:49","subject":"Re: Soft ISP TODO list (was: Re: [PATCH v6 17/18] libcamera:\n\tsoftware_isp: Apply black level compensation)","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Apr 22, 2024 at 09:28:08PM +0200, Pavel Machek wrote:\n> Hi!\n> \n> > > > - Or if we add support for non-linear gains, as mentioned by Pavel.\n> > \n> > Sensors are mostly linear these days. That is, until they reach\n> > saturation of course. I don't think the soft ISP needs a linearization\n> > LUT.\n> \n> That's not what Martijn measured:\n> https://blog.brixit.nl/fixing-the-megapixels-sensor-linearization/\n> Librem 5 is reasonably modern.\n> \n> Basically it is linear in 0..70% range, and then it is linear again in\n> 90%..100% range, but at different slope. Take a look, it is fun article.\n\nI did, and I don't reach the same conclusion. Obviously sensors\nsaturate, so close to saturation there will be non linearities. This is\nnot something that really requires compensation as far as I understand.\nWhat linearization LUTs are meant to compensate for, unless I'm\nmistaken, is non-linearities in the linear part of the dynamic range.\n\nOn a side note, I wonder if lens shading could play a role in shape of\nthe curve in the above article. Central pixels will saturate quicker\nthan pixels closer to the corners. It would be interesting to make the\nsame measurements taking only the central part of the image into\naccount. Another interesting experiment would be to measure the sensor\nlinearity without the lens (but that will be difficult in many cases\nwithout destroying the camera).\n\n> > > > - If we split the processing to pre-bayer and post-bayer parts, we should\n> > > > probably work with uint16_t or float's, which may have impact on performance.\n> > > > \n> > > > - Pavel couldn't get a better performance by using SIMD CPU instructions for\n> > > > debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n> > > > CPU is hard to use and may differ on architectures, so the question is whether\n> > > > it's worth to invest into it.\n> > \n> > Good question :-)\n> \n> Oh, so good news is you write SIMD code once with gcc intristics, and\n> gcc does its magic. You don't have to know assembly for that, but it\n> certainly helps to look at the assembly if it looks reasonable.\n\nThere are also potentially interesting helper libraries such as\nhttps://github.com/vectorclass/version2 (I haven't checked the license\ncompatibility).\n\n> (Debayering needs too much of byte shuffling to work well with SIMD\n> extensions. Matrix multiply could be better, but I'm not sure if 3x3\n> matrix is big enough to get advantage).","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 88131BE08B\n\tfor ;\n\tMon, 22 Apr 2024 19:48:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B93DF63415;\n\tMon, 22 Apr 2024 21:47:59 +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 89DD46340F\n\tfor ;\n\tMon, 22 Apr 2024 21:47:57 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3F4921B44;\n\tMon, 22 Apr 2024 21:47:06 +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=\"hivKW5sJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713815226;\n\tbh=fbgwSRIPPz2zXd2i1VySvgmcRYUs8icrhrO4n305psY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hivKW5sJ7teyYPcBDh6JVRtll4o/xMWrpGradYMu8wketcggbUECzpT3bYpXESCW7\n\tdAwyctnZjNDegXlfhbHwBZi/akQ8bZu/Vk3rwel5OgqJNJYIzju6tnf8nqE8wecFpg\n\tl3oRgFGuF9SP4CaUSHPxYztvClxOVEiPIJpFl2j8=","Date":"Mon, 22 Apr 2024 22:47:49 +0300","From":"Laurent Pinchart ","To":"Pavel Machek ","Cc":"Hans de Goede , Milan Zamazal ,\n\tlibcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","Subject":"Re: Soft ISP TODO list (was: Re: [PATCH v6 17/18] libcamera:\n\tsoftware_isp: Apply black level compensation)","Message-ID":"<20240422194749.GE2438@pendragon.ideasonboard.com>","References":"<20240420104247.GE6414@pendragon.ideasonboard.com>\n\t\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t <87ttjty707.fsf@redhat.com>\n\t<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>\n\t<20240422121246.GA2438@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":29306,"web_url":"https://patchwork.libcamera.org/comment/29306/","msgid":"","date":"2024-04-23T06:59:53","subject":"SIMD black level correction -- Re: Soft ISP TODO list","submitter":{"id":49,"url":"https://patchwork.libcamera.org/api/people/49/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"Hi!\n\n> > > > > - If we split the processing to pre-bayer and post-bayer parts, we should\n> > > > > probably work with uint16_t or float's, which may have impact on performance.\n> > > > > \n> > > > > - Pavel couldn't get a better performance by using SIMD CPU instructions for\n> > > > > debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n> > > > > CPU is hard to use and may differ on architectures, so the question is whether\n> > > > > it's worth to invest into it.\n> > > \n> > > Good question :-)\n> > \n> > Oh, so good news is you write SIMD code once with gcc intristics, and\n> > gcc does its magic. You don't have to know assembly for that, but it\n> > certainly helps to look at the assembly if it looks reasonable.\n> \n> There are also potentially interesting helper libraries such as\n> https://github.com/vectorclass/version2 (I haven't checked the license\n> compatibility).\n\nOk, so I played with black level correction a bit and got pleasant\nsurprise: [Ignore wrong name].\n\nvoid debayer8(uint8_t *dst, const uint8_t *src)\n{\n for (int x = 0; x < (int)WIDTH; x++) {\n uint8_t v = src[x];\n if (v < 16)\n dst[x] = 0;\n else\n dst[x] = v-16;\n\t}\n}\n\ngcc translates it to vector code automatically, and results is only\n10% slower than plain memcpy. Test was done on thinkpad x60. If I\ndisable vector instructions, result is 4x time of plain memcpy. I'm\nquite impressed both by vector unit and by the gcc :-).\n\nBest regards,\n\t\t\t\t\t\t\t\tPavel\n\n00001340 :\n 1340: e8 f0 ff ff ff call 1335 <__x86.get_pc_thunk.dx>\n 1345: 81 c2 af 2c 00 00 add $0x2caf,%edx\n 134b: 56 push %esi\n 134c: 53 push %ebx\n 134d: 8b 4c 24 0c mov 0xc(%esp),%ecx\n 1351: 8b 5c 24 10 mov 0x10(%esp),%ebx\n 1355: 89 c8 mov %ecx,%eax\n 1357: 8d 73 01 lea 0x1(%ebx),%esi\n 135a: 29 f0 sub %esi,%eax\n 135c: 83 f8 0e cmp $0xe,%eax\n 135f: b8 00 00 00 00 mov $0x0,%eax\n 1364: 76 58 jbe 13be \n 1366: 66 0f 6f a2 4c e0 ff movdqa -0x1fb4(%edx),%xmm4\n 136d: ff \n 136e: 66 0f 6f 9a 5c e0 ff movdqa -0x1fa4(%edx),%xmm3\n 1375: ff \n 1376: 66 0f ef d2 pxor %xmm2,%xmm2\n 137a: 8d b6 00 00 00 00 lea 0x0(%esi),%esi\n 1380: f3 0f 6f 04 03 movdqu (%ebx,%eax,1),%xmm0\n 1385: f3 0f 6f 0c 03 movdqu (%ebx,%eax,1),%xmm1\n 138a: 66 0f d8 c4 psubusb %xmm4,%xmm0\n 138e: 66 0f fc cb paddb %xmm3,%xmm1\n 1392: 66 0f 74 c2 pcmpeqb %xmm2,%xmm0\n 1396: 66 0f df c1 pandn %xmm1,%xmm0\n 139a: 0f 11 04 01 movups %xmm0,(%ecx,%eax,1)\n 139e: 83 c0 10 add $0x10,%eax\n 13a1: 3d 80 07 00 00 cmp $0x780,%eax\n 13a6: 75 d8 jne 1380 \n 13a8: 5b pop %ebx\n 13a9: 5e pop %esi\n 13aa: c3 ret\n 13ab: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi\n 13af: 90 nop\n 13b0: c6 04 01 00 movb $0x0,(%ecx,%eax,1)\n 13b4: 83 c0 01 add $0x1,%eax\n 13b7: 3d 80 07 00 00 cmp $0x780,%eax\n 13bc: 74 ea je 13a8 \n 13be: 0f b6 14 03 movzbl (%ebx,%eax,1),%edx\n 13c2: 80 fa 0f cmp $0xf,%dl\n 13c5: 76 e9 jbe 13b0 \n 13c7: 83 ea 10 sub $0x10,%edx\n 13ca: 88 14 01 mov %dl,(%ecx,%eax,1)\n 13cd: 83 c0 01 add $0x1,%eax\n 13d0: 3d 80 07 00 00 cmp $0x780,%eax\n 13d5: 75 e7 jne 13be \n 13d7: eb cf jmp 13a8 \n 13d9: 8d b4 26 00 00 00 00 lea 0x0(%esi,%eiz,1),%esi","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 6C161C3200\n\tfor ;\n\tTue, 23 Apr 2024 06:59:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B33B63417;\n\tTue, 23 Apr 2024 08:59:57 +0200 (CEST)","from jabberwock.ucw.cz (jabberwock.ucw.cz [46.255.230.98])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B28CA63408\n\tfor ;\n\tTue, 23 Apr 2024 08:59:54 +0200 (CEST)","by jabberwock.ucw.cz (Postfix, from userid 1017)\n\tid 207F51C0080; Tue, 23 Apr 2024 08:59:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ucw.cz header.i=@ucw.cz header.b=\"ZhYm+De6\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1;\n\tt=1713855594;\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=OTr9IUWsLpdl9Ja4vzjdXlQAYEvki+F9lVx9NQweuNQ=;\n\tb=ZhYm+De63SAwvz9RktMfYlDRNucfnIFeIPjFIiIIekkp6iJ5wZxOl98r3qUNPMSYJAoWmz\n\trTqfA3A0zPGClMHmJCCm9VFXcV0kod6RU4u7oPOKq6ZF9ak4JVRoo0GIxn6KWeRbWfRAE5\n\tqMMSk023gzPmVDpn1sYrtnNGpxJ4lzM=","Date":"Tue, 23 Apr 2024 08:59:53 +0200","From":"Pavel Machek ","To":"Laurent Pinchart ","Cc":"Hans de Goede , Milan Zamazal ,\n\tlibcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","Subject":"SIMD black level correction -- Re: Soft ISP TODO list","Message-ID":"","References":"\n\t<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t <87ttjty707.fsf@redhat.com>\n\t<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>\n\t<20240422121246.GA2438@pendragon.ideasonboard.com>\n\t\n\t<20240422194749.GE2438@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qqRjQuZ9IejBOjlQ\"","Content-Disposition":"inline","In-Reply-To":"<20240422194749.GE2438@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29315,"web_url":"https://patchwork.libcamera.org/comment/29315/","msgid":"","date":"2024-04-23T11:11:56","subject":"Re: SIMD black level correction -- Re: Soft ISP TODO list","submitter":{"id":49,"url":"https://patchwork.libcamera.org/api/people/49/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"Hi!\n\n> > > > > > - If we split the processing to pre-bayer and post-bayer parts, we should\n> > > > > > probably work with uint16_t or float's, which may have impact on performance.\n> > > > > > \n> > > > > > - Pavel couldn't get a better performance by using SIMD CPU instructions for\n> > > > > > debayering. Applying a CCM matrix may be a different matter. Anyway, SIMD on\n> > > > > > CPU is hard to use and may differ on architectures, so the question is whether\n> > > > > > it's worth to invest into it.\n> > > > \n> > > > Good question :-)\n> > > \n> > > Oh, so good news is you write SIMD code once with gcc intristics, and\n> > > gcc does its magic. You don't have to know assembly for that, but it\n> > > certainly helps to look at the assembly if it looks reasonable.\n> > \n> > There are also potentially interesting helper libraries such as\n> > https://github.com/vectorclass/version2 (I haven't checked the license\n> > compatibility).\n> \n> Ok, so I played with black level correction a bit and got pleasant\n> surprise: [Ignore wrong name].\n> \n> void debayer8(uint8_t *dst, const uint8_t *src)\n> {\n> for (int x = 0; x < (int)WIDTH; x++) {\n> uint8_t v = src[x];\n> if (v < 16)\n> dst[x] = 0;\n> else\n> dst[x] = v-16;\n> \t}\n> }\n> \n> gcc translates it to vector code automatically, and results is only\n> 10% slower than plain memcpy. Test was done on thinkpad x60. If I\n> disable vector instructions, result is 4x time of plain memcpy. I'm\n> quite impressed both by vector unit and by the gcc :-).\n\nOk, disassembly below was for different function than benchmark was\nrunning due to inlining, but you got the idea. Code is at\n\nhttps://gitlab.com/tui/tui/-/blob/master/cam/blacklevel.c?ref_type=heads\n\nif someone wants to play.\n\nI tried to do matrix multiply, and while I do get small improvoement\nfrom \"tree-vectorize\", it is from 1.05 sec to 0.94 sec... additional\nimprovement to cca 0.8 sec is possible with \"fma\". But this is still\nmany times slower than memcpy(), so I'm not sure if we can get good\nperformance there.\n\nmatmult.c code in same directory.\n\nBest regards,\n\t\t\t\t\t\t\t\tPavel","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 8AA24BE08B\n\tfor ;\n\tTue, 23 Apr 2024 11:11:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA9A161B24;\n\tTue, 23 Apr 2024 13:11:58 +0200 (CEST)","from jabberwock.ucw.cz (jabberwock.ucw.cz [46.255.230.98])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F221E61AC1\n\tfor ;\n\tTue, 23 Apr 2024 13:11:57 +0200 (CEST)","by jabberwock.ucw.cz (Postfix, from userid 1017)\n\tid 557A61C0080; Tue, 23 Apr 2024 13:11:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ucw.cz header.i=@ucw.cz header.b=\"X5v66Q/Q\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1;\n\tt=1713870717;\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=IoJ0Nr4CTKFdVxG0F4THYs8qXijEufwW71EFRUwfasg=;\n\tb=X5v66Q/QDD4lzyt6QwZg4fAp+txxjDh0QqUh5oWJSdAo37mesrSJDUJYUdYmL1kQgmZoBe\n\tPiSheHar8UEWDR6psRqHp0hHRBq7VxspWCrmOjiSKZFaAbSuyzML76+MNYBijc5D7uSvLa\n\te58m9h9kkvLx7TyZyu+O8kimtwJpoEY=","Date":"Tue, 23 Apr 2024 13:11:56 +0200","From":"Pavel Machek ","To":"Laurent Pinchart ","Cc":"Hans de Goede , Milan Zamazal ,\n\tlibcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard ,\n\tKieran Bingham ","Subject":"Re: SIMD black level correction -- Re: Soft ISP TODO list","Message-ID":"","References":"<20240421122239.GB19147@pendragon.ideasonboard.com>\n\t\n\t<20240421145923.GC29222@pendragon.ideasonboard.com>\n\t <87ttjty707.fsf@redhat.com>\n\t<381116e0-7f08-4c99-a3e7-2de6bb29ca1f@redhat.com>\n\t<20240422121246.GA2438@pendragon.ideasonboard.com>\n\t\n\t<20240422194749.GE2438@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"OBxrPjzlg8DeRiD9\"","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]