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

[Laurent Pinchart]

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 at ideasonboard.com>
> Tested-by: Ashok Sidipotu <ashok.sidipotu at collabora.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> 
> v4
>  - Fix includes
>  - Fix sort order of header in meson.build
>  - Fix doxygen file references
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at 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 at 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)
>  {

-- 
Regards,

Laurent Pinchart