From patchwork Fri Jan 18 23:26:14 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 275 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 80A9960C78 for ; Sat, 19 Jan 2019 00:26:21 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B8DA53F for ; Sat, 19 Jan 2019 00:26:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1547853981; bh=zAKRI7atiW0LPy9DC4Rg7GRvlrl1BUFgEErdv6hVTkc=; h=From:To:Subject:Date:In-Reply-To:References:From; b=rcdte6FJ9aplpEqeddJaVx6db218BtHDYqhrRW4bCMAOH9y0+epZ8tZGj5ztJql+o xIi2mf2h/AdoeBbO2ImN2Doy3ObTkciQyiIZOdtzjNpa8xfzDK8ssvhy2mi0E5Ra7h U26Ehgu3gVFrYObqg0Hl3+7TprPSQsn3zyQVHkbA= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Sat, 19 Jan 2019 01:26:14 +0200 Message-Id: <20190118232617.14631-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.19.2 In-Reply-To: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> References: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 1/4] Documentation: coding_style: Add object ownership rules X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Jan 2019 23:26:21 -0000 Object ownership is a complex topic that can lead to many issues, from memory leak to crashes. Document the rules that libcamera enforces to make object ownership tracking explicit. This is a first version of the rules and is expected to be expanded as the library is developed. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund Reviewed-by: Jacopo Mondi --- Changes since v1: - Clarify documentation - Reference the object ownership rules from the Chromium C++ style guide --- Documentation/coding-style.rst | 84 ++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst index f8d2fdfeda8e..66db3cebe132 100644 --- a/Documentation/coding-style.rst +++ b/Documentation/coding-style.rst @@ -81,6 +81,90 @@ C++-11-specific features: overused. * Variadic class and function templates +Object Ownership +~~~~~~~~~~~~~~~~ + +libcamera creates and destroys many objects at runtime, for both objects +internal to the library and objects exposed to the user. To guarantee proper +operation without use after free, double free or memory leaks, knowing who owns +each object at any time is crucial. The project has enacted a set of rules to +make object ownership tracking as explicit and fool-proof as possible. + +In the context of this section, the terms object and instance are used +interchangeably and both refer to an instance of a class. The term reference +refers to both C++ references and C++ pointers in their capacity to refer to an +object. Passing a reference means offering a way to a callee to obtain a +reference to an object that the caller has a valid reference to. Borrowing a +reference means using a reference passed by a caller without ownership transfer +based on the assumption that the caller guarantees the validity of the +reference for the duration of the operation that borrows it. + +#. Single Owner Objects + + * By default an object has a single owner at any time. + * Storage of single owner objects varies depending on how the object + ownership will evolve through the lifetime of the object. + + * Objects whose ownership needs to be transferred shall be stored as + std::unique_ptr<> as much as possible to emphasize the single ownership. + * Objects whose owner doesn't change may be embedded in other objects, or + stored as pointer or references. They may be stored as std::unique_ptr<> + for automatic deletion if desired. + + * Ownership is transferred by passing the reference as a std::unique_ptr<> + and using std::move(). After ownership transfer the former owner has no + valid reference to the object anymore and shall not access it without first + obtaining a valid reference. + * Objects may be borrowed by passing an object reference from the owner to + the borrower, providing that + + * the owner guarantees the validity of the reference for the whole duration + of the borrowing, and + * the borrower doesn't access the reference after the end of the borrowing. + + When borrowing from caller to callee for the duration of a function call, + this implies that the callee shall not keep any stored reference after it + returns. These rules apply to the callee and all the functions it calls, + directly or indirectly. + + When the object is stored in a std::unique_ptr<>, borrowing passes a + reference to the object, not to the std::unique_ptr<>, as + + * a 'const &' when the object doesn't need to be modified and may not be + null. + * a pointer when the object may be modified or may be null. Unless + otherwise specified, pointers passed to functions are considered as + borrowed references valid for the duration of the function only. + +#. Shared Objects + + * Objects that may have multiple owners at a given time are called shared + objects. They are reference-counted and live as long as any references to + the object exist. + * Shared objects are created with std::make_shared<> or + std::allocate_shared<> and stored in an std::shared_ptr<>. + * Ownership is shared by creating and passing copies of any valid + std::shared_ptr<>. Ownership is released by destroying the corresponding + std::shared_ptr<>. + * When passed to a function, std::shared_ptr<> are always passed by value, + never by reference. The caller can decide whether to transfer its ownership + of the std::shared_ptr<> with std::move() or retain it. The callee shall + use std::move() if it needs to store the shared pointer. + * Borrowed references to shared objects are passed as references to the + objects themselves, not to the std::shared_ptr<>, with the same rules as + for single owner objects. + +These rules match the `object ownership rules from the Chromium C++ Style Guide`_. + +.. _object ownership rules from the Chromium C++ Style Guide: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions + +.. attention:: Long term borrowing of single owner objects is allowed. Example + use cases are implementation of the singleton pattern (where the singleton + guarantees the validity of the reference forever), or returning references + to global objects whose lifetime matches the lifetime of the application. As + long term borrowing isn't marked through language constructs, it shall be + documented explicitly in details in the API. + Tools ----- From patchwork Fri Jan 18 23:26:15 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 276 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id DD7F560C78 for ; Sat, 19 Jan 2019 00:26:21 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 5353253E for ; Sat, 19 Jan 2019 00:26:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1547853981; bh=9Djyc7v/dOhluNfzsyGTNJdHSRrI7VLC07N9DnanbEo=; h=From:To:Subject:Date:In-Reply-To:References:From; b=KnUkKjC4eJzkzaCSDdS78rHz4WW6eJ+XPtKVr9Y9khmgJJnly413mz7xcy8BY532U jX59VynCaoKaMsM7hw+rksCgSitsRNRnMredVJufGToPL1z/BcyX6InSyEQjBYduqM KsHf4FDWdtuPFLUxunhBcgmwSOPmJgMaLSVhtqTs= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Sat, 19 Jan 2019 01:26:15 +0200 Message-Id: <20190118232617.14631-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.19.2 In-Reply-To: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> References: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 2/4] libcamera: camera_manager: Use std::unique_ptr to store event dispatcher X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Jan 2019 23:26:22 -0000 The CameraManager takes ownership of the dispatcher passed to the setEventDispatcher() function. Enforces this by using std::unique_ptr<>. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund Reviewed-by: Jacopo Mondi --- Changes since v1: - Use make_unique<> in CameraManager::setEventDispatcher() --- include/libcamera/camera_manager.h | 4 ++-- src/libcamera/camera_manager.cpp | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 6cfcba3c3933..b82a8ce95b9f 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -29,7 +29,7 @@ public: static CameraManager *instance(); - void setEventDispatcher(EventDispatcher *dispatcher); + void setEventDispatcher(std::unique_ptr dispatcher); EventDispatcher *eventDispatcher(); private: @@ -41,7 +41,7 @@ private: std::unique_ptr enumerator_; std::vector pipes_; - EventDispatcher *dispatcher_; + std::unique_ptr dispatcher_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 9f554be57e33..1430bb0d75a5 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -12,6 +12,7 @@ #include "event_dispatcher_poll.h" #include "log.h" #include "pipeline_handler.h" +#include "utils.h" /** * \file camera_manager.h @@ -58,7 +59,6 @@ CameraManager::CameraManager() CameraManager::~CameraManager() { - delete dispatcher_; } /** @@ -209,14 +209,14 @@ CameraManager *CameraManager::instance() * The CameraManager takes ownership of the event dispatcher and will delete it * when the application terminates. */ -void CameraManager::setEventDispatcher(EventDispatcher *dispatcher) +void CameraManager::setEventDispatcher(std::unique_ptr dispatcher) { if (dispatcher_) { LOG(Warning) << "Event dispatcher is already set"; return; } - dispatcher_ = dispatcher; + dispatcher_ = std::move(dispatcher); } /** @@ -226,14 +226,16 @@ void CameraManager::setEventDispatcher(EventDispatcher *dispatcher) * If no dispatcher has been set, a default poll-based implementation is created * and returned, and no custom event dispatcher may be installed anymore. * + * The returned event dispatcher is valid until the camera manager is destroyed. + * * \return Pointer to the event dispatcher */ EventDispatcher *CameraManager::eventDispatcher() { if (!dispatcher_) - dispatcher_ = new EventDispatcherPoll(); + dispatcher_ = utils::make_unique(); - return dispatcher_; + return dispatcher_.get(); } } /* namespace libcamera */ From patchwork Fri Jan 18 23:26:16 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 277 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 461C360C78 for ; Sat, 19 Jan 2019 00:26:22 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D549053F for ; Sat, 19 Jan 2019 00:26:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1547853982; bh=q9SFLfoBCQsnaIxGAmEUw0C6N1tUFfYwwgSB/8P1JuQ=; h=From:To:Subject:Date:In-Reply-To:References:From; b=RsVasJK8uGkT6WhewZVQe6iLJVrBD08XHLf8yqtbQ0obrQJIoRJFrCU1qRpdIaMZl 1Z9U6WD9W+G6lKkpOawZnNePRynxc3gJPL2BLrsJue9OTvUzTHwsPhawV6CqPEq6+n J7J3GSKO5Bo2ZT9Oxq4BM0bYk0hMop7jPbysmmvM= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Sat, 19 Jan 2019 01:26:16 +0200 Message-Id: <20190118232617.14631-4-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.19.2 In-Reply-To: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> References: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 3/4] libcamera: camera_manager: Register cameras with the camera manager X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Jan 2019 23:26:23 -0000 Cameras are listed through a double indirection, first iterating over all available pipeline handlers, and then listing the cameras they each support. To simplify the API make the pipeline handlers register the cameras with the camera manager directly, which lets the camera manager easily expose the list of all available cameras. The PipelineHandler API gets simplified as the handlers don't need to expose the list of cameras they have created. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund Reviewed-by: Jacopo Mondi --- Changes since v1: - Warn when registering a camera with a duplicate name --- include/libcamera/camera_manager.h | 5 ++- src/libcamera/camera_manager.cpp | 52 +++++++++++++----------- src/libcamera/include/pipeline_handler.h | 6 +-- src/libcamera/pipeline/vimc.cpp | 30 +++----------- src/libcamera/pipeline_handler.cpp | 21 +++------- test/list-cameras.cpp | 5 ++- 6 files changed, 48 insertions(+), 71 deletions(-) diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index b82a8ce95b9f..4b941fd9183b 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -24,9 +24,11 @@ public: int start(); void stop(); - std::vector list() const; + const std::vector &cameras() const { return cameras_; } Camera *get(const std::string &name); + void addCamera(Camera *camera); + static CameraManager *instance(); void setEventDispatcher(std::unique_ptr dispatcher); @@ -40,6 +42,7 @@ private: std::unique_ptr enumerator_; std::vector pipes_; + std::vector cameras_; std::unique_ptr dispatcher_; }; diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 1430bb0d75a5..3fa9f23c8e0d 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -94,7 +94,7 @@ int CameraManager::start() */ while (1) { PipelineHandler *pipe = factory->create(); - if (!pipe->match(enumerator_.get())) { + if (!pipe->match(this, enumerator_.get())) { delete pipe; break; } @@ -133,26 +133,14 @@ void CameraManager::stop() } /** - * \brief List all detected cameras + * \fn CameraManager::cameras() + * \brief Retrieve all available cameras * * Before calling this function the caller is responsible for ensuring that * the camera manger is running. * - * \return List of names for all detected cameras + * \return List of all available cameras */ -std::vector CameraManager::list() const -{ - std::vector list; - - for (PipelineHandler *pipe : pipes_) { - for (unsigned int i = 0; i < pipe->count(); i++) { - Camera *cam = pipe->camera(i); - list.push_back(cam->name()); - } - } - - return list; -} /** * \brief Get a camera based on name @@ -168,19 +156,35 @@ std::vector CameraManager::list() const */ Camera *CameraManager::get(const std::string &name) { - for (PipelineHandler *pipe : pipes_) { - for (unsigned int i = 0; i < pipe->count(); i++) { - Camera *cam = pipe->camera(i); - if (cam->name() == name) { - cam->get(); - return cam; - } - } + for (Camera *camera : cameras_) { + if (camera->name() == name) + return camera; } return nullptr; } +/** + * \brief Add a camera to the camera manager + * \param[in] camera The camera to be added + * + * This function is called by pipeline handlers to register the cameras they + * handle with the camera manager. Registered cameras are immediately made + * available to the system. + */ +void CameraManager::addCamera(Camera *camera) +{ + for (Camera *c : cameras_) { + if (c->name() == camera->name()) { + LOG(Warning) << "Registering camera with duplicate name '" + << camera->name() << "'"; + break; + } + } + + cameras_.push_back(camera); +} + /** * \brief Retrieve the camera manager instance * diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index e976aaa13546..f05f201f7ca8 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -15,6 +15,7 @@ namespace libcamera { +class CameraManager; class DeviceEnumerator; class PipelineHandler @@ -22,10 +23,7 @@ class PipelineHandler public: virtual ~PipelineHandler() { }; - virtual bool match(DeviceEnumerator *enumerator) = 0; - - virtual unsigned int count() = 0; - virtual Camera *camera(unsigned int id) = 0; + virtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0; }; class PipelineHandlerFactory diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 720d9c2031c9..8742e0bae9a8 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -6,6 +6,7 @@ */ #include +#include #include "device_enumerator.h" #include "media_device.h" @@ -19,44 +20,24 @@ public: PipeHandlerVimc(); ~PipeHandlerVimc(); - bool match(DeviceEnumerator *enumerator); - - unsigned int count(); - Camera *camera(unsigned int id) final; + bool match(CameraManager *manager, DeviceEnumerator *enumerator); private: MediaDevice *dev_; - Camera *camera_; }; PipeHandlerVimc::PipeHandlerVimc() - : dev_(nullptr), camera_(nullptr) + : dev_(nullptr) { } PipeHandlerVimc::~PipeHandlerVimc() { - if (camera_) - camera_->put(); - if (dev_) dev_->release(); } -unsigned int PipeHandlerVimc::count() -{ - return 1; -} - -Camera *PipeHandlerVimc::camera(unsigned int id) -{ - if (id != 0) - return nullptr; - - return camera_; -} - -bool PipeHandlerVimc::match(DeviceEnumerator *enumerator) +bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator) { DeviceMatch dm("vimc"); @@ -83,7 +64,8 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator) * will be chosen depends on how the Camera * object is modeled. */ - camera_ = new Camera("Dummy VIMC Camera"); + Camera *camera = new Camera("Dummy VIMC Camera"); + manager->addCamera(camera); return true; } diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index c19ab94f1774..f2e08a6a7315 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -35,13 +35,15 @@ namespace libcamera { /** * \fn PipelineHandler::match(DeviceEnumerator *enumerator) * \brief Match media devices and create camera instances + * \param manager The camera manager * \param enumerator The enumerator providing all media devices found in the * system * * This function is the main entry point of the pipeline handler. It is called - * by the camera manager with the \a enumerator passed as an argument. It - * shall acquire from the \a enumerator all the media devices it needs for a - * single pipeline and create one or multiple Camera instances. + * by the camera manager with the \a manager and \a enumerator passed as + * arguments. It shall acquire from the \a enumerator all the media devices it + * needs for a single pipeline, create one or multiple Camera instances and + * register them with the \a manager. * * If all media devices needed by the pipeline handler are found, they must all * be acquired by a call to MediaDevice::acquire(). This function shall then @@ -62,19 +64,6 @@ namespace libcamera { * created, or false otherwise */ -/** - * \fn PipelineHandler::count() - * \brief Retrieve the number of cameras handled by this pipeline handler - * \return the number of cameras that were created by the match() function - */ - -/** - * \fn PipelineHandler::camera(unsigned int id) - * \brief Retrieve one of the cameras handled by this pipeline handler - * \param[in] id the camera index - * \return a pointer to the Camera identified by \a id - */ - /** * \class PipelineHandlerFactory * \brief Registration of PipelineHandler classes and creation of instances diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp index e2026c99c5b8..fdbbda0957b2 100644 --- a/test/list-cameras.cpp +++ b/test/list-cameras.cpp @@ -7,6 +7,7 @@ #include +#include #include #include "test.h" @@ -29,8 +30,8 @@ protected: { unsigned int count = 0; - for (auto name : cm->list()) { - cout << "- " << name << endl; + for (Camera *camera : cm->cameras()) { + cout << "- " << camera->name() << endl; count++; } From patchwork Fri Jan 18 23:26:17 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 278 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D0BD60C9B for ; Sat, 19 Jan 2019 00:26:22 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 3267553E for ; Sat, 19 Jan 2019 00:26:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1547853982; bh=/5MlneqZLpR0AfZBUo1V/Snk2MBQ0uCwkLao+/Lrndo=; h=From:To:Subject:Date:In-Reply-To:References:From; b=KwbiUCocVgtTtC2BcbblXP18+yHwAcR5DbTHysVba1qagp0ctIXmwC+f2SQizFW8U IdDjPMgEKI1gdgt/t/1no7zGnLMTp+J4hI1qYEd4gyL2Kh2XOwfUHN1vWqgbUjvRso xorxH5fI/aoOB/E3WnkoFSd5JvtVYJKcS44Zug9U= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Sat, 19 Jan 2019 01:26:17 +0200 Message-Id: <20190118232617.14631-5-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.19.2 In-Reply-To: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> References: <20190118232617.14631-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 4/4] libcamera: camera: Handle camera objects through shared pointers X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Jan 2019 23:26:23 -0000 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. This API change prevents pipeline handlers from subclassing the Camera class. This isn't deemed to be an issue. Mark the class final to make this explicit. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- Changes since v1: - Make the Camera class final - Make the Camera::Camera(const std::string &) constructor explicit - Pass the std::shared_ptr<> by value to CameraManager::addCamera() --- include/libcamera/camera.h | 15 ++++++---- include/libcamera/camera_manager.h | 8 +++--- src/libcamera/camera.cpp | 46 +++++++++++++++++------------- src/libcamera/camera_manager.cpp | 24 ++++++++-------- src/libcamera/pipeline/vimc.cpp | 4 +-- test/list-cameras.cpp | 2 +- 6 files changed, 54 insertions(+), 45 deletions(-) diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 9a7579d61fa3..2ea1a6883311 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -7,22 +7,25 @@ #ifndef __LIBCAMERA_CAMERA_H__ #define __LIBCAMERA_CAMERA_H__ +#include #include namespace libcamera { -class Camera +class Camera final { public: - Camera(const std::string &name); + static std::shared_ptr create(const std::string &name); + + Camera(const Camera &) = delete; + void operator=(const Camera &) = delete; const std::string &name() const; - void get(); - void put(); private: - virtual ~Camera() { }; - int ref_; + explicit Camera(const std::string &name); + ~Camera(); + std::string name_; }; diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 4b941fd9183b..9ade29d76692 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 &cameras() const { return cameras_; } - Camera *get(const std::string &name); + const std::vector> &cameras() const { return cameras_; } + std::shared_ptr get(const std::string &name); - void addCamera(Camera *camera); + void addCamera(std::shared_ptr camera); static CameraManager *instance(); @@ -42,7 +42,7 @@ private: std::unique_ptr enumerator_; std::vector pipes_; - std::vector cameras_; + std::vector> cameras_; std::unique_ptr dispatcher_; }; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 6da2b20137d4..acf912bee95c 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -41,19 +41,36 @@ namespace libcamera { * streams from a single image source. It provides the main interface to * configuring and controlling the device, and capturing image streams. It is * the central object exposed by libcamera. + * + * To support the central nature of Camera objects, libcamera manages the + * lifetime of camera instances with std::shared_ptr<>. Instances shall be + * created with the create() function which returns a shared pointer. The + * Camera constructors and destructor are private, to prevent instances from + * being constructed and destroyed manually. */ /** - * \brief Construct a named camera device + * \brief Create a camera instance + * \param[in] name The name of the camera device * - * \param[in] name The name to set on the camera device + * The caller is responsible for guaranteeing unicity of the camera name. * - * The caller is responsible for guaranteeing unicity of the camera - * device name. + * \return A shared pointer to the newly created camera object */ -Camera::Camera(const std::string &name) - : ref_(1), name_(name) +std::shared_ptr Camera::create(const std::string &name) { + struct Allocator : std::allocator { + void construct(void *p, const std::string &name) + { + ::new(p) Camera(name); + } + void destroy(Camera *p) + { + p->~Camera(); + } + }; + + return std::allocate_shared(Allocator(), name); } /** @@ -66,24 +83,13 @@ const std::string &Camera::name() const return name_; } -/** - * \brief Acquire a reference to the camera - */ -void Camera::get() +Camera::Camera(const std::string &name) + : name_(name) { - ref_++; } -/** - * \brief Release a reference to the camera - * - * When the last reference is released the camera device is deleted. Callers - * shall not access the camera device after calling this function. - */ -void Camera::put() +Camera::~Camera() { - if (--ref_ == 0) - delete this; } } /* namespace libcamera */ diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 3fa9f23c8e0d..d76eaa7ace86 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -40,9 +40,11 @@ namespace libcamera { * This will enumerate all the cameras present in the system, which can then be * listed with list() and retrieved with get(). * - * Cameras are reference-counted, and shall be returned to the camera manager - * with Camera::put() after being used. Once all cameras have been returned to - * the manager, it can be stopped with stop(). + * Cameras are shared through std::shared_ptr<>, ensuring that a camera will + * stay valid until the last reference is released without requiring any special + * action from the application. Once the application has released all the + * references it held to cameras, the camera manager can be stopped with + * stop(). * * \todo Add ability to add and remove media devices based on hot-(un)plug * events coming from the device enumerator. @@ -148,15 +150,13 @@ void CameraManager::stop() * \param[in] name Name of camera to get * * Before calling this function the caller is responsible for ensuring that - * the camera manger is running. A camera fetched this way shall be - * released by the user with the put() method of the Camera object once - * it is done using the camera. + * the camera manger is running. * - * \return Pointer to Camera object or nullptr if camera not found + * \return Shared pointer to Camera object or nullptr if camera not found */ -Camera *CameraManager::get(const std::string &name) +std::shared_ptr CameraManager::get(const std::string &name) { - for (Camera *camera : cameras_) { + for (std::shared_ptr camera : cameras_) { if (camera->name() == name) return camera; } @@ -172,9 +172,9 @@ Camera *CameraManager::get(const std::string &name) * handle with the camera manager. Registered cameras are immediately made * available to the system. */ -void CameraManager::addCamera(Camera *camera) +void CameraManager::addCamera(std::shared_ptr camera) { - for (Camera *c : cameras_) { + for (std::shared_ptr c : cameras_) { if (c->name() == camera->name()) { LOG(Warning) << "Registering camera with duplicate name '" << camera->name() << "'"; @@ -182,7 +182,7 @@ void CameraManager::addCamera(Camera *camera) } } - cameras_.push_back(camera); + cameras_.push_back(std::move(camera)); } /** diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 8742e0bae9a8..82b9237a3d7d 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -64,8 +64,8 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator * will be chosen depends on how the Camera * object is modeled. */ - Camera *camera = new Camera("Dummy VIMC Camera"); - manager->addCamera(camera); + std::shared_ptr camera = Camera::create("Dummy VIMC Camera"); + manager->addCamera(std::move(camera)); return true; } diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp index fdbbda0957b2..070cbf2be977 100644 --- a/test/list-cameras.cpp +++ b/test/list-cameras.cpp @@ -30,7 +30,7 @@ protected: { unsigned int count = 0; - for (Camera *camera : cm->cameras()) { + for (const std::shared_ptr &camera : cm->cameras()) { cout << "- " << camera->name() << endl; count++; }