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

17787 diff mbox series

This notifies pipeline handlers when a camera is released, in case
they want to free any resources or memory buffers.

Signed-off-by: David Plowman <david.plowman@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(-)

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@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@ideasonboard.com>

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

Hi David,

Thank you for fixing this.

On Fri, 11 Nov 2022 at 13:30, David Plowman via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> This notifies pipeline handlers when a camera is released, in case
> they want to free any resources or memory buffers.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

Reviewed-by: Naushir Patuck <naush@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.
> + */
> +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> +{
> +}
> +
>  void PipelineHandler::unlockMediaDevices()
>  {
>         for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> --
> 2.30.2
>
>

Hi David,

Thank you for the patch.

On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:
> This notifies pipeline handlers when a camera is released, in case
> they want to free any resources or memory buffers.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  4 +++-
>  src/libcamera/camera.cpp                      |  2 +-
>  src/libcamera/pipeline_handler.cpp            | 16 +++++++++++++++-

The pipeline handler writer's guide
(Documentation/guides/pipeline-handler.rst) needs to be udpated. The
vivid pipeline handler may also need some attention, but we can handle
that.

>  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 doesn't 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.

This needs further documentation, to clearly explain what can or can't
be done here. Examples could help.

This being said, I'm not sure hooking up this feature in the
Camera::release() function is the right option. In addition to the lack
of balance between acquire() and release() (in this series the release()
function does more than counterbalance acquire()), the usage pattern
that was envisioned is that application can hold onto cameras for a long
time. Having to call release() to free memory means that another
application could then acquire the camera.

> + */
> +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> +{
> +}
> +
>  void PipelineHandler::unlockMediaDevices()
>  {
>  	for (std::shared_ptr<MediaDevice> &media : mediaDevices_)

Quoting Laurent Pinchart via libcamera-devel (2022-11-14 16:56:21)
> Hi David,
> 
> Thank you for the patch.
> 
> On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:
> > This notifies pipeline handlers when a camera is released, in case
> > they want to free any resources or memory buffers.
> > 
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/internal/pipeline_handler.h |  4 +++-
> >  src/libcamera/camera.cpp                      |  2 +-
> >  src/libcamera/pipeline_handler.cpp            | 16 +++++++++++++++-
> 
> The pipeline handler writer's guide
> (Documentation/guides/pipeline-handler.rst) needs to be udpated. The
> vivid pipeline handler may also need some attention, but we can handle
> that.
> 
> >  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 doesn't 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.
> 
> This needs further documentation, to clearly explain what can or can't
> be done here. Examples could help.
> 
> This being said, I'm not sure hooking up this feature in the
> Camera::release() function is the right option. In addition to the lack
> of balance between acquire() and release() (in this series the release()
> function does more than counterbalance acquire()), the usage pattern
> that was envisioned is that application can hold onto cameras for a long
> time. Having to call release() to free memory means that another
> application could then acquire the camera.

I was actually trying to balance this up today, and add the
corresponding acquires so that pipeline handlers can ensure they only
take resources when they are acquired. For exactly the reason that
Pipewire already obtains and holds on to 'Camera' devices, and I
expect it shouldn't keep device nodes open when not in use.

I expected this would allow us to do things like 'only open UVC devices
when the camera is actually acquired' for instance, but it seems there
is something complex going on, meaning if I close the device after
PipelineHandlerUVC::init() has completed, and reopen it at 'acquire()'
time .. I don't get any frame completion events, seemingly at the fd
notifier layer.

Perhaps just a bug that needs clearing up...

--
Kieran


> 
> > + */
> > +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> > +{
> > +}
> > +
> >  void PipelineHandler::unlockMediaDevices()
> >  {
> >       for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Laurent

Thanks for the comments. I see that this patch is already merged so
please let me know how you'd like to proceed.

On Mon, 14 Nov 2022 at 16:56, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:
> > This notifies pipeline handlers when a camera is released, in case
> > they want to free any resources or memory buffers.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/internal/pipeline_handler.h |  4 +++-
> >  src/libcamera/camera.cpp                      |  2 +-
> >  src/libcamera/pipeline_handler.cpp            | 16 +++++++++++++++-
>
> The pipeline handler writer's guide
> (Documentation/guides/pipeline-handler.rst) needs to be udpated. The
> vivid pipeline handler may also need some attention, but we can handle
> that.

Thank you!

>
> >  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 doesn't release "data".

Sorry, I was using the word "data" generically. Perhaps "resources"?
Let me know if you'd like me to update the patch.

>
> >   *
> >   * 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.
>
> This needs further documentation, to clearly explain what can or can't
> be done here. Examples could help.
>
> This being said, I'm not sure hooking up this feature in the
> Camera::release() function is the right option. In addition to the lack
> of balance between acquire() and release() (in this series the release()
> function does more than counterbalance acquire()), the usage pattern
> that was envisioned is that application can hold onto cameras for a long
> time. Having to call release() to free memory means that another
> application could then acquire the camera.

I did wonder about "acquire", but it seemed less urgent as you always
find out if someone really wants to use the camera! But anyway, I see
that Kieran is doing work in this area now.

I guess I might be a bit confused about the purpose of "release". I
expected the idea would be that other applications could then use it,
but maybe there are 2 levels of "releasing"? One really means
"release" (and someone else can have it). The other is perhaps closer
to "disable it" (but don't let anyone else have it)? Does that sound
right?

Thanks!
David

>
> > + */
> > +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> > +{
> > +}
> > +
> >  void PipelineHandler::unlockMediaDevices()
> >  {
> >       for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
>
> --
> Regards,
>
> Laurent Pinchart

Quoting David Plowman via libcamera-devel (2022-11-15 10:37:46)
> Hi Laurent
> 
> Thanks for the comments. I see that this patch is already merged so
> please let me know how you'd like to proceed.
> 
> On Mon, 14 Nov 2022 at 16:56, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:
> > > This notifies pipeline handlers when a camera is released, in case
> > > they want to free any resources or memory buffers.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/pipeline_handler.h |  4 +++-
> > >  src/libcamera/camera.cpp                      |  2 +-
> > >  src/libcamera/pipeline_handler.cpp            | 16 +++++++++++++++-
> >
> > The pipeline handler writer's guide
> > (Documentation/guides/pipeline-handler.rst) needs to be udpated. The
> > vivid pipeline handler may also need some attention, but we can handle
> > that.
> 
> Thank you!
> 
> >
> > >  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 doesn't release "data".
> 
> Sorry, I was using the word "data" generically. Perhaps "resources"?
> Let me know if you'd like me to update the patch.
> 
> >
> > >   *
> > >   * 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.
> >
> > This needs further documentation, to clearly explain what can or can't
> > be done here. Examples could help.
> >
> > This being said, I'm not sure hooking up this feature in the
> > Camera::release() function is the right option. In addition to the lack
> > of balance between acquire() and release() (in this series the release()
> > function does more than counterbalance acquire()), the usage pattern
> > that was envisioned is that application can hold onto cameras for a long
> > time. Having to call release() to free memory means that another
> > application could then acquire the camera.
> 
> I did wonder about "acquire", but it seemed less urgent as you always
> find out if someone really wants to use the camera! But anyway, I see
> that Kieran is doing work in this area now.
> 
> I guess I might be a bit confused about the purpose of "release". I
> expected the idea would be that other applications could then use it,
> but maybe there are 2 levels of "releasing"? One really means
> "release" (and someone else can have it). The other is perhaps closer
> to "disable it" (but don't let anyone else have it)? Does that sound
> right?

In my understanding, a Camera is created when an application launches
the CameraManager. Those Camera's exist and have been initialised, so if
that process is long living (like a Camera Daemon, or Pipewire), those
objects will persist. But I don't think they should acquire or lock
resources until an explicit call to acquire() has occurred.

It may be unlikely that a Pipeline should need to allocate memory during
require, but I'm sure that during release, they should release anything
that was allocated additionally during configure()!

It would be nicer to keep the base implementation symetrical I think, so
an equivalent acquire() could be passed to the pipeline handler
(optionally) ... but it does depend on it being needed. I tried only
opening the video node for UVC on acquire (or rather, open it during
init(), obtain information required, then close it until acquire() ),
and hit (unexpected) issues.

I think I'll just post the patches anyway, with a [DNI] tag on the UVC
part to share. Maybe it will be obvious to someone what either I've done
wrong, or what the bug is.

--
Kieran



> 
> Thanks!
> David
> 
> >
> > > + */
> > > +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> > > +{
> > > +}
> > > +
> > >  void PipelineHandler::unlockMediaDevices()
> > >  {
> > >       for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> >
> > --
> > Regards,
> >
> > Laurent Pinchart