[libcamera-devel,v4,2/5] libcamera: camera_manager: Move private implementation to internal

18739 diff mbox series

The CameraManager makes use of the Extensible pattern to provide an
internal private implementation that is not exposed in the public API.

Move the Private declaration to an internal header to make it available
from other internal components in preperation for reducing the surface
area of the public interface of the CameraManager.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Tested-by: Ashok Sidipotu <ashok.sidipotu@collabora.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---

v4
 - Fix includes
 - Fix sort order of header in meson.build
 - Fix doxygen file references

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/camera_manager.h | 70 +++++++++++++++++++++
 include/libcamera/internal/meson.build      |  1 +
 src/libcamera/camera_manager.cpp            | 59 +++--------------
 3 files changed, 80 insertions(+), 50 deletions(-)
 create mode 100644 include/libcamera/internal/camera_manager.h

Hi Kieran,

Thank you for the patch.

On Thu, Jun 15, 2023 at 06:26:05PM +0100, Kieran Bingham via libcamera-devel wrote:
> The CameraManager makes use of the Extensible pattern to provide an
> internal private implementation that is not exposed in the public API.
> 
> Move the Private declaration to an internal header to make it available
> from other internal components in preperation for reducing the surface
> area of the public interface of the CameraManager.
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Tested-by: Ashok Sidipotu <ashok.sidipotu@collabora.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> 
> v4
>  - Fix includes
>  - Fix sort order of header in meson.build
>  - Fix doxygen file references
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_manager.h | 70 +++++++++++++++++++++
>  include/libcamera/internal/meson.build      |  1 +
>  src/libcamera/camera_manager.cpp            | 59 +++--------------
>  3 files changed, 80 insertions(+), 50 deletions(-)
>  create mode 100644 include/libcamera/internal/camera_manager.h
> 
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> new file mode 100644
> index 000000000000..96a83bd7ef3c
> --- /dev/null
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -0,0 +1,70 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Ideas on Board Oy.
> + *
> + * camera_manager.h - Camera manager private data
> + */
> +
> +#pragma once
> +
> +#include <libcamera/camera_manager.h>
> +
> +#include <map>
> +#include <memory>
> +#include <sys/types.h>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread.h>
> +#include <libcamera/base/thread_annotations.h>
> +
> +#include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/ipa_manager.h"
> +#include "libcamera/internal/process.h"
> +
> +namespace libcamera {
> +
> +class Camera;
> +
> +class CameraManager::Private : public Extensible::Private, public Thread
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(CameraManager)
> +
> +public:
> +	Private();
> +
> +	int start();
> +	void addCamera(std::shared_ptr<Camera> camera,
> +		       const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> +	void removeCamera(Camera *camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> +
> +	/*
> +	 * This mutex protects
> +	 *
> +	 * - initialized_ and status_ during initialization
> +	 * - cameras_ and camerasByDevnum_ after initialization
> +	 */
> +	mutable Mutex mutex_;
> +	std::vector<std::shared_ptr<Camera>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> +	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_ LIBCAMERA_TSA_GUARDED_BY(mutex_);

These three variables can be moved to the private section.

> +
> +protected:
> +	void run() override;
> +
> +private:
> +	int init();
> +	void createPipelineHandlers();
> +	void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
> +
> +	ConditionVariable cv_;
> +	bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> +	int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> +
> +	std::unique_ptr<DeviceEnumerator> enumerator_;

I think DeviceEnumerator can be a forward declaration, you don't need to
include device_enumerator.h.

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	IPAManager ipaManager_;
> +	ProcessManager processManager_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index d75088059996..4b2756a4a251 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -15,6 +15,7 @@ libcamera_internal_headers = files([
>      'camera.h',
>      'camera_controls.h',
>      'camera_lens.h',
> +    'camera_manager.h',
>      'camera_sensor.h',
>      'camera_sensor_properties.h',
>      'control_serializer.h',
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index c1edefdad160..882b2d4b234c 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -5,27 +5,26 @@
>   * camera_manager.h - Camera management
>   */
>  
> -#include <libcamera/camera_manager.h>
> -
> -#include <map>
> -
> -#include <libcamera/camera.h>
> +#include "libcamera/internal/camera_manager.h"
>  
>  #include <libcamera/base/log.h>
> -#include <libcamera/base/mutex.h>
> -#include <libcamera/base/thread.h>
>  #include <libcamera/base/utils.h>
>  
> +#include <libcamera/camera.h>
> +
>  #include "libcamera/internal/device_enumerator.h"
> -#include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/pipeline_handler.h"
> -#include "libcamera/internal/process.h"
>  
>  /**
> - * \file camera_manager.h
> + * \file libcamera/camera_manager.h
>   * \brief The camera manager
>   */
>  
> +/**
> + * \file libcamera/internal/camera_manager.h
> + * \brief Internal camera manager support
> + */
> +
>  /**
>   * \brief Top-level libcamera namespace
>   */
> @@ -33,46 +32,6 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(Camera)
>  
> -class CameraManager::Private : public Extensible::Private, public Thread
> -{
> -	LIBCAMERA_DECLARE_PUBLIC(CameraManager)
> -
> -public:
> -	Private();
> -
> -	int start();
> -	void addCamera(std::shared_ptr<Camera> camera,
> -		       const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> -	void removeCamera(Camera *camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> -
> -	/*
> -	 * This mutex protects
> -	 *
> -	 * - initialized_ and status_ during initialization
> -	 * - cameras_ and camerasByDevnum_ after initialization
> -	 */
> -	mutable Mutex mutex_;
> -	std::vector<std::shared_ptr<Camera>> cameras_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> -	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> -
> -protected:
> -	void run() override;
> -
> -private:
> -	int init();
> -	void createPipelineHandlers();
> -	void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
> -
> -	ConditionVariable cv_;
> -	bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> -	int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
> -
> -	std::unique_ptr<DeviceEnumerator> enumerator_;
> -
> -	IPAManager ipaManager_;
> -	ProcessManager processManager_;
> -};
> -
>  CameraManager::Private::Private()
>  	: initialized_(false)
>  {