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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-03-26 23:37:26 +0200, Laurent Pinchart wrote:
> 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 :-)

I will test it.

> 
> > +
> > +	return ipa_->start();
> 
> Shouldn't start be called in the target thread, as the thread is already
> running ?

Good point.

> 
> > +}
> > +
> > +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().

I think that could be neat. I thought about it but decided against it as 
it could create discrepancies in behavior for different IPAProxy 
implementations.

But since we will have relative few IPAProxy implementations and 
hopefully many pipelines I think it could make sens to move it here, I 
will do so for next version, if you don't object.

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

-- 
Regards,
Niklas Söderlund