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

[Ricky Liang]

Hi Laurent,


On Tue, Jan 8, 2019 at 7:10 AM Laurent Pinchart
<laurent.pinchart at 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 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.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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel