[libcamera-devel] [PATCH v2 04/11] libcamera: camera_manager: Make the class a singleton

[Ricky Liang]

Hi Laurent,

On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Ricky,
>
> 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_enumerator.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?

>
> >                       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.

Do we restrict ourselves in C++11 for compatibility reason?

>
> >       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
>
>