[libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera objects through shared pointers
Ricky Liang
jcliang at chromium.org
Fri Jan 18 18:39:35 CET 2019
Hi Laurent,
On Fri, Jan 18, 2019 at 7:59 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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
> {
> 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;
We probably want to delete the implicit constructor as well:
Camera() = delete;
>
> const std::string &name() const;
> - void get();
> - void put();
>
> private:
> - virtual ~Camera() { };
> - int ref_;
> + Camera(const std::string &name);
Can we add the 'explicit' specifier for constructor with only one
argument? Not super important for this patch as it's a private
constructor here, but may be a good general guideline.
> + ~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);
Some high level design questions:
1. Who are the callers of these getter methods?
2. What's the threading model that we plan to use for exercising the
Camera objects?
While making Camera objects as shared pointers makes the coding
easier, it may also be hard to track the ownership of the objects once
the code base gets large. In some cases it may lead to memory/fd leaks
that are hard to debug, e.g. a singleton object holding a shared
pointer reference to an object and never releases the reference.
I tend to agree that fundamental objects like Camera has the necessity
to be shared pointers, but would also like to know if it's possible to
design the threading model such that CameraManager is the main owner
of the Camera objects. For example, if we can design it such that all
the access to Camera objects is serialized on a thread, then we
probably don't need to use shared pointers. Or, we can achieve
concurrency by storing Camera objects as std::shared_ptr in
CameraManager and only giving out std::weak_ptr of the Camera objects.
>
> - void addCamera(Camera *camera);
> + void addCamera(std::shared_ptr<Camera> &camera);
We can pass by value here so that we provide the caller with the
freedom to pass its own shared ownership to the callee through
std::move().
>
> 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_;
If the camera name is required to be unique then perhaps we can store
the cameras as
std::unordered_map<std::string, 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> {
> + void construct(void *p, const std::string &name)
> + {
> + ::new(p) Camera(name);
> + }
> + 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++;
> }
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list