[{"id":39219,"web_url":"https://patchwork.libcamera.org/comment/39219/","msgid":"<178211649173.1686300.8921906901870435192@ping.linuxembedded.co.uk>","date":"2026-06-22T08:21:31","subject":"Re: [PATCH 3/7] ipa: simple: Limit the black level value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2026-06-21 00:00:30)\n> From: Milan Zamazal \n> \n> The black level value is passed to image processing, where it's used in\n> pixel value computations. To avoid troubles with weird black level\n> values (which are mostly theoretical but we should be still safe against\n> e.g. crazy values in testing tuning files) like division by zero, let's\n> make sure the black level passed to the image processing is lower than\n> the maximum pixel value.\n> \n> Signed-off-by: Milan Zamazal \n> Reviewed-by: Bryan O'Donoghue \n\nAs this one is from Milan, I guess I can also throw my thoughts in here.\n\nClamping to make sure we never have a black level of 1.0/1.0 is probably\n'sane' to make sure we never hit a /0 but it's very unlikely, and would\nsimply be a user error anyway.\n\nI wonder if there is a more 'sane' upper limit for black level at like\n10% (0.1) ... but if I ever try to set a limit I'm sure someone will say\n\"Oh this one is higher.\" But there must be some sane legitimate range\nhere.\n\nAnyway, that could be later if it's ever an issue so:\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/simple/algorithms/blc.cpp | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index 677be56ed6699ae8aaa8a786ab16f7299a82eb46..058cb372620134cb15a0655d3553b3b767b47f0e 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2024-2025, Red Hat Inc.\n> + * Copyright (C) 2024-2026, Red Hat Inc.\n> *\n> * Black level handling\n> */\n> @@ -52,8 +52,9 @@ void BlackLevel::prepare(IPAContext &context,\n> [[maybe_unused]] IPAFrameContext &frameContext,\n> DebayerParams *params)\n> {\n> - /* Latch the blacklevel gain so GPUISP can apply. */\n> - params->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n> + /* Make sure the black level is sane, i.e. below maximum pixel value. */\n> + params->blackLevel = RGB(context.activeState.blc.level / 255.0f)\n> + .min(0.99);\n> }\n> \n> void BlackLevel::process(IPAContext &context,\n> \n> -- \n> 2.53.0\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 9BAC4BF415\n\tfor ;\n\tMon, 22 Jun 2026 08:21:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DF1265708;\n\tMon, 22 Jun 2026 10:21:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CE69A623CC\n\tfor ;\n\tMon, 22 Jun 2026 10:21:34 +0200 (CEST)","from monstersaurus.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 E8538227;\n\tMon, 22 Jun 2026 10:20: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=\"sJsYbPVl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1782116457;\n\tbh=ejasAd556doG7GuYwm1cJPEHYSZCTw1YsJRT6OY5m3g=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sJsYbPVlpM1BP21947UDGyrWcoKqxeImEFks/40OTQvvCfihBVGvlkEvDsprCmJG1\n\tNUVP2QBa/PmsYtZ4M1IxyeHIJVMIfhF62peLfHb/zHh3GREtM513GEuYJpy8ccy+y4\n\tf6qcQ97xruyB2FmocX1OOa+nUoREoBe9pxgNPwp8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260621-kbingham-awb-saturation-v1-3-b91ea59c6cfb@ideasonboard.com>","References":"<20260621-kbingham-awb-saturation-v1-0-b91ea59c6cfb@ideasonboard.com>\n\t<20260621-kbingham-awb-saturation-v1-3-b91ea59c6cfb@ideasonboard.com>","Subject":"Re: [PATCH 3/7] ipa: simple: Limit the black level value","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tBryan O'Donoghue ","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 22 Jun 2026 09:21:31 +0100","Message-ID":"<178211649173.1686300.8921906901870435192@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\" "}},{"id":39228,"web_url":"https://patchwork.libcamera.org/comment/39228/","msgid":"<20260622092346.GO3552167@killaraus.ideasonboard.com>","date":"2026-06-22T09:23:46","subject":"Re: [PATCH 3/7] ipa: simple: Limit the black level value","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 22, 2026 at 09:21:31AM +0100, Kieran Bingham wrote:\n> Quoting Kieran Bingham (2026-06-21 00:00:30)\n> > From: Milan Zamazal \n> > \n> > The black level value is passed to image processing, where it's used in\n> > pixel value computations. To avoid troubles with weird black level\n> > values (which are mostly theoretical but we should be still safe against\n> > e.g. crazy values in testing tuning files) like division by zero, let's\n> > make sure the black level passed to the image processing is lower than\n> > the maximum pixel value.\n> > \n> > Signed-off-by: Milan Zamazal \n> > Reviewed-by: Bryan O'Donoghue \n> \n> As this one is from Milan, I guess I can also throw my thoughts in here.\n> \n> Clamping to make sure we never have a black level of 1.0/1.0 is probably\n> 'sane' to make sure we never hit a /0 but it's very unlikely, and would\n> simply be a user error anyway.\n\nI would have a preference for clamping at the location where\ncontext.activeState.blc.level is set (or where other variables that are\nused to compute that value are set). If the value comes from the tuning\nfile, we should error out. If it comes from a sensor helper, we should\nerror out too.\n\n> I wonder if there is a more 'sane' upper limit for black level at like\n> 10% (0.1) ... but if I ever try to set a limit I'm sure someone will say\n> \"Oh this one is higher.\" But there must be some sane legitimate range\n> here.\n> \n> Anyway, that could be later if it's ever an issue so:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/simple/algorithms/blc.cpp | 7 ++++---\n> > 1 file changed, 4 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> > index 677be56ed6699ae8aaa8a786ab16f7299a82eb46..058cb372620134cb15a0655d3553b3b767b47f0e 100644\n> > --- a/src/ipa/simple/algorithms/blc.cpp\n> > +++ b/src/ipa/simple/algorithms/blc.cpp\n> > @@ -1,6 +1,6 @@\n> > /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > /*\n> > - * Copyright (C) 2024-2025, Red Hat Inc.\n> > + * Copyright (C) 2024-2026, Red Hat Inc.\n> > *\n> > * Black level handling\n> > */\n> > @@ -52,8 +52,9 @@ void BlackLevel::prepare(IPAContext &context,\n> > [[maybe_unused]] IPAFrameContext &frameContext,\n> > DebayerParams *params)\n> > {\n> > - /* Latch the blacklevel gain so GPUISP can apply. */\n> > - params->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n> > + /* Make sure the black level is sane, i.e. below maximum pixel value. */\n> > + params->blackLevel = RGB(context.activeState.blc.level / 255.0f)\n> > + .min(0.99);\n> > }\n> > \n> > void BlackLevel::process(IPAContext &context,\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 08F64BF415\n\tfor ;\n\tMon, 22 Jun 2026 09:23:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E06C6571B;\n\tMon, 22 Jun 2026 11:23:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F7576570B\n\tfor ;\n\tMon, 22 Jun 2026 11:23:48 +0200 (CEST)","from killaraus.ideasonboard.com\n\t(2001-14ba-70f3-e800--a06.rev.dnainternet.fi\n\t[IPv6:2001:14ba:70f3:e800::a06])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 085237CA;\n\tMon, 22 Jun 2026 11:23: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=\"FSkkaJOg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1782120190;\n\tbh=NspSJpvOBTL2SqqEjZu1EU+dRTbho527zb8+uX+9hi8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FSkkaJOgQG64/humIQ4QSRhweWdY3ghbFfZzWFAUVtysftEKqD859bc2qj7ZuVKXQ\n\tlGCK+OmS5u5Oil1HphQY7HXgFM2AQrsB0T0KUait1aJcCdjmYLh/MyQS78t80HEfLf\n\te2h9q1I5M1SyJL+plPgDp18WolOF0yrQujE55nqg=","Date":"Mon, 22 Jun 2026 12:23:46 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tBryan O'Donoghue ","Subject":"Re: [PATCH 3/7] ipa: simple: Limit the black level value","Message-ID":"<20260622092346.GO3552167@killaraus.ideasonboard.com>","References":"<20260621-kbingham-awb-saturation-v1-0-b91ea59c6cfb@ideasonboard.com>\n\t<20260621-kbingham-awb-saturation-v1-3-b91ea59c6cfb@ideasonboard.com>\n\t<178211649173.1686300.8921906901870435192@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<178211649173.1686300.8921906901870435192@ping.linuxembedded.co.uk>","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":39253,"web_url":"https://patchwork.libcamera.org/comment/39253/","msgid":"<178213431308.1686300.15797400321799120406@ping.linuxembedded.co.uk>","date":"2026-06-22T13:18:33","subject":"Re: [PATCH 3/7] ipa: simple: Limit the black level value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2026-06-22 10:23:46)\n> On Mon, Jun 22, 2026 at 09:21:31AM +0100, Kieran Bingham wrote:\n> > Quoting Kieran Bingham (2026-06-21 00:00:30)\n> > > From: Milan Zamazal \n> > > \n> > > The black level value is passed to image processing, where it's used in\n> > > pixel value computations. To avoid troubles with weird black level\n> > > values (which are mostly theoretical but we should be still safe against\n> > > e.g. crazy values in testing tuning files) like division by zero, let's\n> > > make sure the black level passed to the image processing is lower than\n> > > the maximum pixel value.\n> > > \n> > > Signed-off-by: Milan Zamazal \n> > > Reviewed-by: Bryan O'Donoghue \n> > \n> > As this one is from Milan, I guess I can also throw my thoughts in here.\n> > \n> > Clamping to make sure we never have a black level of 1.0/1.0 is probably\n> > 'sane' to make sure we never hit a /0 but it's very unlikely, and would\n> > simply be a user error anyway.\n> \n> I would have a preference for clamping at the location where\n> context.activeState.blc.level is set (or where other variables that are\n> used to compute that value are set). If the value comes from the tuning\n> file, we should error out. If it comes from a sensor helper, we should\n> error out too.\n\nBlack level is read into a uint8_t, (after converting from 16bit to\n8bit) so it can't be bigger than 255, but 255 would make the divide by\nzero occur.\n\nI'm half tempted to just drop this patch, and leave things as they are,\nbut we can add an error check when we read the tuning files. Though I\nmight leave that to potential rework into libipa to use something common\nand not worry about this patch at all. It's only here to guard an edge\ncase on some code that we're about to refactor anyway.\n\n--\nKieran.\n\n\n> \n> > I wonder if there is a more 'sane' upper limit for black level at like\n> > 10% (0.1) ... but if I ever try to set a limit I'm sure someone will say\n> > \"Oh this one is higher.\" But there must be some sane legitimate range\n> > here.\n> > \n> > Anyway, that could be later if it's ever an issue so:\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/ipa/simple/algorithms/blc.cpp | 7 ++++---\n> > > 1 file changed, 4 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> > > index 677be56ed6699ae8aaa8a786ab16f7299a82eb46..058cb372620134cb15a0655d3553b3b767b47f0e 100644\n> > > --- a/src/ipa/simple/algorithms/blc.cpp\n> > > +++ b/src/ipa/simple/algorithms/blc.cpp\n> > > @@ -1,6 +1,6 @@\n> > > /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > /*\n> > > - * Copyright (C) 2024-2025, Red Hat Inc.\n> > > + * Copyright (C) 2024-2026, Red Hat Inc.\n> > > *\n> > > * Black level handling\n> > > */\n> > > @@ -52,8 +52,9 @@ void BlackLevel::prepare(IPAContext &context,\n> > > [[maybe_unused]] IPAFrameContext &frameContext,\n> > > DebayerParams *params)\n> > > {\n> > > - /* Latch the blacklevel gain so GPUISP can apply. */\n> > > - params->blackLevel = RGB(context.activeState.blc.level / 255.0f);\n> > > + /* Make sure the black level is sane, i.e. below maximum pixel value. */\n> > > + params->blackLevel = RGB(context.activeState.blc.level / 255.0f)\n> > > + .min(0.99);\n> > > }\n> > > \n> > > void BlackLevel::process(IPAContext &context,\n> > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 E4A54BF415\n\tfor ;\n\tMon, 22 Jun 2026 13:18:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EACFE6573A;\n\tMon, 22 Jun 2026 15:18:37 +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 0DFB3656D3\n\tfor ;\n\tMon, 22 Jun 2026 15:18:36 +0200 (CEST)","from monstersaurus.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 05DAF7CA;\n\tMon, 22 Jun 2026 15:17: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=\"KdX3yg+l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1782134278;\n\tbh=IEA3jOgVqvJVG9WIajzyr3LdbBgwt9arghluH21Ygos=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=KdX3yg+l8vvCnKEAbx1vzjXxmpP4CDlGQq5bUNLNdTvZcO6drYARw7KXuWJW13YOx\n\twZ1SQiMM8Wy2KxSZcRSA03m7Q7mFTzOK11J6ZZvZi8x9Pj23A5ujg895Nj4Fjy3dwR\n\tkcI9dN+9oPrYGb8J0lslgZySYhEdfoA+ONme1JiM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260622092346.GO3552167@killaraus.ideasonboard.com>","References":"<20260621-kbingham-awb-saturation-v1-0-b91ea59c6cfb@ideasonboard.com>\n\t<20260621-kbingham-awb-saturation-v1-3-b91ea59c6cfb@ideasonboard.com>\n\t<178211649173.1686300.8921906901870435192@ping.linuxembedded.co.uk>\n\t<20260622092346.GO3552167@killaraus.ideasonboard.com>","Subject":"Re: [PATCH 3/7] ipa: simple: Limit the black level value","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Milan Zamazal ,\n\tBryan O'Donoghue ","To":"Laurent Pinchart ","Date":"Mon, 22 Jun 2026 14:18:33 +0100","Message-ID":"<178213431308.1686300.15797400321799120406@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\" "}}]