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

[Tomasz Figa]

On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Ricky,
>
> On Tue, Jan 22, 2019 at 11:19:01AM +0800, Ricky Liang wrote:
> >  On Sat, Jan 19, 2019 at 6:51 AM Laurent Pinchart
> >  <laurent.pinchart at ideasonboard.com> wrote:
> > > 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.
>
> Correct, the camera object would be marked as disconnected, API calls
> would start failing, and when the camera is released by the application
> it will be deleted.
>
> >  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?
>
> I meant hotplugging a USB webcam for instance, but from a userspace
> point of view unbinding a device from its kernel driver should achieve
> the same effect. I'd like pipeline handlers to handle that gracefully if
> it happens, but it's not a priority.
>
> We don't plan to support concurrent access to a camera by multiple
> applications, that would be handled by a layer above libcamera if
> needed.

So I'm actually wondering if we're trying to solve a real problem
here. Physical camera hotplug is already handled for us by the kernel.
As long as the userspace has the V4L2 file descriptors open, it can
interact with them and get error codes when the camera goes away. We
could just put our internal objects in some error state, fail any
pending capture requests and have any further calls on this Camera
object just fail, in a way that would explicitly tell the consumer
that the reason for the failure was disconnection. Then it would be up
to the consumer to actually destroy the Camera object.

Best regards,
Tomasz