<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 5 Dec 2020 at 19:50, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.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">Hi Naush and David,<br>
<br>
On Fri, Dec 04, 2020 at 04:01:42PM +0000, Naushir Patuck wrote:<br>
> On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:<br>
> <br>
> > 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<br>
> > 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<br>
> > 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,<br>
> > 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>
> <br>
> All looks good with this change, however, I have a possibly silly<br>
> question. In the previous code, our startup frames would account for<br>
> convergence in AGC, AWB, and ALS. Here we are explicitly accounting for<br>
> convergence only in AGC since helper_->HideFramesStartup() will return 0<br>
> by default. Does it matter? Should each derived CamHelper return a<br>
> non-zero number here?<br>
<br>
Unless I'm mistaken, HideFramesStartup() is meant to report how many<br>
incorrectly exposed (and "gained") frames are produced by the sensor at<br>
startup, even if exposure time and gain are programmed before starting<br>
the sensor. This will certainly impact AWB and ALS as they will have<br>
trouble operating if the frame is greatly underexposed, but the sensor<br>
is otherwise not involved in AWB and ALS. I thus don't think CamHelper<br>
should take AWB and ALS into account.<br>
<br>
With this new split of responsibilities, with CamHelper reporting the<br>
number of frames that are bad (for different reasons, underexposed,<br>
incorrect metadata, ...) and the algorithms then deciding how long they<br>
need before initially converging, Shouldn't agc->GetDropFrames() be<br>
given the HideFramesStartup() value as a parameter, and return a new<br>
number, instead of taking the maximum between the two ?<br>
<br>
I also wonder if we then need the hide/mistrust split anymore,<br>
especially with the comment in CamHelperOv5647::MistrustFramesStartup()<br>
that mentions underexposed frames. Is there still a difference between<br>
the two concepts ?<br>
<br>
Finally, do we actually need to report a number of frames to drop at<br>
startup to the pipeline handler, can't we rely on the algorithms status<br>
reported through AeLocked and AwbLocked ? </blockquote><div><br></div><div>In my opinion, doing it this way (making the pipeline handler drop all frames where AE/AWB is not locked) would be very inefficient. Knowing how many buffers are meant to be dropped on startup as we currently do allows the pipeline handler to efficiently pipeline(!) the buffers queued to the device.</div><div><br></div><div>As an example, if the first Request after startup asks for the RAW frame to be returned out to the application, we can pre-queue internally allocated buffers to the count of the number of expected drop frames. We then queue up the Request provided RAW buffer to the device and have no additional latency involved with handling dropped frames and returning the RAW buffer. If we were to wait for AE/AWB locked status without knowing the number of dropped frames beforehand, we would have to continually queue the Request provided RAW buffer to the device over and over again while we keep dropping them in the pipeline handler. Typically, that doubles your capture latency as you cannot pipeline the buffers queued to the streaming device as is the current case. It's a similar story for queueing ISP output buffers, but (at least for Raspberry Pi) given the mem-to-mem behavior of the HW, you don't take as big a hit on latency. Hope that all makes sense!</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">Maybe we should report only<br>
the number of frames that are definitely bad based on the CamHelper, and<br>
use algorithm status to report initial convergence of the algorithms ?<br>
<br>
> > mistrustCount_ = helper_->MistrustFramesStartup();<br>
> > } else {<br>
> > dropFrame = helper_->HideFramesModeSwitch();<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>