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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, May 15, 2023 at 01:45:47PM +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 Camera Manager.

s/Camera Manager/CameraManager/

> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  include/libcamera/internal/camera_manager.h | 64 +++++++++++++++++++++
>  include/libcamera/internal/meson.build      |  1 +
>  src/libcamera/camera_manager.cpp            | 50 ++--------------
>  3 files changed, 69 insertions(+), 46 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..05a1e4df8add
> --- /dev/null
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -0,0 +1,64 @@
> +/* 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 <map>

#include <memory>

for std::shared_ptr, std::unique_ptr and std::weak_ptr, and

#include <vector>

for std::vector.

> +

#include <libcamera/base/class.h>

for Extensible::Private.

> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread.h>
> +
> +#include <libcamera/camera_manager.h>

Should this go first to improve the self-contained header test coverage
?

> +
> +#include "libcamera/internal/ipa_manager.h"
> +#include "libcamera/internal/process.h"
> +
> +namespace libcamera {
> +

class Camera;

> +class DeviceEnumerator;
> +
> +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_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index d75088059996..0028ed0dc27f 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -13,6 +13,7 @@ libcamera_internal_headers = files([
>      'bayer_format.h',
>      'byte_stream_buffer.h',
>      'camera.h',
> +    'camera_manager.h',

Alphabetical order.

>      'camera_controls.h',
>      'camera_lens.h',
>      'camera_sensor.h',
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index c1edefdad160..d3c297b888d8 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -5,24 +5,22 @@
>   * camera_manager.h - Camera management
>   */
>  
> -#include <libcamera/camera_manager.h>
> +#include "libcamera/internal/camera_manager.h"
>  
>  #include <map>

You can drop this.

>  
> -#include <libcamera/camera.h>
> -
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/mutex.h>
>  #include <libcamera/base/thread.h>

And mutex.h and thread.h too.

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

I think you need both, see framebuffer.cpp.

>   * \brief The camera manager
>   */
>  
> @@ -33,46 +31,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