[{"id":31033,"web_url":"https://patchwork.libcamera.org/comment/31033/","msgid":"<20240901210338.GB31148@pendragon.ideasonboard.com>","date":"2024-09-01T21:03:38","subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to an\n\talgorithm module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Fri, Aug 30, 2024 at 09:25:49AM +0200, 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> 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> The comment about getting black level from the tuning files is dropped.\n> The black level will be taken from CameraSensorHelper if available and\n> that will be implemented in one of the followup patches.\n> \n> Black level is now recomputed on each stats processing. In a future\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> Reviewed-by: Umang Jain \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..3b73d830\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> +\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\nIs init() the right place, shouldn't this be done in configure() ?\nSetting the initial value in init() means that a new capture session\nwill not start with a reinitialized black level value.\n\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\nPlease reflow to 80 columns.\n\n> +\t */\n> +\tconstexpr float ignoredPercentage_ = 0.02;\n\nIt's not a member variable, so no trailing underscore.\n\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..2f73db52\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> +\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\nI think you can drop controls.h, swisp_stats.h and ipa_context.h.\nThey're included only for classes and structures that are part of the\nAlgorithm class API, so they're guaranteed to be included through\nalgorithm.h.\n\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 3c1c7262..268cff32 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 e7d8b8df..22156569 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -7,6 +7,8 @@\n> \n> #pragma once\n> \n> +#include \n> +\n> #include \n> \n> namespace libcamera {\n> @@ -17,6 +19,9 @@ struct IPASessionConfiguration {\n> };\n> \n> struct IPAActiveState {\n> +\tstruct {\n> +\t\tuint8_t level;\n> +\t} black;\n\nHow about naming this blc ?\n\nReviewed-by: Laurent Pinchart \n\n> };\n> \n> struct IPAFrameContext : public FrameContext {\n> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> index dcd7c70a..2f9f15f4 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 4b9201dc..a1bd2f0c 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -29,7 +29,6 @@\n> \n> #include \"libipa/camera_sensor_helper.h\"\n> \n> -#include \"black_level.h\"\n> #include \"module.h\"\n> \n> namespace libcamera {\n> @@ -61,7 +60,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> @@ -93,7 +92,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(const uint32_t frame,\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 DAD14C324C\n\tfor ;\n\tSun, 1 Sep 2024 21:04:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC9EF63469;\n\tSun, 1 Sep 2024 23:04:10 +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 3305563466\n\tfor ;\n\tSun, 1 Sep 2024 23:04:09 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2E7E44CE;\n\tSun, 1 Sep 2024 23:02:58 +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=\"uetH6lUS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725224578;\n\tbh=PT5H+t0UXx8raG+Xbh/yq3qTzGW5xfFt6h+X2q55pwc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uetH6lUS7YRGLHcFXsZColh0Oz9xpp9f3Q4fprNd+iTOPoeIwE28tfs1re8/KCc/Z\n\trYEZAu3irU/SeUr9xEKPaJGBrI00EGKPFQt9tFwmUsIMelD7KkmInmRHDejqXvQfpg\n\tQ2U8OkTFH+bGMMD4nnLi5SOZxQczInuhBBZe2NzU=","Date":"Mon, 2 Sep 2024 00:03:38 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tUmang Jain ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to an\n\talgorithm module","Message-ID":"<20240901210338.GB31148@pendragon.ideasonboard.com>","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-14-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240830072554.180672-14-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":31100,"web_url":"https://patchwork.libcamera.org/comment/31100/","msgid":"<87msklyv82.fsf@redhat.com>","date":"2024-09-06T09:42:05","subject":"Re: [PATCH v5 13/18] 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 Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 30, 2024 at 09:25:49AM +0200, 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>> 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>> The comment about getting black level from the tuning files is dropped.\n>> The black level will be taken from CameraSensorHelper if available and\n>> that will be implemented in one of the followup patches.\n>> \n>> Black level is now recomputed on each stats processing. In a future\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>> Reviewed-by: Umang Jain \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..3b73d830\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>> +\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>\n> Is init() the right place, shouldn't this be done in configure() ?\n> Setting the initial value in init() means that a new capture session\n> will not start with a reinitialized black level value.\n\nThat should be fine as long as the sensor is the same, which I suppose\nis the case.\n\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>\n> Please reflow to 80 columns.\n>\n>> +\t */\n>> +\tconstexpr float ignoredPercentage_ = 0.02;\n>\n> It's not a member variable, so no trailing underscore.\n>\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..2f73db52\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>> +\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> I think you can drop controls.h, swisp_stats.h and ipa_context.h.\n> They're included only for classes and structures that are part of the\n> Algorithm class API, so they're guaranteed to be included through\n> algorithm.h.\n>\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 3c1c7262..268cff32 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 e7d8b8df..22156569 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -7,6 +7,8 @@\n>> \n>> #pragma once\n>> \n>> +#include \n>> +\n>> #include \n>> \n>> namespace libcamera {\n>> @@ -17,6 +19,9 @@ struct IPASessionConfiguration {\n>> };\n>> \n>> struct IPAActiveState {\n>> +\tstruct {\n>> +\t\tuint8_t level;\n>> +\t} black;\n>\n> How about naming this blc ?\n>\n> Reviewed-by: Laurent Pinchart \n>\n>> };\n>> \n>> struct IPAFrameContext : public FrameContext {\n>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n>> index dcd7c70a..2f9f15f4 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 4b9201dc..a1bd2f0c 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -29,7 +29,6 @@\n>> \n>> #include \"libipa/camera_sensor_helper.h\"\n>> \n>> -#include \"black_level.h\"\n>> #include \"module.h\"\n>> \n>> namespace libcamera {\n>> @@ -61,7 +60,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>> @@ -93,7 +92,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(const uint32_t frame,\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 3BF8EBF415\n\tfor ;\n\tFri, 6 Sep 2024 09:42:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A447D634EA;\n\tFri, 6 Sep 2024 11:42:15 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF67F618FA\n\tfor ;\n\tFri, 6 Sep 2024 11:42:12 +0200 (CEST)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-225-2jmZIbpxPgyu8POpUvaxCg-1; Fri, 06 Sep 2024 05:42:08 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-42bac946976so14515785e9.2\n\tfor ;\n\tFri, 06 Sep 2024 02:42:08 -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\t5b1f17b1804b1-42ca05d7f6csm14652395e9.31.2024.09.06.02.42.05\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Sep 2024 02:42:06 -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=\"WTmsgMk1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1725615731;\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=sJ6ATkq5AS+cXkoy++D8z2Hw85feXqZ49nSC1pS9pS4=;\n\tb=WTmsgMk1uIAUQW39aDa2VtAGCUYHGQP0Ecn8jB1X73U2eFFhacCFcuD+Vi67AGr6bClBoq\n\tTEcR/Z/qhkOgqjtX230/Ypon2u3hUbFFCcKhrP4LQ5gEza3kcsFqTT4Ox7CWhGpmzcCgRq\n\tweL96nQguMk71s61FQu6w5MZjZBK4Eo=","X-MC-Unique":"2jmZIbpxPgyu8POpUvaxCg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725615728; x=1726220528;\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=sJ6ATkq5AS+cXkoy++D8z2Hw85feXqZ49nSC1pS9pS4=;\n\tb=koH/uzo+r9MIPTDy/wepXtFMnjIBdhtvsiNFgpchOWPfIFgLmhEL7Uq6pGH013V7DL\n\tetkL2JpGGr7m10zPtX+Up2uyUJ4KwfQKk2uuVdofwTPHoUHjVPCfVAMeYljw+aJMiJcT\n\tCCL3AIBCsG3sUGrCZnkRr4hqoZ4jvvsMF57Bn6f/G0Hh2GIcytrG8ZrqwjntSL1WbS9t\n\tpMLBeVgj4LTkTgmKfnyg5zl7wMduYfk7MGqds/4tJ8+/XderBOv6lShkcVYzVX30Xmup\n\t5gyJ2R6AVm2ZaTirs9QFl1UeilWOZuk9DhS719HoXiMF6zxRi1+vNJWclAvbbDNIogpP\n\tRQVw==","X-Gm-Message-State":"AOJu0Yw4KMx/vEQJf2fy94DfmUAbS+5lj9YO9l3WReKK46EUJXaWNdQp\n\tJ6M+ymQk5o56G1MxtbYTp6BLKM+KXdFTNR9Nizjew0ws8QBoETXxuWnZ+4Burx5xiLtuwrrDn6u\n\tFLHca+uabw5yiPct7KB/tBUNkg49NEw3n28F9GxPA5G45dpLOYMy46Fc/xzOfP3lAcguP9pI=","X-Received":["by 2002:a05:600c:6c4d:b0:42c:a72a:e8f4 with SMTP id\n\t5b1f17b1804b1-42ca72aeda7mr3921775e9.14.1725615727524; \n\tFri, 06 Sep 2024 02:42:07 -0700 (PDT)","by 2002:a05:600c:6c4d:b0:42c:a72a:e8f4 with SMTP id\n\t5b1f17b1804b1-42ca72aeda7mr3921465e9.14.1725615726913; \n\tFri, 06 Sep 2024 02:42:06 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFPzpOkuDXeCc8k7daHpUspRHYxhW00jVBR71m0tXHbtxNDBknnnRIvmfD0lxzpT2sSiin5Hg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Umang Jain\n\t,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to\n\tan algorithm module","In-Reply-To":"<20240901210338.GB31148@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 2 Sep 2024 00:03:38 +0300\")","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-14-mzamazal@redhat.com>\n\t<20240901210338.GB31148@pendragon.ideasonboard.com>","Date":"Fri, 06 Sep 2024 11:42:05 +0200","Message-ID":"<87msklyv82.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":31103,"web_url":"https://patchwork.libcamera.org/comment/31103/","msgid":"<20240906125743.GA27086@pendragon.ideasonboard.com>","date":"2024-09-06T12:57:43","subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to an\n\talgorithm module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 06, 2024 at 11:42:05AM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> >\n> > On Fri, Aug 30, 2024 at 09:25:49AM +0200, 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> >> 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> >> The comment about getting black level from the tuning files is dropped.\n> >> The black level will be taken from CameraSensorHelper if available and\n> >> that will be implemented in one of the followup patches.\n> >> \n> >> Black level is now recomputed on each stats processing. In a future\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> >> Reviewed-by: Umang Jain \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..3b73d830\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> >> +\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> >\n> > Is init() the right place, shouldn't this be done in configure() ?\n> > Setting the initial value in init() means that a new capture session\n> > will not start with a reinitialized black level value.\n> \n> That should be fine as long as the sensor is the same, which I suppose\n> is the case.\n\nIs it fine ? Should an application expect that the camera will start\nwith identical parameters for the first capture session and for all\nsubsequent ones ?\n\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> >\n> > Please reflow to 80 columns.\n> >\n> >> +\t */\n> >> +\tconstexpr float ignoredPercentage_ = 0.02;\n> >\n> > It's not a member variable, so no trailing underscore.\n> >\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..2f73db52\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> >> +\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> > I think you can drop controls.h, swisp_stats.h and ipa_context.h.\n> > They're included only for classes and structures that are part of the\n> > Algorithm class API, so they're guaranteed to be included through\n> > algorithm.h.\n> >\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 3c1c7262..268cff32 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 e7d8b8df..22156569 100644\n> >> --- a/src/ipa/simple/ipa_context.h\n> >> +++ b/src/ipa/simple/ipa_context.h\n> >> @@ -7,6 +7,8 @@\n> >> \n> >> #pragma once\n> >> \n> >> +#include \n> >> +\n> >> #include \n> >> \n> >> namespace libcamera {\n> >> @@ -17,6 +19,9 @@ struct IPASessionConfiguration {\n> >> };\n> >> \n> >> struct IPAActiveState {\n> >> +\tstruct {\n> >> +\t\tuint8_t level;\n> >> +\t} black;\n> >\n> > How about naming this blc ?\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> >> };\n> >> \n> >> struct IPAFrameContext : public FrameContext {\n> >> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> >> index dcd7c70a..2f9f15f4 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 4b9201dc..a1bd2f0c 100644\n> >> --- a/src/ipa/simple/soft_simple.cpp\n> >> +++ b/src/ipa/simple/soft_simple.cpp\n> >> @@ -29,7 +29,6 @@\n> >> \n> >> #include \"libipa/camera_sensor_helper.h\"\n> >> \n> >> -#include \"black_level.h\"\n> >> #include \"module.h\"\n> >> \n> >> namespace libcamera {\n> >> @@ -61,7 +60,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> >> @@ -93,7 +92,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(const uint32_t frame,\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 96537C324C\n\tfor ;\n\tFri, 6 Sep 2024 12:57:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C949634E5;\n\tFri, 6 Sep 2024 14:57: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 D5921633CC\n\tfor ;\n\tFri, 6 Sep 2024 14:57:45 +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 5093F3C7;\n\tFri, 6 Sep 2024 14:56:31 +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=\"VkiX5vMl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725627391;\n\tbh=oFaiFz4z15zy9afQ3mV6+CV7bZXVHgzq792YsMieMmw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VkiX5vMlUJJciiQaGl84x3d6tJXeVpG2ayIeoaQ3QYYFwaSn/jSBiw8Ds5pxet6uu\n\tcvDFXvKyYf+YGXkttlvn5YX3x0iZh4cW8Lft6AqqqTMSKDEgSI0R1XJvXT6tQRD+YV\n\tpNbIbr/FEsgip3QC79fBryRxja/M3huiCeTctK2Y=","Date":"Fri, 6 Sep 2024 15:57:43 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tUmang Jain ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to an\n\talgorithm module","Message-ID":"<20240906125743.GA27086@pendragon.ideasonboard.com>","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-14-mzamazal@redhat.com>\n\t<20240901210338.GB31148@pendragon.ideasonboard.com>\n\t<87msklyv82.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87msklyv82.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":31105,"web_url":"https://patchwork.libcamera.org/comment/31105/","msgid":"<871q1wzxsm.fsf@redhat.com>","date":"2024-09-06T14:01:13","subject":"Re: [PATCH v5 13/18] 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":"Laurent Pinchart writes:\n\n> On Fri, Sep 06, 2024 at 11:42:05AM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>> >\n>\n>> > On Fri, Aug 30, 2024 at 09:25:49AM +0200, 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>> >> 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>> >> The comment about getting black level from the tuning files is dropped.\n>> >> The black level will be taken from CameraSensorHelper if available and\n>> >> that will be implemented in one of the followup patches.\n>> >> \n>> >> Black level is now recomputed on each stats processing. In a future\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>> >> Reviewed-by: Umang Jain \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..3b73d830\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>> >> +\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>> >\n>> > Is init() the right place, shouldn't this be done in configure() ?\n>> > Setting the initial value in init() means that a new capture session\n>> > will not start with a reinitialized black level value.\n>> \n>> That should be fine as long as the sensor is the same, which I suppose\n>> is the case.\n>\n> Is it fine ? Should an application expect that the camera will start\n> with identical parameters for the first capture session and for all\n> subsequent ones ?\n\nI'm not sure. Since init() is what takes tuningData as a parameter, I'd\nthink a new instance should be created any time something substantial\nchanges (e.g. a different sensor is used). OTOH in the patch (posted\nseparately) taking black level from CameraSensorHelper if available, I\nmove the initialization from init() to configure() because this is where\nCameraSensorHelper is available in software ISP.\n\nIn any case, moving it to configure() right now doesn't harm and I can\ndo it in v7 (together with moving the gamma initialization).\n\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>> >\n>> > Please reflow to 80 columns.\n>> >\n>> >> +\t */\n>> >> +\tconstexpr float ignoredPercentage_ = 0.02;\n>> >\n>> > It's not a member variable, so no trailing underscore.\n>> >\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..2f73db52\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>> >> +\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>> > I think you can drop controls.h, swisp_stats.h and ipa_context.h.\n>> > They're included only for classes and structures that are part of the\n>> > Algorithm class API, so they're guaranteed to be included through\n>> > algorithm.h.\n>> >\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 3c1c7262..268cff32 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 e7d8b8df..22156569 100644\n>> >> --- a/src/ipa/simple/ipa_context.h\n>> >> +++ b/src/ipa/simple/ipa_context.h\n>> >> @@ -7,6 +7,8 @@\n>> >> \n>> >> #pragma once\n>> >> \n>> >> +#include \n>> >> +\n>> >> #include \n>> >> \n>> >> namespace libcamera {\n>> >> @@ -17,6 +19,9 @@ struct IPASessionConfiguration {\n>> >> };\n>> >> \n>> >> struct IPAActiveState {\n>> >> +\tstruct {\n>> >> +\t\tuint8_t level;\n>> >> +\t} black;\n>> >\n>> > How about naming this blc ?\n>> >\n>> > Reviewed-by: Laurent Pinchart \n>> >\n>> >> };\n>> >> \n>> >> struct IPAFrameContext : public FrameContext {\n>> >> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n>> >> index dcd7c70a..2f9f15f4 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 4b9201dc..a1bd2f0c 100644\n>> >> --- a/src/ipa/simple/soft_simple.cpp\n>> >> +++ b/src/ipa/simple/soft_simple.cpp\n>> >> @@ -29,7 +29,6 @@\n>> >> \n>> >> #include \"libipa/camera_sensor_helper.h\"\n>> >> \n>> >> -#include \"black_level.h\"\n>> >> #include \"module.h\"\n>> >> \n>> >> namespace libcamera {\n>> >> @@ -61,7 +60,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>> >> @@ -93,7 +92,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(const uint32_t frame,\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 0108AC324C\n\tfor ;\n\tFri, 6 Sep 2024 14:01:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C2DA634E5;\n\tFri, 6 Sep 2024 16:01:24 +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 F0737633CC\n\tfor ;\n\tFri, 6 Sep 2024 16:01:21 +0200 (CEST)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-148-Hx3cOOzAMbyEwBNu-rW1uw-1; Fri, 06 Sep 2024 10:01:18 -0400","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-a8a6fee3ab1so186008866b.3\n\tfor ;\n\tFri, 06 Sep 2024 07:01:18 -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-a8a6236d23esm284373466b.142.2024.09.06.07.01.13\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Sep 2024 07:01:14 -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=\"UEoHgyNv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1725631280;\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=ZJp7hItYukV823WZ8cG9ipw74zzXcKxmca2d/3lqzIg=;\n\tb=UEoHgyNvTXxCAd2dWeq9xGnCi5zp+J0YXNmEVrlIS/D654rKEZR2sRVJY8krlK6qf/B9ZD\n\tWEt8Lp9QMkPezeGZC/djPFTgZJCX7YN/FmTM6CZ4cJSL5koSvWv4Sw2KxYIy4jv1yjNaux\n\t/YOUKmmtofY8ohJlRp3QyQJpOKJCWSc=","X-MC-Unique":"Hx3cOOzAMbyEwBNu-rW1uw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725631277; x=1726236077;\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=ZJp7hItYukV823WZ8cG9ipw74zzXcKxmca2d/3lqzIg=;\n\tb=LsUoQFdZoxYhX05dzIG2BJcgVC8pk1udu+CQJf9Pa13Pbf0djWhVur/Q4dCdFbnwMy\n\t4Sak79FlrLUpXd2Q2+Mv/7r9jNN9n026+5mARnQCnOysCJ5UsHteyZ7eh+qilAa7ejK2\n\ts+BfgR0f/wWTTlmiCzxY5SMuWFd64cmCiU92QOddmItRBLoS5sgPzufq4qpdrF2L6dpd\n\tYf0fWMlax9nUUYs+oMlvyBndHR57aaKU9H3/s2n+35+HrL1MJD8zwgRTzrJlQTx4hr8a\n\tld7ff+NG+RKSLt8lfM13KB3nxmSYFYb6FTgXBfE0cO1NAVyh1xdmgNTV9caeVAumtx7q\n\tnS5Q==","X-Gm-Message-State":"AOJu0YwnBxsAGGVtYC94JmrvgkXqNe1hmeLZnqpfaThYk71EJLF2m4Gz\n\tt45DkFDBt9VPdN9d4fRW6S5Bvh4Ifb4FIz7AZIhU84wNAmhuycHF1mRhvQY3ijm5mInTLKS1JI4\n\tLIr4/tt9YKigQaeYOpU+7ayoPh5LmsLi50wWAS/iZrcXV0CBiHeg+dSJSr86eQHN08OvCPnI=","X-Received":["by 2002:a17:907:7243:b0:a86:842a:104a with SMTP id\n\ta640c23a62f3a-a8a888dae61mr176815866b.57.1725631277263; \n\tFri, 06 Sep 2024 07:01:17 -0700 (PDT)","by 2002:a17:907:7243:b0:a86:842a:104a with SMTP id\n\ta640c23a62f3a-a8a888dae61mr176802866b.57.1725631275069; \n\tFri, 06 Sep 2024 07:01:15 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH/MyXReGj/1cRsEIV1azbk4EuAWcatZtZ74xgzAdpuywgPrJ6AMHy2NgErvDysctjP14LUjQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Umang Jain\n\t,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to\n\tan algorithm module","In-Reply-To":"<20240906125743.GA27086@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Fri, 6 Sep 2024 15:57:43 +0300\")","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-14-mzamazal@redhat.com>\n\t<20240901210338.GB31148@pendragon.ideasonboard.com>\n\t<87msklyv82.fsf@redhat.com>\n\t<20240906125743.GA27086@pendragon.ideasonboard.com>","Date":"Fri, 06 Sep 2024 16:01:13 +0200","Message-ID":"<871q1wzxsm.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":31106,"web_url":"https://patchwork.libcamera.org/comment/31106/","msgid":"<20240906211807.GG27086@pendragon.ideasonboard.com>","date":"2024-09-06T21:18:07","subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to an\n\talgorithm module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 06, 2024 at 04:01:13PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Fri, Sep 06, 2024 at 11:42:05AM +0200, Milan Zamazal wrote:\n> >> Laurent Pinchart writes:\n> >> > On Fri, Aug 30, 2024 at 09:25:49AM +0200, 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> >> >> 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> >> >> The comment about getting black level from the tuning files is dropped.\n> >> >> The black level will be taken from CameraSensorHelper if available and\n> >> >> that will be implemented in one of the followup patches.\n> >> >> \n> >> >> Black level is now recomputed on each stats processing. In a future\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> >> >> Reviewed-by: Umang Jain \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..3b73d830\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> >> >> +\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> >> >\n> >> > Is init() the right place, shouldn't this be done in configure() ?\n> >> > Setting the initial value in init() means that a new capture session\n> >> > will not start with a reinitialized black level value.\n> >> \n> >> That should be fine as long as the sensor is the same, which I suppose\n> >> is the case.\n> >\n> > Is it fine ? Should an application expect that the camera will start\n> > with identical parameters for the first capture session and for all\n> > subsequent ones ?\n> \n> I'm not sure. Since init() is what takes tuningData as a parameter, I'd\n> think a new instance should be created any time something substantial\n> changes (e.g. a different sensor is used). OTOH in the patch (posted\n> separately) taking black level from CameraSensorHelper if available, I\n> move the initialization from init() to configure() because this is where\n> CameraSensorHelper is available in software ISP.\n\ninit() is meant to read parameters from the tuning file, and perform\none-time initialization of data that do not vary during the life time of\nthe camera. Here you're initializing the active state, which is then\nmodified by process(). The active state should be reinitialized with\neach capture session to avoid leaking state between sessions. Actually,\nif you look for instance at IPARkISP1::configure(), you will see\n\n\t/* Clear the IPA context before the streaming session. */\n\tcontext_.configuration = {};\n\tcontext_.activeState = {}; \n\tcontext_.frameContexts.clear();\n\nNow, I think there are use cases for retaining some state between\nsessions, to speed up convergence of algorithms, but I think that should\nbe designed and built on top of a base where the state is cleared,\ninstead of leaking the state between sessions in an uncontrolled way.\n\n> In any case, moving it to configure() right now doesn't harm and I can\n> do it in v7 (together with moving the gamma initialization).\n> \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> >> >\n> >> > Please reflow to 80 columns.\n> >> >\n> >> >> +\t */\n> >> >> +\tconstexpr float ignoredPercentage_ = 0.02;\n> >> >\n> >> > It's not a member variable, so no trailing underscore.\n> >> >\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..2f73db52\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> >> >> +\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> >> > I think you can drop controls.h, swisp_stats.h and ipa_context.h.\n> >> > They're included only for classes and structures that are part of the\n> >> > Algorithm class API, so they're guaranteed to be included through\n> >> > algorithm.h.\n> >> >\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 3c1c7262..268cff32 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 e7d8b8df..22156569 100644\n> >> >> --- a/src/ipa/simple/ipa_context.h\n> >> >> +++ b/src/ipa/simple/ipa_context.h\n> >> >> @@ -7,6 +7,8 @@\n> >> >> \n> >> >> #pragma once\n> >> >> \n> >> >> +#include \n> >> >> +\n> >> >> #include \n> >> >> \n> >> >> namespace libcamera {\n> >> >> @@ -17,6 +19,9 @@ struct IPASessionConfiguration {\n> >> >> };\n> >> >> \n> >> >> struct IPAActiveState {\n> >> >> +\tstruct {\n> >> >> +\t\tuint8_t level;\n> >> >> +\t} black;\n> >> >\n> >> > How about naming this blc ?\n> >> >\n> >> > Reviewed-by: Laurent Pinchart \n> >> >\n> >> >> };\n> >> >> \n> >> >> struct IPAFrameContext : public FrameContext {\n> >> >> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> >> >> index dcd7c70a..2f9f15f4 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 4b9201dc..a1bd2f0c 100644\n> >> >> --- a/src/ipa/simple/soft_simple.cpp\n> >> >> +++ b/src/ipa/simple/soft_simple.cpp\n> >> >> @@ -29,7 +29,6 @@\n> >> >> \n> >> >> #include \"libipa/camera_sensor_helper.h\"\n> >> >> \n> >> >> -#include \"black_level.h\"\n> >> >> #include \"module.h\"\n> >> >> \n> >> >> namespace libcamera {\n> >> >> @@ -61,7 +60,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> >> >> @@ -93,7 +92,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(const uint32_t frame,\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 9CC88BF415\n\tfor ;\n\tFri, 6 Sep 2024 21:18:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 921CE634EE;\n\tFri, 6 Sep 2024 23:18:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCC29634E3\n\tfor ;\n\tFri, 6 Sep 2024 23:18:09 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A7D3160;\n\tFri, 6 Sep 2024 23:16:55 +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=\"nFZmoNrf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725657415;\n\tbh=bkFaXkj/NQ0xzSAiJO9Za3CcaBRMuBnwDXCnBmY8uQo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nFZmoNrfLt8CNPQuHJbxgCLlboPb/4JWRGsDxjRfqkRST62dB6nIl8L7gF/AEM2pc\n\txEWzGZ8VQqNPVGp2NMENXiAIa4svOFTyvpx80lzonm8lpFfRzNZhd4sg9Rnq3LXg1m\n\tYO1ljT5P/6VnMSeFTaNGxJWsK+cN9niCobBAP4pE=","Date":"Sat, 7 Sep 2024 00:18:07 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tUmang Jain ,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to an\n\talgorithm module","Message-ID":"<20240906211807.GG27086@pendragon.ideasonboard.com>","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-14-mzamazal@redhat.com>\n\t<20240901210338.GB31148@pendragon.ideasonboard.com>\n\t<87msklyv82.fsf@redhat.com>\n\t<20240906125743.GA27086@pendragon.ideasonboard.com>\n\t<871q1wzxsm.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<871q1wzxsm.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":31248,"web_url":"https://patchwork.libcamera.org/comment/31248/","msgid":"<87plp38vyn.fsf@redhat.com>","date":"2024-09-16T15:22:24","subject":"Re: [PATCH v5 13/18] 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":"Laurent Pinchart writes:\n\n> On Fri, Sep 06, 2024 at 04:01:13PM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>> > On Fri, Sep 06, 2024 at 11:42:05AM +0200, Milan Zamazal wrote:\n>\n>> >> Laurent Pinchart writes:\n>> >> > On Fri, Aug 30, 2024 at 09:25:49AM +0200, 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>> >> >> 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>> >> >> The comment about getting black level from the tuning files is dropped.\n>> >> >> The black level will be taken from CameraSensorHelper if available and\n>> >> >> that will be implemented in one of the followup patches.\n>> >> >> \n>> >> >> Black level is now recomputed on each stats processing. In a future\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>> >> >> Reviewed-by: Umang Jain \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..3b73d830\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>> >> >> +\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>> >> >\n>> >> > Is init() the right place, shouldn't this be done in configure() ?\n>> >> > Setting the initial value in init() means that a new capture session\n>> >> > will not start with a reinitialized black level value.\n>> >> \n>> >> That should be fine as long as the sensor is the same, which I suppose\n>> >> is the case.\n>> >\n>> > Is it fine ? Should an application expect that the camera will start\n>> > with identical parameters for the first capture session and for all\n>> > subsequent ones ?\n>> \n>> I'm not sure. Since init() is what takes tuningData as a parameter, I'd\n>> think a new instance should be created any time something substantial\n>> changes (e.g. a different sensor is used). OTOH in the patch (posted\n>> separately) taking black level from CameraSensorHelper if available, I\n>> move the initialization from init() to configure() because this is where\n>> CameraSensorHelper is available in software ISP.\n>\n> init() is meant to read parameters from the tuning file, and perform\n> one-time initialization of data that do not vary during the life time of\n> the camera. Here you're initializing the active state, which is then\n> modified by process(). The active state should be reinitialized with\n> each capture session to avoid leaking state between sessions. Actually,\n> if you look for instance at IPARkISP1::configure(), you will see\n>\n> \t/* Clear the IPA context before the streaming session. */\n> \tcontext_.configuration = {};\n> \tcontext_.activeState = {}; \n> \tcontext_.frameContexts.clear();\n>\n> Now, I think there are use cases for retaining some state between\n> sessions, to speed up convergence of algorithms, but I think that should\n> be designed and built on top of a base where the state is cleared,\n> instead of leaking the state between sessions in an uncontrolled way.\n\nThank you for explanation. I'll rethink it for v7, most likely simply\nmoving the initialization to configure() + some commentary, to not\ncomplicated things at this stage.\n\n>> In any case, moving it to configure() right now doesn't harm and I can\n>> do it in v7 (together with moving the gamma initialization).\n>> \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>> >> >\n>> >> > Please reflow to 80 columns.\n>> >> >\n>> >> >> +\t */\n>> >> >> +\tconstexpr float ignoredPercentage_ = 0.02;\n>> >> >\n>> >> > It's not a member variable, so no trailing underscore.\n>> >> >\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..2f73db52\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>> >> >> +\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>> >> > I think you can drop controls.h, swisp_stats.h and ipa_context.h.\n>> >> > They're included only for classes and structures that are part of the\n>> >> > Algorithm class API, so they're guaranteed to be included through\n>> >> > algorithm.h.\n>> >> >\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 3c1c7262..268cff32 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 e7d8b8df..22156569 100644\n>> >> >> --- a/src/ipa/simple/ipa_context.h\n>> >> >> +++ b/src/ipa/simple/ipa_context.h\n>> >> >> @@ -7,6 +7,8 @@\n>> >> >> \n>> >> >> #pragma once\n>> >> >> \n>> >> >> +#include \n>> >> >> +\n>> >> >> #include \n>> >> >> \n>> >> >> namespace libcamera {\n>> >> >> @@ -17,6 +19,9 @@ struct IPASessionConfiguration {\n>> >> >> };\n>> >> >> \n>> >> >> struct IPAActiveState {\n>> >> >> +\tstruct {\n>> >> >> +\t\tuint8_t level;\n>> >> >> +\t} black;\n>> >> >\n>> >> > How about naming this blc ?\n>> >> >\n>> >> > Reviewed-by: Laurent Pinchart \n>> >> >\n>> >> >> };\n>> >> >> \n>> >> >> struct IPAFrameContext : public FrameContext {\n>> >> >> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n>> >> >> index dcd7c70a..2f9f15f4 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 4b9201dc..a1bd2f0c 100644\n>> >> >> --- a/src/ipa/simple/soft_simple.cpp\n>> >> >> +++ b/src/ipa/simple/soft_simple.cpp\n>> >> >> @@ -29,7 +29,6 @@\n>> >> >> \n>> >> >> #include \"libipa/camera_sensor_helper.h\"\n>> >> >> \n>> >> >> -#include \"black_level.h\"\n>> >> >> #include \"module.h\"\n>> >> >> \n>> >> >> namespace libcamera {\n>> >> >> @@ -61,7 +60,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>> >> >> @@ -93,7 +92,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(const uint32_t frame,\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 AF547C324C\n\tfor ;\n\tMon, 16 Sep 2024 15:22:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 956B6634FE;\n\tMon, 16 Sep 2024 17:22:32 +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 B3702634F5\n\tfor ;\n\tMon, 16 Sep 2024 17:22:30 +0200 (CEST)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-536-vGoKR6mAMMS10Xy7eFtJsA-1; Mon, 16 Sep 2024 11:22:28 -0400","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-378ab5b74e1so1268473f8f.3\n\tfor ;\n\tMon, 16 Sep 2024 08:22:28 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-378e71f0600sm7529404f8f.9.2024.09.16.08.22.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 16 Sep 2024 08:22: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=\"PBm12A06\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726500149;\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=tGOyCvK51q7IOKl+XxmMKrC1wIce1XtLUs9v6Ike+rw=;\n\tb=PBm12A06iPZTrLHUscZFsRrg/tRYnZSIkYCd1Sq2yX41SxTFC/9D5hHDGoAWyWw7KcYMbg\n\tiPHBZT4DsJY6A+4Dhaap8HD18y2EGd3qH0akn1dEANNLDl3Doo9RtsAakhX3afPfqjslCt\n\tqA9/wLdNiha1iTo9p+jFf4uKKOb/dfI=","X-MC-Unique":"vGoKR6mAMMS10Xy7eFtJsA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726500147; x=1727104947;\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=tGOyCvK51q7IOKl+XxmMKrC1wIce1XtLUs9v6Ike+rw=;\n\tb=I0IRdFTs4z2/JVtw71RDNfTNcLIKLGKT73cWBArsvZ1i7kefBQi3oxYRrw+ofH0aIp\n\tSsZjBjzJ243UEi6vfQDvuv0o8t2zpaHvwWPWw8B6XUzsVYwh/3bpocitD8YkW6scI2No\n\tsCF6v/Yv+b37crs/k7HF71SiCA2o9LWMtlvGL3Bfsh303XkN6ZxvceQbE259z9S2SdsQ\n\tGgS0ikPCvznDlZYh6iQKb2TTQqeNfZjyMuoWQheh+Hm4mxdE7oXuP7qvG4+wowIHs9Ll\n\tR2oihwYrBFS9R++ViYjlZka6Nu5BL3RouxbJG32KyPJBgIN0Be/256g1Mds/5ADUKII7\n\tBsoA==","X-Gm-Message-State":"AOJu0YzIKm1974Y5w2LRCtDqXoBMqeUuEXKbaDW6w4T2NDLUgVHjId2X\n\t9HtwtZVHmIiVGQGKxmApWmDZk3MlVlMVnbxdlXj5SirrntEu+y8EFzJtF2sNgOtNH559RsvpOE8\n\tY6jbJaMrXCm5DegRqh+LByalP41QF5FEjJa+ao+RwAuLiRZcIcJo+Cq2+qAj87J8NZgg/TF8=","X-Received":["by 2002:adf:ff8f:0:b0:378:89d8:8242 with SMTP id\n\tffacd0b85a97d-378d61f0f87mr5756706f8f.26.1726500146770; \n\tMon, 16 Sep 2024 08:22:26 -0700 (PDT)","by 2002:adf:ff8f:0:b0:378:89d8:8242 with SMTP id\n\tffacd0b85a97d-378d61f0f87mr5756691f8f.26.1726500146088; \n\tMon, 16 Sep 2024 08:22:26 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IF2kkQarOgvhjIO/Yjn64V0vrOVOYscetuDSwuTbDGCMlo9a5Bnfp2vZFXo9XxQ1aQQxQsO4A==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Umang Jain\n\t,\n\tDaniel Scally ","Subject":"Re: [PATCH v5 13/18] libcamera: software_isp: Move black level to\n\tan algorithm module","In-Reply-To":"<20240906211807.GG27086@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Sat, 7 Sep 2024 00:18:07 +0300\")","References":"<20240830072554.180672-1-mzamazal@redhat.com>\n\t<20240830072554.180672-14-mzamazal@redhat.com>\n\t<20240901210338.GB31148@pendragon.ideasonboard.com>\n\t<87msklyv82.fsf@redhat.com>\n\t<20240906125743.GA27086@pendragon.ideasonboard.com>\n\t<871q1wzxsm.fsf@redhat.com>\n\t<20240906211807.GG27086@pendragon.ideasonboard.com>","Date":"Mon, 16 Sep 2024 17:22:24 +0200","Message-ID":"<87plp38vyn.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\" "}}]