| Message ID | 20241015203820.574326-3-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2024-10-15 21:38:13) > Sometimes the ISP produces statistics only with a subset of statistic > types being valid. It doesn't happen often, but it happens. Check for > the RKISP1_CIF_ISP_STAT_AWB bit to prevent using invalid or outdated > data. > > For simpler code structure, the ColourTemperature metadata entry gets > written unconditionally and overwritten later if needed. > This makes me wonder if we need to better handle how metadata is filled by algorithms too ... but I don't object to this as it stands anyway. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 955a9ff4a897..86bcf20f6527 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -215,6 +215,10 @@ void Awb::process(IPAContext &context, > static_cast<float>(frameContext.awb.gains.red), > static_cast<float>(frameContext.awb.gains.blue) > }); > + metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > + > + if (!(stats && stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) > + return; > > if (rgbMode_) { > greenMean = awb->awb_mean[0].mean_y_or_g; > @@ -270,10 +274,8 @@ void Awb::process(IPAContext &context, > * meaningfully calculate gains. Freeze the algorithm in that case. > */ > if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && > - blueMean < kMeanMinThreshold) { > - metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > + blueMean < kMeanMinThreshold) > return; > - } > > activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > > -- > 2.43.0 >
Hi Stefan, Thank you for the patch. On Tue, Oct 15, 2024 at 10:38:13PM +0200, Stefan Klug wrote: > Sometimes the ISP produces statistics only with a subset of statistic > types being valid. It doesn't happen often, but it happens. Check for > the RKISP1_CIF_ISP_STAT_AWB bit to prevent using invalid or outdated > data. > > For simpler code structure, the ColourTemperature metadata entry gets > written unconditionally and overwritten later if needed. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 955a9ff4a897..86bcf20f6527 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -215,6 +215,10 @@ void Awb::process(IPAContext &context, > static_cast<float>(frameContext.awb.gains.red), > static_cast<float>(frameContext.awb.gains.blue) > }); > + metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > + > + if (!(stats && stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) As in 1/4, I think if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) is more readable. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + return; > > if (rgbMode_) { > greenMean = awb->awb_mean[0].mean_y_or_g; > @@ -270,10 +274,8 @@ void Awb::process(IPAContext &context, > * meaningfully calculate gains. Freeze the algorithm in that case. > */ > if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && > - blueMean < kMeanMinThreshold) { > - metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > + blueMean < kMeanMinThreshold) > return; > - } > > activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); >
On Wed, Oct 16, 2024 at 05:44:19PM +0300, Laurent Pinchart wrote: > Hi Stefan, > > Thank you for the patch. > > On Tue, Oct 15, 2024 at 10:38:13PM +0200, Stefan Klug wrote: > > Sometimes the ISP produces statistics only with a subset of statistic > > types being valid. It doesn't happen often, but it happens. Check for > > the RKISP1_CIF_ISP_STAT_AWB bit to prevent using invalid or outdated > > data. > > > > For simpler code structure, the ColourTemperature metadata entry gets > > written unconditionally and overwritten later if needed. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 955a9ff4a897..86bcf20f6527 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -215,6 +215,10 @@ void Awb::process(IPAContext &context, > > static_cast<float>(frameContext.awb.gains.red), > > static_cast<float>(frameContext.awb.gains.blue) > > }); > > + metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > > + > > + if (!(stats && stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) > > As in 1/4, I think > > if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) +1 Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > is more readable. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + return; > > > > if (rgbMode_) { > > greenMean = awb->awb_mean[0].mean_y_or_g; > > @@ -270,10 +274,8 @@ void Awb::process(IPAContext &context, > > * meaningfully calculate gains. Freeze the algorithm in that case. > > */ > > if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && > > - blueMean < kMeanMinThreshold) { > > - metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > > + blueMean < kMeanMinThreshold) > > return; > > - } > > > > activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 955a9ff4a897..86bcf20f6527 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -215,6 +215,10 @@ void Awb::process(IPAContext &context, static_cast<float>(frameContext.awb.gains.red), static_cast<float>(frameContext.awb.gains.blue) }); + metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); + + if (!(stats && stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) + return; if (rgbMode_) { greenMean = awb->awb_mean[0].mean_y_or_g; @@ -270,10 +274,8 @@ void Awb::process(IPAContext &context, * meaningfully calculate gains. Freeze the algorithm in that case. */ if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold && - blueMean < kMeanMinThreshold) { - metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); + blueMean < kMeanMinThreshold) return; - } activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
Sometimes the ISP produces statistics only with a subset of statistic types being valid. It doesn't happen often, but it happens. Check for the RKISP1_CIF_ISP_STAT_AWB bit to prevent using invalid or outdated data. For simpler code structure, the ColourTemperature metadata entry gets written unconditionally and overwritten later if needed. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)