<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 Mon, 7 Dec 2020 at 18:02, 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/AWB converging). This wasn't really appropriate,<br>
it's better for the algorithms to do it as they know how many frames<br>
they might need.<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/AWB. For many sensors, the correct value<br>
for this is zero. But the ov5647 needs updating as it must return 2.<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/cam_helper_ov5647.cpp | 10 ++++++++++<br>
src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++++++++<br>
3 files changed, 29 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/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
index dc5d8275..0b841cd1 100644<br>
--- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
+++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
@@ -19,6 +19,7 @@ public:<br>
uint32_t GainCode(double gain) const override;<br>
double Gain(uint32_t gain_code) const override;<br>
void GetDelays(int &exposure_delay, int &gain_delay) const override;<br>
+ unsigned int HideFramesStartup() const override;<br>
unsigned int HideFramesModeSwitch() const override;<br>
unsigned int MistrustFramesStartup() const override;<br>
unsigned int MistrustFramesModeSwitch() const override;<br>
@@ -54,6 +55,15 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const<br>
gain_delay = 2;<br>
}<br>
<br>
+unsigned int CamHelperOv5647::HideFramesStartup() const<br>
+{<br>
+ /*<br>
+ * On startup, we get a couple of under-exposed frames which<br>
+ * we don't want shown.<br>
+ */<br>
+ return 2;<br>
+}<br>
+<br>
unsigned int CamHelperOv5647::HideFramesModeSwitch() const<br>
{<br>
/*<br>
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
index 0e89af00..da92e492 100644<br>
--- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
+++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
@@ -193,6 +193,22 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)<br>
if (firstStart_) {<br>
dropFrame = helper_->HideFramesStartup();<br>
mistrustCount_ = helper_->MistrustFramesStartup();<br>
+<br>
+ /* The AGC and/or AWB algorithms may want us to drop more frames. */<br>
+ unsigned int convergence_frames = 0;<br></blockquote><div><br></div><div>Minor nit, but could you use camel case for convergence_frames to match the rest of the variables?</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">
+ RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
+ controller_.GetAlgorithm("agc"));<br>
+ if (agc)<br>
+ convergence_frames = agc->GetConvergenceFrames(mistrustCount_);<br>
+<br>
+ RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(<br>
+ controller_.GetAlgorithm("awb"));<br>
+ if (awb)<br>
+ convergence_frames = std::max(convergence_frames,<br>
+ awb->GetConvergenceFrames(mistrustCount_));<br>
+<br>
+ dropFrame = std::max(dropFrame, convergence_frames);<br>
+ LOG(IPARPI, Debug) << "Drop " << dropFrame << " frames on startup";<br></blockquote><div><br></div><div>Apart from the above nit, and the earlier question of passing convergence_frames into GetConvergenceFrames(),</div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
} else {<br>
dropFrame = helper_->HideFramesModeSwitch();<br>
mistrustCount_ = helper_->MistrustFramesModeSwitch();<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>