[libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager: Move private data members to private implementation
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Jan 23 21:30:34 CET 2020
Hi Laurent,
Thanks for your patch.
On 2020-01-22 22:57:12 +0200, Laurent Pinchart wrote:
> Use the d-pointer idiom ([1], [2]) to hide the private data members from
> the CameraManager class interface. This will ease maintaining ABI
> compatibility, and prepares for the implementation of the CameraManager
> class threading model.
>
> [1] https://wiki.qt.io/D-Pointer
> [2] https://en.cppreference.com/w/cpp/language/pimpl
>
> libcamera: camera_manager: Run the camera manager in a thread:q
As Kieran points out I assume this line is not meant to be here :-)
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
With that fixed,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> Documentation/Doxyfile.in | 1 +
> include/libcamera/camera_manager.h | 13 +-
> src/libcamera/camera_manager.cpp | 218 ++++++++++++++++++-----------
> 3 files changed, 143 insertions(+), 89 deletions(-)
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 8e6fbdbb92b6..1f746393568a 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -877,6 +877,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \
> libcamera::BoundMethodPackBase \
> libcamera::BoundMethodStatic \
> libcamera::SignalBase \
> + libcamera::*::Private \
> std::*
>
> # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 094197668698..068afd58762f 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -7,7 +7,6 @@
> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
> #define __LIBCAMERA_CAMERA_MANAGER_H__
>
> -#include <map>
> #include <memory>
> #include <string>
> #include <sys/types.h>
> @@ -18,9 +17,7 @@
> namespace libcamera {
>
> class Camera;
> -class DeviceEnumerator;
> class EventDispatcher;
> -class PipelineHandler;
>
> class CameraManager : public Object
> {
> @@ -33,7 +30,7 @@ public:
> int start();
> void stop();
>
> - const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> + const std::vector<std::shared_ptr<Camera>> &cameras() const;
> std::shared_ptr<Camera> get(const std::string &name);
> std::shared_ptr<Camera> get(dev_t devnum);
>
> @@ -46,13 +43,11 @@ public:
> EventDispatcher *eventDispatcher();
>
> private:
> - std::unique_ptr<DeviceEnumerator> enumerator_;
> - std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> - std::vector<std::shared_ptr<Camera>> cameras_;
> - std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
> -
> static const std::string version_;
> static CameraManager *self_;
> +
> + class Private;
> + std::unique_ptr<Private> p_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index a71caf8536bb..e0a07ec557d3 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -7,6 +7,8 @@
>
> #include <libcamera/camera_manager.h>
>
> +#include <map>
> +
> #include <libcamera/camera.h>
> #include <libcamera/event_dispatcher.h>
>
> @@ -26,6 +28,124 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(Camera)
>
> +class CameraManager::Private
> +{
> +public:
> + Private(CameraManager *cm);
> +
> + int start();
> + void stop();
> +
> + void addCamera(std::shared_ptr<Camera> &camera, dev_t devnum);
> + void removeCamera(Camera *camera);
> +
> + std::vector<std::shared_ptr<Camera>> cameras_;
> + std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
> +
> +private:
> + CameraManager *cm_;
> +
> + std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> + std::unique_ptr<DeviceEnumerator> enumerator_;
> +};
> +
> +CameraManager::Private::Private(CameraManager *cm)
> + : cm_(cm)
> +{
> +}
> +
> +int CameraManager::Private::start()
> +{
> + enumerator_ = DeviceEnumerator::create();
> + if (!enumerator_ || enumerator_->enumerate())
> + return -ENODEV;
> +
> + /*
> + * TODO: Try to read handlers and order from configuration
> + * file and only fallback on all handlers if there is no
> + * configuration file.
> + */
> + std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> +
> + for (PipelineHandlerFactory *factory : factories) {
> + /*
> + * Try each pipeline handler until it exhaust
> + * all pipelines it can provide.
> + */
> + while (1) {
> + std::shared_ptr<PipelineHandler> pipe = factory->create(cm_);
> + if (!pipe->match(enumerator_.get()))
> + break;
> +
> + LOG(Camera, Debug)
> + << "Pipeline handler \"" << factory->name()
> + << "\" matched";
> + pipes_.push_back(std::move(pipe));
> + }
> + }
> +
> + /* TODO: register hot-plug callback here */
> +
> + return 0;
> +}
> +
> +void CameraManager::Private::stop()
> +{
> + /* TODO: unregister hot-plug callback here */
> +
> + /*
> + * Release all references to cameras and pipeline handlers to ensure
> + * they all get destroyed before the device enumerator deletes the
> + * media devices.
> + */
> + pipes_.clear();
> + cameras_.clear();
> +
> + enumerator_.reset(nullptr);
> +}
> +
> +void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> + dev_t devnum)
> +{
> + for (std::shared_ptr<Camera> c : cameras_) {
> + if (c->name() == camera->name()) {
> + LOG(Camera, Warning)
> + << "Registering camera with duplicate name '"
> + << camera->name() << "'";
> + break;
> + }
> + }
> +
> + cameras_.push_back(std::move(camera));
> +
> + if (devnum) {
> + unsigned int index = cameras_.size() - 1;
> + camerasByDevnum_[devnum] = cameras_[index];
> + }
> +}
> +
> +void CameraManager::Private::removeCamera(Camera *camera)
> +{
> + auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> + [camera](std::shared_ptr<Camera> &c) {
> + return c.get() == camera;
> + });
> + if (iter == cameras_.end())
> + return;
> +
> + LOG(Camera, Debug)
> + << "Unregistering camera '" << camera->name() << "'";
> +
> + auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
> + [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
> + return p.second.lock().get() == camera;
> + });
> + if (iter_d != camerasByDevnum_.end())
> + camerasByDevnum_.erase(iter_d);
> +
> + cameras_.erase(iter);
> +}
> +
> /**
> * \class CameraManager
> * \brief Provide access and manage all cameras in the system
> @@ -62,7 +182,7 @@ LOG_DEFINE_CATEGORY(Camera)
> CameraManager *CameraManager::self_ = nullptr;
>
> CameraManager::CameraManager()
> - : enumerator_(nullptr)
> + : p_(new CameraManager::Private(this))
> {
> if (self_)
> LOG(Camera, Fatal)
> @@ -73,6 +193,8 @@ CameraManager::CameraManager()
>
> CameraManager::~CameraManager()
> {
> + stop();
> +
> self_ = nullptr;
> }
>
> @@ -88,42 +210,14 @@ CameraManager::~CameraManager()
> */
> int CameraManager::start()
> {
> - if (enumerator_)
> - return -EBUSY;
> -
> LOG(Camera, Info) << "libcamera " << version_;
>
> - enumerator_ = DeviceEnumerator::create();
> - if (!enumerator_ || enumerator_->enumerate())
> - return -ENODEV;
> -
> - /*
> - * TODO: Try to read handlers and order from configuration
> - * file and only fallback on all handlers if there is no
> - * configuration file.
> - */
> - std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> + int ret = p_->start();
> + if (ret)
> + LOG(Camera, Error) << "Failed to start camera manager: "
> + << strerror(-ret);
>
> - for (PipelineHandlerFactory *factory : factories) {
> - /*
> - * Try each pipeline handler until it exhaust
> - * all pipelines it can provide.
> - */
> - while (1) {
> - std::shared_ptr<PipelineHandler> pipe = factory->create(this);
> - if (!pipe->match(enumerator_.get()))
> - break;
> -
> - LOG(Camera, Debug)
> - << "Pipeline handler \"" << factory->name()
> - << "\" matched";
> - pipes_.push_back(std::move(pipe));
> - }
> - }
> -
> - /* TODO: register hot-plug callback here */
> -
> - return 0;
> + return ret;
> }
>
> /**
> @@ -138,17 +232,7 @@ int CameraManager::start()
> */
> void CameraManager::stop()
> {
> - /* TODO: unregister hot-plug callback here */
> -
> - /*
> - * Release all references to cameras and pipeline handlers to ensure
> - * they all get destroyed before the device enumerator deletes the
> - * media devices.
> - */
> - pipes_.clear();
> - cameras_.clear();
> -
> - enumerator_.reset(nullptr);
> + p_->stop();
> }
>
> /**
> @@ -160,6 +244,10 @@ void CameraManager::stop()
> *
> * \return List of all available cameras
> */
> +const std::vector<std::shared_ptr<Camera>> &CameraManager::cameras() const
> +{
> + return p_->cameras_;
> +}
>
> /**
> * \brief Get a camera based on name
> @@ -172,7 +260,7 @@ void CameraManager::stop()
> */
> std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> {
> - for (std::shared_ptr<Camera> camera : cameras_) {
> + for (std::shared_ptr<Camera> camera : p_->cameras_) {
> if (camera->name() == name)
> return camera;
> }
> @@ -196,8 +284,8 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> */
> std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> {
> - auto iter = camerasByDevnum_.find(devnum);
> - if (iter == camerasByDevnum_.end())
> + auto iter = p_->camerasByDevnum_.find(devnum);
> + if (iter == p_->camerasByDevnum_.end())
> return nullptr;
>
> return iter->second.lock();
> @@ -217,21 +305,8 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> */
> void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
> {
> - for (std::shared_ptr<Camera> c : cameras_) {
> - if (c->name() == camera->name()) {
> - LOG(Camera, Warning)
> - << "Registering camera with duplicate name '"
> - << camera->name() << "'";
> - break;
> - }
> - }
>
> - cameras_.push_back(std::move(camera));
> -
> - if (devnum) {
> - unsigned int index = cameras_.size() - 1;
> - camerasByDevnum_[devnum] = cameras_[index];
> - }
> + p_->addCamera(camera, devnum);
> }
>
> /**
> @@ -244,24 +319,7 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
> */
> void CameraManager::removeCamera(Camera *camera)
> {
> - auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> - [camera](std::shared_ptr<Camera> &c) {
> - return c.get() == camera;
> - });
> - if (iter == cameras_.end())
> - return;
> -
> - LOG(Camera, Debug)
> - << "Unregistering camera '" << camera->name() << "'";
> -
> - auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
> - [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
> - return p.second.lock().get() == camera;
> - });
> - if (iter_d != camerasByDevnum_.end())
> - camerasByDevnum_.erase(iter_d);
> -
> - cameras_.erase(iter);
> + p_->removeCamera(camera);
> }
>
> /**
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list