| Message ID | 20220617083211.28407-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jean-Michel, Thank you for the patch. I'd write "Correct the gains calculation" in the subject, as it's not just the factor. On Fri, Jun 17, 2022 at 10:32:11AM +0200, Jean-Michel Hautbois via libcamera-devel wrote: > The factor used right now in the IPU3 is 8192, as a multiplier of the > estimated gain. This is wrong, as the isp is adding 1.0 to the gain > applied, ie Pout = { Pin * (1 + Gx) }. > > Fix it, and to ease the reading, introduce a small helper function. The effect of this patch on image quality is really great ! > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++---- > src/ipa/ipu3/algorithms/awb.h | 1 + > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index 5c232d92..6abaf75f 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value) > return value * 8191; > } > > +constexpr uint16_t Awb::fixedGain(double gain) Does "fixed" refer to the fact that the return value is a fixed-point number ? As the function does more than conversion to fixed point, I'd call it gainValue(). > +{ > + /* > + * For BNR parameters WB gain factor for the three channels I count four channels :-) > + * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8) Gb and Gr are slightly confusing here, they usually refer to the green-blue and green-red components, while I suppose they stand for gain-blue and gain-red in this context. > + * and the actual gain is Gx + 1, it is typically Gx = 1. > + * > + * Pout = {Pin * (1 + Gx)}. > + */ If I may propose a small improvement to the documentation (as it's important to record this, given that the kernel API doesn't document the gain format correctly): /* * The colour gains applied by the BNR for the four channels (Gr, R, B * and Gb) are expressed in the parameters structure as 16-bit integers * that store a fixed-point U3.13 value in the range [0, 8[. * * The real gain value is equal to the gain parameter plus one, i.e. * * Pout = Pin * (1 * gain / 8192) * * where 'Pin' is the input pixel value, 'Pout' the output pixel value, * and 'gain' the gain in the parameters structure as a 16-bit integer. */ You can also write [0, 8) instead of [0, 8[. > + gain = std::clamp((gain - 1.0), 0.0, 8.0); No need for the inner parentheses. > + return gain * 8192; And this holds on a single line: return std::clamp(gain - 1.0, 0.0, 8.0) * 8192; But we should actually limit the value to [0.0, 8.0[, not [0.0, 8.0]. One option would be return std::clamp((gain - 1.0) * 8192, 0.0, 65535.0); > +} > + > /** > * \copydoc libcamera::ipa::Algorithm::prepare > */ > @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > 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; > /* Convert to u3.13 fixed point values */ Let's drop this comment, it's now part of fixedGain(). Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - params->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green; > - params->acc_param.bnr.wb_gains.r = 8192 * context.activeState.awb.gains.red; > - params->acc_param.bnr.wb_gains.b = 8192 * context.activeState.awb.gains.blue; > - params->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green; > + params->acc_param.bnr.wb_gains.gr = fixedGain(context.activeState.awb.gains.green); > + params->acc_param.bnr.wb_gains.r = fixedGain(context.activeState.awb.gains.red); > + params->acc_param.bnr.wb_gains.b = fixedGain(context.activeState.awb.gains.blue); > + params->acc_param.bnr.wb_gains.gb = fixedGain(context.activeState.awb.gains.green); > > LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; > > diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h > index 9a50a985..3154541d 100644 > --- a/src/ipa/ipu3/algorithms/awb.h > +++ b/src/ipa/ipu3/algorithms/awb.h > @@ -73,6 +73,7 @@ private: > void awbGreyWorld(); > uint32_t estimateCCT(double red, double green, double blue); > static constexpr uint16_t threshold(float value); > + static constexpr uint16_t fixedGain(double gain); > > std::vector<RGB> zones_; > Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
Hi Laurent, On 17/06/2022 22:39, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > I'd write "Correct the gains calculation" in the subject, as it's not > just the factor. > > On Fri, Jun 17, 2022 at 10:32:11AM +0200, Jean-Michel Hautbois via libcamera-devel wrote: >> The factor used right now in the IPU3 is 8192, as a multiplier of the >> estimated gain. This is wrong, as the isp is adding 1.0 to the gain >> applied, ie Pout = { Pin * (1 + Gx) }. >> >> Fix it, and to ease the reading, introduce a small helper function. > > The effect of this patch on image quality is really great ! > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++---- >> src/ipa/ipu3/algorithms/awb.h | 1 + >> 2 files changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >> index 5c232d92..6abaf75f 100644 >> --- a/src/ipa/ipu3/algorithms/awb.cpp >> +++ b/src/ipa/ipu3/algorithms/awb.cpp >> @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value) >> return value * 8191; >> } >> >> +constexpr uint16_t Awb::fixedGain(double gain) > > Does "fixed" refer to the fact that the return value is a fixed-point > number ? As the function does more than conversion to fixed point, I'd > call it gainValue(). > >> +{ >> + /* >> + * For BNR parameters WB gain factor for the three channels > > I count four channels :-) > >> + * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8) > > Gb and Gr are slightly confusing here, they usually refer to the > green-blue and green-red components, while I suppose they stand for > gain-blue and gain-red in this context. > >> + * and the actual gain is Gx + 1, it is typically Gx = 1. >> + * >> + * Pout = {Pin * (1 + Gx)}. >> + */ > > If I may propose a small improvement to the documentation (as it's > important to record this, given that the kernel API doesn't document the > gain format correctly): > > /* > * The colour gains applied by the BNR for the four channels (Gr, R, B > * and Gb) are expressed in the parameters structure as 16-bit integers > * that store a fixed-point U3.13 value in the range [0, 8[. > * > * The real gain value is equal to the gain parameter plus one, i.e. > * > * Pout = Pin * (1 * gain / 8192) > * > * where 'Pin' is the input pixel value, 'Pout' the output pixel value, > * and 'gain' the gain in the parameters structure as a 16-bit integer. > */ Thanks for the documentation rewriting ! I think it should be: - * Pout = Pin * (1 * gain / 8192) + * Pout = Pin * (1 + gain / 8192) > > You can also write [0, 8) instead of [0, 8[. > >> + gain = std::clamp((gain - 1.0), 0.0, 8.0); > > No need for the inner parentheses. > >> + return gain * 8192; > > And this holds on a single line: > > return std::clamp(gain - 1.0, 0.0, 8.0) * 8192; > > But we should actually limit the value to [0.0, 8.0[, not [0.0, 8.0]. > One option would be > > return std::clamp((gain - 1.0) * 8192, 0.0, 65535.0); I like this one :-) ! > >> +} >> + >> /** >> * \copydoc libcamera::ipa::Algorithm::prepare >> */ >> @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >> 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; >> /* Convert to u3.13 fixed point values */ > > Let's drop this comment, it's now part of fixedGain(). Indeed. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks. > >> - params->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green; >> - params->acc_param.bnr.wb_gains.r = 8192 * context.activeState.awb.gains.red; >> - params->acc_param.bnr.wb_gains.b = 8192 * context.activeState.awb.gains.blue; >> - params->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green; >> + params->acc_param.bnr.wb_gains.gr = fixedGain(context.activeState.awb.gains.green); >> + params->acc_param.bnr.wb_gains.r = fixedGain(context.activeState.awb.gains.red); >> + params->acc_param.bnr.wb_gains.b = fixedGain(context.activeState.awb.gains.blue); >> + params->acc_param.bnr.wb_gains.gb = fixedGain(context.activeState.awb.gains.green); >> >> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; >> >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h >> index 9a50a985..3154541d 100644 >> --- a/src/ipa/ipu3/algorithms/awb.h >> +++ b/src/ipa/ipu3/algorithms/awb.h >> @@ -73,6 +73,7 @@ private: >> void awbGreyWorld(); >> uint32_t estimateCCT(double red, double green, double blue); >> static constexpr uint16_t threshold(float value); >> + static constexpr uint16_t fixedGain(double gain); >> >> std::vector<RGB> zones_; >> Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; >
Hi Jean-Michel, On Fri, Jun 17, 2022 at 11:25:53PM +0200, Jean-Michel Hautbois wrote: > On 17/06/2022 22:39, Laurent Pinchart wrote: > > Hi Jean-Michel, > > > > Thank you for the patch. > > > > I'd write "Correct the gains calculation" in the subject, as it's not > > just the factor. > > > > On Fri, Jun 17, 2022 at 10:32:11AM +0200, Jean-Michel Hautbois via libcamera-devel wrote: > >> The factor used right now in the IPU3 is 8192, as a multiplier of the > >> estimated gain. This is wrong, as the isp is adding 1.0 to the gain > >> applied, ie Pout = { Pin * (1 + Gx) }. > >> > >> Fix it, and to ease the reading, introduce a small helper function. > > > > The effect of this patch on image quality is really great ! > > > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- > >> src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++---- > >> src/ipa/ipu3/algorithms/awb.h | 1 + > >> 2 files changed, 18 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > >> index 5c232d92..6abaf75f 100644 > >> --- a/src/ipa/ipu3/algorithms/awb.cpp > >> +++ b/src/ipa/ipu3/algorithms/awb.cpp > >> @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value) > >> return value * 8191; > >> } > >> > >> +constexpr uint16_t Awb::fixedGain(double gain) > > > > Does "fixed" refer to the fact that the return value is a fixed-point > > number ? As the function does more than conversion to fixed point, I'd > > call it gainValue(). > > > >> +{ > >> + /* > >> + * For BNR parameters WB gain factor for the three channels > > > > I count four channels :-) > > > >> + * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8) > > > > Gb and Gr are slightly confusing here, they usually refer to the > > green-blue and green-red components, while I suppose they stand for > > gain-blue and gain-red in this context. > > > >> + * and the actual gain is Gx + 1, it is typically Gx = 1. > >> + * > >> + * Pout = {Pin * (1 + Gx)}. > >> + */ > > > > If I may propose a small improvement to the documentation (as it's > > important to record this, given that the kernel API doesn't document the > > gain format correctly): > > > > /* > > * The colour gains applied by the BNR for the four channels (Gr, R, B > > * and Gb) are expressed in the parameters structure as 16-bit integers > > * that store a fixed-point U3.13 value in the range [0, 8[. > > * > > * The real gain value is equal to the gain parameter plus one, i.e. > > * > > * Pout = Pin * (1 * gain / 8192) > > * > > * where 'Pin' is the input pixel value, 'Pout' the output pixel value, > > * and 'gain' the gain in the parameters structure as a 16-bit integer. > > */ > > Thanks for the documentation rewriting ! > I think it should be: > > - * Pout = Pin * (1 * gain / 8192) > + * Pout = Pin * (1 + gain / 8192) Indeed, that's what I meant. If we cross-review our reviews we'll achieve a good result :-) > > > > You can also write [0, 8) instead of [0, 8[. > > > >> + gain = std::clamp((gain - 1.0), 0.0, 8.0); > > > > No need for the inner parentheses. > > > >> + return gain * 8192; > > > > And this holds on a single line: > > > > return std::clamp(gain - 1.0, 0.0, 8.0) * 8192; > > > > But we should actually limit the value to [0.0, 8.0[, not [0.0, 8.0]. > > One option would be > > > > return std::clamp((gain - 1.0) * 8192, 0.0, 65535.0); > > I like this one :-) ! > > >> +} > >> + > >> /** > >> * \copydoc libcamera::ipa::Algorithm::prepare > >> */ > >> @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > >> 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; > >> /* Convert to u3.13 fixed point values */ > > > > Let's drop this comment, it's now part of fixedGain(). > > Indeed. > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks. > > >> - params->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green; > >> - params->acc_param.bnr.wb_gains.r = 8192 * context.activeState.awb.gains.red; > >> - params->acc_param.bnr.wb_gains.b = 8192 * context.activeState.awb.gains.blue; > >> - params->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green; > >> + params->acc_param.bnr.wb_gains.gr = fixedGain(context.activeState.awb.gains.green); > >> + params->acc_param.bnr.wb_gains.r = fixedGain(context.activeState.awb.gains.red); > >> + params->acc_param.bnr.wb_gains.b = fixedGain(context.activeState.awb.gains.blue); > >> + params->acc_param.bnr.wb_gains.gb = fixedGain(context.activeState.awb.gains.green); > >> > >> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; > >> > >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h > >> index 9a50a985..3154541d 100644 > >> --- a/src/ipa/ipu3/algorithms/awb.h > >> +++ b/src/ipa/ipu3/algorithms/awb.h > >> @@ -73,6 +73,7 @@ private: > >> void awbGreyWorld(); > >> uint32_t estimateCCT(double red, double green, double blue); > >> static constexpr uint16_t threshold(float value); > >> + static constexpr uint16_t fixedGain(double gain); > >> > >> std::vector<RGB> zones_; > >> Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index 5c232d92..6abaf75f 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value) return value * 8191; } +constexpr uint16_t Awb::fixedGain(double gain) +{ + /* + * For BNR parameters WB gain factor for the three channels + * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8) + * and the actual gain is Gx + 1, it is typically Gx = 1. + * + * Pout = {Pin * (1 + Gx)}. + */ + gain = std::clamp((gain - 1.0), 0.0, 8.0); + return gain * 8192; +} + /** * \copydoc libcamera::ipa::Algorithm::prepare */ @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) 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; /* Convert to u3.13 fixed point values */ - params->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green; - params->acc_param.bnr.wb_gains.r = 8192 * context.activeState.awb.gains.red; - params->acc_param.bnr.wb_gains.b = 8192 * context.activeState.awb.gains.blue; - params->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green; + params->acc_param.bnr.wb_gains.gr = fixedGain(context.activeState.awb.gains.green); + params->acc_param.bnr.wb_gains.r = fixedGain(context.activeState.awb.gains.red); + params->acc_param.bnr.wb_gains.b = fixedGain(context.activeState.awb.gains.blue); + params->acc_param.bnr.wb_gains.gb = fixedGain(context.activeState.awb.gains.green); LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h index 9a50a985..3154541d 100644 --- a/src/ipa/ipu3/algorithms/awb.h +++ b/src/ipa/ipu3/algorithms/awb.h @@ -73,6 +73,7 @@ private: void awbGreyWorld(); uint32_t estimateCCT(double red, double green, double blue); static constexpr uint16_t threshold(float value); + static constexpr uint16_t fixedGain(double gain); std::vector<RGB> zones_; Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
The factor used right now in the IPU3 is 8192, as a multiplier of the estimated gain. This is wrong, as the isp is adding 1.0 to the gain applied, ie Pout = { Pin * (1 + Gx) }. Fix it, and to ease the reading, introduce a small helper function. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++---- src/ipa/ipu3/algorithms/awb.h | 1 + 2 files changed, 18 insertions(+), 4 deletions(-)