| Message ID | 20191108205409.18845-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, As far as I can see, there are only a few minor comments across this series, and has been reviewed extensively enough to say lets get this in, with the minors resolved. A topic such as this, which is so intrinsic to the rest of the system will only block further development, so I think it's important that we get this in soon so that development and testing can continue on top. Thanks for tackling such a large series. -- Kieran On 08/11/2019 20:53, Laurent Pinchart wrote: > Hello, > > This patch series turns the IPA API towards IPA modules to a plain C > API. The rationale is explained in patch 19/24: > > 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, define a plain > C IPA API. The corresponding C objects are stored in plain C structures > or have their binary format documented, removing the need for linking to > libcamera code on the IPA side. > > Patches 01/24 to 12/24 are preparatory changes. Patches 01/24 to 03/24 > add a test case for and fix a bug in the ControlInfoMap. Patches 04/24 > to 12/24 then rework and expand the BufferMemory and control classes to > make translation between the C++ and C API possible. > > A plain C API requires storing all data passed to IPA context operations > in plain C data structures. For most of the data used in the > IPAInterface methods, this only requires defining corresponding C > structures and replacing std::vector and std::map with arrays. However, > for ControlInfoMap and ControlList, a different approach is needed due > to their complexity. > > Patch 13/24 defines a serialization format for those two classes, and > patches 14/24 to 17/24 implement serialization and deserialization > (including a test case). > > Patches 18/24 to 20/24 then define a plain C IPA API and switch to it. > Patch 21/24 is a small improvement to compile time test coverage related > to IPA modules, and patch 22/24 allows short-circuiting the C API for > IPA modules that are loaded without isolation. Patch 23/24 adds a test > case, and patch 24/24 finally fixes a typo. > > I would like to thank both Kieran and Jacopo for all their preparatory > work on this. We all spend lots of time burning brain cells, and > hopefully the result will make everybody happy. > > Jacopo Mondi (6): > libcamera: controls: Make ControlId constructor public > libcamera: controls: Store reference to the InfoMap > ipa: Define serialized controls > libcamera: Add ByteStreamBuffer > test: Add control serialization test > ipa: Switch to the plain C API > > Laurent Pinchart (18): > test: Extract CameraTest class out of camera tests to libtest > test: controls: Add ControlInfoMap test > libcamera: controls: Avoid exception in ControlList count() and find() > libcamera: controls: Add operator== and operator!= to ControlRange > libcamera: controls: Index ControlList by unsigned int > libcamera: controls: Add move constructor to ControlInfoMap > libcamera: controls: Make ControList constructor public > libcamera: controls: Catch type mismatch in ControlInfoMap > libcamera: v4l2_controls: Fix control range construction for bool > libcamera: buffer: Add const accessor to Buffer planes > test: Add ByteStreamBuffer test > libcamera: Add controls serializer > ipa: Pass ControlInfoMap references to IPAInterface::configure() > ipa: Define a plain C API > ipa: Declare the ipaCreate() function prototype > ipa: Allow short-circuiting the ipa_context_ops > test: ipa: Add IPA wrappers test > libcamera: Fix typo related to serialization > > Documentation/Doxyfile.in | 1 + > Documentation/meson.build | 2 + > include/ipa/ipa_controls.h | 54 ++ > include/ipa/ipa_interface.h | 82 ++- > include/ipa/meson.build | 1 + > include/libcamera/buffer.h | 1 + > include/libcamera/controls.h | 50 +- > src/ipa/ipa_vimc.cpp | 9 +- > src/ipa/libipa/ipa_interface_wrapper.cpp | 248 +++++++++ > src/ipa/libipa/ipa_interface_wrapper.h | 57 ++ > src/ipa/libipa/meson.build | 13 + > src/ipa/meson.build | 3 + > src/ipa/rkisp1/meson.build | 3 +- > src/ipa/rkisp1/rkisp1.cpp | 9 +- > src/libcamera/buffer.cpp | 6 + > src/libcamera/byte_stream_buffer.cpp | 286 ++++++++++ > src/libcamera/camera_controls.cpp | 4 +- > src/libcamera/control_serializer.cpp | 501 ++++++++++++++++++ > src/libcamera/controls.cpp | 139 +++-- > src/libcamera/include/byte_stream_buffer.h | 63 +++ > src/libcamera/include/camera_controls.h | 2 +- > src/libcamera/include/control_serializer.h | 52 ++ > src/libcamera/include/control_validator.h | 2 +- > src/libcamera/include/ipa_context_wrapper.h | 47 ++ > src/libcamera/include/ipa_module.h | 5 +- > src/libcamera/include/meson.build | 3 + > src/libcamera/ipa_context_wrapper.cpp | 249 +++++++++ > src/libcamera/ipa_controls.cpp | 187 +++++++ > src/libcamera/ipa_interface.cpp | 325 ++++++++++-- > src/libcamera/ipa_manager.cpp | 67 ++- > src/libcamera/ipa_module.cpp | 23 +- > src/libcamera/meson.build | 4 + > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > src/libcamera/pipeline/uvcvideo.cpp | 4 +- > src/libcamera/pipeline/vimc.cpp | 4 +- > src/libcamera/proxy/ipa_proxy_linux.cpp | 2 +- > .../proxy/worker/ipa_proxy_linux_worker.cpp | 8 +- > src/libcamera/v4l2_controls.cpp | 14 +- > src/libcamera/v4l2_device.cpp | 23 +- > test/byte-stream-buffer.cpp | 172 ++++++ > test/camera/buffer_import.cpp | 14 +- > test/camera/capture.cpp | 15 +- > test/camera/configuration_default.cpp | 16 +- > test/camera/configuration_set.cpp | 15 +- > test/camera/meson.build | 2 +- > test/camera/statemachine.cpp | 15 +- > test/controls/control_info.cpp | 77 +++ > test/controls/control_list.cpp | 39 +- > test/controls/meson.build | 1 + > test/ipa/ipa_wrappers_test.cpp | 390 ++++++++++++++ > test/ipa/meson.build | 5 +- > test/{camera => libtest}/camera_test.cpp | 24 +- > test/{camera => libtest}/camera_test.h | 12 +- > test/libtest/meson.build | 1 + > test/meson.build | 2 + > test/serialization/control_serialization.cpp | 161 ++++++ > test/serialization/meson.build | 11 + > test/serialization/serialization_test.cpp | 89 ++++ > test/serialization/serialization_test.h | 33 ++ > 59 files changed, 3432 insertions(+), 217 deletions(-) > create mode 100644 include/ipa/ipa_controls.h > 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/byte_stream_buffer.cpp > create mode 100644 src/libcamera/control_serializer.cpp > create mode 100644 src/libcamera/include/byte_stream_buffer.h > create mode 100644 src/libcamera/include/control_serializer.h > create mode 100644 src/libcamera/include/ipa_context_wrapper.h > create mode 100644 src/libcamera/ipa_context_wrapper.cpp > create mode 100644 src/libcamera/ipa_controls.cpp > create mode 100644 test/byte-stream-buffer.cpp > create mode 100644 test/controls/control_info.cpp > create mode 100644 test/ipa/ipa_wrappers_test.cpp > rename test/{camera => libtest}/camera_test.cpp (55%) > rename test/{camera => libtest}/camera_test.h (77%) > create mode 100644 test/serialization/control_serialization.cpp > create mode 100644 test/serialization/meson.build > create mode 100644 test/serialization/serialization_test.cpp > create mode 100644 test/serialization/serialization_test.h >
Hello, This patch series turns the IPA API towards IPA modules to a plain C API. The rationale is explained in patch 19/24: 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, define a plain C IPA API. The corresponding C objects are stored in plain C structures or have their binary format documented, removing the need for linking to libcamera code on the IPA side. Patches 01/24 to 12/24 are preparatory changes. Patches 01/24 to 03/24 add a test case for and fix a bug in the ControlInfoMap. Patches 04/24 to 12/24 then rework and expand the BufferMemory and control classes to make translation between the C++ and C API possible. A plain C API requires storing all data passed to IPA context operations in plain C data structures. For most of the data used in the IPAInterface methods, this only requires defining corresponding C structures and replacing std::vector and std::map with arrays. However, for ControlInfoMap and ControlList, a different approach is needed due to their complexity. Patch 13/24 defines a serialization format for those two classes, and patches 14/24 to 17/24 implement serialization and deserialization (including a test case). Patches 18/24 to 20/24 then define a plain C IPA API and switch to it. Patch 21/24 is a small improvement to compile time test coverage related to IPA modules, and patch 22/24 allows short-circuiting the C API for IPA modules that are loaded without isolation. Patch 23/24 adds a test case, and patch 24/24 finally fixes a typo. I would like to thank both Kieran and Jacopo for all their preparatory work on this. We all spend lots of time burning brain cells, and hopefully the result will make everybody happy. Jacopo Mondi (6): libcamera: controls: Make ControlId constructor public libcamera: controls: Store reference to the InfoMap ipa: Define serialized controls libcamera: Add ByteStreamBuffer test: Add control serialization test ipa: Switch to the plain C API Laurent Pinchart (18): test: Extract CameraTest class out of camera tests to libtest test: controls: Add ControlInfoMap test libcamera: controls: Avoid exception in ControlList count() and find() libcamera: controls: Add operator== and operator!= to ControlRange libcamera: controls: Index ControlList by unsigned int libcamera: controls: Add move constructor to ControlInfoMap libcamera: controls: Make ControList constructor public libcamera: controls: Catch type mismatch in ControlInfoMap libcamera: v4l2_controls: Fix control range construction for bool libcamera: buffer: Add const accessor to Buffer planes test: Add ByteStreamBuffer test libcamera: Add controls serializer ipa: Pass ControlInfoMap references to IPAInterface::configure() ipa: Define a plain C API ipa: Declare the ipaCreate() function prototype ipa: Allow short-circuiting the ipa_context_ops test: ipa: Add IPA wrappers test libcamera: Fix typo related to serialization Documentation/Doxyfile.in | 1 + Documentation/meson.build | 2 + include/ipa/ipa_controls.h | 54 ++ include/ipa/ipa_interface.h | 82 ++- include/ipa/meson.build | 1 + include/libcamera/buffer.h | 1 + include/libcamera/controls.h | 50 +- src/ipa/ipa_vimc.cpp | 9 +- src/ipa/libipa/ipa_interface_wrapper.cpp | 248 +++++++++ src/ipa/libipa/ipa_interface_wrapper.h | 57 ++ src/ipa/libipa/meson.build | 13 + src/ipa/meson.build | 3 + src/ipa/rkisp1/meson.build | 3 +- src/ipa/rkisp1/rkisp1.cpp | 9 +- src/libcamera/buffer.cpp | 6 + src/libcamera/byte_stream_buffer.cpp | 286 ++++++++++ src/libcamera/camera_controls.cpp | 4 +- src/libcamera/control_serializer.cpp | 501 ++++++++++++++++++ src/libcamera/controls.cpp | 139 +++-- src/libcamera/include/byte_stream_buffer.h | 63 +++ src/libcamera/include/camera_controls.h | 2 +- src/libcamera/include/control_serializer.h | 52 ++ src/libcamera/include/control_validator.h | 2 +- src/libcamera/include/ipa_context_wrapper.h | 47 ++ src/libcamera/include/ipa_module.h | 5 +- src/libcamera/include/meson.build | 3 + src/libcamera/ipa_context_wrapper.cpp | 249 +++++++++ src/libcamera/ipa_controls.cpp | 187 +++++++ src/libcamera/ipa_interface.cpp | 325 ++++++++++-- src/libcamera/ipa_manager.cpp | 67 ++- src/libcamera/ipa_module.cpp | 23 +- src/libcamera/meson.build | 4 + src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- src/libcamera/pipeline/uvcvideo.cpp | 4 +- src/libcamera/pipeline/vimc.cpp | 4 +- src/libcamera/proxy/ipa_proxy_linux.cpp | 2 +- .../proxy/worker/ipa_proxy_linux_worker.cpp | 8 +- src/libcamera/v4l2_controls.cpp | 14 +- src/libcamera/v4l2_device.cpp | 23 +- test/byte-stream-buffer.cpp | 172 ++++++ test/camera/buffer_import.cpp | 14 +- test/camera/capture.cpp | 15 +- test/camera/configuration_default.cpp | 16 +- test/camera/configuration_set.cpp | 15 +- test/camera/meson.build | 2 +- test/camera/statemachine.cpp | 15 +- test/controls/control_info.cpp | 77 +++ test/controls/control_list.cpp | 39 +- test/controls/meson.build | 1 + test/ipa/ipa_wrappers_test.cpp | 390 ++++++++++++++ test/ipa/meson.build | 5 +- test/{camera => libtest}/camera_test.cpp | 24 +- test/{camera => libtest}/camera_test.h | 12 +- test/libtest/meson.build | 1 + test/meson.build | 2 + test/serialization/control_serialization.cpp | 161 ++++++ test/serialization/meson.build | 11 + test/serialization/serialization_test.cpp | 89 ++++ test/serialization/serialization_test.h | 33 ++ 59 files changed, 3432 insertions(+), 217 deletions(-) create mode 100644 include/ipa/ipa_controls.h 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/byte_stream_buffer.cpp create mode 100644 src/libcamera/control_serializer.cpp create mode 100644 src/libcamera/include/byte_stream_buffer.h create mode 100644 src/libcamera/include/control_serializer.h create mode 100644 src/libcamera/include/ipa_context_wrapper.h create mode 100644 src/libcamera/ipa_context_wrapper.cpp create mode 100644 src/libcamera/ipa_controls.cpp create mode 100644 test/byte-stream-buffer.cpp create mode 100644 test/controls/control_info.cpp create mode 100644 test/ipa/ipa_wrappers_test.cpp rename test/{camera => libtest}/camera_test.cpp (55%) rename test/{camera => libtest}/camera_test.h (77%) create mode 100644 test/serialization/control_serialization.cpp create mode 100644 test/serialization/meson.build create mode 100644 test/serialization/serialization_test.cpp create mode 100644 test/serialization/serialization_test.h