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

18631 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 Camera Manager.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@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

Tested-by: Ashok Sidipotu <ashok.sidipotu@collabora.com>
On Monday, May 15, 2023 18:15 IST, Kieran Bingham <kieran.bingham@ideasonboard.com> 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@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@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 <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 */
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..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>

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

@@ -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)
{
--
2.34.1

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@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@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)
>  {

Quoting Laurent Pinchart (2023-06-06 17:05:35)
> 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@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@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.

Also #include <sys/types.h> for dev_t;

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

clang-format really doesn't like that ;-) I don't think 'headers'
including a component header have any concept in the include handling
there.

But as the core camera_manager.cpp now includes this file first, indeed
I think it should come first to complete the chain.

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

Ah, yes that's what I was missing.

> 
> >   * \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