[v2,2/4] ipa: rkisp1: algorithms: awb: Check for correct stats type
diff mbox series

Message ID 20241018145856.914816-3-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • A few small fixes
Related show

Commit Message

Stefan Klug Oct. 18, 2024, 2:58 p.m. UTC 
Sometimes the ISP produces statistics only with a subset of statistic
types being valid. It doesn't happen normally, but was observed in the
wild. Check for the RKISP1_CIF_ISP_STAT_AWB bit to prevent using invalid
or outdated data. As it doesn't happen regularly add an error message to
get notified when it happens.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- Added error message
- Made condition more readable
- Collected tags
---
 src/ipa/rkisp1/algorithms/awb.cpp | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Kieran Bingham Oct. 18, 2024, 3:08 p.m. UTC | #1 
Quoting Stefan Klug (2024-10-18 15:58:36)
> Sometimes the ISP produces statistics only with a subset of statistic
> types being valid. It doesn't happen normally, but was observed in the
> wild. Check for the RKISP1_CIF_ISP_STAT_AWB bit to prevent using invalid
> or outdated data. As it doesn't happen regularly add an error message to
> get notified when it happens.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - Added error message
> - Made condition more readable
> - Collected tags
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 955a9ff4a897..b3c00bef9b7e 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -215,6 +215,12 @@ 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)) {
> +               LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";

Same here ?

If we want to treat one as valid and one as erroneous - we might need to
break it into two conditionals, with

	if (!stats)
		return;

	if (!(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {
		LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
		return;
	}

As one path is an acceptable silent path, but the other is a condition
we want to know about.

> +               return;
> +       }
>  
>         if (rgbMode_) {
>                 greenMean = awb->awb_mean[0].mean_y_or_g;
> @@ -270,10 +276,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
>
Stefan Klug Oct. 18, 2024, 3:28 p.m. UTC | #2 
On Fri, Oct 18, 2024 at 04:08:22PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-10-18 15:58:36)
> > Sometimes the ISP produces statistics only with a subset of statistic
> > types being valid. It doesn't happen normally, but was observed in the
> > wild. Check for the RKISP1_CIF_ISP_STAT_AWB bit to prevent using invalid
> > or outdated data. As it doesn't happen regularly add an error message to
> > get notified when it happens.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - Added error message
> > - Made condition more readable
> > - Collected tags
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 955a9ff4a897..b3c00bef9b7e 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -215,6 +215,12 @@ 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)) {
> > +               LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
> 
> Same here ?

No, because the awb algorithm has supportsRaw_=false. So I would keep
that as is.

> 
> If we want to treat one as valid and one as erroneous - we might need to
> break it into two conditionals, with
> 
> 	if (!stats)
> 		return;
> 
> 	if (!(stats->meas_type & RKISP1_CIF_ISP_STAT_AWB)) {
> 		LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
> 		return;
> 	}
> 
> As one path is an acceptable silent path, but the other is a condition
> we want to know about.

Yes, that will go in v3 (for agc).

Thanks.

Stefan

> 
> > +               return;
> > +       }
> >  
> >         if (rgbMode_) {
> >                 greenMean = awb->awb_mean[0].mean_y_or_g;
> > @@ -270,10 +276,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
> >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
index 955a9ff4a897..b3c00bef9b7e 100644
--- a/src/ipa/rkisp1/algorithms/awb.cpp
+++ b/src/ipa/rkisp1/algorithms/awb.cpp
@@ -215,6 +215,12 @@  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)) {
+		LOG(RkISP1Awb, Error) << "AWB data is missing in statistics";
+		return;
+	}
 
 	if (rgbMode_) {
 		greenMean = awb->awb_mean[0].mean_y_or_g;
@@ -270,10 +276,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);