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

[David Plowman]

Hi Laurent

Thanks for the review.

On Sat, 5 Dec 2020 at 19:57, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> 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).

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