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

[Kieran Bingham]

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 at 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