[libcamera-devel] [PATCH 2/2] ipa: raspberrypi: AWB: Fix race condition setting manual gains
Naushir Patuck
naush at raspberrypi.com
Wed Feb 10 18:20:10 CET 2021
Hi David,
Thank you for your work.
On Wed, 10 Feb 2021 at 11:17, David Plowman <david.plowman at 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 at raspberrypi.com>
>
I agree with Laurent's suggestion about having a isAutoEnabled() method.
However, either way:
Reviewed-by: Naushir Patuck <naush 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;
> }
> }
>
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210210/40f1ff0e/attachment-0001.htm>
More information about the libcamera-devel
mailing list