[libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move initial frame drop decision to AGC

[Laurent Pinchart]

Hi Naush,

On Sun, Dec 06, 2020 at 10:44:44AM +0000, Naushir Patuck wrote:
> On Sat, 5 Dec 2020 at 19:50, Laurent Pinchart wrote:
> > On Fri, Dec 04, 2020 at 04:01:42PM +0000, Naushir Patuck wrote:
> > > On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:
> > >
> > > > Previously the CamHelper was returning the number of frames to drop
> > > > (on account of AGC converging). This wasn't really appropriate, it's
> > > > better for the AGC to do it, which now also knows when exposure and
> > > > gain have been explicitly set and therefore fewer (or no) frame drops
> > > > are necessary at all.
> > > >
> > > > The CamHelper::HideFramesStartup method should now just be returning
> > > > the number of frames to hide because they're bad/invalid in some way,
> > > > not worrying about the AGC. For many sensors, the correct value for
> > > > this is zero.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > >  src/ipa/raspberrypi/cam_helper.cpp  | 6 +++---
> > > >  src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++
> > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > > > index c8ac3232..6efa0d7f 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const
> > > >  unsigned int CamHelper::HideFramesStartup() const
> > > >  {
> > > >         /*
> > > > -        * By default, hide 6 frames completely at start-up while AGC etc. sort
> > > > -        * themselves out (converge).
> > > > +        * The number of frames when a camera first starts that shouldn't be
> > > > +        * displayed as they are invalid in some way.
> > > >          */
> > > > -       return 6;
> > > > +       return 0;
> > > >  }
> > > >
> > > >  unsigned int CamHelper::HideFramesModeSwitch() const
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index 0300b8d9..ddabdb31 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)
> > > >         unsigned int dropFrame = 0;
> > > >         if (firstStart_) {
> > > >                 dropFrame = helper_->HideFramesStartup();
> > > > +
> > > > +               /* The AGC algorithm may want us to drop more frames. */
> > > > +               RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > +                       controller_.GetAlgorithm("agc"));
> > > > +               if (agc)
> > > > +                       dropFrame = std::max(dropFrame, agc->GetDropFrames());
> > > > +               LOG(IPARPI, Debug) << "Drop " << dropFrame << " frames on startup";
> > > > +
> > >
> > > 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?
> >
> > Unless I'm mistaken, HideFramesStartup() is meant to report how many
> > incorrectly exposed (and "gained") frames are produced by the sensor at
> > startup, even if exposure time and gain are programmed before starting
> > the sensor. This will certainly impact AWB and ALS as they will have
> > trouble operating if the frame is greatly underexposed, but the sensor
> > is otherwise not involved in AWB and ALS. I thus don't think CamHelper
> > should take AWB and ALS into account.
> >
> > With this new split of responsibilities, with CamHelper reporting the
> > number of frames that are bad (for different reasons, underexposed,
> > incorrect metadata, ...) and the algorithms then deciding how long they
> > need before initially converging, Shouldn't agc->GetDropFrames() be
> > given the HideFramesStartup() value as a parameter, and return a new
> > number, instead of taking the maximum between the two ?
> >
> > I also wonder if we then need the hide/mistrust split anymore,
> > especially with the comment in CamHelperOv5647::MistrustFramesStartup()
> > that mentions underexposed frames. Is there still a difference between
> > the two concepts ?
> >
> > Finally, do we actually need to report a number of frames to drop at
> > startup to the pipeline handler, can't we rely on the algorithms status
> > reported through AeLocked and AwbLocked ?
> 
> 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.
> 
> 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!

That's a very good point too, I hadn't considered that. Thanks.

> > Maybe we should report only
> > the number of frames that are definitely bad based on the CamHelper, and
> > use algorithm status to report initial convergence of the algorithms ?
> >
> > > >                 mistrustCount_ = helper_->MistrustFramesStartup();
> > > >         } else {
> > > >                 dropFrame = helper_->HideFramesModeSwitch();

-- 
Regards,

Laurent Pinchart