[libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera objects through shared pointers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 18 17:47:22 CET 2019
Hi Jacopo,
On Fri, Jan 18, 2019 at 04:39:56PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 18, 2019 at 01:59:16AM +0200, Laurent Pinchart wrote:
> > The Camera class is explicitly reference-counted to manage the lifetime
> > of camera objects. Replace this open-coded implementation with usage of
> > the std::shared_ptr<> class.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/camera.h | 13 +++++----
> > include/libcamera/camera_manager.h | 8 +++---
> > src/libcamera/camera.cpp | 46 +++++++++++++++++-------------
> > src/libcamera/camera_manager.cpp | 20 ++++++-------
> > src/libcamera/pipeline/vimc.cpp | 2 +-
> > test/list-cameras.cpp | 2 +-
> > 6 files changed, 50 insertions(+), 41 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 9a7579d61fa3..ef0a794e3a82 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -7,6 +7,7 @@
> > #ifndef __LIBCAMERA_CAMERA_H__
> > #define __LIBCAMERA_CAMERA_H__
> >
> > +#include <memory>
> > #include <string>
> >
> > namespace libcamera {
> > @@ -14,15 +15,17 @@ namespace libcamera {
> > class Camera
>
> Should we make this 'final', it cannot be extended anyhow, let's make
> this a clear design choice.
Good point, I'll do that.
> > {
> > public:
> > - Camera(const std::string &name);
> > + static std::shared_ptr<Camera> create(const std::string &name);
> > +
> > + Camera(const Camera &) = delete;
> > + void operator=(const Camera &) = delete;
> >
> > const std::string &name() const;
> > - void get();
> > - void put();
> >
> > private:
> > - virtual ~Camera() { };
> > - int ref_;
> > + Camera(const std::string &name);
> > + ~Camera();
> > +
> > std::string name_;
> > };
> >
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > index 4b941fd9183b..738b13f4cfb1 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -24,10 +24,10 @@ public:
> > int start();
> > void stop();
> >
> > - const std::vector<Camera *> &cameras() const { return cameras_; }
> > - Camera *get(const std::string &name);
> > + const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> > + std::shared_ptr<Camera> get(const std::string &name);
>
> This return by value guarantees that even if the manager dies, cameras "got()"
> by the applications will stay around as long as application will be alive,
> right?
Correct, it returns a new shared reference, so the camera instance won't
disappear.
> >
> > - void addCamera(Camera *camera);
> > + void addCamera(std::shared_ptr<Camera> &camera);
> >
> > static CameraManager *instance();
> >
> > @@ -42,7 +42,7 @@ private:
> >
> > std::unique_ptr<DeviceEnumerator> enumerator_;
> > std::vector<PipelineHandler *> pipes_;
> > - std::vector<Camera *> cameras_;
> > + std::vector<std::shared_ptr<Camera>> cameras_;
> >
> > std::unique_ptr<EventDispatcher> dispatcher_;
> > };
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 6da2b20137d4..acf912bee95c 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -41,19 +41,36 @@ namespace libcamera {
> > * streams from a single image source. It provides the main interface to
> > * configuring and controlling the device, and capturing image streams. It is
> > * the central object exposed by libcamera.
> > + *
> > + * To support the central nature of Camera objects, libcamera manages the
> > + * lifetime of camera instances with std::shared_ptr<>. Instances shall be
> > + * created with the create() function which returns a shared pointer. The
> > + * Camera constructors and destructor are private, to prevent instances from
> > + * being constructed and destroyed manually.
> > */
> >
> > /**
> > - * \brief Construct a named camera device
> > + * \brief Create a camera instance
> > + * \param[in] name The name of the camera device
> > *
> > - * \param[in] name The name to set on the camera device
> > + * The caller is responsible for guaranteeing unicity of the camera name.
> > *
> > - * The caller is responsible for guaranteeing unicity of the camera
> > - * device name.
> > + * \return A shared pointer to the newly created camera object
> > */
> > -Camera::Camera(const std::string &name)
> > - : ref_(1), name_(name)
> > +std::shared_ptr<Camera> Camera::create(const std::string &name)
> > {
> > + struct Allocator : std::allocator<Camera> {
>
> out of curiosity, why a struct and not a class?
No reason, I think I found this in sample code somewhere. Should I make
it a class ?
> > + void construct(void *p, const std::string &name)
> > + {
> > + ::new(p) Camera(name);
>
> This "new(p)" scares me :)
Shhhhhh... :-)
> > + }
> > + void destroy(Camera *p)
> > + {
> > + p->~Camera();
> > + }
> > + };
> > +
> > + return std::allocate_shared<Camera>(Allocator(), name);
> > }
> >
> > /**
> > @@ -66,24 +83,13 @@ const std::string &Camera::name() const
> > return name_;
> > }
> >
> > -/**
> > - * \brief Acquire a reference to the camera
> > - */
> > -void Camera::get()
> > +Camera::Camera(const std::string &name)
> > + : name_(name)
> > {
> > - ref_++;
> > }
> >
> > -/**
> > - * \brief Release a reference to the camera
> > - *
> > - * When the last reference is released the camera device is deleted. Callers
> > - * shall not access the camera device after calling this function.
> > - */
> > -void Camera::put()
> > +Camera::~Camera()
> > {
> > - if (--ref_ == 0)
> > - delete this;
> > }
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 852e5ed70fe3..d12bd42001ae 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -39,9 +39,11 @@ namespace libcamera {
> > * This will enumerate all the cameras present in the system, which can then be
> > * listed with list() and retrieved with get().
> > *
> > - * Cameras are reference-counted, and shall be returned to the camera manager
> > - * with Camera::put() after being used. Once all cameras have been returned to
> > - * the manager, it can be stopped with stop().
> > + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will
> > + * stay valid until the last reference is released without requiring any special
> > + * action from the application. Once the application has released all the
> > + * references it held to cameras, the camera manager can be stopped with
> > + * stop().
> > *
> > * \todo Add ability to add and remove media devices based on hot-(un)plug
> > * events coming from the device enumerator.
> > @@ -147,15 +149,13 @@ void CameraManager::stop()
> > * \param[in] name Name of camera to get
> > *
> > * Before calling this function the caller is responsible for ensuring that
> > - * the camera manger is running. A camera fetched this way shall be
> > - * released by the user with the put() method of the Camera object once
> > - * it is done using the camera.
> > + * the camera manger is running.
> > *
> > - * \return Pointer to Camera object or nullptr if camera not found
> > + * \return Shared pointer to Camera object or nullptr if camera not found
> > */
> > -Camera *CameraManager::get(const std::string &name)
> > +std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> > {
> > - for (Camera *camera : cameras_) {
> > + for (std::shared_ptr<Camera> camera : cameras_) {
> > if (camera->name() == name)
> > return camera;
> > }
> > @@ -171,7 +171,7 @@ Camera *CameraManager::get(const std::string &name)
> > * handle with the camera manager. Registered cameras are immediately made
> > * available to the system.
> > */
> > -void CameraManager::addCamera(Camera *camera)
> > +void CameraManager::addCamera(std::shared_ptr<Camera> &camera)
> > {
> > cameras_.push_back(camera);
> > }
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 8742e0bae9a8..306a5b85cd60 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -64,7 +64,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator
> > * will be chosen depends on how the Camera
> > * object is modeled.
> > */
> > - Camera *camera = new Camera("Dummy VIMC Camera");
> > + std::shared_ptr<Camera> camera = Camera::create("Dummy VIMC Camera");
> > manager->addCamera(camera);
> >
> > return true;
> > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> > index fdbbda0957b2..070cbf2be977 100644
> > --- a/test/list-cameras.cpp
> > +++ b/test/list-cameras.cpp
> > @@ -30,7 +30,7 @@ protected:
> > {
> > unsigned int count = 0;
> >
> > - for (Camera *camera : cm->cameras()) {
> > + for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
> > cout << "- " << camera->name() << endl;
> > count++;
> > }
>
> These are just minor comments, documentation apart, feel free to add
> my tag to the whole series :)
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list