[{"id":18225,"web_url":"https://patchwork.libcamera.org/comment/18225/","msgid":"<20210719060920.GL2395@pyrite.rasen.tech>","date":"2021-07-19T06:09:20","subject":"Re: [libcamera-devel] [PATCH v2] 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 Fri, Jul 16, 2021 at 02:19:02PM +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> ---\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> include/libcamera/ipa/core.mojom | 67 +++++++++++++++++++-------------\n> 1 file changed, 40 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index b32f30939454..39fac624658a 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -15,40 +15,53 @@ 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 mojo file for the Mojo core to accept it, except for\n\nHere you say \"Mojo core\" and below you say \"build system\"... idk which\none is better, though. We don't really have a \"Mojo core\" (we only use\nthe mojo lexer and parser; the code generator and certainly the IPC\nsystem is custom), but also \"build system\" sounds too ambiguous?\n\n> + * types used as map/array members for which a definition is not required\n\nIf the type is *only* used as a map/array member, then the type doesn't\nneed to be defined in mojom. It still needs to be defined in C++, and a\n(de)serializer implemented, though. I think you point it out below under\n\"Rules\", but it might be good to point out the caveat here too?\n\n> + * - This attribute allows defining a symbol for the Mojo core 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> + * - Do not generate a (de)serializer for the structure\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> + * - All types need a (de)serializer to be defined in order to be transported\n> + * over the IPA protocol. 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 mojo interfaces\n\nMaybe clarify that the second situation is when skipSerdes is *not*\nspecified?\n\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 contained in an array/map do not require a mojom definition\n\nAgain, types that are *only* used as array/map members.\n\n> + * if one exists in libcamera\n\nYes that is correct, if there is no definition of the type in libcamera,\nthen you can define it in mojom and let the code generator generate the\nC++ definition and the (de)serializer. (technically it's if-and-only-if\nbut whatever)\n\n> + * - Nested types (e.g. FrameBuffer::Plane) cannot be directly used in mojom\n\nThe original says cannot be *defined*.\n\n> + * - If used directly in mojom, the nested type shall be defined in a C++\n\n\"cannot be directly used\" \"if used directly\" :D\n\n> + * header and a (de)serializer shall be provided\n\nThis is the condition for using nested types. The restriction is that\nthey can only be used as map/array members. I think you mention that\nbelow but bringing it up here might be better?\n\n> + * - The C++ namespace separator :: is replaced with a dot\n> + * - In example, to directly use the FrameBuffer::Plane type:\n> + * - Provide a definition of the Plane type in a C++ header\n\ns/Plane/FrameBuffer::Plane/ ?\n\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\n\nThis can only be referenced as a map/array member.\n\n> + * - Nested types can be used without an associated C++ definition if used\n\ns/can be/can only be/ because nested types can't be defined.\n\ns/C++ definition/mojom definition/\n\nThe C++ definition + (de)serializer is certainly required.\n\n\nSorry this is so complicated :(\n\n\nPaul\n\n> + * as part of an array/map container\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 7FEFFC0109\n\tfor ;\n\tMon, 19 Jul 2021 06:09:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D479B6853A;\n\tMon, 19 Jul 2021 08:09:29 +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 B644360278\n\tfor ;\n\tMon, 19 Jul 2021 08:09:28 +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 1C832465;\n\tMon, 19 Jul 2021 08:09:26 +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=\"l92R2OjI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626674968;\n\tbh=kyVPe75+z/fbweqr1qZQm+iP72n2zQgs012qe+544cg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l92R2OjI7CQ2pDDOv6zt+6LFVkDLEAixE28ZUeD65TuokoSaVKWKO3loRYYGQgbnf\n\tVYP+TgfgnekBvsMSO4C0uSaVBZaDcnSAqwjX/2JeNeFOFlXtuZbb3iQaAmeFq6e45s\n\t/4GK9Adeosfb9tu81+hsv9On0IvazTnGixBZco/k=","Date":"Mon, 19 Jul 2021 15:09:20 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210719060920.GL2395@pyrite.rasen.tech>","References":"<20210716121902.48338-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210716121902.48338-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":18362,"web_url":"https://patchwork.libcamera.org/comment/18362/","msgid":"<20210726143137.ta2hr4td25smgesv@uno.localdomain>","date":"2021-07-26T14:31:37","subject":"Re: [libcamera-devel] [PATCH v2] ipa: core.mojom: Rework core file\n\tdocumentation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n thanks for your comment\n\nOn Mon, Jul 19, 2021 at 03:09:20PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Fri, Jul 16, 2021 at 02:19:02PM +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> > ---\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> > include/libcamera/ipa/core.mojom | 67 +++++++++++++++++++-------------\n> > 1 file changed, 40 insertions(+), 27 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > index b32f30939454..39fac624658a 100644\n> > --- a/include/libcamera/ipa/core.mojom\n> > +++ b/include/libcamera/ipa/core.mojom\n> > @@ -15,40 +15,53 @@ 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 mojo file for the Mojo core to accept it, except for\n>\n> Here you say \"Mojo core\" and below you say \"build system\"... idk which\n> one is better, though. We don't really have a \"Mojo core\" (we only use\n> the mojo lexer and parser; the code generator and certainly the IPC\n> system is custom), but also \"build system\" sounds too ambiguous?\n>\n\nThe \"code generator\" ?\n\n> > + * types used as map/array members for which a definition is not required\n>\n> If the type is *only* used as a map/array member, then the type doesn't\n> need to be defined in mojom. It still needs to be defined in C++, and a\n> (de)serializer implemented, though. I think you point it out below under\n> \"Rules\", but it might be good to point out the caveat here too?\n\nYes, I missed the fact that is should be *only* used as a container\nmember.\n\n>\n> > + * - This attribute allows defining a symbol for the Mojo core 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> > + * - Do not generate a (de)serializer for the structure\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> > + * - All types need a (de)serializer to be defined in order to be transported\n> > + * over the IPA protocol. 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 mojo interfaces\n>\n> Maybe clarify that the second situation is when skipSerdes is *not*\n> specified?\n>\n\nI just said above \"Do not generate a (de)serializer...\"\n\nI'll swap points, to make this one the first one and give a general\nintroduction and then introduce skipSerdes\n\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 contained in an array/map do not require a mojom definition\n>\n> Again, types that are *only* used as array/map members.\n>\n> > + * if one exists in libcamera\n>\n> Yes that is correct, if there is no definition of the type in libcamera,\n> then you can define it in mojom and let the code generator generate the\n> C++ definition and the (de)serializer. (technically it's if-and-only-if\n> but whatever)\n>\n> > + * - Nested types (e.g. FrameBuffer::Plane) cannot be directly used in mojom\n>\n> The original says cannot be *defined*.\n>\n> > + * - If used directly in mojom, the nested type shall be defined in a C++\n>\n> \"cannot be directly used\" \"if used directly\" :D\n>\n> > + * header and a (de)serializer shall be provided\n>\n> This is the condition for using nested types. The restriction is that\n> they can only be used as map/array members. I think you mention that\n> below but bringing it up here might be better?\n>\n> > + * - The C++ namespace separator :: is replaced with a dot\n> > + * - In example, to directly use the FrameBuffer::Plane type:\n> > + * - Provide a definition of the Plane type in a C++ header\n>\n> s/Plane/FrameBuffer::Plane/ ?\n>\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\n>\n> This can only be referenced as a map/array member.\n>\n> > + * - Nested types can be used without an associated C++ definition if used\n>\n> s/can be/can only be/ because nested types can't be defined.\n>\n> s/C++ definition/mojom definition/\n>\n> The C++ definition + (de)serializer is certainly required.\n\nThis now looks like\n\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 part of array/map containers\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\n>\n>\n> Sorry this is so complicated :(\n\nNah, don't worry, not easy to express stuff, thank you for sticking with it and\nrectify my understanding.\nv3 out soon\n\nThanks\n j\n\n>\n>\n> Paul\n>\n> > + * as part of an array/map container\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 4395FC322C\n\tfor ;\n\tMon, 26 Jul 2021 14:30:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95D31687C0;\n\tMon, 26 Jul 2021 16:30:51 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4ADD9687AF\n\tfor ;\n\tMon, 26 Jul 2021 16:30:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id AEB02FF807;\n\tMon, 26 Jul 2021 14:30:49 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 16:31:37 +0200","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20210726143137.ta2hr4td25smgesv@uno.localdomain>","References":"<20210716121902.48338-1-jacopo@jmondi.org>\n\t<20210719060920.GL2395@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210719060920.GL2395@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2] 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\" "}}]