[libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera objects through shared pointers
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 18 16:39:56 CET 2019
Hi Laurent
thanks, very nice :)
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.
> {
> 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?
>
> - 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?
> + void construct(void *p, const std::string &name)
> + {
> + ::new(p) Camera(name);
This "new(p)" scares me :)
> + }
> + 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>
Thanks
j
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190118/6e20fe7f/attachment.sig>
More information about the libcamera-devel
mailing list