[{"id":30402,"web_url":"https://patchwork.libcamera.org/comment/30402/","msgid":"","date":"2024-07-13T16:40:48","subject":"Re: [PATCH v2 14/19] libcamera: software_isp: Move black level to an\n\talgorithm module","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Milan\n\nThank you for the patch\n\nOn 03/07/24 11:21 pm, Milan Zamazal wrote:\n> The black level determination, already present as a separate class, can\n> be moved to the prepared Algorithm processing structure. It is the\n> first of the current software ISP algorithms that is called in the stats\n> processing sequence, which means it is also the first one that should be\n> moved to the new structure. Stats processing starts with calling\n\nS/  Stats/ Stats/\n\n(extra space)\n> Algorithm-based processing so the call order of the algorithms is\n> retained.\n>\n> Movement of this algorithm is relatively straightforward because all it\n> does is processing stats.\n>\n> Black level is now recomputed on each stats processing. In a future\ns/  In/ In/\n(ditto)\n> patch, after DelayedControls are used, this will be changed to recompute\n> the black level only after exposure/gain changes.\n>\n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/algorithms/blc.cpp | 71 ++++++++++++++++++++\n> src/ipa/simple/algorithms/blc.h | 37 +++++++++++\n> src/ipa/simple/algorithms/meson.build | 1 +\n> src/ipa/simple/black_level.cpp | 93 ---------------------------\n> src/ipa/simple/black_level.h | 33 ----------\n> src/ipa/simple/data/uncalibrated.yaml | 1 +\n> src/ipa/simple/ipa_context.cpp | 8 +++\n> src/ipa/simple/ipa_context.h | 5 ++\n> src/ipa/simple/meson.build | 1 -\n> src/ipa/simple/soft_simple.cpp | 8 +--\n> 10 files changed, 125 insertions(+), 133 deletions(-)\n> create mode 100644 src/ipa/simple/algorithms/blc.cpp\n> create mode 100644 src/ipa/simple/algorithms/blc.h\n> delete mode 100644 src/ipa/simple/black_level.cpp\n> delete mode 100644 src/ipa/simple/black_level.h\n>\n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> new file mode 100644\n> index 00000000..e98c5804\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -0,0 +1,71 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * black level handling\n\ns/black/Black/\n> + */\n> +\n> +#include \"blc.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoftBL)\n> +\n> +BlackLevel::BlackLevel()\n> +{\n> +}\n> +\n> +int BlackLevel::init(IPAContext &context,\n> +\t\t [[maybe_unused]] const YamlObject &tuningData)\n> +{\n> +\tcontext.activeState.black.level = 255;\n> +\treturn 0;\n> +}\n> +\n> +void BlackLevel::process(IPAContext &context,\n> +\t\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t\t const SwIspStats *stats,\n> +\t\t\t [[maybe_unused]] ControlList &metadata)\n> +{\n> +\tconst SwIspStats::Histogram &histogram = stats->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(histogram), end(histogram), 0);\n> +\tconst unsigned int pixelThreshold = ignoredPercentage_ * total;\n> +\tconst unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;\n> +\tconst unsigned int currentBlackIdx =\n> +\t\tcontext.activeState.black.level / histogramRatio;\n> +\n> +\tfor (unsigned int i = 0, seen = 0;\n> +\t i < currentBlackIdx && i < SwIspStats::kYHistogramSize;\n> +\t i++) {\n> +\t\tseen += histogram[i];\n> +\t\tif (seen >= pixelThreshold) {\n> +\t\t\tcontext.activeState.black.level = i * histogramRatio;\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 - histogram[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> +\n> +REGISTER_IPA_ALGORITHM(BlackLevel, \"BlackLevel\")\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h\n> new file mode 100644\n> index 00000000..c8b8d92d\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/blc.h\n> @@ -0,0 +1,37 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * black level handling\n\nDitto\n\nThese can be applied when merging the series, so for this patch LGTM\n\nReviewed-by: Umang Jain \n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> +\n> +#include \"algorithm.h\"\n> +#include \"ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +class BlackLevel : public Algorithm\n> +{\n> +public:\n> +\tBlackLevel();\n> +\t~BlackLevel() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> +\t\toverride;\n> +\tvoid process(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t const SwIspStats *stats,\n> +\t\t ControlList &metadata) override;\n> +};\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n> index 31d26e43..1f63c220 100644\n> --- a/src/ipa/simple/algorithms/meson.build\n> +++ b/src/ipa/simple/algorithms/meson.build\n> @@ -1,4 +1,5 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> soft_simple_ipa_algorithms = files([\n> + 'blc.cpp',\n> ])\n> diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp\n> deleted file mode 100644\n> index 37e0109c..00000000\n> --- a/src/ipa/simple/black_level.cpp\n> +++ /dev/null\n> @@ -1,93 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2024, Red Hat Inc.\n> - *\n> - * 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> -namespace ipa::soft {\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> -uint8_t 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> -\n> -} /* namespace ipa::soft */\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h\n> deleted file mode 100644\n> index a04230c9..00000000\n> --- a/src/ipa/simple/black_level.h\n> +++ /dev/null\n> @@ -1,33 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2024, Red Hat Inc.\n> - *\n> - * black level handling\n> - */\n> -\n> -#pragma once\n> -\n> -#include \n> -#include \n> -\n> -#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> -\n> -namespace libcamera {\n> -\n> -namespace ipa::soft {\n> -\n> -class BlackLevel\n> -{\n> -public:\n> -\tBlackLevel();\n> -\tuint8_t get() const;\n> -\tvoid update(SwIspStats::Histogram &yHistogram);\n> -\n> -private:\n> -\tuint8_t blackLevel_;\n> -\tbool blackLevelSet_;\n> -};\n> -\n> -} /* namespace ipa::soft */\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n> index 2cdc39a8..e0d03d96 100644\n> --- a/src/ipa/simple/data/uncalibrated.yaml\n> +++ b/src/ipa/simple/data/uncalibrated.yaml\n> @@ -3,4 +3,5 @@\n> ---\n> version: 1\n> algorithms:\n> + - BlackLevel:\n> ...\n> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp\n> index 0898e591..bb5daa0d 100644\n> --- a/src/ipa/simple/ipa_context.cpp\n> +++ b/src/ipa/simple/ipa_context.cpp\n> @@ -50,4 +50,12 @@ namespace libcamera::ipa::soft {\n> * \\brief The current state of IPA algorithms\n> */\n> \n> +/**\n> + * \\var IPAActiveState::black\n> + * \\brief Context for the Black Level algorithm\n> + *\n> + * \\var IPAActiveState::black.level\n> + * \\brief Current determined black level\n> + */\n> +\n> } /* namespace libcamera::ipa::soft */\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index bc1235b6..ed07dbb8 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -8,6 +8,8 @@\n> \n> #pragma once\n> \n> +#include \n> +\n> #include \n> \n> namespace libcamera {\n> @@ -18,6 +20,9 @@ struct IPASessionConfiguration {\n> };\n> \n> struct IPAActiveState {\n> +\tstruct {\n> +\t\tuint8_t level;\n> +\t} black;\n> };\n> \n> struct IPAFrameContext : public FrameContext {\n> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> index 7515a8d8..92aa5744 100644\n> --- a/src/ipa/simple/meson.build\n> +++ b/src/ipa/simple/meson.build\n> @@ -8,7 +8,6 @@ ipa_name = 'ipa_soft_simple'\n> soft_simple_sources = files([\n> 'ipa_context.cpp',\n> 'soft_simple.cpp',\n> - 'black_level.cpp',\n> ])\n> \n> soft_simple_sources += soft_simple_ipa_algorithms\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index a8499f29..2fb3a473 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -28,7 +28,6 @@\n> \n> #include \"libipa/camera_sensor_helper.h\"\n> \n> -#include \"black_level.h\"\n> #include \"module.h\"\n> \n> namespace libcamera {\n> @@ -60,7 +59,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module\n> {\n> public:\n> \tIPASoftSimple()\n> -\t\t: params_(nullptr), stats_(nullptr), blackLevel_(BlackLevel()),\n> +\t\t: params_(nullptr), stats_(nullptr),\n> \t\t context_({ {}, {}, { kMaxFrameContexts } }),\n> \t\t ignoreUpdates_(0)\n> \t{\n> @@ -92,7 +91,6 @@ private:\n> \tSwIspStats *stats_;\n> \tstd::unique_ptr camHelper_;\n> \tControlInfoMap sensorInfoMap_;\n> -\tBlackLevel blackLevel_;\n> \n> \tstatic constexpr unsigned int kGammaLookupSize = 1024;\n> \tstd::array gammaTable_;\n> @@ -303,9 +301,7 @@ void IPASoftSimple::processStats(\n> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n> \n> \tSwIspStats::Histogram histogram = stats_->yHistogram;\n> -\tif (ignoreUpdates_ > 0)\n> -\t\tblackLevel_.update(histogram);\n> -\tconst uint8_t blackLevel = blackLevel_.get();\n> +\tconst uint8_t blackLevel = context_.activeState.black.level;\n> \n> \t/*\n> \t * Black level must be subtracted to get the correct AWB ratios, they","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 60C70BD87C\n\tfor ;\n\tSat, 13 Jul 2024 16:40:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 776E66336F;\n\tSat, 13 Jul 2024 18:40:54 +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 9949A63365\n\tfor ;\n\tSat, 13 Jul 2024 18:40:53 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8A5AD7E4;\n\tSat, 13 Jul 2024 18:40:17 +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=\"Qwm0ruy8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720888818;\n\tbh=sigBVSd2SwWFtLitBG7VrMgmoyQuKR9lTxZV341clnE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Qwm0ruy8tiakLff3Le6FPKkEPNeB6vr7yWI3BmGj89gvpH0+x45HCjH1xeacp729m\n\txMWQs22vgtWq39lsEOARxIbBboWH3dZ3rfdielD/HEiEFh59G/2Jz1QI8uICfUCa3h\n\tkNQVexLul98BzT7N+3PHN+ka8BSGMxWpSNXV75kI=","Message-ID":"","Date":"Sat, 13 Jul 2024 22:10:48 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 14/19] libcamera: software_isp: Move black level to an\n\talgorithm module","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","References":"<20240703175119.1872585-1-mzamazal@redhat.com>\n\t<20240703175119.1872585-15-mzamazal@redhat.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240703175119.1872585-15-mzamazal@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":30403,"web_url":"https://patchwork.libcamera.org/comment/30403/","msgid":"<87o771i62i.fsf@redhat.com>","date":"2024-07-13T16:56:37","subject":"Re: [PATCH v2 14/19] libcamera: software_isp: Move black level to\n\tan algorithm module","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Umang,\n\nthank you for review.\n\nUmang Jain writes:\n\n> Hi Milan\n>\n> Thank you for the patch\n>\n> On 03/07/24 11:21 pm, Milan Zamazal wrote:\n>> The black level determination, already present as a separate class, can\n>> be moved to the prepared Algorithm processing structure. It is the\n>> first of the current software ISP algorithms that is called in the stats\n>> processing sequence, which means it is also the first one that should be\n>> moved to the new structure. Stats processing starts with calling\n>\n> S/  Stats/ Stats/\n>\n> (extra space)\n\nThis is deliberate -- it tells Emacs about an end of sentence. I don't\nthink we have a style for commit messages so I suppose I'm free to use\nthis style there :-). OTOH I don't mind if it is changed when merging.\n\n>> Algorithm-based processing so the call order of the algorithms is\n>> retained.\n>>\n>> Movement of this algorithm is relatively straightforward because all it\n>> does is processing stats.\n>>\n>> Black level is now recomputed on each stats processing. In a future\n> s/  In/ In/\n> (ditto)\n>> patch, after DelayedControls are used, this will be changed to recompute\n>> the black level only after exposure/gain changes.\n>>\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/algorithms/blc.cpp | 71 ++++++++++++++++++++\n>> src/ipa/simple/algorithms/blc.h | 37 +++++++++++\n>> src/ipa/simple/algorithms/meson.build | 1 +\n>> src/ipa/simple/black_level.cpp | 93 ---------------------------\n>> src/ipa/simple/black_level.h | 33 ----------\n>> src/ipa/simple/data/uncalibrated.yaml | 1 +\n>> src/ipa/simple/ipa_context.cpp | 8 +++\n>> src/ipa/simple/ipa_context.h | 5 ++\n>> src/ipa/simple/meson.build | 1 -\n>> src/ipa/simple/soft_simple.cpp | 8 +--\n>> 10 files changed, 125 insertions(+), 133 deletions(-)\n>> create mode 100644 src/ipa/simple/algorithms/blc.cpp\n>> create mode 100644 src/ipa/simple/algorithms/blc.h\n>> delete mode 100644 src/ipa/simple/black_level.cpp\n>> delete mode 100644 src/ipa/simple/black_level.h\n>>\n>> diff --git a/src/ipa/simple/algorithms/blc.cpp\n>> b/src/ipa/simple/algorithms/blc.cpp\n>> new file mode 100644\n>> index 00000000..e98c5804\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -0,0 +1,71 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * black level handling\n>\n> s/black/Black/\n>> + */\n>> +\n>> +#include \"blc.h\"\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPASoftBL)\n>> +\n>> +BlackLevel::BlackLevel()\n>> +{\n>> +}\n>> +\n>> +int BlackLevel::init(IPAContext &context,\n>> +\t\t [[maybe_unused]] const YamlObject &tuningData)\n>> +{\n>> +\tcontext.activeState.black.level = 255;\n>> +\treturn 0;\n>> +}\n>> +\n>> +void BlackLevel::process(IPAContext &context,\n>> +\t\t\t [[maybe_unused]] const uint32_t frame,\n>> +\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n>> +\t\t\t const SwIspStats *stats,\n>> +\t\t\t [[maybe_unused]] ControlList &metadata)\n>> +{\n>> +\tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n>> +\n>> +\t/*\n>> + * 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(histogram), end(histogram), 0);\n>> +\tconst unsigned int pixelThreshold = ignoredPercentage_ * total;\n>> +\tconst unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;\n>> +\tconst unsigned int currentBlackIdx =\n>> +\t\tcontext.activeState.black.level / histogramRatio;\n>> +\n>> +\tfor (unsigned int i = 0, seen = 0;\n>> +\t i < currentBlackIdx && i < SwIspStats::kYHistogramSize;\n>> +\t i++) {\n>> +\t\tseen += histogram[i];\n>> +\t\tif (seen >= pixelThreshold) {\n>> +\t\t\tcontext.activeState.black.level = i * histogramRatio;\n>> +\t\t\tLOG(IPASoftBL, Debug)\n>> +\t\t\t\t<< \"Auto-set black level: \"\n>> +\t\t\t\t<< i << \"/\" << SwIspStats::kYHistogramSize\n>> + << \" (\" << 100 * (seen - histogram[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>> +\n>> +REGISTER_IPA_ALGORITHM(BlackLevel, \"BlackLevel\")\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h\n>> new file mode 100644\n>> index 00000000..c8b8d92d\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/blc.h\n>> @@ -0,0 +1,37 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * black level handling\n>\n> Ditto\n>\n> These can be applied when merging the series, so for this patch LGTM\n>\n> Reviewed-by: Umang Jain \n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n>> +\n>> +#include \"algorithm.h\"\n>> +#include \"ipa_context.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +class BlackLevel : public Algorithm\n>> +{\n>> +public:\n>> +\tBlackLevel();\n>> +\t~BlackLevel() = default;\n>> +\n>> +\tint init(IPAContext &context, const YamlObject &tuningData)\n>> +\t\toverride;\n>> +\tvoid process(IPAContext &context, const uint32_t frame,\n>> +\t\t IPAFrameContext &frameContext,\n>> +\t\t const SwIspStats *stats,\n>> +\t\t ControlList &metadata) override;\n>> +};\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n>> index 31d26e43..1f63c220 100644\n>> --- a/src/ipa/simple/algorithms/meson.build\n>> +++ b/src/ipa/simple/algorithms/meson.build\n>> @@ -1,4 +1,5 @@\n>> # SPDX-License-Identifier: CC0-1.0\n>> soft_simple_ipa_algorithms = files([\n>> + 'blc.cpp',\n>> ])\n>> diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp\n>> deleted file mode 100644\n>> index 37e0109c..00000000\n>> --- a/src/ipa/simple/black_level.cpp\n>> +++ /dev/null\n>> @@ -1,93 +0,0 @@\n>> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> -/*\n>> - * Copyright (C) 2024, Red Hat Inc.\n>> - *\n>> - * 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>> -namespace ipa::soft {\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>> -uint8_t 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>> -\n>> -} /* namespace ipa::soft */\n>> -\n>> -} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h\n>> deleted file mode 100644\n>> index a04230c9..00000000\n>> --- a/src/ipa/simple/black_level.h\n>> +++ /dev/null\n>> @@ -1,33 +0,0 @@\n>> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> -/*\n>> - * Copyright (C) 2024, Red Hat Inc.\n>> - *\n>> - * black level handling\n>> - */\n>> -\n>> -#pragma once\n>> -\n>> -#include \n>> -#include \n>> -\n>> -#include \"libcamera/internal/software_isp/swisp_stats.h\"\n>> -\n>> -namespace libcamera {\n>> -\n>> -namespace ipa::soft {\n>> -\n>> -class BlackLevel\n>> -{\n>> -public:\n>> -\tBlackLevel();\n>> -\tuint8_t get() const;\n>> -\tvoid update(SwIspStats::Histogram &yHistogram);\n>> -\n>> -private:\n>> -\tuint8_t blackLevel_;\n>> -\tbool blackLevelSet_;\n>> -};\n>> -\n>> -} /* namespace ipa::soft */\n>> -\n>> -} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n>> index 2cdc39a8..e0d03d96 100644\n>> --- a/src/ipa/simple/data/uncalibrated.yaml\n>> +++ b/src/ipa/simple/data/uncalibrated.yaml\n>> @@ -3,4 +3,5 @@\n>> ---\n>> version: 1\n>> algorithms:\n>> + - BlackLevel:\n>> ...\n>> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp\n>> index 0898e591..bb5daa0d 100644\n>> --- a/src/ipa/simple/ipa_context.cpp\n>> +++ b/src/ipa/simple/ipa_context.cpp\n>> @@ -50,4 +50,12 @@ namespace libcamera::ipa::soft {\n>> * \\brief The current state of IPA algorithms\n>> */\n>> +/**\n>> + * \\var IPAActiveState::black\n>> + * \\brief Context for the Black Level algorithm\n>> + *\n>> + * \\var IPAActiveState::black.level\n>> + * \\brief Current determined black level\n>> + */\n>> +\n>> } /* namespace libcamera::ipa::soft */\n>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index bc1235b6..ed07dbb8 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -8,6 +8,8 @@\n>> #pragma once\n>> +#include \n>> +\n>> #include \n>> namespace libcamera {\n>> @@ -18,6 +20,9 @@ struct IPASessionConfiguration {\n>> };\n>> struct IPAActiveState {\n>> +\tstruct {\n>> +\t\tuint8_t level;\n>> +\t} black;\n>> };\n>> struct IPAFrameContext : public FrameContext {\n>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n>> index 7515a8d8..92aa5744 100644\n>> --- a/src/ipa/simple/meson.build\n>> +++ b/src/ipa/simple/meson.build\n>> @@ -8,7 +8,6 @@ ipa_name = 'ipa_soft_simple'\n>> soft_simple_sources = files([\n>> 'ipa_context.cpp',\n>> 'soft_simple.cpp',\n>> - 'black_level.cpp',\n>> ])\n>> soft_simple_sources += soft_simple_ipa_algorithms\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index a8499f29..2fb3a473 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -28,7 +28,6 @@\n>> #include \"libipa/camera_sensor_helper.h\"\n>> -#include \"black_level.h\"\n>> #include \"module.h\"\n>> namespace libcamera {\n>> @@ -60,7 +59,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module\n>> {\n>> public:\n>> \tIPASoftSimple()\n>> -\t\t: params_(nullptr), stats_(nullptr), blackLevel_(BlackLevel()),\n>> +\t\t: params_(nullptr), stats_(nullptr),\n>> \t\t context_({ {}, {}, { kMaxFrameContexts } }),\n>> \t\t ignoreUpdates_(0)\n>> \t{\n>> @@ -92,7 +91,6 @@ private:\n>> \tSwIspStats *stats_;\n>> \tstd::unique_ptr camHelper_;\n>> \tControlInfoMap sensorInfoMap_;\n>> -\tBlackLevel blackLevel_;\n>> \tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>> \tstd::array gammaTable_;\n>> @@ -303,9 +301,7 @@ void IPASoftSimple::processStats(\n>> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n>> \tSwIspStats::Histogram histogram = stats_->yHistogram;\n>> -\tif (ignoreUpdates_ > 0)\n>> -\t\tblackLevel_.update(histogram);\n>> -\tconst uint8_t blackLevel = blackLevel_.get();\n>> +\tconst uint8_t blackLevel = context_.activeState.black.level;\n>> \t/*\n>> \t * Black level must be subtracted to get the correct AWB ratios, they","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 CC5F6BDB1C\n\tfor ;\n\tSat, 13 Jul 2024 16:56:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D37896336F;\n\tSat, 13 Jul 2024 18:56:45 +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 1A69E63365\n\tfor ;\n\tSat, 13 Jul 2024 18:56:43 +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-96-r2CsegX9OgydqphdhdELyg-1; Sat, 13 Jul 2024 12:56:41 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a77c5dfbd16so230058666b.0\n\tfor ;\n\tSat, 13 Jul 2024 09:56: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\ta640c23a62f3a-a79bc820ebesm64744666b.197.2024.07.13.09.56.38\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 13 Jul 2024 09:56:38 -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=\"BlMCyNrL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1720889803;\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=WdKPkbytVXk/1/iNrqgX6Mar//9ixm/DsH7vmJNlI78=;\n\tb=BlMCyNrLISq1Q2QiKDUkjdNsMzrZixu4BRKnqsJl2CkgEm6TkH2fuRhpOscodg0plRjEY1\n\tSZ/BVLKrJh/34xNqyACbHAKy3Rg9Xt9+4m2upEcN+g/X8q69Td3rjlO5Tl/gqZza9O0fjB\n\tVhhP9vGqN7Mnnu1DlUQstrHUmWTqbxo=","X-MC-Unique":"r2CsegX9OgydqphdhdELyg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1720889800; x=1721494600;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=afuiv7gB5YZe6GOoJ37FVsoAfNc3uqhdxWIPeqn9Qc8=;\n\tb=hRxKuxJVCkJEjLgvIRkmRMylFibafp34cLBnmyGTXJ3I4H42GliSS9bwSx+LIWxAxi\n\tS6t8KeAY9MAbbKaPPajMspzYrvThv0bmdwYDrGNcDLfhymyJF4Tn5I9aiWE0H0OHMG6J\n\tllq5/ifO+vGOXNC+Plq/0qfbpFWKxkQmkDR1vvoKoG4dt8tKza57ZrK5QlYqvjWsFVS5\n\tH03dqVhAF07RxOrnn/K+RWbRVoStLSH0vpQmXJgzvyjrNIU/m0mRMTkcXPhYTJEz5+pn\n\td+zit+EXsgKkR2BC6IsGaW5YFtI3eGaRKQQ7wOnzzd7KM0mgQgSKNaElCL4BRI8MhsUs\n\tNu7A==","X-Gm-Message-State":"AOJu0YyIx8Qf6Y+2JEvfTRsImJn4Nr4+1xC5exsqeJQwDSH7duuxqjJc\n\t9XLN4mIov9EMGTTafF8yQI9DTBuB95n/y/WcUXzCgfpFy42feJ9Hq/j1BOtCbAUxiszLPHP/n4+\n\tLuXceJS5JAkB9ey2aXD3xz/wxp65CX+K5dpl2JCH935qXaWPVwYczCxszxClsWB9DqXDi3hOTJN\n\tQNmUvi1SDaY1OLNpSu2Yu1Snj+bgx14cS19NjCThQcJNDNjc9kPlrF5mQ=","X-Received":["by 2002:a17:906:b2d7:b0:a72:4676:4f8 with SMTP id\n\ta640c23a62f3a-a780b882742mr1068855566b.62.1720889799694; \n\tSat, 13 Jul 2024 09:56:39 -0700 (PDT)","by 2002:a17:906:b2d7:b0:a72:4676:4f8 with SMTP id\n\ta640c23a62f3a-a780b882742mr1068852966b.62.1720889798895; \n\tSat, 13 Jul 2024 09:56:38 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IE47Z5avB/z7bcXYHYP1RRfF4EAkcT0ewT0eSMwi+ynxzdQGXG/hd3PClos3xRXCbn/5NsGTQ==","From":"Milan Zamazal ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 14/19] libcamera: software_isp: Move black level to\n\tan algorithm module","In-Reply-To":" (Umang\n\tJain's message of \"Sat, 13 Jul 2024 22:10:48 +0530\")","References":"<20240703175119.1872585-1-mzamazal@redhat.com>\n\t<20240703175119.1872585-15-mzamazal@redhat.com>\n\t","Date":"Sat, 13 Jul 2024 18:56:37 +0200","Message-ID":"<87o771i62i.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; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}}]