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

167 diff mbox series

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@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@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(-)

Hi Laurent,


On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@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.cpp#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_.

std::shared_ptr, on the other hand, shall be used only if absolutely
necessary, or it can be a recipe for creating ownership spaghetti.

-Ricky

>  };
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 50a805fc665c..1a9d2f38e3b9 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -161,4 +161,19 @@ Camera *CameraManager::get(const std::string &name)
>         return nullptr;
>  }
>
> +/**
> + * \brief Retrieve the camera manager instance
> + *
> + * The CameraManager is a singleton and can't be constructed manually. This
> + * function shall instead be used to retrieve the single global instance of the
> + * manager.
> + *
> + * \return The camera manager instance
> + */
> +CameraManager *CameraManager::instance()
> +{
> +       static CameraManager manager;
> +       return &manager;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/test/list.cpp b/test/list.cpp
> index 39b8a41d1fef..e2026c99c5b8 100644
> --- a/test/list.cpp
> +++ b/test/list.cpp
> @@ -19,10 +19,7 @@ class ListTest : public Test
>  protected:
>         int init()
>         {
> -               cm = new CameraManager();
> -               if (!cm)
> -                       return -ENOMEM;
> -
> +               cm = CameraManager::instance();
>                 cm->start();
>
>                 return 0;
> @@ -43,8 +40,6 @@ protected:
>         void cleanup()
>         {
>                 cm->stop();
> -
> -               delete cm;
>         }
>
>  private:
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@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 (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 ?

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

Hi Laurent,

On Sat, Jan 12, 2019 at 12:02 AM Laurent Pinchart
<laurent.pinchart@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@ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo@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
>
>

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@ideasonboard.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>> Reviewed-by: Jacopo Mondi <jacopo@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.

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 ?

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

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

That was the rationale, but it could be reconsidered if needed.

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

Hi Laurent,

On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
<laurent.pinchart@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@ideasonboard.com>
> > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >>> Reviewed-by: Jacopo Mondi <jacopo@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
>
>
>

+ 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@chromium.org> wrote:
>
> Hi Laurent,
>
> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> <laurent.pinchart@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@ideasonboard.com>
> > > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > >>> Reviewed-by: Jacopo Mondi <jacopo@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
> >
> >
> >

Hi Ricky,

On Tuesday, 15 January 2019 05:16:00 EET Ricky Liang wrote:
> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart wrote:
> > 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@ideasonboard.com>
> >>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>>>> Reviewed-by: Jacopo Mondi <jacopo@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(-)

[snip]

> >>>> 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_enume
> >>>> rator.cpp#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).

[snip]

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

[snip]

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

We already do, with one exception that is just an oversight and can easily be 
fixed (I'll submit a patch).

To recap, the idea woulc be to standardize on the following semantics:

- Passing ownership: std::unique_ptr<>
- Passing a reference to a shared object: std::shared_ptr<>
- Passing a borrowed reference when the object doesn't need to be modified: 
const &
- Passing a borrowed reference when the object can be modified: pointer
- Passing a borrowed reference to an output parameter: pointer
- Passing a borrowed reference to an object that can be null: pointer

This implies that the callee can never store a reference to a pointer it 
receives, as all the use cases for pointers pass borrowed references.

Am I missing something ?

> >>>>                       if (pipe)
> >>>>                               pipes_.push_back(pipe);
> >>>>               } while (pipe);

[snip]

Hi Laurent,

On Tue, Jan 15, 2019 at 10:57 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricky,
>
> On Tuesday, 15 January 2019 05:16:00 EET Ricky Liang wrote:
> > On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart wrote:
> > > 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@ideasonboard.com>
> > >>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >>>>> Reviewed-by: Jacopo Mondi <jacopo@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(-)
>
> [snip]
>
> > >>>> 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_enume
> > >>>> rator.cpp#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).
>
> [snip]
>
> > >>>> 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
>
> [snip]
>
> > >>>> @@ -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
>
> We already do, with one exception that is just an oversight and can easily be
> fixed (I'll submit a patch).
>
> To recap, the idea woulc be to standardize on the following semantics:
>
> - Passing ownership: std::unique_ptr<>
> - Passing a reference to a shared object: std::shared_ptr<>
> - Passing a borrowed reference when the object doesn't need to be modified:
> const &
> - Passing a borrowed reference when the object can be modified: pointer
> - Passing a borrowed reference to an output parameter: pointer
> - Passing a borrowed reference to an object that can be null: pointer
>
> This implies that the callee can never store a reference to a pointer it
> receives, as all the use cases for pointers pass borrowed references.
>
> Am I missing something ?

I believe as the code base grows there may be cases where we'll need
to store a reference to a pointer. When that happens we can document
in comments what makes it safe to store and access the pointer.

Otherwise your summary looks good to me.

>
> > >>>>                       if (pipe)
> > >>>>                               pipes_.push_back(pipe);
> > >>>>               } while (pipe);
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Hi all,

On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang <jcliang@chromium.org> wrote:
>
> + 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@chromium.org> wrote:
> >
> > Hi Laurent,
> >
> > On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> > <laurent.pinchart@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@ideasonboard.com>
> > > > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > >>> Reviewed-by: Jacopo Mondi <jacopo@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.

Implementing make_unique or anything already defined in the newer standards need
be done with extra care. Users would expect it to work in the same way, so any
inconsistency with the standards might be a hidden trap. Making things forward
compatible is not an easy task. For example, the implementation below does not
provide the make_unique<T[]>(std::size_t) overload defined in C++14.

I'd suggest not to reinvent the wheel if there is no strong objection. Is it
possible to bump the C++ version to C++14/17? We can still limit the scope of
new language features as we did in [1], so we can opt-in the features gradually.
Another possibility is adopting some existing library such as abseil [2] which
includes make_unique and many other goodies for projects in C++11.

[1] http://www.libcamera.org/coding-style.html#c-specific-rules
[2] https://abseil.io/

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

Sincerely,
Shik

Hi Shik,

On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:
> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang <jcliang@chromium.org> wrote:
> > On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang <jcliang@chromium.org> wrote:
> >> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> >> <laurent.pinchart@ideasonboard.com> wrote:
> >>> 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@ideasonboard.com>
> >>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>>>>>> Reviewed-by: Jacopo Mondi <jacopo@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(-)

[snip]

> >>>>>> 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.
> 
> Implementing make_unique or anything already defined in the newer standards need
> be done with extra care. Users would expect it to work in the same way, so any
> inconsistency with the standards might be a hidden trap. Making things forward
> compatible is not an easy task. For example, the implementation below does not
> provide the make_unique<T[]>(std::size_t) overload defined in C++14.
> 
> I'd suggest not to reinvent the wheel if there is no strong objection. Is it
> possible to bump the C++ version to C++14/17? We can still limit the scope of
> new language features as we did in [1], so we can opt-in the features gradually.

This worries me for two reasons. The first one is that depending on
C++14/C++17 lock us out of platform that don't support those newer
standards. This is more of a general concern, I don't have any data to
tell whether this is a valid concern or not.

The second concern is that we may start using features of C++14/C++17
without noticing, possibly creating problems later if we want to support
C++11-based systems.

> Another possibility is adopting some existing library such as abseil [2] which
> includes make_unique and many other goodies for projects in C++11.
> 
> [1] http://www.libcamera.org/coding-style.html#c-specific-rules
> [2] https://abseil.io/

Thank you for the pointer, that's an interesting library. However, at
this point, I would like to avoid adding an external dependency just to
provide make_unique support. I'm thus tempted to keep our in-house
implementation (which by the way is proposed by
https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and
revisit this topic if/when we will need more feature of newer C++
versions. Does this sound acceptable to you ?

> >>
> >>>
> >>>>>> if (!enumerator->init())
> >>>>>> return enumerator;
> >>>>>>
> >>>>>> -     delete enumerator;
> >>>>>> -
> >>>>>> /*
> >>>>>> * Either udev is not available or udev initialization failed.
> >>>>>> Fall back
> >>>>>> * on the sysfs enumerator.

[snip]

Hi Laurent,

On Fri, Jan 18, 2019 at 8:30 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Shik,
>
> On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:
> > On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang <jcliang@chromium.org> wrote:
> > > On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang <jcliang@chromium.org> wrote:
> > >> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> > >> <laurent.pinchart@ideasonboard.com> wrote:
> > >>> 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@ideasonboard.com>
> > >>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >>>>>>> Reviewed-by: Jacopo Mondi <jacopo@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(-)
>
> [snip]
>
> > >>>>>> 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.
> >
> > Implementing make_unique or anything already defined in the newer standards need
> > be done with extra care. Users would expect it to work in the same way, so any
> > inconsistency with the standards might be a hidden trap. Making things forward
> > compatible is not an easy task. For example, the implementation below does not
> > provide the make_unique<T[]>(std::size_t) overload defined in C++14.
> >
> > I'd suggest not to reinvent the wheel if there is no strong objection. Is it
> > possible to bump the C++ version to C++14/17? We can still limit the scope of
> > new language features as we did in [1], so we can opt-in the features gradually.
>
> This worries me for two reasons. The first one is that depending on
> C++14/C++17 lock us out of platform that don't support those newer
> standards. This is more of a general concern, I don't have any data to
> tell whether this is a valid concern or not.

Yes it's hard to make decision without having the concrete target platforms. The
compiler support for C++14 are complete and stable enough in GCC/Clang, and
it's the default C++ standard version since GCC 5 and Clang 6. But some
popularish Linux distros might ship with older compilers that does not support
C++14 well, such as Ubuntu 14.04 and RHEL 6 although they are approaching EOL.

>
> The second concern is that we may start using features of C++14/C++17
> without noticing, possibly creating problems later if we want to support
> C++11-based systems.

This is actually a good point. Note that the compiler flag "-std=c++XX" might
not strictly align with the standard. We are already using at least one C++17
feature "nested namespace" in utils.h, which uses "namespace libcamera::utils"
instead of "namespace libcamera { namespace utils". I found this because it fail
to compile on one of my machine (Ubuntu 17.10).

>
> > Another possibility is adopting some existing library such as abseil [2] which
> > includes make_unique and many other goodies for projects in C++11.
> >
> > [1] http://www.libcamera.org/coding-style.html#c-specific-rules
> > [2] https://abseil.io/
>
> Thank you for the pointer, that's an interesting library. However, at
> this point, I would like to avoid adding an external dependency just to
> provide make_unique support. I'm thus tempted to keep our in-house
> implementation (which by the way is proposed by
> https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and
> revisit this topic if/when we will need more feature of newer C++
> versions. Does this sound acceptable to you ?

It's reasonable that not to add an external dependency just for make_unique. We
could revisit this topic as we need it.

>
> > >>
> > >>>
> > >>>>>> if (!enumerator->init())
> > >>>>>> return enumerator;
> > >>>>>>
> > >>>>>> -     delete enumerator;
> > >>>>>> -
> > >>>>>> /*
> > >>>>>> * Either udev is not available or udev initialization failed.
> > >>>>>> Fall back
> > >>>>>> * on the sysfs enumerator.
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart

Sincerely,
Shik

Hi Laurent,

On 18/01/2019 00:30, Laurent Pinchart wrote:
> Hi Shik,
> 
> On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:
>> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang <jcliang@chromium.org> wrote:
>>> On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang <jcliang@chromium.org> wrote:
>>>> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
>>>> <laurent.pinchart@ideasonboard.com> wrote:
>>>>> 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@ideasonboard.com>
>>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>>>>>>>> Reviewed-by: Jacopo Mondi <jacopo@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(-)
> 
> [snip]
> 
>>>>>>>> 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.
>>
>> Implementing make_unique or anything already defined in the newer standards need
>> be done with extra care. Users would expect it to work in the same way, so any
>> inconsistency with the standards might be a hidden trap. Making things forward
>> compatible is not an easy task. For example, the implementation below does not
>> provide the make_unique<T[]>(std::size_t) overload defined in C++14.
>>
>> I'd suggest not to reinvent the wheel if there is no strong objection. Is it
>> possible to bump the C++ version to C++14/17? We can still limit the scope of
>> new language features as we did in [1], so we can opt-in the features gradually.
> 
> This worries me for two reasons. The first one is that depending on
> C++14/C++17 lock us out of platform that don't support those newer
> standards. This is more of a general concern, I don't have any data to
> tell whether this is a valid concern or not.
> 
> The second concern is that we may start using features of C++14/C++17
> without noticing, possibly creating problems later if we want to support
> C++11-based systems.
> 
>> Another possibility is adopting some existing library such as abseil [2] which
>> includes make_unique and many other goodies for projects in C++11.
>>
>> [1] http://www.libcamera.org/coding-style.html#c-specific-rules
>> [2] https://abseil.io/
> 
> Thank you for the pointer, that's an interesting library. However, at
> this point, I would like to avoid adding an external dependency just to
> provide make_unique support. I'm thus tempted to keep our in-house
> implementation (which by the way is proposed by
> https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and
> revisit this topic if/when we will need more feature of newer C++
> versions. Does this sound acceptable to you ?

If we're going to have our 'own' implementation of make_unique - could
you add a test to test/meson.build::internal_tests please?




>>>>>>>> if (!enumerator->init())
>>>>>>>> return enumerator;
>>>>>>>>
>>>>>>>> -     delete enumerator;
>>>>>>>> -
>>>>>>>> /*
>>>>>>>> * Either udev is not available or udev initialization failed.
>>>>>>>> Fall back
>>>>>>>> * on the sysfs enumerator.
> 
> [snip]
>

Hi Kieran,

On Fri, Jan 18, 2019 at 11:12:56AM +0000, Kieran Bingham wrote:
> On 18/01/2019 00:30, Laurent Pinchart wrote:
> > On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:
> >> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang <jcliang@chromium.org> wrote:
> >>> On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang <jcliang@chromium.org> wrote:
> >>>> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> >>>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>>> 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@ideasonboard.com>
> >>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>>>>>>>> Reviewed-by: Jacopo Mondi <jacopo@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(-)
> > 
> > [snip]
> > 
> >>>>>>>> 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.
> >>
> >> Implementing make_unique or anything already defined in the newer standards need
> >> be done with extra care. Users would expect it to work in the same way, so any
> >> inconsistency with the standards might be a hidden trap. Making things forward
> >> compatible is not an easy task. For example, the implementation below does not
> >> provide the make_unique<T[]>(std::size_t) overload defined in C++14.
> >>
> >> I'd suggest not to reinvent the wheel if there is no strong objection. Is it
> >> possible to bump the C++ version to C++14/17? We can still limit the scope of
> >> new language features as we did in [1], so we can opt-in the features gradually.
> > 
> > This worries me for two reasons. The first one is that depending on
> > C++14/C++17 lock us out of platform that don't support those newer
> > standards. This is more of a general concern, I don't have any data to
> > tell whether this is a valid concern or not.
> > 
> > The second concern is that we may start using features of C++14/C++17
> > without noticing, possibly creating problems later if we want to support
> > C++11-based systems.
> > 
> >> Another possibility is adopting some existing library such as abseil [2] which
> >> includes make_unique and many other goodies for projects in C++11.
> >>
> >> [1] http://www.libcamera.org/coding-style.html#c-specific-rules
> >> [2] https://abseil.io/
> > 
> > Thank you for the pointer, that's an interesting library. However, at
> > this point, I would like to avoid adding an external dependency just to
> > provide make_unique support. I'm thus tempted to keep our in-house
> > implementation (which by the way is proposed by
> > https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and
> > revisit this topic if/when we will need more feature of newer C++
> > versions. Does this sound acceptable to you ?
> 
> If we're going to have our 'own' implementation of make_unique - could
> you add a test to test/meson.build::internal_tests please?

That's a good point. I'm however not sure how to test it meaningfully
other than making sure it compiles, which is already handled by the
libcamera code base. Feel free to propose a test :-)

> >>>>>>>> if (!enumerator->init())
> >>>>>>>> return enumerator;
> >>>>>>>>
> >>>>>>>> -     delete enumerator;
> >>>>>>>> -
> >>>>>>>> /*
> >>>>>>>> * Either udev is not available or udev initialization failed.
> >>>>>>>> Fall back
> >>>>>>>> * on the sysfs enumerator.
> > 
> > [snip]
> >

Hi Shik,

On Fri, Jan 18, 2019 at 03:42:28PM +0800, Shik Chen wrote:
> On Fri, Jan 18, 2019 at 8:30 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Wed, Jan 16, 2019 at 09:46:30PM +0800, Shik Chen wrote:
> >> On Tue, Jan 15, 2019 at 10:26 PM Ricky Liang <jcliang@chromium.org> wrote:
> >>> On Tue, Jan 15, 2019 at 11:16 AM Ricky Liang <jcliang@chromium.org> wrote:
> >>>> On Tue, Jan 15, 2019 at 10:19 AM Laurent Pinchart
> >>>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>>> 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@ideasonboard.com>
> >>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>>>>>>>> Reviewed-by: Jacopo Mondi <jacopo@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(-)
> > 
> > [snip]
> > 
> >>>>>>>> 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.
> >> 
> >> Implementing make_unique or anything already defined in the newer standards need
> >> be done with extra care. Users would expect it to work in the same way, so any
> >> inconsistency with the standards might be a hidden trap. Making things forward
> >> compatible is not an easy task. For example, the implementation below does not
> >> provide the make_unique<T[]>(std::size_t) overload defined in C++14.
> >> 
> >> I'd suggest not to reinvent the wheel if there is no strong objection. Is it
> >> possible to bump the C++ version to C++14/17? We can still limit the scope of
> >> new language features as we did in [1], so we can opt-in the features gradually.
> > 
> > This worries me for two reasons. The first one is that depending on
> > C++14/C++17 lock us out of platform that don't support those newer
> > standards. This is more of a general concern, I don't have any data to
> > tell whether this is a valid concern or not.
>  
>  Yes it's hard to make decision without having the concrete target platforms. The
>  compiler support for C++14 are complete and stable enough in GCC/Clang, and
>  it's the default C++ standard version since GCC 5 and Clang 6. But some
>  popularish Linux distros might ship with older compilers that does not support
>  C++14 well, such as Ubuntu 14.04 and RHEL 6 although they are approaching EOL.

I expect we'll be able to upgrade to C++14, but let's delay that until
necessary.

> > The second concern is that we may start using features of C++14/C++17
> > without noticing, possibly creating problems later if we want to support
> > C++11-based systems.
>  
>  This is actually a good point. Note that the compiler flag "-std=c++XX" might
>  not strictly align with the standard. We are already using at least one C++17
>  feature "nested namespace" in utils.h, which uses "namespace libcamera::utils"
>  instead of "namespace libcamera { namespace utils". I found this because it fail
>  to compile on one of my machine (Ubuntu 17.10).

Thank you for reporting this. I'll fix it.

> >> Another possibility is adopting some existing library such as abseil [2] which
> >> includes make_unique and many other goodies for projects in C++11.
> >> 
> >> [1] http://www.libcamera.org/coding-style.html#c-specific-rules
> >> [2] https://abseil.io/
> > 
> > Thank you for the pointer, that's an interesting library. However, at
> > this point, I would like to avoid adding an external dependency just to
> > provide make_unique support. I'm thus tempted to keep our in-house
> > implementation (which by the way is proposed by
> > https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique), and
> > revisit this topic if/when we will need more feature of newer C++
> > versions. Does this sound acceptable to you ?
>  
>  It's reasonable that not to add an external dependency just for make_unique. We
>  could revisit this topic as we need it.
>  
> >>>>>>>> if (!enumerator->init())
> >>>>>>>> return enumerator;
> >>>>>>>> 
> >>>>>>>> -     delete enumerator;
> >>>>>>>> -
> >>>>>>>> /*
> >>>>>>>> * Either udev is not available or udev initialization failed.
> >>>>>>>> Fall back
> >>>>>>>> * on the sysfs enumerator.
> > 
> > [snip]