[libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera objects through shared pointers

[Laurent Pinchart]

Hi Ricky,

On Sat, Jan 19, 2019 at 01:39:35AM +0800, Ricky Liang wrote:
> 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;

The compiler only creates an implicit constructor when no explicit
constructor is specified. As we define Camera(const std::string &name),
there's no need to delete the implicit constructor.

> >
> >         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.

Good point, I'll do that.

> > +       ~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?

These methods are meant to be called by applications. I expect the API
to change as we continue developing though, but the general idea will
stay the same.

> 2. What's the threading model that we plan to use for exercising the
> Camera objects?

We don't support multiple threads yet. This will be fixed down the road,
but not all details have been decided yet.

> 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,

I think they will, possibly with the exception of request queuing and
request completion handling. This remains to be decided. In any case, I
don't think we should support camera lookup from multiple threads.

> 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.

It's not just a concurrency issue. Support for hot-pluggable cameras
will be added in the future, and we'll need to track users of camera
objects in order to delete the camera only when no users are left. This
doesn't require std::shared_ptr<>, a manual reference count would work
too (and it exists today, this patch is replacing it), but I think it's
better to use std::shared_ptr<> rather than reinventing (and
open-coding) the wheel.

Giving out weak pointers would I think cause more issues than it would
solve, as any user of a camera instance would need to be prepared for
the pointer becoming null at any time when the camera is unplugged.

> > -       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().

In the typical use case the caller will likely retain a shared pointer
to the camera. I can pass by value and use std::move() in the
addCamera() function.

> >
> >         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_;

We used to store them in a std::map<>, but it make it cumbersome to
retrieve the list of all cameras. As I expect the camera lookup API to
change anyway I think we can revisit this later.

> >
> >         std::unique_ptr<EventDispatcher> dispatcher_;
> >  };

-- 
Regards,

Laurent Pinchart