[libcamera-devel,v2,1/4] libcamera: camera_manager: Move private implementation to internal
diff mbox series

Message ID 20230511225833.361699-2-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Add new Camera devices property
Related show

Commit Message

Kieran Bingham May 11, 2023, 10:58 p.m. UTC 
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>
---
 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

Comments

Jacopo Mondi May 13, 2023, 2:45 p.m. UTC | #1 
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@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@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
>
Kieran Bingham May 13, 2023, 3:35 p.m. UTC | #2 
Quoting Jacopo Mondi (2023-05-13 15:45:07)
> 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@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 ?

 Yes :-) Not sure how I missed that.

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

Hrm ... the public one already is. I guess it makes sense to have both
up at the top?


> 
> >  #include "libcamera/internal/device_enumerator.h"
> > -#include "libcamera/internal/ipa_manager.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> > -#include "libcamera/internal/pruocess.h"
> >
> >  /**
> > - * \file camera_manager.h
> > + * \file libcamera/camera_manager.h
> 
> is this needed ?

Yes, otherwise doxygen complains.

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

Thanks


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

Patch
diff mbox series 

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__
+
+#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"
 #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 +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)
 {