[libcamera-devel] [PATCH v4 31/37] ipa: remove libipa
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Nov 6 12:12:23 CET 2020
Hi Paul,
On 06/11/2020 10:37, Paul Elder wrote:
> As every pipeline and have its own proxy, IPAInterfaceWrapper is no
> longer necessary. Since it's the only member of libipa, remove libipa
> completely.
Ah, I think I foresee the (imminent?) addition of items to libipa to be
able to share some common features across open-source IPA modules.
(closed source ones won't share of course).
Could we have this patch strip out the dead code, but /keep/ the libipa
itself as a skeleton at least?
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> ---
> No change in v4
>
> No change in v3
>
> No change in v2
> ---
> Documentation/Doxyfile.in | 1 -
> Documentation/meson.build | 2 -
> src/ipa/libipa/ipa_interface_wrapper.cpp | 285 -----------------------
> src/ipa/libipa/ipa_interface_wrapper.h | 61 -----
> src/ipa/libipa/meson.build | 15 --
> src/ipa/meson.build | 2 -
> src/ipa/raspberrypi/meson.build | 2 -
> src/ipa/raspberrypi/raspberrypi.cpp | 2 -
> src/ipa/rkisp1/meson.build | 3 +-
> src/ipa/rkisp1/rkisp1.cpp | 2 -
> src/ipa/vimc/meson.build | 3 +-
> src/ipa/vimc/vimc.cpp | 2 -
> test/ipa/ipa_wrappers_test.cpp | 1 -
> test/ipa/meson.build | 4 +-
> 14 files changed, 4 insertions(+), 381 deletions(-)
> delete mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp
> delete mode 100644 src/ipa/libipa/ipa_interface_wrapper.h
> delete mode 100644 src/ipa/libipa/meson.build
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 20fa1349..9a7a3fb4 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -789,7 +789,6 @@ WARN_LOGFILE =
> # Note: If this tag is empty the current directory is searched.
>
> INPUT = "@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 26a12fcd..59107151 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -27,8 +27,6 @@ if doxygen.found() and dot.found()
> libcamera_ipa_headers,
> libcamera_public_headers,
> libcamera_sources,
> - libipa_headers,
> - libipa_sources,
> ],
> output : 'api-html',
> command : [doxygen, doxyfile],
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> deleted file mode 100644
> index cee532e3..00000000
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ /dev/null
> @@ -1,285 +0,0 @@
> -/* 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 <libcamera/ipa/ipa_interface.h>
> -
> -#include "libcamera/internal/byte_stream_buffer.h"
> -#include "libcamera/internal/camera_sensor.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(std::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::get_interface(struct ipa_context *_ctx)
> -{
> - IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> -
> - return ctx->ipa_.get();
> -}
> -
> -void IPAInterfaceWrapper::init(struct ipa_context *_ctx,
> - const struct ipa_settings *settings)
> -{
> - IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> -
> - IPASettings ipaSettings{
> - .configurationFile = settings->configuration_file
> - };
> - ctx->ipa_->init(ipaSettings);
> -}
> -
> -int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
> -{
> - IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> -
> - return ctx->ipa_->start();
> -}
> -
> -void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> -{
> - IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> -
> - ctx->ipa_->stop();
> -}
> -
> -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_sensor_info *sensor_info,
> - 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 sensor info. */
> - CameraSensorInfo sensorInfo{};
> - sensorInfo.model = sensor_info->model;
> - sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;
> - sensorInfo.activeAreaSize = { sensor_info->active_area.width,
> - sensor_info->active_area.height };
> - sensorInfo.analogCrop = { sensor_info->analog_crop.left,
> - sensor_info->analog_crop.top,
> - sensor_info->analog_crop.width,
> - sensor_info->analog_crop.height };
> - sensorInfo.outputSize = { sensor_info->output_size.width,
> - sensor_info->output_size.height };
> - sensorInfo.pixelRate = sensor_info->pixel_rate;
> - sensorInfo.lineLength = sensor_info->line_length;
> -
> - /* 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]);
> - }
> -
> - /* \todo Translate the ipaConfig and result. */
> - IPAOperationData ipaConfig;
> - ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> - nullptr);
> -}
> -
> -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<FrameBuffer::Plane> &planes = buffer.planes;
> -
> - buffer.id = _buffer.id;
> -
> - planes.resize(_buffer.num_planes);
> - for (unsigned int j = 0; j < _buffer.num_planes; ++j) {
> - planes[j].fd = FileDescriptor(_buffer.planes[j].dmabuf);
> - planes[j].length = _buffer.planes[j].length;
> - }
> - }
> -
> - 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,
> - .get_interface = &IPAInterfaceWrapper::get_interface,
> - .init = &IPAInterfaceWrapper::init,
> - .start = &IPAInterfaceWrapper::start,
> - .stop = &IPAInterfaceWrapper::stop,
> - .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
> deleted file mode 100644
> index a1c70159..00000000
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ /dev/null
> @@ -1,61 +0,0 @@
> -/* 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 <libcamera/ipa/ipa_interface.h>
> -
> -#include "libcamera/internal/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 *get_interface(struct ipa_context *ctx);
> - static void init(struct ipa_context *ctx,
> - const struct ipa_settings *settings);
> - static int start(struct ipa_context *ctx);
> - static void stop(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_sensor_info *sensor_info,
> - 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
> deleted file mode 100644
> index 22626405..00000000
> --- a/src/ipa/libipa/meson.build
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -# SPDX-License-Identifier: CC0-1.0
> -
> -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 5a5de267..b11e5d23 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -15,8 +15,6 @@ config_h.set('IPA_CONFIG_DIR',
> config_h.set('IPA_MODULE_DIR',
> '"' + join_paths(get_option('prefix'), ipa_install_dir) + '"')
>
> -subdir('libipa')
> -
> ipa_sign = files('ipa-sign.sh')
>
> ipas = ['raspberrypi', 'rkisp1', 'vimc']
> diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> index 4331f718..4a0e18cf 100644
> --- a/src/ipa/raspberrypi/meson.build
> +++ b/src/ipa/raspberrypi/meson.build
> @@ -10,7 +10,6 @@ rpi_ipa_deps = [
>
> rpi_ipa_includes = [
> ipa_includes,
> - libipa_includes,
> include_directories('controller')
> ]
>
> @@ -46,7 +45,6 @@ mod = shared_module(ipa_name,
> name_prefix : '',
> include_directories : rpi_ipa_includes,
> dependencies : rpi_ipa_deps,
> - link_with : libipa,
> install : true,
> install_dir : ipa_install_dir)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 316da7bd..592436f8 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -23,8 +23,6 @@
> #include <libcamera/request.h>
> #include <libcamera/span.h>
>
> -#include <libipa/ipa_interface_wrapper.h>
> -
> #include "libcamera/internal/camera_sensor.h"
> #include "libcamera/internal/log.h"
> #include "libcamera/internal/utils.h"
> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> index 3061d53c..bfe8d362 100644
> --- a/src/ipa/rkisp1/meson.build
> +++ b/src/ipa/rkisp1/meson.build
> @@ -5,9 +5,8 @@ ipa_name = 'ipa_rkisp1'
> mod = shared_module(ipa_name,
> ['rkisp1.cpp', libcamera_generated_ipa_headers],
> name_prefix : '',
> - include_directories : [ipa_includes, libipa_includes],
> + include_directories : ipa_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 7bb261c6..c88f1271 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -23,8 +23,6 @@
> #include <libcamera/ipa/rkisp1_ipa_interface.h>
> #include <libcamera/request.h>
>
> -#include <libipa/ipa_interface_wrapper.h>
> -
> #include "libcamera/internal/log.h"
>
> namespace libcamera {
> diff --git a/src/ipa/vimc/meson.build b/src/ipa/vimc/meson.build
> index e982ebba..67440fd8 100644
> --- a/src/ipa/vimc/meson.build
> +++ b/src/ipa/vimc/meson.build
> @@ -5,9 +5,8 @@ ipa_name = 'ipa_vimc'
> mod = shared_module(ipa_name,
> ['vimc.cpp', libcamera_generated_ipa_headers],
> name_prefix : '',
> - include_directories : [ipa_includes, libipa_includes],
> + include_directories : ipa_includes,
> dependencies : libcamera_dep,
> - link_with : libipa,
> install : true,
> install_dir : ipa_install_dir)
>
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index fbf7325e..6b088466 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -18,8 +18,6 @@
> #include <libcamera/ipa/ipa_interface.h>
> #include <libcamera/ipa/ipa_module_info.h>
>
> -#include <libipa/ipa_interface_wrapper.h>
> -
> #include "libcamera/internal/file.h"
> #include "libcamera/internal/log.h"
>
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index 59d991cb..0133d8d2 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -13,7 +13,6 @@
> #include <unistd.h>
>
> #include <libcamera/controls.h>
> -#include <libipa/ipa_interface_wrapper.h>
>
> #include "libcamera/internal/camera_sensor.h"
> #include "libcamera/internal/device_enumerator.h"
> diff --git a/test/ipa/meson.build b/test/ipa/meson.build
> index ba672f3f..b25bfcf4 100644
> --- a/test/ipa/meson.build
> +++ b/test/ipa/meson.build
> @@ -9,8 +9,8 @@ ipa_test = [
> foreach t : ipa_test
> exe = executable(t[0], t[1],
> dependencies : libcamera_dep,
> - link_with : [libipa, test_libraries],
> - include_directories : [libipa_includes, test_includes_internal])
> + link_with : test_libraries,
> + include_directories : test_includes_internal)
>
> test(t[0], exe, suite : 'ipa')
> endforeach
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list