ipa: raspberrypi: AWB: Fix race condition setting manual gains

Applying the manual_r_ and manual_b_ values is entirely removed from
the asynchronous thread where their use constituted a race hazard. The
main thread now deals with them entirely, involving the following
changes.

1. SetManualGains() applies the new values directly to the
"sync_results", meaning that Prepare() will jump to the new values
immediately (which is a better behaviour).

2. Process() does not restart the asynchronous thread when manual
gains are in force.

3. The asynchronous thread might be running when manual gains are set,
so we ignore the results produced in this case.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

src/ipa/raspberrypi/controller/rpi/awb.cpp

@@ -175,9 +175,9 @@ void Awb::Initialise()
 
 unsigned int Awb::GetConvergenceFrames() const
 {
-	// If colour gains have been explicitly set, there is no convergence
+	// If not in auto mode, there is no convergence
 	// to happen, so no need to drop any frames - return zero.
-	if (manual_r_ && manual_b_)
+	if (!isAutoEnabled())
 		return 0;
 	else
 		return config_.convergence_frames;
@@ -193,38 +193,47 @@ void Awb::SetManualGains(double manual_r, double manual_b)
 	// If any of these are 0.0, we swich back to auto.
 	manual_r_ = manual_r;
 	manual_b_ = manual_b;
+	// If not in auto mode, set these values into the sync_results which
+	// means that Prepare() will adopt them immediately.
+	if (!isAutoEnabled()) {
+		sync_results_.gain_r = prev_sync_results_.gain_r = manual_r_;
+		sync_results_.gain_g = prev_sync_results_.gain_g = 1.0;
+		sync_results_.gain_b = prev_sync_results_.gain_b = manual_b_;
+	}
 }
 
 void Awb::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,
 		     Metadata *metadata)
 {
-	// If fixed colour gains have been set, we should let other algorithms
+	// On the first mode switch we'll have no meaningful colour
-	// know by writing it into the image metadata.
+	// temperature, so try to dead reckon one if in manual mode.
-	if (manual_r_ != 0.0 && manual_b_ != 0.0) {
+	if (!isAutoEnabled() && first_switch_mode_ && config_.bayes) {
-		prev_sync_results_.gain_r = manual_r_;
+		Pwl ct_r_inverse = config_.ct_r.Inverse();
-		prev_sync_results_.gain_g = 1.0;
+		Pwl ct_b_inverse = config_.ct_b.Inverse();
-		prev_sync_results_.gain_b = manual_b_;
+		double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_));
-		// If we're starting up for the first time, try and
+		double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_));
-		// "dead reckon" the corresponding colour temperature.
+		prev_sync_results_.temperature_K = (ct_r + ct_b) / 2;
-		if (first_switch_mode_ && config_.bayes) {
+		sync_results_.temperature_K = prev_sync_results_.temperature_K;
-			Pwl ct_r_inverse = config_.ct_r.Inverse();
-			Pwl ct_b_inverse = config_.ct_b.Inverse();
-			double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_));
-			double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_));
-			prev_sync_results_.temperature_K = (ct_r + ct_b) / 2;
-		}
-		sync_results_ = prev_sync_results_;
 	}
+	// Let other algorithms know the current white balance values.
 	metadata->Set("awb.status", prev_sync_results_);
 	first_switch_mode_ = false;
 }
 
+bool Awb::isAutoEnabled() const
+{
+	return manual_r_ == 0.0 || manual_b_ == 0.0;
+}
+
 void Awb::fetchAsyncResults()
 {
 	LOG(RPiAwb, Debug) << "Fetch AWB results";
 	async_finished_ = false;
 	async_started_ = false;
-	sync_results_ = async_results_;
+	// It's possible manual gains could be set even while the async
+	// thread was running, so only copy the results if still in auto mode.
+	if (isAutoEnabled())
+		sync_results_ = async_results_;
 }
 
 void Awb::restartAsync(StatisticsPtr &stats, double lux)
@@ -289,8 +298,10 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
 	if (frame_phase_ < (int)config_.frame_period)
 		frame_phase_++;
 	LOG(RPiAwb, Debug) << "frame_phase " << frame_phase_;
-	if (frame_phase_ >= (int)config_.frame_period ||
+	// We do not restart the async thread if we're not in auto mode.
-	    frame_count_ < (int)config_.startup_frames) {
+	if (isAutoEnabled() &&
+	    (frame_phase_ >= (int)config_.frame_period ||
+	     frame_count_ < (int)config_.startup_frames)) {
 		// Update any settings and any image metadata that we need.
 		struct LuxStatus lux_status = {};
 		lux_status.lux = 400; // in case no metadata
@@ -614,29 +625,18 @@ void Awb::awbGrey()
 
 void Awb::doAwb()
 {
-	if (manual_r_ != 0.0 && manual_b_ != 0.0) {
+	prepareStats();
-		async_results_.temperature_K = 4500; // don't know what it is
+	LOG(RPiAwb, Debug) << "Valid zones: " << zones_.size();
-		async_results_.gain_r = manual_r_;
+	if (zones_.size() > config_.min_regions) {
-		async_results_.gain_g = 1.0;
+		if (config_.bayes)
-		async_results_.gain_b = manual_b_;
+			awbBayes();
+		else
+			awbGrey();
 		LOG(RPiAwb, Debug)
-			<< "Using manual white balance: gain_r "
+			<< "CT found is "
-			<< async_results_.gain_r << " gain_b "
+			<< async_results_.temperature_K
-			<< async_results_.gain_b;
+			<< " with gains r " << async_results_.gain_r
-	} else {
+			<< " and b " << async_results_.gain_b;
-		prepareStats();
-		LOG(RPiAwb, Debug) << "Valid zones: " << zones_.size();
-		if (zones_.size() > config_.min_regions) {
-			if (config_.bayes)
-				awbBayes();
-			else
-				awbGrey();
-			LOG(RPiAwb, Debug)
-				<< "CT found is "
-				<< async_results_.temperature_K
-				<< " with gains r " << async_results_.gain_r
-				<< " and b " << async_results_.gain_b;
-		}
 	}
 }
 

src/ipa/raspberrypi/controller/rpi/awb.hpp

@@ -108,6 +108,7 @@ public:
 	};
 
 private:
+	bool isAutoEnabled() const;
 	// configuration is read-only, and available to both threads
 	AwbConfig config_;
 	std::thread async_thread_;