[{"id":36207,"web_url":"https://patchwork.libcamera.org/comment/36207/","msgid":"<32827e94-09fe-484d-a2c3-676431ba1901@ideasonboard.com>","date":"2025-10-11T19:01:41","subject":"Re: [RFC PATCH 7/7] ipa: softipa: Confine black level configuration","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 10. 11. 18:03 keltezéssel, Kieran Bingham írta:\n> Move the black level handling entirely into the blc module.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/simple/algorithms/blc.cpp | 11 +++++++++++\n> src/ipa/simple/soft_simple.cpp | 10 ----------\n> 2 files changed, 11 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index 370385afc683..060006d350ee 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -40,10 +40,21 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n> int BlackLevel::configure(IPAContext &context,\n> \t\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> +\t/*\n> +\t * The black level from camHelper_ is a 16 bit value, software ISP\n> +\t * works with 8 bit pixel values, both regardless of the actual\n> +\t * sensor pixel width. Hence we obtain the pixel-based black value\n> +\t * by dividing the value from the helper by 256.\n> +\t */\n> +\tcontext.configuration.black.level =\n> +\t\tcontext.camHelper->blackLevel().value() / 256;\n> +\n\nThis no longer checks if `blackLevel()` returns an empty optional or not.\nIt also does not check if `!!context.camHelper`. Is that intentional?\n\nAlso maybe it makes sense to move `context.configuration.black` into the\n`BlackLevel` class since it is not used outside.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \tif (definedLevel_.has_value())\n> \t\tcontext.configuration.black.level = definedLevel_;\n> +\n> \tcontext.activeState.blc.level =\n> \t\tcontext.configuration.black.level.value_or(16);\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index 68ddf71e9f24..caae2c586e9b 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -227,16 +227,6 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo,\n> \t\t\t(context_.configuration.agc.againMax -\n> \t\t\t context_.configuration.agc.againMin) /\n> \t\t\t100.0;\n> -\t\tif (context_.camHelper->blackLevel().has_value()) {\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 */\n> -\t\t\tcontext_.configuration.black.level =\n> -\t\t\t\tcontext_.camHelper->blackLevel().value() / 256;\n> -\t\t}\n> \t} else {\n> \t\t/*\n> \t\t * The camera sensor gain (g) is usually not equal to the value written","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 1446FBE080\n\tfor ;\n\tSat, 11 Oct 2025 19:01:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 649EF60443;\n\tSat, 11 Oct 2025 21:01:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA3BA603DF\n\tfor ;\n\tSat, 11 Oct 2025 21:01:45 +0200 (CEST)","from [192.168.33.30] (185.182.214.105.nat.pool.zt.hu\n\t[185.182.214.105])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 24BB9C67;\n\tSat, 11 Oct 2025 21:00:09 +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=\"ZKA4mZzS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760209209;\n\tbh=PfMqZQbNOPJSDmdxoiJozc7dpUDHtu+GWKUoMIGw/SI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ZKA4mZzSbQhfPi6gIEpiHePgm2pMMWJXXGDnFI34TToUrq6RM2JHhBALOYmI9AQEE\n\tTXi6863aftsTcTNWsQF/EV+PS9zR0WwyhIQKD7SWG65hiNBw0ppHR/iO+3mHUKlaXI\n\txu5+urN07imX/jvkX9icpZTSuaukoLVlPsQpG8+U=","Message-ID":"<32827e94-09fe-484d-a2c3-676431ba1901@ideasonboard.com>","Date":"Sat, 11 Oct 2025 21:01:41 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH 7/7] ipa: softipa: Confine black level configuration","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20251011160335.50578-1-kieran.bingham@ideasonboard.com>\n\t<20251011160335.50578-8-kieran.bingham@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251011160335.50578-8-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36208,"web_url":"https://patchwork.libcamera.org/comment/36208/","msgid":"<176027891999.935713.3652913500427368138@ping.linuxembedded.co.uk>","date":"2025-10-12T14:21:59","subject":"Re: [RFC PATCH 7/7] ipa: softipa: Confine black level configuration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-10-11 20:01:41)\n> Hi\n> \n> \n> 2025. 10. 11. 18:03 keltezéssel, Kieran Bingham írta:\n> > Move the black level handling entirely into the blc module.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/simple/algorithms/blc.cpp | 11 +++++++++++\n> > src/ipa/simple/soft_simple.cpp | 10 ----------\n> > 2 files changed, 11 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> > index 370385afc683..060006d350ee 100644\n> > --- a/src/ipa/simple/algorithms/blc.cpp\n> > +++ b/src/ipa/simple/algorithms/blc.cpp\n> > @@ -40,10 +40,21 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n> > int BlackLevel::configure(IPAContext &context,\n> > [[maybe_unused]] const IPAConfigInfo &configInfo)\n> > {\n> > + /*\n> > + * The black level from camHelper_ is a 16 bit value, software ISP\n> > + * works with 8 bit pixel values, both regardless of the actual\n> > + * sensor pixel width. Hence we obtain the pixel-based black value\n> > + * by dividing the value from the helper by 256.\n> > + */\n> > + context.configuration.black.level =\n> > + context.camHelper->blackLevel().value() / 256;\n> > +\n> \n> This no longer checks if `blackLevel()` returns an empty optional or not.\n> It also does not check if `!!context.camHelper`. Is that intentional?\n\nEeep. Good spot. It is intentional when this patch is tied with the\nother patches in the series I'm working on where I bring in the\nassumption that CameraHelpers are mandatory (to bring in the full\nAgcMeanLuminance and DelayedControls timings) ... but indeed it would\nbreak here, so it can't be merged on it's own.\n\nBut one of my main RFC curiosities is if it's still reasonable to put\nthe CamHelpers into the IPAContext to make it easier to get access to\nthings deeper in the algorithms, or maybe we need to move more \n\n> Also maybe it makes sense to move `context.configuration.black` into the\n> `BlackLevel` class since it is not used outside.\n\nYes, excellent spot. That can already be done.\n\nMaybe that and bringing back the checks on having a camHelper means we\ncan tidy this up already and keep current behaviour.\n\n> Regards,\n> Barnabás Pőcze\n\n\nThanks\n--\nKieran\n\n\n> > if (definedLevel_.has_value())\n> > context.configuration.black.level = definedLevel_;\n> > +\n> > context.activeState.blc.level =\n> > context.configuration.black.level.value_or(16);\n> > +\n> > return 0;\n> > }\n> > \n> > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> > index 68ddf71e9f24..caae2c586e9b 100644\n> > --- a/src/ipa/simple/soft_simple.cpp\n> > +++ b/src/ipa/simple/soft_simple.cpp\n> > @@ -227,16 +227,6 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo,\n> > (context_.configuration.agc.againMax -\n> > context_.configuration.agc.againMin) /\n> > 100.0;\n> > - if (context_.camHelper->blackLevel().has_value()) {\n> > - /*\n> > - * The black level from camHelper_ is a 16 bit value, software ISP\n> > - * works with 8 bit pixel values, both regardless of the actual\n> > - * sensor pixel width. Hence we obtain the pixel-based black value\n> > - * by dividing the value from the helper by 256.\n> > - */\n> > - context_.configuration.black.level =\n> > - context_.camHelper->blackLevel().value() / 256;\n> > - }\n> > } else {\n> > /*\n> > * The camera sensor gain (g) is usually not equal to the value written\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 8DF73BE080\n\tfor ;\n\tSun, 12 Oct 2025 14:22:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3EABF60456;\n\tSun, 12 Oct 2025 16:22:04 +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 6B3DC60448\n\tfor ;\n\tSun, 12 Oct 2025 16:22:02 +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 724A529A;\n\tSun, 12 Oct 2025 16:20:25 +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=\"fN+6p9te\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760278825;\n\tbh=i2dDFvpLjhiRiielC4wJlQHa8ve/M8DjZe4TIHK/fxA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=fN+6p9teB5Fxlhov9aGETBkgPGnR2rutBv7Yl3VQqQy1+IyB/wS3cKt5ZqKl55QRS\n\t2en6tJppPiFlnW/CR0SBcNG98/R/FVKqcQRz7c6iXLc1hmMLyiVGK7E9henTfpsAsS\n\tJgrGgORZWcsu/BWuL0jhZgGFTy8SYXw1490Uxs5E=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<32827e94-09fe-484d-a2c3-676431ba1901@ideasonboard.com>","References":"<20251011160335.50578-1-kieran.bingham@ideasonboard.com>\n\t<20251011160335.50578-8-kieran.bingham@ideasonboard.com>\n\t<32827e94-09fe-484d-a2c3-676431ba1901@ideasonboard.com>","Subject":"Re: [RFC PATCH 7/7] ipa: softipa: Confine black level configuration","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera devel ","Date":"Sun, 12 Oct 2025 15:21:59 +0100","Message-ID":"<176027891999.935713.3652913500427368138@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]