Message ID | 20190915190408.2507-1-laurent.pinchart@ideasonboard.com |
---|---|
State | Superseded |
Delegated to: | Laurent Pinchart |
Headers | show |
Series |
|
Related | show |
Hi Jacopo and Laurent, Thanks for your work. On 2019-09-15 22:04:08 +0300, Laurent Pinchart wrote: > From: Jacopo Mondi <jacopo@jmondi.org> > > The C++ objects that are expected to convey data through the IPA API > will have associated methods that would require IPAs to link to > libcamera. Even though the libcamera license allows this, suppliers of > closed-source IPAs may have a different interpretation. To ease their > mind and clearly separate vendor code and libcamera code, turn the IPA > API to plain C. The corresponding C objects will be stored in plain C > structures or have their binary format documented, removing the need for > linking to libcamera code on the IPA side. > > This is implemented by adding two new C structures, ipa_context and > ipa_operations. The ipa_operations contains function pointers for all > the IPA API operations. The ipa_context represents a context of > operation for the IPA, and is passed to the IPA oparations. The > IPAInterface class is retained as it is easier to use than a plain C API > for pipeline handlers, with a new IPAWrapper class that wraps the > ipa_context and ipa_operations into and IPAInterface. > > On the IPA module side usage of IPAInterface may be desired for IPAs > implemented in C++ that want to link to libcamera. For those IPAs, a new > IPAWrapperContext helper class is introduce to wrap the IPAInterface > implemented internally by the IPA module into an ipa_context and > ipa_operations. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> The fix in vimc.cpp should really be it's own patch as it's not related to the C API ;-) Other than that, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > Documentation/Doxyfile.in | 1 + > Documentation/meson.build | 2 + > include/ipa/ipa_interface.h | 16 ++++ > src/ipa/ipa_dummy.cpp | 6 +- > src/ipa/libipa/ipa_interface_wrapper.cpp | 74 ++++++++++++++++++ > src/ipa/libipa/ipa_interface_wrapper.h | 29 +++++++ > src/ipa/libipa/meson.build | 10 +++ > src/ipa/meson.build | 3 + > src/libcamera/include/ipa_context_wrapper.h | 26 +++++++ > src/libcamera/include/ipa_module.h | 5 +- > src/libcamera/include/meson.build | 1 + > src/libcamera/ipa_context_wrapper.cpp | 52 +++++++++++++ > src/libcamera/ipa_interface.cpp | 77 ++++++++++++++++++- > src/libcamera/ipa_manager.cpp | 67 +++++++++++++++- > src/libcamera/ipa_module.cpp | 23 +++--- > src/libcamera/meson.build | 1 + > src/libcamera/pipeline/vimc.cpp | 2 +- > .../proxy/worker/ipa_proxy_linux_worker.cpp | 8 +- > 18 files changed, 381 insertions(+), 22 deletions(-) > create mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp > create mode 100644 src/ipa/libipa/ipa_interface_wrapper.h > create mode 100644 src/ipa/libipa/meson.build > create mode 100644 src/libcamera/include/ipa_context_wrapper.h > create mode 100644 src/libcamera/ipa_context_wrapper.cpp > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > index ecc058eec683..2ab93687755f 100644 > --- a/Documentation/Doxyfile.in > +++ b/Documentation/Doxyfile.in > @@ -793,6 +793,7 @@ WARN_LOGFILE = > > INPUT = "@TOP_SRCDIR@/include/ipa" \ > "@TOP_SRCDIR@/include/libcamera" \ > + "@TOP_SRCDIR@/src/ipa/libipa" \ > "@TOP_SRCDIR@/src/libcamera" > > # This tag can be used to specify the character encoding of the source files > diff --git a/Documentation/meson.build b/Documentation/meson.build > index 4ff3fbeb0674..9136506f5d9c 100644 > --- a/Documentation/meson.build > +++ b/Documentation/meson.build > @@ -24,6 +24,8 @@ if doxygen.found() > libcamera_ipa_api, > libcamera_headers, > libcamera_sources, > + libipa_headers, > + libipa_sources, > ], > output : 'api-html', > command : [doxygen, doxyfile], > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > index 9bbc4cf58ec6..f1ebac20f151 100644 > --- a/include/ipa/ipa_interface.h > +++ b/include/ipa/ipa_interface.h > @@ -7,6 +7,21 @@ > #ifndef __LIBCAMERA_IPA_INTERFACE_H__ > #define __LIBCAMERA_IPA_INTERFACE_H__ > > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +struct ipa_context { > + const struct ipa_operations *ops; > +}; > + > +struct ipa_operations { > + void (*destroy)(struct ipa_context *ctx); > +}; > + > +#ifdef __cplusplus > +} > + > namespace libcamera { > > class IPAInterface > @@ -16,5 +31,6 @@ public: > }; > > } /* namespace libcamera */ > +#endif > > #endif /* __LIBCAMERA_IPA_INTERFACE_H__ */ > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp > index c833e5fb0b2d..6dc9448a3f56 100644 > --- a/src/ipa/ipa_dummy.cpp > +++ b/src/ipa/ipa_dummy.cpp > @@ -8,6 +8,8 @@ > #include <ipa/ipa_interface.h> > #include <ipa/ipa_module_info.h> > > +#include "libipa/ipa_interface_wrapper.h" > + > namespace libcamera { > > class IPADummy : public IPAInterface > @@ -27,9 +29,9 @@ const struct IPAModuleInfo ipaModuleInfo = { > LICENSE, > }; > > -IPAInterface *ipaCreate() > +struct ipa_context *ipaCreate() > { > - return new IPADummy(); > + return new IPAInterfaceWrapper(new IPADummy()); > } > }; > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp > new file mode 100644 > index 000000000000..aacd189851c3 > --- /dev/null > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp > @@ -0,0 +1,74 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * ipa_interface_wrapper.cpp - Image Processing Algorithm interface wrapper > + */ > + > +#include "ipa_interface_wrapper.h" > + > +#include <ipa/ipa_interface.h> > + > +/** > + * \file ipa_interface_wrapper.h > + * \brief Image Processing Algorithm interface wrapper > + */ > + > +namespace libcamera { > + > +/** > + * \class IPAInterfaceWrapper > + * \brief Wrap an IPAInterface and expose it as an ipa_context > + * > + * This class implements the ipa_context API based on a provided IPAInterface. > + * It helps IPAs that implement the IPAInterface API to provide the external > + * ipa_context API. > + * > + * To use the wrapper, an IPA module simple creates a new instance of its > + * IPAInterface implementation, and passes it to the constructor of the > + * IPAInterfaceWrapper. As IPAInterfaceWrapper inherits from ipa_context, the > + * constructed wrapper can then be directly returned from the IPA module's > + * ipaCreate() function. > + * > + * \code{.cpp} > + * class MyIPA : public IPAInterface > + * { > + * ... > + * }; > + * > + * struct ipa_context *ipaCreate() > + * { > + * return new IPAInterfaceWrapper(new MyIPA()); > + * } > + * \endcode > + */ > + > +/** > + * \brief Construct an IPAInterfaceWrapper wrapping \a interface > + * \param[in] interface The interface to wrap > + */ > +IPAInterfaceWrapper::IPAInterfaceWrapper(IPAInterface *interface) > + : ipa(interface) > +{ > + ops = &operations; > +} > + > +void IPAInterfaceWrapper::destroy(struct ipa_context *_ctx) > +{ > + IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx); > + > + delete ctx->ipa; > + delete ctx; > +} > + > +#ifndef __DOXYGEN__ > +/* > + * This construct confuses Doygen and makes it believe that all members of the > + * operations is a member of IPAInterfaceWrapper. It must thus be hidden. > + */ > +const struct ipa_operations IPAInterfaceWrapper::operations = { > + .destroy = &IPAInterfaceWrapper::destroy, > +}; > +#endif > + > +}; /* namespace libcamera */ > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h > new file mode 100644 > index 000000000000..d2ab46f50d3c > --- /dev/null > +++ b/src/ipa/libipa/ipa_interface_wrapper.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * ipa_interface_wrapper.h - Image Processing Algorithm interface wrapper > + */ > +#ifndef __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ > +#define __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ > + > +#include <ipa/ipa_interface.h> > + > +namespace libcamera { > + > +class IPAInterfaceWrapper : public ipa_context > +{ > +public: > + IPAInterfaceWrapper(IPAInterface *interface); > + > +private: > + static void destroy(struct ipa_context *ctx); > + > + static const struct ipa_operations operations; > + > + IPAInterface *ipa; > +}; > + > +} /* namespace libcamera */ > + > +#endif /* __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ */ > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > new file mode 100644 > index 000000000000..ab3515ed2021 > --- /dev/null > +++ b/src/ipa/libipa/meson.build > @@ -0,0 +1,10 @@ > +libipa_headers = files([ > + 'ipa_interface_wrapper.h', > +]) > + > +libipa_sources = files([ > + 'ipa_interface_wrapper.cpp', > +]) > + > +libipa = static_library('libipa', libipa_sources, > + dependencies : libcamera_dep) > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > index f09915bc1388..6483ea66a478 100644 > --- a/src/ipa/meson.build > +++ b/src/ipa/meson.build > @@ -1,3 +1,5 @@ > +subdir('libipa') > + > ipa_dummy_sources = [ > ['ipa_dummy', 'LGPL-2.1-or-later'], > ['ipa_dummy_isolate', 'Proprietary'], > @@ -9,6 +11,7 @@ foreach t : ipa_dummy_sources > ipa = shared_module(t[0], 'ipa_dummy.cpp', > name_prefix : '', > include_directories : libcamera_includes, > + link_with : libipa, > install : true, > install_dir : ipa_install_dir, > cpp_args : '-DLICENSE="' + t[1] + '"') > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h > new file mode 100644 > index 000000000000..12894ac6885e > --- /dev/null > +++ b/src/libcamera/include/ipa_context_wrapper.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * ipa_context_wrapper.h - Image Processing Algorithm context wrapper > + */ > +#ifndef __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ > +#define __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ > + > +#include <ipa/ipa_interface.h> > + > +namespace libcamera { > + > +class IPAContextWrapper final : public IPAInterface > +{ > +public: > + IPAContextWrapper(struct ipa_context *context); > + ~IPAContextWrapper(); > + > +private: > + struct ipa_context *ctx_; > +}; > + > +} /* namespace libcamera */ > + > +#endif /* __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ */ > diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h > index 97737587ab3a..2028b76a1913 100644 > --- a/src/libcamera/include/ipa_module.h > +++ b/src/libcamera/include/ipa_module.h > @@ -7,7 +7,6 @@ > #ifndef __LIBCAMERA_IPA_MODULE_H__ > #define __LIBCAMERA_IPA_MODULE_H__ > > -#include <memory> > #include <string> > > #include <ipa/ipa_interface.h> > @@ -30,7 +29,7 @@ public: > > bool load(); > > - std::unique_ptr<IPAInterface> createInstance(); > + struct ipa_context *createContext(); > > bool match(PipelineHandler *pipe, > uint32_t minVersion, uint32_t maxVersion) const; > @@ -45,7 +44,7 @@ private: > bool loaded_; > > void *dlHandle_; > - typedef IPAInterface *(*IPAIntfFactory)(void); > + typedef struct ipa_context *(*IPAIntfFactory)(void); > IPAIntfFactory ipaCreate_; > > int loadIPAModuleInfo(); > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build > index 933be8543a8d..0abb978a389a 100644 > --- a/src/libcamera/include/meson.build > +++ b/src/libcamera/include/meson.build > @@ -5,6 +5,7 @@ libcamera_headers = files([ > 'device_enumerator_udev.h', > 'event_dispatcher_poll.h', > 'formats.h', > + 'ipa_context_wrapper.h', > 'ipa_manager.h', > 'ipa_module.h', > 'ipa_proxy.h', > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp > new file mode 100644 > index 000000000000..87ff98d45c99 > --- /dev/null > +++ b/src/libcamera/ipa_context_wrapper.cpp > @@ -0,0 +1,52 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * ipa_context_wrapper.cpp - Image Processing Algorithm context wrapper > + */ > + > +#include "ipa_context_wrapper.h" > + > +#include <libcamera/controls.h> > + > +/** > + * \file ipa_context_wrapper.h > + * \brief Image Processing Algorithm context wrapper > + */ > + > +namespace libcamera { > + > +/** > + * \class IPAContextWrapper > + * \brief Wrap an ipa_context and expose it as an IPAInterface > + * > + * The IPAContextWrapper class wraps an ipa_context, provided by an IPA module, and > + * exposes an IPAInterface. This mechanism is used for IPAs that are not > + * isolated in a separate process to allow direct calls from pipeline handler > + * using the IPAInterface API instead of the lower-level ipa_context API. > + * > + * The IPAInterface methods are converted to the ipa_context API by serialising > + * all C++ arguments into plain C structures or byte arrays that contain no > + * pointer, as required by the ipa_context API. > + */ > + > +/** > + * \brief Construct an IPAContextWrapper instance that wraps the \a context > + * \param[in] context The IPA module context > + * > + * Ownership of the \a context is passed to the IPAContextWrapper. The context remains > + * valid for the whole lifetime of the wrapper and is destroyed automatically > + * with it. > + */ > +IPAContextWrapper::IPAContextWrapper(struct ipa_context *context) > + : ctx_(context) > +{ > +} > + > +IPAContextWrapper::~IPAContextWrapper() > +{ > + if (ctx_) > + ctx_->ops->destroy(ctx_); > +} > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > index f70d91ded1ab..6e83aab0fb73 100644 > --- a/src/libcamera/ipa_interface.cpp > +++ b/src/libcamera/ipa_interface.cpp > @@ -10,13 +10,88 @@ > /** > * \file ipa_interface.h > * \brief Image Processing Algorithm interface > + * > + * libcamera interfaces with IPA modules through a plain C interface. IPA > + * modules shall expose a public function name ipaCreate() with the following > + * prototype. > + * > + * \code{.c} > + * struct ipa_context *ipaCreate(); > + * \endcode > + * > + * The ipaCreate() function creates an instance of an IPA context, which models > + * a context of execution for the IPA. IPA modules shall support creating one > + * or multiple contexts, as required by their associated pipeline handler. > + * > + * The IPA module operations are defined in the struct ipa_operations. An IPA > + * module stores a pointer to the operations corresponding to its context in > + * the ipa_context::ops field. That pointer is immutable for the lifetime of > + * the context, and may be different for multiple contexts created by the same > + * IPA module. > + * > + * All argument to ipa_operations members are Plain Old Data and are documented > + * either in the form of C data types, or as a textual description for byte > + * arrays that can't be expressed using C data types (such as variable-length > + * arrays). IPA modules can thus use the C API without calling into libcamera > + * to access the data passed to the IPA operations. > + * > + * The IPAInterface class is a C++ representation of the ipa_operations, using > + * C++ data classes provided by libcamera. This is the API exposed to pipeline > + * handlers to communicate with IPA modules. IPA modules may use the > + * IPAInterface API internally if they want to benefit from the data and helper > + * classes offered by libcamera. > + */ > + > +/** > + * \struct ipa_context > + * \brief IPA module context of execution > + * > + * This structure models a context of execution for an IPA module. It is > + * instantiated by the IPA module ipaCreate() function. IPA modules allocate > + * context instances in an implementation-defined way, contexts shall thus be > + * destroyed using the ipa_operation::destroy operation only. > + * > + * The ipa_context structure provides a pointer to the IPA operations. It shall > + * otherwise be treated as a constant black-box cookie and passed unmodified to > + * the operations defined in struct ipa_operations. > + * > + * IPA modules are expected to extend struct ipa_context by inheriting from it, > + * either through structure embedding to model inheritance in plain C, or > + * through C++ class inheritance. A simple example of the latter is available > + * in the IPAContextWrapper class implementation. > + * > + * \var ipa_context::ops > + * \brief The IPA context operations > + */ > + > +/** > + * \struct ipa_operations > + * \brief IPA context operations as a set of function pointers > + * > + * To allow for isolation of IPA modules in separate processes, the functions > + * defined in the ipa_operations structure return only data related to the > + * libcamera side of the operations. In particular, error related to the > + * libcamera side of the IPC may be returned. Data returned by the IPA, > + * including status information, shall be provided through callbacks from the > + * IPA to libcamera. > + * > + * \var ipa_operations::destroy > + * \brief Destroy the ipa_context created by the module's ipaCreate() function > */ > > namespace libcamera { > > /** > * \class IPAInterface > - * \brief Interface for IPA implementation > + * \brief IPA module interface > + * > + * This pure virtual class defines a C++ API corresponding to the ipa_context > + * and ipa_operations API. It is used by pipeline handlers to interact with IPA > + * modules, and may be used internally in IPA modules if desired to benefit > + * from the data and helper classes provided by libcamera. > + * > + * As for the operations defined in struct ipa_operations, the methods defined > + * by this class shall not return data from the IPA. > */ > > } /* namespace libcamera */ > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index 4276d995bdd5..0a908eae6261 100644 > --- a/src/libcamera/ipa_manager.cpp > +++ b/src/libcamera/ipa_manager.cpp > @@ -11,6 +11,7 @@ > #include <string.h> > #include <sys/types.h> > > +#include "ipa_context_wrapper.h" > #include "ipa_module.h" > #include "ipa_proxy.h" > #include "log.h" > @@ -29,6 +30,66 @@ LOG_DEFINE_CATEGORY(IPAManager) > /** > * \class IPAManager > * \brief Manager for IPA modules > + * > + * The IPA module manager discovers IPA modules from disk, queries and loads > + * them, and creates IPA contexts. It supports isolation of the modules in a > + * separate process with IPC communication and offers a unified IPAInterface > + * view of the IPA contexts to pipeline handlers regardless of whether the > + * modules are isolated or loaded in the same process. > + * > + * Module isolation is based on the module licence. Open-source modules are > + * loaded without isolation, while closed-source module are forcefully isolated. > + * The isolation mechanism ensures that no code from a closed-source module is > + * ever run in the libcamera process. > + * > + * To create an IPA context, pipeline handlers call the IPAManager::ipaCreate() > + * method. For a directly loaded module, the manager calls the module's > + * ipaCreate() function directly and wraps the returned context in an > + * IPAContextWrapper that exposes an IPAInterface. > + * > + * ~~~~ > + * +---------------+ > + * | Pipeline | > + * | Handler | > + * +---------------+ > + * | > + * v > + * +---------------+ +---------------+ > + * | IPA | | Open Source | > + * | Interface | | IPA Module | > + * | - - - - - - - | | - - - - - - - | > + * | IPA Context | ipa_operations | ipa_context | > + * | Wrapper | ----------------> | | > + * +---------------+ +---------------+ > + * ~~~~ > + * > + * For an isolated module, the manager instantiates an IPAProxy which spawns a > + * new process for an IPA proxy worker. The worker loads the IPA module and > + * creates the IPA context. The IPAProxy alse exposes an IPAInterface. > + * > + * ~~~~ > + * +---------------+ +---------------+ > + * | Pipeline | | Closed Source | > + * | Handler | | IPA Module | > + * +---------------+ | - - - - - - - | > + * | | ipa_context | > + * v | | > + * +---------------+ +---------------+ > + * | IPA | ipa_operations ^ > + * | Interface | | > + * | - - - - - - - | +---------------+ > + * | IPA Proxy | operations | IPA Proxy | > + * | | ----------------> | Worker | > + * +---------------+ over IPC +---------------+ > + * ~~~~ > + * > + * The IPAInterface implemented by the IPAContextWrapper or IPAProxy is > + * returned to the pipeline handler, and all interactions with the IPA context > + * go the same interface regarless of process isolation. > + * > + * In all cases the data passed to the IPAInterface methods is serialised to > + * Plain Old Data, either for the purpose of passing it to the IPA context > + * plain C API, or to transmit the data to the isolated process through IPC. > */ > > IPAManager::IPAManager() > @@ -177,7 +238,11 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe, > if (!m->load()) > return nullptr; > > - return m->createInstance(); > + struct ipa_context *ctx = m->createContext(); > + if (!ctx) > + return nullptr; > + > + return utils::make_unique<IPAContextWrapper>(ctx); > } > > } /* namespace libcamera */ > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > index 99d308efd47b..9f5a01418f73 100644 > --- a/src/libcamera/ipa_module.cpp > +++ b/src/libcamera/ipa_module.cpp > @@ -385,13 +385,13 @@ const std::string &IPAModule::path() const > /** > * \brief Load the IPA implementation factory from the shared object > * > - * The IPA module shared object implements an IPAInterface class to be used > + * The IPA module shared object implements an ipa_context object to be used > * by pipeline handlers. This method loads the factory function from the > - * shared object. Later, createInstance() can be called to instantiate the > - * IPAInterface. > + * shared object. Later, createContext() can be called to instantiate the > + * ipa_context. > * > * This method only needs to be called successfully once, after which > - * createInstance() can be called as many times as IPAInterface instances are > + * createContext() can be called as many times as ipa_context instances are > * needed. > * > * Calling this function on an invalid module (as returned by isValid()) is > @@ -433,24 +433,25 @@ bool IPAModule::load() > } > > /** > - * \brief Instantiate an IPAInterface > + * \brief Instantiate an IPA context > * > - * After loading the IPA module with load(), this method creates an > - * instance of the IPA module interface. > + * After loading the IPA module with load(), this method creates an instance of > + * the IPA module context. Ownership of the context is passed to the caller, and > + * the context shall be destroyed by calling the \ref ipa_operations::destroy > + * "ipa_context::ops::destroy()" operation. > * > * Calling this function on a module that has not yet been loaded, or an > * invalid module (as returned by load() and isValid(), respectively) is > * an error. > * > - * \return The IPA implementation as a new IPAInterface instance on success, > - * or nullptr on error > + * \return The IPA context on success, or nullptr on error > */ > -std::unique_ptr<IPAInterface> IPAModule::createInstance() > +struct ipa_context *IPAModule::createContext() > { > if (!valid_ || !loaded_) > return nullptr; > > - return std::unique_ptr<IPAInterface>(ipaCreate_()); > + return ipaCreate_(); > } > > /** > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index 755149c55c7b..d3ae305e5655 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -12,6 +12,7 @@ libcamera_sources = files([ > 'event_notifier.cpp', > 'formats.cpp', > 'geometry.cpp', > + 'ipa_context_wrapper.cpp', > 'ipa_interface.cpp', > 'ipa_manager.cpp', > 'ipa_module.cpp', > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index a401b1099f3c..612edcda69d6 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -362,7 +362,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > return false; > > ipa_ = IPAManager::instance()->createIPA(this, 0, 0); > - if (ipa_ == nullptr) > + if (!ipa_) > LOG(VIMC, Warning) << "no matching IPA found"; > > std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this); > diff --git a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > index a10761e52b32..07380c16e2d5 100644 > --- a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > +++ b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > @@ -72,9 +72,9 @@ int main(int argc, char **argv) > } > socket.readyRead.connect(&readyRead); > > - std::unique_ptr<IPAInterface> ipa = ipam->createInstance(); > - if (!ipa) { > - LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA interface"; > + struct ipa_context *ipac = ipam->createContext(); > + if (!ipac) { > + LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA context"; > return EXIT_FAILURE; > } > > @@ -85,5 +85,7 @@ int main(int argc, char **argv) > while (1) > dispatcher->processEvents(); > > + ipac->ops->destroy(ipac); > + > return 0; > } > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Tue, Sep 17, 2019 at 11:36:03AM +0200, Niklas Söderlund wrote: > On 2019-09-15 22:04:08 +0300, Laurent Pinchart wrote: > > From: Jacopo Mondi <jacopo@jmondi.org> > > > > The C++ objects that are expected to convey data through the IPA API > > will have associated methods that would require IPAs to link to > > libcamera. Even though the libcamera license allows this, suppliers of > > closed-source IPAs may have a different interpretation. To ease their > > mind and clearly separate vendor code and libcamera code, turn the IPA > > API to plain C. The corresponding C objects will be stored in plain C > > structures or have their binary format documented, removing the need for > > linking to libcamera code on the IPA side. > > > > This is implemented by adding two new C structures, ipa_context and > > ipa_operations. The ipa_operations contains function pointers for all > > the IPA API operations. The ipa_context represents a context of > > operation for the IPA, and is passed to the IPA oparations. The > > IPAInterface class is retained as it is easier to use than a plain C API > > for pipeline handlers, with a new IPAWrapper class that wraps the > > ipa_context and ipa_operations into and IPAInterface. > > > > On the IPA module side usage of IPAInterface may be desired for IPAs > > implemented in C++ that want to link to libcamera. For those IPAs, a new > > IPAWrapperContext helper class is introduce to wrap the IPAInterface > > implemented internally by the IPA module into an ipa_context and > > ipa_operations. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The fix in vimc.cpp should really be it's own patch as it's not related > to the C API ;-) Other than that, Agreed, it could have been split. It was the result of a previous API version, and when undoing that I've left the != nullptr to ! change. Would you like me to send a new version, or can I leave it as-is this time ? > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > Documentation/Doxyfile.in | 1 + > > Documentation/meson.build | 2 + > > include/ipa/ipa_interface.h | 16 ++++ > > src/ipa/ipa_dummy.cpp | 6 +- > > src/ipa/libipa/ipa_interface_wrapper.cpp | 74 ++++++++++++++++++ > > src/ipa/libipa/ipa_interface_wrapper.h | 29 +++++++ > > src/ipa/libipa/meson.build | 10 +++ > > src/ipa/meson.build | 3 + > > src/libcamera/include/ipa_context_wrapper.h | 26 +++++++ > > src/libcamera/include/ipa_module.h | 5 +- > > src/libcamera/include/meson.build | 1 + > > src/libcamera/ipa_context_wrapper.cpp | 52 +++++++++++++ > > src/libcamera/ipa_interface.cpp | 77 ++++++++++++++++++- > > src/libcamera/ipa_manager.cpp | 67 +++++++++++++++- > > src/libcamera/ipa_module.cpp | 23 +++--- > > src/libcamera/meson.build | 1 + > > src/libcamera/pipeline/vimc.cpp | 2 +- > > .../proxy/worker/ipa_proxy_linux_worker.cpp | 8 +- > > 18 files changed, 381 insertions(+), 22 deletions(-) > > create mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp > > create mode 100644 src/ipa/libipa/ipa_interface_wrapper.h > > create mode 100644 src/ipa/libipa/meson.build > > create mode 100644 src/libcamera/include/ipa_context_wrapper.h > > create mode 100644 src/libcamera/ipa_context_wrapper.cpp > > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > > index ecc058eec683..2ab93687755f 100644 > > --- a/Documentation/Doxyfile.in > > +++ b/Documentation/Doxyfile.in > > @@ -793,6 +793,7 @@ WARN_LOGFILE = > > > > INPUT = "@TOP_SRCDIR@/include/ipa" \ > > "@TOP_SRCDIR@/include/libcamera" \ > > + "@TOP_SRCDIR@/src/ipa/libipa" \ > > "@TOP_SRCDIR@/src/libcamera" > > > > # This tag can be used to specify the character encoding of the source files > > diff --git a/Documentation/meson.build b/Documentation/meson.build > > index 4ff3fbeb0674..9136506f5d9c 100644 > > --- a/Documentation/meson.build > > +++ b/Documentation/meson.build > > @@ -24,6 +24,8 @@ if doxygen.found() > > libcamera_ipa_api, > > libcamera_headers, > > libcamera_sources, > > + libipa_headers, > > + libipa_sources, > > ], > > output : 'api-html', > > command : [doxygen, doxyfile], > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > > index 9bbc4cf58ec6..f1ebac20f151 100644 > > --- a/include/ipa/ipa_interface.h > > +++ b/include/ipa/ipa_interface.h > > @@ -7,6 +7,21 @@ > > #ifndef __LIBCAMERA_IPA_INTERFACE_H__ > > #define __LIBCAMERA_IPA_INTERFACE_H__ > > > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +struct ipa_context { > > + const struct ipa_operations *ops; > > +}; > > + > > +struct ipa_operations { > > + void (*destroy)(struct ipa_context *ctx); > > +}; > > + > > +#ifdef __cplusplus > > +} > > + > > namespace libcamera { > > > > class IPAInterface > > @@ -16,5 +31,6 @@ public: > > }; > > > > } /* namespace libcamera */ > > +#endif > > > > #endif /* __LIBCAMERA_IPA_INTERFACE_H__ */ > > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp > > index c833e5fb0b2d..6dc9448a3f56 100644 > > --- a/src/ipa/ipa_dummy.cpp > > +++ b/src/ipa/ipa_dummy.cpp > > @@ -8,6 +8,8 @@ > > #include <ipa/ipa_interface.h> > > #include <ipa/ipa_module_info.h> > > > > +#include "libipa/ipa_interface_wrapper.h" > > + > > namespace libcamera { > > > > class IPADummy : public IPAInterface > > @@ -27,9 +29,9 @@ const struct IPAModuleInfo ipaModuleInfo = { > > LICENSE, > > }; > > > > -IPAInterface *ipaCreate() > > +struct ipa_context *ipaCreate() > > { > > - return new IPADummy(); > > + return new IPAInterfaceWrapper(new IPADummy()); > > } > > }; > > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp > > new file mode 100644 > > index 000000000000..aacd189851c3 > > --- /dev/null > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp > > @@ -0,0 +1,74 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * ipa_interface_wrapper.cpp - Image Processing Algorithm interface wrapper > > + */ > > + > > +#include "ipa_interface_wrapper.h" > > + > > +#include <ipa/ipa_interface.h> > > + > > +/** > > + * \file ipa_interface_wrapper.h > > + * \brief Image Processing Algorithm interface wrapper > > + */ > > + > > +namespace libcamera { > > + > > +/** > > + * \class IPAInterfaceWrapper > > + * \brief Wrap an IPAInterface and expose it as an ipa_context > > + * > > + * This class implements the ipa_context API based on a provided IPAInterface. > > + * It helps IPAs that implement the IPAInterface API to provide the external > > + * ipa_context API. > > + * > > + * To use the wrapper, an IPA module simple creates a new instance of its > > + * IPAInterface implementation, and passes it to the constructor of the > > + * IPAInterfaceWrapper. As IPAInterfaceWrapper inherits from ipa_context, the > > + * constructed wrapper can then be directly returned from the IPA module's > > + * ipaCreate() function. > > + * > > + * \code{.cpp} > > + * class MyIPA : public IPAInterface > > + * { > > + * ... > > + * }; > > + * > > + * struct ipa_context *ipaCreate() > > + * { > > + * return new IPAInterfaceWrapper(new MyIPA()); > > + * } > > + * \endcode > > + */ > > + > > +/** > > + * \brief Construct an IPAInterfaceWrapper wrapping \a interface > > + * \param[in] interface The interface to wrap > > + */ > > +IPAInterfaceWrapper::IPAInterfaceWrapper(IPAInterface *interface) > > + : ipa(interface) > > +{ > > + ops = &operations; > > +} > > + > > +void IPAInterfaceWrapper::destroy(struct ipa_context *_ctx) > > +{ > > + IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx); > > + > > + delete ctx->ipa; > > + delete ctx; > > +} > > + > > +#ifndef __DOXYGEN__ > > +/* > > + * This construct confuses Doygen and makes it believe that all members of the > > + * operations is a member of IPAInterfaceWrapper. It must thus be hidden. > > + */ > > +const struct ipa_operations IPAInterfaceWrapper::operations = { > > + .destroy = &IPAInterfaceWrapper::destroy, > > +}; > > +#endif > > + > > +}; /* namespace libcamera */ > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h > > new file mode 100644 > > index 000000000000..d2ab46f50d3c > > --- /dev/null > > +++ b/src/ipa/libipa/ipa_interface_wrapper.h > > @@ -0,0 +1,29 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * ipa_interface_wrapper.h - Image Processing Algorithm interface wrapper > > + */ > > +#ifndef __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ > > +#define __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ > > + > > +#include <ipa/ipa_interface.h> > > + > > +namespace libcamera { > > + > > +class IPAInterfaceWrapper : public ipa_context > > +{ > > +public: > > + IPAInterfaceWrapper(IPAInterface *interface); > > + > > +private: > > + static void destroy(struct ipa_context *ctx); > > + > > + static const struct ipa_operations operations; > > + > > + IPAInterface *ipa; > > +}; > > + > > +} /* namespace libcamera */ > > + > > +#endif /* __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ */ > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > > new file mode 100644 > > index 000000000000..ab3515ed2021 > > --- /dev/null > > +++ b/src/ipa/libipa/meson.build > > @@ -0,0 +1,10 @@ > > +libipa_headers = files([ > > + 'ipa_interface_wrapper.h', > > +]) > > + > > +libipa_sources = files([ > > + 'ipa_interface_wrapper.cpp', > > +]) > > + > > +libipa = static_library('libipa', libipa_sources, > > + dependencies : libcamera_dep) > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > > index f09915bc1388..6483ea66a478 100644 > > --- a/src/ipa/meson.build > > +++ b/src/ipa/meson.build > > @@ -1,3 +1,5 @@ > > +subdir('libipa') > > + > > ipa_dummy_sources = [ > > ['ipa_dummy', 'LGPL-2.1-or-later'], > > ['ipa_dummy_isolate', 'Proprietary'], > > @@ -9,6 +11,7 @@ foreach t : ipa_dummy_sources > > ipa = shared_module(t[0], 'ipa_dummy.cpp', > > name_prefix : '', > > include_directories : libcamera_includes, > > + link_with : libipa, > > install : true, > > install_dir : ipa_install_dir, > > cpp_args : '-DLICENSE="' + t[1] + '"') > > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h > > new file mode 100644 > > index 000000000000..12894ac6885e > > --- /dev/null > > +++ b/src/libcamera/include/ipa_context_wrapper.h > > @@ -0,0 +1,26 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * ipa_context_wrapper.h - Image Processing Algorithm context wrapper > > + */ > > +#ifndef __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ > > +#define __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ > > + > > +#include <ipa/ipa_interface.h> > > + > > +namespace libcamera { > > + > > +class IPAContextWrapper final : public IPAInterface > > +{ > > +public: > > + IPAContextWrapper(struct ipa_context *context); > > + ~IPAContextWrapper(); > > + > > +private: > > + struct ipa_context *ctx_; > > +}; > > + > > +} /* namespace libcamera */ > > + > > +#endif /* __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ */ > > diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h > > index 97737587ab3a..2028b76a1913 100644 > > --- a/src/libcamera/include/ipa_module.h > > +++ b/src/libcamera/include/ipa_module.h > > @@ -7,7 +7,6 @@ > > #ifndef __LIBCAMERA_IPA_MODULE_H__ > > #define __LIBCAMERA_IPA_MODULE_H__ > > > > -#include <memory> > > #include <string> > > > > #include <ipa/ipa_interface.h> > > @@ -30,7 +29,7 @@ public: > > > > bool load(); > > > > - std::unique_ptr<IPAInterface> createInstance(); > > + struct ipa_context *createContext(); > > > > bool match(PipelineHandler *pipe, > > uint32_t minVersion, uint32_t maxVersion) const; > > @@ -45,7 +44,7 @@ private: > > bool loaded_; > > > > void *dlHandle_; > > - typedef IPAInterface *(*IPAIntfFactory)(void); > > + typedef struct ipa_context *(*IPAIntfFactory)(void); > > IPAIntfFactory ipaCreate_; > > > > int loadIPAModuleInfo(); > > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build > > index 933be8543a8d..0abb978a389a 100644 > > --- a/src/libcamera/include/meson.build > > +++ b/src/libcamera/include/meson.build > > @@ -5,6 +5,7 @@ libcamera_headers = files([ > > 'device_enumerator_udev.h', > > 'event_dispatcher_poll.h', > > 'formats.h', > > + 'ipa_context_wrapper.h', > > 'ipa_manager.h', > > 'ipa_module.h', > > 'ipa_proxy.h', > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp > > new file mode 100644 > > index 000000000000..87ff98d45c99 > > --- /dev/null > > +++ b/src/libcamera/ipa_context_wrapper.cpp > > @@ -0,0 +1,52 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * ipa_context_wrapper.cpp - Image Processing Algorithm context wrapper > > + */ > > + > > +#include "ipa_context_wrapper.h" > > + > > +#include <libcamera/controls.h> > > + > > +/** > > + * \file ipa_context_wrapper.h > > + * \brief Image Processing Algorithm context wrapper > > + */ > > + > > +namespace libcamera { > > + > > +/** > > + * \class IPAContextWrapper > > + * \brief Wrap an ipa_context and expose it as an IPAInterface > > + * > > + * The IPAContextWrapper class wraps an ipa_context, provided by an IPA module, and > > + * exposes an IPAInterface. This mechanism is used for IPAs that are not > > + * isolated in a separate process to allow direct calls from pipeline handler > > + * using the IPAInterface API instead of the lower-level ipa_context API. > > + * > > + * The IPAInterface methods are converted to the ipa_context API by serialising > > + * all C++ arguments into plain C structures or byte arrays that contain no > > + * pointer, as required by the ipa_context API. > > + */ > > + > > +/** > > + * \brief Construct an IPAContextWrapper instance that wraps the \a context > > + * \param[in] context The IPA module context > > + * > > + * Ownership of the \a context is passed to the IPAContextWrapper. The context remains > > + * valid for the whole lifetime of the wrapper and is destroyed automatically > > + * with it. > > + */ > > +IPAContextWrapper::IPAContextWrapper(struct ipa_context *context) > > + : ctx_(context) > > +{ > > +} > > + > > +IPAContextWrapper::~IPAContextWrapper() > > +{ > > + if (ctx_) > > + ctx_->ops->destroy(ctx_); > > +} > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > > index f70d91ded1ab..6e83aab0fb73 100644 > > --- a/src/libcamera/ipa_interface.cpp > > +++ b/src/libcamera/ipa_interface.cpp > > @@ -10,13 +10,88 @@ > > /** > > * \file ipa_interface.h > > * \brief Image Processing Algorithm interface > > + * > > + * libcamera interfaces with IPA modules through a plain C interface. IPA > > + * modules shall expose a public function name ipaCreate() with the following > > + * prototype. > > + * > > + * \code{.c} > > + * struct ipa_context *ipaCreate(); > > + * \endcode > > + * > > + * The ipaCreate() function creates an instance of an IPA context, which models > > + * a context of execution for the IPA. IPA modules shall support creating one > > + * or multiple contexts, as required by their associated pipeline handler. > > + * > > + * The IPA module operations are defined in the struct ipa_operations. An IPA > > + * module stores a pointer to the operations corresponding to its context in > > + * the ipa_context::ops field. That pointer is immutable for the lifetime of > > + * the context, and may be different for multiple contexts created by the same > > + * IPA module. > > + * > > + * All argument to ipa_operations members are Plain Old Data and are documented > > + * either in the form of C data types, or as a textual description for byte > > + * arrays that can't be expressed using C data types (such as variable-length > > + * arrays). IPA modules can thus use the C API without calling into libcamera > > + * to access the data passed to the IPA operations. > > + * > > + * The IPAInterface class is a C++ representation of the ipa_operations, using > > + * C++ data classes provided by libcamera. This is the API exposed to pipeline > > + * handlers to communicate with IPA modules. IPA modules may use the > > + * IPAInterface API internally if they want to benefit from the data and helper > > + * classes offered by libcamera. > > + */ > > + > > +/** > > + * \struct ipa_context > > + * \brief IPA module context of execution > > + * > > + * This structure models a context of execution for an IPA module. It is > > + * instantiated by the IPA module ipaCreate() function. IPA modules allocate > > + * context instances in an implementation-defined way, contexts shall thus be > > + * destroyed using the ipa_operation::destroy operation only. > > + * > > + * The ipa_context structure provides a pointer to the IPA operations. It shall > > + * otherwise be treated as a constant black-box cookie and passed unmodified to > > + * the operations defined in struct ipa_operations. > > + * > > + * IPA modules are expected to extend struct ipa_context by inheriting from it, > > + * either through structure embedding to model inheritance in plain C, or > > + * through C++ class inheritance. A simple example of the latter is available > > + * in the IPAContextWrapper class implementation. > > + * > > + * \var ipa_context::ops > > + * \brief The IPA context operations > > + */ > > + > > +/** > > + * \struct ipa_operations > > + * \brief IPA context operations as a set of function pointers > > + * > > + * To allow for isolation of IPA modules in separate processes, the functions > > + * defined in the ipa_operations structure return only data related to the > > + * libcamera side of the operations. In particular, error related to the > > + * libcamera side of the IPC may be returned. Data returned by the IPA, > > + * including status information, shall be provided through callbacks from the > > + * IPA to libcamera. > > + * > > + * \var ipa_operations::destroy > > + * \brief Destroy the ipa_context created by the module's ipaCreate() function > > */ > > > > namespace libcamera { > > > > /** > > * \class IPAInterface > > - * \brief Interface for IPA implementation > > + * \brief IPA module interface > > + * > > + * This pure virtual class defines a C++ API corresponding to the ipa_context > > + * and ipa_operations API. It is used by pipeline handlers to interact with IPA > > + * modules, and may be used internally in IPA modules if desired to benefit > > + * from the data and helper classes provided by libcamera. > > + * > > + * As for the operations defined in struct ipa_operations, the methods defined > > + * by this class shall not return data from the IPA. > > */ > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > index 4276d995bdd5..0a908eae6261 100644 > > --- a/src/libcamera/ipa_manager.cpp > > +++ b/src/libcamera/ipa_manager.cpp > > @@ -11,6 +11,7 @@ > > #include <string.h> > > #include <sys/types.h> > > > > +#include "ipa_context_wrapper.h" > > #include "ipa_module.h" > > #include "ipa_proxy.h" > > #include "log.h" > > @@ -29,6 +30,66 @@ LOG_DEFINE_CATEGORY(IPAManager) > > /** > > * \class IPAManager > > * \brief Manager for IPA modules > > + * > > + * The IPA module manager discovers IPA modules from disk, queries and loads > > + * them, and creates IPA contexts. It supports isolation of the modules in a > > + * separate process with IPC communication and offers a unified IPAInterface > > + * view of the IPA contexts to pipeline handlers regardless of whether the > > + * modules are isolated or loaded in the same process. > > + * > > + * Module isolation is based on the module licence. Open-source modules are > > + * loaded without isolation, while closed-source module are forcefully isolated. > > + * The isolation mechanism ensures that no code from a closed-source module is > > + * ever run in the libcamera process. > > + * > > + * To create an IPA context, pipeline handlers call the IPAManager::ipaCreate() > > + * method. For a directly loaded module, the manager calls the module's > > + * ipaCreate() function directly and wraps the returned context in an > > + * IPAContextWrapper that exposes an IPAInterface. > > + * > > + * ~~~~ > > + * +---------------+ > > + * | Pipeline | > > + * | Handler | > > + * +---------------+ > > + * | > > + * v > > + * +---------------+ +---------------+ > > + * | IPA | | Open Source | > > + * | Interface | | IPA Module | > > + * | - - - - - - - | | - - - - - - - | > > + * | IPA Context | ipa_operations | ipa_context | > > + * | Wrapper | ----------------> | | > > + * +---------------+ +---------------+ > > + * ~~~~ > > + * > > + * For an isolated module, the manager instantiates an IPAProxy which spawns a > > + * new process for an IPA proxy worker. The worker loads the IPA module and > > + * creates the IPA context. The IPAProxy alse exposes an IPAInterface. > > + * > > + * ~~~~ > > + * +---------------+ +---------------+ > > + * | Pipeline | | Closed Source | > > + * | Handler | | IPA Module | > > + * +---------------+ | - - - - - - - | > > + * | | ipa_context | > > + * v | | > > + * +---------------+ +---------------+ > > + * | IPA | ipa_operations ^ > > + * | Interface | | > > + * | - - - - - - - | +---------------+ > > + * | IPA Proxy | operations | IPA Proxy | > > + * | | ----------------> | Worker | > > + * +---------------+ over IPC +---------------+ > > + * ~~~~ > > + * > > + * The IPAInterface implemented by the IPAContextWrapper or IPAProxy is > > + * returned to the pipeline handler, and all interactions with the IPA context > > + * go the same interface regarless of process isolation. > > + * > > + * In all cases the data passed to the IPAInterface methods is serialised to > > + * Plain Old Data, either for the purpose of passing it to the IPA context > > + * plain C API, or to transmit the data to the isolated process through IPC. > > */ > > > > IPAManager::IPAManager() > > @@ -177,7 +238,11 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe, > > if (!m->load()) > > return nullptr; > > > > - return m->createInstance(); > > + struct ipa_context *ctx = m->createContext(); > > + if (!ctx) > > + return nullptr; > > + > > + return utils::make_unique<IPAContextWrapper>(ctx); > > } > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > > index 99d308efd47b..9f5a01418f73 100644 > > --- a/src/libcamera/ipa_module.cpp > > +++ b/src/libcamera/ipa_module.cpp > > @@ -385,13 +385,13 @@ const std::string &IPAModule::path() const > > /** > > * \brief Load the IPA implementation factory from the shared object > > * > > - * The IPA module shared object implements an IPAInterface class to be used > > + * The IPA module shared object implements an ipa_context object to be used > > * by pipeline handlers. This method loads the factory function from the > > - * shared object. Later, createInstance() can be called to instantiate the > > - * IPAInterface. > > + * shared object. Later, createContext() can be called to instantiate the > > + * ipa_context. > > * > > * This method only needs to be called successfully once, after which > > - * createInstance() can be called as many times as IPAInterface instances are > > + * createContext() can be called as many times as ipa_context instances are > > * needed. > > * > > * Calling this function on an invalid module (as returned by isValid()) is > > @@ -433,24 +433,25 @@ bool IPAModule::load() > > } > > > > /** > > - * \brief Instantiate an IPAInterface > > + * \brief Instantiate an IPA context > > * > > - * After loading the IPA module with load(), this method creates an > > - * instance of the IPA module interface. > > + * After loading the IPA module with load(), this method creates an instance of > > + * the IPA module context. Ownership of the context is passed to the caller, and > > + * the context shall be destroyed by calling the \ref ipa_operations::destroy > > + * "ipa_context::ops::destroy()" operation. > > * > > * Calling this function on a module that has not yet been loaded, or an > > * invalid module (as returned by load() and isValid(), respectively) is > > * an error. > > * > > - * \return The IPA implementation as a new IPAInterface instance on success, > > - * or nullptr on error > > + * \return The IPA context on success, or nullptr on error > > */ > > -std::unique_ptr<IPAInterface> IPAModule::createInstance() > > +struct ipa_context *IPAModule::createContext() > > { > > if (!valid_ || !loaded_) > > return nullptr; > > > > - return std::unique_ptr<IPAInterface>(ipaCreate_()); > > + return ipaCreate_(); > > } > > > > /** > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index 755149c55c7b..d3ae305e5655 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -12,6 +12,7 @@ libcamera_sources = files([ > > 'event_notifier.cpp', > > 'formats.cpp', > > 'geometry.cpp', > > + 'ipa_context_wrapper.cpp', > > 'ipa_interface.cpp', > > 'ipa_manager.cpp', > > 'ipa_module.cpp', > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index a401b1099f3c..612edcda69d6 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -362,7 +362,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > > return false; > > > > ipa_ = IPAManager::instance()->createIPA(this, 0, 0); > > - if (ipa_ == nullptr) > > + if (!ipa_) > > LOG(VIMC, Warning) << "no matching IPA found"; > > > > std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this); > > diff --git a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > > index a10761e52b32..07380c16e2d5 100644 > > --- a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > > +++ b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > > @@ -72,9 +72,9 @@ int main(int argc, char **argv) > > } > > socket.readyRead.connect(&readyRead); > > > > - std::unique_ptr<IPAInterface> ipa = ipam->createInstance(); > > - if (!ipa) { > > - LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA interface"; > > + struct ipa_context *ipac = ipam->createContext(); > > + if (!ipac) { > > + LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA context"; > > return EXIT_FAILURE; > > } > > > > @@ -85,5 +85,7 @@ int main(int argc, char **argv) > > while (1) > > dispatcher->processEvents(); > > > > + ipac->ops->destroy(ipac); > > + > > return 0; > > }
Hi Laurent, On 2019-09-18 21:05:52 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Tue, Sep 17, 2019 at 11:36:03AM +0200, Niklas Söderlund wrote: > > On 2019-09-15 22:04:08 +0300, Laurent Pinchart wrote: > > > From: Jacopo Mondi <jacopo@jmondi.org> > > > > > > The C++ objects that are expected to convey data through the IPA API > > > will have associated methods that would require IPAs to link to > > > libcamera. Even though the libcamera license allows this, suppliers of > > > closed-source IPAs may have a different interpretation. To ease their > > > mind and clearly separate vendor code and libcamera code, turn the IPA > > > API to plain C. The corresponding C objects will be stored in plain C > > > structures or have their binary format documented, removing the need for > > > linking to libcamera code on the IPA side. > > > > > > This is implemented by adding two new C structures, ipa_context and > > > ipa_operations. The ipa_operations contains function pointers for all > > > the IPA API operations. The ipa_context represents a context of > > > operation for the IPA, and is passed to the IPA oparations. The > > > IPAInterface class is retained as it is easier to use than a plain C API > > > for pipeline handlers, with a new IPAWrapper class that wraps the > > > ipa_context and ipa_operations into and IPAInterface. > > > > > > On the IPA module side usage of IPAInterface may be desired for IPAs > > > implemented in C++ that want to link to libcamera. For those IPAs, a new > > > IPAWrapperContext helper class is introduce to wrap the IPAInterface > > > implemented internally by the IPA module into an ipa_context and > > > ipa_operations. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > The fix in vimc.cpp should really be it's own patch as it's not related > > to the C API ;-) Other than that, > > Agreed, it could have been split. It was the result of a previous API > version, and when undoing that I've left the != nullptr to ! change. > Would you like me to send a new version, or can I leave it as-is this > time ? I'm fine either way ;-) > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > --- > > > Documentation/Doxyfile.in | 1 + > > > Documentation/meson.build | 2 + > > > include/ipa/ipa_interface.h | 16 ++++ > > > src/ipa/ipa_dummy.cpp | 6 +- > > > src/ipa/libipa/ipa_interface_wrapper.cpp | 74 ++++++++++++++++++ > > > src/ipa/libipa/ipa_interface_wrapper.h | 29 +++++++ > > > src/ipa/libipa/meson.build | 10 +++ > > > src/ipa/meson.build | 3 + > > > src/libcamera/include/ipa_context_wrapper.h | 26 +++++++ > > > src/libcamera/include/ipa_module.h | 5 +- > > > src/libcamera/include/meson.build | 1 + > > > src/libcamera/ipa_context_wrapper.cpp | 52 +++++++++++++ > > > src/libcamera/ipa_interface.cpp | 77 ++++++++++++++++++- > > > src/libcamera/ipa_manager.cpp | 67 +++++++++++++++- > > > src/libcamera/ipa_module.cpp | 23 +++--- > > > src/libcamera/meson.build | 1 + > > > src/libcamera/pipeline/vimc.cpp | 2 +- > > > .../proxy/worker/ipa_proxy_linux_worker.cpp | 8 +- > > > 18 files changed, 381 insertions(+), 22 deletions(-) > > > create mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp > > > create mode 100644 src/ipa/libipa/ipa_interface_wrapper.h > > > create mode 100644 src/ipa/libipa/meson.build > > > create mode 100644 src/libcamera/include/ipa_context_wrapper.h > > > create mode 100644 src/libcamera/ipa_context_wrapper.cpp > > > > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > > > index ecc058eec683..2ab93687755f 100644 > > > --- a/Documentation/Doxyfile.in > > > +++ b/Documentation/Doxyfile.in > > > @@ -793,6 +793,7 @@ WARN_LOGFILE = > > > > > > INPUT = "@TOP_SRCDIR@/include/ipa" \ > > > "@TOP_SRCDIR@/include/libcamera" \ > > > + "@TOP_SRCDIR@/src/ipa/libipa" \ > > > "@TOP_SRCDIR@/src/libcamera" > > > > > > # This tag can be used to specify the character encoding of the source files > > > diff --git a/Documentation/meson.build b/Documentation/meson.build > > > index 4ff3fbeb0674..9136506f5d9c 100644 > > > --- a/Documentation/meson.build > > > +++ b/Documentation/meson.build > > > @@ -24,6 +24,8 @@ if doxygen.found() > > > libcamera_ipa_api, > > > libcamera_headers, > > > libcamera_sources, > > > + libipa_headers, > > > + libipa_sources, > > > ], > > > output : 'api-html', > > > command : [doxygen, doxyfile], > > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > > > index 9bbc4cf58ec6..f1ebac20f151 100644 > > > --- a/include/ipa/ipa_interface.h > > > +++ b/include/ipa/ipa_interface.h > > > @@ -7,6 +7,21 @@ > > > #ifndef __LIBCAMERA_IPA_INTERFACE_H__ > > > #define __LIBCAMERA_IPA_INTERFACE_H__ > > > > > > +#ifdef __cplusplus > > > +extern "C" { > > > +#endif > > > + > > > +struct ipa_context { > > > + const struct ipa_operations *ops; > > > +}; > > > + > > > +struct ipa_operations { > > > + void (*destroy)(struct ipa_context *ctx); > > > +}; > > > + > > > +#ifdef __cplusplus > > > +} > > > + > > > namespace libcamera { > > > > > > class IPAInterface > > > @@ -16,5 +31,6 @@ public: > > > }; > > > > > > } /* namespace libcamera */ > > > +#endif > > > > > > #endif /* __LIBCAMERA_IPA_INTERFACE_H__ */ > > > diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp > > > index c833e5fb0b2d..6dc9448a3f56 100644 > > > --- a/src/ipa/ipa_dummy.cpp > > > +++ b/src/ipa/ipa_dummy.cpp > > > @@ -8,6 +8,8 @@ > > > #include <ipa/ipa_interface.h> > > > #include <ipa/ipa_module_info.h> > > > > > > +#include "libipa/ipa_interface_wrapper.h" > > > + > > > namespace libcamera { > > > > > > class IPADummy : public IPAInterface > > > @@ -27,9 +29,9 @@ const struct IPAModuleInfo ipaModuleInfo = { > > > LICENSE, > > > }; > > > > > > -IPAInterface *ipaCreate() > > > +struct ipa_context *ipaCreate() > > > { > > > - return new IPADummy(); > > > + return new IPAInterfaceWrapper(new IPADummy()); > > > } > > > }; > > > > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp > > > new file mode 100644 > > > index 000000000000..aacd189851c3 > > > --- /dev/null > > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp > > > @@ -0,0 +1,74 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Google Inc. > > > + * > > > + * ipa_interface_wrapper.cpp - Image Processing Algorithm interface wrapper > > > + */ > > > + > > > +#include "ipa_interface_wrapper.h" > > > + > > > +#include <ipa/ipa_interface.h> > > > + > > > +/** > > > + * \file ipa_interface_wrapper.h > > > + * \brief Image Processing Algorithm interface wrapper > > > + */ > > > + > > > +namespace libcamera { > > > + > > > +/** > > > + * \class IPAInterfaceWrapper > > > + * \brief Wrap an IPAInterface and expose it as an ipa_context > > > + * > > > + * This class implements the ipa_context API based on a provided IPAInterface. > > > + * It helps IPAs that implement the IPAInterface API to provide the external > > > + * ipa_context API. > > > + * > > > + * To use the wrapper, an IPA module simple creates a new instance of its > > > + * IPAInterface implementation, and passes it to the constructor of the > > > + * IPAInterfaceWrapper. As IPAInterfaceWrapper inherits from ipa_context, the > > > + * constructed wrapper can then be directly returned from the IPA module's > > > + * ipaCreate() function. > > > + * > > > + * \code{.cpp} > > > + * class MyIPA : public IPAInterface > > > + * { > > > + * ... > > > + * }; > > > + * > > > + * struct ipa_context *ipaCreate() > > > + * { > > > + * return new IPAInterfaceWrapper(new MyIPA()); > > > + * } > > > + * \endcode > > > + */ > > > + > > > +/** > > > + * \brief Construct an IPAInterfaceWrapper wrapping \a interface > > > + * \param[in] interface The interface to wrap > > > + */ > > > +IPAInterfaceWrapper::IPAInterfaceWrapper(IPAInterface *interface) > > > + : ipa(interface) > > > +{ > > > + ops = &operations; > > > +} > > > + > > > +void IPAInterfaceWrapper::destroy(struct ipa_context *_ctx) > > > +{ > > > + IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx); > > > + > > > + delete ctx->ipa; > > > + delete ctx; > > > +} > > > + > > > +#ifndef __DOXYGEN__ > > > +/* > > > + * This construct confuses Doygen and makes it believe that all members of the > > > + * operations is a member of IPAInterfaceWrapper. It must thus be hidden. > > > + */ > > > +const struct ipa_operations IPAInterfaceWrapper::operations = { > > > + .destroy = &IPAInterfaceWrapper::destroy, > > > +}; > > > +#endif > > > + > > > +}; /* namespace libcamera */ > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h > > > new file mode 100644 > > > index 000000000000..d2ab46f50d3c > > > --- /dev/null > > > +++ b/src/ipa/libipa/ipa_interface_wrapper.h > > > @@ -0,0 +1,29 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Google Inc. > > > + * > > > + * ipa_interface_wrapper.h - Image Processing Algorithm interface wrapper > > > + */ > > > +#ifndef __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ > > > +#define __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ > > > + > > > +#include <ipa/ipa_interface.h> > > > + > > > +namespace libcamera { > > > + > > > +class IPAInterfaceWrapper : public ipa_context > > > +{ > > > +public: > > > + IPAInterfaceWrapper(IPAInterface *interface); > > > + > > > +private: > > > + static void destroy(struct ipa_context *ctx); > > > + > > > + static const struct ipa_operations operations; > > > + > > > + IPAInterface *ipa; > > > +}; > > > + > > > +} /* namespace libcamera */ > > > + > > > +#endif /* __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ */ > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > > > new file mode 100644 > > > index 000000000000..ab3515ed2021 > > > --- /dev/null > > > +++ b/src/ipa/libipa/meson.build > > > @@ -0,0 +1,10 @@ > > > +libipa_headers = files([ > > > + 'ipa_interface_wrapper.h', > > > +]) > > > + > > > +libipa_sources = files([ > > > + 'ipa_interface_wrapper.cpp', > > > +]) > > > + > > > +libipa = static_library('libipa', libipa_sources, > > > + dependencies : libcamera_dep) > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > > > index f09915bc1388..6483ea66a478 100644 > > > --- a/src/ipa/meson.build > > > +++ b/src/ipa/meson.build > > > @@ -1,3 +1,5 @@ > > > +subdir('libipa') > > > + > > > ipa_dummy_sources = [ > > > ['ipa_dummy', 'LGPL-2.1-or-later'], > > > ['ipa_dummy_isolate', 'Proprietary'], > > > @@ -9,6 +11,7 @@ foreach t : ipa_dummy_sources > > > ipa = shared_module(t[0], 'ipa_dummy.cpp', > > > name_prefix : '', > > > include_directories : libcamera_includes, > > > + link_with : libipa, > > > install : true, > > > install_dir : ipa_install_dir, > > > cpp_args : '-DLICENSE="' + t[1] + '"') > > > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h > > > new file mode 100644 > > > index 000000000000..12894ac6885e > > > --- /dev/null > > > +++ b/src/libcamera/include/ipa_context_wrapper.h > > > @@ -0,0 +1,26 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Google Inc. > > > + * > > > + * ipa_context_wrapper.h - Image Processing Algorithm context wrapper > > > + */ > > > +#ifndef __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ > > > +#define __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ > > > + > > > +#include <ipa/ipa_interface.h> > > > + > > > +namespace libcamera { > > > + > > > +class IPAContextWrapper final : public IPAInterface > > > +{ > > > +public: > > > + IPAContextWrapper(struct ipa_context *context); > > > + ~IPAContextWrapper(); > > > + > > > +private: > > > + struct ipa_context *ctx_; > > > +}; > > > + > > > +} /* namespace libcamera */ > > > + > > > +#endif /* __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ */ > > > diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h > > > index 97737587ab3a..2028b76a1913 100644 > > > --- a/src/libcamera/include/ipa_module.h > > > +++ b/src/libcamera/include/ipa_module.h > > > @@ -7,7 +7,6 @@ > > > #ifndef __LIBCAMERA_IPA_MODULE_H__ > > > #define __LIBCAMERA_IPA_MODULE_H__ > > > > > > -#include <memory> > > > #include <string> > > > > > > #include <ipa/ipa_interface.h> > > > @@ -30,7 +29,7 @@ public: > > > > > > bool load(); > > > > > > - std::unique_ptr<IPAInterface> createInstance(); > > > + struct ipa_context *createContext(); > > > > > > bool match(PipelineHandler *pipe, > > > uint32_t minVersion, uint32_t maxVersion) const; > > > @@ -45,7 +44,7 @@ private: > > > bool loaded_; > > > > > > void *dlHandle_; > > > - typedef IPAInterface *(*IPAIntfFactory)(void); > > > + typedef struct ipa_context *(*IPAIntfFactory)(void); > > > IPAIntfFactory ipaCreate_; > > > > > > int loadIPAModuleInfo(); > > > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build > > > index 933be8543a8d..0abb978a389a 100644 > > > --- a/src/libcamera/include/meson.build > > > +++ b/src/libcamera/include/meson.build > > > @@ -5,6 +5,7 @@ libcamera_headers = files([ > > > 'device_enumerator_udev.h', > > > 'event_dispatcher_poll.h', > > > 'formats.h', > > > + 'ipa_context_wrapper.h', > > > 'ipa_manager.h', > > > 'ipa_module.h', > > > 'ipa_proxy.h', > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp > > > new file mode 100644 > > > index 000000000000..87ff98d45c99 > > > --- /dev/null > > > +++ b/src/libcamera/ipa_context_wrapper.cpp > > > @@ -0,0 +1,52 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Google Inc. > > > + * > > > + * ipa_context_wrapper.cpp - Image Processing Algorithm context wrapper > > > + */ > > > + > > > +#include "ipa_context_wrapper.h" > > > + > > > +#include <libcamera/controls.h> > > > + > > > +/** > > > + * \file ipa_context_wrapper.h > > > + * \brief Image Processing Algorithm context wrapper > > > + */ > > > + > > > +namespace libcamera { > > > + > > > +/** > > > + * \class IPAContextWrapper > > > + * \brief Wrap an ipa_context and expose it as an IPAInterface > > > + * > > > + * The IPAContextWrapper class wraps an ipa_context, provided by an IPA module, and > > > + * exposes an IPAInterface. This mechanism is used for IPAs that are not > > > + * isolated in a separate process to allow direct calls from pipeline handler > > > + * using the IPAInterface API instead of the lower-level ipa_context API. > > > + * > > > + * The IPAInterface methods are converted to the ipa_context API by serialising > > > + * all C++ arguments into plain C structures or byte arrays that contain no > > > + * pointer, as required by the ipa_context API. > > > + */ > > > + > > > +/** > > > + * \brief Construct an IPAContextWrapper instance that wraps the \a context > > > + * \param[in] context The IPA module context > > > + * > > > + * Ownership of the \a context is passed to the IPAContextWrapper. The context remains > > > + * valid for the whole lifetime of the wrapper and is destroyed automatically > > > + * with it. > > > + */ > > > +IPAContextWrapper::IPAContextWrapper(struct ipa_context *context) > > > + : ctx_(context) > > > +{ > > > +} > > > + > > > +IPAContextWrapper::~IPAContextWrapper() > > > +{ > > > + if (ctx_) > > > + ctx_->ops->destroy(ctx_); > > > +} > > > + > > > +} /* namespace libcamera */ > > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > > > index f70d91ded1ab..6e83aab0fb73 100644 > > > --- a/src/libcamera/ipa_interface.cpp > > > +++ b/src/libcamera/ipa_interface.cpp > > > @@ -10,13 +10,88 @@ > > > /** > > > * \file ipa_interface.h > > > * \brief Image Processing Algorithm interface > > > + * > > > + * libcamera interfaces with IPA modules through a plain C interface. IPA > > > + * modules shall expose a public function name ipaCreate() with the following > > > + * prototype. > > > + * > > > + * \code{.c} > > > + * struct ipa_context *ipaCreate(); > > > + * \endcode > > > + * > > > + * The ipaCreate() function creates an instance of an IPA context, which models > > > + * a context of execution for the IPA. IPA modules shall support creating one > > > + * or multiple contexts, as required by their associated pipeline handler. > > > + * > > > + * The IPA module operations are defined in the struct ipa_operations. An IPA > > > + * module stores a pointer to the operations corresponding to its context in > > > + * the ipa_context::ops field. That pointer is immutable for the lifetime of > > > + * the context, and may be different for multiple contexts created by the same > > > + * IPA module. > > > + * > > > + * All argument to ipa_operations members are Plain Old Data and are documented > > > + * either in the form of C data types, or as a textual description for byte > > > + * arrays that can't be expressed using C data types (such as variable-length > > > + * arrays). IPA modules can thus use the C API without calling into libcamera > > > + * to access the data passed to the IPA operations. > > > + * > > > + * The IPAInterface class is a C++ representation of the ipa_operations, using > > > + * C++ data classes provided by libcamera. This is the API exposed to pipeline > > > + * handlers to communicate with IPA modules. IPA modules may use the > > > + * IPAInterface API internally if they want to benefit from the data and helper > > > + * classes offered by libcamera. > > > + */ > > > + > > > +/** > > > + * \struct ipa_context > > > + * \brief IPA module context of execution > > > + * > > > + * This structure models a context of execution for an IPA module. It is > > > + * instantiated by the IPA module ipaCreate() function. IPA modules allocate > > > + * context instances in an implementation-defined way, contexts shall thus be > > > + * destroyed using the ipa_operation::destroy operation only. > > > + * > > > + * The ipa_context structure provides a pointer to the IPA operations. It shall > > > + * otherwise be treated as a constant black-box cookie and passed unmodified to > > > + * the operations defined in struct ipa_operations. > > > + * > > > + * IPA modules are expected to extend struct ipa_context by inheriting from it, > > > + * either through structure embedding to model inheritance in plain C, or > > > + * through C++ class inheritance. A simple example of the latter is available > > > + * in the IPAContextWrapper class implementation. > > > + * > > > + * \var ipa_context::ops > > > + * \brief The IPA context operations > > > + */ > > > + > > > +/** > > > + * \struct ipa_operations > > > + * \brief IPA context operations as a set of function pointers > > > + * > > > + * To allow for isolation of IPA modules in separate processes, the functions > > > + * defined in the ipa_operations structure return only data related to the > > > + * libcamera side of the operations. In particular, error related to the > > > + * libcamera side of the IPC may be returned. Data returned by the IPA, > > > + * including status information, shall be provided through callbacks from the > > > + * IPA to libcamera. > > > + * > > > + * \var ipa_operations::destroy > > > + * \brief Destroy the ipa_context created by the module's ipaCreate() function > > > */ > > > > > > namespace libcamera { > > > > > > /** > > > * \class IPAInterface > > > - * \brief Interface for IPA implementation > > > + * \brief IPA module interface > > > + * > > > + * This pure virtual class defines a C++ API corresponding to the ipa_context > > > + * and ipa_operations API. It is used by pipeline handlers to interact with IPA > > > + * modules, and may be used internally in IPA modules if desired to benefit > > > + * from the data and helper classes provided by libcamera. > > > + * > > > + * As for the operations defined in struct ipa_operations, the methods defined > > > + * by this class shall not return data from the IPA. > > > */ > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > > index 4276d995bdd5..0a908eae6261 100644 > > > --- a/src/libcamera/ipa_manager.cpp > > > +++ b/src/libcamera/ipa_manager.cpp > > > @@ -11,6 +11,7 @@ > > > #include <string.h> > > > #include <sys/types.h> > > > > > > +#include "ipa_context_wrapper.h" > > > #include "ipa_module.h" > > > #include "ipa_proxy.h" > > > #include "log.h" > > > @@ -29,6 +30,66 @@ LOG_DEFINE_CATEGORY(IPAManager) > > > /** > > > * \class IPAManager > > > * \brief Manager for IPA modules > > > + * > > > + * The IPA module manager discovers IPA modules from disk, queries and loads > > > + * them, and creates IPA contexts. It supports isolation of the modules in a > > > + * separate process with IPC communication and offers a unified IPAInterface > > > + * view of the IPA contexts to pipeline handlers regardless of whether the > > > + * modules are isolated or loaded in the same process. > > > + * > > > + * Module isolation is based on the module licence. Open-source modules are > > > + * loaded without isolation, while closed-source module are forcefully isolated. > > > + * The isolation mechanism ensures that no code from a closed-source module is > > > + * ever run in the libcamera process. > > > + * > > > + * To create an IPA context, pipeline handlers call the IPAManager::ipaCreate() > > > + * method. For a directly loaded module, the manager calls the module's > > > + * ipaCreate() function directly and wraps the returned context in an > > > + * IPAContextWrapper that exposes an IPAInterface. > > > + * > > > + * ~~~~ > > > + * +---------------+ > > > + * | Pipeline | > > > + * | Handler | > > > + * +---------------+ > > > + * | > > > + * v > > > + * +---------------+ +---------------+ > > > + * | IPA | | Open Source | > > > + * | Interface | | IPA Module | > > > + * | - - - - - - - | | - - - - - - - | > > > + * | IPA Context | ipa_operations | ipa_context | > > > + * | Wrapper | ----------------> | | > > > + * +---------------+ +---------------+ > > > + * ~~~~ > > > + * > > > + * For an isolated module, the manager instantiates an IPAProxy which spawns a > > > + * new process for an IPA proxy worker. The worker loads the IPA module and > > > + * creates the IPA context. The IPAProxy alse exposes an IPAInterface. > > > + * > > > + * ~~~~ > > > + * +---------------+ +---------------+ > > > + * | Pipeline | | Closed Source | > > > + * | Handler | | IPA Module | > > > + * +---------------+ | - - - - - - - | > > > + * | | ipa_context | > > > + * v | | > > > + * +---------------+ +---------------+ > > > + * | IPA | ipa_operations ^ > > > + * | Interface | | > > > + * | - - - - - - - | +---------------+ > > > + * | IPA Proxy | operations | IPA Proxy | > > > + * | | ----------------> | Worker | > > > + * +---------------+ over IPC +---------------+ > > > + * ~~~~ > > > + * > > > + * The IPAInterface implemented by the IPAContextWrapper or IPAProxy is > > > + * returned to the pipeline handler, and all interactions with the IPA context > > > + * go the same interface regarless of process isolation. > > > + * > > > + * In all cases the data passed to the IPAInterface methods is serialised to > > > + * Plain Old Data, either for the purpose of passing it to the IPA context > > > + * plain C API, or to transmit the data to the isolated process through IPC. > > > */ > > > > > > IPAManager::IPAManager() > > > @@ -177,7 +238,11 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe, > > > if (!m->load()) > > > return nullptr; > > > > > > - return m->createInstance(); > > > + struct ipa_context *ctx = m->createContext(); > > > + if (!ctx) > > > + return nullptr; > > > + > > > + return utils::make_unique<IPAContextWrapper>(ctx); > > > } > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > > > index 99d308efd47b..9f5a01418f73 100644 > > > --- a/src/libcamera/ipa_module.cpp > > > +++ b/src/libcamera/ipa_module.cpp > > > @@ -385,13 +385,13 @@ const std::string &IPAModule::path() const > > > /** > > > * \brief Load the IPA implementation factory from the shared object > > > * > > > - * The IPA module shared object implements an IPAInterface class to be used > > > + * The IPA module shared object implements an ipa_context object to be used > > > * by pipeline handlers. This method loads the factory function from the > > > - * shared object. Later, createInstance() can be called to instantiate the > > > - * IPAInterface. > > > + * shared object. Later, createContext() can be called to instantiate the > > > + * ipa_context. > > > * > > > * This method only needs to be called successfully once, after which > > > - * createInstance() can be called as many times as IPAInterface instances are > > > + * createContext() can be called as many times as ipa_context instances are > > > * needed. > > > * > > > * Calling this function on an invalid module (as returned by isValid()) is > > > @@ -433,24 +433,25 @@ bool IPAModule::load() > > > } > > > > > > /** > > > - * \brief Instantiate an IPAInterface > > > + * \brief Instantiate an IPA context > > > * > > > - * After loading the IPA module with load(), this method creates an > > > - * instance of the IPA module interface. > > > + * After loading the IPA module with load(), this method creates an instance of > > > + * the IPA module context. Ownership of the context is passed to the caller, and > > > + * the context shall be destroyed by calling the \ref ipa_operations::destroy > > > + * "ipa_context::ops::destroy()" operation. > > > * > > > * Calling this function on a module that has not yet been loaded, or an > > > * invalid module (as returned by load() and isValid(), respectively) is > > > * an error. > > > * > > > - * \return The IPA implementation as a new IPAInterface instance on success, > > > - * or nullptr on error > > > + * \return The IPA context on success, or nullptr on error > > > */ > > > -std::unique_ptr<IPAInterface> IPAModule::createInstance() > > > +struct ipa_context *IPAModule::createContext() > > > { > > > if (!valid_ || !loaded_) > > > return nullptr; > > > > > > - return std::unique_ptr<IPAInterface>(ipaCreate_()); > > > + return ipaCreate_(); > > > } > > > > > > /** > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index 755149c55c7b..d3ae305e5655 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -12,6 +12,7 @@ libcamera_sources = files([ > > > 'event_notifier.cpp', > > > 'formats.cpp', > > > 'geometry.cpp', > > > + 'ipa_context_wrapper.cpp', > > > 'ipa_interface.cpp', > > > 'ipa_manager.cpp', > > > 'ipa_module.cpp', > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > > index a401b1099f3c..612edcda69d6 100644 > > > --- a/src/libcamera/pipeline/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc.cpp > > > @@ -362,7 +362,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > > > return false; > > > > > > ipa_ = IPAManager::instance()->createIPA(this, 0, 0); > > > - if (ipa_ == nullptr) > > > + if (!ipa_) > > > LOG(VIMC, Warning) << "no matching IPA found"; > > > > > > std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this); > > > diff --git a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > > > index a10761e52b32..07380c16e2d5 100644 > > > --- a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > > > +++ b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp > > > @@ -72,9 +72,9 @@ int main(int argc, char **argv) > > > } > > > socket.readyRead.connect(&readyRead); > > > > > > - std::unique_ptr<IPAInterface> ipa = ipam->createInstance(); > > > - if (!ipa) { > > > - LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA interface"; > > > + struct ipa_context *ipac = ipam->createContext(); > > > + if (!ipac) { > > > + LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA context"; > > > return EXIT_FAILURE; > > > } > > > > > > @@ -85,5 +85,7 @@ int main(int argc, char **argv) > > > while (1) > > > dispatcher->processEvents(); > > > > > > + ipac->ops->destroy(ipac); > > > + > > > return 0; > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in index ecc058eec683..2ab93687755f 100644 --- a/Documentation/Doxyfile.in +++ b/Documentation/Doxyfile.in @@ -793,6 +793,7 @@ WARN_LOGFILE = INPUT = "@TOP_SRCDIR@/include/ipa" \ "@TOP_SRCDIR@/include/libcamera" \ + "@TOP_SRCDIR@/src/ipa/libipa" \ "@TOP_SRCDIR@/src/libcamera" # This tag can be used to specify the character encoding of the source files diff --git a/Documentation/meson.build b/Documentation/meson.build index 4ff3fbeb0674..9136506f5d9c 100644 --- a/Documentation/meson.build +++ b/Documentation/meson.build @@ -24,6 +24,8 @@ if doxygen.found() libcamera_ipa_api, libcamera_headers, libcamera_sources, + libipa_headers, + libipa_sources, ], output : 'api-html', command : [doxygen, doxyfile], diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h index 9bbc4cf58ec6..f1ebac20f151 100644 --- a/include/ipa/ipa_interface.h +++ b/include/ipa/ipa_interface.h @@ -7,6 +7,21 @@ #ifndef __LIBCAMERA_IPA_INTERFACE_H__ #define __LIBCAMERA_IPA_INTERFACE_H__ +#ifdef __cplusplus +extern "C" { +#endif + +struct ipa_context { + const struct ipa_operations *ops; +}; + +struct ipa_operations { + void (*destroy)(struct ipa_context *ctx); +}; + +#ifdef __cplusplus +} + namespace libcamera { class IPAInterface @@ -16,5 +31,6 @@ public: }; } /* namespace libcamera */ +#endif #endif /* __LIBCAMERA_IPA_INTERFACE_H__ */ diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp index c833e5fb0b2d..6dc9448a3f56 100644 --- a/src/ipa/ipa_dummy.cpp +++ b/src/ipa/ipa_dummy.cpp @@ -8,6 +8,8 @@ #include <ipa/ipa_interface.h> #include <ipa/ipa_module_info.h> +#include "libipa/ipa_interface_wrapper.h" + namespace libcamera { class IPADummy : public IPAInterface @@ -27,9 +29,9 @@ const struct IPAModuleInfo ipaModuleInfo = { LICENSE, }; -IPAInterface *ipaCreate() +struct ipa_context *ipaCreate() { - return new IPADummy(); + return new IPAInterfaceWrapper(new IPADummy()); } }; diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp new file mode 100644 index 000000000000..aacd189851c3 --- /dev/null +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * ipa_interface_wrapper.cpp - Image Processing Algorithm interface wrapper + */ + +#include "ipa_interface_wrapper.h" + +#include <ipa/ipa_interface.h> + +/** + * \file ipa_interface_wrapper.h + * \brief Image Processing Algorithm interface wrapper + */ + +namespace libcamera { + +/** + * \class IPAInterfaceWrapper + * \brief Wrap an IPAInterface and expose it as an ipa_context + * + * This class implements the ipa_context API based on a provided IPAInterface. + * It helps IPAs that implement the IPAInterface API to provide the external + * ipa_context API. + * + * To use the wrapper, an IPA module simple creates a new instance of its + * IPAInterface implementation, and passes it to the constructor of the + * IPAInterfaceWrapper. As IPAInterfaceWrapper inherits from ipa_context, the + * constructed wrapper can then be directly returned from the IPA module's + * ipaCreate() function. + * + * \code{.cpp} + * class MyIPA : public IPAInterface + * { + * ... + * }; + * + * struct ipa_context *ipaCreate() + * { + * return new IPAInterfaceWrapper(new MyIPA()); + * } + * \endcode + */ + +/** + * \brief Construct an IPAInterfaceWrapper wrapping \a interface + * \param[in] interface The interface to wrap + */ +IPAInterfaceWrapper::IPAInterfaceWrapper(IPAInterface *interface) + : ipa(interface) +{ + ops = &operations; +} + +void IPAInterfaceWrapper::destroy(struct ipa_context *_ctx) +{ + IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx); + + delete ctx->ipa; + delete ctx; +} + +#ifndef __DOXYGEN__ +/* + * This construct confuses Doygen and makes it believe that all members of the + * operations is a member of IPAInterfaceWrapper. It must thus be hidden. + */ +const struct ipa_operations IPAInterfaceWrapper::operations = { + .destroy = &IPAInterfaceWrapper::destroy, +}; +#endif + +}; /* namespace libcamera */ diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h new file mode 100644 index 000000000000..d2ab46f50d3c --- /dev/null +++ b/src/ipa/libipa/ipa_interface_wrapper.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * ipa_interface_wrapper.h - Image Processing Algorithm interface wrapper + */ +#ifndef __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ +#define __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ + +#include <ipa/ipa_interface.h> + +namespace libcamera { + +class IPAInterfaceWrapper : public ipa_context +{ +public: + IPAInterfaceWrapper(IPAInterface *interface); + +private: + static void destroy(struct ipa_context *ctx); + + static const struct ipa_operations operations; + + IPAInterface *ipa; +}; + +} /* namespace libcamera */ + +#endif /* __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ */ diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build new file mode 100644 index 000000000000..ab3515ed2021 --- /dev/null +++ b/src/ipa/libipa/meson.build @@ -0,0 +1,10 @@ +libipa_headers = files([ + 'ipa_interface_wrapper.h', +]) + +libipa_sources = files([ + 'ipa_interface_wrapper.cpp', +]) + +libipa = static_library('libipa', libipa_sources, + dependencies : libcamera_dep) diff --git a/src/ipa/meson.build b/src/ipa/meson.build index f09915bc1388..6483ea66a478 100644 --- a/src/ipa/meson.build +++ b/src/ipa/meson.build @@ -1,3 +1,5 @@ +subdir('libipa') + ipa_dummy_sources = [ ['ipa_dummy', 'LGPL-2.1-or-later'], ['ipa_dummy_isolate', 'Proprietary'], @@ -9,6 +11,7 @@ foreach t : ipa_dummy_sources ipa = shared_module(t[0], 'ipa_dummy.cpp', name_prefix : '', include_directories : libcamera_includes, + link_with : libipa, install : true, install_dir : ipa_install_dir, cpp_args : '-DLICENSE="' + t[1] + '"') diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h new file mode 100644 index 000000000000..12894ac6885e --- /dev/null +++ b/src/libcamera/include/ipa_context_wrapper.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * ipa_context_wrapper.h - Image Processing Algorithm context wrapper + */ +#ifndef __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ +#define __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ + +#include <ipa/ipa_interface.h> + +namespace libcamera { + +class IPAContextWrapper final : public IPAInterface +{ +public: + IPAContextWrapper(struct ipa_context *context); + ~IPAContextWrapper(); + +private: + struct ipa_context *ctx_; +}; + +} /* namespace libcamera */ + +#endif /* __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ */ diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h index 97737587ab3a..2028b76a1913 100644 --- a/src/libcamera/include/ipa_module.h +++ b/src/libcamera/include/ipa_module.h @@ -7,7 +7,6 @@ #ifndef __LIBCAMERA_IPA_MODULE_H__ #define __LIBCAMERA_IPA_MODULE_H__ -#include <memory> #include <string> #include <ipa/ipa_interface.h> @@ -30,7 +29,7 @@ public: bool load(); - std::unique_ptr<IPAInterface> createInstance(); + struct ipa_context *createContext(); bool match(PipelineHandler *pipe, uint32_t minVersion, uint32_t maxVersion) const; @@ -45,7 +44,7 @@ private: bool loaded_; void *dlHandle_; - typedef IPAInterface *(*IPAIntfFactory)(void); + typedef struct ipa_context *(*IPAIntfFactory)(void); IPAIntfFactory ipaCreate_; int loadIPAModuleInfo(); diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build index 933be8543a8d..0abb978a389a 100644 --- a/src/libcamera/include/meson.build +++ b/src/libcamera/include/meson.build @@ -5,6 +5,7 @@ libcamera_headers = files([ 'device_enumerator_udev.h', 'event_dispatcher_poll.h', 'formats.h', + 'ipa_context_wrapper.h', 'ipa_manager.h', 'ipa_module.h', 'ipa_proxy.h', diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp new file mode 100644 index 000000000000..87ff98d45c99 --- /dev/null +++ b/src/libcamera/ipa_context_wrapper.cpp @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * ipa_context_wrapper.cpp - Image Processing Algorithm context wrapper + */ + +#include "ipa_context_wrapper.h" + +#include <libcamera/controls.h> + +/** + * \file ipa_context_wrapper.h + * \brief Image Processing Algorithm context wrapper + */ + +namespace libcamera { + +/** + * \class IPAContextWrapper + * \brief Wrap an ipa_context and expose it as an IPAInterface + * + * The IPAContextWrapper class wraps an ipa_context, provided by an IPA module, and + * exposes an IPAInterface. This mechanism is used for IPAs that are not + * isolated in a separate process to allow direct calls from pipeline handler + * using the IPAInterface API instead of the lower-level ipa_context API. + * + * The IPAInterface methods are converted to the ipa_context API by serialising + * all C++ arguments into plain C structures or byte arrays that contain no + * pointer, as required by the ipa_context API. + */ + +/** + * \brief Construct an IPAContextWrapper instance that wraps the \a context + * \param[in] context The IPA module context + * + * Ownership of the \a context is passed to the IPAContextWrapper. The context remains + * valid for the whole lifetime of the wrapper and is destroyed automatically + * with it. + */ +IPAContextWrapper::IPAContextWrapper(struct ipa_context *context) + : ctx_(context) +{ +} + +IPAContextWrapper::~IPAContextWrapper() +{ + if (ctx_) + ctx_->ops->destroy(ctx_); +} + +} /* namespace libcamera */ diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp index f70d91ded1ab..6e83aab0fb73 100644 --- a/src/libcamera/ipa_interface.cpp +++ b/src/libcamera/ipa_interface.cpp @@ -10,13 +10,88 @@ /** * \file ipa_interface.h * \brief Image Processing Algorithm interface + * + * libcamera interfaces with IPA modules through a plain C interface. IPA + * modules shall expose a public function name ipaCreate() with the following + * prototype. + * + * \code{.c} + * struct ipa_context *ipaCreate(); + * \endcode + * + * The ipaCreate() function creates an instance of an IPA context, which models + * a context of execution for the IPA. IPA modules shall support creating one + * or multiple contexts, as required by their associated pipeline handler. + * + * The IPA module operations are defined in the struct ipa_operations. An IPA + * module stores a pointer to the operations corresponding to its context in + * the ipa_context::ops field. That pointer is immutable for the lifetime of + * the context, and may be different for multiple contexts created by the same + * IPA module. + * + * All argument to ipa_operations members are Plain Old Data and are documented + * either in the form of C data types, or as a textual description for byte + * arrays that can't be expressed using C data types (such as variable-length + * arrays). IPA modules can thus use the C API without calling into libcamera + * to access the data passed to the IPA operations. + * + * The IPAInterface class is a C++ representation of the ipa_operations, using + * C++ data classes provided by libcamera. This is the API exposed to pipeline + * handlers to communicate with IPA modules. IPA modules may use the + * IPAInterface API internally if they want to benefit from the data and helper + * classes offered by libcamera. + */ + +/** + * \struct ipa_context + * \brief IPA module context of execution + * + * This structure models a context of execution for an IPA module. It is + * instantiated by the IPA module ipaCreate() function. IPA modules allocate + * context instances in an implementation-defined way, contexts shall thus be + * destroyed using the ipa_operation::destroy operation only. + * + * The ipa_context structure provides a pointer to the IPA operations. It shall + * otherwise be treated as a constant black-box cookie and passed unmodified to + * the operations defined in struct ipa_operations. + * + * IPA modules are expected to extend struct ipa_context by inheriting from it, + * either through structure embedding to model inheritance in plain C, or + * through C++ class inheritance. A simple example of the latter is available + * in the IPAContextWrapper class implementation. + * + * \var ipa_context::ops + * \brief The IPA context operations + */ + +/** + * \struct ipa_operations + * \brief IPA context operations as a set of function pointers + * + * To allow for isolation of IPA modules in separate processes, the functions + * defined in the ipa_operations structure return only data related to the + * libcamera side of the operations. In particular, error related to the + * libcamera side of the IPC may be returned. Data returned by the IPA, + * including status information, shall be provided through callbacks from the + * IPA to libcamera. + * + * \var ipa_operations::destroy + * \brief Destroy the ipa_context created by the module's ipaCreate() function */ namespace libcamera { /** * \class IPAInterface - * \brief Interface for IPA implementation + * \brief IPA module interface + * + * This pure virtual class defines a C++ API corresponding to the ipa_context + * and ipa_operations API. It is used by pipeline handlers to interact with IPA + * modules, and may be used internally in IPA modules if desired to benefit + * from the data and helper classes provided by libcamera. + * + * As for the operations defined in struct ipa_operations, the methods defined + * by this class shall not return data from the IPA. */ } /* namespace libcamera */ diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index 4276d995bdd5..0a908eae6261 100644 --- a/src/libcamera/ipa_manager.cpp +++ b/src/libcamera/ipa_manager.cpp @@ -11,6 +11,7 @@ #include <string.h> #include <sys/types.h> +#include "ipa_context_wrapper.h" #include "ipa_module.h" #include "ipa_proxy.h" #include "log.h" @@ -29,6 +30,66 @@ LOG_DEFINE_CATEGORY(IPAManager) /** * \class IPAManager * \brief Manager for IPA modules + * + * The IPA module manager discovers IPA modules from disk, queries and loads + * them, and creates IPA contexts. It supports isolation of the modules in a + * separate process with IPC communication and offers a unified IPAInterface + * view of the IPA contexts to pipeline handlers regardless of whether the + * modules are isolated or loaded in the same process. + * + * Module isolation is based on the module licence. Open-source modules are + * loaded without isolation, while closed-source module are forcefully isolated. + * The isolation mechanism ensures that no code from a closed-source module is + * ever run in the libcamera process. + * + * To create an IPA context, pipeline handlers call the IPAManager::ipaCreate() + * method. For a directly loaded module, the manager calls the module's + * ipaCreate() function directly and wraps the returned context in an + * IPAContextWrapper that exposes an IPAInterface. + * + * ~~~~ + * +---------------+ + * | Pipeline | + * | Handler | + * +---------------+ + * | + * v + * +---------------+ +---------------+ + * | IPA | | Open Source | + * | Interface | | IPA Module | + * | - - - - - - - | | - - - - - - - | + * | IPA Context | ipa_operations | ipa_context | + * | Wrapper | ----------------> | | + * +---------------+ +---------------+ + * ~~~~ + * + * For an isolated module, the manager instantiates an IPAProxy which spawns a + * new process for an IPA proxy worker. The worker loads the IPA module and + * creates the IPA context. The IPAProxy alse exposes an IPAInterface. + * + * ~~~~ + * +---------------+ +---------------+ + * | Pipeline | | Closed Source | + * | Handler | | IPA Module | + * +---------------+ | - - - - - - - | + * | | ipa_context | + * v | | + * +---------------+ +---------------+ + * | IPA | ipa_operations ^ + * | Interface | | + * | - - - - - - - | +---------------+ + * | IPA Proxy | operations | IPA Proxy | + * | | ----------------> | Worker | + * +---------------+ over IPC +---------------+ + * ~~~~ + * + * The IPAInterface implemented by the IPAContextWrapper or IPAProxy is + * returned to the pipeline handler, and all interactions with the IPA context + * go the same interface regarless of process isolation. + * + * In all cases the data passed to the IPAInterface methods is serialised to + * Plain Old Data, either for the purpose of passing it to the IPA context + * plain C API, or to transmit the data to the isolated process through IPC. */ IPAManager::IPAManager() @@ -177,7 +238,11 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe, if (!m->load()) return nullptr; - return m->createInstance(); + struct ipa_context *ctx = m->createContext(); + if (!ctx) + return nullptr; + + return utils::make_unique<IPAContextWrapper>(ctx); } } /* namespace libcamera */ diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp index 99d308efd47b..9f5a01418f73 100644 --- a/src/libcamera/ipa_module.cpp +++ b/src/libcamera/ipa_module.cpp @@ -385,13 +385,13 @@ const std::string &IPAModule::path() const /** * \brief Load the IPA implementation factory from the shared object * - * The IPA module shared object implements an IPAInterface class to be used + * The IPA module shared object implements an ipa_context object to be used * by pipeline handlers. This method loads the factory function from the - * shared object. Later, createInstance() can be called to instantiate the - * IPAInterface. + * shared object. Later, createContext() can be called to instantiate the + * ipa_context. * * This method only needs to be called successfully once, after which - * createInstance() can be called as many times as IPAInterface instances are + * createContext() can be called as many times as ipa_context instances are * needed. * * Calling this function on an invalid module (as returned by isValid()) is @@ -433,24 +433,25 @@ bool IPAModule::load() } /** - * \brief Instantiate an IPAInterface + * \brief Instantiate an IPA context * - * After loading the IPA module with load(), this method creates an - * instance of the IPA module interface. + * After loading the IPA module with load(), this method creates an instance of + * the IPA module context. Ownership of the context is passed to the caller, and + * the context shall be destroyed by calling the \ref ipa_operations::destroy + * "ipa_context::ops::destroy()" operation. * * Calling this function on a module that has not yet been loaded, or an * invalid module (as returned by load() and isValid(), respectively) is * an error. * - * \return The IPA implementation as a new IPAInterface instance on success, - * or nullptr on error + * \return The IPA context on success, or nullptr on error */ -std::unique_ptr<IPAInterface> IPAModule::createInstance() +struct ipa_context *IPAModule::createContext() { if (!valid_ || !loaded_) return nullptr; - return std::unique_ptr<IPAInterface>(ipaCreate_()); + return ipaCreate_(); } /** diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 755149c55c7b..d3ae305e5655 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -12,6 +12,7 @@ libcamera_sources = files([ 'event_notifier.cpp', 'formats.cpp', 'geometry.cpp', + 'ipa_context_wrapper.cpp', 'ipa_interface.cpp', 'ipa_manager.cpp', 'ipa_module.cpp', diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index a401b1099f3c..612edcda69d6 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -362,7 +362,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) return false; ipa_ = IPAManager::instance()->createIPA(this, 0, 0); - if (ipa_ == nullptr) + if (!ipa_) LOG(VIMC, Warning) << "no matching IPA found"; std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this); diff --git a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp index a10761e52b32..07380c16e2d5 100644 --- a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp +++ b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp @@ -72,9 +72,9 @@ int main(int argc, char **argv) } socket.readyRead.connect(&readyRead); - std::unique_ptr<IPAInterface> ipa = ipam->createInstance(); - if (!ipa) { - LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA interface"; + struct ipa_context *ipac = ipam->createContext(); + if (!ipac) { + LOG(IPAProxyLinuxWorker, Error) << "Failed to create IPA context"; return EXIT_FAILURE; } @@ -85,5 +85,7 @@ int main(int argc, char **argv) while (1) dispatcher->processEvents(); + ipac->ops->destroy(ipac); + return 0; }