[libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager: Move private data members to private implementation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jan 23 21:35:32 CET 2020
Hi Kieran,
On Thu, Jan 23, 2020 at 02:35:54PM +0000, Kieran Bingham wrote:
> On 22/01/2020 20:57, 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
>
> Trying to escape vim? ":q" ?
Oops :-)
> Perhaps this was a squashed commit or a fixup?
>
> Otherwise, It's certainly interesting moving things to an internal
> private class. The public objects then just become lightweight wrappers.
In the CameraManager case I need an internal class inheriting from
Thread for a subsequent patch in this series, so it makes sense to use
it to hide the details from the public API and help with ABI stability.
> Seems like it might end up generating a lot of boilerplate wrapping
> around classes, but getting some ABI stability will be a good thing...
>
> It looks like the actual code is only moved, so there's no functional
> changes intended here (except of course for the allocations being
> different and wrapped).
Correct.
> So a generalised:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > 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 \
>
> I think I saw another patch from you that used ::details for something
> similar to this.
>
> Will ::Private be the preferred naming? Or will both end up being used?
Qt would have used CameraManagerPrivate. I like CameraManager::Private
because it's shorter in contexts where CameraManager is the active name
space. details:: come from C++ STL and is used to hide implementation
details from the std:: namespace. I'd rather use it for the same purpose
when needed (to support templates with helpers that need to be present
in public headers but are not part of the official API), but I'm open to
discussing this. In any case this is a class, so it would need to be
write CameraManager::Details, and in that case I think Private is a
better match (but I may be biased).
> > 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)
>
> A private private? :-D (I see it's the constructor it just reads oddly
> because of the namespacing.)
:-) I think CameraManagerPrivate::CameraManagerPrivate is a bit worse
(at the very least it's longer), but I could again be biased.
> > + : 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))
>
> The std::unique_ptr<> will take care of free-ing this allocation right?
Correct.
> > {
> > 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
More information about the libcamera-devel
mailing list