[libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager: Make the class a singleton
Ricky Liang
jcliang at chromium.org
Tue Jan 15 04:16:00 CET 2019
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