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

[Laurent Pinchart]

Hi Ricky,

On Tuesday, 8 January 2019 16:48:12 EET Ricky Liang wrote:
> On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart wrote:
> > There can only be a single camera manager instance in the application.
> > Creating it as a singleton helps avoiding mistakes. It also allows the
> > camera manager to be used as a storage of global data, such as the
> > future event dispatcher.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > Changes since v1:
> > 
> > - Delete copy constructor and assignment operator
> > - Fix documentation style issue
> > ---
> > 
> >  include/libcamera/camera_manager.h |  8 ++++++--
> >  src/libcamera/camera_manager.cpp   | 15 +++++++++++++++
> >  test/list.cpp                      |  7 +------
> >  3 files changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/libcamera/camera_manager.h
> > b/include/libcamera/camera_manager.h index 2768a5bd2384..e14da0f893b3
> > 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -19,15 +19,19 @@ class PipelineHandler;
> >  class CameraManager
> >  {
> >  public:
> > -       CameraManager();
> > -
> >         int start();
> >         void stop();
> >         
> >         std::vector<std::string> list() const;
> >         Camera *get(const std::string &name);
> > 
> > +       static CameraManager *instance();
> > +
> >  private:
> > +       CameraManager();
> > +       CameraManager(const CameraManager &) = delete;
> > +       void operator=(const CameraManager &) = delete;
> > +
> >         DeviceEnumerator *enumerator_;
> >         std::vector<PipelineHandler *> pipes_;
> 
> Not directly related to this patch, but have we considered making
> these pointers std::unique_ptr? From our experience working in
> Chromium we find it informative, easier to follow the code, and less
> error-prone if an object's ownership can be clearly identified through
> an std::unique_ptr.
> 
> For example, in the current libcamera code base I noticed there's a
> potential leak in DeviceEnumerator::create() if enumerator->init()
> fails:
> 
> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/device_enumerator.c
> pp#n137
> 
> If we instead only create and pass std::unique_ptr<DeviceEnumerator>
> around then we'd avoid leak like this, and people can also look at the
> code and clearly understands that CameraManager has the ownership of
> enumerator_.

I agree with you, std::unique_ptr<> would here both provide the advantages of
a scoped pointer with automatic deletion, and make it clear who owns the
object. I gave it a try for enumerator_, and here is what I ended up with
(quote characters added to comment inline).

> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 15e7c162032a..6cfcba3c3933 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
>  #define __LIBCAMERA_CAMERA_MANAGER_H__
>  
> +#include <memory>
>  #include <string>
>  #include <vector>
>  
> @@ -37,7 +38,7 @@ private:
>  	void operator=(const CameraManager &) = delete;
>  	~CameraManager();
>  
> -	DeviceEnumerator *enumerator_;
> +	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::vector<PipelineHandler *> pipes_;

pipes_ left out as they will disappear in the near future, to be replaced with
a vector of reference-counted camera objects.

>  
>  	EventDispatcher *dispatcher_;
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index be327f5d5638..432f2b0a11c5 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -61,7 +61,6 @@ CameraManager::~CameraManager()
>   */
>  int CameraManager::start()
>  {
> -
>  	if (enumerator_)
>  		return -ENODEV;
>  
> @@ -84,7 +83,7 @@ int CameraManager::start()
>  		 * all pipelines it can provide.
>  		 */
>  		do {
> -			pipe = PipelineHandlerFactory::create(handler, enumerator_);
> +			pipe = PipelineHandlerFactory::create(handler, enumerator_.get());

We already break the unique_ptr<> promise :-) If this acceptable according to
you, or is there a better way ?

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

-- 
Regards,

Laurent Pinchart