[{"id":3090,"web_url":"https://patchwork.libcamera.org/comment/3090/","msgid":"<20191118225851.GO8072@bigcity.dyn.berto.se>","date":"2019-11-18T22:58:51","subject":"Re: [libcamera-devel] [PATCH v2 20/24] ipa: Switch to the plain C\n\tAPI","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-11-08 22:54:05 +0200, Laurent Pinchart wrote:\n> From: Jacopo Mondi \n> \n> Switch IPA communication to the plain C API. As the IPAInterface class\n> is easier to use for pipeline handlers than a plain C API, retain it and\n> add an IPAContextWrapper that translate between the C++ and the C APIs.\n> \n> On the IPA module side usage of IPAInterface may be desired for IPAs\n> implemented in C++ that want to link to libcamera. For those IPAs, a new\n> IPAInterfaceWrapper helper class is introduced to wrap the IPAInterface\n> implemented internally by the IPA module into an ipa_context,\n> ipa_context_ops and ipa_callback_ops.\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Laurent Pinchart \n\nOne small nit bellow, other then that\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> Documentation/Doxyfile.in | 1 +\n> Documentation/meson.build | 2 +\n> src/ipa/ipa_vimc.cpp | 7 +-\n> src/ipa/libipa/ipa_interface_wrapper.cpp | 240 ++++++++++++++++++\n> src/ipa/libipa/ipa_interface_wrapper.h | 56 ++++\n> src/ipa/libipa/meson.build | 13 +\n> src/ipa/meson.build | 3 +\n> src/ipa/rkisp1/meson.build | 3 +-\n> src/ipa/rkisp1/rkisp1.cpp | 5 +-\n> src/libcamera/include/ipa_context_wrapper.h | 43 ++++\n> src/libcamera/include/ipa_module.h | 5 +-\n> src/libcamera/include/meson.build | 1 +\n> src/libcamera/ipa_context_wrapper.cpp | 219 ++++++++++++++++\n> src/libcamera/ipa_manager.cpp | 67 ++++-\n> src/libcamera/ipa_module.cpp | 23 +-\n> src/libcamera/meson.build | 1 +\n> .../proxy/worker/ipa_proxy_linux_worker.cpp | 8 +-\n> 17 files changed, 674 insertions(+), 23 deletions(-)\n> create mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp\n> create mode 100644 src/ipa/libipa/ipa_interface_wrapper.h\n> create mode 100644 src/ipa/libipa/meson.build\n> create mode 100644 src/libcamera/include/ipa_context_wrapper.h\n> create mode 100644 src/libcamera/ipa_context_wrapper.cpp\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 24babfd8b366..840c1b4c76c5 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -793,6 +793,7 @@ WARN_LOGFILE =\n> \n> INPUT = \"@TOP_SRCDIR@/include/ipa\" \\\n> \t\t\t \"@TOP_SRCDIR@/include/libcamera\" \\\n> +\t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n> \t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> index 4ff3fbeb0674..9136506f5d9c 100644\n> --- a/Documentation/meson.build\n> +++ b/Documentation/meson.build\n> @@ -24,6 +24,8 @@ if doxygen.found()\n> libcamera_ipa_api,\n> libcamera_headers,\n> libcamera_sources,\n> + libipa_headers,\n> + libipa_sources,\n> ],\n> output : 'api-html',\n> command : [doxygen, doxyfile],\n> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp\n> index 50ca8dd805fb..8f03e811acc7 100644\n> --- a/src/ipa/ipa_vimc.cpp\n> +++ b/src/ipa/ipa_vimc.cpp\n> @@ -17,7 +17,10 @@\n> #include \n> #include \n> \n> +#include \"libipa/ipa_interface_wrapper.h\"\n> +\n> #include \"log.h\"\n> +#include \"utils.h\"\n> \n> namespace libcamera {\n> \n> @@ -108,9 +111,9 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> \tLICENSE,\n> };\n> \n> -IPAInterface *ipaCreate()\n> +struct ipa_context *ipaCreate()\n> {\n> -\treturn new IPAVimc();\n> +\treturn new IPAInterfaceWrapper(utils::make_unique());\n> }\n> }\n> \n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> new file mode 100644\n> index 000000000000..80c5648ffed6\n> --- /dev/null\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -0,0 +1,240 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_interface_wrapper.cpp - Image Processing Algorithm interface wrapper\n> + */\n> +\n> +#include \"ipa_interface_wrapper.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"byte_stream_buffer.h\"\n> +\n> +/**\n> + * \\file ipa_interface_wrapper.h\n> + * \\brief Image Processing Algorithm interface wrapper\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class IPAInterfaceWrapper\n> + * \\brief Wrap an IPAInterface and expose it as an ipa_context\n> + *\n> + * This class implements the ipa_context API based on a provided IPAInterface.\n> + * It helps IPAs that implement the IPAInterface API to provide the external\n> + * ipa_context API.\n> + *\n> + * To use the wrapper, an IPA module simple creates a new instance of its\n> + * IPAInterface implementation, and passes it to the constructor of the\n> + * IPAInterfaceWrapper. As IPAInterfaceWrapper inherits from ipa_context, the\n> + * constructed wrapper can then be directly returned from the IPA module's\n> + * ipaCreate() function.\n> + *\n> + * \\code{.cpp}\n> + * class MyIPA : public IPAInterface\n> + * {\n> + * \t...\n> + * };\n> + *\n> + * struct ipa_context *ipaCreate()\n> + * {\n> + * \treturn new IPAInterfaceWrapper(utils::make_unique());\n> + * }\n> + * \\endcode\n> + *\n> + * The wrapper takes ownership of the IPAInterface and will automatically\n> + * delete it when the wrapper is destroyed.\n> + */\n> +\n> +/**\n> + * \\brief Construct an IPAInterfaceWrapper wrapping \\a interface\n> + * \\param[in] interface The interface to wrap\n> + */\n> +IPAInterfaceWrapper::IPAInterfaceWrapper(std::unique_ptr interface)\n> +\t: ipa_(std::move(interface)), callbacks_(nullptr), cb_ctx_(nullptr)\n> +{\n> +\tops = &operations_;\n> +\n> +\tipa_->queueFrameAction.connect(this, &IPAInterfaceWrapper::queueFrameAction);\n> +}\n> +\n> +void IPAInterfaceWrapper::destroy(struct ipa_context *_ctx)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> +\n> +\tdelete ctx;\n> +}\n> +\n> +void IPAInterfaceWrapper::init(struct ipa_context *_ctx)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> +\n> +\tctx->ipa_->init();\n> +}\n> +\n> +void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n> +\t\t\t\t\t const struct ipa_callback_ops *callbacks,\n> +\t\t\t\t\t void *cb_ctx)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> +\n> +\tctx->callbacks_ = callbacks;\n> +\tctx->cb_ctx_ = cb_ctx;\n> +}\n> +\n> +void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> +\t\t\t\t const struct ipa_stream *streams,\n> +\t\t\t\t unsigned int num_streams,\n> +\t\t\t\t const struct ipa_control_info_map *maps,\n> +\t\t\t\t unsigned int num_maps)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> +\n> +\tctx->serializer_.reset();\n> +\n> +\t/* Translate the IPA stream configurations map. */\n> +\tstd::map ipaStreams;\n> +\n> +\tfor (unsigned int i = 0; i < num_streams; ++i) {\n> +\t\tconst struct ipa_stream &stream = streams[i];\n> +\n> +\t\tipaStreams[stream.id] = {\n> +\t\t\tstream.pixel_format,\n> +\t\t\tSize(stream.width, stream.height),\n> +\t\t};\n> +\t}\n> +\n> +\t/* Translate the IPA entity controls map. */\n> +\tstd::map entityControls;\n> +\tstd::map infoMaps;\n> +\n> +\tfor (unsigned int i = 0; i < num_maps; ++i) {\n> +\t\tconst struct ipa_control_info_map &ipa_map = maps[i];\n> +\t\tByteStreamBuffer byteStream(ipa_map.data, ipa_map.size);\n> +\t\tunsigned int id = ipa_map.id;\n> +\n> +\t\tinfoMaps[id] = ctx->serializer_.deserialize(byteStream);\n> +\t\tentityControls.emplace(id, infoMaps[id]);\n> +\t}\n> +\n> +\tctx->ipa_->configure(ipaStreams, entityControls);\n> +}\n> +\n> +void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> +\t\t\t\t const struct ipa_buffer *_buffers,\n> +\t\t\t\t size_t num_buffers)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> +\tstd::vector buffers(num_buffers);\n> +\n> +\tfor (unsigned int i = 0; i < num_buffers; ++i) {\n> +\t\tconst struct ipa_buffer &_buffer = _buffers[i];\n> +\t\tIPABuffer &buffer = buffers[i];\n> +\t\tstd::vector &planes = buffer.memory.planes();\n> +\n> +\t\tbuffer.id = _buffer.id;\n> +\n> +\t\tplanes.resize(_buffer.num_planes);\n> +\t\tfor (unsigned int j = 0; j < _buffer.num_planes; ++j) {\n> +\t\t\tif (_buffer.planes[j].dmabuf != -1)\n> +\t\t\t\tplanes[j].setDmabuf(_buffer.planes[j].dmabuf,\n> +\t\t\t\t\t\t _buffer.planes[j].length);\n> +\t\t\t/** \\todo Create a Dmabuf class to implement RAII. */\n> +\t\t\t::close(_buffer.planes[j].dmabuf);\n> +\t\t}\n> +\t}\n> +\n> +\tctx->ipa_->mapBuffers(buffers);\n> +}\n> +\n> +void IPAInterfaceWrapper::unmap_buffers(struct ipa_context *_ctx,\n> +\t\t\t\t\tconst unsigned int *_ids,\n> +\t\t\t\t\tsize_t num_buffers)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> +\tstd::vector ids(_ids, _ids + num_buffers);\n> +\tctx->ipa_->unmapBuffers(ids);\n> +}\n> +\n> +void IPAInterfaceWrapper::process_event(struct ipa_context *_ctx,\n> +\t\t\t\t\tconst struct ipa_operation_data *data)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> +\tIPAOperationData opData;\n> +\n> +\topData.operation = data->operation;\n> +\n> +\topData.data.resize(data->num_data);\n> +\tmemcpy(opData.data.data(), data->data,\n> +\t data->num_data * sizeof(*data->data));\n> +\n> +\topData.controls.resize(data->num_lists);\n> +\tfor (unsigned int i = 0; i < data->num_lists; ++i) {\n> +\t\tconst struct ipa_control_list *c_list = &data->lists[i];\n> +\t\tByteStreamBuffer byteStream(c_list->data, c_list->size);\n> +\t\topData.controls[i] = ctx->serializer_.deserialize(byteStream);\n> +\t}\n> +\n> +\tctx->ipa_->processEvent(opData);\n> +}\n> +\n> +void IPAInterfaceWrapper::queueFrameAction(unsigned int frame,\n> +\t\t\t\t\t const IPAOperationData &data)\n> +{\n> +\tif (!callbacks_)\n> +\t\treturn;\n> +\n> +\tstruct ipa_operation_data c_data;\n> +\tc_data.operation = data.operation;\n> +\tc_data.data = data.data.data();\n> +\tc_data.num_data = data.data.size();\n> +\n> +\tstruct ipa_control_list control_lists[data.controls.size()];\n> +\tc_data.lists = control_lists;\n> +\tc_data.num_lists = data.controls.size();\n> +\n> +\tstd::size_t listsSize = 0;\n> +\tfor (const auto &list : data.controls)\n> +\t\tlistsSize += serializer_.binarySize(list);\n> +\n> +\tstd::vector binaryData(listsSize);\n> +\tByteStreamBuffer byteStreamBuffer(binaryData.data(), listsSize);\n> +\n> +\tunsigned int i = 0;\n> +\tfor (const auto &list : data.controls) {\n> +\t\tstruct ipa_control_list &c_list = control_lists[i];\n> +\t\tc_list.size = serializer_.binarySize(list);\n> +\n> +\t\tByteStreamBuffer b = byteStreamBuffer.carveOut(c_list.size);\n> +\t\tserializer_.serialize(list, b);\n> +\n> +\t\tc_list.data = b.base();\n> +\t}\n> +\n> +\tcallbacks_->queue_frame_action(cb_ctx_, frame, c_data);\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +/*\n> + * This construct confuses Doygen and makes it believe that all members of the\n> + * operations is a member of IPAInterfaceWrapper. It must thus be hidden.\n> + */\n> +const struct ipa_context_ops IPAInterfaceWrapper::operations_ = {\n> +\t.destroy = &IPAInterfaceWrapper::destroy,\n> +\t.init = &IPAInterfaceWrapper::init,\n> +\t.register_callbacks = &IPAInterfaceWrapper::register_callbacks,\n> +\t.configure = &IPAInterfaceWrapper::configure,\n> +\t.map_buffers = &IPAInterfaceWrapper::map_buffers,\n> +\t.unmap_buffers = &IPAInterfaceWrapper::unmap_buffers,\n> +\t.process_event = &IPAInterfaceWrapper::process_event,\n> +};\n> +#endif\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> new file mode 100644\n> index 000000000000..17be2062b6c7\n> --- /dev/null\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> @@ -0,0 +1,56 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_interface_wrapper.h - Image Processing Algorithm interface wrapper\n> + */\n> +#ifndef __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__\n> +#define __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"control_serializer.h\"\n> +\n> +namespace libcamera {\n> +\n> +class IPAInterfaceWrapper : public ipa_context\n> +{\n> +public:\n> +\tIPAInterfaceWrapper(std::unique_ptr interface);\n> +\n> +private:\n> +\tstatic void destroy(struct ipa_context *ctx);\n> +\tstatic void init(struct ipa_context *ctx);\n> +\tstatic void register_callbacks(struct ipa_context *ctx,\n> +\t\t\t\t const struct ipa_callback_ops *callbacks,\n> +\t\t\t\t void *cb_ctx);\n> +\tstatic void configure(struct ipa_context *ctx,\n> +\t\t\t const struct ipa_stream *streams,\n> +\t\t\t unsigned int num_streams,\n> +\t\t\t const struct ipa_control_info_map *maps,\n> +\t\t\t unsigned int num_maps);\n> +\tstatic void map_buffers(struct ipa_context *ctx,\n> +\t\t\t\tconst struct ipa_buffer *c_buffers,\n> +\t\t\t\tsize_t num_buffers);\n> +\tstatic void unmap_buffers(struct ipa_context *ctx,\n> +\t\t\t\t const unsigned int *ids,\n> +\t\t\t\t size_t num_buffers);\n> +\tstatic void process_event(struct ipa_context *ctx,\n> +\t\t\t\t const struct ipa_operation_data *data);\n> +\n> +\tstatic const struct ipa_context_ops operations_;\n> +\n> +\tvoid queueFrameAction(unsigned int frame, const IPAOperationData &data);\n> +\n> +\tstd::unique_ptr ipa_;\n> +\tconst struct ipa_callback_ops *callbacks_;\n> +\tvoid *cb_ctx_;\n> +\n> +\tControlSerializer serializer_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_INTERFACE_WRAPPER_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> new file mode 100644\n> index 000000000000..6f3cd4866ce3\n> --- /dev/null\n> +++ b/src/ipa/libipa/meson.build\n> @@ -0,0 +1,13 @@\n> +libipa_headers = files([\n> + 'ipa_interface_wrapper.h',\n> +])\n> +\n> +libipa_sources = files([\n> + 'ipa_interface_wrapper.cpp',\n> +])\n> +\n> +libipa_includes = include_directories('..')\n> +\n> +libipa = static_library('ipa', libipa_sources,\n> + include_directories : ipa_includes,\n> + dependencies : libcamera_dep)\n> diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> index 4f2a45771201..421803243e32 100644\n> --- a/src/ipa/meson.build\n> +++ b/src/ipa/meson.build\n> @@ -10,11 +10,14 @@ ipa_includes = [\n> libcamera_internal_includes,\n> ]\n> \n> +subdir('libipa')\n> +\n> foreach t : ipa_vimc_sources\n> ipa = shared_module(t[0], 'ipa_vimc.cpp',\n> name_prefix : '',\n> include_directories : ipa_includes,\n> dependencies : libcamera_dep,\n> + link_with : libipa,\n> install : true,\n> install_dir : ipa_install_dir,\n> cpp_args : '-DLICENSE=\"' + t[1] + '\"')\n> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> index 1cab319ce6be..521518bd1237 100644\n> --- a/src/ipa/rkisp1/meson.build\n> +++ b/src/ipa/rkisp1/meson.build\n> @@ -1,7 +1,8 @@\n> rkisp1_ipa = shared_module('ipa_rkisp1',\n> 'rkisp1.cpp',\n> name_prefix : '',\n> - include_directories : ipa_includes,\n> + include_directories : [ipa_includes, libipa_includes],\n> dependencies : libcamera_dep,\n> + link_with : libipa,\n> install : true,\n> install_dir : ipa_install_dir)\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 41babf0c4140..744a16ae5b9a 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -19,6 +19,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"log.h\"\n> #include \"utils.h\"\n> @@ -247,9 +248,9 @@ const struct IPAModuleInfo ipaModuleInfo = {\n> \t\"LGPL-2.1-or-later\",\n> };\n> \n> -IPAInterface *ipaCreate()\n> +struct ipa_context *ipaCreate()\n> {\n> -\treturn new IPARkISP1();\n> +\treturn new IPAInterfaceWrapper(utils::make_unique());\n> }\n> }\n> \n> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h\n> new file mode 100644\n> index 000000000000..060888218838\n> --- /dev/null\n> +++ b/src/libcamera/include/ipa_context_wrapper.h\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_context_wrapper.h - Image Processing Algorithm context wrapper\n> + */\n> +#ifndef __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__\n> +#define __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__\n> +\n> +#include \n> +\n> +#include \"control_serializer.h\"\n> +\n> +namespace libcamera {\n> +\n> +class IPAContextWrapper final : public IPAInterface\n> +{\n> +public:\n> +\tIPAContextWrapper(struct ipa_context *context);\n> +\t~IPAContextWrapper();\n> +\n> +\tint init() override;\n> +\tvoid configure(const std::map &streamConfig,\n> +\t\t const std::map &entityControls) override;\n> +\n> +\tvoid mapBuffers(const std::vector &buffers) override;\n> +\tvoid unmapBuffers(const std::vector &ids) override;\n> +\n> +\tvirtual void processEvent(const IPAOperationData &data) override;\n> +\n> +private:\n> +\tstatic void queue_frame_action(void *ctx, unsigned int frame,\n> +\t\t\t\t struct ipa_operation_data &data);\n> +\tstatic const struct ipa_callback_ops callbacks_;\n> +\n> +\tstruct ipa_context *ctx_;\n> +\n> +\tControlSerializer serializer_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_CONTEXT_WRAPPER_H__ */\n> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h\n> index 97737587ab3a..2028b76a1913 100644\n> --- a/src/libcamera/include/ipa_module.h\n> +++ b/src/libcamera/include/ipa_module.h\n> @@ -7,7 +7,6 @@\n> #ifndef __LIBCAMERA_IPA_MODULE_H__\n> #define __LIBCAMERA_IPA_MODULE_H__\n> \n> -#include \n> #include \n> \n> #include \n> @@ -30,7 +29,7 @@ public:\n> \n> \tbool load();\n> \n> -\tstd::unique_ptr createInstance();\n> +\tstruct ipa_context *createContext();\n> \n> \tbool match(PipelineHandler *pipe,\n> \t\t uint32_t minVersion, uint32_t maxVersion) const;\n> @@ -45,7 +44,7 @@ private:\n> \tbool loaded_;\n> \n> \tvoid *dlHandle_;\n> -\ttypedef IPAInterface *(*IPAIntfFactory)(void);\n> +\ttypedef struct ipa_context *(*IPAIntfFactory)(void);\n> \tIPAIntfFactory ipaCreate_;\n> \n> \tint loadIPAModuleInfo();\n> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> index 697294f4b09b..17e2bed93fba 100644\n> --- a/src/libcamera/include/meson.build\n> +++ b/src/libcamera/include/meson.build\n> @@ -9,6 +9,7 @@ libcamera_headers = files([\n> 'device_enumerator_udev.h',\n> 'event_dispatcher_poll.h',\n> 'formats.h',\n> + 'ipa_context_wrapper.h',\n> 'ipa_manager.h',\n> 'ipa_module.h',\n> 'ipa_proxy.h',\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> new file mode 100644\n> index 000000000000..66fc59b82373\n> --- /dev/null\n> +++ b/src/libcamera/ipa_context_wrapper.cpp\n> @@ -0,0 +1,219 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipa_context_wrapper.cpp - Image Processing Algorithm context wrapper\n> + */\n> +\n> +#include \"ipa_context_wrapper.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"byte_stream_buffer.h\"\n> +\n> +/**\n> + * \\file ipa_context_wrapper.h\n> + * \\brief Image Processing Algorithm context wrapper\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class IPAContextWrapper\n> + * \\brief Wrap an ipa_context and expose it as an IPAInterface\n> + *\n> + * The IPAContextWrapper class wraps an ipa_context, provided by an IPA module, and\n> + * exposes an IPAInterface. This mechanism is used for IPAs that are not\n> + * isolated in a separate process to allow direct calls from pipeline handler\n> + * using the IPAInterface API instead of the lower-level ipa_context API.\n> + *\n> + * The IPAInterface methods are converted to the ipa_context API by translating\n> + * all C++ arguments into plain C structures or byte arrays that contain no\n> + * pointer, as required by the ipa_context API.\n> + */\n> +\n> +/**\n> + * \\brief Construct an IPAContextWrapper instance that wraps the \\a context\n> + * \\param[in] context The IPA module context\n> + *\n> + * Ownership of the \\a context is passed to the IPAContextWrapper. The context remains\n> + * valid for the whole lifetime of the wrapper and is destroyed automatically\n> + * with it.\n> + */\n> +IPAContextWrapper::IPAContextWrapper(struct ipa_context *context)\n> +\t: ctx_(context)\n> +{\n> +\tif (!ctx_)\n> +\t\treturn;\n> +\n> +\tctx_->ops->register_callbacks(ctx_, &IPAContextWrapper::callbacks_,\n> +\t\t\t\t this);\n> +}\n> +\n> +IPAContextWrapper::~IPAContextWrapper()\n> +{\n> +\tif (ctx_)\n\nTo align with other methods this check should be inverted,\n\n if !(ctx_)\n return;\n\n> +\t\tctx_->ops->destroy(ctx_);\n> +}\n> +\n> +int IPAContextWrapper::init()\n> +{\n> +\tif (!ctx_)\n> +\t\treturn 0;\n> +\n> +\tctx_->ops->init(ctx_);\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IPAContextWrapper::configure(const std::map &streamConfig,\n> +\t\t\t\t const std::map &entityControls)\n> +{\n> +\tif (!ctx_)\n> +\t\treturn;\n> +\n> +\tserializer_.reset();\n> +\n> +\t/* Translate the IPA stream configurations map. */\n> +\tstruct ipa_stream c_streams[streamConfig.size()];\n> +\n> +\tunsigned int i = 0;\n> +\tfor (const auto &stream : streamConfig) {\n> +\t\tstruct ipa_stream *c_stream = &c_streams[i];\n> +\t\tunsigned int id = stream.first;\n> +\t\tconst IPAStream &ipaStream = stream.second;\n> +\n> +\t\tc_stream->id = id;\n> +\t\tc_stream->pixel_format = ipaStream.pixelFormat;\n> +\t\tc_stream->width = ipaStream.size.width;\n> +\t\tc_stream->height = ipaStream.size.height;\n> +\n> +\t\t++i;\n> +\t}\n> +\n> +\t/* Translate the IPA entity controls map. */\n> +\tstruct ipa_control_info_map c_info_maps[entityControls.size()];\n> +\tstd::vector> data(entityControls.size());\n> +\n> +\ti = 0;\n> +\tfor (const auto &info : entityControls) {\n> +\t\tstruct ipa_control_info_map &c_info_map = c_info_maps[i];\n> +\t\tunsigned int id = info.first;\n> +\t\tconst ControlInfoMap &infoMap = info.second;\n> +\n> +\t\tsize_t infoMapSize = serializer_.binarySize(infoMap);\n> +\t\tdata[i].resize(infoMapSize);\n> +\t\tByteStreamBuffer byteStream(data[i].data(), data[i].size());\n> +\t\tserializer_.serialize(infoMap, byteStream);\n> +\n> +\t\tc_info_map.id = id;\n> +\t\tc_info_map.data = byteStream.base();\n> +\t\tc_info_map.size = byteStream.size();\n> +\n> +\t\t++i;\n> +\t}\n> +\n> +\tctx_->ops->configure(ctx_, c_streams, streamConfig.size(),\n> +\t\t\t c_info_maps, entityControls.size());\n> +}\n> +\n> +void IPAContextWrapper::mapBuffers(const std::vector &buffers)\n> +{\n> +\tif (!ctx_)\n> +\t\treturn;\n> +\n> +\tstruct ipa_buffer c_buffers[buffers.size()];\n> +\n> +\tfor (unsigned int i = 0; i < buffers.size(); ++i) {\n> +\t\tstruct ipa_buffer &c_buffer = c_buffers[i];\n> +\t\tconst IPABuffer &buffer = buffers[i];\n> +\t\tconst std::vector &planes = buffer.memory.planes();\n> +\n> +\t\tc_buffer.id = buffer.id;\n> +\t\tc_buffer.num_planes = planes.size();\n> +\n> +\t\tfor (unsigned int j = 0; j < planes.size(); ++j) {\n> +\t\t\tconst Plane &plane = planes[j];\n> +\t\t\tc_buffer.planes[j].dmabuf = plane.dmabuf();\n> +\t\t\tc_buffer.planes[j].length = plane.length();\n> +\t\t}\n> +\t}\n> +\n> +\tctx_->ops->map_buffers(ctx_, c_buffers, buffers.size());\n> +}\n> +\n> +void IPAContextWrapper::unmapBuffers(const std::vector &ids)\n> +{\n> +\tif (!ctx_)\n> +\t\treturn;\n> +\n> +\tctx_->ops->unmap_buffers(ctx_, ids.data(), ids.size());\n> +}\n> +\n> +void IPAContextWrapper::processEvent(const IPAOperationData &data)\n> +{\n> +\tif (!ctx_)\n> +\t\treturn;\n> +\n> +\tstruct ipa_operation_data c_data;\n> +\tc_data.operation = data.operation;\n> +\tc_data.data = data.data.data();\n> +\tc_data.num_data = data.data.size();\n> +\n> +\tstruct ipa_control_list control_lists[data.controls.size()];\n> +\tc_data.lists = control_lists;\n> +\tc_data.num_lists = data.controls.size();\n> +\n> +\tstd::size_t listsSize = 0;\n> +\tfor (const auto &list : data.controls)\n> +\t\tlistsSize += serializer_.binarySize(list);\n> +\n> +\tstd::vector binaryData(listsSize);\n> +\tByteStreamBuffer byteStreamBuffer(binaryData.data(), listsSize);\n> +\n> +\tunsigned int i = 0;\n> +\tfor (const auto &list : data.controls) {\n> +\t\tstruct ipa_control_list &c_list = control_lists[i];\n> +\t\tc_list.size = serializer_.binarySize(list);\n> +\t\tByteStreamBuffer b = byteStreamBuffer.carveOut(c_list.size);\n> +\n> +\t\tserializer_.serialize(list, b);\n> +\n> +\t\tc_list.data = b.base();\n> +\t}\n> +\n> +\tctx_->ops->process_event(ctx_, &c_data);\n> +}\n> +\n> +void IPAContextWrapper::queue_frame_action(void *ctx, unsigned int frame,\n> +\t\t\t\t\t struct ipa_operation_data &data)\n> +{\n> +\tIPAContextWrapper *_this = static_cast(ctx);\n> +\tIPAOperationData opData;\n> +\n> +\topData.operation = data.operation;\n> +\tfor (unsigned int i = 0; i < data.num_data; ++i)\n> +\t\topData.data.push_back(data.data[i]);\n> +\n> +\tfor (unsigned int i = 0; i < data.num_lists; ++i) {\n> +\t\tconst struct ipa_control_list &c_list = data.lists[i];\n> +\t\tByteStreamBuffer b(c_list.data, c_list.size);\n> +\t\topData.controls.push_back(_this->serializer_.deserialize(b));\n> +\t}\n> +\n> +\t_this->queueFrameAction.emit(frame, opData);\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +/*\n> + * This construct confuses Doygen and makes it believe that all members of the\n> + * operations is a member of IPAInterfaceWrapper. It must thus be hidden.\n> + */\n> +const struct ipa_callback_ops IPAContextWrapper::callbacks_ = {\n> +\t.queue_frame_action = &IPAContextWrapper::queue_frame_action,\n> +};\n> +#endif\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index f3180c0739cb..90eef12dbaf5 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> \n> +#include \"ipa_context_wrapper.h\"\n> #include \"ipa_module.h\"\n> #include \"ipa_proxy.h\"\n> #include \"log.h\"\n> @@ -30,6 +31,66 @@ LOG_DEFINE_CATEGORY(IPAManager)\n> /**\n> * \\class IPAManager\n> * \\brief Manager for IPA modules\n> + *\n> + * The IPA module manager discovers IPA modules from disk, queries and loads\n> + * them, and creates IPA contexts. It supports isolation of the modules in a\n> + * separate process with IPC communication and offers a unified IPAInterface\n> + * view of the IPA contexts to pipeline handlers regardless of whether the\n> + * modules are isolated or loaded in the same process.\n> + *\n> + * Module isolation is based on the module licence. Open-source modules are\n> + * loaded without isolation, while closed-source module are forcefully isolated.\n> + * The isolation mechanism ensures that no code from a closed-source module is\n> + * ever run in the libcamera process.\n> + *\n> + * To create an IPA context, pipeline handlers call the IPAManager::ipaCreate()\n> + * method. For a directly loaded module, the manager calls the module's\n> + * ipaCreate() function directly and wraps the returned context in an\n> + * IPAContextWrapper that exposes an IPAInterface.\n> + *\n> + * ~~~~\n> + * +---------------+\n> + * | Pipeline |\n> + * | Handler |\n> + * +---------------+\n> + * |\n> + * v\n> + * +---------------+ +---------------+\n> + * | IPA | | Open Source |\n> + * | Interface | | IPA Module |\n> + * | - - - - - - - | | - - - - - - - |\n> + * | IPA Context | ipa_context_ops | ipa_context |\n> + * | Wrapper | ----------------> | |\n> + * +---------------+ +---------------+\n> + * ~~~~\n> + *\n> + * For an isolated module, the manager instantiates an IPAProxy which spawns a\n> + * new process for an IPA proxy worker. The worker loads the IPA module and\n> + * creates the IPA context. The IPAProxy alse exposes an IPAInterface.\n> + *\n> + * ~~~~\n> + * +---------------+ +---------------+\n> + * | Pipeline | | Closed Source |\n> + * | Handler | | IPA Module |\n> + * +---------------+ | - - - - - - - |\n> + * | | ipa_context |\n> + * v | |\n> + * +---------------+ +---------------+\n> + * | IPA | ipa_context_ops ^\n> + * | Interface | |\n> + * | - - - - - - - | +---------------+\n> + * | IPA Proxy | operations | IPA Proxy |\n> + * | | ----------------> | Worker |\n> + * +---------------+ over IPC +---------------+\n> + * ~~~~\n> + *\n> + * The IPAInterface implemented by the IPAContextWrapper or IPAProxy is\n> + * returned to the pipeline handler, and all interactions with the IPA context\n> + * go the same interface regardless of process isolation.\n> + *\n> + * In all cases the data passed to the IPAInterface methods is serialized to\n> + * Plain Old Data, either for the purpose of passing it to the IPA context\n> + * plain C API, or to transmit the data to the isolated process through IPC.\n> */\n> \n> IPAManager::IPAManager()\n> @@ -199,7 +260,11 @@ std::unique_ptr IPAManager::createIPA(PipelineHandler *pipe,\n> \tif (!m->load())\n> \t\treturn nullptr;\n> \n> -\treturn m->createInstance();\n> +\tstruct ipa_context *ctx = m->createContext();\n> +\tif (!ctx)\n> +\t\treturn nullptr;\n> +\n> +\treturn utils::make_unique(ctx);\n> }\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 99d308efd47b..2c355ea8b5e5 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -385,13 +385,13 @@ const std::string &IPAModule::path() const\n> /**\n> * \\brief Load the IPA implementation factory from the shared object\n> *\n> - * The IPA module shared object implements an IPAInterface class to be used\n> + * The IPA module shared object implements an ipa_context object to be used\n> * by pipeline handlers. This method loads the factory function from the\n> - * shared object. Later, createInstance() can be called to instantiate the\n> - * IPAInterface.\n> + * shared object. Later, createContext() can be called to instantiate the\n> + * ipa_context.\n> *\n> * This method only needs to be called successfully once, after which\n> - * createInstance() can be called as many times as IPAInterface instances are\n> + * createContext() can be called as many times as ipa_context instances are\n> * needed.\n> *\n> * Calling this function on an invalid module (as returned by isValid()) is\n> @@ -433,24 +433,25 @@ bool IPAModule::load()\n> }\n> \n> /**\n> - * \\brief Instantiate an IPAInterface\n> + * \\brief Instantiate an IPA context\n> *\n> - * After loading the IPA module with load(), this method creates an\n> - * instance of the IPA module interface.\n> + * After loading the IPA module with load(), this method creates an instance of\n> + * the IPA module context. Ownership of the context is passed to the caller, and\n> + * the context shall be destroyed by calling the \\ref ipa_context_ops::destroy\n> + * \"ipa_context::ops::destroy()\" function.\n> *\n> * Calling this function on a module that has not yet been loaded, or an\n> * invalid module (as returned by load() and isValid(), respectively) is\n> * an error.\n> *\n> - * \\return The IPA implementation as a new IPAInterface instance on success,\n> - * or nullptr on error\n> + * \\return The IPA context on success, or nullptr on error\n> */\n> -std::unique_ptr IPAModule::createInstance()\n> +struct ipa_context *IPAModule::createContext()\n> {\n> \tif (!valid_ || !loaded_)\n> \t\treturn nullptr;\n> \n> -\treturn std::unique_ptr(ipaCreate_());\n> +\treturn ipaCreate_();\n> }\n> \n> /**\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 59cf582580c4..c4f965bd7413 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -16,6 +16,7 @@ libcamera_sources = files([\n> 'event_notifier.cpp',\n> 'formats.cpp',\n> 'geometry.cpp',\n> + 'ipa_context_wrapper.cpp',\n> 'ipa_controls.cpp',\n> 'ipa_interface.cpp',\n> 'ipa_manager.cpp',\n> diff --git a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp\n> index a10761e52b32..07380c16e2d5 100644\n> --- a/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp\n> +++ b/src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp\n> @@ -72,9 +72,9 @@ int main(int argc, char **argv)\n> \t}\n> \tsocket.readyRead.connect(&readyRead);\n> \n> -\tstd::unique_ptr ipa = ipam->createInstance();\n> -\tif (!ipa) {\n> -\t\tLOG(IPAProxyLinuxWorker, Error) << \"Failed to create IPA interface\";\n> +\tstruct ipa_context *ipac = ipam->createContext();\n> +\tif (!ipac) {\n> +\t\tLOG(IPAProxyLinuxWorker, Error) << \"Failed to create IPA context\";\n> \t\treturn EXIT_FAILURE;\n> \t}\n> \n> @@ -85,5 +85,7 @@ int main(int argc, char **argv)\n> \twhile (1)\n> \t\tdispatcher->processEvents();\n> \n> +\tipac->ops->destroy(ipac);\n> +\n> \treturn 0;\n> }\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECF6660F1C\n\tfor ;\n\tMon, 18 Nov 2019 23:58:52 +0100 (CET)","by mail-lj1-x244.google.com with SMTP id e9so20945218ljp.13\n\tfor ;\n\tMon, 18 Nov 2019 14:58:52 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tj23sm9293895lji.41.2019.11.18.14.58.51\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 18 Nov 2019 14:58:51 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=SlbH9wsgGLeT638Mg29ZmLUw+Cq8Le8daTMfF2fn76Y=;\n\tb=g06dBUEZwHtnA1hjiTeW6eZdCngRvzGUfgPfe7hlNRaR4c5oHHOaLBlIGmdTtbIsJ/\n\tRwUARQDeqJUKq34RdDWuaruVatCaPcTgP/rwQ4VP6YcQU2k8hCNB3JBDHceuIWUH5jZh\n\tr+vxVKJPCiUWwUHnd4428c99zYEr7XawWcySI/f8asnjaCYNDUfNZE4SQnb6STMbFozB\n\tjpHoTWSoQnwPHG8QSA2t+20rDLypyc/lwUUjg8zofkCSWCoMK/gWmaqJk4SknGqG1oMe\n\tFsOCLfdzbolssJ+qOtIJ3eh2WGs+ZA48Uwmxj47rCZqQq0ruS5mcrhQoqJM1YWdIrCRi\n\tKcQQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=SlbH9wsgGLeT638Mg29ZmLUw+Cq8Le8daTMfF2fn76Y=;\n\tb=W/SHpGwlvysPJZH5m1VjfwD7rZt/157drPp2ZLvbii9KMHN4bdjn/4HuisjU0cBofX\n\txoDXAOW+E6qHNp38TRlX8MVIjjtpr8Lkgn2NwNrWSm76CSnhxmAl87MD1lJus3OtikgT\n\tAcoLhuHgsR9E7HZyiQqAJFn2UANRjX93Hdi4v/Am6NvL31LE1nwdw2GdXLX3jmXZaqBf\n\tVM1Jt0EhHYa0i+J5wcuxJZGX4nGOtkNDeE47lMk1vLUdgEfhL5teUfSGPhEB07idG+xd\n\t/3ZUUiK5ysJMMwhOXA52xstJsYsbnEys+pwKbNx6c8RvuNnrj8JLv1Z30vyyD0EKU5L+\n\tVjKw==","X-Gm-Message-State":"APjAAAWt6ryay8yYLGlN55REXuWk8Dr1RPdSM+oqFUs9v2pvFKSzmnSx\n\tiEvNI8b4imDWfTCMAE3Sh6QxDGGdjLU=","X-Google-Smtp-Source":"APXvYqyOnKzbTTW5h79KXlK518Ua0Ri1l0HYnLPzIh0WSCNDr0WDOG9VOt3emgbnIvUITV7TzzvRMg==","X-Received":"by 2002:a2e:9097:: with SMTP id\n\tl23mr1335509ljg.103.1574117932061; \n\tMon, 18 Nov 2019 14:58:52 -0800 (PST)","Date":"Mon, 18 Nov 2019 23:58:51 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191118225851.GO8072@bigcity.dyn.berto.se>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-21-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191108205409.18845-21-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 20/24] ipa: Switch to the plain C\n\tAPI","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 18 Nov 2019 22:58:53 -0000"}}]