[{"id":18377,"web_url":"https://patchwork.libcamera.org/comment/18377/","msgid":"<20210727100042.GC2167@pyrite.rasen.tech>","date":"2021-07-27T10:00:42","subject":"Re: [libcamera-devel] [PATCH v4] ipa: core.mojom: Rework core file\n\tdocumentation","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Jul 27, 2021 at 09:50:23AM +0200, Jacopo Mondi wrote:\n> The comment block at the beginning of the core.mojom file is meant to\n> provide an overview of how to use libcamera defined types in the definition\n> of mojom interfaces.\n> \n> As the IPA/IPC interface definition mechanism evolved, the documentation\n> has not been updated accordingly.\n> \n> Update the file comments to match the most recent IPA/IPC\n> interface definition and generation mechanism.\n> \n> Signed-off-by: Jacopo Mondi \n\nLooks good!\n\nReviewed-by: Paul Elder \n\n> ---\n> v3->v4:\n> - Apply wording suggestions from Paul\n> \n> v2-v3:\n> - Address Paul's comment:\n> - s/[Mojo core|build system]/code generator/\n> - Swap points in skipSerdes description to make clear the flag instruct\n> the code generator not to generate a deserializer\n> - Clarify that nested types can *solely* be used as map/array members\n> \n> v1->v2:\n> - Address Paul's comment and clarify points that were not clear to me when I\n> wrote v1 :)\n> - (de)serializers implementations go in ipa_data_serializer.cpp\n> - types used as array/map members do not require a mojom definition\n> - remove duplications\n> - s/the library/libcamera\n> - While at it, make all statements start with a capital letter and end without a\n> full-stop.\n> ---\n> ---\n> include/libcamera/ipa/core.mojom | 68 +++++++++++++++++++-------------\n> 1 file changed, 41 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index b32f30939454..29ba35482b39 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -15,40 +15,54 @@ module libcamera;\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> + * - Do not generate a C++ definition for the structure\n> + * - Any type used in a mojom interface definition must have a corresponding\n> + * definition in a mojom file for the code generator to accept it, except\n> + * for types solely used as map/array members for which a definition is not\n> + * required\n> + * - This attribute allows defining a symbol for the code generator that\n> + * corresponds to a libcamera type without duplicating its definition in the\n> + * generated C++ headers\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> + * - All types need a (de)serializer to be defined in order to be transported\n> + * over IPC. The (de)serializer can be:\n> + * - Manually implemented as a template specialization in\n> + * ipa_data_serializer.cpp in the libcamera sources\n> + * - Generated at build time for types defined in a mojom file\n> + * - This attribute instructs the build system that a (de)serializer is\n> + * available for the type and there's no need to generate one\n> * - hasFd - struct fields or empty structs only\n> - * - designate that this field or empty struct contains a FileDescriptor\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> + * - If the type is defined in a libcamera C++ header *and* a (de)serializer is\n> + * available then the type shall be declared as empty with both attributes\n> + * associated and specified as: [skipHeader, skipSerdes]\n> + * - Example: [skipHeader, skipSerdes] ControlList {};\n> + * - If the type is defined in libcamera but no (de)serializer is available\n> + * then the type definition in the core.mojom file should have the\n> + * [skipHeader] attribute only\n> + * - A (de)serializer will be generated for the type\n> + * - If a type definition has [skipHeader], then the header where the type is\n> + * defined must be included in ipa_interface.h\n> + * - Types that are solely used as array/map members do not require a mojom\n> + * definition if one exists in libcamera\n> + * - Nested types (e.g. FrameBuffer::Plane) cannot be defined in mojom\n> + * - If used in mojom, the nested type shall be defined in a C++ header\n> + * and a (de)serializer shall be provided\n> + * - Nested types can only be used as array/map members\n> + * - When using the type, the C++ namespace separator :: is replaced with a\n> + * dot\n> + * - In example, to use the FrameBuffer::Plane type in mojom:\n> + * - Provide a definition of the FrameBuffer::Plane type in a C++ header\n> + * - Include the header in ipa_interface.h\n> + * - Provide a (de)serializer implementation ipa_data_serializer.cpp\n> + * - In mojom, reference the type as FrameBuffer.Plane and only as map/array\n> + * member\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 in 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> + * 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> --\n> 2.32.0\n>","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 A7CF0C322C\n\tfor ;\n\tTue, 27 Jul 2021 10:00:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D4A1687C1;\n\tTue, 27 Jul 2021 12:00:52 +0200 (CEST)","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 9BB0C687B6\n\tfor ;\n\tTue, 27 Jul 2021 12:00:50 +0200 (CEST)","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 21C4FEE;\n\tTue, 27 Jul 2021 12:00:48 +0200 (CEST)"],"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=\"DxUmOKse\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627380050;\n\tbh=XyyoMCplwmC8vXQFhLCUfHocsh5zedgEHYKWzmxfs0k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DxUmOKseFoB+Kz/Q9iXhDXD76M+0hw0SZUPxRZqRyrDoXBRu/X15jshdu54eFu+oi\n\t5V/eJcPZ9DFMXUUCHTsrpmfdAXhuIrVA9D5THqX1s6YomU5yIukENE203EnExl/Uwr\n\tlQ0OI9GS7E/R6YBQHj7koheYD4TyZ0uSfvCSb9I4=","Date":"Tue, 27 Jul 2021 19:00:42 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210727100042.GC2167@pyrite.rasen.tech>","References":"<20210727075023.3053-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210727075023.3053-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4] ipa: core.mojom: Rework core file\n\tdocumentation","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18379,"web_url":"https://patchwork.libcamera.org/comment/18379/","msgid":"<632a7923-a05f-d90e-daff-b35980c737aa@ideasonboard.com>","date":"2021-07-27T10:28:26","subject":"Re: [libcamera-devel] [PATCH v4] ipa: core.mojom: Rework core file\n\tdocumentation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 27/07/2021 08:50, Jacopo Mondi wrote:\n> The comment block at the beginning of the core.mojom file is meant to\n> provide an overview of how to use libcamera defined types in the definition\n> of mojom interfaces.\n> \n> As the IPA/IPC interface definition mechanism evolved, the documentation\n> has not been updated accordingly.\n> \n> Update the file comments to match the most recent IPA/IPC\n> interface definition and generation mechanism.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> v3->v4:\n> - Apply wording suggestions from Paul\n> \n> v2-v3:\n> - Address Paul's comment:\n> - s/[Mojo core|build system]/code generator/\n> - Swap points in skipSerdes description to make clear the flag instruct\n> the code generator not to generate a deserializer\n> - Clarify that nested types can *solely* be used as map/array members\n> \n> v1->v2:\n> - Address Paul's comment and clarify points that were not clear to me when I\n> wrote v1 :)\n> - (de)serializers implementations go in ipa_data_serializer.cpp\n> - types used as array/map members do not require a mojom definition\n> - remove duplications\n> - s/the library/libcamera\n> - While at it, make all statements start with a capital letter and end without a\n> full-stop.\n> ---\n> ---\n\nLots of extra --- to watch out for when applying there ;-)\n\nSounds good.\n\nReviewed-by: Kieran Bingham \n\n> include/libcamera/ipa/core.mojom | 68 +++++++++++++++++++-------------\n> 1 file changed, 41 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index b32f30939454..29ba35482b39 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -15,40 +15,54 @@ module libcamera;\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> + * - Do not generate a C++ definition for the structure\n> + * - Any type used in a mojom interface definition must have a corresponding\n> + * definition in a mojom file for the code generator to accept it, except\n> + * for types solely used as map/array members for which a definition is not\n> + * required\n> + * - This attribute allows defining a symbol for the code generator that\n> + * corresponds to a libcamera type without duplicating its definition in the\n> + * generated C++ headers\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> + * - All types need a (de)serializer to be defined in order to be transported\n> + * over IPC. The (de)serializer can be:\n> + * - Manually implemented as a template specialization in\n> + * ipa_data_serializer.cpp in the libcamera sources\n> + * - Generated at build time for types defined in a mojom file\n> + * - This attribute instructs the build system that a (de)serializer is\n> + * available for the type and there's no need to generate one\n> * - hasFd - struct fields or empty structs only\n> - * - designate that this field or empty struct contains a FileDescriptor\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> + * - If the type is defined in a libcamera C++ header *and* a (de)serializer is\n> + * available then the type shall be declared as empty with both attributes\n> + * associated and specified as: [skipHeader, skipSerdes]\n> + * - Example: [skipHeader, skipSerdes] ControlList {};\n> + * - If the type is defined in libcamera but no (de)serializer is available\n> + * then the type definition in the core.mojom file should have the\n> + * [skipHeader] attribute only\n> + * - A (de)serializer will be generated for the type\n> + * - If a type definition has [skipHeader], then the header where the type is\n> + * defined must be included in ipa_interface.h\n> + * - Types that are solely used as array/map members do not require a mojom\n> + * definition if one exists in libcamera\n> + * - Nested types (e.g. FrameBuffer::Plane) cannot be defined in mojom\n> + * - If used in mojom, the nested type shall be defined in a C++ header\n> + * and a (de)serializer shall be provided\n> + * - Nested types can only be used as array/map members\n> + * - When using the type, the C++ namespace separator :: is replaced with a\n> + * dot\n> + * - In example, to use the FrameBuffer::Plane type in mojom:\n> + * - Provide a definition of the FrameBuffer::Plane type in a C++ header\n> + * - Include the header in ipa_interface.h\n> + * - Provide a (de)serializer implementation ipa_data_serializer.cpp\n> + * - In mojom, reference the type as FrameBuffer.Plane and only as map/array\n> + * member\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 in 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> + * 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> --\n> 2.32.0\n>","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 EBBDDC322C\n\tfor ;\n\tTue, 27 Jul 2021 10:28:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A275687C3;\n\tTue, 27 Jul 2021 12:28:31 +0200 (CEST)","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 D30DA60272\n\tfor ;\n\tTue, 27 Jul 2021 12:28:29 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65F19EE;\n\tTue, 27 Jul 2021 12:28:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"stqnX+mb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627381709;\n\tbh=jy+M9lFNlOI5fUoIMhl/I76IUUJ3XYvp5j/ReP+FqIE=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=stqnX+mbCUkXfQOYat1p7uz4BnG/56RGdWdU+w3WhktwNBowc1tMq8ChiAOmm/Q68\n\tjD2B8IaP/nO3+sNYRkskxx54DnkeVrJSAK0pnQ5WpzOebnZYJSGMEPI80fcMwl0HnZ\n\tZmKP6jVfhViRjncle2hCNPzpjMNJDI0V/RlTyrug=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210727075023.3053-1-jacopo@jmondi.org>","From":"Kieran Bingham ","Message-ID":"<632a7923-a05f-d90e-daff-b35980c737aa@ideasonboard.com>","Date":"Tue, 27 Jul 2021 11:28:26 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210727075023.3053-1-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v4] ipa: core.mojom: Rework core file\n\tdocumentation","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]