[libcamera-devel] [PATCH v2 4/4] libcamera: pipeline: Register device numbers with camera

[Kieran Bingham]

Quoting Jacopo Mondi (2023-05-13 16:07:54)
> Hi Kieran
> 
> On Thu, May 11, 2023 at 11:58:33PM +0100, Kieran Bingham via libcamera-devel wrote:
> > Register the identified device numbers with each camera as the Devices
> > property.
> >
> > This facilitates camera daemons or other systems to identify which
> > devices are being managed by libcamera, and can prevent duplication of
> > camera resources.
> 
> more than duplication isn't this about concurrent access to the same
> resources ?

No not specifically concurrent. It's (in the feature request) so that
pipewire doesn't create a video object for the same UVC camera through
both libcamera and V4L2 for instance. They don't have to be used
concurrently even - it's the fact that pipewire doesn't know if the
device is managed by libcamera - and it can be managed through both V4L2
and libcamera - so pipewire ends up showing two camera objects that are
actually the same device.

Technically both could be used (individually) but this lets pipewire
decide to only show one (and it's up to PW to decide which one it
selects between libcamera and v4l2 directly).



> 
> >
> > As the Devices property now provides this list of devices, use it
> > directly from within the CameraManager when adding a Camera rather than
> > passing it through the internal API.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_manager.h |  3 +--
> >  src/libcamera/camera_manager.cpp            | 13 ++++++++-----
> >  src/libcamera/pipeline_handler.cpp          | 12 ++++++++++--
> >  3 files changed, 19 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> > index 8386e90e0f60..061d2c5083b0 100644
> > --- a/include/libcamera/internal/camera_manager.h
> > +++ b/include/libcamera/internal/camera_manager.h
> > @@ -29,8 +29,7 @@ public:
> >       Private();
> >
> >       int start();
> > -     void addCamera(std::shared_ptr<Camera> camera,
> > -                    const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> > +     void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >       void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >
> >       /*
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 34f09a4d181b..df5fdede85da 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -15,7 +15,9 @@
> >  #include <libcamera/base/utils.h>
> >
> >  #include <libcamera/camera.h>
> > +#include <libcamera/property_ids.h>
> >
> > +#include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/camera_manager.h"
> >  #include "libcamera/internal/device_enumerator.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> > @@ -151,19 +153,17 @@ void CameraManager::Private::cleanup()
> >  /**
> >   * \brief Add a camera to the camera manager
> >   * \param[in] camera The camera to be added
> > - * \param[in] devnums The device numbers to associate with \a camera
> >   *
> >   * This function is called by pipeline handlers to register the cameras they
> >   * handle with the camera manager. Registered cameras are immediately made
> >   * available to the system.
> >   *
> > - * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
> > - * to Camera instances.
> > + * Device numbers from the Devices property are used by the V4L2 compatibility
>                           properties::Devices
> 
> > + * layer to map V4L2 device nodes to Camera instances.
> 
> I would also add here what you have reported in the commit message
> about higher level frameworks
> 
> >   *
> >   * \context This function shall be called from the CameraManager thread.
> >   */
> > -void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> > -                                    const std::vector<dev_t> &devnums)
> > +void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera)
> >  {
> >       ASSERT(Thread::current() == this);
> >
> > @@ -178,6 +178,9 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> >               }
> >       }
> >
> > +     Camera::Private *data = camera->_d();
> > +     auto devnums = data->properties_.get(properties::Devices).value_or(std::vector<int64_t>{});
> 
> No need to get a pointer to the Private implementation to access the
> Camera properties.
> 
>         auto devnums = camera->properties().get...

Oh yeah ! That's easier/cleaner ;-)

> 
> I would also have expected
>         .value_or(Span<const int64_t>{});
> 
> does this work because of an implicit type conversion ?

I expect so! I wish we had Laurents' default emtpy constructors to be
able to do 
	.value_or({}); directly :-(

> > +
> >       cameras_.push_back(std::move(camera));
> >
> >       unsigned int index = cameras_.size() - 1;
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 49092ea88a58..0b5fac7ad113 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -17,6 +17,7 @@
> >
> >  #include <libcamera/camera.h>
> >  #include <libcamera/framebuffer.h>
> > +#include <libcamera/property_ids.h>
> >
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/camera_manager.h"
> > @@ -612,7 +613,7 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> >        * Walk the entity list and map the devnums of all capture video nodes
> >        * to the camera.
> >        */
> > -     std::vector<dev_t> devnums;
> > +     std::vector<int64_t> devnums;
> >       for (const std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> >               for (const MediaEntity *entity : media->entities()) {
> >                       if (entity->pads().size() == 1 &&
> > @@ -624,7 +625,14 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> >               }
> >       }
> >
> > -     manager_->_d()->addCamera(std::move(camera), devnums);
> > +     /*
> > +      * Store the associated devices as a property of the camera to allow
> > +      * systems to identify which devices are managed by libcamera.
> > +      */
> > +     Camera::Private *data = camera->_d();
> > +     data->properties_.set(properties::Devices, devnums);
> > +
> > +     manager_->_d()->addCamera(std::move(camera));
> 
> nice

Yes, this bit worked out nicely indeed.

> 
> >  }
> >
> >  /**
> > --
> > 2.34.1
> >