<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your work.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 10 Feb 2021 at 11:17, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Applying the manual_r_ and manual_b_ values is entirely removed from<br>
the asynchronous thread where their use constituted a race hazard. The<br>
main thread now deals with them entirely, involving the following<br>
changes.<br>
<br>
1. SetManualGains() applies the new values directly to the<br>
"sync_results", meaning that Prepare() will jump to the new values<br>
immediately (which is a better behaviour).<br>
<br>
2. Process() does not restart the asynchronous thread when manual<br>
gains are in force.<br>
<br>
3. The asynchronous thread might be running when manual gains are set,<br>
so we ignore the results produced in this case.<br>
<br>
Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br></blockquote><div><br></div><div>I agree with Laurent's suggestion about having a isAutoEnabled() method. However, either way:</div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
src/ipa/raspberrypi/controller/rpi/awb.cpp | 78 ++++++++++------------<br>
1 file changed, 37 insertions(+), 41 deletions(-)<br>
<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
index 1c65eda8..46a8ce2a 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
@@ -193,28 +193,30 @@ void Awb::SetManualGains(double manual_r, double manual_b)<br>
// If any of these are 0.0, we swich back to auto.<br>
manual_r_ = manual_r;<br>
manual_b_ = manual_b;<br>
+ // If non-zero, set these values into the sync_results which means that<br>
+ // Prepare() will adopt them immediately.<br>
+ if (manual_r_ != 0.0 && manual_b_ != 0.0) {<br>
+ sync_results_.gain_r = prev_sync_results_.gain_r = manual_r_;<br>
+ sync_results_.gain_g = prev_sync_results_.gain_g = 1.0;<br>
+ sync_results_.gain_b = prev_sync_results_.gain_b = manual_b_;<br>
+ }<br>
}<br>
<br>
void Awb::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,<br>
Metadata *metadata)<br>
{<br>
- // If fixed colour gains have been set, we should let other algorithms<br>
- // know by writing it into the image metadata.<br>
- if (manual_r_ != 0.0 && manual_b_ != 0.0) {<br>
- prev_sync_results_.gain_r = manual_r_;<br>
- prev_sync_results_.gain_g = 1.0;<br>
- prev_sync_results_.gain_b = manual_b_;<br>
- // If we're starting up for the first time, try and<br>
- // "dead reckon" the corresponding colour temperature.<br>
- if (first_switch_mode_ && config_.bayes) {<br>
- Pwl ct_r_inverse = config_.ct_r.Inverse();<br>
- Pwl ct_b_inverse = config_.ct_b.Inverse();<br>
- double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_));<br>
- double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_));<br>
- prev_sync_results_.temperature_K = (ct_r + ct_b) / 2;<br>
- }<br>
- sync_results_ = prev_sync_results_;<br>
+ // On the first mode switch we'll have no meaningful colour<br>
+ // temperature, so try to dead reckon one.<br>
+ if (manual_r_ != 0.0 && manual_b_ != 0.0 &&<br>
+ first_switch_mode_ && config_.bayes) {<br>
+ Pwl ct_r_inverse = config_.ct_r.Inverse();<br>
+ Pwl ct_b_inverse = config_.ct_b.Inverse();<br>
+ double ct_r = ct_r_inverse.Eval(ct_r_inverse.Domain().Clip(1 / manual_r_));<br>
+ double ct_b = ct_b_inverse.Eval(ct_b_inverse.Domain().Clip(1 / manual_b_));<br>
+ prev_sync_results_.temperature_K = (ct_r + ct_b) / 2;<br>
+ sync_results_.temperature_K = prev_sync_results_.temperature_K;<br>
}<br>
+ // Let other algorithms know the current white balance values.<br>
metadata->Set("awb.status", prev_sync_results_);<br>
first_switch_mode_ = false;<br>
}<br>
@@ -224,7 +226,10 @@ void Awb::fetchAsyncResults()<br>
LOG(RPiAwb, Debug) << "Fetch AWB results";<br>
async_finished_ = false;<br>
async_started_ = false;<br>
- sync_results_ = async_results_;<br>
+ // It's possible manual gains could be set even while the async<br>
+ // thread was running. In this case, we ignore its results.<br>
+ if (manual_r_ == 0.0 || manual_b_ == 0.0)<br>
+ sync_results_ = async_results_;<br>
}<br>
<br>
void Awb::restartAsync(StatisticsPtr &stats, double lux)<br>
@@ -289,8 +294,10 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)<br>
if (frame_phase_ < (int)config_.frame_period)<br>
frame_phase_++;<br>
LOG(RPiAwb, Debug) << "frame_phase " << frame_phase_;<br>
- if (frame_phase_ >= (int)config_.frame_period ||<br>
- frame_count_ < (int)config_.startup_frames) {<br>
+ // We do not restart the async thread when we have fixed colour gains.<br>
+ if ((manual_r_ == 0.0 || manual_b_ == 0.0) &&<br>
+ (frame_phase_ >= (int)config_.frame_period ||<br>
+ frame_count_ < (int)config_.startup_frames)) {<br>
// Update any settings and any image metadata that we need.<br>
struct LuxStatus lux_status = {};<br>
lux_status.lux = 400; // in case no metadata<br>
@@ -614,29 +621,18 @@ void Awb::awbGrey()<br>
<br>
void Awb::doAwb()<br>
{<br>
- if (manual_r_ != 0.0 && manual_b_ != 0.0) {<br>
- async_results_.temperature_K = 4500; // don't know what it is<br>
- async_results_.gain_r = manual_r_;<br>
- async_results_.gain_g = 1.0;<br>
- async_results_.gain_b = manual_b_;<br>
+ prepareStats();<br>
+ LOG(RPiAwb, Debug) << "Valid zones: " << zones_.size();<br>
+ if (zones_.size() > config_.min_regions) {<br>
+ if (config_.bayes)<br>
+ awbBayes();<br>
+ else<br>
+ awbGrey();<br>
LOG(RPiAwb, Debug)<br>
- << "Using manual white balance: gain_r "<br>
- << async_results_.gain_r << " gain_b "<br>
- << async_results_.gain_b;<br>
- } else {<br>
- prepareStats();<br>
- LOG(RPiAwb, Debug) << "Valid zones: " << zones_.size();<br>
- if (zones_.size() > config_.min_regions) {<br>
- if (config_.bayes)<br>
- awbBayes();<br>
- else<br>
- awbGrey();<br>
- LOG(RPiAwb, Debug)<br>
- << "CT found is "<br>
- << async_results_.temperature_K<br>
- << " with gains r " << async_results_.gain_r<br>
- << " and b " << async_results_.gain_b;<br>
- }<br>
+ << "CT found is "<br>
+ << async_results_.temperature_K<br>
+ << " with gains r " << async_results_.gain_r<br>
+ << " and b " << async_results_.gain_b;<br>
}<br>
}<br>
<br>
-- <br>
2.20.1<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>