[{"id":30154,"web_url":"https://patchwork.libcamera.org/comment/30154/","msgid":"<15edfeb2-91ba-4328-9aed-f281615cb3c1@ideasonboard.com>","date":"2024-06-29T05:42:18","subject":"Re: [PATCH 14/19] libcamera: software_isp: Move black level to an\n\talgorithm module","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Milan\n\nOn 26/06/24 12:50 pm, Milan Zamazal wrote:\n> The black level determination, already present as a separate class, can\n> be moved to the prepared Algorithm processing structure.  It is the\n> first of the current software ISP algorithms that is called in the stats\n> processing sequence, which means it is also the first one that should be\n> moved to the new structure.  Stats processing starts with calling\n> Algorithm-based processing so the call order of the algorithms is\n> retained.\n>\n> Movement of this algorithm is relatively straightforward because all it\n> does is processing stats.\n>\n> Black level is now recomputed on each stats processing.  In a future\n> 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 <mzamazal@redhat.com>\n> ---\n>   src/ipa/simple/algorithms/blc.cpp     | 79 +++++++++++++++++++++++\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.h          |  7 ++\n>   src/ipa/simple/meson.build            |  1 -\n>   src/ipa/simple/soft_simple.cpp        |  8 +--\n>   9 files changed, 127 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..e216138d\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -0,0 +1,79 @@\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 <numeric>\n> +\n> +#include <libcamera/base/log.h>\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.configuration.black.set = false;\n> +\tcontext.configuration.black.changed = true;\n> +\tcontext.configuration.black.level = 255;\n> +\treturn 0;\n> +}\n> +\n> +void BlackLevel::process(IPAContext &context,\n> +\t\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t\t const SwIspStats *stats,\n> +\t\t\t [[maybe_unused]] ControlList &metadata)\n> +{\n> +\tif (context.configuration.black.set) {\n> +\t\tcontext.configuration.black.changed = false;\n> +\t\treturn;\n> +\t}\n> +\n> +\tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n> +\n> +\t/*\n> +\t * The constant is selected to be \"good enough\", not overly conservative or\n> +\t * aggressive. There is no magic about the given value.\n> +\t */\n> +\tconstexpr float ignoredPercentage_ = 0.02;\n> +\tconst unsigned int total =\n> +\t\tstd::accumulate(begin(histogram), end(histogram), 0);\n> +\tconst unsigned int pixelThreshold = ignoredPercentage_ * total;\n> +\tconst unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;\n> +\tconst unsigned int currentBlackIdx =\n> +\t\tcontext.configuration.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.configuration.black.level = i * histogramRatio;\n> +\t\t\tcontext.configuration.black.changed = 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 - histogram[i]) / total << \"% below, \"\n> +\t\t\t\t<< 100 * seen / total << \"% at or below)\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t};\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(BlackLevel, \"BlackLevel\")\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h\n> new file mode 100644\n> index 00000000..c8b8d92d\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/blc.h\n> @@ -0,0 +1,37 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * black level handling\n> + */\n> +\n> +#pragma once\n> +\n> +#include <libcamera/controls.h>\n> +\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> +\n> +#include \"algorithm.h\"\n> +#include \"ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +class BlackLevel : public Algorithm\n> +{\n> +public:\n> +\tBlackLevel();\n> +\t~BlackLevel() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData)\n> +\t\toverride;\n> +\tvoid process(IPAContext &context, const uint32_t frame,\n> +\t\t     IPAFrameContext &frameContext,\n> +\t\t     const SwIspStats *stats,\n> +\t\t     ControlList &metadata) override;\n> +};\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n> index 31d26e43..1f63c220 100644\n> --- a/src/ipa/simple/algorithms/meson.build\n> +++ b/src/ipa/simple/algorithms/meson.build\n> @@ -1,4 +1,5 @@\n>   # SPDX-License-Identifier: CC0-1.0\n>   \n>   soft_simple_ipa_algorithms = files([\n> +    'blc.cpp',\n>   ])\n> diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp\n> deleted file mode 100644\n> index 37e0109c..00000000\n> --- a/src/ipa/simple/black_level.cpp\n> +++ /dev/null\n> @@ -1,93 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2024, Red Hat Inc.\n> - *\n> - * black level handling\n> - */\n> -\n> -#include \"black_level.h\"\n> -\n> -#include <numeric>\n> -\n> -#include <libcamera/base/log.h>\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 <array>\n> -#include <stdint.h>\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.h b/src/ipa/simple/ipa_context.h\n> index bc1235b6..c79bf6f8 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -8,6 +8,8 @@\n>   \n>   #pragma once\n>   \n> +#include <stdint.h>\n> +\n>   #include <libipa/fc_queue.h>\n>   \n>   namespace libcamera {\n> @@ -15,6 +17,11 @@ namespace libcamera {\n>   namespace ipa::soft {\n>   \n>   struct IPASessionConfiguration {\n> +\tstruct {\n> +\t\tuint8_t level;\n> +\t\tbool set;\n> +\t\tbool changed;\n> +\t} black;\n\nShouldn't this go under IPAActiveState instead ?\n\nAlready needs to be documented as done in other IPAs.\n>   };\n>   \n>   struct IPAActiveState {\n> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> index 08d877bc..cbc928c5 100644\n> --- a/src/ipa/simple/meson.build\n> +++ b/src/ipa/simple/meson.build\n> @@ -7,7 +7,6 @@ ipa_name = 'ipa_soft_simple'\n>   \n>   soft_simple_sources = files([\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 7200abaf..99fb953c 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<CameraSensorHelper> camHelper_;\n>   \tControlInfoMap sensorInfoMap_;\n> -\tBlackLevel blackLevel_;\n>   \n>   \tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>   \tstd::array<uint8_t, kGammaLookupSize> gammaTable_;\n> @@ -305,9 +303,7 @@ void IPASoftSimple::processStats(\n>   \t}\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_.configuration.black.level;\n>   \n>   \t/*\n>   \t * Black level must be subtracted to get the correct AWB ratios, they","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3F269BDB1D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 29 Jun 2024 05:42:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6ACEC62C99;\n\tSat, 29 Jun 2024 07:42:25 +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 A210D619C7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 29 Jun 2024 07:42:23 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B56604B0;\n\tSat, 29 Jun 2024 07:41:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YDMupLQn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719639718;\n\tbh=utQIUWp5d1A5sE10Q1ixdrmr9GvNvyoN4/FtgaMHwg4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=YDMupLQn50uP/gjM3W6penqhrfOELafh/qD3A+hTTfYyuzB0oHEwEu5XAIofBalgO\n\tPQ6PtVVqtnpyRoSlETlz8nRqsfDnCuHXAHkb/UnaRvEjiZ+hbSu1LRIGhnVw53udoV\n\ti67kZSFvR22dOBnsaUPp+4Pz0+GOvwXGQCtKcbWs=","Message-ID":"<15edfeb2-91ba-4328-9aed-f281615cb3c1@ideasonboard.com>","Date":"Sat, 29 Jun 2024 11:12:18 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 14/19] libcamera: software_isp: Move black level to an\n\talgorithm module","Content-Language":"en-US","To":"Milan Zamazal <mzamazal@redhat.com>, libcamera-devel@lists.libcamera.org","References":"<20240626072100.55497-1-mzamazal@redhat.com>\n\t<20240626072100.55497-15-mzamazal@redhat.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","In-Reply-To":"<20240626072100.55497-15-mzamazal@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30283,"web_url":"https://patchwork.libcamera.org/comment/30283/","msgid":"<87tth6idgm.fsf@redhat.com>","date":"2024-07-03T17:37:29","subject":"Re: [PATCH 14/19] libcamera: software_isp: Move black level to an\n\talgorithm module","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Umang,\n\nthank you for review.\n\nUmang Jain <umang.jain@ideasonboard.com> writes:\n\n> Hi Milan\n>\n> On 26/06/24 12:50 pm, Milan Zamazal wrote:\n>> The black level determination, already present as a separate class, can\n>> be moved to the prepared Algorithm processing structure.  It is the\n>> first of the current software ISP algorithms that is called in the stats\n>> processing sequence, which means it is also the first one that should be\n>> moved to the new structure.  Stats processing starts with calling\n>> Algorithm-based processing so the call order of the algorithms is\n>> retained.\n>>\n>> Movement of this algorithm is relatively straightforward because all it\n>> does is processing stats.\n>>\n>> Black level is now recomputed on each stats processing.  In a future\n>> 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 <mzamazal@redhat.com>\n>> ---\n>>   src/ipa/simple/algorithms/blc.cpp     | 79 +++++++++++++++++++++++\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.h          |  7 ++\n>>   src/ipa/simple/meson.build            |  1 -\n>>   src/ipa/simple/soft_simple.cpp        |  8 +--\n>>   9 files changed, 127 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..e216138d\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -0,0 +1,79 @@\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 <numeric>\n>> +\n>> +#include <libcamera/base/log.h>\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.configuration.black.set = false;\n>> +\tcontext.configuration.black.changed = true;\n>> +\tcontext.configuration.black.level = 255;\n>> +\treturn 0;\n>> +}\n>> +\n>> +void BlackLevel::process(IPAContext &context,\n>> +\t\t\t [[maybe_unused]] const uint32_t frame,\n>> +\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n>> +\t\t\t const SwIspStats *stats,\n>> +\t\t\t [[maybe_unused]] ControlList &metadata)\n>> +{\n>> +\tif (context.configuration.black.set) {\n>> +\t\tcontext.configuration.black.changed = false;\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n>> +\n>> +\t/*\n>> +\t * The constant is selected to be \"good enough\", not overly conservative or\n>> +\t * aggressive. There is no magic about the given value.\n>> +\t */\n>> +\tconstexpr float ignoredPercentage_ = 0.02;\n>> +\tconst unsigned int total =\n>> +\t\tstd::accumulate(begin(histogram), end(histogram), 0);\n>> +\tconst unsigned int pixelThreshold = ignoredPercentage_ * total;\n>> +\tconst unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;\n>> +\tconst unsigned int currentBlackIdx =\n>> +\t\tcontext.configuration.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.configuration.black.level = i * histogramRatio;\n>> +\t\t\tcontext.configuration.black.changed = 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 - histogram[i]) / total << \"% below, \"\n>> +\t\t\t\t<< 100 * seen / total << \"% at or below)\";\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t};\n>> +}\n>> +\n>> +REGISTER_IPA_ALGORITHM(BlackLevel, \"BlackLevel\")\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h\n>> new file mode 100644\n>> index 00000000..c8b8d92d\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/blc.h\n>> @@ -0,0 +1,37 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Red Hat Inc.\n>> + *\n>> + * black level handling\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include <libcamera/controls.h>\n>> +\n>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n>> +\n>> +#include \"algorithm.h\"\n>> +#include \"ipa_context.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +class BlackLevel : public Algorithm\n>> +{\n>> +public:\n>> +\tBlackLevel();\n>> +\t~BlackLevel() = default;\n>> +\n>> +\tint init(IPAContext &context, const YamlObject &tuningData)\n>> +\t\toverride;\n>> +\tvoid process(IPAContext &context, const uint32_t frame,\n>> +\t\t     IPAFrameContext &frameContext,\n>> +\t\t     const SwIspStats *stats,\n>> +\t\t     ControlList &metadata) override;\n>> +};\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n>> index 31d26e43..1f63c220 100644\n>> --- a/src/ipa/simple/algorithms/meson.build\n>> +++ b/src/ipa/simple/algorithms/meson.build\n>> @@ -1,4 +1,5 @@\n>>   # SPDX-License-Identifier: CC0-1.0\n>>     soft_simple_ipa_algorithms = files([\n>> +    'blc.cpp',\n>>   ])\n>> diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp\n>> deleted file mode 100644\n>> index 37e0109c..00000000\n>> --- a/src/ipa/simple/black_level.cpp\n>> +++ /dev/null\n>> @@ -1,93 +0,0 @@\n>> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> -/*\n>> - * Copyright (C) 2024, Red Hat Inc.\n>> - *\n>> - * black level handling\n>> - */\n>> -\n>> -#include \"black_level.h\"\n>> -\n>> -#include <numeric>\n>> -\n>> -#include <libcamera/base/log.h>\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 <array>\n>> -#include <stdint.h>\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.h b/src/ipa/simple/ipa_context.h\n>> index bc1235b6..c79bf6f8 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -8,6 +8,8 @@\n>>     #pragma once\n>>   +#include <stdint.h>\n>> +\n>>   #include <libipa/fc_queue.h>\n>>     namespace libcamera {\n>> @@ -15,6 +17,11 @@ namespace libcamera {\n>>   namespace ipa::soft {\n>>     struct IPASessionConfiguration {\n>> +\tstruct {\n>> +\t\tuint8_t level;\n>> +\t\tbool set;\n>> +\t\tbool changed;\n>> +\t} black;\n>\n> Shouldn't this go under IPAActiveState instead ?\n\nIt should.  I thought too much forward about the black level as a\npredefined value but it changes in the current implementation.  There\nare other problems with this patch, I'll rework it a bit.\n\n> Already needs to be documented as done in other IPAs.\n\nOh yes, will do.\n\nThank you for your reviews, what I haven't responded to is clear and\nI'll address it in v2.\n\n>>   };\n>>     struct IPAActiveState {\n>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n>> index 08d877bc..cbc928c5 100644\n>> --- a/src/ipa/simple/meson.build\n>> +++ b/src/ipa/simple/meson.build\n>> @@ -7,7 +7,6 @@ ipa_name = 'ipa_soft_simple'\n>>     soft_simple_sources = files([\n>>       'soft_simple.cpp',\n>> -    'black_level.cpp',\n>>   ])\n>>     soft_simple_sources += soft_simple_ipa_algorithms\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index 7200abaf..99fb953c 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -29,7 +29,6 @@\n>>     #include \"libipa/camera_sensor_helper.h\"\n>>   -#include \"black_level.h\"\n>>   #include \"module.h\"\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<CameraSensorHelper> camHelper_;\n>>   \tControlInfoMap sensorInfoMap_;\n>> -\tBlackLevel blackLevel_;\n>>     \tstatic constexpr unsigned int kGammaLookupSize = 1024;\n>>   \tstd::array<uint8_t, kGammaLookupSize> gammaTable_;\n>> @@ -305,9 +303,7 @@ void IPASoftSimple::processStats(\n>>   \t}\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_.configuration.black.level;\n>>     \t/*\n>>   \t * Black level must be subtracted to get the correct AWB ratios, they","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 632B1BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  3 Jul 2024 17:37:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F8DC62C96;\n\tWed,  3 Jul 2024 19:37:39 +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 446C662C95\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  3 Jul 2024 19:37:36 +0200 (CEST)","from mail-ej1-f72.google.com (mail-ej1-f72.google.com\n\t[209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-635-Fyu6sIUXP2egBO6Wx45Pvw-1; Wed, 03 Jul 2024 13:37:34 -0400","by mail-ej1-f72.google.com with SMTP id\n\ta640c23a62f3a-a77a41fb726so83407366b.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 03 Jul 2024 10:37:33 -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-a72ab08cf7csm530081166b.155.2024.07.03.10.37.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 03 Jul 2024 10:37:30 -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=\"K6Iy/Lbf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1720028255;\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=QYEQjgJpqvqTdCFSUWprlaFs54IhJVbEKuyNU8gHlbQ=;\n\tb=K6Iy/Lbfp5EzmHiRFqNguTFJA2jFI4erZHlbZSiLA5np8Gc6qK31IJXnOSC2yqrTOFDH0T\n\tUIJiKRhdR7qRN8pv7EqCD5UoB9tBTf0m8uvZQvAzbKV2j2ex4OIv3lon3Iur8wc+jGqYUH\n\tyYJLBRZtmBinZfcuH0EZJve8BYDJWj8=","X-MC-Unique":"Fyu6sIUXP2egBO6Wx45Pvw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1720028252; x=1720633052;\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=QYEQjgJpqvqTdCFSUWprlaFs54IhJVbEKuyNU8gHlbQ=;\n\tb=mdnlFeoGKelvvuzy6bIkVUOp0bK+JxxvPprwXI/aLCeMBwfkH8gW/SBcHkJSg6qoDA\n\thv/Ku3fV9dRnZ1BJzpZEF000CUIkTXYPd7d2hJzFze7QDTeMB2pcm1iDvkK0QLBWUGjK\n\tLS9ulcVDXIgFO1eI4R+rg8879hfFeCFaSUYAYJpp1mhyxwKVc9DlHCtKoecGrLOtKOkv\n\trAgEpGASGKgHOQHi/02SyZECg3B3OQbhpM4yC/LvgnfvqDQjj8T2wCj87XU2eCwXsGmq\n\trqBnO/4XeNIEJpffJ422+tiT2l251Uy98tMfnZ+y7VoktVJEiGYE/bxMdkY7jKTkUWGn\n\tmzSg==","X-Gm-Message-State":"AOJu0YwNg6M6/OW6NUNNAttPPw1nThgAsiEc3Zm/EuJWF8ihyWWQcUzz\n\tvgyXRDDT0pv+DQ2d1hUqQePzko8q0O0wSo9loAsdSkzVXLvhdZ+/SYF9IdIIthOW8xb9c0Ly6qo\n\t7Ob4qX6nIiLjtO3T2xX4cK3ILGBYkpzNFGPOMcq1Lfz45eLxN6lAWjqj9o9iSBgUw5aXY0R8hEa\n\tj89WfqCxzPMlIawjGCTcpOiZPv512UmOExv5Done2DGPCIaXolp9AFAbA=","X-Received":["by 2002:a17:907:97c6:b0:a77:a529:4457 with SMTP id\n\ta640c23a62f3a-a77a52944e9mr177694566b.25.1720028251663; \n\tWed, 03 Jul 2024 10:37:31 -0700 (PDT)","by 2002:a17:907:97c6:b0:a77:a529:4457 with SMTP id\n\ta640c23a62f3a-a77a52944e9mr177692066b.25.1720028251007; \n\tWed, 03 Jul 2024 10:37:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IE52iAQDXS87D1/eTZttHbM8Ww/LCjkVVbN7skysNi7CAzeIWw+LrIN25u/zxJNppu8eOGEXw==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 14/19] libcamera: software_isp: Move black level to an\n\talgorithm module","In-Reply-To":"<15edfeb2-91ba-4328-9aed-f281615cb3c1@ideasonboard.com> (Umang\n\tJain's message of \"Sat, 29 Jun 2024 11:12:18 +0530\")","References":"<20240626072100.55497-1-mzamazal@redhat.com>\n\t<20240626072100.55497-15-mzamazal@redhat.com>\n\t<15edfeb2-91ba-4328-9aed-f281615cb3c1@ideasonboard.com>","Date":"Wed, 03 Jul 2024 19:37:29 +0200","Message-ID":"<87tth6idgm.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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]