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

[Laurent Pinchart]

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 at 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_;

-- 
Regards,

Laurent Pinchart