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

[Laurent Pinchart]

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.

> 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