| Message ID | 20220908014200.28728-32-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:59) > When the RGB means are too small, gains and color temperature can't be > meaningfully calculated. Freeze the AWB in that case, using the > previously calculated values. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 17 ++++++++++++++++- > src/ipa/rkisp1/ipa_context.cpp | 3 +++ > src/ipa/rkisp1/ipa_context.h | 1 + > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 5f2535688c93..404ad66e6953 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -31,6 +31,9 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > +/* Minimum mean value below which AWB can't operate. */ > +constexpr double kMeanMinThreshold = 2.0; Will this be a tuning parameter? Is it something to calibrate later? or better document the expected ranges that should be used here? What has led to the choice of 2.0, - is it simply that we can't estimate a colour temparature from a near black / dark image? If so - should we have a representation of overall brightness? > + > Awb::Awb() > : rgbMode_(false) > { > @@ -263,7 +266,17 @@ void Awb::process(IPAContext &context, > greenMean /= frameContext.awb.gains.green; > blueMean /= frameContext.awb.gains.blue; > > - frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > + /* > + * If the means are too small we don't have enough information to > + * meaningfully calculate gains. Freeze the algorithm in that case. > + */ > + if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && > + blueMean < kMeanMinThreshold) { > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > + return; > + } > + > + activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); Aha - good, here's the active state view of the colour temperature. While I have questions about how the threshold is determined, I think this is still good to get in. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > /* > * Estimate the red and blue gains to apply in a grey world. The green > @@ -290,6 +303,8 @@ void Awb::process(IPAContext &context, > activeState.awb.gains.automatic.blue = blueGain; > activeState.awb.gains.automatic.green = 1.0; > > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > + > LOG(RkISP1Awb, Debug) << std::showpoint > << "Means [" << redMean << ", " << greenMean << ", " << blueMean > << "], gains [" << activeState.awb.gains.automatic.red << ", " > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 335cb32c538d..9cac05bec79b 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -156,6 +156,9 @@ namespace libcamera::ipa::rkisp1 { > * \var IPAActiveState::awb.gains.automatic.blue > * \brief Automatic white balance gain for B channel > * > + * \var IPAActiveState::awb.temperatureK > + * \brief Estimated color temperature > + * > * \var IPAActiveState::awb.autoEnabled > * \brief Whether the Auto White Balance algorithm is enabled > */ > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 53787320e565..d0bc9090a8f1 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -68,6 +68,7 @@ struct IPAActiveState { > } automatic; > } gains; > > + unsigned int temperatureK; > bool autoEnabled; > } awb; > > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Thu, Sep 08, 2022 at 04:41:59AM +0300, Laurent Pinchart via libcamera-devel wrote: > When the RGB means are too small, gains and color temperature can't be > meaningfully calculated. Freeze the AWB in that case, using the > previously calculated values. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 17 ++++++++++++++++- > src/ipa/rkisp1/ipa_context.cpp | 3 +++ > src/ipa/rkisp1/ipa_context.h | 1 + > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 5f2535688c93..404ad66e6953 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -31,6 +31,9 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > +/* Minimum mean value below which AWB can't operate. */ > +constexpr double kMeanMinThreshold = 2.0; > + Is this a software constraint specified somewhere ? > Awb::Awb() > : rgbMode_(false) > { > @@ -263,7 +266,17 @@ void Awb::process(IPAContext &context, > greenMean /= frameContext.awb.gains.green; > blueMean /= frameContext.awb.gains.blue; > > - frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > + /* > + * If the means are too small we don't have enough information to > + * meaningfully calculate gains. Freeze the algorithm in that case. > + */ > + if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && > + blueMean < kMeanMinThreshold) { > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > + return; > + } > + > + activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > > /* > * Estimate the red and blue gains to apply in a grey world. The green > @@ -290,6 +303,8 @@ void Awb::process(IPAContext &context, > activeState.awb.gains.automatic.blue = blueGain; > activeState.awb.gains.automatic.green = 1.0; > > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > + > LOG(RkISP1Awb, Debug) << std::showpoint > << "Means [" << redMean << ", " << greenMean << ", " << blueMean > << "], gains [" << activeState.awb.gains.automatic.red << ", " > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 335cb32c538d..9cac05bec79b 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -156,6 +156,9 @@ namespace libcamera::ipa::rkisp1 { > * \var IPAActiveState::awb.gains.automatic.blue > * \brief Automatic white balance gain for B channel > * > + * \var IPAActiveState::awb.temperatureK > + * \brief Estimated color temperature > + * > * \var IPAActiveState::awb.autoEnabled > * \brief Whether the Auto White Balance algorithm is enabled > */ > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 53787320e565..d0bc9090a8f1 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -68,6 +68,7 @@ struct IPAActiveState { > } automatic; > } gains; > > + unsigned int temperatureK; > bool autoEnabled; > } awb; > > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Wed, Sep 21, 2022 at 12:46:19AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:59) > > When the RGB means are too small, gains and color temperature can't be > > meaningfully calculated. Freeze the AWB in that case, using the > > previously calculated values. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 17 ++++++++++++++++- > > src/ipa/rkisp1/ipa_context.cpp | 3 +++ > > src/ipa/rkisp1/ipa_context.h | 1 + > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 5f2535688c93..404ad66e6953 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -31,6 +31,9 @@ namespace ipa::rkisp1::algorithms { > > > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > > > +/* Minimum mean value below which AWB can't operate. */ > > +constexpr double kMeanMinThreshold = 2.0; > > Will this be a tuning parameter? Is it something to calibrate later? or > better document the expected ranges that should be used here? > > What has led to the choice of 2.0, - is it simply that we can't estimate > a colour temparature from a near black / dark image? > > If so - should we have a representation of overall brightness? The means are supposed to be in the 0-255 range. A value below 2 is just too low to do any meaningful computation. We'll probably need to adjust this, as I think the threshold is too low, but I haven't spent any time on that. I don't think it should be a tuning parameter, it's just a threshold to avoid numerical instability. > > + > > Awb::Awb() > > : rgbMode_(false) > > { > > @@ -263,7 +266,17 @@ void Awb::process(IPAContext &context, > > greenMean /= frameContext.awb.gains.green; > > blueMean /= frameContext.awb.gains.blue; > > > > - frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > > + /* > > + * If the means are too small we don't have enough information to > > + * meaningfully calculate gains. Freeze the algorithm in that case. > > + */ > > + if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && > > + blueMean < kMeanMinThreshold) { > > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > > + return; > > + } > > + > > + activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > > Aha - good, here's the active state view of the colour temperature. > > > While I have questions about how the threshold is determined, I think > this is still good to get in. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > /* > > * Estimate the red and blue gains to apply in a grey world. The green > > @@ -290,6 +303,8 @@ void Awb::process(IPAContext &context, > > activeState.awb.gains.automatic.blue = blueGain; > > activeState.awb.gains.automatic.green = 1.0; > > > > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > > + > > LOG(RkISP1Awb, Debug) << std::showpoint > > << "Means [" << redMean << ", " << greenMean << ", " << blueMean > > << "], gains [" << activeState.awb.gains.automatic.red << ", " > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > index 335cb32c538d..9cac05bec79b 100644 > > --- a/src/ipa/rkisp1/ipa_context.cpp > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > @@ -156,6 +156,9 @@ namespace libcamera::ipa::rkisp1 { > > * \var IPAActiveState::awb.gains.automatic.blue > > * \brief Automatic white balance gain for B channel > > * > > + * \var IPAActiveState::awb.temperatureK > > + * \brief Estimated color temperature > > + * > > * \var IPAActiveState::awb.autoEnabled > > * \brief Whether the Auto White Balance algorithm is enabled > > */ > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index 53787320e565..d0bc9090a8f1 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -68,6 +68,7 @@ struct IPAActiveState { > > } automatic; > > } gains; > > > > + unsigned int temperatureK; > > bool autoEnabled; > > } awb; > >
Hi Jacopo, On Thu, Sep 22, 2022 at 12:45:24PM +0200, Jacopo Mondi wrote: > On Thu, Sep 08, 2022 at 04:41:59AM +0300, Laurent Pinchart via libcamera-devel wrote: > > When the RGB means are too small, gains and color temperature can't be > > meaningfully calculated. Freeze the AWB in that case, using the > > previously calculated values. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 17 ++++++++++++++++- > > src/ipa/rkisp1/ipa_context.cpp | 3 +++ > > src/ipa/rkisp1/ipa_context.h | 1 + > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 5f2535688c93..404ad66e6953 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -31,6 +31,9 @@ namespace ipa::rkisp1::algorithms { > > > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > > > +/* Minimum mean value below which AWB can't operate. */ > > +constexpr double kMeanMinThreshold = 2.0; > > + > > Is this a software constraint specified somewhere ? We compute the red and blue gains as greenMean / (redMean + 1) greenMean / (blueMean + 1) The means are in the 0-255 range. A value lower than 2 is just too low to give any meaningful result. I expect we'll need to increase the threshold actually, this is just an initial value to avoid numerical instability in the calculations. > > Awb::Awb() > > : rgbMode_(false) > > { > > @@ -263,7 +266,17 @@ void Awb::process(IPAContext &context, > > greenMean /= frameContext.awb.gains.green; > > blueMean /= frameContext.awb.gains.blue; > > > > - frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > > + /* > > + * If the means are too small we don't have enough information to > > + * meaningfully calculate gains. Freeze the algorithm in that case. > > + */ > > + if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && > > + blueMean < kMeanMinThreshold) { > > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > > + return; > > + } > > + > > + activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > > > > /* > > * Estimate the red and blue gains to apply in a grey world. The green > > @@ -290,6 +303,8 @@ void Awb::process(IPAContext &context, > > activeState.awb.gains.automatic.blue = blueGain; > > activeState.awb.gains.automatic.green = 1.0; > > > > + frameContext.awb.temperatureK = activeState.awb.temperatureK; > > + > > LOG(RkISP1Awb, Debug) << std::showpoint > > << "Means [" << redMean << ", " << greenMean << ", " << blueMean > > << "], gains [" << activeState.awb.gains.automatic.red << ", " > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > index 335cb32c538d..9cac05bec79b 100644 > > --- a/src/ipa/rkisp1/ipa_context.cpp > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > @@ -156,6 +156,9 @@ namespace libcamera::ipa::rkisp1 { > > * \var IPAActiveState::awb.gains.automatic.blue > > * \brief Automatic white balance gain for B channel > > * > > + * \var IPAActiveState::awb.temperatureK > > + * \brief Estimated color temperature > > + * > > * \var IPAActiveState::awb.autoEnabled > > * \brief Whether the Auto White Balance algorithm is enabled > > */ > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index 53787320e565..d0bc9090a8f1 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -68,6 +68,7 @@ struct IPAActiveState { > > } automatic; > > } gains; > > > > + unsigned int temperatureK; > > bool autoEnabled; > > } awb; > >
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 5f2535688c93..404ad66e6953 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -31,6 +31,9 @@ namespace ipa::rkisp1::algorithms { LOG_DEFINE_CATEGORY(RkISP1Awb) +/* Minimum mean value below which AWB can't operate. */ +constexpr double kMeanMinThreshold = 2.0; + Awb::Awb() : rgbMode_(false) { @@ -263,7 +266,17 @@ void Awb::process(IPAContext &context, greenMean /= frameContext.awb.gains.green; blueMean /= frameContext.awb.gains.blue; - frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); + /* + * If the means are too small we don't have enough information to + * meaningfully calculate gains. Freeze the algorithm in that case. + */ + if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && + blueMean < kMeanMinThreshold) { + frameContext.awb.temperatureK = activeState.awb.temperatureK; + return; + } + + activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); /* * Estimate the red and blue gains to apply in a grey world. The green @@ -290,6 +303,8 @@ void Awb::process(IPAContext &context, activeState.awb.gains.automatic.blue = blueGain; activeState.awb.gains.automatic.green = 1.0; + frameContext.awb.temperatureK = activeState.awb.temperatureK; + LOG(RkISP1Awb, Debug) << std::showpoint << "Means [" << redMean << ", " << greenMean << ", " << blueMean << "], gains [" << activeState.awb.gains.automatic.red << ", " diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp index 335cb32c538d..9cac05bec79b 100644 --- a/src/ipa/rkisp1/ipa_context.cpp +++ b/src/ipa/rkisp1/ipa_context.cpp @@ -156,6 +156,9 @@ namespace libcamera::ipa::rkisp1 { * \var IPAActiveState::awb.gains.automatic.blue * \brief Automatic white balance gain for B channel * + * \var IPAActiveState::awb.temperatureK + * \brief Estimated color temperature + * * \var IPAActiveState::awb.autoEnabled * \brief Whether the Auto White Balance algorithm is enabled */ diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 53787320e565..d0bc9090a8f1 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -68,6 +68,7 @@ struct IPAActiveState { } automatic; } gains; + unsigned int temperatureK; bool autoEnabled; } awb;
When the RGB means are too small, gains and color temperature can't be meaningfully calculated. Freeze the AWB in that case, using the previously calculated values. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 17 ++++++++++++++++- src/ipa/rkisp1/ipa_context.cpp | 3 +++ src/ipa/rkisp1/ipa_context.h | 1 + 3 files changed, 20 insertions(+), 1 deletion(-)