[libcamera-devel] [PATCH v2 1/4] libcamera: camera_manager: Move private implementation to internal

[Jacopo Mondi]

Hi Kieran

On Thu, May 11, 2023 at 11:58:30PM +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.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/camera_manager.h | 66 +++++++++++++++++++++
>  include/libcamera/internal/meson.build      |  1 +
>  src/libcamera/camera_manager.cpp            | 49 ++-------------
>  3 files changed, 71 insertions(+), 45 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..4bba3ac2eb29
> --- /dev/null
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Ideas on Board Oy.
> + *
> + * camera_manager.h - Camera manager private data
> + */
> +#ifndef __LIBCAMERA_INTERNAL_CAMERA_MANAGER_H__
> +#define __LIBCAMERA_INTERNAL_CAMERA_MANAGER_H__

#pragma once ?

> +
> +#include <map>
> +
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread.h>
> +
> +#include <libcamera/camera_manager.h>
> +
> +#include "libcamera/internal/ipa_manager.h"
> +#include "libcamera/internal/process.h"
> +
> +namespace libcamera {
> +
> +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 */
> +
> +#endif // __LIBCAMERA_INTERNAL_CAMERA_MANAGER_H__
> 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',
>      '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..b95284ba5a80 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -9,20 +9,19 @@
>
>  #include <map>
>
> -#include <libcamera/camera.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/camera_manager.h"

I would include this one first to make sure it is self-contained

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

is this needed ?

All minors:
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j


>   * \brief The camera manager
>   */
>
> @@ -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)
>  {
> --
> 2.34.1
>