[libcamera-devel] [PATCH 2/2] ipa: raspberrypi: AWB: Fix race condition setting manual gains

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Wed, Feb 10, 2021 at 11:17:43AM +0000, David Plowman wrote:
> 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.

The code looks right to me,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

I'd appreciate a review from Naush if possible though.

A possible improvement would be to add a function to check if AWB is
enabled:

bool Awb::isAutoEnabled() const
{
	return manual_r_ == 0.0 || manual_b_ == 0.0;
}

The rest of the code will be easier to read. This could be done in a v2
of this patch, or in a separate patch.

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/awb.cpp | 78 ++++++++++------------
>  1 file changed, 37 insertions(+), 41 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> index 1c65eda8..46a8ce2a 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> @@ -193,28 +193,30 @@ 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 non-zero, set these values into the sync_results which means that
> +	// Prepare() will adopt them immediately.
> +	if (manual_r_ != 0.0 && manual_b_ != 0.0) {
> +		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
> -	// know by writing it into the image metadata.
> -	if (manual_r_ != 0.0 && manual_b_ != 0.0) {
> -		prev_sync_results_.gain_r = manual_r_;
> -		prev_sync_results_.gain_g = 1.0;
> -		prev_sync_results_.gain_b = manual_b_;
> -		// If we're starting up for the first time, try and
> -		// "dead reckon" the corresponding colour temperature.
> -		if (first_switch_mode_ && config_.bayes) {
> -			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_;
> +	// On the first mode switch we'll have no meaningful colour
> +	// temperature, so try to dead reckon one.
> +	if (manual_r_ != 0.0 && manual_b_ != 0.0 &&
> +	    first_switch_mode_ && config_.bayes) {
> +		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_.temperature_K = prev_sync_results_.temperature_K;
>  	}
> +	// Let other algorithms know the current white balance values.
>  	metadata->Set("awb.status", prev_sync_results_);
>  	first_switch_mode_ = false;
>  }
> @@ -224,7 +226,10 @@ 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. In this case, we ignore its results.
> +	if (manual_r_ == 0.0 || manual_b_ == 0.0)
> +		sync_results_ = async_results_;
>  }
>  
>  void Awb::restartAsync(StatisticsPtr &stats, double lux)
> @@ -289,8 +294,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 ||
> -	    frame_count_ < (int)config_.startup_frames) {
> +	// We do not restart the async thread when we have fixed colour gains.
> +	if ((manual_r_ == 0.0 || manual_b_ == 0.0) &&
> +	    (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 +621,18 @@ void Awb::awbGrey()
>  
>  void Awb::doAwb()
>  {
> -	if (manual_r_ != 0.0 && manual_b_ != 0.0) {
> -		async_results_.temperature_K = 4500; // don't know what it is
> -		async_results_.gain_r = manual_r_;
> -		async_results_.gain_g = 1.0;
> -		async_results_.gain_b = manual_b_;
> +	prepareStats();
> +	LOG(RPiAwb, Debug) << "Valid zones: " << zones_.size();
> +	if (zones_.size() > config_.min_regions) {
> +		if (config_.bayes)
> +			awbBayes();
> +		else
> +			awbGrey();
>  		LOG(RPiAwb, Debug)
> -			<< "Using manual white balance: gain_r "
> -			<< async_results_.gain_r << " gain_b "
> -			<< async_results_.gain_b;
> -	} else {
> -		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;
> -		}
> +			<< "CT found is "
> +			<< async_results_.temperature_K
> +			<< " with gains r " << async_results_.gain_r
> +			<< " and b " << async_results_.gain_b;
>  	}
>  }
>  

-- 
Regards,

Laurent Pinchart