[libcamera-devel,v2,1/7] IPAManager: make IPAManager lifetime explicitly managed
diff mbox series

Message ID 20200605090106.15424-2-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • Support qv4l2 with v4l2-compat
Related show

Commit Message

Paul Elder June 5, 2020, 9:01 a.m. UTC 
If any ipa_context instances are destroyed after the IPAManager is
destroyed, then a segfault will occur, since the modules have been
unloaded by the IPAManager and the context function pointers have been
freed.

Fix this by making the lifetime of the IPAManager explicit, and make the
CameraManager construct and deconstruct (automatically, via a unique
pointer) the IPAManager.

Also update the IPA interface test to do the construction and
deconstruction of the IPAManager, as it does not use the CameraManager.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2: Move the IPAManager instance into
	       CameraManager::Private from CameraManager
---
 include/libcamera/camera_manager.h       |  1 -
 include/libcamera/internal/ipa_manager.h |  6 ++++--
 src/libcamera/camera_manager.cpp         |  3 +++
 src/libcamera/ipa_manager.cpp            | 20 ++++++++++++++++++--
 test/ipa/ipa_interface_test.cpp          |  5 +++++
 5 files changed, 30 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart June 5, 2020, 10:19 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Fri, Jun 05, 2020 at 06:01:00PM +0900, Paul Elder wrote:
> If any ipa_context instances are destroyed after the IPAManager is
> destroyed, then a segfault will occur, since the modules have been
> unloaded by the IPAManager and the context function pointers have been
> freed.
> 
> Fix this by making the lifetime of the IPAManager explicit, and make the
> CameraManager construct and deconstruct (automatically, via a unique
> pointer) the IPAManager.
> 
> Also update the IPA interface test to do the construction and
> deconstruction of the IPAManager, as it does not use the CameraManager.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2: Move the IPAManager instance into
> 	       CameraManager::Private from CameraManager
> ---
>  include/libcamera/camera_manager.h       |  1 -
>  include/libcamera/internal/ipa_manager.h |  6 ++++--
>  src/libcamera/camera_manager.cpp         |  3 +++
>  src/libcamera/ipa_manager.cpp            | 20 ++++++++++++++++++--
>  test/ipa/ipa_interface_test.cpp          |  5 +++++
>  5 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848a..00658a70 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -18,7 +18,6 @@ namespace libcamera {
>  
>  class Camera;
>  class EventDispatcher;
> -
>  class CameraManager : public Object
>  {
>  public:
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 16d74291..43f700d3 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -22,6 +22,9 @@ namespace libcamera {
>  class IPAManager
>  {
>  public:
> +	IPAManager();
> +	~IPAManager();
> +
>  	static IPAManager *instance();
>  
>  	std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
> @@ -29,8 +32,7 @@ public:
>  					    uint32_t minVersion);
>  
>  private:
> -	IPAManager();
> -	~IPAManager();
> +	static IPAManager *self_;
>  
>  	void parseDir(const char *libDir, unsigned int maxDepth,
>  		      std::vector<std::string> &files);
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 849377ad..da806fa7 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -15,6 +15,7 @@
>  
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/event_dispatcher_poll.h"
> +#include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/thread.h"
> @@ -63,6 +64,8 @@ private:
>  
>  	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
> +
> +	IPAManager ipaManager_;
>  };
>  
>  CameraManager::Private::Private(CameraManager *cm)
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 505cf610..abb681a3 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -93,8 +93,21 @@ LOG_DEFINE_CATEGORY(IPAManager)
>   * plain C API, or to transmit the data to the isolated process through IPC.
>   */
>  
> +IPAManager *IPAManager::self_ = nullptr;
> +
> +/**
> + * \brief Construct an IPAManager instance
> + *
> + * The IPAManager class is meant to only be instantiated once, by the
> + * CameraManager. Pipeline handlers shall use the instance() function to access
> + * the IPAManager instance.
> + */
>  IPAManager::IPAManager()
>  {
> +	if (self_)
> +		LOG(IPAManager, Fatal)
> +			<< "Multiple IPAManager objects are not allowed";
> +
>  	unsigned int ipaCount = 0;
>  
>  	/* User-specified paths take precedence. */
> @@ -134,12 +147,16 @@ IPAManager::IPAManager()
>  	if (!ipaCount)
>  		LOG(IPAManager, Warning)
>  			<< "No IPA found in '" IPA_MODULE_DIR "'";
> +
> +	self_ = this;
>  }
>  
>  IPAManager::~IPAManager()
>  {
>  	for (IPAModule *module : modules_)
>  		delete module;
> +
> +	self_ = nullptr;
>  }
>  
>  /**
> @@ -153,8 +170,7 @@ IPAManager::~IPAManager()
>   */
>  IPAManager *IPAManager::instance()

I think the documentation needs to be updated too. With this handled,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  {
> -	static IPAManager ipaManager;
> -	return &ipaManager;
> +	return self_;
>  }
>  
>  /**
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 2f02af49..153493ba 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -39,11 +39,15 @@ public:
>  	~IPAInterfaceTest()
>  	{
>  		delete notifier_;
> +		ipa_.reset();
> +		ipaManager_.reset();
>  	}
>  
>  protected:
>  	int init() override
>  	{
> +		ipaManager_ = make_unique<IPAManager>();
> +
>  		/* Create a pipeline handler for vimc. */
>  		std::vector<PipelineHandlerFactory *> &factories =
>  			PipelineHandlerFactory::factories();
> @@ -161,6 +165,7 @@ private:
>  
>  	std::shared_ptr<PipelineHandler> pipe_;
>  	std::unique_ptr<IPAProxy> ipa_;
> +	std::unique_ptr<IPAManager> ipaManager_;
>  	enum IPAOperationCode trace_;
>  	EventNotifier *notifier_;
>  	int fd_;
Niklas Söderlund June 5, 2020, 6:08 p.m. UTC | #2 
Hi Paul,

Thanks for your work.

On 2020-06-05 18:01:00 +0900, Paul Elder wrote:
> If any ipa_context instances are destroyed after the IPAManager is
> destroyed, then a segfault will occur, since the modules have been
> unloaded by the IPAManager and the context function pointers have been
> freed.
> 
> Fix this by making the lifetime of the IPAManager explicit, and make the
> CameraManager construct and deconstruct (automatically, via a unique
> pointer) the IPAManager.
> 
> Also update the IPA interface test to do the construction and
> deconstruction of the IPAManager, as it does not use the CameraManager.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> ---
> Changes in v2: Move the IPAManager instance into
> 	       CameraManager::Private from CameraManager
> ---
>  include/libcamera/camera_manager.h       |  1 -
>  include/libcamera/internal/ipa_manager.h |  6 ++++--
>  src/libcamera/camera_manager.cpp         |  3 +++
>  src/libcamera/ipa_manager.cpp            | 20 ++++++++++++++++++--
>  test/ipa/ipa_interface_test.cpp          |  5 +++++
>  5 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848a..00658a70 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -18,7 +18,6 @@ namespace libcamera {
>  
>  class Camera;
>  class EventDispatcher;
> -
>  class CameraManager : public Object
>  {
>  public:
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 16d74291..43f700d3 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -22,6 +22,9 @@ namespace libcamera {
>  class IPAManager
>  {
>  public:
> +	IPAManager();
> +	~IPAManager();
> +
>  	static IPAManager *instance();
>  
>  	std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
> @@ -29,8 +32,7 @@ public:
>  					    uint32_t minVersion);
>  
>  private:
> -	IPAManager();
> -	~IPAManager();
> +	static IPAManager *self_;
>  
>  	void parseDir(const char *libDir, unsigned int maxDepth,
>  		      std::vector<std::string> &files);
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 849377ad..da806fa7 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -15,6 +15,7 @@
>  
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/event_dispatcher_poll.h"
> +#include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/thread.h"
> @@ -63,6 +64,8 @@ private:
>  
>  	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
> +
> +	IPAManager ipaManager_;
>  };
>  
>  CameraManager::Private::Private(CameraManager *cm)
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 505cf610..abb681a3 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -93,8 +93,21 @@ LOG_DEFINE_CATEGORY(IPAManager)
>   * plain C API, or to transmit the data to the isolated process through IPC.
>   */
>  
> +IPAManager *IPAManager::self_ = nullptr;
> +
> +/**
> + * \brief Construct an IPAManager instance
> + *
> + * The IPAManager class is meant to only be instantiated once, by the
> + * CameraManager. Pipeline handlers shall use the instance() function to access
> + * the IPAManager instance.
> + */
>  IPAManager::IPAManager()
>  {
> +	if (self_)
> +		LOG(IPAManager, Fatal)
> +			<< "Multiple IPAManager objects are not allowed";
> +
>  	unsigned int ipaCount = 0;
>  
>  	/* User-specified paths take precedence. */
> @@ -134,12 +147,16 @@ IPAManager::IPAManager()
>  	if (!ipaCount)
>  		LOG(IPAManager, Warning)
>  			<< "No IPA found in '" IPA_MODULE_DIR "'";
> +
> +	self_ = this;
>  }
>  
>  IPAManager::~IPAManager()
>  {
>  	for (IPAModule *module : modules_)
>  		delete module;
> +
> +	self_ = nullptr;
>  }
>  
>  /**
> @@ -153,8 +170,7 @@ IPAManager::~IPAManager()
>   */
>  IPAManager *IPAManager::instance()
>  {
> -	static IPAManager ipaManager;
> -	return &ipaManager;
> +	return self_;
>  }
>  
>  /**
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 2f02af49..153493ba 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -39,11 +39,15 @@ public:
>  	~IPAInterfaceTest()
>  	{
>  		delete notifier_;
> +		ipa_.reset();
> +		ipaManager_.reset();
>  	}
>  
>  protected:
>  	int init() override
>  	{
> +		ipaManager_ = make_unique<IPAManager>();
> +
>  		/* Create a pipeline handler for vimc. */
>  		std::vector<PipelineHandlerFactory *> &factories =
>  			PipelineHandlerFactory::factories();
> @@ -161,6 +165,7 @@ private:
>  
>  	std::shared_ptr<PipelineHandler> pipe_;
>  	std::unique_ptr<IPAProxy> ipa_;
> +	std::unique_ptr<IPAManager> ipaManager_;
>  	enum IPAOperationCode trace_;
>  	EventNotifier *notifier_;
>  	int fd_;
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
index 079f848a..00658a70 100644
--- a/include/libcamera/camera_manager.h
+++ b/include/libcamera/camera_manager.h
@@ -18,7 +18,6 @@  namespace libcamera {
 
 class Camera;
 class EventDispatcher;
-
 class CameraManager : public Object
 {
 public:
diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index 16d74291..43f700d3 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -22,6 +22,9 @@  namespace libcamera {
 class IPAManager
 {
 public:
+	IPAManager();
+	~IPAManager();
+
 	static IPAManager *instance();
 
 	std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
@@ -29,8 +32,7 @@  public:
 					    uint32_t minVersion);
 
 private:
-	IPAManager();
-	~IPAManager();
+	static IPAManager *self_;
 
 	void parseDir(const char *libDir, unsigned int maxDepth,
 		      std::vector<std::string> &files);
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 849377ad..da806fa7 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -15,6 +15,7 @@ 
 
 #include "libcamera/internal/device_enumerator.h"
 #include "libcamera/internal/event_dispatcher_poll.h"
+#include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/log.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/thread.h"
@@ -63,6 +64,8 @@  private:
 
 	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
 	std::unique_ptr<DeviceEnumerator> enumerator_;
+
+	IPAManager ipaManager_;
 };
 
 CameraManager::Private::Private(CameraManager *cm)
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index 505cf610..abb681a3 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -93,8 +93,21 @@  LOG_DEFINE_CATEGORY(IPAManager)
  * plain C API, or to transmit the data to the isolated process through IPC.
  */
 
+IPAManager *IPAManager::self_ = nullptr;
+
+/**
+ * \brief Construct an IPAManager instance
+ *
+ * The IPAManager class is meant to only be instantiated once, by the
+ * CameraManager. Pipeline handlers shall use the instance() function to access
+ * the IPAManager instance.
+ */
 IPAManager::IPAManager()
 {
+	if (self_)
+		LOG(IPAManager, Fatal)
+			<< "Multiple IPAManager objects are not allowed";
+
 	unsigned int ipaCount = 0;
 
 	/* User-specified paths take precedence. */
@@ -134,12 +147,16 @@  IPAManager::IPAManager()
 	if (!ipaCount)
 		LOG(IPAManager, Warning)
 			<< "No IPA found in '" IPA_MODULE_DIR "'";
+
+	self_ = this;
 }
 
 IPAManager::~IPAManager()
 {
 	for (IPAModule *module : modules_)
 		delete module;
+
+	self_ = nullptr;
 }
 
 /**
@@ -153,8 +170,7 @@  IPAManager::~IPAManager()
  */
 IPAManager *IPAManager::instance()
 {
-	static IPAManager ipaManager;
-	return &ipaManager;
+	return self_;
 }
 
 /**
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
index 2f02af49..153493ba 100644
--- a/test/ipa/ipa_interface_test.cpp
+++ b/test/ipa/ipa_interface_test.cpp
@@ -39,11 +39,15 @@  public:
 	~IPAInterfaceTest()
 	{
 		delete notifier_;
+		ipa_.reset();
+		ipaManager_.reset();
 	}
 
 protected:
 	int init() override
 	{
+		ipaManager_ = make_unique<IPAManager>();
+
 		/* Create a pipeline handler for vimc. */
 		std::vector<PipelineHandlerFactory *> &factories =
 			PipelineHandlerFactory::factories();
@@ -161,6 +165,7 @@  private:
 
 	std::shared_ptr<PipelineHandler> pipe_;
 	std::unique_ptr<IPAProxy> ipa_;
+	std::unique_ptr<IPAManager> ipaManager_;
 	enum IPAOperationCode trace_;
 	EventNotifier *notifier_;
 	int fd_;