[libcamera-devel,v2,00/24] Control serialization and IPA C API
mbox series

Message ID 20191108205409.18845-1-laurent.pinchart@ideasonboard.com
Headers show
Series
  • Control serialization and IPA C API
Related show

Message

Laurent Pinchart Nov. 8, 2019, 8:53 p.m. UTC
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

Comments

Kieran Bingham Nov. 19, 2019, 5:06 p.m. UTC | #1
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
>