| Message ID | 20210923081625.60276-9-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Sep 23, 2021 at 10:16:21AM +0200, Jean-Michel Hautbois wrote: > The gains have a precision u3.13, range [0, 8) which means that a gain s/)/]/ or s/[/(/ > multiplier value of 1.0 is represented as a multiplication by 8192 in > the ImgU. Correct the gains as this was misunderstood in the first > place. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index 800d023c..8a926691 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -313,6 +313,7 @@ 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); > asyncResults_.redGain = redGain; > + /* Green gains should not be touched and considered 1. */ > asyncResults_.greenGain = 1.0; > asyncResults_.blueGain = blueGain; > } > @@ -376,15 +377,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > * params->acc_param.bnr.opt_center.x_reset; > params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset > * params->acc_param.bnr.opt_center.y_reset; > - /* > - * Green gains should not be touched and considered 1. > - * Default is 16, so do not change it at all. > - * 4096 is the value for a gain of 1.0 > - */ > - params->acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.green; > - params->acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.red; > - params->acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blue; > - params->acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.green; > + /* 8192 is the multiplier for a gain of 1.0 */ I'd be a bit more explicit to say at least /* Convert to u3.13 Fixed point values with a base of 8192 */ But I'm not sure the '8192' is a base as such, perhaps just: /* Convert to u3.13 fixed point values */ Sometime I'm sure we could use C++ to make a u3_13 type or a FixedPoint class that handles this a bit more elegantly... But I don't think we want to dwell on that right now. Whatever works out best in the end (and I think we'll revist this with a FixedPoint type sometime)...: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green; > + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red; > + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue; > + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green; > > LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; > > -- > 2.30.2 > -- Kieran
On 28/09/2021 17:53, Kieran Bingham wrote: > On Thu, Sep 23, 2021 at 10:16:21AM +0200, Jean-Michel Hautbois wrote: >> The gains have a precision u3.13, range [0, 8) which means that a gain > > s/)/]/ or s/[/(/ Mmmh, I must disagree here, it is going from 0 to 7,99987793 and not 8 (all ones). So the interval is opened for 8. > >> multiplier value of 1.0 is represented as a multiplication by 8192 in >> the ImgU. Correct the gains as this was misunderstood in the first >> place. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++--------- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >> index 800d023c..8a926691 100644 >> --- a/src/ipa/ipu3/algorithms/awb.cpp >> +++ b/src/ipa/ipu3/algorithms/awb.cpp >> @@ -313,6 +313,7 @@ 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); >> asyncResults_.redGain = redGain; >> + /* Green gains should not be touched and considered 1. */ >> asyncResults_.greenGain = 1.0; >> asyncResults_.blueGain = blueGain; >> } >> @@ -376,15 +377,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >> * params->acc_param.bnr.opt_center.x_reset; >> params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset >> * params->acc_param.bnr.opt_center.y_reset; >> - /* >> - * Green gains should not be touched and considered 1. >> - * Default is 16, so do not change it at all. >> - * 4096 is the value for a gain of 1.0 >> - */ >> - params->acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.green; >> - params->acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.red; >> - params->acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blue; >> - params->acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.green; >> + /* 8192 is the multiplier for a gain of 1.0 */ > > I'd be a bit more explicit to say at least > > /* Convert to u3.13 Fixed point values with a base of 8192 */ > > But I'm not sure the '8192' is a base as such, perhaps just: > > /* Convert to u3.13 fixed point values */ > > Sometime I'm sure we could use C++ to make a u3_13 type or a FixedPoint > class that handles this a bit more elegantly... > > But I don't think we want to dwell on that right now. > > Whatever works out best in the end (and I think we'll revist this with > a FixedPoint type sometime)...: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> OK, thx. > >> + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green; >> + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red; >> + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue; >> + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green; >> >> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; >> >> -- >> 2.30.2 >> > > -- > Kieran >
On 28/09/2021 16:59, Jean-Michel Hautbois wrote: > On 28/09/2021 17:53, Kieran Bingham wrote: >> On Thu, Sep 23, 2021 at 10:16:21AM +0200, Jean-Michel Hautbois wrote: >>> The gains have a precision u3.13, range [0, 8) which means that a gain >> >> s/)/]/ or s/[/(/ > > Mmmh, I must disagree here, it is going from 0 to 7,99987793 and not 8 > (all ones). So the interval is opened for 8. It looks like a typo ... Is that a defined way to describe that range otherwise? It wasn't obvious to me that you were trying to convey that by the different symbols. >>> multiplier value of 1.0 is represented as a multiplication by 8192 in >>> the ImgU. Correct the gains as this was misunderstood in the first >>> place. >>> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++--------- >>> 1 file changed, 6 insertions(+), 9 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>> index 800d023c..8a926691 100644 >>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>> @@ -313,6 +313,7 @@ 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); >>> asyncResults_.redGain = redGain; >>> + /* Green gains should not be touched and considered 1. */ >>> asyncResults_.greenGain = 1.0; >>> asyncResults_.blueGain = blueGain; >>> } >>> @@ -376,15 +377,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >>> * params->acc_param.bnr.opt_center.x_reset; >>> params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset >>> * params->acc_param.bnr.opt_center.y_reset; >>> - /* >>> - * Green gains should not be touched and considered 1. >>> - * Default is 16, so do not change it at all. >>> - * 4096 is the value for a gain of 1.0 >>> - */ >>> - params->acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.green; >>> - params->acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.red; >>> - params->acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blue; >>> - params->acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.green; >>> + /* 8192 is the multiplier for a gain of 1.0 */ >> >> I'd be a bit more explicit to say at least >> >> /* Convert to u3.13 Fixed point values with a base of 8192 */ >> >> But I'm not sure the '8192' is a base as such, perhaps just: >> >> /* Convert to u3.13 fixed point values */ >> >> Sometime I'm sure we could use C++ to make a u3_13 type or a FixedPoint >> class that handles this a bit more elegantly... >> >> But I don't think we want to dwell on that right now. >> >> Whatever works out best in the end (and I think we'll revist this with >> a FixedPoint type sometime)...: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > OK, thx. > >> >>> + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green; >>> + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red; >>> + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue; >>> + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green; >>> >>> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; >>> >>> -- >>> 2.30.2 >>> >> >> -- >> Kieran >>
On 28/09/2021 18:31, Kieran Bingham wrote: > On 28/09/2021 16:59, Jean-Michel Hautbois wrote: >> On 28/09/2021 17:53, Kieran Bingham wrote: >>> On Thu, Sep 23, 2021 at 10:16:21AM +0200, Jean-Michel Hautbois wrote: >>>> The gains have a precision u3.13, range [0, 8) which means that a gain >>> >>> s/)/]/ or s/[/(/ >> >> Mmmh, I must disagree here, it is going from 0 to 7,99987793 and not 8 >> (all ones). So the interval is opened for 8. > > It looks like a typo ... Is that a defined way to describe that range > otherwise? It wasn't obvious to me that you were trying to convey that > by the different symbols. > > Sorry about that, it is the mathematical definition, maybe should we write it differently... ? >>>> multiplier value of 1.0 is represented as a multiplication by 8192 in >>>> the ImgU. Correct the gains as this was misunderstood in the first >>>> place. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++--------- >>>> 1 file changed, 6 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>>> index 800d023c..8a926691 100644 >>>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>>> @@ -313,6 +313,7 @@ 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); >>>> asyncResults_.redGain = redGain; >>>> + /* Green gains should not be touched and considered 1. */ >>>> asyncResults_.greenGain = 1.0; >>>> asyncResults_.blueGain = blueGain; >>>> } >>>> @@ -376,15 +377,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >>>> * params->acc_param.bnr.opt_center.x_reset; >>>> params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset >>>> * params->acc_param.bnr.opt_center.y_reset; >>>> - /* >>>> - * Green gains should not be touched and considered 1. >>>> - * Default is 16, so do not change it at all. >>>> - * 4096 is the value for a gain of 1.0 >>>> - */ >>>> - params->acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.green; >>>> - params->acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.red; >>>> - params->acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blue; >>>> - params->acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.green; >>>> + /* 8192 is the multiplier for a gain of 1.0 */ >>> >>> I'd be a bit more explicit to say at least >>> >>> /* Convert to u3.13 Fixed point values with a base of 8192 */ >>> >>> But I'm not sure the '8192' is a base as such, perhaps just: >>> >>> /* Convert to u3.13 fixed point values */ >>> >>> Sometime I'm sure we could use C++ to make a u3_13 type or a FixedPoint >>> class that handles this a bit more elegantly... >>> >>> But I don't think we want to dwell on that right now. >>> >>> Whatever works out best in the end (and I think we'll revist this with >>> a FixedPoint type sometime)...: >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> OK, thx. >> >>> >>>> + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green; >>>> + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red; >>>> + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue; >>>> + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green; >>>> >>>> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; >>>> >>>> -- >>>> 2.30.2 >>>> >>> >>> -- >>> Kieran >>>
On 28/09/2021 17:32, Jean-Michel Hautbois wrote: > On 28/09/2021 18:31, Kieran Bingham wrote: >> On 28/09/2021 16:59, Jean-Michel Hautbois wrote: >>> On 28/09/2021 17:53, Kieran Bingham wrote: >>>> On Thu, Sep 23, 2021 at 10:16:21AM +0200, Jean-Michel Hautbois wrote: >>>>> The gains have a precision u3.13, range [0, 8) which means that a gain >>>> >>>> s/)/]/ or s/[/(/ >>> >>> Mmmh, I must disagree here, it is going from 0 to 7,99987793 and not 8 >>> (all ones). So the interval is opened for 8. >> >> It looks like a typo ... Is that a defined way to describe that range >> otherwise? It wasn't obvious to me that you were trying to convey that >> by the different symbols. >> >> > > Sorry about that, it is the mathematical definition, maybe should we > write it differently... ? If it's a standard mathematical definition to use in that way, that's fine. >>>>> multiplier value of 1.0 is represented as a multiplication by 8192 in >>>>> the ImgU. Correct the gains as this was misunderstood in the first >>>>> place. >>>>> >>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>>> --- >>>>> src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++--------- >>>>> 1 file changed, 6 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>>>> index 800d023c..8a926691 100644 >>>>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>>>> @@ -313,6 +313,7 @@ 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); >>>>> asyncResults_.redGain = redGain; >>>>> + /* Green gains should not be touched and considered 1. */ >>>>> asyncResults_.greenGain = 1.0; >>>>> asyncResults_.blueGain = blueGain; >>>>> } >>>>> @@ -376,15 +377,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >>>>> * params->acc_param.bnr.opt_center.x_reset; >>>>> params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset >>>>> * params->acc_param.bnr.opt_center.y_reset; >>>>> - /* >>>>> - * Green gains should not be touched and considered 1. >>>>> - * Default is 16, so do not change it at all. >>>>> - * 4096 is the value for a gain of 1.0 >>>>> - */ >>>>> - params->acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.green; >>>>> - params->acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.red; >>>>> - params->acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blue; >>>>> - params->acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.green; >>>>> + /* 8192 is the multiplier for a gain of 1.0 */ >>>> >>>> I'd be a bit more explicit to say at least >>>> >>>> /* Convert to u3.13 Fixed point values with a base of 8192 */ >>>> >>>> But I'm not sure the '8192' is a base as such, perhaps just: >>>> >>>> /* Convert to u3.13 fixed point values */ >>>> >>>> Sometime I'm sure we could use C++ to make a u3_13 type or a FixedPoint >>>> class that handles this a bit more elegantly... >>>> >>>> But I don't think we want to dwell on that right now. >>>> >>>> Whatever works out best in the end (and I think we'll revist this with >>>> a FixedPoint type sometime)...: >>>> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> OK, thx. >>> >>>> >>>>> + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green; >>>>> + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red; >>>>> + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue; >>>>> + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green; >>>>> >>>>> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; >>>>> >>>>> -- >>>>> 2.30.2 >>>>> >>>> >>>> -- >>>> Kieran >>>>
On Tue, Sep 28, 2021 at 05:36:24PM +0100, Kieran Bingham wrote: > On 28/09/2021 17:32, Jean-Michel Hautbois wrote: > > On 28/09/2021 18:31, Kieran Bingham wrote: > >> On 28/09/2021 16:59, Jean-Michel Hautbois wrote: > >>> On 28/09/2021 17:53, Kieran Bingham wrote: > >>>> On Thu, Sep 23, 2021 at 10:16:21AM +0200, Jean-Michel Hautbois wrote: > >>>>> The gains have a precision u3.13, range [0, 8) which means that a gain > >>>> > >>>> s/)/]/ or s/[/(/ > >>> > >>> Mmmh, I must disagree here, it is going from 0 to 7,99987793 and not 8 > >>> (all ones). So the interval is opened for 8. > >> > >> It looks like a typo ... Is that a defined way to describe that range > >> otherwise? It wasn't obvious to me that you were trying to convey that > >> by the different symbols. > > > > Sorry about that, it is the mathematical definition, maybe should we > > write it differently... ? > > If it's a standard mathematical definition to use in that way, that's fine. Another option is [0, 8[ which I find more readable (in particular it's not immediately visible that (0, 8) means ]0, 8[ in my opinion), but that may just be me. https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints > >>>>> multiplier value of 1.0 is represented as a multiplication by 8192 in > >>>>> the ImgU. Correct the gains as this was misunderstood in the first > >>>>> place. > >>>>> > >>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >>>>> --- > >>>>> src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++--------- > >>>>> 1 file changed, 6 insertions(+), 9 deletions(-) > >>>>> > >>>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > >>>>> index 800d023c..8a926691 100644 > >>>>> --- a/src/ipa/ipu3/algorithms/awb.cpp > >>>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp > >>>>> @@ -313,6 +313,7 @@ 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); > >>>>> asyncResults_.redGain = redGain; > >>>>> + /* Green gains should not be touched and considered 1. */ /*Hardcode the green gain to 1.0. */ > >>>>> asyncResults_.greenGain = 1.0; > >>>>> asyncResults_.blueGain = blueGain; > >>>>> } > >>>>> @@ -376,15 +377,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > >>>>> * params->acc_param.bnr.opt_center.x_reset; > >>>>> params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset > >>>>> * params->acc_param.bnr.opt_center.y_reset; > >>>>> - /* > >>>>> - * Green gains should not be touched and considered 1. > >>>>> - * Default is 16, so do not change it at all. > >>>>> - * 4096 is the value for a gain of 1.0 > >>>>> - */ > >>>>> - params->acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.green; > >>>>> - params->acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.red; > >>>>> - params->acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blue; > >>>>> - params->acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.green; > >>>>> + /* 8192 is the multiplier for a gain of 1.0 */ > >>>> > >>>> I'd be a bit more explicit to say at least > >>>> > >>>> /* Convert to u3.13 Fixed point values with a base of 8192 */ > >>>> > >>>> But I'm not sure the '8192' is a base as such, perhaps just: > >>>> > >>>> /* Convert to u3.13 fixed point values */ Looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> Sometime I'm sure we could use C++ to make a u3_13 type or a FixedPoint > >>>> class that handles this a bit more elegantly... > >>>> > >>>> But I don't think we want to dwell on that right now. > >>>> > >>>> Whatever works out best in the end (and I think we'll revist this with > >>>> a FixedPoint type sometime)...: Sometime indeed :-) > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> > >>> OK, thx. > >>> > >>>>> + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green; > >>>>> + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red; > >>>>> + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue; > >>>>> + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green; > >>>>> > >>>>> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; > >>>>>
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index 800d023c..8a926691 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -313,6 +313,7 @@ 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); asyncResults_.redGain = redGain; + /* Green gains should not be touched and considered 1. */ asyncResults_.greenGain = 1.0; asyncResults_.blueGain = blueGain; } @@ -376,15 +377,11 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) * params->acc_param.bnr.opt_center.x_reset; params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset * params->acc_param.bnr.opt_center.y_reset; - /* - * Green gains should not be touched and considered 1. - * Default is 16, so do not change it at all. - * 4096 is the value for a gain of 1.0 - */ - params->acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.green; - params->acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.red; - params->acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blue; - params->acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.green; + /* 8192 is the multiplier for a gain of 1.0 */ + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green; + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red; + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue; + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green; LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
The gains have a precision u3.13, range [0, 8) which means that a gain multiplier value of 1.0 is represented as a multiplication by 8192 in the ImgU. Correct the gains as this was misunderstood in the first place. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/awb.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)