[libcamera-devel,1/5] IPAManager: make IPAManager lifetime explicitly managed

3905 diff mbox series

If any ipa_context instances are destroyed before the IPAManager is
destroyed, then a segfault will occur when the IPAManager is
deconstructed, as it tries to destroy all of the IPAs that it manages.
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>
---
 include/libcamera/camera_manager.h       |  4 ++++
 include/libcamera/internal/ipa_manager.h |  7 ++++---
 src/libcamera/camera_manager.cpp         |  4 +++-
 src/libcamera/ipa_manager.cpp            | 13 +++++++++++--
 test/ipa/ipa_interface_test.cpp          |  5 +++++
 5 files changed, 27 insertions(+), 6 deletions(-)

Hi Paul,

Thank you for the patch.

On Wed, Jun 03, 2020 at 11:16:05PM +0900, Paul Elder wrote:
> If any ipa_context instances are destroyed before the IPAManager is
> destroyed, then a segfault will occur when the IPAManager is
> deconstructed, as it tries to destroy all of the IPAs that it manages.

Isn't it the other way around, destroying ipa_context after the
IPAManager, as the modules are unloaded by the IPAManager and the
context function pointers thus become invalid ?

> 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>
> ---
>  include/libcamera/camera_manager.h       |  4 ++++
>  include/libcamera/internal/ipa_manager.h |  7 ++++---
>  src/libcamera/camera_manager.cpp         |  4 +++-
>  src/libcamera/ipa_manager.cpp            | 13 +++++++++++--
>  test/ipa/ipa_interface_test.cpp          |  5 +++++
>  5 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848a..8f9398fa 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -14,6 +14,8 @@
>  
>  #include <libcamera/object.h>
>  
> +#include "libcamera/internal/ipa_manager.h"
> +

Including an internal header in a public header will cause issues once
libcamera is installed, as the internal headers are not installed. As
you include ipa_manager.h to get the IPAManager class declaration only,
you could just forward-declare it.

>  namespace libcamera {
>  
>  class Camera;
> @@ -48,6 +50,8 @@ private:
>  
>  	class Private;
>  	std::unique_ptr<Private> p_;
> +
> +	std::unique_ptr<IPAManager> ipaManager_;

But let's move this to the CameraManager::Private class, to avoid
exposing the IPAManager in a public header, and to help with ABI
compatibility in the future (any non-static member data of a class is
part of the ABI, the less we expose, the less we risk breaking).

>  };
>  
>  } /* namespace libcamera */
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 2412d757..65303d6d 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,
> @@ -30,9 +33,7 @@ public:
>  
>  private:
>  	std::vector<IPAModule *> modules_;
> -
> -	IPAManager();
> -	~IPAManager();

Ideally the constructor should be private, with a friend class
CameraManager::Private, but that will break the IPA interface test :-S
It would be nice if we could avoid making it public, but otherwise I can
live with that.

> +	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..d19348df 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"
> @@ -246,7 +247,8 @@ void CameraManager::Private::removeCamera(Camera *camera)
>  CameraManager *CameraManager::self_ = nullptr;
>  
>  CameraManager::CameraManager()
> -	: p_(new CameraManager::Private(this))
> +	: p_(new CameraManager::Private(this)), ipaManager_(new IPAManager())

As the IPAManager constructor takes no argument, I think you can embed
an instance in CameraManager (or rather in CameraManager::Private)
instead of using a pointer.

> +

Extra blank line.

>  {
>  	if (self_)
>  		LOG(Camera, Fatal)
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 505cf610..74112cde 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -93,8 +93,14 @@ LOG_DEFINE_CATEGORY(IPAManager)
>   * plain C API, or to transmit the data to the isolated process through IPC.
>   */
>  
> +IPAManager *IPAManager::self_ = nullptr;
> +

Now that the constructor is public, it should be documented, especially
to state that nobody but the CameraManager should create an instance of
IPAManager.

/**
 * \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 +140,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 +163,7 @@ IPAManager::~IPAManager()
>   */
>  IPAManager *IPAManager::instance()
>  {
> -	static IPAManager ipaManager;
> -	return &ipaManager;
> +	return self_;
>  }

As the only usage of instance() is to call createIPA() in pipeline
handlers, would it make sense (in another patch) to drop the instance()
function, and turn the createIPA() function into a static function
(using self_ to access the IPA manager instance) ?

>  
>  /**
> 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_;