[libcamera-devel] [PATCH 1/5] src: ipa: raspberrypi: Distinguish the first camera start from others

[Laurent Pinchart]

Hi David,

On Sat, Dec 05, 2020 at 11:10:48PM +0000, David Plowman wrote:
> On Sat, 5 Dec 2020 at 23:03, Laurent Pinchart wrote:
> > On Sat, Dec 05, 2020 at 10:39:55PM +0000, David Plowman wrote:
> > > On Sat, 5 Dec 2020 at 19:57, Laurent Pinchart wrote:
> > > > On Wed, Dec 02, 2020 at 11:52:49AM +0000, David Plowman wrote:
> > > > > This makes it possible to tell whether we're starting the sensor for
> > > > > the first time, or whether it's happening because of a mode switch or
> > > > > because the camera has been paused and re-started. Depending on this,
> > > > > some sensors may require us to drop different numbers of frames.
> > > > >
> > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > >
> > > > This looks good to me, but I'd squash it with patch 2/5 as the chance is
> > > > very small and it's easier to review it when also seeing how the new
> > > > variable is used.
> > >
> > > Will do!
> > >
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >
> > > > On a side note, how does mode change and initial start differ ? Don't we
> > > > stop the sensor when reconfiguring the camera ?
> > >
> > > The difference is that on first camera start, the algorithms are
> > > starting "from scratch", with no idea what initial values to use. On
> > > subsequent mode switches, the algorithms are known to be in a sensible
> > > place and we just leave them be (we don't reset them, though the
> > > change of camera mode can some some other effects).
> >
> > Ah yes that makes sense. What if the application stops the camera and
> > restarts it "a long time" later ? Would we need some kind of timeout
> > after which algorithms should drop their state (or at least consider the
> > state doesn't shorten the convergence time anymore) ?
> 
> Yes, I guess that's a possibility. Though I think I'd be reluctant to
> make the algorithms handle that themselves, probably an application
> should know better whether this might be the situation - at which
> point there would have to be some way to "reset" the camera system,
> which would in turn "reset" all the IPAs.

That's a good point. A timeout in the IPA would be fairly arbitrary,
especially consider the use case of stop motion capture in a controlled
light environment (although in that case one would probably want to set
exposure and gains explicitly).

> So maybe that's one to think about a bit. In the meantime I suppose
> closing and re-opening the camera system is not a terrible
> workaround...

Having to stop and restart the camera manager is a bit harsh as it will
affect all cameras. A different API is likely needed. Let's just keep it
in mind.

> > > > > ---
> > > > >  src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index 69be5e4e..b8298768 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -67,7 +67,7 @@ public:
> > > > >       IPARPi()
> > > > >               : lastMode_({}), controller_(), controllerInit_(false),
> > > > >                 frameCount_(0), checkCount_(0), mistrustCount_(0),
> > > > > -               lsTable_(nullptr)
> > > > > +               lsTable_(nullptr), firstStart_(true)
> > > > >       {
> > > > >       }
> > > > >
> > > > > @@ -145,6 +145,9 @@ private:
> > > > >       /* LS table allocation passed in from the pipeline handler. */
> > > > >       FileDescriptor lsTableHandle_;
> > > > >       void *lsTable_;
> > > > > +
> > > > > +     /* Distinguish the first camera start from others. */
> > > > > +     bool firstStart_;
> > > > >  };
> > > > >
> > > > >  int IPARPi::init(const IPASettings &settings)
> > > > > @@ -179,6 +182,8 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)
> > > > >               result->operation |= RPi::IPA_CONFIG_SENSOR;
> > > > >       }
> > > > >
> > > > > +     firstStart_ = false;
> > > > > +
> > > > >       return 0;
> > > > >  }
> > > > >

-- 
Regards,

Laurent Pinchart