| Message ID | 20220224151113.109858-5-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi JM On 2/24/22 20:41, Jean-Michel Hautbois wrote: > The gain values are coded as u3.13 fixed point values, ie they can not > be more than 8. Clampt the values in order to avoid any off limits value s/Clampt/Clamp > which could make the IPU3 behave weirdly. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index 1dc27fc9..dc25be81 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -353,6 +353,14 @@ void Awb::awbGreyWorld() > > /* Color temperature is not relevant in Grey world but still useful to estimate it :-) */ > asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B); > + > + /* > + * Gain values are unsigned integer value, range 0 to 8 with 13 bit I am not able to decipher "integer value" with "fractional part". The redGain and blueGain are floating valuesright..? > + * fractional part. > + */ > + redGain = std::clamp(redGain, 0.0, 65535.0 / 8192); > + blueGain = std::clamp(blueGain, 0.0, 65535.0 / 8192); > + > asyncResults_.redGain = redGain; > /* Hardcode the green gain to 1.0. */ > asyncResults_.greenGain = 1.0;
Quoting Umang Jain (2022-02-28 17:02:43) > Hi JM > > On 2/24/22 20:41, Jean-Michel Hautbois wrote: > > The gain values are coded as u3.13 fixed point values, ie they can not > > be more than 8. Clampt the values in order to avoid any off limits value > > > s/Clampt/Clamp > > > which could make the IPU3 behave weirdly. > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > > index 1dc27fc9..dc25be81 100644 > > --- a/src/ipa/ipu3/algorithms/awb.cpp > > +++ b/src/ipa/ipu3/algorithms/awb.cpp > > @@ -353,6 +353,14 @@ void Awb::awbGreyWorld() > > > > /* Color temperature is not relevant in Grey world but still useful to estimate it :-) */ > > asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B); > > + > > + /* > > + * Gain values are unsigned integer value, range 0 to 8 with 13 bit Techincally, we can't store '8' in this representation though can we. It looks like the max is something like 7.99987792969 ... so maybe this should be range 0 to < 8 ? Unless there's a better notation to say up to but not including 8? > I am not able to decipher "integer value" with "fractional part". The > redGain and blueGain are floating valuesright..? Perhaps stating that they are stored as unsigned integer 3.13 fixed point might be more clear. I would love to see a proper fixed-point class representing that, but that could be a big task, so I think this is fine until we get something like that. With Clampt/Clamp, and this updated if needed/desired. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + * fractional part. > > + */ > > + redGain = std::clamp(redGain, 0.0, 65535.0 / 8192); > > + blueGain = std::clamp(blueGain, 0.0, 65535.0 / 8192); > > + > > asyncResults_.redGain = redGain; > > /* Hardcode the green gain to 1.0. */ > > asyncResults_.greenGain = 1.0;
On Wed, Mar 02, 2022 at 12:37:07PM +0000, Kieran Bingham wrote: > Quoting Umang Jain (2022-02-28 17:02:43) > > Hi JM > > > > On 2/24/22 20:41, Jean-Michel Hautbois wrote: > > > The gain values are coded as u3.13 fixed point values, ie they can not > > > be more than 8. Clampt the values in order to avoid any off limits value > > > > > > s/Clampt/Clamp > > > > > which could make the IPU3 behave weirdly. > > > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > --- > > > src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > > > index 1dc27fc9..dc25be81 100644 > > > --- a/src/ipa/ipu3/algorithms/awb.cpp > > > +++ b/src/ipa/ipu3/algorithms/awb.cpp > > > @@ -353,6 +353,14 @@ void Awb::awbGreyWorld() > > > > > > /* Color temperature is not relevant in Grey world but still useful to estimate it :-) */ > > > asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B); > > > + > > > + /* > > > + * Gain values are unsigned integer value, range 0 to 8 with 13 bit > > Techincally, we can't store '8' in this representation though can we. > It looks like the max is something like 7.99987792969 ... so maybe this > should be range 0 to < 8 ? > > Unless there's a better notation to say up to but not including 8? [0, 8[ and [0, 8) are commonly used for this purpose. > > I am not able to decipher "integer value" with "fractional part". The > > redGain and blueGain are floating valuesright..? > > Perhaps stating that they are stored as unsigned integer 3.13 fixed > point might be more clear. > > I would love to see a proper fixed-point class representing that, but > that could be a big task, so I think this is fine until we get something > like that. > > With Clampt/Clamp, and this updated if needed/desired. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > + * fractional part. > > > + */ > > > + redGain = std::clamp(redGain, 0.0, 65535.0 / 8192); > > > + blueGain = std::clamp(blueGain, 0.0, 65535.0 / 8192); > > > + > > > asyncResults_.redGain = redGain; > > > /* Hardcode the green gain to 1.0. */ > > > asyncResults_.greenGain = 1.0;
Hi Umang, Kieran and Laurent, On 02/03/2022 13:43, Laurent Pinchart wrote: > On Wed, Mar 02, 2022 at 12:37:07PM +0000, Kieran Bingham wrote: >> Quoting Umang Jain (2022-02-28 17:02:43) >>> Hi JM >>> >>> On 2/24/22 20:41, Jean-Michel Hautbois wrote: >>>> The gain values are coded as u3.13 fixed point values, ie they can not >>>> be more than 8. Clampt the values in order to avoid any off limits value >>> >>> >>> s/Clampt/Clamp >>> >>>> which could make the IPU3 behave weirdly. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>>> index 1dc27fc9..dc25be81 100644 >>>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>>> @@ -353,6 +353,14 @@ void Awb::awbGreyWorld() >>>> >>>> /* Color temperature is not relevant in Grey world but still useful to estimate it :-) */ >>>> asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B); >>>> + >>>> + /* >>>> + * Gain values are unsigned integer value, range 0 to 8 with 13 bit >> >> Techincally, we can't store '8' in this representation though can we. >> It looks like the max is something like 7.99987792969 ... so maybe this >> should be range 0 to < 8 ? >> >> Unless there's a better notation to say up to but not including 8? > > [0, 8[ and [0, 8) are commonly used for this purpose. > Indeed, sorry for this, I will correct it. >>> I am not able to decipher "integer value" with "fractional part". The >>> redGain and blueGain are floating valuesright..? >> >> Perhaps stating that they are stored as unsigned integer 3.13 fixed >> point might be more clear. >> >> I would love to see a proper fixed-point class representing that, but >> that could be a big task, so I think this is fine until we get something >> like that. >> >> With Clampt/Clamp, and this updated if needed/desired. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> >>>> + * fractional part. >>>> + */ >>>> + redGain = std::clamp(redGain, 0.0, 65535.0 / 8192); >>>> + blueGain = std::clamp(blueGain, 0.0, 65535.0 / 8192); >>>> + >>>> asyncResults_.redGain = redGain; >>>> /* Hardcode the green gain to 1.0. */ >>>> asyncResults_.greenGain = 1.0; >
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index 1dc27fc9..dc25be81 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -353,6 +353,14 @@ void Awb::awbGreyWorld() /* Color temperature is not relevant in Grey world but still useful to estimate it :-) */ asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B); + + /* + * Gain values are unsigned integer value, range 0 to 8 with 13 bit + * fractional part. + */ + redGain = std::clamp(redGain, 0.0, 65535.0 / 8192); + blueGain = std::clamp(blueGain, 0.0, 65535.0 / 8192); + asyncResults_.redGain = redGain; /* Hardcode the green gain to 1.0. */ asyncResults_.greenGain = 1.0;
The gain values are coded as u3.13 fixed point values, ie they can not be more than 8. Clampt the values in order to avoid any off limits value which could make the IPU3 behave weirdly. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)