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

[Laurent Pinchart]

Hi Tomasz,

On Mon, Jan 28, 2019 at 04:53:39PM +0900, Tomasz Figa wrote:
> On Fri, Jan 25, 2019 at 7:34 PM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> > On Fri, Jan 25, 2019 at 03:55:12PM +0900, Tomasz Figa wrote:
> >> On Tue, Jan 22, 2019 at 9:22 PM Laurent Pinchart
> >> <laurent.pinchart at ideasonboard.com> wrote:
> >>> 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.
> >
> > One core design principle in libcamera is that the library enumerates
> > cameras by scanning for media devices (in the MC sense) and associating
> > them with pipeline handlers (which can be seen as a kind of userspace
> > counterpart of the kernel drivers, exposing various MC device nodes as
> > higher level camera objects, the same way a kernel driver exposes
> > various pieces of hardware as standardized device nodes). The library is
> > thus responsible for creating the camera objects, which are then
> > retrieved and used by applications. In the context of hotplug handling
> > we need to signal hot-unplug to applications, certainly in the form of
> > completing pending capture requests with an error and failing futher
> > calls, but I think also as an explicit signal to the application. For
> > this to work I believe handling camera objects as shared pointers is the
> > best option, as the consumer has to release the object, but shouldn't
> > destroy it. Am I biased ? :-)
> 
> I'd give the consumer an object it can destroy, which only points to
> the camera object created by the framework. That would eliminate the
> shared pointers from the public API, but still give you the ability to
> use them internally.

That's an interesting idea. I'll give it a try, even if it means I'll
have to find two class names for the external and internal camera
objects :-)

> For any signals to the application, we need some event loop on the
> application side anyway, which would poll the object for messages, so
> that should work as well with this kind of "consumer" object.

libcamera already contains an event dispatcher API that allows the
library to plug itself in the application event loop (with a default
poll-based event loop provided by libcamera itself to ease application
development), and a signal/slot mechanism inspired by Qt. I think that
will be enough to signal camera disconnection, even though more work
will likely be needed to support multi-threaded applications in this
regard.

-- 
Regards,

Laurent Pinchart