[libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager: Make the class a singleton
Ricky Liang
jcliang at chromium.org
Tue Jan 15 15:26:14 CET 2019
+ Shik, who has some ideas about the C++ version to use and useful
third-party C++ libraries
On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang <jcliang at chromium.org> wrote:
>
> Hi Laurent,
>
> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi Ricky,
> >
> > On Monday, 14 January 2019 10:19:50 EET Ricky Liang wrote:
> > > On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart wrote:
> > > > On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:
> > > >> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:
> > > >>> There can only be a single camera manager instance in the application.
> > > >>> Creating it as a singleton helps avoiding mistakes. It also allows the
> > > >>> camera manager to be used as a storage of global data, such as the
> > > >>> future event dispatcher.
> > > >>>
> > > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > >>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > >>> ---
> > > >>> Changes since v1:
> > > >>>
> > > >>> - Delete copy constructor and assignment operator
> > > >>> - Fix documentation style issue
> > > >>> ---
> > > >>>
> > > >>> include/libcamera/camera_manager.h | 8 ++++++--
> > > >>> src/libcamera/camera_manager.cpp | 15 +++++++++++++++
> > > >>> test/list.cpp | 7 +------
> > > >>> 3 files changed, 22 insertions(+), 8 deletions(-)
> > > >>>
> > > >>> diff --git a/include/libcamera/camera_manager.h
> > > >>> b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3
> > > >>> 100644
> > > >>> --- a/include/libcamera/camera_manager.h
> > > >>> +++ b/include/libcamera/camera_manager.h
> > > >>> @@ -19,15 +19,19 @@ class PipelineHandler;
> > > >>> class CameraManager
> > > >>> {
> > > >>> public:
> > > >>> - CameraManager();
> > > >>> -
> > > >>> int start();
> > > >>> void stop();
> > > >>>
> > > >>> std::vector<std::string> list() const;
> > > >>> Camera *get(const std::string &name);
> > > >>>
> > > >>> + static CameraManager *instance();
> > > >>> +
> > > >>> private:
> > > >>> + CameraManager();
> > > >>> + CameraManager(const CameraManager &) = delete;
> > > >>> + void operator=(const CameraManager &) = delete;
> > > >>> +
> > > >>> DeviceEnumerator *enumerator_;
> > > >>> std::vector<PipelineHandler *> pipes_;
> > > >>
> > > >> Not directly related to this patch, but have we considered making
> > > >> these pointers std::unique_ptr? From our experience working in
> > > >> Chromium we find it informative, easier to follow the code, and less
> > > >> error-prone if an object's ownership can be clearly identified through
> > > >> an std::unique_ptr.
> > > >>
> > > >> For example, in the current libcamera code base I noticed there's a
> > > >> potential leak in DeviceEnumerator::create() if enumerator->init()
> > > >> fails:
> > > >>
> > > >> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerat
> > > >> or.c pp#n137
> > > >>
> > > >> If we instead only create and pass std::unique_ptr<DeviceEnumerator>
> > > >> around then we'd avoid leak like this, and people can also look at the
> > > >> code and clearly understands that CameraManager has the ownership of
> > > >> enumerator_.
> > > >
> > > > I agree with you, std::unique_ptr<> would here both provide the advantages
> > > > of a scoped pointer with automatic deletion, and make it clear who owns
> > > > the object. I gave it a try for enumerator_, and here is what I ended up
> > > > with (quote characters added to comment inline).
> > > >
> > > >> diff --git a/include/libcamera/camera_manager.h
> > > >> b/include/libcamera/camera_manager.h index 15e7c162032a..6cfcba3c3933
> > > >> 100644
> > > >> --- a/include/libcamera/camera_manager.h
> > > >> +++ b/include/libcamera/camera_manager.h
> > > >> @@ -7,6 +7,7 @@
> > > >> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
> > > >> #define __LIBCAMERA_CAMERA_MANAGER_H__
> > > >>
> > > >> +#include <memory>
> > > >> #include <string>
> > > >> #include <vector>
> > > >>
> > > >> @@ -37,7 +38,7 @@ private:
> > > >> void operator=(const CameraManager &) = delete;
> > > >> ~CameraManager();
> > > >>
> > > >> - DeviceEnumerator *enumerator_;
> > > >> + std::unique_ptr<DeviceEnumerator> enumerator_;
> > > >>
> > > >> std::vector<PipelineHandler *> pipes_;
> > > >
> > > > pipes_ left out as they will disappear in the near future, to be replaced
> > > > with a vector of reference-counted camera objects.
> > > >
> > > >> EventDispatcher *dispatcher_;
> > > >>
> > > >> diff --git a/src/libcamera/camera_manager.cpp
> > > >> b/src/libcamera/camera_manager.cpp index be327f5d5638..432f2b0a11c5
> > > >> 100644
> > > >> --- a/src/libcamera/camera_manager.cpp
> > > >> +++ b/src/libcamera/camera_manager.cpp
> > > >> @@ -61,7 +61,6 @@ CameraManager::~CameraManager()
> > > >> */
> > > >> int CameraManager::start()
> > > >> {
> > > >> -
> > > >> if (enumerator_)
> > > >> return -ENODEV;
> > > >>
> > > >> @@ -84,7 +83,7 @@ int CameraManager::start()
> > > >> * all pipelines it can provide.
> > > >> */
> > > >> do {
> > > >> - pipe = PipelineHandlerFactory::create(handler,
> > > >> enumerator_);
> > > >> + pipe = PipelineHandlerFactory::create(handler,
> > > >> enumerator_.get());
> > > >
> > > > We already break the unique_ptr<> promise :-) If this acceptable according
> > > > to you, or is there a better way ?
> > >
> > > If we're not going to change the internal state of enumerator_, then
> > > we can make PipelineHandlerFactory::create take a const reference
> > > instead of a pointer. In general we use const reference for
> > > method/function arguments that stay unchanged after calling the
> > > method/function, and pointer for output arguments.
> > >
> > > Wdyt?
> >
> > While we don't have to change the state of the enumerator strictly speaking,
> > we need to change the state of the MediaDevice instances it stores (as
> > pointers in a vector). We call the following method for that purpose:
> >
> > MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm) const
> >
> > which I think should not be const as it allows changing the state of an object
> > owned by the enumerator.
>
> I see. Thanks for the detail!
>
> >
> > What we really need to convey through the API is that the called function
> > PipelineHandlerFactory::create() receive a borrowed references to the
> > enumerator and may not access it after it returns. As far as I know there's no
> > simple construct in C++ that is universally understood as expressing this,
> > unlike passing a unique_ptr to express that ownership is transferred. We could
> > possibly standardize on using references for that purpose (const and non-
> > const), but in some cases we still need pointers when passing nullptr is
> > valid, so it wouldn't be a great solution. What's your opinion on this, could
> > we set in stone the rule that a reference received by a function means that
> > the reference is borrowed for the duration of the function only ?
>
> Personally I'm not a big fan of non-const reference. I find it easily
> confused when reviewing the code as it has value syntax with pointer
> semantics. Having raw pointer arguments and/or return values is fine
> and often necessary.
>
> I'd suggest we use std::unique_ptr<> whenever possible to establish
> clear object ownership, and const reference whenever we don't plan to
> alter the state of the object. We then can put most of our attention
> to the remaining raw pointers.
>
> As we're following the Google C++ coding style I'd suggest we follow:
>
> https://google.github.io/styleguide/cppguide.html#Reference_Argument
>
> >
> > > >> if (pipe)
> > > >> pipes_.push_back(pipe);
> > > >> } while (pipe);
> > > >> @@ -114,10 +113,7 @@ void CameraManager::stop()
> > > >>
> > > >> pipes_.clear();
> > > >>
> > > >> - if (enumerator_)
> > > >> - delete enumerator_;
> > > >> -
> > > >> - enumerator_ = nullptr;
> > > >> + enumerator_.reset(nullptr);
> > > >> }
> > > >>
> > > >> /**
> > > >> diff --git a/src/libcamera/device_enumerator.cpp
> > > >> b/src/libcamera/device_enumerator.cpp index 0d18e75525af..2b03b191fa5d
> > > >> 100644
> > > >> --- a/src/libcamera/device_enumerator.cpp
> > > >> +++ b/src/libcamera/device_enumerator.cpp
> > > >> @@ -14,6 +14,7 @@
> > > >> #include "device_enumerator.h"
> > > >> #include "log.h"
> > > >> #include "media_device.h"
> > > >> +#include "utils.h"
> > > >>
> > > >> /**
> > > >> * \file device_enumerator.h
> > > >> @@ -128,20 +129,20 @@ bool DeviceMatch::match(const MediaDevice *device)
> > > >> const
> > > >> * \return A pointer to the newly created device enumerator on success,
> > > >> or
> > > >> * nullptr if an error occurs
> > > >> */
> > > >> -DeviceEnumerator *DeviceEnumerator::create()
> > > >> +std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()
> > > >> {
> > > >> - DeviceEnumerator *enumerator;
> > > >> + std::unique_ptr<DeviceEnumerator> enumerator;
> > > >>
> > > >> /**
> > > >> * \todo Add compile time checks to only try udev enumerator if
> > > >> libudev
> > > >> * is available.
> > > >> */
> > > >> - enumerator = new DeviceEnumeratorUdev();
> > > >> + enumerator = std::make_unique<DeviceEnumeratorUdev>(); /* [1] */
> > > >> + enumerator = std::unique_ptr<DeviceEnumeratorUdev>(new
> > > >> DeviceEnumeratorUdev()); /* [2] */
> > > >> + enumerator.reset(new> DeviceEnumeratorUdev()); /* [3] */
> > > >
> > > > Here are three different ways of implementing the same operation.
> > > > std::make_unique<> doesn't exist in C++11, so I've defined it in utils.c.
> > > > Adding functions to the std namespace could be considered a big of a hack
> > > > though, so two other implementations are proposed. The second option is
> > > > explicit, but a bit too long for my taste, while the third option is short
> > > > but uses reset() which sounds a bit strange for an assignment. Do you
> > > > have any advice ?
> > >
> > > Before we have C++11 in Chromium we also had a base::MakeUnique<>
> > > template in the Chromium libbase handling creation of unique pointers.
> > > If we have to stick with C++11 then we might consider doing the same,
> > > probably with a utils:: namespace. Hacking the std:: namespace is
> > > indeed a bad idea an can cause build errors.
> > >
> > > Semantically I'd say [2] is more accurate than [3], but I don't really
> > > have strong opinions between these two. If we don't want to define our
> > > own make_unique template then we can use either.
> >
> > I'm tempted to add our own make_unique implementation then, most likely in the
> > libcamera:: or libcamera::utils:: namespace though.
>
> Sounds good!
>
> >
> > > Do we restrict ourselves in C++11 for compatibility reason?
> >
> > That was the rationale, but it could be reconsidered if needed.
>
> I suppose C++11 shall be sufficient. We can indeed reconsider if we
> have strong use cases for newer standards one day.
>
> >
> > > >> if (!enumerator->init())
> > > >> return enumerator;
> > > >>
> > > >> - delete enumerator;
> > > >> -
> > > >> /*
> > > >> * Either udev is not available or udev initialization failed.
> > > >> Fall back
> > > >> * on the sysfs enumerator.
> > > >> diff --git a/src/libcamera/include/device_enumerator.h
> > > >> b/src/libcamera/include/device_enumerator.h index
> > > >> 29737da7a225..0afaf88ce1ca 100644
> > > >> --- a/src/libcamera/include/device_enumerator.h
> > > >> +++ b/src/libcamera/include/device_enumerator.h
> > > >> @@ -8,6 +8,7 @@
> > > >> #define __LIBCAMERA_DEVICE_ENUMERATOR_H__
> > > >>
> > > >> #include <map>
> > > >> +#include <memory>
> > > >> #include <string>
> > > >> #include <vector>
> > > >>
> > > >> @@ -34,7 +35,7 @@ private:
> > > >> class DeviceEnumerator
> > > >> {
> > > >> public:
> > > >> - static DeviceEnumerator *create();
> > > >> + static std::unique_ptr<DeviceEnumerator> create();
> > > >> virtual ~DeviceEnumerator();
> > > >>
> > > >> diff --git a/src/libcamera/include/utils.h
> > > >> b/src/libcamera/include/utils.h
> > > >> index 3ffa6f4ea591..6df54e758aa4 100644
> > > >> --- a/src/libcamera/include/utils.h
> > > >> +++ b/src/libcamera/include/utils.h
> > > >> @@ -7,6 +7,19 @@
> > > >>
> > > >> #ifndef __LIBCAMERA_UTILS_H__
> > > >> #define __LIBCAMERA_UTILS_H__
> > > >>
> > > >> +#include <memory>
> > > >> +
> > > >> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> > > >>
> > > >> +namespace std {
> > > >> +
> > > >> +/* C++11 doesn't provide std::make_unique */
> > > >> +template<typename T, typename... Args>
> > > >> +std::unique_ptr<T> make_unique(Args&&... args)
> > > >> +{
> > > >> + return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
> > > >> +}
> > > >> +
> > > >> +} /* namespace std */
> > > >> +
> > > >> #endif /* __LIBCAMERA_UTILS_H__ */
> > > >>
> > > >> std::shared_ptr, on the other hand, shall be used only if absolutely
> > > >> necessary, or it can be a recipe for creating ownership spaghetti.
> > > >>
> > > >>> };
> > > >
> > > > [snip]
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >
More information about the libcamera-devel
mailing list