| Message ID | 20220908014200.28728-33-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:42:00) > The red and blue gains are computed by dividing the green mean by the > red and blue means respectively. An offset of 1 is added to the dividers > to avoid divisions by zero. This introduces a bias in the gain values. > Fix it by clamping the divisors to a minimum of 1.0 instead of adding an > offset. > Sounds resonable. That 'hardcoded green gain of 0' is there, which doesn't make sense to me, but I've questioned that in an earlier patch. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 404ad66e6953..fe1ba09a99a2 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -280,10 +280,11 @@ void Awb::process(IPAContext &context, > > /* > * Estimate the red and blue gains to apply in a grey world. The green > - * gain is hardcoded to 0. > + * gain is hardcoded to 0. Avoid divisions by zero by clamping the > + * divisor to a minimum value of 1.0. > */ > - double redGain = greenMean / (redMean + 1); > - double blueGain = greenMean / (blueMean + 1); > + double redGain = greenMean / std::max(redMean, 1.0); > + double blueGain = greenMean / std::max(blueMean, 1.0); > > /* > * Clamp the gain values to the hardware, which expresses gains as Q2.8 > -- > Regards, > > Laurent Pinchart >
On Thu, Sep 08, 2022 at 04:42:00AM +0300, Laurent Pinchart via libcamera-devel wrote: > The red and blue gains are computed by dividing the green mean by the > red and blue means respectively. An offset of 1 is added to the dividers > to avoid divisions by zero. This introduces a bias in the gain values. > Fix it by clamping the divisors to a minimum of 1.0 instead of adding an > offset. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 404ad66e6953..fe1ba09a99a2 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -280,10 +280,11 @@ void Awb::process(IPAContext &context, > > /* > * Estimate the red and blue gains to apply in a grey world. The green > - * gain is hardcoded to 0. > + * gain is hardcoded to 0. Avoid divisions by zero by clamping the > + * divisor to a minimum value of 1.0. > */ > - double redGain = greenMean / (redMean + 1); > - double blueGain = greenMean / (blueMean + 1); > + double redGain = greenMean / std::max(redMean, 1.0); > + double blueGain = greenMean / std::max(blueMean, 1.0); Dumb question, do we care about divisors in the ]0.0, 1.0[ range ? > > /* > * Clamp the gain values to the hardware, which expresses gains as Q2.8 > -- > Regards, > > Laurent Pinchart >
On Wed, Sep 21, 2022 at 12:48:35AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:42:00) > > The red and blue gains are computed by dividing the green mean by the > > red and blue means respectively. An offset of 1 is added to the dividers > > to avoid divisions by zero. This introduces a bias in the gain values. > > Fix it by clamping the divisors to a minimum of 1.0 instead of adding an > > offset. > > Sounds resonable. That 'hardcoded green gain of 0' is there, which > doesn't make sense to me, but I've questioned that in an earlier patch. I meant 1.0, not 0. I've fixed it. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 404ad66e6953..fe1ba09a99a2 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -280,10 +280,11 @@ void Awb::process(IPAContext &context, > > > > /* > > * Estimate the red and blue gains to apply in a grey world. The green > > - * gain is hardcoded to 0. > > + * gain is hardcoded to 0. Avoid divisions by zero by clamping the > > + * divisor to a minimum value of 1.0. > > */ > > - double redGain = greenMean / (redMean + 1); > > - double blueGain = greenMean / (blueMean + 1); > > + double redGain = greenMean / std::max(redMean, 1.0); > > + double blueGain = greenMean / std::max(blueMean, 1.0); > > > > /* > > * Clamp the gain values to the hardware, which expresses gains as Q2.8
Hi Jacopo, On Thu, Sep 22, 2022 at 12:51:48PM +0200, Jacopo Mondi wrote: > On Thu, Sep 08, 2022 at 04:42:00AM +0300, Laurent Pinchart via libcamera-devel wrote: > > The red and blue gains are computed by dividing the green mean by the > > red and blue means respectively. An offset of 1 is added to the dividers > > to avoid divisions by zero. This introduces a bias in the gain values. > > Fix it by clamping the divisors to a minimum of 1.0 instead of adding an > > offset. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 404ad66e6953..fe1ba09a99a2 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -280,10 +280,11 @@ void Awb::process(IPAContext &context, > > > > /* > > * Estimate the red and blue gains to apply in a grey world. The green > > - * gain is hardcoded to 0. > > + * gain is hardcoded to 0. Avoid divisions by zero by clamping the > > + * divisor to a minimum value of 1.0. > > */ > > - double redGain = greenMean / (redMean + 1); > > - double blueGain = greenMean / (blueMean + 1); > > + double redGain = greenMean / std::max(redMean, 1.0); > > + double blueGain = greenMean / std::max(blueMean, 1.0); > > Dumb question, do we care about divisors in the ]0.0, 1.0[ range ? Given that the range for the mean values is [0, 255] and that we freeze AWB when the means are below 2.0, we don't. I wrote this patch before adding the thresholds, the std::max isn't strictly needed anymore, but I feel safer keeping it in case we change the threshold mechanism later. > > > > /* > > * Clamp the gain values to the hardware, which expresses gains as Q2.8
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 404ad66e6953..fe1ba09a99a2 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -280,10 +280,11 @@ void Awb::process(IPAContext &context, /* * Estimate the red and blue gains to apply in a grey world. The green - * gain is hardcoded to 0. + * gain is hardcoded to 0. Avoid divisions by zero by clamping the + * divisor to a minimum value of 1.0. */ - double redGain = greenMean / (redMean + 1); - double blueGain = greenMean / (blueMean + 1); + double redGain = greenMean / std::max(redMean, 1.0); + double blueGain = greenMean / std::max(blueMean, 1.0); /* * Clamp the gain values to the hardware, which expresses gains as Q2.8
The red and blue gains are computed by dividing the green mean by the red and blue means respectively. An offset of 1 is added to the dividers to avoid divisions by zero. This introduces a bias in the gain values. Fix it by clamping the divisors to a minimum of 1.0 instead of adding an offset. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)