[libcamera-devel] [PATCH v7 10/10] ipa: Add core.mojom

[Laurent Pinchart]

Hi Paul,

On Fri, Feb 12, 2021 at 05:54:12PM +0900, paul.elder at ideasonboard.com wrote:
> On Fri, Feb 12, 2021 at 04:46:32AM +0200, Laurent Pinchart wrote:
> > On Thu, Feb 11, 2021 at 04:18:05PM +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>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > 
> > > ---
> > > Changes in v7:
> > > - update documentation and code for the switch from genHeader/genSerdes to
> > >   skipHeader/skipSerdes
> > > - rebase on new CameraSensorInfo
> > > - other cosmetic changes
> > > 
> > > 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 | 215 +++++++++++++++++++++++++++++++
> > >  1 file changed, 215 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..353eee95
> > > --- /dev/null
> > > +++ b/include/libcamera/ipa/core.mojom
> > > @@ -0,0 +1,215 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +
> > > +/*
> > > + * Things that can be defined here (and in other mojom files):
> > > + * - consts
> > > + * - enums
> > > + * - structs
> > > + *
> > > + * Attributes:
> > > + * - skipHeader - structs only, and only in core.mojom
> > > + *   - designate that this struct shall not have a C++ header definition
> > > + *     generated
> > > + * - skipSerdes - structs only, and only in core.mojom
> > > + *   - designate that this struct shall not have a (de)serializer generated
> > > + *   - all fields need a (de)serializer to be defined, either hand-written
> > > + *     in ipa_data_serializer.h
> > > + * - 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,
> > > + *     with both the [skipHeader] and [skipSerdes] attributes
> > > + *   - If the struct only has a definition in a C++ header, but no
> > > + *     (de)serializer, then the struct definition should have the [skipHeader]
> > > + *     attribute
> > > + * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.
> > > + *   - 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 if it is
> > > + *   defined in a C++ header.
> > > + *   - This can be used to embed nested structures. The C++ double colon is
> > > + *     replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)
> > > + *   - 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
> > > + * - [skipHeader] and [skipSerdes] only work here in core.mojom.
> > > + * - If a struct definition has [skipHeader], 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.
> > > + *
> > > + * \todo Generate documentation from Doxygen comments in .mojom files
> > > + * \todo Figure out how to keep the skipHeader structs in sync with their
> > > + * C++ definitions, and the skipSerdes structs in sync with their
> > > + * (de)serializers
> > > + */
> > > +[skipSerdes, skipHeader] struct ControlInfoMap {};
> > > +[skipSerdes, skipHeader] struct ControlList {};
> > > +[skipSerdes, skipHeader] struct FileDescriptor {};
> > > +
> > > +[skipHeader] struct Point {
> > > +	int32 x;
> > > +	int32 y;
> > > +};
> > > +
> > > +[skipHeader] struct Size {
> > > +	uint32 width;
> > > +	uint32 height;
> > > +};
> > > +
> > > +[skipHeader] struct SizeRange {
> > > +	Size min;
> > > +	Size max;
> > > +	uint32 hStep;
> > > +	uint32 vStep;
> > > +};
> > > +
> > > +[skipHeader] struct Rectangle {
> > > +	int32 x;
> > > +	int32 y;
> > > +	uint32 width;
> > > +	uint32 height;
> > > +};
> > > +
> > > +[skipHeader] struct CameraSensorInfo {
> > > +	string model;
> > > +
> > > +	uint32 bitsPerPixel;
> > > +
> > > +	Size activeAreaSize;
> > > +	Rectangle analogCrop;
> > > +	Size outputSize;
> > > +
> > > +	uint64 pixelRate;
> > > +	uint32 lineLength;
> > > +
> > > +	uint32 minFrameLength;
> > > +	uint32 maxFrameLength;
> > > +};
> > > +
> > > +/**
> > > + * \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.
> > > + */
> > > +
> > > +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.
> > > + */
> > > +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
> > > + */
> > > +struct IPAStream {
> > > +	uint32 pixelFormat;
> > > +	Size size;
> > > +};
> > > +
> > > +/**
> > > + * \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
> > > + * be implemented in IPADataSerializer.
> > > + *
> > > + * The pipeline handlers shall use the IPAManager to locate a compatible
> > > + * IPAInterface. The interface may then be used to interact with the IPA module.
> > > + */
> > > +
> > > +/**
> > > + * \fn 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 IPAInterface::stop()
> > > + * \brief Stop the IPA
> > > + *
> > > + * This method informs the IPA module that the camera is stopped. The IPA module
> > > + * shall release resources prepared in start().
> > > + */
> > 
> > Should we keep the documentation of the IPAInterface base class in the
> > .cpp file ? Otherwise we'll get a doxygen warning.
> 
> Yeah, but then we get a warning that IPAInterface::init() and
> IPAInterface::stop() don't exist :/
> Either way we get warnings. Maybe remove the docs for these two
> functions? Or we put the function docs here an the IPAInterface base
> class docs in the cpp file?

The latter sounds best to me for now.

> Not sure which is best.

-- 
Regards,

Laurent Pinchart