[libcamera-devel] [PATCH 1/2] libcamera: Add a PipelineHandler::releaseDevice method

[Kieran Bingham]

Hi David,

Quoting David Plowman via libcamera-devel (2022-11-11 13:30:24)
> This notifies pipeline handlers when a camera is released, in case
> they want to free any resources or memory buffers.
> 

Indeed, that does sound like something they might need!

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  4 +++-
>  src/libcamera/camera.cpp                      |  2 +-
>  src/libcamera/pipeline_handler.cpp            | 16 +++++++++++++++-
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 96aab9d6..ec4f662d 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -46,7 +46,7 @@ public:
>                                         const DeviceMatch &dm);
>  
>         bool acquire();
> -       void release();
> +       void release(Camera *camera);
>  
>         virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>                 const StreamRoles &roles) = 0;
> @@ -74,6 +74,8 @@ protected:
>         virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
>         virtual void stopDevice(Camera *camera) = 0;
>  
> +       virtual void releaseDevice(Camera *camera);
> +
>         CameraManager *manager_;
>  
>  private:
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index c4f65c1a..2d947a44 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -870,7 +870,7 @@ int Camera::release()
>                 return ret == -EACCES ? -EBUSY : ret;
>  
>         if (d->isAcquired())
> -               d->pipe_->release();
> +               d->pipe_->release(this);
>  
>         d->setState(Private::CameraAvailable);
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 825aff5a..cfade490 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()
>  
>  /**
>   * \brief Release exclusive access to the pipeline handler
> + * \param[in] camera The camera for which to release data
>   *
>   * This function releases access to the pipeline handler previously acquired by
>   * a call to acquire(). Every release() call shall match a previous successful
> @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()
>   *
>   * \sa acquire()
>   */
> -void PipelineHandler::release()
> +void PipelineHandler::release(Camera *camera)
>  {
>         MutexLocker locker(lock_);
>  
> @@ -204,9 +205,22 @@ void PipelineHandler::release()
>  
>         unlockMediaDevices();
>  
> +       releaseDevice(camera);
> +
>         --useCount_;
>  }
>  
> +/**
> + * \brief Release resources associated with this camera
> + * \param[in] camera The camera for which to release resources
> + *
> + * Pipeline handlers may override this in order to perform cleanup operations
> + * when a camera is released, such as freeing memory.

I don't know if it helps/is accurate or correct so only as a mere
possible suggestion to clarify:

'as freeing memory allocated during acquire()' ?

But equally - fine without.

> + */
> +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> +{
> +}
> +

This is certainly fine, and looks like it works correctly. I suspect it
could also have been the documentation added here, with just an
empty implementation in the header at something like:

	virtual void releaseDevice(Camera *camera) {};

But I think having the base/empty implementation here - does make it
clearer that the documentation is associated with it directly, and it
only costs 3 easy lines so:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  void PipelineHandler::unlockMediaDevices()
>  {
>         for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> -- 
> 2.30.2
>