[{"id":31677,"web_url":"https://patchwork.libcamera.org/comment/31677/","msgid":"<172851311151.532453.13882033552453641081@ping.linuxembedded.co.uk>","date":"2024-10-09T22:31:51","subject":"Re: [PATCH v2 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-10-09 20:19:33)\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> 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 | 6 +++++-\n> src/ipa/simple/ipa_context.h | 4 ++++\n> src/ipa/simple/soft_simple.cpp | 3 +++\n> 3 files changed, 12 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index b9f2aaa6d..a7af2e12c 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n> int BlackLevel::configure(IPAContext &context,\n> [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> - context.activeState.blc.level = 255;\n> + context.activeState.blc.level =\n> + context.configuration.black.level.value_or(255);\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/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index 3519f20f6..fd121eebe 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> #include \n> \n> #include \n> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n> int32_t exposureMin, exposureMax;\n> double againMin, againMax, againMinStep;\n> } agc;\n> + struct {\n> + std::optional level;\n\nMaybe I asked this before. Is 8 bits enough ? I guess it is if you're\ndividing by 256 ... but I don't understand what happens on a 12 bit\nsensor here ...\n\n> + } black;\n> };\n> \n> struct IPAActiveState {\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index b28c7039f..ad6334ff6 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() / 256;\n\nIs 256 going to work for all sensor modes here? What happens if a sensor\nsupports 8, 10, and 12 bits per pixel?\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 355A1C32DE\n\tfor ;\n\tWed, 9 Oct 2024 22:31:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6482C65369;\n\tThu, 10 Oct 2024 00:31:55 +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 5DBFE63525\n\tfor ;\n\tThu, 10 Oct 2024 00:31:54 +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 A86C4226;\n\tThu, 10 Oct 2024 00:30:16 +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=\"ICL85BqA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728513016;\n\tbh=KEE7k9Ox4xa6Fn/mp/DMPjWZSqtSDMCqiNXMuvTDtXI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ICL85BqAZ2MJRX2gJZRERFyIQ6aauKg01U95J2s7prvJHc4t7gPoDK8JmhrbaCmmQ\n\tXAXIf6zppRTR7IgNnC2beIZAIayE/8RMDqeaG2J8UlGimCHr+kRDbqc9H+qRqPfkHE\n\toHplvhK+ASIlP5fTZr31AyioxVJpC74vzin3wCUE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241009191940.2359227-2-mzamazal@redhat.com>","References":"<20241009191940.2359227-1-mzamazal@redhat.com>\n\t<20241009191940.2359227-2-mzamazal@redhat.com>","Subject":"Re: [PATCH v2 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":"Wed, 09 Oct 2024 23:31:51 +0100","Message-ID":"<172851311151.532453.13882033552453641081@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":31692,"web_url":"https://patchwork.libcamera.org/comment/31692/","msgid":"<87iku0qrvy.fsf@redhat.com>","date":"2024-10-10T08:33:21","subject":"Re: [PATCH v2 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-10-09 20:19:33)\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>> 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 | 6 +++++-\n>> src/ipa/simple/ipa_context.h | 4 ++++\n>> src/ipa/simple/soft_simple.cpp | 3 +++\n>> 3 files changed, 12 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> index b9f2aaa6d..a7af2e12c 100644\n>> --- a/src/ipa/simple/algorithms/blc.cpp\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n>> int BlackLevel::configure(IPAContext &context,\n>> [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> {\n>> - context.activeState.blc.level = 255;\n>> + context.activeState.blc.level =\n>> + context.configuration.black.level.value_or(255);\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/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index 3519f20f6..fd121eebe 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>> #include \n>> \n>> #include \n>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n>> int32_t exposureMin, exposureMax;\n>> double againMin, againMax, againMinStep;\n>> } agc;\n>> + struct {\n>> + std::optional level;\n>\n> Maybe I asked this before. Is 8 bits enough ? I guess it is if you're\n> dividing by 256 ... but I don't understand what happens on a 12 bit\n> sensor here ...\n\nSoftware ISP currently works with 8 bits everywhere. The sensor pixels\nare cut to 8bit if needed before processing, see e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168.\n\nThis might change with GPU processing but in such a case all the\nalgorithms would have to be revised.\n\n>> + } black;\n>> };\n>> \n>> struct IPAActiveState {\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index b28c7039f..ad6334ff6 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() / 256;\n>\n> Is 256 going to work for all sensor modes here? What happens if a sensor\n> supports 8, 10, and 12 bits per pixel?\n\nThe black level from CameraSensorHelper is a 16 bit value\n(https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174)\nand software ISP works with 8 bit values regardless of the number of\nsensor pixel bits. That means all the values are sensor pixel width\nindependent.\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 068F1C32E0\n\tfor ;\n\tThu, 10 Oct 2024 08:33:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD6296536C;\n\tThu, 10 Oct 2024 10:33:29 +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 ADD2763536\n\tfor ;\n\tThu, 10 Oct 2024 10:33:27 +0200 (CEST)","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-145-yMpdxJz7No26z3a-fdk0YQ-1; Thu, 10 Oct 2024 04:33:25 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-4311a383111so858195e9.3\n\tfor ;\n\tThu, 10 Oct 2024 01:33:24 -0700 (PDT)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-37d4b7edebfsm830234f8f.98.2024.10.10.01.33.22\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Oct 2024 01:33:22 -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=\"X1e7mm15\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728549206;\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=0kSwzXLZfxmv6tLj0nrMwgzUxMOdggx/4zIKEMdhxyQ=;\n\tb=X1e7mm15A9ora/5CbB5X0adPbeaI05Jyg+bHVEojBz37XbiKBvP+uFjAvWyQcvTrPYhe8P\n\thO4KVKIYang3fr25Tto5WAVGjKvZF0wVGvSfwjFJkQ5JW9fQhRgIx8r+XR/Q16GUbXTTqT\n\tjoTr5sn7absDN31asCixKoLRSviz1Mg=","X-MC-Unique":"yMpdxJz7No26z3a-fdk0YQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728549203; x=1729154003;\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=0kSwzXLZfxmv6tLj0nrMwgzUxMOdggx/4zIKEMdhxyQ=;\n\tb=OYF9i7N6Xvz/DqWzlfg5IX5h4WNLs0ECOP8PsE+dVbn63X29iIlbbIIrupJBfu3X4w\n\tF0hjdpVkkqlsjeBkFwg+JLiFkSYyVy6t6uH2MkaW+VWBZQKLOqfgvusSs9KslgNp4YQK\n\tixA6yYbdblcOr5pVQ27kSFfxqPiQImqr1yYOiQU+EX1AuqlfRpS5bFWtT/ZmCMeMJo53\n\tUfMLh2W9X3bDmUtMAweqi9K2Lz7Tsb0Q54zEp1shBL4q/n3qYDadZa5aNEqyVJzzwCgf\n\t3+Jznbxb92th/9I17xGLAXDwfrcuzBgl4DnNzJsQiNfmu8QJZRT74rSuIt1y6b7bUzwI\n\tOh9w==","X-Gm-Message-State":"AOJu0YxbgCEfegCdByhxyL6rjM+2e9ZwJcp2jnIwHC+C5vRO26IsHrw+\n\tTEQC8E3s1+jmuvka4Pz1g5YdR5VM6p3ULp1pLQeY2nZn8C5/ew+v6yHWUNsA3iFEtIfgDATvSz7\n\tb5h3FkXd34znz1dfUfZD0I9Z3YyYLEJpzK91xnMGylTBAt7yXyKKR948QclIzxty0Zjt0Ie6xgj\n\tWrMVk=","X-Received":["by 2002:a05:600c:b92:b0:42f:7639:d88d with SMTP id\n\t5b1f17b1804b1-430d70b460bmr33474845e9.35.1728549203647; \n\tThu, 10 Oct 2024 01:33:23 -0700 (PDT)","by 2002:a05:600c:b92:b0:42f:7639:d88d with SMTP id\n\t5b1f17b1804b1-430d70b460bmr33474625e9.35.1728549203163; \n\tThu, 10 Oct 2024 01:33:23 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFbHu0d8M4XX+RAaZWA/tUqVElbS+n7dQxErfnnmxSe6FXRpN5YkVG4qVlr8D/UrR8EtciTYA==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Daniel Scally\n\t","Subject":"Re: [PATCH v2 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","In-Reply-To":"<172851311151.532453.13882033552453641081@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Wed, 09 Oct 2024 23:31:51 +0100\")","References":"<20241009191940.2359227-1-mzamazal@redhat.com>\n\t<20241009191940.2359227-2-mzamazal@redhat.com>\n\t<172851311151.532453.13882033552453641081@ping.linuxembedded.co.uk>","Date":"Thu, 10 Oct 2024 10:33:21 +0200","Message-ID":"<87iku0qrvy.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":31693,"web_url":"https://patchwork.libcamera.org/comment/31693/","msgid":"<172854951069.3353069.14633741246270586827@ping.linuxembedded.co.uk>","date":"2024-10-10T08:38:30","subject":"Re: [PATCH v2 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-10-10 09:33:21)\n> Kieran Bingham writes:\n> \n> > Quoting Milan Zamazal (2024-10-09 20:19:33)\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> >> 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 | 6 +++++-\n> >> src/ipa/simple/ipa_context.h | 4 ++++\n> >> src/ipa/simple/soft_simple.cpp | 3 +++\n> >> 3 files changed, 12 insertions(+), 1 deletion(-)\n> >> \n> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> >> index b9f2aaa6d..a7af2e12c 100644\n> >> --- a/src/ipa/simple/algorithms/blc.cpp\n> >> +++ b/src/ipa/simple/algorithms/blc.cpp\n> >> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n> >> int BlackLevel::configure(IPAContext &context,\n> >> [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> {\n> >> - context.activeState.blc.level = 255;\n> >> + context.activeState.blc.level =\n> >> + context.configuration.black.level.value_or(255);\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/ipa_context.h b/src/ipa/simple/ipa_context.h\n> >> index 3519f20f6..fd121eebe 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> >> #include \n> >> \n> >> #include \n> >> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n> >> int32_t exposureMin, exposureMax;\n> >> double againMin, againMax, againMinStep;\n> >> } agc;\n> >> + struct {\n> >> + std::optional level;\n> >\n> > Maybe I asked this before. Is 8 bits enough ? I guess it is if you're\n> > dividing by 256 ... but I don't understand what happens on a 12 bit\n> > sensor here ...\n> \n> Software ISP currently works with 8 bits everywhere. The sensor pixels\n> are cut to 8bit if needed before processing, see e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168.\n> \n> This might change with GPU processing but in such a case all the\n> algorithms would have to be revised.\n> \n> >> + } black;\n> >> };\n> >> \n> >> struct IPAActiveState {\n> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> >> index b28c7039f..ad6334ff6 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() / 256;\n> >\n> > Is 256 going to work for all sensor modes here? What happens if a sensor\n> > supports 8, 10, and 12 bits per pixel?\n> \n> The black level from CameraSensorHelper is a 16 bit value\n> (https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174)\n> and software ISP works with 8 bit values regardless of the number of\n> sensor pixel bits. That means all the values are sensor pixel width\n> independent.\n\nIt would be really helpful to add a comment here stating that I think...\n\n\nReviewed-by: Kieran Bingham \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> >>\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 27AAAC32EA\n\tfor ;\n\tThu, 10 Oct 2024 08:38:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4346363536;\n\tThu, 10 Oct 2024 10:38:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B55463536\n\tfor ;\n\tThu, 10 Oct 2024 10:38:34 +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 492B04D4;\n\tThu, 10 Oct 2024 10:36:56 +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=\"Gs8wWb/3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728549416;\n\tbh=AULOSnXYD2Hm48DFUjG3bF2OzTFbp65yDnqJVU5mjps=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Gs8wWb/3BKQhfvfUxfRBM+TEmhK025IIJkA1XyXMJOvRs5rdh2k3W8iBlJsULgYbj\n\tzHeoSAYF9C8W3y0TWxETBGiju1EuIWtTgHA37xfcnM0z8LTdDTqRfz3A5zY0nihPa/\n\tvKflYfJRZQGDDBavVO5GFab2P6+1o/+XlQs50uB4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<87iku0qrvy.fsf@redhat.com>","References":"<20241009191940.2359227-1-mzamazal@redhat.com>\n\t<20241009191940.2359227-2-mzamazal@redhat.com>\n\t<172851311151.532453.13882033552453641081@ping.linuxembedded.co.uk>\n\t<87iku0qrvy.fsf@redhat.com>","Subject":"Re: [PATCH v2 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDaniel Scally ","To":"Milan Zamazal ","Date":"Thu, 10 Oct 2024 09:38:30 +0100","Message-ID":"<172854951069.3353069.14633741246270586827@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":31716,"web_url":"https://patchwork.libcamera.org/comment/31716/","msgid":"<87a5faabs6.fsf@redhat.com>","date":"2024-10-11T15:40:09","subject":"Re: [PATCH v2 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-10-10 09:33:21)\n>> Kieran Bingham writes:\n>> \n>\n>> > Quoting Milan Zamazal (2024-10-09 20:19:33)\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>> >> 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 | 6 +++++-\n>> >> src/ipa/simple/ipa_context.h | 4 ++++\n>> >> src/ipa/simple/soft_simple.cpp | 3 +++\n>> >> 3 files changed, 12 insertions(+), 1 deletion(-)\n>> >> \n>> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> >> index b9f2aaa6d..a7af2e12c 100644\n>> >> --- a/src/ipa/simple/algorithms/blc.cpp\n>> >> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> >> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n>> >> int BlackLevel::configure(IPAContext &context,\n>> >> [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> >> {\n>> >> - context.activeState.blc.level = 255;\n>> >> + context.activeState.blc.level =\n>> >> + context.configuration.black.level.value_or(255);\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/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> >> index 3519f20f6..fd121eebe 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>> >> #include \n>> >> \n>> >> #include \n>> >> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n>> >> int32_t exposureMin, exposureMax;\n>> >> double againMin, againMax, againMinStep;\n>> >> } agc;\n>> >> + struct {\n>> >> + std::optional level;\n>> >\n>> > Maybe I asked this before. Is 8 bits enough ? I guess it is if you're\n>> > dividing by 256 ... but I don't understand what happens on a 12 bit\n>> > sensor here ...\n>> \n>> Software ISP currently works with 8 bits everywhere. The sensor pixels\n>> are cut to 8bit if needed before processing, see\n>> e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168.\n>> \n>> This might change with GPU processing but in such a case all the\n>> algorithms would have to be revised.\n>> \n>> >> + } black;\n>> >> };\n>> >> \n>> >> struct IPAActiveState {\n>> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> >> index b28c7039f..ad6334ff6 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() / 256;\n>> >\n>> > Is 256 going to work for all sensor modes here? What happens if a sensor\n>> > supports 8, 10, and 12 bits per pixel?\n>> \n>> The black level from CameraSensorHelper is a 16 bit value\n>> (https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174)\n>> and software ISP works with 8 bit values regardless of the number of\n>> sensor pixel bits. That means all the values are sensor pixel width\n>> independent.\n>\n> It would be really helpful to add a comment here stating that I think...\n\nAdded in v3 (the only change there).\n\n> Reviewed-by: Kieran Bingham \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>> >>\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 3CADCC3260\n\tfor ;\n\tFri, 11 Oct 2024 15:40:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF95B65373;\n\tFri, 11 Oct 2024 17:40:16 +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 9B0FF65370\n\tfor ;\n\tFri, 11 Oct 2024 17:40:14 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-642-P22z7YXeNF-7u59-vmlXiw-1; Fri, 11 Oct 2024 11:40:12 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-43052e05c8fso15149365e9.2\n\tfor ;\n\tFri, 11 Oct 2024 08:40:12 -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-37d4b6bcf04sm4185735f8f.33.2024.10.11.08.40.09\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 11 Oct 2024 08:40:10 -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=\"HE7k/oG3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728661213;\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=YkljiUtdvEvk/JbxK9DRXI94sLukTyY5jEgxAQJym+M=;\n\tb=HE7k/oG3bIpe6O8G3OF2oHbB506ASLouA4SZxvONbiAGd/zjFzM32HEph6/gWtmnqfZjnx\n\tk73YZC3K9bd08QkvKASl1AuiwBcpl0aRZ+n8qOXvk7JSDNQ5ocAj+4m/lQpWiH+ohtUZXE\n\tKfXNBd4/RER3Tzx6et3dRP5KVvW0I6Q=","X-MC-Unique":"P22z7YXeNF-7u59-vmlXiw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728661211; x=1729266011;\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=YkljiUtdvEvk/JbxK9DRXI94sLukTyY5jEgxAQJym+M=;\n\tb=p68p5s8AGARpS7NU+SCm09q0UMbWsJE3ow6nDe39WKry5d6kjuXkfR0X+fXAkXNg2e\n\twIK6tx3pit5ZLyLWYN+MiH7RTAsONeSgzi1qP1DJuHk9R1syiJ1i+TBAaEPOjLRlQ347\n\tgLq8zL+dxAQ9U1onoAvv+A8ZoOG2bhD3cBR998W+/vThGt4iFC5+h0UbYNt+ttkiBr/a\n\tsYQXgifastTRoG6HxrtaNUjc1Xs0dPRybwRz/7oz0rPA5RxiW688YpTEuTNOBm/BhpU+\n\tnYdLfJ/RdYKBTyxlmcOxDRKgr0+H2Xq5UAfW8max9rwK9E8qGPq0npebuD1ale8EFxS6\n\tRJrw==","X-Gm-Message-State":"AOJu0YyrfAMK4UQ6PVh+e+sQbUe2MzaIe2GOSu+K4TlYZ0cHP/eF70Bs\n\tgsP4zLu68h1EPuXPtiB5s034REfTGHyh9Ri80j0mefiqFpdj9nGXGLHlHSh8TU673FK9VJM3YBH\n\tjAxyyn+w+VVNAsC0Ll9uRPpIGUhqeuBrn38NJKZjzy3iSfHimqpNKiRcshG853w/HTIiTB9JxmU\n\tORk7M=","X-Received":["by 2002:a05:600c:5122:b0:42f:8d36:855e with SMTP id\n\t5b1f17b1804b1-4311deb5f47mr29273995e9.5.1728661210910; \n\tFri, 11 Oct 2024 08:40:10 -0700 (PDT)","by 2002:a05:600c:5122:b0:42f:8d36:855e with SMTP id\n\t5b1f17b1804b1-4311deb5f47mr29273775e9.5.1728661210540; \n\tFri, 11 Oct 2024 08:40:10 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFqFrMGqKaTu48xdalOSXHITXXQ1W33nYmLimNr8FyDQl3IbkTLdlK7PmQIwc1TO66gByidRA==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Daniel Scally\n\t","Subject":"Re: [PATCH v2 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","In-Reply-To":"<172854951069.3353069.14633741246270586827@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Thu, 10 Oct 2024 09:38:30 +0100\")","References":"<20241009191940.2359227-1-mzamazal@redhat.com>\n\t<20241009191940.2359227-2-mzamazal@redhat.com>\n\t<172851311151.532453.13882033552453641081@ping.linuxembedded.co.uk>\n\t<87iku0qrvy.fsf@redhat.com>\n\t<172854951069.3353069.14633741246270586827@ping.linuxembedded.co.uk>","Date":"Fri, 11 Oct 2024 17:40:09 +0200","Message-ID":"<87a5faabs6.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\" "}}]