[libcamera-devel] [RFCv2 7/7] libcamera: ipa_manager: Proxy open-source IPAs to a thread
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 26 22:53:05 CET 2020
Hi Niklas,
On Thu, Mar 26, 2020 at 10:48:43PM +0100, Niklas Söderlund wrote:
> On 2020-03-26 23:37:26 +0200, Laurent Pinchart wrote:
> > 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.
I don't object as I've proposed it :-) Please make sure to update the
proxy documentation to explain that the queueFrameAction signal will not
be delivered after stop() returns.
> > > + 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
More information about the libcamera-devel
mailing list