[libcamera-devel] [PATCH v1 3/6] pipeline: raspberrypi: Free buffers in the RPiCamera destructor and re-configure

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback!

On Sun, 13 Mar 2022 at 16:02, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> H Naush,
>
> Thank you for the patch.
>
> On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via
> libcamera-devel wrote:
> > Currently, all framebuffer allocations get freed and cleared on a stop in
> > PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then
> called
> > without an intermediate PipelineHandlerRPi::configure(), it will
> re-allocate and
> > prepare all the buffers again, which is unnecessary.
> >
> > Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(),
> but
> > insted doing it in PipelineHandlerRPi::configure(), as the buffers might
> have
> > to be resized.
>
> I see where you're going, but doesn't this mean that buffers will stay
> allocated forever ? If an application uses a camera for some time and
> then stops it, memory won't be released until the application
> terminates. That's trading an issue for another one, and which one is
> worse really depends on the use case.
>
> There are (at least) two ways to address this. The simplest one would be
> to fire a timer at stop() time, and free buffers when it elapses. The
> timer would be cancelled if the camera is restarted first.
>
> The second option is to make this controllable by the application. We
> could hook it up to the Camera::release() call for instance, adding a
> new pipeline handler operation to handle it. release() may not be the
> best option though, maybe we need a new cleanup function. Or maybe an
> argument to stop(), to tell if the camera is expected to be restarted
> soon ? I haven't really thought about the pros and cons of the different
> options, I'm just brainstorming here.
>

Yes, I do see a possible issue here with holding onto buffers for longer
than
expected.  My preferred option would be to have a Camera::release() call
that explicitly requests the pipeline handler to remove all buffer
allocations.
That way, the application controls the intended behavior if it chooses to,
but
the pipeline handler keeps buffers allocated otherwise.

Regards,
Naush


>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 3781e4e0e3c4..8bb9fc429912 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -189,6 +189,11 @@ public:
> >       {
> >       }
> >
> > +     ~RPiCameraData()
> > +     {
> > +             freeBuffers();
> > +     }
> > +
> >       void freeBuffers();
> >       void frameStarted(uint32_t sequence);
> >
> > @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >       RPiCameraData *data = cameraData(camera);
> >       int ret;
> >
> > -     /* Start by resetting the Unicam and ISP stream states. */
> > +     /* Start by freeing all buffers and resetting the Unicam and ISP
> stream states. */
> > +     data->freeBuffers();
> >       for (auto const stream : data->streams_)
> >               stream->reset();
> >
> > @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
> >
> >       /* Stop the IPA. */
> >       data->ipa_->stop();
> > -
> > -     data->freeBuffers();
> >  }
> >
> >  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request
> *request)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220314/da380c9f/attachment.htm>