[libcamera-devel] [PATCH 4/4] libcamera: camera: Handle camera objects through shared pointers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 22 13:22:12 CET 2019
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.
> > 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
More information about the libcamera-devel
mailing list