[{"id":31195,"web_url":"https://patchwork.libcamera.org/comment/31195/","msgid":"<172614404840.1037309.5604310505759954702@ping.linuxembedded.co.uk>","date":"2024-09-12T12:27:28","subject":"Re: [PATCH 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-08-15 09:37:16)\n> The black level in software ISP is unconditionally guessed from the\n> obtained frames. CameraSensorHelper optionally provides the black level\n> from camera specifications now. Let's use the value if available.\n> \n> The black level value initialization is moved from init to configure\n> because this is where we get the other configuration (gains) from\n> CameraSensorHelper in software ISP.\n> \n> If the black level is not available from the given CameraSensorHelper\n> instance, it's still determined on the fly.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/algorithms/blc.cpp | 10 +++++++---\n> src/ipa/simple/algorithms/blc.h | 3 ++-\n> src/ipa/simple/ipa_context.h | 3 ++-\n> src/ipa/simple/soft_simple.cpp | 3 +++\n> 4 files changed, 14 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index d01c2c14..5dc924bb 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -21,10 +21,11 @@ BlackLevel::BlackLevel()\n> {\n> }\n> \n> -int BlackLevel::init(IPAContext &context,\n> - [[maybe_unused]] const YamlObject &tuningData)\n> +int BlackLevel::configure(typename Module::Context &context,\n> + [[maybe_unused]] const typename Module::Config &configInfo)\n> {\n> - context.activeState.black.level = 1.0;\n> + context.activeState.black.level =\n> + context.configuration.black.level.value_or(1.0);\n> return 0;\n> }\n> \n> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n> const SwIspStats *stats,\n> [[maybe_unused]] ControlList &metadata)\n> {\n> + if (context.configuration.black.level.has_value())\n> + return;\n> +\n> if (frameContext.sensor.exposure == exposure_ &&\n> frameContext.sensor.gain == gain_) {\n> return;\n> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h\n> index 9b9daab1..586a23d2 100644\n> --- a/src/ipa/simple/algorithms/blc.h\n> +++ b/src/ipa/simple/algorithms/blc.h\n> @@ -24,7 +24,8 @@ public:\n> BlackLevel();\n> ~BlackLevel() = default;\n> \n> - int init(IPAContext &context, const YamlObject &tuningData)\n> + int configure(typename Module::Context &context,\n> + const typename Module::Config &configInfo)\n> override;\n> void process(IPAContext &context, const uint32_t frame,\n> IPAFrameContext &frameContext,\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index 0e2096f8..08f965f4 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -8,6 +8,7 @@\n> #pragma once\n> \n> #include \n> +#include \n> \n> #include \n> \n> @@ -24,7 +25,7 @@ struct IPASessionConfiguration {\n> double againMin, againMax, againMinStep;\n> } agc;\n> struct {\n> - double level;\n> + std::optional level;\n> } black;\n> };\n> \n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index aef5f6e5..ac7a22b7 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> (context_.configuration.agc.againMax -\n> context_.configuration.agc.againMin) /\n> 100.0;\n> + if (camHelper_->blackLevel().has_value())\n> + context_.configuration.black.level =\n> + camHelper_->blackLevel().value() / 65536.0;\n\nShouldn't this scale the value depending on the cameras configured bit\ndepth or such for when you use it in your calculations on pixel values?\n\nWhy 65536? \n\nI've recently just pushed the IMX283 black level value as:\n\n> +\t\t/* From datasheet: 0x32 at 10bits. */\n> +\t\tblackLevel_ = 3200;\n\n\nDo you treat black level as value between 0 and 1 ?\n\n--\nKieran\n\n\n\n> } else {\n> /*\n> * The camera sensor gain (g) is usually not equal to the value written\n> -- \n> 2.44.1\n>","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 F0C62C324C\n\tfor ;\n\tThu, 12 Sep 2024 12:27:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D71D634FC;\n\tThu, 12 Sep 2024 14:27:33 +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 CB87A634F2\n\tfor ;\n\tThu, 12 Sep 2024 14:27:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6E3976AF;\n\tThu, 12 Sep 2024 14:26:13 +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=\"XSWKcoA+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726143973;\n\tbh=5/AEDji2f9RbuGRKhr5VBQ4UikWz75bR5NlXEZBbKjs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=XSWKcoA+JhEz88CDMt5V3/Oh8A7xzbgVaaQIGE4rQ0h1/QBxN7q0aRkbXexLnZgns\n\tO5qjGLLIKUeV0D6iPGXI1fFoatCm40RzPaE2D+uXVowF6o7gH/UgBq0BZjzHfE0VwW\n\toIEkp/USyeYmqHzdOiZCeJGHhD7im8E2zb5vqI0Q=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240815083722.237229-2-mzamazal@redhat.com>","References":"<20240815083722.237229-1-mzamazal@redhat.com>\n\t<20240815083722.237229-2-mzamazal@redhat.com>","Subject":"Re: [PATCH 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tDaniel Scally ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Thu, 12 Sep 2024 13:27:28 +0100","Message-ID":"<172614404840.1037309.5604310505759954702@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":31245,"web_url":"https://patchwork.libcamera.org/comment/31245/","msgid":"<87y13r8y3x.fsf@redhat.com>","date":"2024-09-16T14:36:02","subject":"Re: [PATCH 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Kieran,\n\nthank you for review.\n\nKieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-08-15 09:37:16)\n>> The black level in software ISP is unconditionally guessed from the\n>> obtained frames. CameraSensorHelper optionally provides the black level\n>\n>> from camera specifications now. Let's use the value if available.\n>> \n>> The black level value initialization is moved from init to configure\n>> because this is where we get the other configuration (gains) from\n>> CameraSensorHelper in software ISP.\n>> \n>> If the black level is not available from the given CameraSensorHelper\n>> instance, it's still determined on the fly.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/algorithms/blc.cpp | 10 +++++++---\n>> src/ipa/simple/algorithms/blc.h | 3 ++-\n>> src/ipa/simple/ipa_context.h | 3 ++-\n>> src/ipa/simple/soft_simple.cpp | 3 +++\n>> 4 files changed, 14 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> index d01c2c14..5dc924bb 100644\n>> --- a/src/ipa/simple/algorithms/blc.cpp\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -21,10 +21,11 @@ BlackLevel::BlackLevel()\n>> {\n>> }\n>> \n>> -int BlackLevel::init(IPAContext &context,\n>> - [[maybe_unused]] const YamlObject &tuningData)\n>> +int BlackLevel::configure(typename Module::Context &context,\n>> + [[maybe_unused]] const typename Module::Config &configInfo)\n>> {\n>> - context.activeState.black.level = 1.0;\n>> + context.activeState.black.level =\n>> + context.configuration.black.level.value_or(1.0);\n>> return 0;\n>> }\n>> \n>> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n>> const SwIspStats *stats,\n>> [[maybe_unused]] ControlList &metadata)\n>> {\n>> + if (context.configuration.black.level.has_value())\n>> + return;\n>> +\n>> if (frameContext.sensor.exposure == exposure_ &&\n>> frameContext.sensor.gain == gain_) {\n>> return;\n>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h\n>> index 9b9daab1..586a23d2 100644\n>> --- a/src/ipa/simple/algorithms/blc.h\n>> +++ b/src/ipa/simple/algorithms/blc.h\n>> @@ -24,7 +24,8 @@ public:\n>> BlackLevel();\n>> ~BlackLevel() = default;\n>> \n>> - int init(IPAContext &context, const YamlObject &tuningData)\n>> + int configure(typename Module::Context &context,\n>> + const typename Module::Config &configInfo)\n>> override;\n>> void process(IPAContext &context, const uint32_t frame,\n>> IPAFrameContext &frameContext,\n>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index 0e2096f8..08f965f4 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -8,6 +8,7 @@\n>> #pragma once\n>> \n>> #include \n>> +#include \n>> \n>> #include \n>> \n>> @@ -24,7 +25,7 @@ struct IPASessionConfiguration {\n>> double againMin, againMax, againMinStep;\n>> } agc;\n>> struct {\n>> - double level;\n>> + std::optional level;\n>> } black;\n>> };\n>> \n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index aef5f6e5..ac7a22b7 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>> (context_.configuration.agc.againMax -\n>> context_.configuration.agc.againMin) /\n>> 100.0;\n>> + if (camHelper_->blackLevel().has_value())\n>> + context_.configuration.black.level =\n>> + camHelper_->blackLevel().value() / 65536.0;\n>\n> Shouldn't this scale the value depending on the cameras configured bit\n> depth or such for when you use it in your calculations on pixel values?\n>\n> Why 65536? \n\nCameraSensorHelper::blackLevel() docstring says:\n\n This function returns the black level of the sensor scaled to a 16bit\n pixel width.\n\n> I've recently just pushed the IMX283 black level value as:\n>\n>> +\t\t/* From datasheet: 0x32 at 10bits. */\n>> +\t\tblackLevel_ = 3200;\n\nWhich is in accordance with that docstring:\n50 (== 0x32) * 64 (== 2^(16-10)) = 3200\n\n> Do you treat black level as value between 0 and 1 ?\n\nYes, this is what it is expected to be after the current software ISP\nrefactoring patches, but it may change based on the final version.\n\n> --\n> Kieran\n>\n>\n>\n>> } else {\n>> /*\n>> * The camera sensor gain (g) is usually not equal to the value written\n>> -- \n>> 2.44.1\n>>","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 1E815C324C\n\tfor ;\n\tMon, 16 Sep 2024 14:36:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B3BBC634FD;\n\tMon, 16 Sep 2024 16:36:10 +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 DDEEC634F5\n\tfor ;\n\tMon, 16 Sep 2024 16:36:08 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-683-YnzcKHHKMYKcBbr5jN9q-Q-1; Mon, 16 Sep 2024 10:36:06 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-374c35b8e38so1082471f8f.2\n\tfor ;\n\tMon, 16 Sep 2024 07:36:06 -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-42da2426d0esm78944445e9.39.2024.09.16.07.36.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 16 Sep 2024 07:36:03 -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=\"cXomPuio\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726497367;\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=H2E/aycfbLUe8igpONyn+6TP4X711tkazqnEg7ZkgIA=;\n\tb=cXomPuiogu/kxbt7t/MfVRuOJYipxRCIg7jEpATQLnK06okCTiv8A0o0txF75enk08vYIb\n\twsERB3GztRbqe7DacWLKr+jLbzk4PzAeLQCbAHZCV2QzWLlPCKBd63MlsSdzs8a+VBjrDX\n\t5trh7IdwUFz/fny5+MMsW3alzyzi0EI=","X-MC-Unique":"YnzcKHHKMYKcBbr5jN9q-Q-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726497365; x=1727102165;\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=H2E/aycfbLUe8igpONyn+6TP4X711tkazqnEg7ZkgIA=;\n\tb=kXuiYUkY1S+mKyeh29eogjaBecJF40gSxQpgUiYJnClP4j+0WCYFNwqIni6VBgz50M\n\twF9H+vZqSe5N4pYPM3uMA3e/pe8buVUwKNaBAYZ1mHs35NPiDQTnJ99av2PbhNTFByX1\n\tgEJtC2wAvfxbI3qx5lfW9dz6SUD/NpDMGNSLav/WKuqZ1B2L6Cw0z6qHobGBUo1MiiIi\n\tflZAqop3iQJqOq1MmS9ba0v0HOoZNHqo09TrFYaHAXgvOOXOmLNeht1tUlqB3w9P2r8b\n\tn2/2Hr3HWd2K9lnhJhr6aEfWliMMq/hpm9obmb8A6+yadJiuyA9t5rg3cadiRaEiiKms\n\twQZA==","X-Gm-Message-State":"AOJu0YwW5WvyaaHsHJs8sMClxSw5o61+qE4txbLoHixIdNCBEXGNOGtK\n\tdrtxQTvHemEgLP/+mL3YCV5p+mwFXstqXC+98V/5GVSQesgCSz6f96+AwuP9dZlHM5thRrCoNXF\n\trmEkFht2taCxZ3CHuzrfWOGxMtvvYKL/0TFy3MLL/ZZaHDOxAW6hergax9OIdXxpeXqdaiE04J/\n\tjNsiw=","X-Received":["by 2002:a5d:5f56:0:b0:374:d28e:c8f0 with SMTP id\n\tffacd0b85a97d-378d61d51c7mr7526762f8f.11.1726497364981; \n\tMon, 16 Sep 2024 07:36:04 -0700 (PDT)","by 2002:a5d:5f56:0:b0:374:d28e:c8f0 with SMTP id\n\tffacd0b85a97d-378d61d51c7mr7526733f8f.11.1726497364470; \n\tMon, 16 Sep 2024 07:36:04 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHiwYelPRwwGMdOTwZ087ddyoFcDutP7lfPgV+Y5qj2I/dyt66RBawNOXGAY9+SThMT/aAu8w==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Daniel Scally\n\t","Subject":"Re: [PATCH 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","In-Reply-To":"<172614404840.1037309.5604310505759954702@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Thu, 12 Sep 2024 13:27:28 +0100\")","References":"<20240815083722.237229-1-mzamazal@redhat.com>\n\t<20240815083722.237229-2-mzamazal@redhat.com>\n\t<172614404840.1037309.5604310505759954702@ping.linuxembedded.co.uk>","Date":"Mon, 16 Sep 2024 16:36:02 +0200","Message-ID":"<87y13r8y3x.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\" "}}]