| Message ID | 20220908014200.28728-30-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:57) > Due to hardware rounding errors in the YCbCr means, the calculated RGB > means may be negative. This would lead to negative gains, messing up > calculation. Prevent this by clamping the means to positive values. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index ed91e9277a16..de54c4d24650 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -242,6 +242,16 @@ void Awb::process(IPAContext &context, > redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; > greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; > blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; > + > + /* > + * Due to hardware rounding errors in the YCbCr means, the > + * calculated RGB means may be negative. This would lead to > + * negative gains, messing up calculation. Prevent this by > + * clamping the means to positive values. > + */ > + redMean = std::max(redMean, 0.0); > + greenMean = std::max(greenMean, 0.0); > + blueMean = std::max(blueMean, 0.0); We don't need to clamp to a value greater than zero for any reason here do we? I was trying to conjour up in my head if perhaps this needs to clamp to a max with 1.0 for any reason, but these are not the gains - but the values used later I believe. So I think it sounds fine... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > /* > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Thu, Sep 08, 2022 at 04:41:57AM +0300, Laurent Pinchart via libcamera-devel wrote: > Due to hardware rounding errors in the YCbCr means, the calculated RGB Is it due to rounding or simply because the values of yMean, cbMean and crMean double yMean = awb->awb_mean[0].mean_y_or_g; double cbMean = awb->awb_mean[0].mean_cb_or_b; double crMean = awb->awb_mean[0].mean_cr_or_r; /* * Convert from YCbCr to RGB. * The hardware uses the following formulas: * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B * * The inverse matrix is thus: * [[1,1636, -0,0623, 1,6008] * [1,1636, -0,4045, -0,7949] * [1,1636, 1,9912, -0,0250]] */ yMean -= 16; cbMean -= 128; crMean -= 128; are smaller than the above offsets ? I guess this can happen when the image is particularly dark, right ? Anyway, this certainly fixes an issue Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > means may be negative. This would lead to negative gains, messing up > calculation. Prevent this by clamping the means to positive values. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index ed91e9277a16..de54c4d24650 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -242,6 +242,16 @@ void Awb::process(IPAContext &context, > redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; > greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; > blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; > + > + /* > + * Due to hardware rounding errors in the YCbCr means, the > + * calculated RGB means may be negative. This would lead to > + * negative gains, messing up calculation. Prevent this by > + * clamping the means to positive values. > + */ > + redMean = std::max(redMean, 0.0); > + greenMean = std::max(greenMean, 0.0); > + blueMean = std::max(blueMean, 0.0); > } > > /* > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Thu, Sep 22, 2022 at 12:42:48PM +0200, Jacopo Mondi wrote: > Hi Laurent > > On Thu, Sep 08, 2022 at 04:41:57AM +0300, Laurent Pinchart via libcamera-devel wrote: > > Due to hardware rounding errors in the YCbCr means, the calculated RGB > > Is it due to rounding or simply because > the values of yMean, cbMean and crMean > > double yMean = awb->awb_mean[0].mean_y_or_g; > double cbMean = awb->awb_mean[0].mean_cb_or_b; > double crMean = awb->awb_mean[0].mean_cr_or_r; > > /* > * Convert from YCbCr to RGB. > * The hardware uses the following formulas: > * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B > * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B > * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B > * > * The inverse matrix is thus: > * [[1,1636, -0,0623, 1,6008] > * [1,1636, -0,4045, -0,7949] > * [1,1636, 1,9912, -0,0250]] > */ > yMean -= 16; > cbMean -= 128; > crMean -= 128; > > are smaller than the above offsets ? I guess this can happen when the > image is particularly dark, right ? When the image is fully black yMean should be equal to 16 and cbMean and crMean to 128. I haven't seen yMean smaller than 16, but cbMean and crMean were often 127. This leads to RGB means smaller than 0. The only plausible cause I found was hardware rounding errors. > Anyway, this certainly fixes an issue > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > means may be negative. This would lead to negative gains, messing up > > calculation. Prevent this by clamping the means to positive values. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index ed91e9277a16..de54c4d24650 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -242,6 +242,16 @@ void Awb::process(IPAContext &context, > > redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; > > greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; > > blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; > > + > > + /* > > + * Due to hardware rounding errors in the YCbCr means, the > > + * calculated RGB means may be negative. This would lead to > > + * negative gains, messing up calculation. Prevent this by > > + * clamping the means to positive values. > > + */ > > + redMean = std::max(redMean, 0.0); > > + greenMean = std::max(greenMean, 0.0); > > + blueMean = std::max(blueMean, 0.0); > > } > > > > /*
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index ed91e9277a16..de54c4d24650 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -242,6 +242,16 @@ void Awb::process(IPAContext &context, redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean; greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean; blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean; + + /* + * Due to hardware rounding errors in the YCbCr means, the + * calculated RGB means may be negative. This would lead to + * negative gains, messing up calculation. Prevent this by + * clamping the means to positive values. + */ + redMean = std::max(redMean, 0.0); + greenMean = std::max(greenMean, 0.0); + blueMean = std::max(blueMean, 0.0); } /*
Due to hardware rounding errors in the YCbCr means, the calculated RGB means may be negative. This would lead to negative gains, messing up calculation. Prevent this by clamping the means to positive values. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+)