| Message ID | 20250217100203.297894-8-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2025-02-17 10:01:48) > Damp the regulation of the color temperature with the same factor as the > gains. Not damping the color temperature leads to visible flicker, as > the CCM changes too much. Interesting, I thought we had something on the CCM interplotator so it wouldn't change too much on small movements of the colour temperature, but I guess smoothing out the colour temperature is reasonable anyway - especially as it's an estimate. I wonder if we should have some sort of IIR instances to make these a bit more specific, but the speed is fine now. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Or maybe we should report the real calculated value to metadata, but filter it on the input to the CCM interpolator ? > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 9244a1e64f41..347d38d226b4 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -320,8 +320,6 @@ void Awb::process(IPAContext &context, > rgbMeans.b() < kMeanMinThreshold) > return; > > - activeState.awb.automatic.temperatureK = estimateCCT(rgbMeans); > - > /* > * Estimate the red and blue gains to apply in a grey world. The green > * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the > @@ -343,8 +341,11 @@ void Awb::process(IPAContext &context, > > /* Filter the values to avoid oscillations. */ > double speed = 0.2; > + double ct = estimateCCT(rgbMeans); > + ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed); > gains = gains * speed + activeState.awb.automatic.gains * (1 - speed); > > + activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct); > activeState.awb.automatic.gains = gains; > > LOG(RkISP1Awb, Debug) > -- > 2.43.0 >
Hi Kieran, Thank you for the review. On Thu, Feb 20, 2025 at 12:20:18PM +0000, Kieran Bingham wrote: > Quoting Stefan Klug (2025-02-17 10:01:48) > > Damp the regulation of the color temperature with the same factor as the > > gains. Not damping the color temperature leads to visible flicker, as > > the CCM changes too much. > > Interesting, I thought we had something on the CCM interplotator so it > wouldn't change too much on small movements of the colour temperature, > but I guess smoothing out the colour temperature is reasonable anyway - > especially as it's an estimate. I wonder if we should have some sort of > IIR instances to make these a bit more specific, but the speed is fine > now. The interpolator has support for quantizations. On lsc we use that. But I'm not really confident if that is the best strategy. The damping is merely to damp larger spikes, which wouldn't get caught by quantization. With the IIR instance you are completely right. I even had some prototype somewhere. But properly tuning an IIR filter can also be a pain :-) > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Or maybe we should report the real calculated value to metadata, but > filter it on the input to the CCM interpolator ? I'm quite undecided here. The filtered data might be better than the unfiltered one? I think I'd stick to the filtered one, as it is in sync with the gains. Regards, Stefan > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 9244a1e64f41..347d38d226b4 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -320,8 +320,6 @@ void Awb::process(IPAContext &context, > > rgbMeans.b() < kMeanMinThreshold) > > return; > > > > - activeState.awb.automatic.temperatureK = estimateCCT(rgbMeans); > > - > > /* > > * Estimate the red and blue gains to apply in a grey world. The green > > * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the > > @@ -343,8 +341,11 @@ void Awb::process(IPAContext &context, > > > > /* Filter the values to avoid oscillations. */ > > double speed = 0.2; > > + double ct = estimateCCT(rgbMeans); > > + ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed); > > gains = gains * speed + activeState.awb.automatic.gains * (1 - speed); > > > > + activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct); > > activeState.awb.automatic.gains = gains; > > > > LOG(RkISP1Awb, Debug) > > -- > > 2.43.0 > >
Quoting Stefan Klug (2025-03-19 15:33:48) > Hi Kieran, > > Thank you for the review. > > On Thu, Feb 20, 2025 at 12:20:18PM +0000, Kieran Bingham wrote: > > Quoting Stefan Klug (2025-02-17 10:01:48) > > > Damp the regulation of the color temperature with the same factor as the > > > gains. Not damping the color temperature leads to visible flicker, as > > > the CCM changes too much. > > > > Interesting, I thought we had something on the CCM interplotator so it > > wouldn't change too much on small movements of the colour temperature, > > but I guess smoothing out the colour temperature is reasonable anyway - > > especially as it's an estimate. I wonder if we should have some sort of > > IIR instances to make these a bit more specific, but the speed is fine > > now. > > The interpolator has support for quantizations. On lsc we use that. But > I'm not really confident if that is the best strategy. The damping is > merely to damp larger spikes, which wouldn't get caught by quantization. > With the IIR instance you are completely right. I even had some > prototype somewhere. But properly tuning an IIR filter can also be a > pain :-) Yes, indeed! they're difficult to get right for sure. No need to complicate things here now. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > Or maybe we should report the real calculated value to metadata, but > > filter it on the input to the CCM interpolator ? > > I'm quite undecided here. The filtered data might be better than the > unfiltered one? I think I'd stick to the filtered one, as it is in sync > with the gains. I think that makes a lot of sense! If we report a colour temperature, we should make sure the gains we use correspond to it! > > Regards, > Stefan > > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/awb.cpp | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > index 9244a1e64f41..347d38d226b4 100644 > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > @@ -320,8 +320,6 @@ void Awb::process(IPAContext &context, > > > rgbMeans.b() < kMeanMinThreshold) > > > return; > > > > > > - activeState.awb.automatic.temperatureK = estimateCCT(rgbMeans); > > > - > > > /* > > > * Estimate the red and blue gains to apply in a grey world. The green > > > * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the > > > @@ -343,8 +341,11 @@ void Awb::process(IPAContext &context, > > > > > > /* Filter the values to avoid oscillations. */ > > > double speed = 0.2; > > > + double ct = estimateCCT(rgbMeans); > > > + ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed); > > > gains = gains * speed + activeState.awb.automatic.gains * (1 - speed); > > > > > > + activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct); > > > activeState.awb.automatic.gains = gains; > > > > > > LOG(RkISP1Awb, Debug) > > > -- > > > 2.43.0 > > >
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 9244a1e64f41..347d38d226b4 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -320,8 +320,6 @@ void Awb::process(IPAContext &context, rgbMeans.b() < kMeanMinThreshold) return; - activeState.awb.automatic.temperatureK = estimateCCT(rgbMeans); - /* * Estimate the red and blue gains to apply in a grey world. The green * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the @@ -343,8 +341,11 @@ void Awb::process(IPAContext &context, /* Filter the values to avoid oscillations. */ double speed = 0.2; + double ct = estimateCCT(rgbMeans); + ct = ct * speed + activeState.awb.automatic.temperatureK * (1 - speed); gains = gains * speed + activeState.awb.automatic.gains * (1 - speed); + activeState.awb.automatic.temperatureK = static_cast<unsigned int>(ct); activeState.awb.automatic.gains = gains; LOG(RkISP1Awb, Debug)
Damp the regulation of the color temperature with the same factor as the gains. Not damping the color temperature leads to visible flicker, as the CCM changes too much. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)