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

11207 diff mbox series

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>
---
 src/ipa/raspberrypi/controller/rpi/awb.cpp | 78 ++++++++++------------
 1 file changed, 37 insertions(+), 41 deletions(-)

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@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@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;
>  	}
>  }
>

Hi David,

Thank you for your work.

On Wed, 10 Feb 2021 at 11:17, David Plowman <david.plowman@raspberrypi.com>
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.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

I agree with Laurent's suggestion about having a isAutoEnabled() method.
However, either way:

Reviewed-by: Naushir Patuck <naush@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;
>         }
>  }
>
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>