<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 2 Dec 2020 at 11:53, 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 the CamHelper was returning the number of frames to drop<br>
(on account of AGC converging). This wasn't really appropriate, it's<br>
better for the AGC to do it, which now also knows when exposure and<br>
gain have been explicitly set and therefore fewer (or no) frame drops<br>
are necessary at all.<br>
<br>
The CamHelper::HideFramesStartup method should now just be returning<br>
the number of frames to hide because they're bad/invalid in some way,<br>
not worrying about the AGC. For many sensors, the correct value for<br>
this is zero.<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/cam_helper.cpp | 6 +++---<br>
src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++<br>
2 files changed, 11 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
index c8ac3232..6efa0d7f 100644<br>
--- a/src/ipa/raspberrypi/cam_helper.cpp<br>
+++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
@@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const<br>
unsigned int CamHelper::HideFramesStartup() const<br>
{<br>
/*<br>
- * By default, hide 6 frames completely at start-up while AGC etc. sort<br>
- * themselves out (converge).<br>
+ * The number of frames when a camera first starts that shouldn't be<br>
+ * displayed as they are invalid in some way.<br>
*/<br>
- return 6;<br>
+ return 0;<br>
}<br>
<br>
unsigned int CamHelper::HideFramesModeSwitch() const<br>
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
index 0300b8d9..ddabdb31 100644<br>
--- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
+++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
@@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)<br>
unsigned int dropFrame = 0;<br>
if (firstStart_) {<br>
dropFrame = helper_->HideFramesStartup();<br>
+<br>
+ /* The AGC algorithm may want us to drop more frames. */<br>
+ RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
+ controller_.GetAlgorithm("agc"));<br>
+ if (agc)<br>
+ dropFrame = std::max(dropFrame, agc->GetDropFrames());<br>
+ LOG(IPARPI, Debug) << "Drop " << dropFrame << " frames on startup";<br>
+<br></blockquote><div><br></div><div>All looks good with this change, however, I have a possibly silly question. In the previous code, our startup frames would account for convergence in AGC, AWB, and ALS. Here we are explicitly accounting for convergence only in AGC since
helper_->HideFramesStartup() will return 0 by default. Does it matter? Should each derived CamHelper return a non-zero number here?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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">
mistrustCount_ = helper_->MistrustFramesStartup();<br>
} else {<br>
dropFrame = helper_->HideFramesModeSwitch();<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>