[libcamera-devel] [PATCH v4 3/5] libcamera: camera_manager: Move {add, remove}Camera to internal

[Kieran Bingham]

Quoting Barnabás Pőcze (2023-06-15 22:03:20)
> Hi
> 
> 
> 2023. június 15., csütörtök 19:26 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel at lists.libcamera.org> írta:
> 
> > The CameraManager exposes addCamera and removeCamera as public API
> > calls, while they should never be called from an application. These
> > calls are only expected to be used by PipelineHandlers to update the
> > CameraManager that a new Camera has been created and allow the Camera
> > Manager to expose it to applications.
> > 
> > Remove the public calls and update the private implementations such that
> > they can be used directly by the PipelineHandler through the internal
> > CameraManager::Private provided by the Extensible class.
> > 
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > Tested-by: Ashok Sidipotu <ashok.sidipotu at collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/camera_manager.h          |  4 -
> >  include/libcamera/internal/camera_manager.h |  2 +-
> >  src/libcamera/camera_manager.cpp            | 87 +++++++++------------
> >  src/libcamera/pipeline_handler.cpp          |  6 +-
> >  4 files changed, 43 insertions(+), 56 deletions(-)
> > [...]
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 882b2d4b234c..cafd7bce574e 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -148,9 +148,25 @@ void CameraManager::Private::cleanup()
> >       enumerator_.reset(nullptr);
> >  }
> > 
> > +/**
> > + * \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.
> > + *
> > + * \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)
> >  {
> > +     ASSERT(Thread::current() == this);
> > +
> >       MutexLocker locker(mutex_);
> > 
> >       for (const std::shared_ptr<Camera> &c : cameras_) {
> > @@ -167,15 +183,31 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> >       unsigned int index = cameras_.size() - 1;
> >       for (dev_t devnum : devnums)
> >               camerasByDevnum_[devnum] = cameras_[index];
> > +
> > +     /* Report the addition to the public signal */
> > +     CameraManager *const o = LIBCAMERA_O_PTR();
> > +     o->cameraAdded.emit(cameras_[index]);
> >  }
> > 
> > -void CameraManager::Private::removeCamera(Camera *camera)
> > +/**
> > + * \brief Remove a camera from the camera manager
> > + * \param[in] camera The camera to be removed
> > + *
> > + * This function is called by pipeline handlers to unregister cameras from the
> > + * camera manager. Unregistered cameras won't be reported anymore by the
> > + * cameras() and get() calls, but references may still exist in applications.
> > + *
> > + * \context This function shall be called from the CameraManager thread.
> > + */
> > +void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
> >  {
> > +     ASSERT(Thread::current() == this);
> > +
> >       MutexLocker locker(mutex_);
> > 
> >       auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> >                                [camera](std::shared_ptr<Camera> &c) {
> > -                                      return c.get() == camera;
> > +                                      return c.get() == camera.get();
> >                                });
> >       if (iter == cameras_.end())
> >               return;
> > @@ -185,12 +217,16 @@ void CameraManager::Private::removeCamera(Camera *camera)
> > 
> >       auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
> >                                  [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
> > -                                        return p.second.lock().get() == camera;
> > +                                        return p.second.lock().get() == camera.get();
> >                                  });
> >       if (iter_d != camerasByDevnum_.end())
> >               camerasByDevnum_.erase(iter_d);
> 
> As far as I can see the above will only ever remove at most a single entry from the
> `camerasByDevnum_` map, but it is allowed for a camera to have multiple underlying devices.
> I know this part is not touched by this series, but it should be fixed sooner or later.
> (Also now that `camera` is a shared_ptr, it would be better to capture it by reference I think.
> Although the reason for that change is not clear to me.)

I think all of the camerasByDevnum_ is just a way to support 

	std::shared_ptr<Camera> get(dev_t devnum);

Which is only there for the V4L2 Adaptation layer I think.
I think it's probably better to remove this map and replace it with a
search trhough the cameras() list that examines the new SystemDevices
property on each one.

Of course it may have to be a first match or something as there could be
multiple cameras matching / using the same underlying devices ...

I think the V4L2 layer should probably be reworked quite a lot if anyone
actually wants to use it though - but I'm not sure of anyone who is
presently.


I'll have a quick go now at removing the camerasByDevnum_ entirely.

--
Kieran



> 
> 
> > 
> >       cameras_.erase(iter);
> > +
> > +     /* Report the removal to the public signal */
> > +     CameraManager *const o = LIBCAMERA_O_PTR();
> > +     o->cameraRemoved.emit(camera);
> >  }
> > 
> >  /**
> > @@ -383,51 +419,6 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> >   * perform any blocking operation.
> >   */
> > 
> > -/**
> > - * \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.
> > - *
> > - * \context This function shall be called from the CameraManager thread.
> > - */
> > -void CameraManager::addCamera(std::shared_ptr<Camera> camera,
> > -                           const std::vector<dev_t> &devnums)
> > -{
> > -     Private *const d = _d();
> > -
> > -     ASSERT(Thread::current() == d);
> > -
> > -     d->addCamera(camera, devnums);
> > -     cameraAdded.emit(camera);
> > -}
> > -
> > -/**
> > - * \brief Remove a camera from the camera manager
> > - * \param[in] camera The camera to be removed
> > - *
> > - * This function is called by pipeline handlers to unregister cameras from the
> > - * camera manager. Unregistered cameras won't be reported anymore by the
> > - * cameras() and get() calls, but references may still exist in applications.
> > - *
> > - * \context This function shall be called from the CameraManager thread.
> > - */
> > -void CameraManager::removeCamera(std::shared_ptr<Camera> camera)
> > -{
> > -     Private *const d = _d();
> > -
> > -     ASSERT(Thread::current() == d);
> > -
> > -     d->removeCamera(camera.get());
> > -     cameraRemoved.emit(camera);
> > -}
> > -
> >  /**
> >   * \fn const std::string &CameraManager::version()
> >   * \brief Retrieve the libcamera version string
> > [...]
> 
> 
> Regards,
> Barnabás Pőcze