[libcamera-devel] [RFCv2 7/7] libcamera: ipa_manager: Proxy open-source IPAs to a thread

[Laurent Pinchart]

Hi Niklas,

Thank you for my patch :-)

On Thu, Mar 26, 2020 at 05:08:19PM +0100, Niklas Söderlund wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> While closed-source IPA modules will always be sandboxed, open-source
> IPA modules may be run in the main libcamera process or be sandboxed,
> depending on platform configuration. These two models exhibit very
> different timings, which require extensive testing with both
> configurations.
> 
> When run into the main libcamera process, IPA modules are executed in
> the pipeline handler thread (which is currently a global CameraManager
> thread). Time-consuming operations in the IPA may thus slow down the
> pipeline handler and compromise real-time behaviour. At least some
> pipeline handlers will thus likely spawn a thread to isolate the IPA,
> leading to code duplication in pipeline handlers.
> 
> Solve both issues by always proxying IPA modules. For open-source IPA
> modules that run in the libcamera process, a new IPAProxyThread class is
> added to run the IPA in a separate thread.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> [Niklas: Move thread start/stop of thread into start()/stop()]
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/ipa_manager.cpp            |  48 ++++----
>  src/libcamera/proxy/ipa_proxy_thread.cpp | 144 +++++++++++++++++++++++
>  src/libcamera/proxy/meson.build          |   1 +
>  3 files changed, 166 insertions(+), 27 deletions(-)
>  create mode 100644 src/libcamera/proxy/ipa_proxy_thread.cpp
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index bcaae3564ea14e07..6d23f470506561b8 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -12,7 +12,6 @@
>  #include <string.h>
>  #include <sys/types.h>
>  
> -#include "ipa_context_wrapper.h"
>  #include "ipa_module.h"
>  #include "ipa_proxy.h"
>  #include "log.h"
> @@ -271,40 +270,35 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe,
>  	if (!m)
>  		return nullptr;
>  
> -	if (!m->isOpenSource()) {
> -		IPAProxyFactory *pf = nullptr;
> -		std::vector<IPAProxyFactory *> &factories = IPAProxyFactory::factories();
> +	/*
> +	 * Load and run the IPA module in a thread if it is open-source, or
> +	 * isolate it in a separate process otherwise.
> +	 *
> +	 * \todo Implement a better proxy selection
> +	 */
> +	const char *proxyName = m->isOpenSource()
> +			      ? "IPAProxyThread" : "IPAProxyLinux";
> +	IPAProxyFactory *pf = nullptr;
>  
> -		for (IPAProxyFactory *factory : factories) {
> -			/* TODO: Better matching */
> -			if (!strcmp(factory->name().c_str(), "IPAProxyLinux")) {
> -				pf = factory;
> -				break;
> -			}
> +	for (IPAProxyFactory *factory : IPAProxyFactory::factories()) {
> +		if (!strcmp(factory->name().c_str(), proxyName)) {
> +			pf = factory;
> +			break;
>  		}
> -
> -		if (!pf) {
> -			LOG(IPAManager, Error) << "Failed to get proxy factory";
> -			return nullptr;
> -		}
> -
> -		std::unique_ptr<IPAProxy> proxy = pf->create(m);
> -		if (!proxy->isValid()) {
> -			LOG(IPAManager, Error) << "Failed to load proxy";
> -			return nullptr;
> -		}
> -
> -		return proxy;
>  	}
>  
> -	if (!m->load())
> +	if (!pf) {
> +		LOG(IPAManager, Error) << "Failed to get proxy factory";
>  		return nullptr;
> +	}
>  
> -	struct ipa_context *ctx = m->createContext();
> -	if (!ctx)
> +	std::unique_ptr<IPAProxy> proxy = pf->create(m);
> +	if (!proxy->isValid()) {
> +		LOG(IPAManager, Error) << "Failed to load proxy";
>  		return nullptr;
> +	}
>  
> -	return std::make_unique<IPAContextWrapper>(ctx);
> +	return proxy;
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> new file mode 100644
> index 0000000000000000..6719bcdda43f3647
> --- /dev/null
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -0,0 +1,144 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_proxy_thread.cpp - Proxy running an Image Processing Algorithm in a thread
> + */
> +
> +#include <memory>
> +
> +#include <ipa/ipa_interface.h>
> +#include <ipa/ipa_module_info.h>
> +
> +#include "ipa_context_wrapper.h"
> +#include "ipa_module.h"
> +#include "ipa_proxy.h"
> +#include "log.h"
> +#include "thread.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAProxy)
> +
> +class IPAProxyThread : public IPAProxy, public Object
> +{
> +public:
> +	IPAProxyThread(IPAModule *ipam);
> +
> +	int init() override;
> +	int start() override;
> +	void stop() override;
> +
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> +	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> +	void processEvent(const IPAOperationData &event) override;
> +
> +private:
> +	void queueFrameAction(unsigned int frame, const IPAOperationData &data);
> +
> +	/* Helper class to invoke processEvent() in another thread. */
> +	class ThreadProxy : public Object
> +	{
> +	public:
> +		void setIPA(IPAInterface *ipa)
> +		{
> +			ipa_ = ipa;
> +		}
> +
> +		void stop()
> +		{
> +			ipa_->stop();
> +		}
> +
> +		void processEvent(const IPAOperationData &event)
> +		{
> +			ipa_->processEvent(event);
> +		}
> +
> +	private:
> +		IPAInterface *ipa_;
> +	};
> +
> +	Thread thread_;
> +	ThreadProxy proxy_;
> +	std::unique_ptr<IPAInterface> ipa_;
> +};
> +
> +IPAProxyThread::IPAProxyThread(IPAModule *ipam)
> +{
> +	if (!ipam->load())
> +		return;
> +
> +	struct ipa_context *ctx = ipam->createContext();
> +	if (!ctx) {
> +		LOG(IPAProxy, Error)
> +			<< "Failed to create IPA context for " << ipam->path();
> +		return;
> +	}
> +
> +	ipa_ = std::make_unique<IPAContextWrapper>(ctx);
> +	proxy_.setIPA(ipa_.get());
> +
> +	/*
> +	 * Proxy the queueFrameAction signal to dispatch it in the caller's
> +	 * thread.
> +	 */
> +	ipa_->queueFrameAction.connect(this, &IPAProxyThread::queueFrameAction);
> +
> +	valid_ = true;
> +}
> +
> +int IPAProxyThread::init()
> +{
> +	return ipa_->init();
> +}
> +
> +int IPAProxyThread::start()
> +{
> +	thread_.start();
> +	proxy_.moveToThread(&thread_);

If we stop and restart, we will move the proxy to the thread twice. It
shouldn't be an issue, but I wonder if we should move the proxy at init
time, or even in the constructor. I *think* that moving and object to a
thread that hasn't been started yet is fine, but please test it :-)

> +
> +	return ipa_->start();

Shouldn't start be called in the target thread, as the thread is already
running ?

> +}
> +
> +void IPAProxyThread::stop()
> +{
> +	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
> +
> +	thread_.exit();
> +	thread_.wait();
> +}
> +
> +void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +{
> +	ipa_->configure(streamConfig, entityControls);
> +}
> +
> +void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> +{
> +	ipa_->mapBuffers(buffers);
> +}
> +
> +void IPAProxyThread::unmapBuffers(const std::vector<unsigned int> &ids)
> +{
> +	ipa_->unmapBuffers(ids);
> +}
> +
> +void IPAProxyThread::processEvent(const IPAOperationData &event)
> +{
> +	/* Dispatch the processEvent() call to the thread. */
> +	proxy_.invokeMethod(&ThreadProxy::processEvent, ConnectionTypeQueued,
> +			    event);
> +}
> +
> +void IPAProxyThread::queueFrameAction(unsigned int frame, const IPAOperationData &data)
> +{

Instead of blocking frame actions in the pipeline handlers, would it
make sense to block them here when we're stopping ? This function is
called in the pipeline handler thread, and so won't race with stop().

> +	IPAInterface::queueFrameAction.emit(frame, data);
> +}
> +
> +REGISTER_IPA_PROXY(IPAProxyThread)
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
> index efc1132302176f63..6c00d5f30ad2e5f0 100644
> --- a/src/libcamera/proxy/meson.build
> +++ b/src/libcamera/proxy/meson.build
> @@ -1,3 +1,4 @@
>  libcamera_sources += files([
>      'ipa_proxy_linux.cpp',
> +    'ipa_proxy_thread.cpp',
>  ])

-- 
Regards,

Laurent Pinchart