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

[paul.elder at ideasonboard.com]

Hi Laurent,

On Fri, Feb 12, 2021 at 04:46:32AM +0200, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> 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?

Not sure which is best.

Paul