<div dir="ltr"><div dir="ltr">Hi David,</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">Previously we required that the sensor absolutely reaches the target<br>
exposure, but this can fail if frame rates or analogue gains are<br>
limited. Instead insist only that we get several frames with the same<br>
exposure time, analogue gain and that the algorithm's target exposure<br>
hasn't changed either.<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 | 65 +++++++++++++---------<br>
src/ipa/raspberrypi/controller/rpi/agc.hpp | 3 +<br>
2 files changed, 42 insertions(+), 26 deletions(-)<br>
<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
index 93b46a28..7be74763 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
@@ -164,6 +164,8 @@ Agc::Agc(Controller *controller)<br>
flicker_period_ = 0.0;<br>
fixed_shutter_ = 0;<br>
fixed_analogue_gain_ = 0.0;<br>
+ memset(&last_device_status_, 0, sizeof(last_device_status_));<br>
+ last_target_exposure_ = 0.0;<br></blockquote><div><br></div><div>Set last_target_exposure_ in the initialiser list?</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>
char const *Agc::Name() const<br>
@@ -264,8 +266,6 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode,<br>
<br>
void Agc::Prepare(Metadata *image_metadata)<br>
{<br>
- int lock_count = lock_count_;<br>
- lock_count_ = 0;<br>
status_.digital_gain = 1.0;<br>
fetchAwbStatus(image_metadata); // always fetch it so that Process knows it's been done<br>
<br>
@@ -289,32 +289,10 @@ void Agc::Prepare(Metadata *image_metadata)<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>
-#define MAX_LOCK_COUNT 3<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>
- lock_count_ =<br>
- std::min(lock_count + 1,<br>
- MAX_LOCK_COUNT);<br>
- else if (actual_exposure <<br>
- status_.target_exposure_value + 1.5 * err &&<br>
- actual_exposure ><br>
- status_.target_exposure_value - 1.5 * err)<br>
- lock_count_ = lock_count;<br>
- LOG(RPiAgc, Debug) << "Lock count: " << lock_count_;<br>
- }<br>
+ updateLockStatus(device_status);<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>
+ LOG(RPiAgc, Warning) << Name() << ": no device metadata";<br>
image_metadata->Set("agc.status", status_);<br>
}<br>
}<br>
@@ -345,6 +323,41 @@ void Agc::Process(StatisticsPtr &stats, Metadata *image_metadata)<br>
writeAndFinish(image_metadata, desaturate);<br>
}<br>
<br>
+void Agc::updateLockStatus(DeviceStatus const &device_status)<br>
+{<br>
+ const double ERROR_FACTOR = 0.10; // make these customisable?<br>
+ const int MAX_LOCK_COUNT = 5;<br>
+<br>
+ // Add 200us to the exposure time error to allow for line quantisation.<br>
+ double exposure_error = last_device_status_.shutter_speed * ERROR_FACTOR + 200;<br>
+ double gain_error = last_device_status_.analogue_gain * ERROR_FACTOR;<br>
+ double target_error = last_target_exposure_ * ERROR_FACTOR;<br>
+<br>
+ // Note that we don't know the exposure/gain limits of the sensor, so<br>
+ // the values we keep requesting may be unachievable. For this reason<br>
+ // we only insist that we're close to values in the past few frames.<br>
+ if (device_status.shutter_speed > last_device_status_.shutter_speed - exposure_error &&<br>
+ device_status.shutter_speed < last_device_status_.shutter_speed + exposure_error &&<br>
+ device_status.analogue_gain > last_device_status_.analogue_gain - gain_error &&<br>
+ device_status.analogue_gain < last_device_status_.analogue_gain + gain_error &&<br>
+ status_.target_exposure_value > last_target_exposure_ - target_error &&<br>
+ status_.target_exposure_value < last_target_exposure_ + target_error)<br>
+ lock_count_ = std::min(lock_count_ + 1, MAX_LOCK_COUNT);<br>
+ else if (device_status.shutter_speed < last_device_status_.shutter_speed - 1.5 * exposure_error ||<br>
+ device_status.shutter_speed > last_device_status_.shutter_speed + 1.5 * exposure_error ||<br>
+ device_status.analogue_gain < last_device_status_.analogue_gain - 1.5 * gain_error ||<br>
+ device_status.analogue_gain > last_device_status_.analogue_gain + 1.5 * gain_error ||<br>
+ status_.target_exposure_value < last_target_exposure_ - 1.5 * target_error ||<br>
+ status_.target_exposure_value > last_target_exposure_ + 1.5 * target_error)<br>
+ lock_count_ = 0;<br></blockquote><div><br></div><div>Does the 1.5 factor need to be a const?</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>
+ last_device_status_ = device_status;<br>
+ last_target_exposure_ = status_.target_exposure_value;<br>
+<br>
+ LOG(RPiAgc, Debug) << "Lock count updated to " << lock_count_;<br>
+ status_.locked = lock_count_ >= MAX_LOCK_COUNT;<br></blockquote><div><br></div><div>Only need a == here :)</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>
static void copy_string(std::string const &s, char *d, size_t size)<br>
{<br>
size_t length = s.copy(d, size - 1);<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
index 859a9650..47ebb324 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
@@ -82,6 +82,7 @@ public:<br>
void Process(StatisticsPtr &stats, Metadata *image_metadata) override;<br>
<br>
private:<br>
+ void updateLockStatus(DeviceStatus const &device_status);<br>
AgcConfig config_;<br>
void housekeepConfig();<br>
void fetchCurrentExposure(Metadata *image_metadata);<br>
@@ -111,6 +112,8 @@ private:<br>
ExposureValues filtered_; // these values are filtered towards target<br>
AgcStatus status_;<br>
int lock_count_;<br>
+ DeviceStatus last_device_status_;<br>
+ double last_target_exposure_;<br>
// Below here the "settings" that applications can change.<br>
std::string metering_mode_name_;<br>
std::string exposure_mode_name_;<br>
-- <br>
2.20.1<br></blockquote><div><br></div><div>Apart from the minor nits:</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>
_______________________________________________<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>