[3/7] ipa: simple: Limit the black level value
diff mbox series

Message ID 20260621-kbingham-awb-saturation-v1-3-b91ea59c6cfb@ideasonboard.com
State Accepted
Headers show
Series
  • softisp: Fix Saturation and Black level handling
Related show

Commit Message

Kieran Bingham June 20, 2026, 11 p.m. UTC 
From: Milan Zamazal <mzamazal@redhat.com>

The black level value is passed to image processing, where it's used in
pixel value computations.  To avoid troubles with weird black level
values (which are mostly theoretical but we should be still safe against
e.g. crazy values in testing tuning files) like division by zero, let's
make sure the black level passed to the image processing is lower than
the maximum pixel value.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/simple/algorithms/blc.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Kieran Bingham June 22, 2026, 8:21 a.m. UTC | #1 
Quoting Kieran Bingham (2026-06-21 00:00:30)
> From: Milan Zamazal <mzamazal@redhat.com>
> 
> The black level value is passed to image processing, where it's used in
> pixel value computations.  To avoid troubles with weird black level
> values (which are mostly theoretical but we should be still safe against
> e.g. crazy values in testing tuning files) like division by zero, let's
> make sure the black level passed to the image processing is lower than
> the maximum pixel value.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

As this one is from Milan, I guess I can also throw my thoughts in here.

Clamping to make sure we never have a black level of 1.0/1.0 is probably
'sane' to make sure we never hit a /0 but it's very unlikely, and would
simply be a user error anyway.

I wonder if there is a more 'sane' upper limit for black level at like
10% (0.1) ... but if I ever try to set a limit I'm sure someone will say
"Oh this one is higher." But there must be some sane legitimate range
here.

Anyway, that could be later if it's ever an issue so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 677be56ed6699ae8aaa8a786ab16f7299a82eb46..058cb372620134cb15a0655d3553b3b767b47f0e 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  /*
> - * Copyright (C) 2024-2025, Red Hat Inc.
> + * Copyright (C) 2024-2026, Red Hat Inc.
>   *
>   * Black level handling
>   */
> @@ -52,8 +52,9 @@ void BlackLevel::prepare(IPAContext &context,
>                          [[maybe_unused]] IPAFrameContext &frameContext,
>                          DebayerParams *params)
>  {
> -       /* Latch the blacklevel gain so GPUISP can apply. */
> -       params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
> +       /* Make sure the black level is sane, i.e. below maximum pixel value. */
> +       params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f)
> +                                    .min(0.99);
>  }
>  
>  void BlackLevel::process(IPAContext &context,
> 
> -- 
> 2.53.0
>
Laurent Pinchart June 22, 2026, 9:23 a.m. UTC | #2 
On Mon, Jun 22, 2026 at 09:21:31AM +0100, Kieran Bingham wrote:
> Quoting Kieran Bingham (2026-06-21 00:00:30)
> > From: Milan Zamazal <mzamazal@redhat.com>
> > 
> > The black level value is passed to image processing, where it's used in
> > pixel value computations.  To avoid troubles with weird black level
> > values (which are mostly theoretical but we should be still safe against
> > e.g. crazy values in testing tuning files) like division by zero, let's
> > make sure the black level passed to the image processing is lower than
> > the maximum pixel value.
> > 
> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> As this one is from Milan, I guess I can also throw my thoughts in here.
> 
> Clamping to make sure we never have a black level of 1.0/1.0 is probably
> 'sane' to make sure we never hit a /0 but it's very unlikely, and would
> simply be a user error anyway.

I would have a preference for clamping at the location where
context.activeState.blc.level is set (or where other variables that are
used to compute that value are set). If the value comes from the tuning
file, we should error out. If it comes from a sensor helper, we should
error out too.

> I wonder if there is a more 'sane' upper limit for black level at like
> 10% (0.1) ... but if I ever try to set a limit I'm sure someone will say
> "Oh this one is higher." But there must be some sane legitimate range
> here.
> 
> Anyway, that could be later if it's ever an issue so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/simple/algorithms/blc.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> > index 677be56ed6699ae8aaa8a786ab16f7299a82eb46..058cb372620134cb15a0655d3553b3b767b47f0e 100644
> > --- a/src/ipa/simple/algorithms/blc.cpp
> > +++ b/src/ipa/simple/algorithms/blc.cpp
> > @@ -1,6 +1,6 @@
> >  /* SPDX-License-Identifier: LGPL-2.1-or-later */
> >  /*
> > - * Copyright (C) 2024-2025, Red Hat Inc.
> > + * Copyright (C) 2024-2026, Red Hat Inc.
> >   *
> >   * Black level handling
> >   */
> > @@ -52,8 +52,9 @@ void BlackLevel::prepare(IPAContext &context,
> >                          [[maybe_unused]] IPAFrameContext &frameContext,
> >                          DebayerParams *params)
> >  {
> > -       /* Latch the blacklevel gain so GPUISP can apply. */
> > -       params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
> > +       /* Make sure the black level is sane, i.e. below maximum pixel value. */
> > +       params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f)
> > +                                    .min(0.99);
> >  }
> >  
> >  void BlackLevel::process(IPAContext &context,
> >
Kieran Bingham June 22, 2026, 1:18 p.m. UTC | #3 
Quoting Laurent Pinchart (2026-06-22 10:23:46)
> On Mon, Jun 22, 2026 at 09:21:31AM +0100, Kieran Bingham wrote:
> > Quoting Kieran Bingham (2026-06-21 00:00:30)
> > > From: Milan Zamazal <mzamazal@redhat.com>
> > > 
> > > The black level value is passed to image processing, where it's used in
> > > pixel value computations.  To avoid troubles with weird black level
> > > values (which are mostly theoretical but we should be still safe against
> > > e.g. crazy values in testing tuning files) like division by zero, let's
> > > make sure the black level passed to the image processing is lower than
> > > the maximum pixel value.
> > > 
> > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > 
> > As this one is from Milan, I guess I can also throw my thoughts in here.
> > 
> > Clamping to make sure we never have a black level of 1.0/1.0 is probably
> > 'sane' to make sure we never hit a /0 but it's very unlikely, and would
> > simply be a user error anyway.
> 
> I would have a preference for clamping at the location where
> context.activeState.blc.level is set (or where other variables that are
> used to compute that value are set). If the value comes from the tuning
> file, we should error out. If it comes from a sensor helper, we should
> error out too.

Black level is read into a uint8_t, (after converting from 16bit to
8bit) so it can't be bigger than 255, but 255 would make the divide by
zero occur.

I'm half tempted to just drop this patch, and leave things as they are,
but we can add an error check when we read the tuning files. Though I
might leave that to potential rework into libipa to use something common
and not worry about this patch at all. It's only here to guard an edge
case on some code that we're about to refactor anyway.

--
Kieran.


> 
> > I wonder if there is a more 'sane' upper limit for black level at like
> > 10% (0.1) ... but if I ever try to set a limit I'm sure someone will say
> > "Oh this one is higher." But there must be some sane legitimate range
> > here.
> > 
> > Anyway, that could be later if it's ever an issue so:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/ipa/simple/algorithms/blc.cpp | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> > > index 677be56ed6699ae8aaa8a786ab16f7299a82eb46..058cb372620134cb15a0655d3553b3b767b47f0e 100644
> > > --- a/src/ipa/simple/algorithms/blc.cpp
> > > +++ b/src/ipa/simple/algorithms/blc.cpp
> > > @@ -1,6 +1,6 @@
> > >  /* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >  /*
> > > - * Copyright (C) 2024-2025, Red Hat Inc.
> > > + * Copyright (C) 2024-2026, Red Hat Inc.
> > >   *
> > >   * Black level handling
> > >   */
> > > @@ -52,8 +52,9 @@ void BlackLevel::prepare(IPAContext &context,
> > >                          [[maybe_unused]] IPAFrameContext &frameContext,
> > >                          DebayerParams *params)
> > >  {
> > > -       /* Latch the blacklevel gain so GPUISP can apply. */
> > > -       params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
> > > +       /* Make sure the black level is sane, i.e. below maximum pixel value. */
> > > +       params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f)
> > > +                                    .min(0.99);
> > >  }
> > >  
> > >  void BlackLevel::process(IPAContext &context,
> > > 
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
index 677be56ed6699ae8aaa8a786ab16f7299a82eb46..058cb372620134cb15a0655d3553b3b767b47f0e 100644
--- a/src/ipa/simple/algorithms/blc.cpp
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -1,6 +1,6 @@ 
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 /*
- * Copyright (C) 2024-2025, Red Hat Inc.
+ * Copyright (C) 2024-2026, Red Hat Inc.
  *
  * Black level handling
  */
@@ -52,8 +52,9 @@  void BlackLevel::prepare(IPAContext &context,
 			 [[maybe_unused]] IPAFrameContext &frameContext,
 			 DebayerParams *params)
 {
-	/* Latch the blacklevel gain so GPUISP can apply. */
-	params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f);
+	/* Make sure the black level is sane, i.e. below maximum pixel value. */
+	params->blackLevel = RGB<float>(context.activeState.blc.level / 255.0f)
+				     .min(0.99);
 }
 
 void BlackLevel::process(IPAContext &context,