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

[Ricky Liang]

Hi Laurent,

On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

Ack.

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

I'd imagine in the hot-pluggable camera case, the action of actually
removing the camera (e.g. un-plugging the USB camera) would make the
camera no longer usable and what we need would be to handle the
exception gracefully.

Or, do you mean hot-pluggable in the software sense, like the
bind/unbind in kernel drivers? Most of the camera devices allow only
one active user at a time. Do we plan to design the APIs to allow
concurrent access to a camera?

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

Ack.

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

Ack.

>
> > >
> > >         std::unique_ptr<EventDispatcher> dispatcher_;
> > >  };
>
> --
> Regards,
>
> Laurent Pinchart