[v2,17/17] ipa: rkisp1: awb: Avoid division by zero

22995 diff mbox series

As the gains can also be specified manually, the regulation can run into
numeric instabilities by dividing by near zero. Mitigate that by
applying a small minium value.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---

Changes in v2:
- Collected tag
---
 src/ipa/rkisp1/algorithms/awb.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi Stefan,

Thank you for the patch.

On Wed, Mar 19, 2025 at 05:11:22PM +0100, Stefan Klug wrote:
> As the gains can also be specified manually, the regulation can run into
> numeric instabilities by dividing by near zero. Mitigate that by
> applying a small minium value.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Collected tag
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index e8b04d03748d..eb4545439ae3 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -421,9 +421,9 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
>  	/*
>  	 * The ISP computes the AWB means after applying the colour gains,
>  	 * divide by the gains that were used to get the raw means from the
> -	 * sensor.
> +	 * sensor. Apply a minimum value to avoid divisions by near-zero.

Shouldn't we instead prevent the user from setting zero (or near-zero)
gains ? 

>  	 */
> -	rgbMeans /= frameContext.awb.gains;
> +	rgbMeans /= frameContext.awb.gains.max(0.01);
>  
>  	return rgbMeans;
>  }

Hi Laurent,

Thank you for the review. 

On Tue, Apr 01, 2025 at 04:21:31AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wed, Mar 19, 2025 at 05:11:22PM +0100, Stefan Klug wrote:
> > As the gains can also be specified manually, the regulation can run into
> > numeric instabilities by dividing by near zero. Mitigate that by
> > applying a small minium value.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Collected tag
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index e8b04d03748d..eb4545439ae3 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -421,9 +421,9 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
> >  	/*
> >  	 * The ISP computes the AWB means after applying the colour gains,
> >  	 * divide by the gains that were used to get the raw means from the
> > -	 * sensor.
> > +	 * sensor. Apply a minimum value to avoid divisions by near-zero.
> 
> Shouldn't we instead prevent the user from setting zero (or near-zero)
> gains ? 

That would also work, but afaik we have no mechanism to check if
controls adhere to the range provided in the ControlInfo. And there are
multiple code paths feeding into framContext.awb.gains. So I think it
is easier to move the check to where it might do harm instead of
ensuring every code path has similar checks.

Best regards,
Stefan

> 
> >  	 */
> > -	rgbMeans /= frameContext.awb.gains;
> > +	rgbMeans /= frameContext.awb.gains.max(0.01);
> >  
> >  	return rgbMeans;
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

On Wed, Apr 02, 2025 at 05:26:55PM +0200, Stefan Klug wrote:
> On Tue, Apr 01, 2025 at 04:21:31AM +0300, Laurent Pinchart wrote:
> > On Wed, Mar 19, 2025 at 05:11:22PM +0100, Stefan Klug wrote:
> > > As the gains can also be specified manually, the regulation can run into
> > > numeric instabilities by dividing by near zero. Mitigate that by
> > > applying a small minium value.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - Collected tag
> > > ---
> > >  src/ipa/rkisp1/algorithms/awb.cpp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > index e8b04d03748d..eb4545439ae3 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > @@ -421,9 +421,9 @@ RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rk
> > >  	/*
> > >  	 * The ISP computes the AWB means after applying the colour gains,
> > >  	 * divide by the gains that were used to get the raw means from the
> > > -	 * sensor.
> > > +	 * sensor. Apply a minimum value to avoid divisions by near-zero.
> > 
> > Shouldn't we instead prevent the user from setting zero (or near-zero)
> > gains ? 
> 
> That would also work, but afaik we have no mechanism to check if
> controls adhere to the range provided in the ControlInfo. And there are
> multiple code paths feeding into framContext.awb.gains. So I think it
> is easier to move the check to where it might do harm instead of
> ensuring every code path has similar checks.

As a short term fix that's fine with me, but I think we should still
address the overall issue, and clamp/adjust control values at queue
request time. That's the entry point for controls, if we handle it
there, it should be safe everywhere else in the IPA module.

> > >  	 */
> > > -	rgbMeans /= frameContext.awb.gains;
> > > +	rgbMeans /= frameContext.awb.gains.max(0.01);
> > >  
> > >  	return rgbMeans;
> > >  }