{"id":19774,"url":"https://patchwork.libcamera.org/api/patches/19774/?format=json","web_url":"https://patchwork.libcamera.org/patch/19774/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20240319123622.675599-18-mzamazal@redhat.com>","date":"2024-03-19T12:36:04","name":"[v6,17/18] libcamera: software_isp: Apply black level compensation","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"3610392818d7fb5b175c3e9fbb6d488df225e508","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/?format=json","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/19774/mbox/","series":[{"id":4231,"url":"https://patchwork.libcamera.org/api/series/4231/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=4231","date":"2024-03-19T12:35:47","name":"libcamera: introduce Software ISP and Software IPA","version":6,"mbox":"https://patchwork.libcamera.org/series/4231/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/19774/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/19774/checks/","tags":{},"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 186F9C3272\n\tfor ;\n\tTue, 19 Mar 2024 12:37:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CEA4D62D2E;\n\tTue, 19 Mar 2024 13:37:45 +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 71F8D63079\n\tfor ;\n\tTue, 19 Mar 2024 13:37:43 +0100 (CET)","from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com\n\t[66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-636-mOhSJ7nUNv6vsC7JPxNrZw-1; Tue, 19 Mar 2024 08:37:37 -0400","from smtp.corp.redhat.com\n\t(int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (2048 bits)\n\tserver-digest SHA256) (No client certificate requested)\n\tby mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6246885200B;\n\tTue, 19 Mar 2024 12:37:37 +0000 (UTC)","from nuthatch.redhat.com (unknown [10.45.226.119])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 66F021C060A4;\n\tTue, 19 Mar 2024 12:37:35 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"QU+w6Xu/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710851862;\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=pzkfNgbaoBQCgkqvscoy+omXdExcya0NexVn9GEpZ3o=;\n\tb=QU+w6Xu/P6yYuyk8CuW8mPC6aUIBcslSjUw1hQwxMcAZ6N5YOLP1FnutYuzEplWfuXBJA+\n\tmG50WUT9ssbmmfkhAc4CpcKD0FVpcVm/oYQAEp8/bFZp2GPpLBEXkcTx5FRPShAxki2NoS\n\taWZkvKCToe6cB4mtq+Ze3TbxG+ob2/M=","X-MC-Unique":"mOhSJ7nUNv6vsC7JPxNrZw-1","From":"Milan Zamazal ","To":"libcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"[PATCH v6 17/18] libcamera: software_isp: Apply black level\n\tcompensation","Date":"Tue, 19 Mar 2024 13:36:04 +0100","Message-ID":"<20240319123622.675599-18-mzamazal@redhat.com>","In-Reply-To":"<20240319123622.675599-1-mzamazal@redhat.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>","MIME-Version":"1.0","X-Scanned-By":"MIMEDefang 3.4.1 on 10.11.54.7","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Transfer-Encoding":"8bit","Content-Type":"text/plain; charset=\"US-ASCII\"; x-default=true","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\" "},"content":"Black may not be represented as 0 pixel value for given hardware, it may be\nhigher. If this is not compensated then various problems may occur such as low\ncontrast or suboptimal exposure.\n\nThe black pixel value can be either retrieved from a tuning file for the given\nhardware, or automatically on fly. The former is the right and correct method,\nwhile the latter can be used when a tuning file is not available for the given\nhardware. Since there is currently no support for tuning files in software ISP,\nthe automatic, hardware independent way, is always used. Support for tuning\nfiles should be added in future but it will require more work than this patch.\n\nThe patch looks at the image histogram and assumes that black starts when pixel\nvalues start occurring on the left. A certain amount of the darkest pixels is\nignored; it doesn't matter whether they represent various kinds of noise or are\nreal, they are better to omit in any case to make the image looking better. It\nalso doesn't matter whether the darkest pixels occur around the supposed black\nlevel or are spread between 0 and the black level, the difference is not\nimportant.\n\nAn arbitrary threshold of 2% darkest pixels is applied; there is no magic about\nthat value.\n\nThe patch assumes that the black values for different colors are the same and\ndoesn't attempt any other non-primitive enhancements. It cannot completely\nreplace tuning files and simplicity, while providing visible benefit, is its\ngoal. Anything more sophisticated is left for future patches.\n\nA possible cheap enhancement, if needed, could be setting exposure + gain to\nminimum values temporarily, before setting the black level. In theory, the\nblack level should be fixed but it may not be reached in all images. For this\nreason, the patch updates black level only if the observed value is lower than\nthe current one; it should be never increased.\n\nThe purpose of the patch is to compensate for hardware properties. General\nimage contrast enhancements are out of scope of this patch.\n\nStats are still gathered as an uncorrected histogram, to avoid any confusion and\nto represent the raw image data. Exposure must be determined after the black\nlevel correction -- it has no influence on the sub-black area and must be\ncorrect after applying the black level correction. The granularity of the\nhistogram is increased from 16 to 64 to provide a better precision (there is no\ntheory behind either of those numbers).\n\nReviewed-by: Hans de Goede \nSigned-off-by: Milan Zamazal \nSigned-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","diff":"diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h\nindex 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 */\ndiff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h\nindex 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 */\ndiff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp\nnew file mode 100644\nindex 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\ndiff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h\nnew file mode 100644\nindex 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+{\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\ndiff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\nindex 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,\ndiff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\nindex 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)\ndiff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\nindex 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_)\ndiff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\nindex 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 */\ndiff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\nindex 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()) {\n","prefixes":["v6","17/18"]}