| Message ID | 20240805120522.1613342-4-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2024-08-05 13:05:04) > There are many use-cases (tuning-validation, working in static > environments) where a manual ColourTemperature control is helpful. > Implement that by interpolating and applying the white balance gains > from the tuning file according to the requested colour temperature. If > colour gains are provided on the same request, they take precedence. As > the colour temperature reported in the metadata is always based on the > measurements, we don't have to touch that. > > Note that in the automatic case, the colour gains are still based on the > gray world model and the ones from the tuning file get ignored. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 957d24fe3425..d482eda5b541 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > +constexpr int32_t kMinColourTemperature = 2500; > +constexpr int32_t kMaxColourTemperature = 10000; > +constexpr int32_t kDefaultColourTemperature = 6500; > + > /* Minimum mean value below which AWB can't operate. */ > constexpr double kMeanMinThreshold = 2.0; > > @@ -44,6 +48,11 @@ Awb::Awb() > */ > int Awb::init(IPAContext &context, const YamlObject &tuningData) > { > + auto &cmap = context.ctrlMap; > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature, > + kMaxColourTemperature, > + kDefaultColourTemperature); > + Should this control only be exposed if there are gains available in the tuning file? Can/Should the minColourTemperature/maxColourTemperature be based on the values that are available in the Tuning file? Or does the interpolator make this possible to extend beyond the limits calibrated if it can extrapolate for wider values? > MatrixInterpolator<double, 2, 1> gains; > int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > if (ret < 0) > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context, > << ", blue: " << awb.gains.manual.blue; > } > > + const auto &colourTemperature = controls.get(controls::ColourTemperature); > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) { > + Matrix<double, 2, 1> gains = gains_->get(*colourTemperature); > + awb.gains.manual.red = gains[0][0]; > + awb.gains.manual.blue = gains[1][0]; > + > + LOG(RkISP1Awb, Debug) > + << "Set colour gains to red: " << awb.gains.manual.red > + << ", blue: " << awb.gains.manual.blue; > + } > + > frameContext.awb.autoEnabled = awb.autoEnabled; > > if (!awb.autoEnabled) { > -- > 2.43.0 >
Hi Kieran, Thanks for the review. On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote: > Quoting Stefan Klug (2024-08-05 13:05:04) > > There are many use-cases (tuning-validation, working in static > > environments) where a manual ColourTemperature control is helpful. > > Implement that by interpolating and applying the white balance gains > > from the tuning file according to the requested colour temperature. If > > colour gains are provided on the same request, they take precedence. As > > the colour temperature reported in the metadata is always based on the > > measurements, we don't have to touch that. > > > > Note that in the automatic case, the colour gains are still based on the > > gray world model and the ones from the tuning file get ignored. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 957d24fe3425..d482eda5b541 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms { > > > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > > > +constexpr int32_t kMinColourTemperature = 2500; > > +constexpr int32_t kMaxColourTemperature = 10000; > > +constexpr int32_t kDefaultColourTemperature = 6500; > > + > > /* Minimum mean value below which AWB can't operate. */ > > constexpr double kMeanMinThreshold = 2.0; > > > > @@ -44,6 +48,11 @@ Awb::Awb() > > */ > > int Awb::init(IPAContext &context, const YamlObject &tuningData) > > { > > + auto &cmap = context.ctrlMap; > > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature, > > + kMaxColourTemperature, > > + kDefaultColourTemperature); > > + > > Should this control only be exposed if there are gains available in the > tuning file? As noted in the previous mail I think unconditional is ok as it also applies to the ccms which should be available. > > Can/Should the minColourTemperature/maxColourTemperature be based on the > values that are available in the Tuning file? Or does the interpolator > make this possible to extend beyond the limits calibrated if it can > extrapolate for wider values? The interpolator extends the values if the temperature out of range. Here we would also need to check the range of ccms and gains. I don't know if it's worth the effort. On the other hand the min/max values are arbitrary. A quick google indicates that we could even lower the min to 1000. > > > > > MatrixInterpolator<double, 2, 1> gains; > > int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > > if (ret < 0) > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context, > > << ", blue: " << awb.gains.manual.blue; > > } > > > > + const auto &colourTemperature = controls.get(controls::ColourTemperature); > > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) { > > + Matrix<double, 2, 1> gains = gains_->get(*colourTemperature); > > + awb.gains.manual.red = gains[0][0]; > > + awb.gains.manual.blue = gains[1][0]; > > + > > + LOG(RkISP1Awb, Debug) > > + << "Set colour gains to red: " << awb.gains.manual.red > > + << ", blue: " << awb.gains.manual.blue; > > + } > > + > > frameContext.awb.autoEnabled = awb.autoEnabled; > > > > if (!awb.autoEnabled) { > > -- > > 2.43.0 > >
Quoting Stefan Klug (2024-08-06 07:37:54) > Hi Kieran, > > Thanks for the review. > > On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote: > > Quoting Stefan Klug (2024-08-05 13:05:04) > > > There are many use-cases (tuning-validation, working in static > > > environments) where a manual ColourTemperature control is helpful. > > > Implement that by interpolating and applying the white balance gains > > > from the tuning file according to the requested colour temperature. If > > > colour gains are provided on the same request, they take precedence. As > > > the colour temperature reported in the metadata is always based on the > > > measurements, we don't have to touch that. > > > > > > Note that in the automatic case, the colour gains are still based on the > > > gray world model and the ones from the tuning file get ignored. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > index 957d24fe3425..d482eda5b541 100644 > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms { > > > > > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > > > > > +constexpr int32_t kMinColourTemperature = 2500; > > > +constexpr int32_t kMaxColourTemperature = 10000; > > > +constexpr int32_t kDefaultColourTemperature = 6500; > > > + > > > /* Minimum mean value below which AWB can't operate. */ > > > constexpr double kMeanMinThreshold = 2.0; > > > > > > @@ -44,6 +48,11 @@ Awb::Awb() > > > */ > > > int Awb::init(IPAContext &context, const YamlObject &tuningData) > > > { > > > + auto &cmap = context.ctrlMap; > > > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature, > > > + kMaxColourTemperature, > > > + kDefaultColourTemperature); > > > + > > > > Should this control only be exposed if there are gains available in the > > tuning file? > > As noted in the previous mail I think unconditional is ok as it also > applies to the ccms which should be available. So - perhaps we should be instantiating this control at the top level and not inside the awb. If someone disables the awb module in the tuning file, it would impact CCM ? > > Can/Should the minColourTemperature/maxColourTemperature be based on the > > values that are available in the Tuning file? Or does the interpolator > > make this possible to extend beyond the limits calibrated if it can > > extrapolate for wider values? > > The interpolator extends the values if the temperature out of range. > Here we would also need to check the range of ccms and gains. I don't > know if it's worth the effort. On the other hand the min/max values are > arbitrary. A quick google indicates that we could even lower the min to > 1000. > > > > MatrixInterpolator<double, 2, 1> gains; > > > int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > > > if (ret < 0) > > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context, > > > << ", blue: " << awb.gains.manual.blue; > > > } > > > > > > + const auto &colourTemperature = controls.get(controls::ColourTemperature); > > > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) { > > > + Matrix<double, 2, 1> gains = gains_->get(*colourTemperature); > > > + awb.gains.manual.red = gains[0][0]; > > > + awb.gains.manual.blue = gains[1][0]; > > > + > > > + LOG(RkISP1Awb, Debug) > > > + << "Set colour gains to red: " << awb.gains.manual.red > > > + << ", blue: " << awb.gains.manual.blue; > > > + } > > > + > > > frameContext.awb.autoEnabled = awb.autoEnabled; > > > > > > if (!awb.autoEnabled) { > > > -- > > > 2.43.0 > > >
Hi Kieran, On Tue, Aug 06, 2024 at 09:54:41AM +0100, Kieran Bingham wrote: > Quoting Stefan Klug (2024-08-06 07:37:54) > > Hi Kieran, > > > > Thanks for the review. > > > > On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote: > > > Quoting Stefan Klug (2024-08-05 13:05:04) > > > > There are many use-cases (tuning-validation, working in static > > > > environments) where a manual ColourTemperature control is helpful. > > > > Implement that by interpolating and applying the white balance gains > > > > from the tuning file according to the requested colour temperature. If > > > > colour gains are provided on the same request, they take precedence. As > > > > the colour temperature reported in the metadata is always based on the > > > > measurements, we don't have to touch that. > > > > > > > > Note that in the automatic case, the colour gains are still based on the > > > > gray world model and the ones from the tuning file get ignored. > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++ > > > > 1 file changed, 20 insertions(+) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > index 957d24fe3425..d482eda5b541 100644 > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms { > > > > > > > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > > > > > > > +constexpr int32_t kMinColourTemperature = 2500; > > > > +constexpr int32_t kMaxColourTemperature = 10000; > > > > +constexpr int32_t kDefaultColourTemperature = 6500; > > > > + > > > > /* Minimum mean value below which AWB can't operate. */ > > > > constexpr double kMeanMinThreshold = 2.0; > > > > > > > > @@ -44,6 +48,11 @@ Awb::Awb() > > > > */ > > > > int Awb::init(IPAContext &context, const YamlObject &tuningData) > > > > { > > > > + auto &cmap = context.ctrlMap; > > > > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature, > > > > + kMaxColourTemperature, > > > > + kDefaultColourTemperature); > > > > + > > > > > > Should this control only be exposed if there are gains available in the > > > tuning file? > > > > As noted in the previous mail I think unconditional is ok as it also > > applies to the ccms which should be available. > > So - perhaps we should be instantiating this control at the top level > and not inside the awb. If someone disables the awb module > in the tuning file, it would impact CCM ? > I thought about that a bit. In automatic mode, the colour temperature gets estimated in the awb module. So we would also need to move the AwbEnable handling to the top level. So code wise, the colour temperature would be set from two different places and the Awb handling would also be spread across two places. I don't think that's worth it. Cheers, Stefan > > > > Can/Should the minColourTemperature/maxColourTemperature be based on the > > > values that are available in the Tuning file? Or does the interpolator > > > make this possible to extend beyond the limits calibrated if it can > > > extrapolate for wider values? > > > > The interpolator extends the values if the temperature out of range. > > Here we would also need to check the range of ccms and gains. I don't > > know if it's worth the effort. On the other hand the min/max values are > > arbitrary. A quick google indicates that we could even lower the min to > > 1000. > > > > > > MatrixInterpolator<double, 2, 1> gains; > > > > int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > > > > if (ret < 0) > > > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context, > > > > << ", blue: " << awb.gains.manual.blue; > > > > } > > > > > > > > + const auto &colourTemperature = controls.get(controls::ColourTemperature); > > > > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) { > > > > + Matrix<double, 2, 1> gains = gains_->get(*colourTemperature); > > > > + awb.gains.manual.red = gains[0][0]; > > > > + awb.gains.manual.blue = gains[1][0]; > > > > + > > > > + LOG(RkISP1Awb, Debug) > > > > + << "Set colour gains to red: " << awb.gains.manual.red > > > > + << ", blue: " << awb.gains.manual.blue; > > > > + } > > > > + > > > > frameContext.awb.autoEnabled = awb.autoEnabled; > > > > > > > > if (!awb.autoEnabled) { > > > > -- > > > > 2.43.0 > > > >
Hi Stefan Thanks for resurrecting this topic. I seem to remember submitting something like this a looong time ago. Don't recall what, if anything, ever happened to it. But indeed, I think it's useful, so glad to see it's not been forgotten! On Tue, 13 Aug 2024 at 08:37, Stefan Klug <stefan.klug@ideasonboard.com> wrote: > > Hi Kieran, > > On Tue, Aug 06, 2024 at 09:54:41AM +0100, Kieran Bingham wrote: > > Quoting Stefan Klug (2024-08-06 07:37:54) > > > Hi Kieran, > > > > > > Thanks for the review. > > > > > > On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote: > > > > Quoting Stefan Klug (2024-08-05 13:05:04) > > > > > There are many use-cases (tuning-validation, working in static > > > > > environments) where a manual ColourTemperature control is helpful. > > > > > Implement that by interpolating and applying the white balance gains > > > > > from the tuning file according to the requested colour temperature. If > > > > > colour gains are provided on the same request, they take precedence. As > > > > > the colour temperature reported in the metadata is always based on the > > > > > measurements, we don't have to touch that. > > > > > > > > > > Note that in the automatic case, the colour gains are still based on the > > > > > gray world model and the ones from the tuning file get ignored. > > > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > > --- > > > > > src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++ > > > > > 1 file changed, 20 insertions(+) > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > > index 957d24fe3425..d482eda5b541 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms { > > > > > > > > > > LOG_DEFINE_CATEGORY(RkISP1Awb) > > > > > > > > > > +constexpr int32_t kMinColourTemperature = 2500; > > > > > +constexpr int32_t kMaxColourTemperature = 10000; > > > > > +constexpr int32_t kDefaultColourTemperature = 6500; > > > > > + > > > > > /* Minimum mean value below which AWB can't operate. */ > > > > > constexpr double kMeanMinThreshold = 2.0; > > > > > > > > > > @@ -44,6 +48,11 @@ Awb::Awb() > > > > > */ > > > > > int Awb::init(IPAContext &context, const YamlObject &tuningData) > > > > > { > > > > > + auto &cmap = context.ctrlMap; > > > > > + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature, > > > > > + kMaxColourTemperature, > > > > > + kDefaultColourTemperature); > > > > > + > > > > > > > > Should this control only be exposed if there are gains available in the > > > > tuning file? > > > > > > As noted in the previous mail I think unconditional is ok as it also > > > applies to the ccms which should be available. > > > > So - perhaps we should be instantiating this control at the top level > > and not inside the awb. If someone disables the awb module > > in the tuning file, it would impact CCM ? > > > > I thought about that a bit. In automatic mode, the colour temperature > gets estimated in the awb module. So we would also need to move the > AwbEnable handling to the top level. So code wise, the colour > temperature would be set from two different places and the Awb handling > would also be spread across two places. I don't think that's worth it. I think it suits us (RPi) better like this too. It's also like our AEC/AGC, where fixed values are actually handled by the algorithm even when you're in manual mode. Thanks David > > Cheers, > Stefan > > > > > > > Can/Should the minColourTemperature/maxColourTemperature be based on the > > > > values that are available in the Tuning file? Or does the interpolator > > > > make this possible to extend beyond the limits calibrated if it can > > > > extrapolate for wider values? > > > > > > The interpolator extends the values if the temperature out of range. > > > Here we would also need to check the range of ccms and gains. I don't > > > know if it's worth the effort. On the other hand the min/max values are > > > arbitrary. A quick google indicates that we could even lower the min to > > > 1000. > > > > > > > > MatrixInterpolator<double, 2, 1> gains; > > > > > int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > > > > > if (ret < 0) > > > > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context, > > > > > << ", blue: " << awb.gains.manual.blue; > > > > > } > > > > > > > > > > + const auto &colourTemperature = controls.get(controls::ColourTemperature); > > > > > + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) { > > > > > + Matrix<double, 2, 1> gains = gains_->get(*colourTemperature); > > > > > + awb.gains.manual.red = gains[0][0]; > > > > > + awb.gains.manual.blue = gains[1][0]; > > > > > + > > > > > + LOG(RkISP1Awb, Debug) > > > > > + << "Set colour gains to red: " << awb.gains.manual.red > > > > > + << ", blue: " << awb.gains.manual.blue; > > > > > + } > > > > > + > > > > > frameContext.awb.autoEnabled = awb.autoEnabled; > > > > > > > > > > if (!awb.autoEnabled) { > > > > > -- > > > > > 2.43.0 > > > > >
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 957d24fe3425..d482eda5b541 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms { LOG_DEFINE_CATEGORY(RkISP1Awb) +constexpr int32_t kMinColourTemperature = 2500; +constexpr int32_t kMaxColourTemperature = 10000; +constexpr int32_t kDefaultColourTemperature = 6500; + /* Minimum mean value below which AWB can't operate. */ constexpr double kMeanMinThreshold = 2.0; @@ -44,6 +48,11 @@ Awb::Awb() */ int Awb::init(IPAContext &context, const YamlObject &tuningData) { + auto &cmap = context.ctrlMap; + cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature, + kMaxColourTemperature, + kDefaultColourTemperature); + MatrixInterpolator<double, 2, 1> gains; int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); if (ret < 0) @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context, << ", blue: " << awb.gains.manual.blue; } + const auto &colourTemperature = controls.get(controls::ColourTemperature); + if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) { + Matrix<double, 2, 1> gains = gains_->get(*colourTemperature); + awb.gains.manual.red = gains[0][0]; + awb.gains.manual.blue = gains[1][0]; + + LOG(RkISP1Awb, Debug) + << "Set colour gains to red: " << awb.gains.manual.red + << ", blue: " << awb.gains.manual.blue; + } + frameContext.awb.autoEnabled = awb.autoEnabled; if (!awb.autoEnabled) {
There are many use-cases (tuning-validation, working in static environments) where a manual ColourTemperature control is helpful. Implement that by interpolating and applying the white balance gains from the tuning file according to the requested colour temperature. If colour gains are provided on the same request, they take precedence. As the colour temperature reported in the metadata is always based on the measurements, we don't have to touch that. Note that in the automatic case, the colour gains are still based on the gray world model and the ones from the tuning file get ignored. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)