[{"id":32924,"web_url":"https://patchwork.libcamera.org/comment/32924/","msgid":"<85zfk82d6e.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-03T14:18:49","subject":"Re: [RFC PATCH 2/2] libcamera: bitdepth: Adapt camera_sensor_helper\n\tto use BitDepth","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Isaac,\n\nthank you for the patch.\n\nIsaac Scott <isaac.scott@ideasonboard.com> writes:\n\n> Adapt the camera_sensor_helper class to use BitDepth instead of bit\n> shifting for black levels as an example of how the BitDepth\n> implementation can be used.\n>\n> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>\n> ---\n>  src/ipa/libipa/camera_sensor_helper.cpp | 18 +++++++++---------\n>  src/ipa/libipa/camera_sensor_helper.h   |  5 +++--\n>  src/ipa/simple/algorithms/awb.cpp       |  3 ++-\n>  src/ipa/simple/algorithms/blc.cpp       | 10 ++++++----\n>  src/ipa/simple/ipa_context.h            |  5 +++--\n>  src/ipa/simple/soft_simple.cpp          |  5 ++---\n>  6 files changed, 25 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index c6169bdc..1031dbd1 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -407,7 +407,7 @@ public:\n>  \tCameraSensorHelperAr0144()\n>  \t{\n>  \t\t/* Power-on default value: 168 at 12bits. */\n> -\t\tblackLevel_ = 2688;\n> +\t\tblackLevel_ = 168_12bit;\n>  \t}\n>  \n>  \tuint32_t gainCode(double gain) const override\n> @@ -525,7 +525,7 @@ public:\n>  \tCameraSensorHelperImx214()\n>  \t{\n>  \t\t/* From datasheet: 64 at 10bits. */\n> -\t\tblackLevel_ = 4096;\n> +\t\tblackLevel_ = 64_10bit;\n>  \t\tgainType_ = AnalogueGainLinear;\n>  \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n>  \t}\n> @@ -538,7 +538,7 @@ public:\n>  \tCameraSensorHelperImx219()\n>  \t{\n>  \t\t/* From datasheet: 64 at 10bits. */\n> -\t\tblackLevel_ = 4096;\n> +\t\tblackLevel_ = 64_10bit;\n>  \t\tgainType_ = AnalogueGainLinear;\n>  \t\tgainConstants_.linear = { 0, 256, -1, 256 };\n>  \t}\n> @@ -551,7 +551,7 @@ public:\n>  \tCameraSensorHelperImx258()\n>  \t{\n>  \t\t/* From datasheet: 0x40 at 10bits. */\n> -\t\tblackLevel_ = 4096;\n> +\t\tblackLevel_ = 0x40_10bit;\n>  \t\tgainType_ = AnalogueGainLinear;\n>  \t\tgainConstants_.linear = { 0, 512, -1, 512 };\n>  \t}\n> @@ -564,7 +564,7 @@ public:\n>  \tCameraSensorHelperImx283()\n>  \t{\n>  \t\t/* From datasheet: 0x32 at 10bits. */\n> -\t\tblackLevel_ = 3200;\n> +\t\tblackLevel_ = 0x32_10bit;\n>  \t\tgainType_ = AnalogueGainLinear;\n>  \t\tgainConstants_.linear = { 0, 2048, -1, 2048 };\n>  \t}\n> @@ -604,7 +604,7 @@ public:\n>  \tCameraSensorHelperImx335()\n>  \t{\n>  \t\t/* From datasheet: 0x32 at 10bits. */\n> -\t\tblackLevel_ = 3200;\n> +\t\tblackLevel_ = 0x32_10bit;\n>  \t\tgainType_ = AnalogueGainExponential;\n>  \t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n>  \t}\n> @@ -665,7 +665,7 @@ public:\n>  \tCameraSensorHelperOv4689()\n>  \t{\n>  \t\t/* From datasheet: 0x40 at 12bits. */\n> -\t\tblackLevel_ = 1024;\n> +\t\tblackLevel_ = 0x40_12bit;\n>  \t\tgainType_ = AnalogueGainLinear;\n>  \t\tgainConstants_.linear = { 1, 0, 0, 128 };\n>  \t}\n> @@ -678,7 +678,7 @@ public:\n>  \tCameraSensorHelperOv5640()\n>  \t{\n>  \t\t/* From datasheet: 0x10 at 10bits. */\n> -\t\tblackLevel_ = 1024;\n> +\t\tblackLevel_ = 0x10_10bit;\n>  \t\tgainType_ = AnalogueGainLinear;\n>  \t\tgainConstants_.linear = { 1, 0, 0, 16 };\n>  \t}\n> @@ -713,7 +713,7 @@ public:\n>  \tCameraSensorHelperOv5675()\n>  \t{\n>  \t\t/* From Linux kernel driver: 0x40 at 10bits. */\n> -\t\tblackLevel_ = 4096;\n> +\t\tblackLevel_ = 0x40_10bit;\n>  \t\tgainType_ = AnalogueGainLinear;\n>  \t\tgainConstants_.linear = { 1, 0, 0, 128 };\n>  \t}\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> index 75868205..b72606bf 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -14,6 +14,7 @@\n>  #include <vector>\n>  \n>  #include <libcamera/base/class.h>\n> +#include \"bitdepth.h\"\n>  \n>  namespace libcamera {\n>  \n> @@ -25,7 +26,7 @@ public:\n>  \tCameraSensorHelper() = default;\n>  \tvirtual ~CameraSensorHelper() = default;\n>  \n> -\tstd::optional<int16_t> blackLevel() const { return blackLevel_; }\n> +\tstd::optional<BitDepthValue<16>> blackLevel() const { return blackLevel_; }\n>  \tvirtual uint32_t gainCode(double gain) const;\n>  \tvirtual double gain(uint32_t gainCode) const;\n>  \n> @@ -52,7 +53,7 @@ protected:\n>  \t\tAnalogueGainExpConstants exp;\n>  \t};\n>  \n> -\tstd::optional<int16_t> blackLevel_;\n> +\tstd::optional<BitDepthValue<16>> blackLevel_;\n>  \tAnalogueGainType gainType_;\n>  \tAnalogueGainConstants gainConstants_;\n>  \n> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp\n> index 195de41d..cfce5869 100644\n> --- a/src/ipa/simple/algorithms/awb.cpp\n> +++ b/src/ipa/simple/algorithms/awb.cpp\n> @@ -12,6 +12,7 @@\n>  \n>  #include <libcamera/base/log.h>\n>  \n> +#include \"libipa/bitdepth.h\"\n>  #include \"simple/ipa_context.h\"\n>  \n>  namespace libcamera {\n> @@ -36,7 +37,7 @@ void Awb::process(IPAContext &context,\n>  \t\t  [[maybe_unused]] ControlList &metadata)\n>  {\n>  \tconst SwIspStats::Histogram &histogram = stats->yHistogram;\n> -\tconst uint8_t blackLevel = context.activeState.blc.level;\n> +\tconst BitDepthValue<8> blackLevel = context.activeState.blc.level;\n>  \n>  \t/*\n>  \t * Black level must be subtracted to get the correct AWB ratios, they\n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index b4e32fe1..7c5d3f6d 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -10,6 +10,7 @@\n>  #include <numeric>\n>  \n>  #include <libcamera/base/log.h>\n> +#include \"libipa/bitdepth.h\"\n>  \n>  namespace libcamera {\n>  \n> @@ -29,7 +30,7 @@ int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)\n>  \t\t * Convert 16 bit values from the tuning file to 8 bit black\n>  \t\t * level for the SoftISP.\n>  \t\t */\n> -\t\tcontext.configuration.black.level = blackLevel.value() >> 8;\n> +\t\tcontext.configuration.black.level->convert<8>();\n\nI think it should be\n\n  context.configuration.black.level = blackLevel.convert<8>();\n\n>  \t}\n>  \treturn 0;\n>  }\n> @@ -38,7 +39,7 @@ int BlackLevel::configure(IPAContext &context,\n>  \t\t\t  [[maybe_unused]] const IPAConfigInfo &configInfo)\n>  {\n>  \tcontext.activeState.blc.level =\n> -\t\tcontext.configuration.black.level.value_or(255);\n> +\t\tcontext.configuration.black.level.value_or(255_8bit);\n>  \treturn 0;\n>  }\n>  \n> @@ -68,14 +69,15 @@ void BlackLevel::process(IPAContext &context,\n>  \tconst unsigned int pixelThreshold = ignoredPercentage * total;\n>  \tconst unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;\n>  \tconst unsigned int currentBlackIdx =\n> -\t\tcontext.activeState.blc.level / histogramRatio;\n> +\t\tcontext.activeState.blc.level.value() / histogramRatio;\n\nThis is a typical example why I think using plain `value()' without a\nspecified depth may be not sufficiently clear and possibly error-prone,\nsince the internal depth of context.activeState.blc.level is not visible\nhere.\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.blc.level = i * histogramRatio;\n> +\t\t\tcontext.activeState.blc.level =\n> +\t\t\t\tBitDepthValue<8>(i * histogramRatio);\n>  \t\t\texposure_ = frameContext.sensor.exposure;\n>  \t\t\tgain_ = frameContext.sensor.gain;\n>  \t\t\tLOG(IPASoftBL, Debug)\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index fd121eeb..be3b967a 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -12,6 +12,7 @@\n>  #include <stdint.h>\n>  \n>  #include <libipa/fc_queue.h>\n> +#include \"libipa/bitdepth.h\"\n>  \n>  namespace libcamera {\n>  \n> @@ -24,13 +25,13 @@ struct IPASessionConfiguration {\n>  \t\tdouble againMin, againMax, againMinStep;\n>  \t} agc;\n>  \tstruct {\n> -\t\tstd::optional<uint8_t> level;\n> +\t\tstd::optional<BitDepthValue<8>> level;\n>  \t} black;\n>  };\n>  \n>  struct IPAActiveState {\n>  \tstruct {\n> -\t\tuint8_t level;\n> +\t\tBitDepthValue<8> level;\n>  \t} blc;\n>  \n>  \tstruct {\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index ac2a9421..292fa81f 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -211,11 +211,10 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>  \t\t\t/*\n>  \t\t\t * The black level from camHelper_ is a 16 bit value, software ISP\n>  \t\t\t * works with 8 bit pixel values, both regardless of the actual\n> -\t\t\t * sensor pixel width. Hence we obtain the pixel-based black value\n> -\t\t\t * by dividing the value from the helper by 256.\n> +\t\t\t * sensor pixel width.\n>  \t\t\t */\n>  \t\t\tcontext_.configuration.black.level =\n> -\t\t\t\tcamHelper_->blackLevel().value() / 256;\n> +\t\t\t\tcamHelper_->blackLevel();\n>  \t\t}\n>  \t} else {\n>  \t\t/*","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 397BBBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  3 Jan 2025 14:18:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53E76684CC;\n\tFri,  3 Jan 2025 15:18:58 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A56761886\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  3 Jan 2025 15:18:56 +0100 (CET)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-390-nQjD1ie2PQWcd5jaFQikwA-1; Fri, 03 Jan 2025 09:18:53 -0500","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-4361d4e8359so97724265e9.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 03 Jan 2025 06:18:53 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-4364a3766dcsm390339245e9.0.2025.01.03.06.18.50\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 03 Jan 2025 06:18:50 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Ycl/RQU4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1735913935;\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=GRfJ6bvv4iHL/VOZUThwPJ2l31xyk++QwWQnrW6RzLU=;\n\tb=Ycl/RQU4rdWHjh54wH+LdCkVLa1vZ8FFFwSwMZ5jt5C/ERXRtL9K88rfSKjZp19U9hYTbw\n\t0kAsujzsZk3RHT2jjCSFgEdzclrq/LSxt7ACsk8RzuMBpeadiaNgKd2nVWuwq/BOi11E5Q\n\tbWpT3wpV55RWYTS2D2W2VoVXGIeFdKU=","X-MC-Unique":"nQjD1ie2PQWcd5jaFQikwA-1","X-Mimecast-MFC-AGG-ID":"nQjD1ie2PQWcd5jaFQikwA","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1735913932; x=1736518732;\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=GRfJ6bvv4iHL/VOZUThwPJ2l31xyk++QwWQnrW6RzLU=;\n\tb=BioRpRiidXB6Au1FVsOFmUdmBT5+T1AH3qZZThYanG4oucD5Bkh6fJUq2xbNiqazUp\n\tvVunnO91RKB7uf9EaBDx9bkQYqXX8jKMVLm7FmRVU72pkR6L9qd98xJ7kUhn8ME9KL8F\n\tQvzQZ68QHj5t6VkZv7oVRvb/Qhy2ahrivq8FOO2D2x4EFBo+4zieelIsMhMnDIIVaCDW\n\txZOeuh5+KtemvSA09sIi6aqp5QXJ1ySnCuwE1pQfjql7RtnleSGV1/vuI+5OCxfKiXML\n\t7p8Vkm1ZqM5mDIbFWt7nO9+rWeBL0YaeD67ofJkFgA89SOVg8IunKhRlmQS+y1u9ZL/g\n\tl1Fg==","X-Gm-Message-State":"AOJu0YzhGG3VJPk2wRCq3ETdkiRNZ+xZ8gT94+zGbzvthXL18EshB3uG\n\tqNWv+Pk9PY5xCQJhzMWK+EecvxpG/F9fcAgqNONzpvC3GUYDfPGY3OgKyqqWS92gaUTgmF+qvHC\n\tBeCQClF/hy3QBtd7ApvgVlQZ/eymylJQuYuCj1WSo04dmUsUJqwdxS9TDZJ069a04XiYaagrqZP\n\tga5zBjj2kpN/EpaQSlzEwNiXyFm7iPXkhYgZoN/Hp1d16e0QGeUi//TYE=","X-Gm-Gg":"ASbGnctHs6WRghXKj2YJfLPGhtfW3dIZOLRWxWmvkxhZmlDcmKlbf+ozVQej5ovHWco\n\t8lHEWu2b+yI3YV0Kn5f6zfcwHG25aUwkH1oppzE5FCKRc2E94TXd18qn7HQXSS4mZucI7kQXcq/\n\tav3WY9OtFUz83u1so8liFc1jaDi7vBFt7GqVzFYBQg0Nw0WJXhtpSN5Hw8ENwTyOoDOSgwFl0st\n\touP7BXMo8GahiklE6/v4TkyBfgecyO201W4glOaHJ+xYut80qNNiV2BBe5Qc0JXUwjswSeEP5YW\n\t+9IKpQy4rQ0CGMImhS3R05a/hanM/RpDAA==","X-Received":["by 2002:a05:600c:1d2a:b0:435:330d:de86 with SMTP id\n\t5b1f17b1804b1-4366790d3c0mr473067205e9.0.1735913931839; \n\tFri, 03 Jan 2025 06:18:51 -0800 (PST)","by 2002:a05:600c:1d2a:b0:435:330d:de86 with SMTP id\n\t5b1f17b1804b1-4366790d3c0mr473066915e9.0.1735913931338; \n\tFri, 03 Jan 2025 06:18:51 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFHdflTkKpS1XeIzUpPKcnZgIq0G/HOA5b6/hkIuAIbRp7Dz242IGtiS5qtV9IJfuoIQoXn7A==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Isaac Scott <isaac.scott@ideasonboard.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Subject":"Re: [RFC PATCH 2/2] libcamera: bitdepth: Adapt camera_sensor_helper\n\tto use BitDepth","In-Reply-To":"<20241127144655.1074720-3-isaac.scott@ideasonboard.com> (Isaac\n\tScott's message of \"Wed, 27 Nov 2024 14:46:55 +0000\")","References":"<20241127144655.1074720-1-isaac.scott@ideasonboard.com>\n\t<20241127144655.1074720-3-isaac.scott@ideasonboard.com>","Date":"Fri, 03 Jan 2025 15:18:49 +0100","Message-ID":"<85zfk82d6e.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"zGlT_1Ds2Qv8K3CS2x1I9iNVS98SWznjS_AHwN81xoY_1735913932","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>"}}]