[libcamera-devel] [PATCH v6 9/9] ipa: Add core.mojom

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Dec 24, 2020 at 05:15:34PM +0900, Paul Elder wrote:
> Add a base mojom file to contain empty mojom definitions of libcamera
> objects, as well as documentation for the IPA interfaces that need to be
> defined in the mojom files.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> ---
> Changes in v6:
> - expand documentation on what can and can't be done in mojom
> - add definitions for geometry.h structs, and the structs that used to
>   be in ipa_interface.h, including their documentation
> - remove documentation for start()
> 
> Changes in v5:
> - add todo for defining some libcamera ipa structs in mojom
> - remove ipa_mojom_core from dependencies of everything in the
>   generator stage
> - add documentation for the base IPA functions (init, stop, start)
> 
> Changes in v4:
> - move docs to IPA guide
> 
> Changes in v3:
> - add doc that structs need to be defined
> - add doc to recommend namespacing
> - change indentation
> - add requirement that base controls *must* be defined in
>   libcamera::{pipeline_name}::Controls
> 
> New in v2
> ---
>  include/libcamera/ipa/core.mojom | 208 +++++++++++++++++++++++++++++++
>  1 file changed, 208 insertions(+)
>  create mode 100644 include/libcamera/ipa/core.mojom
> 
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> new file mode 100644
> index 00000000..d707508b
> --- /dev/null
> +++ b/include/libcamera/ipa/core.mojom
> @@ -0,0 +1,208 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +/*
> + * Things that can be defined here (and in other mojom files):
> + * - consts
> + * - enums
> + * - structs
> + *
> + * Attributes:
> + * - genHeader - structs only
> + *   - designate that this struct needs a C++ header definition to be generated
> + *   - not necessary if this struct is already defined in a C++ header
> + * - genSerdes - structs only
> + *   - designate that this struct needs a (de)serializer to be generated

Would it make sense to do it the other way around, having an attribute
to skip generation of the serdes ? All but three structures in this
files set the genSerdes attribute.

I'm also tempted to do the same for genHeader, to mark the structures
that already have a C++ version. Up to you.

> + *   - all fields need a (de)serializer to be defined, either hand-written
> + *     in ipa_data_serializer.h, or designated here with genSerdes

Should you document that those two attributes are only valid in
core.mojom, and ignored in other files ?

> + * - hasFd - struct fields or empty structs only
> + *   - designate that this field or empty struct contains a FileDescriptor
> + *
> + * Rules:
> + * - Any struct that is used in a struct definition in mojom must also be
> + *   defined in mojom
> + *   - If the struct has both a definition in a C++ header and a (de)serializer
> + *     in ipa_data_serializer.h, then the struct shall be declared as empty
> + *   - If the struct only has a definition in a C++ header, but no
> + *     (de)serializer, then the struct definition should have the [genSerdes]
> + *     attribute
> + *   - If the struct has neither a definition in a C++ header nor a
> + *     (de)serializer, then the struct definition should have both the
> + *     [genHeader] and [genSerdes] attributes
> + * - Nested structures (eg. FrameBuffer::Plane) cannot be defined in mojom.

s/eg./e.g./

> + *   - Avoid them, by defining them in a header in C++ and a (de)serializer in
> + *     ipa_data_serializer.h
> + * - If a struct is in an array/map inside a struct, then the struct that is
> + *   the member of the array/map does not need a mojom definition.

s/definition./definition if it is defined in a C++ header./ ?

> + *   - This can be used to embed nested structures. The C++ double dolon is

s/dolon/colon/

> + *     replaced with a dot (eg. FrameBuffer::Plane -> FrameBuffer.Plane)

s/eg./e.g./

> + *   - The struct must still be defined in a header in C++ and a (de)serializer
> + *     implemented in ipa_data_serializer.h, as it cannot be defined in mojom
> + * - [genHeader] and [genSerdes] only work here in core.mojom. Any struct defined
> + *   in other mojom files will implicitly have both attributes.

Ah here we go :-) Maybe this should be moved above ?

> + * - If a struct definition does not have genHeader, then the header where the
> + *   struct is defined must be #included (or the struct forward-declared) in
> + *   ipa_interface.h
> + * - If a field in a struct has a FileDescriptor, but is not explicitly
> + *   defined so in mojom, then the field must be marked with the [hasFd]
> + *   attribute.
> + */
> +struct ControlInfoMap {};
> +struct ControlList {};
> +struct FileDescriptor {};
> +
> +[genSerdes] struct Point {
> +	int32 x;
> +	int32 y;
> +};
> +
> +[genSerdes] struct Size {
> +	uint32 width;
> +	uint32 height;
> +};
> +
> +[genSerdes] struct SizeRange {
> +	Size min;
> +	Size max;
> +	uint32 hStep;
> +	uint32 vStep;
> +};
> +
> +[genSerdes] struct Rectangle {
> +	int32 x;
> +	int32 y;
> +	uint32 width;
> +	uint32 height;
> +};
> +
> +[genSerdes] struct CameraSensorInfo {
> +	string model;
> +
> +	uint32 bitsPerPixel;
> +
> +	Size activeAreaSize;
> +	Rectangle analogCrop;
> +	Size outputSize;
> +
> +	uint64 pixelRate;
> +	uint32 lineLength;
> +};
> +
> +/**
> + * \struct IPABuffer
> + * \brief Buffer information for the IPA interface
> + *
> + * The IPABuffer structure associates buffer memory with a unique ID. It is
> + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which
> + * buffers will be identified by their ID in the IPA interface.
> + */
> +
> +/**
> + * \var IPABuffer::id
> + * \brief The buffer unique ID
> + *
> + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs
> + * are chosen by the pipeline handler to fulfil the following constraints:
> + *
> + * - IDs shall be positive integers different than zero
> + * - IDs shall be unique among all mapped buffers
> + *
> + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are
> + * freed and may be reused for new buffer mappings.
> + */
> +
> +/**
> + * \var IPABuffer::planes
> + * \brief The buffer planes description
> + *
> + * Stores the dmabuf handle and length for each plane of the buffer.
> + */
> +
> +/**
> + * \class IPAInterface
> + * \brief C++ Interface for IPA implementation
> + *
> + * This pure virtual class defines a skeletal C++ API for IPA modules.
> + * Specializations of this class must be defined in a mojom file in
> + * include/libcamera/ipa/ (see the IPA Writers Guide for details
> + * on how to do so).
> + *
> + * Due to process isolation all arguments to the IPAInterface methods and
> + * signals may need to be transferred over IPC. The class thus uses serializable
> + * data types only. The IPA C++ interface defines custom data structures that
> + * mirror core libcamera structures when the latter are not suitable, such as
> + * IPAStream to carry StreamConfiguration data.
> + *
> + * Custom data structures may also be defined in the mojom file, in which case
> + * the (de)serialization will automatically be generated. If any other libcamera
> + * structures are to be used as parameters, then a de/serializer for them must

s/de\/(de)/ ?

> + * be implemented in IPADataSerializer.
> + *
> + * The pipeline handler shall use the IPAManager to locate a compatible

s/handler/handlers/

> + * IPAInterface. The interface may then be used to interact with the IPA module.
> + */

I'd move this to the end of the file, before the init() and stop()
functions.

> +[genHeader, genSerdes] struct IPABuffer {
> +	uint32 id;
> +	[hasFd] array<FrameBuffer.Plane> planes;
> +};
> +
> +/**
> + * \struct IPASettings
> + * \brief IPA interface initialization settings
> + *
> + * The IPASettings structure stores data passed to the IPAInterface::init()
> + * function. The data contains settings that don't depend on a particular camera
> + * or pipeline configuration and are valid for the whole life time of the IPA
> + * interface.
> + */
> +
> +/**
> + * \var IPASettings::configurationFile
> + * \brief The name of the IPA configuration file
> + *
> + * This field may be an empty string if the IPA doesn't require a configuration
> + * file.
> + */
> +[genHeader, genSerdes] struct IPASettings {
> +	string configurationFile;
> +};
> +
> +/**
> + * \struct IPAStream
> + * \brief Stream configuration for the IPA interface
> + *
> + * The IPAStream structure stores stream configuration parameters needed by the
> + * IPAInterface::configure() method. It mirrors the StreamConfiguration class
> + * that is not suitable for this purpose due to not being serializable.
> + */
> +
> +/**
> + * \var IPAStream::pixelFormat
> + * \brief The stream pixel format
> + */
> +
> +/**
> + * \var IPAStream::size
> + * \brief The stream size in pixels
> + */
> +[genHeader, genSerdes] struct IPAStream {
> +	uint32 pixelFormat;
> +	Size size;
> +};
> +
> +/**
> + * \fn init()

s/init/IPAInterface::init/

> + * \brief Initialise the IPAInterface
> + * \param[in] settings The IPA initialization settings
> + *
> + * This function initializes the IPA interface. It shall be called before any
> + * other function of the IPAInterface. The \a settings carry initialization
> + * parameters that are valid for the whole life time of the IPA interface.
> + */
> +
> +/**
> + * \fn stop()

Same here.

> + * \brief Stop the IPA
> + *
> + * This method informs the IPA module that the camera is stopped. The IPA module
> + * shall release resources prepared in start().
> + */

I wonder where to store this. We can't document those two functions in
ipa_interface.cpp, as the base IPAInterface class doesn't define init()
and stop() functions. We can fix this later.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

-- 
Regards,

Laurent Pinchart