[libcamera-devel] [RFC/PATCH] libcamera: ipa_manager: Proxy open-source IPAs to a thread
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 20 02:54:31 CET 2020
On Fri, Mar 20, 2020 at 03:16:18AM +0200, Laurent Pinchart wrote:
> 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>
> ---
> Hello,
>
> This patch is currently untested, I'm working on expanding the VIMC IPA
> to provide unit tests. I've however decided to send it already to get
> feedback on the idea.
Quick update, thanks to Niklas' testing of the patch, we've noticed an
issue with unmapBuffers() racing with processEvent(). I'll work on a
fix, but the general idea holds.
> src/libcamera/ipa_manager.cpp | 48 ++++-----
> src/libcamera/proxy/ipa_proxy_thread.cpp | 130 +++++++++++++++++++++++
> src/libcamera/proxy/meson.build | 1 +
> 3 files changed, 152 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 bcaae3564ea1..6d23f4705065 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();
> -
> - for (IPAProxyFactory *factory : factories) {
> - /* TODO: Better matching */
> - if (!strcmp(factory->name().c_str(), "IPAProxyLinux")) {
> - pf = factory;
> - break;
> - }
> - }
> -
> - if (!pf) {
> - LOG(IPAManager, Error) << "Failed to get proxy factory";
> - return nullptr;
> - }
> + /*
> + * 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;
>
> - std::unique_ptr<IPAProxy> proxy = pf->create(m);
> - if (!proxy->isValid()) {
> - LOG(IPAManager, Error) << "Failed to load proxy";
> - return nullptr;
> + for (IPAProxyFactory *factory : IPAProxyFactory::factories()) {
> + if (!strcmp(factory->name().c_str(), proxyName)) {
> + pf = factory;
> + break;
> }
> -
> - 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 000000000000..22006b3c98ee
> --- /dev/null
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -0,0 +1,130 @@
> +/* 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);
> + ~IPAProxyThread();
> +
> + int init() 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 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;
> +}
> +
> +IPAProxyThread::~IPAProxyThread()
> +{
> + thread_.exit();
> + thread_.wait();
> +}
> +
> +int IPAProxyThread::init()
> +{
> + thread_.start();
> + proxy_.moveToThread(&thread_);
> +
> + return ipa_->init();
> +}
> +
> +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)
> +{
> + 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 efc113230217..6c00d5f30ad2 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list