[{"id":15122,"web_url":"https://patchwork.libcamera.org/comment/15122/","msgid":"","date":"2021-02-12T02:46:32","subject":"Re: [libcamera-devel] [PATCH v7 10/10] ipa: Add core.mojom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Feb 11, 2021 at 04:18:05PM +0900, Paul Elder wrote:\n> Add a base mojom file to contain empty mojom definitions of libcamera\n> objects, as well as documentation for the IPA interfaces that need to be\n> defined in the mojom files.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> Changes in v7:\n> - update documentation and code for the switch from genHeader/genSerdes to\n> skipHeader/skipSerdes\n> - rebase on new CameraSensorInfo\n> - other cosmetic changes\n> \n> Changes in v6:\n> - expand documentation on what can and can't be done in mojom\n> - add definitions for geometry.h structs, and the structs that used to\n> be in ipa_interface.h, including their documentation\n> - remove documentation for start()\n> \n> Changes in v5:\n> - add todo for defining some libcamera ipa structs in mojom\n> - remove ipa_mojom_core from dependencies of everything in the\n> generator stage\n> - add documentation for the base IPA functions (init, stop, start)\n> \n> Changes in v4:\n> - move docs to IPA guide\n> \n> Changes in v3:\n> - add doc that structs need to be defined\n> - add doc to recommend namespacing\n> - change indentation\n> - add requirement that base controls *must* be defined in\n> libcamera::{pipeline_name}::Controls\n> \n> New in v2\n> ---\n> include/libcamera/ipa/core.mojom | 215 +++++++++++++++++++++++++++++++\n> 1 file changed, 215 insertions(+)\n> create mode 100644 include/libcamera/ipa/core.mojom\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> new file mode 100644\n> index 00000000..353eee95\n> --- /dev/null\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -0,0 +1,215 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +\n> +/*\n> + * Things that can be defined here (and in other mojom files):\n> + * - consts\n> + * - enums\n> + * - structs\n> + *\n> + * Attributes:\n> + * - skipHeader - structs only, and only in core.mojom\n> + * - designate that this struct shall not have a C++ header definition\n> + * generated\n> + * - skipSerdes - structs only, and only in core.mojom\n> + * - designate that this struct shall not have a (de)serializer generated\n> + * - all fields need a (de)serializer to be defined, either hand-written\n> + * in ipa_data_serializer.h\n> + * - hasFd - struct fields or empty structs only\n> + * - designate that this field or empty struct contains a FileDescriptor\n> + *\n> + * Rules:\n> + * - Any struct that is used in a struct definition in mojom must also be\n> + * defined in mojom\n> + * - If the struct has both a definition in a C++ header and a (de)serializer\n> + * in ipa_data_serializer.h, then the struct shall be declared as empty,\n> + * with both the [skipHeader] and [skipSerdes] attributes\n> + * - If the struct only has a definition in a C++ header, but no\n> + * (de)serializer, then the struct definition should have the [skipHeader]\n> + * attribute\n> + * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.\n> + * - Avoid them, by defining them in a header in C++ and a (de)serializer in\n> + * ipa_data_serializer.h\n> + * - If a struct is in an array/map inside a struct, then the struct that is\n> + * the member of the array/map does not need a mojom definition if it is\n> + * defined in a C++ header.\n> + * - This can be used to embed nested structures. The C++ double colon is\n> + * replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)\n> + * - The struct must still be defined in a header in C++ and a (de)serializer\n> + * implemented in ipa_data_serializer.h, as it cannot be defined in mojom\n> + * - [skipHeader] and [skipSerdes] only work here in core.mojom.\n> + * - If a struct definition has [skipHeader], then the header where the\n> + * struct is defined must be #included (or the struct forward-declared) in\n> + * ipa_interface.h\n> + * - If a field in a struct has a FileDescriptor, but is not explicitly\n> + * defined so in mojom, then the field must be marked with the [hasFd]\n> + * attribute.\n> + *\n> + * \\todo Generate documentation from Doxygen comments in .mojom files\n> + * \\todo Figure out how to keep the skipHeader structs in sync with their\n> + * C++ definitions, and the skipSerdes structs in sync with their\n> + * (de)serializers\n> + */\n> +[skipSerdes, skipHeader] struct ControlInfoMap {};\n> +[skipSerdes, skipHeader] struct ControlList {};\n> +[skipSerdes, skipHeader] struct FileDescriptor {};\n> +\n> +[skipHeader] struct Point {\n> +\tint32 x;\n> +\tint32 y;\n> +};\n> +\n> +[skipHeader] struct Size {\n> +\tuint32 width;\n> +\tuint32 height;\n> +};\n> +\n> +[skipHeader] struct SizeRange {\n> +\tSize min;\n> +\tSize max;\n> +\tuint32 hStep;\n> +\tuint32 vStep;\n> +};\n> +\n> +[skipHeader] struct Rectangle {\n> +\tint32 x;\n> +\tint32 y;\n> +\tuint32 width;\n> +\tuint32 height;\n> +};\n> +\n> +[skipHeader] struct CameraSensorInfo {\n> +\tstring model;\n> +\n> +\tuint32 bitsPerPixel;\n> +\n> +\tSize activeAreaSize;\n> +\tRectangle analogCrop;\n> +\tSize outputSize;\n> +\n> +\tuint64 pixelRate;\n> +\tuint32 lineLength;\n> +\n> +\tuint32 minFrameLength;\n> +\tuint32 maxFrameLength;\n> +};\n> +\n> +/**\n> + * \\struct IPABuffer\n> + * \\brief Buffer information for the IPA interface\n> + *\n> + * The IPABuffer structure associates buffer memory with a unique ID. It is\n> + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which\n> + * buffers will be identified by their ID in the IPA interface.\n> + */\n> +\n> +/**\n> + * \\var IPABuffer::id\n> + * \\brief The buffer unique ID\n> + *\n> + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs\n> + * are chosen by the pipeline handler to fulfil the following constraints:\n> + *\n> + * - IDs shall be positive integers different than zero\n> + * - IDs shall be unique among all mapped buffers\n> + *\n> + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are\n> + * freed and may be reused for new buffer mappings.\n> + */\n> +\n> +/**\n> + * \\var IPABuffer::planes\n> + * \\brief The buffer planes description\n> + *\n> + * Stores the dmabuf handle and length for each plane of the buffer.\n> + */\n> +\n> +struct IPABuffer {\n> +\tuint32 id;\n> +\t[hasFd] array planes;\n> +};\n> +\n> +/**\n> + * \\struct IPASettings\n> + * \\brief IPA interface initialization settings\n> + *\n> + * The IPASettings structure stores data passed to the IPAInterface::init()\n> + * function. The data contains settings that don't depend on a particular camera\n> + * or pipeline configuration and are valid for the whole life time of the IPA\n> + * interface.\n> + */\n> +\n> +/**\n> + * \\var IPASettings::configurationFile\n> + * \\brief The name of the IPA configuration file\n> + *\n> + * This field may be an empty string if the IPA doesn't require a configuration\n> + * file.\n> + */\n> +struct IPASettings {\n> +\tstring configurationFile;\n> +};\n> +\n> +/**\n> + * \\struct IPAStream\n> + * \\brief Stream configuration for the IPA interface\n> + *\n> + * The IPAStream structure stores stream configuration parameters needed by the\n> + * IPAInterface::configure() method. It mirrors the StreamConfiguration class\n> + * that is not suitable for this purpose due to not being serializable.\n> + */\n> +\n> +/**\n> + * \\var IPAStream::pixelFormat\n> + * \\brief The stream pixel format\n> + */\n> +\n> +/**\n> + * \\var IPAStream::size\n> + * \\brief The stream size in pixels\n> + */\n> +struct IPAStream {\n> +\tuint32 pixelFormat;\n> +\tSize size;\n> +};\n> +\n> +/**\n> + * \\class IPAInterface\n> + * \\brief C++ Interface for IPA implementation\n> + *\n> + * This pure virtual class defines a skeletal C++ API for IPA modules.\n> + * Specializations of this class must be defined in a mojom file in\n> + * include/libcamera/ipa/ (see the IPA Writers Guide for details\n> + * on how to do so).\n> + *\n> + * Due to process isolation all arguments to the IPAInterface methods and\n> + * signals may need to be transferred over IPC. The class thus uses serializable\n> + * data types only. The IPA C++ interface defines custom data structures that\n> + * mirror core libcamera structures when the latter are not suitable, such as\n> + * IPAStream to carry StreamConfiguration data.\n> + *\n> + * Custom data structures may also be defined in the mojom file, in which case\n> + * the (de)serialization will automatically be generated. If any other libcamera\n> + * structures are to be used as parameters, then a (de)serializer for them must\n> + * be implemented in IPADataSerializer.\n> + *\n> + * The pipeline handlers shall use the IPAManager to locate a compatible\n> + * IPAInterface. The interface may then be used to interact with the IPA module.\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::init()\n> + * \\brief Initialise the IPAInterface\n> + * \\param[in] settings The IPA initialization settings\n> + *\n> + * This function initializes the IPA interface. It shall be called before any\n> + * other function of the IPAInterface. The \\a settings carry initialization\n> + * parameters that are valid for the whole life time of the IPA interface.\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::stop()\n> + * \\brief Stop the IPA\n> + *\n> + * This method informs the IPA module that the camera is stopped. The IPA module\n> + * shall release resources prepared in start().\n> + */\n\nShould we keep the documentation of the IPAInterface base class in the\n.cpp file ? Otherwise we'll get a doxygen warning.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A9725BD162\n\tfor ;\n\tFri, 12 Feb 2021 02:47:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 331D56375D;\n\tFri, 12 Feb 2021 03:47:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C9995602FC\n\tfor ;\n\tFri, 12 Feb 2021 03:46:58 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3CE2B8B5;\n\tFri, 12 Feb 2021 03:46:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ag24cmPt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613098018;\n\tbh=nFofSpkOknpIx1tIWs1NdX2VvryHbmJYhh31Xg0X/D4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ag24cmPtQ+XqX9NjcoYKEXs07YgmL6jtn0No5RQKaBasFnFRgj5MF64r6caVX4W24\n\tRyzX7H9nSwa+8D15jOX/A04im8WtT0ZQwCPFjYSWv+SRvwicZhb3e0W2HLrikFBsse\n\tsU2DlEq4PHXPQkFCUjNZnIVuWUtInWL3fs9VbySw=","Date":"Fri, 12 Feb 2021 04:46:32 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210211071805.34963-1-paul.elder@ideasonboard.com>\n\t<20210211071805.34963-11-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210211071805.34963-11-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 10/10] ipa: Add core.mojom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15126,"web_url":"https://patchwork.libcamera.org/comment/15126/","msgid":"<20210212085412.GC45982@pyrite.rasen.tech>","date":"2021-02-12T08:54:12","subject":"Re: [libcamera-devel] [PATCH v7 10/10] ipa: Add core.mojom","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Feb 12, 2021 at 04:46:32AM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 11, 2021 at 04:18:05PM +0900, Paul Elder wrote:\n> > Add a base mojom file to contain empty mojom definitions of libcamera\n> > objects, as well as documentation for the IPA interfaces that need to be\n> > defined in the mojom files.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Niklas Söderlund \n> > Reviewed-by: Laurent Pinchart \n> > \n> > ---\n> > Changes in v7:\n> > - update documentation and code for the switch from genHeader/genSerdes to\n> > skipHeader/skipSerdes\n> > - rebase on new CameraSensorInfo\n> > - other cosmetic changes\n> > \n> > Changes in v6:\n> > - expand documentation on what can and can't be done in mojom\n> > - add definitions for geometry.h structs, and the structs that used to\n> > be in ipa_interface.h, including their documentation\n> > - remove documentation for start()\n> > \n> > Changes in v5:\n> > - add todo for defining some libcamera ipa structs in mojom\n> > - remove ipa_mojom_core from dependencies of everything in the\n> > generator stage\n> > - add documentation for the base IPA functions (init, stop, start)\n> > \n> > Changes in v4:\n> > - move docs to IPA guide\n> > \n> > Changes in v3:\n> > - add doc that structs need to be defined\n> > - add doc to recommend namespacing\n> > - change indentation\n> > - add requirement that base controls *must* be defined in\n> > libcamera::{pipeline_name}::Controls\n> > \n> > New in v2\n> > ---\n> > include/libcamera/ipa/core.mojom | 215 +++++++++++++++++++++++++++++++\n> > 1 file changed, 215 insertions(+)\n> > create mode 100644 include/libcamera/ipa/core.mojom\n> > \n> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > new file mode 100644\n> > index 00000000..353eee95\n> > --- /dev/null\n> > +++ b/include/libcamera/ipa/core.mojom\n> > @@ -0,0 +1,215 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +\n> > +/*\n> > + * Things that can be defined here (and in other mojom files):\n> > + * - consts\n> > + * - enums\n> > + * - structs\n> > + *\n> > + * Attributes:\n> > + * - skipHeader - structs only, and only in core.mojom\n> > + * - designate that this struct shall not have a C++ header definition\n> > + * generated\n> > + * - skipSerdes - structs only, and only in core.mojom\n> > + * - designate that this struct shall not have a (de)serializer generated\n> > + * - all fields need a (de)serializer to be defined, either hand-written\n> > + * in ipa_data_serializer.h\n> > + * - hasFd - struct fields or empty structs only\n> > + * - designate that this field or empty struct contains a FileDescriptor\n> > + *\n> > + * Rules:\n> > + * - Any struct that is used in a struct definition in mojom must also be\n> > + * defined in mojom\n> > + * - If the struct has both a definition in a C++ header and a (de)serializer\n> > + * in ipa_data_serializer.h, then the struct shall be declared as empty,\n> > + * with both the [skipHeader] and [skipSerdes] attributes\n> > + * - If the struct only has a definition in a C++ header, but no\n> > + * (de)serializer, then the struct definition should have the [skipHeader]\n> > + * attribute\n> > + * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.\n> > + * - Avoid them, by defining them in a header in C++ and a (de)serializer in\n> > + * ipa_data_serializer.h\n> > + * - If a struct is in an array/map inside a struct, then the struct that is\n> > + * the member of the array/map does not need a mojom definition if it is\n> > + * defined in a C++ header.\n> > + * - This can be used to embed nested structures. The C++ double colon is\n> > + * replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)\n> > + * - The struct must still be defined in a header in C++ and a (de)serializer\n> > + * implemented in ipa_data_serializer.h, as it cannot be defined in mojom\n> > + * - [skipHeader] and [skipSerdes] only work here in core.mojom.\n> > + * - If a struct definition has [skipHeader], then the header where the\n> > + * struct is defined must be #included (or the struct forward-declared) in\n> > + * ipa_interface.h\n> > + * - If a field in a struct has a FileDescriptor, but is not explicitly\n> > + * defined so in mojom, then the field must be marked with the [hasFd]\n> > + * attribute.\n> > + *\n> > + * \\todo Generate documentation from Doxygen comments in .mojom files\n> > + * \\todo Figure out how to keep the skipHeader structs in sync with their\n> > + * C++ definitions, and the skipSerdes structs in sync with their\n> > + * (de)serializers\n> > + */\n> > +[skipSerdes, skipHeader] struct ControlInfoMap {};\n> > +[skipSerdes, skipHeader] struct ControlList {};\n> > +[skipSerdes, skipHeader] struct FileDescriptor {};\n> > +\n> > +[skipHeader] struct Point {\n> > +\tint32 x;\n> > +\tint32 y;\n> > +};\n> > +\n> > +[skipHeader] struct Size {\n> > +\tuint32 width;\n> > +\tuint32 height;\n> > +};\n> > +\n> > +[skipHeader] struct SizeRange {\n> > +\tSize min;\n> > +\tSize max;\n> > +\tuint32 hStep;\n> > +\tuint32 vStep;\n> > +};\n> > +\n> > +[skipHeader] struct Rectangle {\n> > +\tint32 x;\n> > +\tint32 y;\n> > +\tuint32 width;\n> > +\tuint32 height;\n> > +};\n> > +\n> > +[skipHeader] struct CameraSensorInfo {\n> > +\tstring model;\n> > +\n> > +\tuint32 bitsPerPixel;\n> > +\n> > +\tSize activeAreaSize;\n> > +\tRectangle analogCrop;\n> > +\tSize outputSize;\n> > +\n> > +\tuint64 pixelRate;\n> > +\tuint32 lineLength;\n> > +\n> > +\tuint32 minFrameLength;\n> > +\tuint32 maxFrameLength;\n> > +};\n> > +\n> > +/**\n> > + * \\struct IPABuffer\n> > + * \\brief Buffer information for the IPA interface\n> > + *\n> > + * The IPABuffer structure associates buffer memory with a unique ID. It is\n> > + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which\n> > + * buffers will be identified by their ID in the IPA interface.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPABuffer::id\n> > + * \\brief The buffer unique ID\n> > + *\n> > + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs\n> > + * are chosen by the pipeline handler to fulfil the following constraints:\n> > + *\n> > + * - IDs shall be positive integers different than zero\n> > + * - IDs shall be unique among all mapped buffers\n> > + *\n> > + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are\n> > + * freed and may be reused for new buffer mappings.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPABuffer::planes\n> > + * \\brief The buffer planes description\n> > + *\n> > + * Stores the dmabuf handle and length for each plane of the buffer.\n> > + */\n> > +\n> > +struct IPABuffer {\n> > +\tuint32 id;\n> > +\t[hasFd] array planes;\n> > +};\n> > +\n> > +/**\n> > + * \\struct IPASettings\n> > + * \\brief IPA interface initialization settings\n> > + *\n> > + * The IPASettings structure stores data passed to the IPAInterface::init()\n> > + * function. The data contains settings that don't depend on a particular camera\n> > + * or pipeline configuration and are valid for the whole life time of the IPA\n> > + * interface.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPASettings::configurationFile\n> > + * \\brief The name of the IPA configuration file\n> > + *\n> > + * This field may be an empty string if the IPA doesn't require a configuration\n> > + * file.\n> > + */\n> > +struct IPASettings {\n> > +\tstring configurationFile;\n> > +};\n> > +\n> > +/**\n> > + * \\struct IPAStream\n> > + * \\brief Stream configuration for the IPA interface\n> > + *\n> > + * The IPAStream structure stores stream configuration parameters needed by the\n> > + * IPAInterface::configure() method. It mirrors the StreamConfiguration class\n> > + * that is not suitable for this purpose due to not being serializable.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAStream::pixelFormat\n> > + * \\brief The stream pixel format\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAStream::size\n> > + * \\brief The stream size in pixels\n> > + */\n> > +struct IPAStream {\n> > +\tuint32 pixelFormat;\n> > +\tSize size;\n> > +};\n> > +\n> > +/**\n> > + * \\class IPAInterface\n> > + * \\brief C++ Interface for IPA implementation\n> > + *\n> > + * This pure virtual class defines a skeletal C++ API for IPA modules.\n> > + * Specializations of this class must be defined in a mojom file in\n> > + * include/libcamera/ipa/ (see the IPA Writers Guide for details\n> > + * on how to do so).\n> > + *\n> > + * Due to process isolation all arguments to the IPAInterface methods and\n> > + * signals may need to be transferred over IPC. The class thus uses serializable\n> > + * data types only. The IPA C++ interface defines custom data structures that\n> > + * mirror core libcamera structures when the latter are not suitable, such as\n> > + * IPAStream to carry StreamConfiguration data.\n> > + *\n> > + * Custom data structures may also be defined in the mojom file, in which case\n> > + * the (de)serialization will automatically be generated. If any other libcamera\n> > + * structures are to be used as parameters, then a (de)serializer for them must\n> > + * be implemented in IPADataSerializer.\n> > + *\n> > + * The pipeline handlers shall use the IPAManager to locate a compatible\n> > + * IPAInterface. The interface may then be used to interact with the IPA module.\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAInterface::init()\n> > + * \\brief Initialise the IPAInterface\n> > + * \\param[in] settings The IPA initialization settings\n> > + *\n> > + * This function initializes the IPA interface. It shall be called before any\n> > + * other function of the IPAInterface. The \\a settings carry initialization\n> > + * parameters that are valid for the whole life time of the IPA interface.\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAInterface::stop()\n> > + * \\brief Stop the IPA\n> > + *\n> > + * This method informs the IPA module that the camera is stopped. The IPA module\n> > + * shall release resources prepared in start().\n> > + */\n> \n> Should we keep the documentation of the IPAInterface base class in the\n> .cpp file ? Otherwise we'll get a doxygen warning.\n\nYeah, but then we get a warning that IPAInterface::init() and\nIPAInterface::stop() don't exist :/\nEither way we get warnings. Maybe remove the docs for these two\nfunctions? Or we put the function docs here an the IPAInterface base\nclass docs in the cpp file?\n\nNot sure which is best.\n\nPaul","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A3960BD162\n\tfor ;\n\tFri, 12 Feb 2021 08:54:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AE9763762;\n\tFri, 12 Feb 2021 09:54:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3626E63760\n\tfor ;\n\tFri, 12 Feb 2021 09:54:21 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C96A8B5;\n\tFri, 12 Feb 2021 09:54:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DKwQRwiq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613120060;\n\tbh=syKqE0wUGry7aI12E5shVOm0WBachUu17SAakZ5hiJI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DKwQRwiqZ4YnkBoeOd8LDQiqZPpKK1d3A3OzfXnhzyCfGc9WE89kDy3JxHmRxa8nz\n\tITvRjeFB1yz3f/LQrLUyi0J1GQmtXaJtt9hlDQ/EiTvdnzGvRMm8MVudxHpSE3k2x1\n\tEz0mi70n756P6qbejhKnlvMFIOo3nNbLTDhJ7y14=","Date":"Fri, 12 Feb 2021 17:54:12 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210212085412.GC45982@pyrite.rasen.tech>","References":"<20210211071805.34963-1-paul.elder@ideasonboard.com>\n\t<20210211071805.34963-11-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 10/10] ipa: Add core.mojom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15136,"web_url":"https://patchwork.libcamera.org/comment/15136/","msgid":"","date":"2021-02-12T10:42:37","subject":"Re: [libcamera-devel] [PATCH v7 10/10] ipa: Add core.mojom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Fri, Feb 12, 2021 at 05:54:12PM +0900, paul.elder@ideasonboard.com wrote:\n> On Fri, Feb 12, 2021 at 04:46:32AM +0200, Laurent Pinchart wrote:\n> > On Thu, Feb 11, 2021 at 04:18:05PM +0900, Paul Elder wrote:\n> > > Add a base mojom file to contain empty mojom definitions of libcamera\n> > > objects, as well as documentation for the IPA interfaces that need to be\n> > > defined in the mojom files.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > Reviewed-by: Niklas Söderlund \n> > > Reviewed-by: Laurent Pinchart \n> > > \n> > > ---\n> > > Changes in v7:\n> > > - update documentation and code for the switch from genHeader/genSerdes to\n> > > skipHeader/skipSerdes\n> > > - rebase on new CameraSensorInfo\n> > > - other cosmetic changes\n> > > \n> > > Changes in v6:\n> > > - expand documentation on what can and can't be done in mojom\n> > > - add definitions for geometry.h structs, and the structs that used to\n> > > be in ipa_interface.h, including their documentation\n> > > - remove documentation for start()\n> > > \n> > > Changes in v5:\n> > > - add todo for defining some libcamera ipa structs in mojom\n> > > - remove ipa_mojom_core from dependencies of everything in the\n> > > generator stage\n> > > - add documentation for the base IPA functions (init, stop, start)\n> > > \n> > > Changes in v4:\n> > > - move docs to IPA guide\n> > > \n> > > Changes in v3:\n> > > - add doc that structs need to be defined\n> > > - add doc to recommend namespacing\n> > > - change indentation\n> > > - add requirement that base controls *must* be defined in\n> > > libcamera::{pipeline_name}::Controls\n> > > \n> > > New in v2\n> > > ---\n> > > include/libcamera/ipa/core.mojom | 215 +++++++++++++++++++++++++++++++\n> > > 1 file changed, 215 insertions(+)\n> > > create mode 100644 include/libcamera/ipa/core.mojom\n> > > \n> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > > new file mode 100644\n> > > index 00000000..353eee95\n> > > --- /dev/null\n> > > +++ b/include/libcamera/ipa/core.mojom\n> > > @@ -0,0 +1,215 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +\n> > > +/*\n> > > + * Things that can be defined here (and in other mojom files):\n> > > + * - consts\n> > > + * - enums\n> > > + * - structs\n> > > + *\n> > > + * Attributes:\n> > > + * - skipHeader - structs only, and only in core.mojom\n> > > + * - designate that this struct shall not have a C++ header definition\n> > > + * generated\n> > > + * - skipSerdes - structs only, and only in core.mojom\n> > > + * - designate that this struct shall not have a (de)serializer generated\n> > > + * - all fields need a (de)serializer to be defined, either hand-written\n> > > + * in ipa_data_serializer.h\n> > > + * - hasFd - struct fields or empty structs only\n> > > + * - designate that this field or empty struct contains a FileDescriptor\n> > > + *\n> > > + * Rules:\n> > > + * - Any struct that is used in a struct definition in mojom must also be\n> > > + * defined in mojom\n> > > + * - If the struct has both a definition in a C++ header and a (de)serializer\n> > > + * in ipa_data_serializer.h, then the struct shall be declared as empty,\n> > > + * with both the [skipHeader] and [skipSerdes] attributes\n> > > + * - If the struct only has a definition in a C++ header, but no\n> > > + * (de)serializer, then the struct definition should have the [skipHeader]\n> > > + * attribute\n> > > + * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.\n> > > + * - Avoid them, by defining them in a header in C++ and a (de)serializer in\n> > > + * ipa_data_serializer.h\n> > > + * - If a struct is in an array/map inside a struct, then the struct that is\n> > > + * the member of the array/map does not need a mojom definition if it is\n> > > + * defined in a C++ header.\n> > > + * - This can be used to embed nested structures. The C++ double colon is\n> > > + * replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)\n> > > + * - The struct must still be defined in a header in C++ and a (de)serializer\n> > > + * implemented in ipa_data_serializer.h, as it cannot be defined in mojom\n> > > + * - [skipHeader] and [skipSerdes] only work here in core.mojom.\n> > > + * - If a struct definition has [skipHeader], then the header where the\n> > > + * struct is defined must be #included (or the struct forward-declared) in\n> > > + * ipa_interface.h\n> > > + * - If a field in a struct has a FileDescriptor, but is not explicitly\n> > > + * defined so in mojom, then the field must be marked with the [hasFd]\n> > > + * attribute.\n> > > + *\n> > > + * \\todo Generate documentation from Doxygen comments in .mojom files\n> > > + * \\todo Figure out how to keep the skipHeader structs in sync with their\n> > > + * C++ definitions, and the skipSerdes structs in sync with their\n> > > + * (de)serializers\n> > > + */\n> > > +[skipSerdes, skipHeader] struct ControlInfoMap {};\n> > > +[skipSerdes, skipHeader] struct ControlList {};\n> > > +[skipSerdes, skipHeader] struct FileDescriptor {};\n> > > +\n> > > +[skipHeader] struct Point {\n> > > +\tint32 x;\n> > > +\tint32 y;\n> > > +};\n> > > +\n> > > +[skipHeader] struct Size {\n> > > +\tuint32 width;\n> > > +\tuint32 height;\n> > > +};\n> > > +\n> > > +[skipHeader] struct SizeRange {\n> > > +\tSize min;\n> > > +\tSize max;\n> > > +\tuint32 hStep;\n> > > +\tuint32 vStep;\n> > > +};\n> > > +\n> > > +[skipHeader] struct Rectangle {\n> > > +\tint32 x;\n> > > +\tint32 y;\n> > > +\tuint32 width;\n> > > +\tuint32 height;\n> > > +};\n> > > +\n> > > +[skipHeader] struct CameraSensorInfo {\n> > > +\tstring model;\n> > > +\n> > > +\tuint32 bitsPerPixel;\n> > > +\n> > > +\tSize activeAreaSize;\n> > > +\tRectangle analogCrop;\n> > > +\tSize outputSize;\n> > > +\n> > > +\tuint64 pixelRate;\n> > > +\tuint32 lineLength;\n> > > +\n> > > +\tuint32 minFrameLength;\n> > > +\tuint32 maxFrameLength;\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\struct IPABuffer\n> > > + * \\brief Buffer information for the IPA interface\n> > > + *\n> > > + * The IPABuffer structure associates buffer memory with a unique ID. It is\n> > > + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which\n> > > + * buffers will be identified by their ID in the IPA interface.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPABuffer::id\n> > > + * \\brief The buffer unique ID\n> > > + *\n> > > + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs\n> > > + * are chosen by the pipeline handler to fulfil the following constraints:\n> > > + *\n> > > + * - IDs shall be positive integers different than zero\n> > > + * - IDs shall be unique among all mapped buffers\n> > > + *\n> > > + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are\n> > > + * freed and may be reused for new buffer mappings.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPABuffer::planes\n> > > + * \\brief The buffer planes description\n> > > + *\n> > > + * Stores the dmabuf handle and length for each plane of the buffer.\n> > > + */\n> > > +\n> > > +struct IPABuffer {\n> > > +\tuint32 id;\n> > > +\t[hasFd] array planes;\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\struct IPASettings\n> > > + * \\brief IPA interface initialization settings\n> > > + *\n> > > + * The IPASettings structure stores data passed to the IPAInterface::init()\n> > > + * function. The data contains settings that don't depend on a particular camera\n> > > + * or pipeline configuration and are valid for the whole life time of the IPA\n> > > + * interface.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPASettings::configurationFile\n> > > + * \\brief The name of the IPA configuration file\n> > > + *\n> > > + * This field may be an empty string if the IPA doesn't require a configuration\n> > > + * file.\n> > > + */\n> > > +struct IPASettings {\n> > > +\tstring configurationFile;\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\struct IPAStream\n> > > + * \\brief Stream configuration for the IPA interface\n> > > + *\n> > > + * The IPAStream structure stores stream configuration parameters needed by the\n> > > + * IPAInterface::configure() method. It mirrors the StreamConfiguration class\n> > > + * that is not suitable for this purpose due to not being serializable.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAStream::pixelFormat\n> > > + * \\brief The stream pixel format\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAStream::size\n> > > + * \\brief The stream size in pixels\n> > > + */\n> > > +struct IPAStream {\n> > > +\tuint32 pixelFormat;\n> > > +\tSize size;\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\class IPAInterface\n> > > + * \\brief C++ Interface for IPA implementation\n> > > + *\n> > > + * This pure virtual class defines a skeletal C++ API for IPA modules.\n> > > + * Specializations of this class must be defined in a mojom file in\n> > > + * include/libcamera/ipa/ (see the IPA Writers Guide for details\n> > > + * on how to do so).\n> > > + *\n> > > + * Due to process isolation all arguments to the IPAInterface methods and\n> > > + * signals may need to be transferred over IPC. The class thus uses serializable\n> > > + * data types only. The IPA C++ interface defines custom data structures that\n> > > + * mirror core libcamera structures when the latter are not suitable, such as\n> > > + * IPAStream to carry StreamConfiguration data.\n> > > + *\n> > > + * Custom data structures may also be defined in the mojom file, in which case\n> > > + * the (de)serialization will automatically be generated. If any other libcamera\n> > > + * structures are to be used as parameters, then a (de)serializer for them must\n> > > + * be implemented in IPADataSerializer.\n> > > + *\n> > > + * The pipeline handlers shall use the IPAManager to locate a compatible\n> > > + * IPAInterface. The interface may then be used to interact with the IPA module.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAInterface::init()\n> > > + * \\brief Initialise the IPAInterface\n> > > + * \\param[in] settings The IPA initialization settings\n> > > + *\n> > > + * This function initializes the IPA interface. It shall be called before any\n> > > + * other function of the IPAInterface. The \\a settings carry initialization\n> > > + * parameters that are valid for the whole life time of the IPA interface.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAInterface::stop()\n> > > + * \\brief Stop the IPA\n> > > + *\n> > > + * This method informs the IPA module that the camera is stopped. The IPA module\n> > > + * shall release resources prepared in start().\n> > > + */\n> > \n> > Should we keep the documentation of the IPAInterface base class in the\n> > .cpp file ? Otherwise we'll get a doxygen warning.\n> \n> Yeah, but then we get a warning that IPAInterface::init() and\n> IPAInterface::stop() don't exist :/\n> Either way we get warnings. Maybe remove the docs for these two\n> functions? Or we put the function docs here an the IPAInterface base\n> class docs in the cpp file?\n\nThe latter sounds best to me for now.\n\n> Not sure which is best.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AD6A5BD160\n\tfor ;\n\tFri, 12 Feb 2021 10:43:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E8CE63773;\n\tFri, 12 Feb 2021 11:43:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D31B363762\n\tfor ;\n\tFri, 12 Feb 2021 11:43:04 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D54B78B5;\n\tFri, 12 Feb 2021 11:43:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jxfMd17u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613126584;\n\tbh=HznWL0qsf18BVeW/VNZYX2N8295fZL9z19pW3pD9dvE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jxfMd17uQInNvcGXYWR+DtR498xhqZCbuYQToq308msimCD6EVwA05wQz1cjN5lmu\n\tFIZQyeZEFYmQ536xpXb7M13hxCqQ6z5ehTg5tiCCui8jHR49EHCtYRMwEHBQJG/cH+\n\tjhyJhoIotNLPsIj8uI8OpXrpaqQgyLJrVTq2s8hc=","Date":"Fri, 12 Feb 2021 12:42:37 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210211071805.34963-1-paul.elder@ideasonboard.com>\n\t<20210211071805.34963-11-paul.elder@ideasonboard.com>\n\t\n\t<20210212085412.GC45982@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210212085412.GC45982@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v7 10/10] ipa: Add core.mojom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]