[libcamera-devel,v2,20/24] ipa: Switch to the plain C API

2316 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

Switch IPA communication to the plain C API. As the IPAInterface class
is easier to use for pipeline handlers than a plain C API, retain it and
add an IPAContextWrapper that translate between the C++ and the C APIs.

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
IPAInterfaceWrapper helper class is introduced to wrap the IPAInterface
implemented internally by the IPA module into an ipa_context,
ipa_context_ops and ipa_callback_ops.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/Doxyfile.in                     |   1 +
 Documentation/meson.build                     |   2 +
 src/ipa/ipa_vimc.cpp                          |   7 +-
 src/ipa/libipa/ipa_interface_wrapper.cpp      | 240 ++++++++++++++++++
 src/ipa/libipa/ipa_interface_wrapper.h        |  56 ++++
 src/ipa/libipa/meson.build                    |  13 +
 src/ipa/meson.build                           |   3 +
 src/ipa/rkisp1/meson.build                    |   3 +-
 src/ipa/rkisp1/rkisp1.cpp                     |   5 +-
 src/libcamera/include/ipa_context_wrapper.h   |  43 ++++
 src/libcamera/include/ipa_module.h            |   5 +-
 src/libcamera/include/meson.build             |   1 +
 src/libcamera/ipa_context_wrapper.cpp         | 219 ++++++++++++++++
 src/libcamera/ipa_manager.cpp                 |  67 ++++-
 src/libcamera/ipa_module.cpp                  |  23 +-
 src/libcamera/meson.build                     |   1 +
 .../proxy/worker/ipa_proxy_linux_worker.cpp   |   8 +-
 17 files changed, 674 insertions(+), 23 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

Hi Jacopo,

Thanks for your work.

On 2019-11-08 22:54:05 +0200, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Switch IPA communication to the plain C API. As the IPAInterface class
> is easier to use for pipeline handlers than a plain C API, retain it and
> add an IPAContextWrapper that translate between the C++ and the C APIs.
> 
> 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
> IPAInterfaceWrapper helper class is introduced to wrap the IPAInterface
> implemented internally by the IPA module into an ipa_context,
> ipa_context_ops and ipa_callback_ops.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

One small nit bellow, other then that

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  Documentation/Doxyfile.in                     |   1 +
>  Documentation/meson.build                     |   2 +
>  src/ipa/ipa_vimc.cpp                          |   7 +-
>  src/ipa/libipa/ipa_interface_wrapper.cpp      | 240 ++++++++++++++++++
>  src/ipa/libipa/ipa_interface_wrapper.h        |  56 ++++
>  src/ipa/libipa/meson.build                    |  13 +
>  src/ipa/meson.build                           |   3 +
>  src/ipa/rkisp1/meson.build                    |   3 +-
>  src/ipa/rkisp1/rkisp1.cpp                     |   5 +-
>  src/libcamera/include/ipa_context_wrapper.h   |  43 ++++
>  src/libcamera/include/ipa_module.h            |   5 +-
>  src/libcamera/include/meson.build             |   1 +
>  src/libcamera/ipa_context_wrapper.cpp         | 219 ++++++++++++++++
>  src/libcamera/ipa_manager.cpp                 |  67 ++++-
>  src/libcamera/ipa_module.cpp                  |  23 +-
>  src/libcamera/meson.build                     |   1 +
>  .../proxy/worker/ipa_proxy_linux_worker.cpp   |   8 +-
>  17 files changed, 674 insertions(+), 23 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 24babfd8b366..840c1b4c76c5 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" \
>  			 "@TOP_BUILDDIR@/include/libcamera" \
>  			 "@TOP_BUILDDIR@/src/libcamera"
> 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/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp
> index 50ca8dd805fb..8f03e811acc7 100644
> --- a/src/ipa/ipa_vimc.cpp
> +++ b/src/ipa/ipa_vimc.cpp
> @@ -17,7 +17,10 @@
>  #include <ipa/ipa_interface.h>
>  #include <ipa/ipa_module_info.h>
>  
> +#include "libipa/ipa_interface_wrapper.h"
> +
>  #include "log.h"
> +#include "utils.h"
>  
>  namespace libcamera {
>  
> @@ -108,9 +111,9 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	LICENSE,
>  };
>  
> -IPAInterface *ipaCreate()
> +struct ipa_context *ipaCreate()
>  {
> -	return new IPAVimc();
> +	return new IPAInterfaceWrapper(utils::make_unique<IPAVimc>());
>  }
>  }
>  
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> new file mode 100644
> index 000000000000..80c5648ffed6
> --- /dev/null
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -0,0 +1,240 @@
> +/* 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 <map>
> +#include <string.h>
> +#include <unistd.h>
> +#include <vector>
> +
> +#include <ipa/ipa_interface.h>
> +
> +#include "byte_stream_buffer.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(utils::make_unique<MyIPA>());
> + * }
> + * \endcode
> + *
> + * The wrapper takes ownership of the IPAInterface and will automatically
> + * delete it when the wrapper is destroyed.
> + */
> +
> +/**
> + * \brief Construct an IPAInterfaceWrapper wrapping \a interface
> + * \param[in] interface The interface to wrap
> + */
> +IPAInterfaceWrapper::IPAInterfaceWrapper(std::unique_ptr<IPAInterface> interface)
> +	: ipa_(std::move(interface)), callbacks_(nullptr), cb_ctx_(nullptr)
> +{
> +	ops = &operations_;
> +
> +	ipa_->queueFrameAction.connect(this, &IPAInterfaceWrapper::queueFrameAction);
> +}
> +
> +void IPAInterfaceWrapper::destroy(struct ipa_context *_ctx)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +
> +	delete ctx;
> +}
> +
> +void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +
> +	ctx->ipa_->init();
> +}
> +
> +void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
> +					     const struct ipa_callback_ops *callbacks,
> +					     void *cb_ctx)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +
> +	ctx->callbacks_ = callbacks;
> +	ctx->cb_ctx_ = cb_ctx;
> +}
> +
> +void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> +				    const struct ipa_stream *streams,
> +				    unsigned int num_streams,
> +				    const struct ipa_control_info_map *maps,
> +				    unsigned int num_maps)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +
> +	ctx->serializer_.reset();
> +
> +	/* Translate the IPA stream configurations map. */
> +	std::map<unsigned int, IPAStream> ipaStreams;
> +
> +	for (unsigned int i = 0; i < num_streams; ++i) {
> +		const struct ipa_stream &stream = streams[i];
> +
> +		ipaStreams[stream.id] = {
> +			stream.pixel_format,
> +			Size(stream.width, stream.height),
> +		};
> +	}
> +
> +	/* Translate the IPA entity controls map. */
> +	std::map<unsigned int, const ControlInfoMap &> entityControls;
> +	std::map<unsigned int, ControlInfoMap> infoMaps;
> +
> +	for (unsigned int i = 0; i < num_maps; ++i) {
> +		const struct ipa_control_info_map &ipa_map = maps[i];
> +		ByteStreamBuffer byteStream(ipa_map.data, ipa_map.size);
> +		unsigned int id = ipa_map.id;
> +
> +		infoMaps[id] = ctx->serializer_.deserialize<ControlInfoMap>(byteStream);
> +		entityControls.emplace(id, infoMaps[id]);
> +	}
> +
> +	ctx->ipa_->configure(ipaStreams, entityControls);
> +}
> +
> +void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> +				      const struct ipa_buffer *_buffers,
> +				      size_t num_buffers)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +	std::vector<IPABuffer> buffers(num_buffers);
> +
> +	for (unsigned int i = 0; i < num_buffers; ++i) {
> +		const struct ipa_buffer &_buffer = _buffers[i];
> +		IPABuffer &buffer = buffers[i];
> +		std::vector<Plane> &planes = buffer.memory.planes();
> +
> +		buffer.id = _buffer.id;
> +
> +		planes.resize(_buffer.num_planes);
> +		for (unsigned int j = 0; j < _buffer.num_planes; ++j) {
> +			if (_buffer.planes[j].dmabuf != -1)
> +				planes[j].setDmabuf(_buffer.planes[j].dmabuf,
> +						    _buffer.planes[j].length);
> +			/** \todo Create a Dmabuf class to implement RAII. */
> +			::close(_buffer.planes[j].dmabuf);
> +		}
> +	}
> +
> +	ctx->ipa_->mapBuffers(buffers);
> +}
> +
> +void IPAInterfaceWrapper::unmap_buffers(struct ipa_context *_ctx,
> +					const unsigned int *_ids,
> +					size_t num_buffers)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +	std::vector<unsigned int> ids(_ids, _ids + num_buffers);
> +	ctx->ipa_->unmapBuffers(ids);
> +}
> +
> +void IPAInterfaceWrapper::process_event(struct ipa_context *_ctx,
> +					const struct ipa_operation_data *data)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +	IPAOperationData opData;
> +
> +	opData.operation = data->operation;
> +
> +	opData.data.resize(data->num_data);
> +	memcpy(opData.data.data(), data->data,
> +	       data->num_data * sizeof(*data->data));
> +
> +	opData.controls.resize(data->num_lists);
> +	for (unsigned int i = 0; i < data->num_lists; ++i) {
> +		const struct ipa_control_list *c_list = &data->lists[i];
> +		ByteStreamBuffer byteStream(c_list->data, c_list->size);
> +		opData.controls[i] = ctx->serializer_.deserialize<ControlList>(byteStream);
> +	}
> +
> +	ctx->ipa_->processEvent(opData);
> +}
> +
> +void IPAInterfaceWrapper::queueFrameAction(unsigned int frame,
> +					   const IPAOperationData &data)
> +{
> +	if (!callbacks_)
> +		return;
> +
> +	struct ipa_operation_data c_data;
> +	c_data.operation = data.operation;
> +	c_data.data = data.data.data();
> +	c_data.num_data = data.data.size();
> +
> +	struct ipa_control_list control_lists[data.controls.size()];
> +	c_data.lists = control_lists;
> +	c_data.num_lists = data.controls.size();
> +
> +	std::size_t listsSize = 0;
> +	for (const auto &list : data.controls)
> +		listsSize += serializer_.binarySize(list);
> +
> +	std::vector<uint8_t> binaryData(listsSize);
> +	ByteStreamBuffer byteStreamBuffer(binaryData.data(), listsSize);
> +
> +	unsigned int i = 0;
> +	for (const auto &list : data.controls) {
> +		struct ipa_control_list &c_list = control_lists[i];
> +		c_list.size = serializer_.binarySize(list);
> +
> +		ByteStreamBuffer b = byteStreamBuffer.carveOut(c_list.size);
> +		serializer_.serialize(list, b);
> +
> +		c_list.data = b.base();
> +	}
> +
> +	callbacks_->queue_frame_action(cb_ctx_, frame, c_data);
> +}
> +
> +#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_context_ops IPAInterfaceWrapper::operations_ = {
> +	.destroy = &IPAInterfaceWrapper::destroy,
> +	.init = &IPAInterfaceWrapper::init,
> +	.register_callbacks = &IPAInterfaceWrapper::register_callbacks,
> +	.configure = &IPAInterfaceWrapper::configure,
> +	.map_buffers = &IPAInterfaceWrapper::map_buffers,
> +	.unmap_buffers = &IPAInterfaceWrapper::unmap_buffers,
> +	.process_event = &IPAInterfaceWrapper::process_event,
> +};
> +#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..17be2062b6c7
> --- /dev/null
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -0,0 +1,56 @@
> +/* 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 <memory>
> +
> +#include <ipa/ipa_interface.h>
> +
> +#include "control_serializer.h"
> +
> +namespace libcamera {
> +
> +class IPAInterfaceWrapper : public ipa_context
> +{
> +public:
> +	IPAInterfaceWrapper(std::unique_ptr<IPAInterface> interface);
> +
> +private:
> +	static void destroy(struct ipa_context *ctx);
> +	static void init(struct ipa_context *ctx);
> +	static void register_callbacks(struct ipa_context *ctx,
> +				       const struct ipa_callback_ops *callbacks,
> +				       void *cb_ctx);
> +	static void configure(struct ipa_context *ctx,
> +			      const struct ipa_stream *streams,
> +			      unsigned int num_streams,
> +			      const struct ipa_control_info_map *maps,
> +			      unsigned int num_maps);
> +	static void map_buffers(struct ipa_context *ctx,
> +				const struct ipa_buffer *c_buffers,
> +				size_t num_buffers);
> +	static void unmap_buffers(struct ipa_context *ctx,
> +				  const unsigned int *ids,
> +				  size_t num_buffers);
> +	static void process_event(struct ipa_context *ctx,
> +				  const struct ipa_operation_data *data);
> +
> +	static const struct ipa_context_ops operations_;
> +
> +	void queueFrameAction(unsigned int frame, const IPAOperationData &data);
> +
> +	std::unique_ptr<IPAInterface> ipa_;
> +	const struct ipa_callback_ops *callbacks_;
> +	void *cb_ctx_;
> +
> +	ControlSerializer serializer_;
> +};
> +
> +} /* 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..6f3cd4866ce3
> --- /dev/null
> +++ b/src/ipa/libipa/meson.build
> @@ -0,0 +1,13 @@
> +libipa_headers = files([
> +    'ipa_interface_wrapper.h',
> +])
> +
> +libipa_sources = files([
> +    'ipa_interface_wrapper.cpp',
> +])
> +
> +libipa_includes = include_directories('..')
> +
> +libipa = static_library('ipa', libipa_sources,
> +                        include_directories : ipa_includes,
> +                        dependencies : libcamera_dep)
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> index 4f2a45771201..421803243e32 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -10,11 +10,14 @@ ipa_includes = [
>      libcamera_internal_includes,
>  ]
>  
> +subdir('libipa')
> +
>  foreach t : ipa_vimc_sources
>      ipa = shared_module(t[0], 'ipa_vimc.cpp',
>                          name_prefix : '',
>                          include_directories : ipa_includes,
>                          dependencies : libcamera_dep,
> +                        link_with : libipa,
>                          install : true,
>                          install_dir : ipa_install_dir,
>                          cpp_args : '-DLICENSE="' + t[1] + '"')
> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> index 1cab319ce6be..521518bd1237 100644
> --- a/src/ipa/rkisp1/meson.build
> +++ b/src/ipa/rkisp1/meson.build
> @@ -1,7 +1,8 @@
>  rkisp1_ipa = shared_module('ipa_rkisp1',
>                             'rkisp1.cpp',
>                             name_prefix : '',
> -                           include_directories : ipa_includes,
> +                           include_directories : [ipa_includes, libipa_includes],
>                             dependencies : libcamera_dep,
> +                           link_with : libipa,
>                             install : true,
>                             install_dir : ipa_install_dir)
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 41babf0c4140..744a16ae5b9a 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -19,6 +19,7 @@
>  #include <libcamera/buffer.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/request.h>
> +#include <libipa/ipa_interface_wrapper.h>
>  
>  #include "log.h"
>  #include "utils.h"
> @@ -247,9 +248,9 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	"LGPL-2.1-or-later",
>  };
>  
> -IPAInterface *ipaCreate()
> +struct ipa_context *ipaCreate()
>  {
> -	return new IPARkISP1();
> +	return new IPAInterfaceWrapper(utils::make_unique<IPARkISP1>());
>  }
>  }
>  
> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> new file mode 100644
> index 000000000000..060888218838
> --- /dev/null
> +++ b/src/libcamera/include/ipa_context_wrapper.h
> @@ -0,0 +1,43 @@
> +/* 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>
> +
> +#include "control_serializer.h"
> +
> +namespace libcamera {
> +
> +class IPAContextWrapper final : public IPAInterface
> +{
> +public:
> +	IPAContextWrapper(struct ipa_context *context);
> +	~IPAContextWrapper();
> +
> +	int init() override;
> +	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +
> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> +	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> +
> +	virtual void processEvent(const IPAOperationData &data) override;
> +
> +private:
> +	static void queue_frame_action(void *ctx, unsigned int frame,
> +				       struct ipa_operation_data &data);
> +	static const struct ipa_callback_ops callbacks_;
> +
> +	struct ipa_context *ctx_;
> +
> +	ControlSerializer serializer_;
> +};
> +
> +} /* 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 697294f4b09b..17e2bed93fba 100644
> --- a/src/libcamera/include/meson.build
> +++ b/src/libcamera/include/meson.build
> @@ -9,6 +9,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..66fc59b82373
> --- /dev/null
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -0,0 +1,219 @@
> +/* 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 <vector>
> +
> +#include <libcamera/controls.h>
> +
> +#include "byte_stream_buffer.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 translating
> + * 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)
> +{
> +	if (!ctx_)
> +		return;
> +
> +	ctx_->ops->register_callbacks(ctx_, &IPAContextWrapper::callbacks_,
> +				      this);
> +}
> +
> +IPAContextWrapper::~IPAContextWrapper()
> +{
> +	if (ctx_)

To align with other methods this check should be inverted,

        if !(ctx_)
            return;

> +		ctx_->ops->destroy(ctx_);
> +}
> +
> +int IPAContextWrapper::init()
> +{
> +	if (!ctx_)
> +		return 0;
> +
> +	ctx_->ops->init(ctx_);
> +
> +	return 0;
> +}
> +
> +void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +{
> +	if (!ctx_)
> +		return;
> +
> +	serializer_.reset();
> +
> +	/* Translate the IPA stream configurations map. */
> +	struct ipa_stream c_streams[streamConfig.size()];
> +
> +	unsigned int i = 0;
> +	for (const auto &stream : streamConfig) {
> +		struct ipa_stream *c_stream = &c_streams[i];
> +		unsigned int id = stream.first;
> +		const IPAStream &ipaStream = stream.second;
> +
> +		c_stream->id = id;
> +		c_stream->pixel_format = ipaStream.pixelFormat;
> +		c_stream->width = ipaStream.size.width;
> +		c_stream->height = ipaStream.size.height;
> +
> +		++i;
> +	}
> +
> +	/* Translate the IPA entity controls map. */
> +	struct ipa_control_info_map c_info_maps[entityControls.size()];
> +	std::vector<std::vector<uint8_t>> data(entityControls.size());
> +
> +	i = 0;
> +	for (const auto &info : entityControls) {
> +		struct ipa_control_info_map &c_info_map = c_info_maps[i];
> +		unsigned int id = info.first;
> +		const ControlInfoMap &infoMap = info.second;
> +
> +		size_t infoMapSize = serializer_.binarySize(infoMap);
> +		data[i].resize(infoMapSize);
> +		ByteStreamBuffer byteStream(data[i].data(), data[i].size());
> +		serializer_.serialize(infoMap, byteStream);
> +
> +		c_info_map.id = id;
> +		c_info_map.data = byteStream.base();
> +		c_info_map.size = byteStream.size();
> +
> +		++i;
> +	}
> +
> +	ctx_->ops->configure(ctx_, c_streams, streamConfig.size(),
> +			     c_info_maps, entityControls.size());
> +}
> +
> +void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
> +{
> +	if (!ctx_)
> +		return;
> +
> +	struct ipa_buffer c_buffers[buffers.size()];
> +
> +	for (unsigned int i = 0; i < buffers.size(); ++i) {
> +		struct ipa_buffer &c_buffer = c_buffers[i];
> +		const IPABuffer &buffer = buffers[i];
> +		const std::vector<Plane> &planes = buffer.memory.planes();
> +
> +		c_buffer.id = buffer.id;
> +		c_buffer.num_planes = planes.size();
> +
> +		for (unsigned int j = 0; j < planes.size(); ++j) {
> +			const Plane &plane = planes[j];
> +			c_buffer.planes[j].dmabuf = plane.dmabuf();
> +			c_buffer.planes[j].length = plane.length();
> +		}
> +	}
> +
> +	ctx_->ops->map_buffers(ctx_, c_buffers, buffers.size());
> +}
> +
> +void IPAContextWrapper::unmapBuffers(const std::vector<unsigned int> &ids)
> +{
> +	if (!ctx_)
> +		return;
> +
> +	ctx_->ops->unmap_buffers(ctx_, ids.data(), ids.size());
> +}
> +
> +void IPAContextWrapper::processEvent(const IPAOperationData &data)
> +{
> +	if (!ctx_)
> +		return;
> +
> +	struct ipa_operation_data c_data;
> +	c_data.operation = data.operation;
> +	c_data.data = data.data.data();
> +	c_data.num_data = data.data.size();
> +
> +	struct ipa_control_list control_lists[data.controls.size()];
> +	c_data.lists = control_lists;
> +	c_data.num_lists = data.controls.size();
> +
> +	std::size_t listsSize = 0;
> +	for (const auto &list : data.controls)
> +		listsSize += serializer_.binarySize(list);
> +
> +	std::vector<uint8_t> binaryData(listsSize);
> +	ByteStreamBuffer byteStreamBuffer(binaryData.data(), listsSize);
> +
> +	unsigned int i = 0;
> +	for (const auto &list : data.controls) {
> +		struct ipa_control_list &c_list = control_lists[i];
> +		c_list.size = serializer_.binarySize(list);
> +		ByteStreamBuffer b = byteStreamBuffer.carveOut(c_list.size);
> +
> +		serializer_.serialize(list, b);
> +
> +		c_list.data = b.base();
> +	}
> +
> +	ctx_->ops->process_event(ctx_, &c_data);
> +}
> +
> +void IPAContextWrapper::queue_frame_action(void *ctx, unsigned int frame,
> +					   struct ipa_operation_data &data)
> +{
> +	IPAContextWrapper *_this = static_cast<IPAContextWrapper *>(ctx);
> +	IPAOperationData opData;
> +
> +	opData.operation = data.operation;
> +	for (unsigned int i = 0; i < data.num_data; ++i)
> +		opData.data.push_back(data.data[i]);
> +
> +	for (unsigned int i = 0; i < data.num_lists; ++i) {
> +		const struct ipa_control_list &c_list = data.lists[i];
> +		ByteStreamBuffer b(c_list.data, c_list.size);
> +		opData.controls.push_back(_this->serializer_.deserialize<ControlList>(b));
> +	}
> +
> +	_this->queueFrameAction.emit(frame, opData);
> +}
> +
> +#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_callback_ops IPAContextWrapper::callbacks_ = {
> +	.queue_frame_action = &IPAContextWrapper::queue_frame_action,
> +};
> +#endif
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index f3180c0739cb..90eef12dbaf5 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -12,6 +12,7 @@
>  #include <string.h>
>  #include <sys/types.h>
>  
> +#include "ipa_context_wrapper.h"
>  #include "ipa_module.h"
>  #include "ipa_proxy.h"
>  #include "log.h"
> @@ -30,6 +31,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_context_ops  |  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_context_ops ^
> + * |   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 regardless of process isolation.
> + *
> + * In all cases the data passed to the IPAInterface methods is serialized 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()
> @@ -199,7 +260,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..2c355ea8b5e5 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_context_ops::destroy
> + * "ipa_context::ops::destroy()" function.
>   *
>   * 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 59cf582580c4..c4f965bd7413 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -16,6 +16,7 @@ libcamera_sources = files([
>      'event_notifier.cpp',
>      'formats.cpp',
>      'geometry.cpp',
> +    'ipa_context_wrapper.cpp',
>      'ipa_controls.cpp',
>      'ipa_interface.cpp',
>      'ipa_manager.cpp',
> 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