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

[David Plowman]

Hi again!

On Sat, 5 Dec 2020 at 23:03, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> 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.

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...

Thanks!
David

>
> > > > ---
> > > >  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