<div dir="ltr"><div dir="ltr">Hi David,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 16 Nov 2020 at 16:49, 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">On the libcamera/VC4 platform the AGC Prepare/Process methods, and any<br>
changes to the AGC settings, run synchronously - so a number of<br>
mutexes and copies are unnecessary and can be removed.<br>
<br>
Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
---<br>
src/ipa/raspberrypi/controller/rpi/agc.cpp | 99 ++++++++--------------<br>
src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +-<br>
2 files changed, 37 insertions(+), 67 deletions(-)<br>
<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
index 8079345b..c4379b99 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
@@ -160,7 +160,6 @@ Agc::Agc(Controller *controller)<br>
status_.total_exposure_value = 0.0;<br>
status_.target_exposure_value = 0.0;<br>
status_.locked = false;<br>
- output_status_ = status_;<br>
}<br>
<br>
char const *Agc::Name() const<br>
@@ -185,43 +184,36 @@ void Agc::Read(boost::property_tree::ptree const ¶ms)<br>
<br>
void Agc::SetEv(double ev)<br>
{<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
ev_ = ev;<br>
}<br>
<br>
void Agc::SetFlickerPeriod(double flicker_period)<br>
{<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
flicker_period_ = flicker_period;<br>
}<br>
<br>
void Agc::SetFixedShutter(double fixed_shutter)<br>
{<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
fixed_shutter_ = fixed_shutter;<br>
}<br>
<br>
void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)<br>
{<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
fixed_analogue_gain_ = fixed_analogue_gain;<br>
}<br>
<br>
void Agc::SetMeteringMode(std::string const &metering_mode_name)<br>
{<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
metering_mode_name_ = metering_mode_name;<br>
}<br>
<br>
void Agc::SetExposureMode(std::string const &exposure_mode_name)<br>
{<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
exposure_mode_name_ = exposure_mode_name;<br>
}<br>
<br>
void Agc::SetConstraintMode(std::string const &constraint_mode_name)<br>
{<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
constraint_mode_name_ = constraint_mode_name;<br>
}<br>
<br>
@@ -240,14 +232,9 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,<br>
<br>
void Agc::Prepare(Metadata *image_metadata)<br>
{<br>
- AgcStatus status;<br>
- {<br>
- std::unique_lock<std::mutex> lock(output_mutex_);<br>
- status = output_status_;<br>
- }<br>
int lock_count = lock_count_;<br>
lock_count_ = 0;<br>
- status.digital_gain = 1.0;<br>
+ status_.digital_gain = 1.0;<br>
if (status_.total_exposure_value) {<br>
// Process has run, so we have meaningful values.<br>
DeviceStatus device_status;<br>
@@ -255,48 +242,46 @@ void Agc::Prepare(Metadata *image_metadata)<br>
double actual_exposure = device_status.shutter_speed *<br>
device_status.analogue_gain;<br>
if (actual_exposure) {<br>
- status.digital_gain =<br>
+ status_.digital_gain =<br>
status_.total_exposure_value /<br>
actual_exposure;<br>
LOG(RPiAgc, Debug) << "Want total exposure " << status_.total_exposure_value;<br>
// Never ask for a gain < 1.0, and also impose<br>
// some upper limit. Make it customisable?<br>
- status.digital_gain = std::max(<br>
+ status_.digital_gain = std::max(<br>
1.0,<br>
- std::min(status.digital_gain, 4.0));<br>
+ std::min(status_.digital_gain, 4.0));<br>
LOG(RPiAgc, Debug) << "Actual exposure " << actual_exposure;<br>
- LOG(RPiAgc, Debug) << "Use digital_gain " << status.digital_gain;<br>
- LOG(RPiAgc, Debug) << "Effective exposure " << actual_exposure * status.digital_gain;<br>
+ LOG(RPiAgc, Debug) << "Use digital_gain " << status_.digital_gain;<br>
+ LOG(RPiAgc, Debug) << "Effective exposure " << actual_exposure * status_.digital_gain;<br>
// Decide whether AEC/AGC has converged.<br>
// Insist AGC is steady for MAX_LOCK_COUNT<br>
// frames before we say we are "locked".<br>
// (The hard-coded constants may need to<br>
// become customisable.)<br>
- if (status.target_exposure_value) {<br>
+ if (status_.target_exposure_value) {<br>
#define MAX_LOCK_COUNT 3<br>
- double err = 0.10 * status.target_exposure_value + 200;<br>
+ double err = 0.10 * status_.target_exposure_value + 200;<br>
if (actual_exposure <<br>
- status.target_exposure_value + err<br>
- && actual_exposure ><br>
- status.target_exposure_value - err)<br>
+ status_.target_exposure_value + err &&<br>
+ actual_exposure ><br>
+ status_.target_exposure_value - err)<br>
lock_count_ =<br>
std::min(lock_count + 1,<br>
- MAX_LOCK_COUNT);<br>
+ MAX_LOCK_COUNT);<br>
else if (actual_exposure <<br>
- status.target_exposure_value<br>
- + 1.5 * err &&<br>
+ status_.target_exposure_value + 1.5 * err &&<br>
actual_exposure ><br>
- status.target_exposure_value<br>
- - 1.5 * err)<br>
+ status_.target_exposure_value - 1.5 * err)<br>
lock_count_ = lock_count;<br>
LOG(RPiAgc, Debug) << "Lock count: " << lock_count_;<br>
}<br>
}<br>
} else<br>
LOG(RPiAgc, Debug) << Name() << ": no device metadata";<br>
- status.locked = lock_count_ >= MAX_LOCK_COUNT;<br>
- //printf("%s\n", status.locked ? "+++++++++" : "-");<br>
- image_metadata->Set("agc.status", status);<br>
+ status_.locked = lock_count_ >= MAX_LOCK_COUNT;<br>
+ //printf("%s\n", status_.locked ? "+++++++++" : "-");<br></blockquote><div><br></div><div>Is it worth getting rid of this commented out printf, or perhaps changing to a trace log point?</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">
+ image_metadata->Set("agc.status", status_);<br>
}<br>
}<br>
<br>
@@ -335,55 +320,47 @@ static void copy_string(std::string const &s, char *d, size_t size)<br>
void Agc::housekeepConfig()<br>
{<br>
// First fetch all the up-to-date settings, so no one else has to do it.<br>
- std::string new_exposure_mode_name, new_constraint_mode_name,<br>
- new_metering_mode_name;<br>
- {<br>
- std::unique_lock<std::mutex> lock(settings_mutex_);<br>
- new_metering_mode_name = metering_mode_name_;<br>
- new_exposure_mode_name = exposure_mode_name_;<br>
- new_constraint_mode_name = constraint_mode_name_;<br>
- status_.ev = ev_;<br>
- status_.fixed_shutter = fixed_shutter_;<br>
- status_.fixed_analogue_gain = fixed_analogue_gain_;<br>
- status_.flicker_period = flicker_period_;<br>
- }<br>
+ status_.ev = ev_;<br>
+ status_.fixed_shutter = fixed_shutter_;<br>
+ status_.fixed_analogue_gain = fixed_analogue_gain_;<br>
+ status_.flicker_period = flicker_period_;<br>
LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixed_shutter "<br>
<< status_.fixed_shutter << " fixed_analogue_gain "<br>
<< status_.fixed_analogue_gain;<br>
// Make sure the "mode" pointers point to the up-to-date things, if<br>
// they've changed.<br>
- if (strcmp(new_metering_mode_name.c_str(), status_.metering_mode)) {<br>
- auto it = config_.metering_modes.find(new_metering_mode_name);<br>
+ if (strcmp(metering_mode_name_.c_str(), status_.metering_mode)) {<br>
+ auto it = config_.metering_modes.find(metering_mode_name_);<br>
if (it == config_.metering_modes.end())<br>
throw std::runtime_error("Agc: no metering mode " +<br>
- new_metering_mode_name);<br>
+ metering_mode_name_);<br>
metering_mode_ = &it->second;<br>
- copy_string(new_metering_mode_name, status_.metering_mode,<br>
+ copy_string(metering_mode_name_, status_.metering_mode,<br>
sizeof(status_.metering_mode));<br>
}<br>
- if (strcmp(new_exposure_mode_name.c_str(), status_.exposure_mode)) {<br>
- auto it = config_.exposure_modes.find(new_exposure_mode_name);<br>
+ if (strcmp(exposure_mode_name_.c_str(), status_.exposure_mode)) {<br>
+ auto it = config_.exposure_modes.find(exposure_mode_name_);<br>
if (it == config_.exposure_modes.end())<br>
throw std::runtime_error("Agc: no exposure profile " +<br>
- new_exposure_mode_name);<br>
+ exposure_mode_name_);<br>
exposure_mode_ = &it->second;<br>
- copy_string(new_exposure_mode_name, status_.exposure_mode,<br>
+ copy_string(exposure_mode_name_, status_.exposure_mode,<br>
sizeof(status_.exposure_mode));<br>
}<br>
- if (strcmp(new_constraint_mode_name.c_str(), status_.constraint_mode)) {<br>
+ if (strcmp(constraint_mode_name_.c_str(), status_.constraint_mode)) {<br>
auto it =<br>
- config_.constraint_modes.find(new_constraint_mode_name);<br>
+ config_.constraint_modes.find(constraint_mode_name_);<br>
if (it == config_.constraint_modes.end())<br>
throw std::runtime_error("Agc: no constraint list " +<br>
- new_constraint_mode_name);<br>
+ constraint_mode_name_);<br>
constraint_mode_ = &it->second;<br>
- copy_string(new_constraint_mode_name, status_.constraint_mode,<br>
+ copy_string(constraint_mode_name_, status_.constraint_mode,<br>
sizeof(status_.constraint_mode));<br>
}<br>
LOG(RPiAgc, Debug) << "exposure_mode "<br>
- << new_exposure_mode_name << " constraint_mode "<br>
- << new_constraint_mode_name << " metering_mode "<br>
- << new_metering_mode_name;<br>
+ << exposure_mode_name_ << " constraint_mode "<br>
+ << constraint_mode_name_ << " metering_mode "<br>
+ << metering_mode_name_;<br>
}<br>
<br>
void Agc::fetchCurrentExposure(Metadata *image_metadata)<br>
@@ -638,10 +615,6 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate)<br>
status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg;<br>
status_.shutter_time = filtered_.shutter;<br>
status_.analogue_gain = filtered_.analogue_gain;<br>
- {<br>
- std::unique_lock<std::mutex> lock(output_mutex_);<br>
- output_status_ = status_;<br>
- }<br>
// Write to metadata as well, in case anyone wants to update the camera<br>
// immediately.<br>
image_metadata->Set("agc.status", status_);<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
index ba7ae092..5a02df4e 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
@@ -106,12 +106,9 @@ private:<br>
ExposureValues current_; // values for the current frame<br>
ExposureValues target_; // calculate the values we want here<br>
ExposureValues filtered_; // these values are filtered towards target<br>
- AgcStatus status_; // to "latch" settings so they can't change<br>
- AgcStatus output_status_; // the status we will write out<br>
- std::mutex output_mutex_;<br>
+ AgcStatus status_;<br>
int lock_count_;<br>
// Below here the "settings" that applications can change.<br>
- std::mutex settings_mutex_;<br>
std::string metering_mode_name_;<br>
std::string exposure_mode_name_;<br>
std::string constraint_mode_name_;<br>
-- <br>
2.20.1<br></blockquote><div><br></div><div>Apart from the minor nit,</div><div><br></div><div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div></div></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>
_______________________________________________<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>