[{"id":17018,"web_url":"https://patchwork.libcamera.org/comment/17018/","msgid":"","date":"2021-05-19T11:11:53","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 19/05/2021 11:19, Umang Jain wrote:\n> Moving the core.mojom documentation to its corresponding .cpp file\n> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> documentation for IPABuffer, IPASettings and IPAStream structures.\n> Since the .mojom files are placed in include/ directory, the .cpp file\n> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> documentation for other mojom generated IPA interfaces in subsequent\n> commit.\n> \n> Also, fix the Doxygen warning for the above IPA structs regarding their\n> constructors not being documented.\n> \n> Signed-off-by: Umang Jain \n> ---\n> Documentation/Doxyfile.in | 8 +-\n> Documentation/meson.build | 1 +\n> include/libcamera/ipa/core.mojom | 72 ----------------\n> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> src/libcamera/ipa/meson.build | 5 ++\n> src/libcamera/meson.build | 1 +\n> 6 files changed, 118 insertions(+), 74 deletions(-)\n> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> create mode 100644 src/libcamera/ipa/meson.build\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index af006724..f4d578fa 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> \n> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> # Note that the wildcards are matched against the file with absolute path, so to\n> # exclude all test directories for example use the pattern */test/*\n> \n> -EXCLUDE_PATTERNS =\n> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n\n\nI was going to suggest that perhaps this should be\n\t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n\nBut that would exclude the core_ipa_interface.h too, which I presume we\nneed to include for Doxygen to map the documentation to.\n\nI wonder if there's a way to include only the files we need under\ninclude/libcamera/ipa if that list is smaller, rather than trying to\nexclude lots of combinations which would need to be updated with every\nnew platform supported.\n\n> \n> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> # (namespaces, classes, functions, etc.) that should be excluded from the\n> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> index c8521574..9ecf4dfc 100644\n> --- a/Documentation/meson.build\n> +++ b/Documentation/meson.build\n> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> doxyfile,\n> libcamera_internal_headers,\n> libcamera_ipa_headers,\n> + libcamera_ipa_interfaces,\n> libcamera_public_headers,\n> libcamera_sources,\n> libipa_headers,\n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index 6caaa63e..e49815d8 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -94,88 +94,16 @@ module libcamera;\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> -\n> - /**\n> - * \\var IPASettings::sensorModel\n> - * \\brief The sensor model name\n> - *\n> - * Provides the sensor model name to the IPA.\n> - */\n> struct IPASettings {\n> \tstring configurationFile;\n> \tstring sensorModel;\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> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> new file mode 100644\n> index 00000000..fe1ecce6\n> --- /dev/null\n> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> @@ -0,0 +1,105 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> + */\n> +\n> +namespace libcamera {\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> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> + * \\param[in] id\n> + * \\param[in] planes\n> + * \\sa id and planes\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> +/**\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> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> + * \\param[in] configurationFile\n> + * \\param[in] sensorModel\n> + * \\sa configurationFile and sensorModel\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> +\n> +/**\n> + * \\var IPASettings::sensorModel\n> + * \\brief The sensor model name\n> + *\n> + * Provides the sensor model name to the IPA.\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> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> + * \\param[in] pixelFormat\n> + * \\param[in] size\n> + * \\sa pixelFormat and size\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> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> new file mode 100644\n> index 00000000..0a16d197\n> --- /dev/null\n> +++ b/src/libcamera/ipa/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> +\n> +libcamera_ipa_interfaces = files([\n> + 'core_ipa_interface.cpp',\n> +])\n\nI'm sure someone will disagree with me, but I almost feel like this .cpp\nfile should get run through a compiler, and therefore just added to the\nlibcamera sources as normal (which I think would then already get picked\nup by doxygen).\n\nThat way the syntax of the file is maintained too, given that it is a\nC++ file.\n\n\n\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 7bc59b84..61b5fe21 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -67,6 +67,7 @@ includes = [\n> libcamera_includes,\n> ]\n> \n> +subdir('ipa')\n> subdir('pipeline')\n> subdir('proxy')\n> \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 98177C31FF\n\tfor ;\n\tWed, 19 May 2021 11:11:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 122766891D;\n\tWed, 19 May 2021 13:11:59 +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 9307B602B2\n\tfor ;\n\tWed, 19 May 2021 13:11:57 +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 0795D8EC;\n\tWed, 19 May 2021 13:11:56 +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=\"WvWn4p1V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621422717;\n\tbh=x5DDzdfIUIRKk87T92YJGAnv74i/TgJ76ImBufMEf9A=;\n\th=Reply-To:To:References:From:Subject:Date:In-Reply-To:From;\n\tb=WvWn4p1VKuSQFanyh8+1JJYz8sNU97tM2jzRoXX0elBADfFvxdvKcqq8aagaXNbma\n\tg0rdivPVUF/ck6/LPDnmaX4gQlGC59u0nLmbPPh/54YF2oerGM8Pp6dMCNMYhd1KcP\n\twZZZ21Wcuq6kmBYBWOQfGl0MDyjzEHnIOHbMSsx0=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 19 May 2021 12:11:53 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<20210519101954.77711-2-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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","Reply-To":"kieran.bingham@ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17030,"web_url":"https://patchwork.libcamera.org/comment/17030/","msgid":"","date":"2021-05-19T15:55:11","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n> On 19/05/2021 11:19, Umang Jain wrote:\n> > Moving the core.mojom documentation to its corresponding .cpp file\n> > (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> > documentation for IPABuffer, IPASettings and IPAStream structures.\n> > Since the .mojom files are placed in include/ directory, the .cpp file\n> > will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> > documentation for other mojom generated IPA interfaces in subsequent\n> > commit.\n> > \n> > Also, fix the Doxygen warning for the above IPA structs regarding their\n> > constructors not being documented.\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > Documentation/Doxyfile.in | 8 +-\n> > Documentation/meson.build | 1 +\n> > include/libcamera/ipa/core.mojom | 72 ----------------\n> > src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> > src/libcamera/ipa/meson.build | 5 ++\n> > src/libcamera/meson.build | 1 +\n> > 6 files changed, 118 insertions(+), 74 deletions(-)\n> > create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> > create mode 100644 src/libcamera/ipa/meson.build\n> > \n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index af006724..f4d578fa 100644\n> > --- a/Documentation/Doxyfile.in\n> > +++ b/Documentation/Doxyfile.in\n> > @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> > \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> > \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> > \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> > -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> > \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> > \n> > # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> > @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> > # Note that the wildcards are matched against the file with absolute path, so to\n> > # exclude all test directories for example use the pattern */test/*\n> > \n> > -EXCLUDE_PATTERNS =\n> > +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> > + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> > + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> > + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> > + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> > + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> \n> I was going to suggest that perhaps this should be\n> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n> \n> But that would exclude the core_ipa_interface.h too, which I presume we\n> need to include for Doxygen to map the documentation to.\n> \n> I wonder if there's a way to include only the files we need under\n> include/libcamera/ipa if that list is smaller, rather than trying to\n> exclude lots of combinations which would need to be updated with every\n> new platform supported.\n\nI'd rather go for a method that would scale indeed :-) One option is to\nmodify the naming scheme of generated files to be able to match them by\npattern more easily.\n\n> > # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> > # (namespaces, classes, functions, etc.) that should be excluded from the\n> > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > index c8521574..9ecf4dfc 100644\n> > --- a/Documentation/meson.build\n> > +++ b/Documentation/meson.build\n> > @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> > doxyfile,\n> > libcamera_internal_headers,\n> > libcamera_ipa_headers,\n> > + libcamera_ipa_interfaces,\n> > libcamera_public_headers,\n> > libcamera_sources,\n> > libipa_headers,\n> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > index 6caaa63e..e49815d8 100644\n> > --- a/include/libcamera/ipa/core.mojom\n> > +++ b/include/libcamera/ipa/core.mojom\n> > @@ -94,88 +94,16 @@ module libcamera;\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> > -\n> > - /**\n> > - * \\var IPASettings::sensorModel\n> > - * \\brief The sensor model name\n> > - *\n> > - * Provides the sensor model name to the IPA.\n> > - */\n> > struct IPASettings {\n> > \tstring configurationFile;\n> > \tstring sensorModel;\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> > diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> > new file mode 100644\n> > index 00000000..fe1ecce6\n> > --- /dev/null\n> > +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> > @@ -0,0 +1,105 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> > + */\n> > +\n> > +namespace libcamera {\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> > + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> > + * \\param[in] id\n> > + * \\param[in] planes\n> > + * \\sa id and planes\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> > +/**\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> > + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> > + * \\param[in] configurationFile\n> > + * \\param[in] sensorModel\n> > + * \\sa configurationFile and sensorModel\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> > +\n> > +/**\n> > + * \\var IPASettings::sensorModel\n> > + * \\brief The sensor model name\n> > + *\n> > + * Provides the sensor model name to the IPA.\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> > + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> > + * \\param[in] pixelFormat\n> > + * \\param[in] size\n> > + * \\sa pixelFormat and size\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> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> > new file mode 100644\n> > index 00000000..0a16d197\n> > --- /dev/null\n> > +++ b/src/libcamera/ipa/meson.build\n> > @@ -0,0 +1,5 @@\n> > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > +\n> > +libcamera_ipa_interfaces = files([\n> > + 'core_ipa_interface.cpp',\n> > +])\n> \n> I'm sure someone will disagree with me, but I almost feel like this .cpp\n> file should get run through a compiler, and therefore just added to the\n> libcamera sources as normal (which I think would then already get picked\n> up by doxygen).\n> \n> That way the syntax of the file is maintained too, given that it is a\n> C++ file.\n\nWhat's the point if it only contains comments though ? Ideally we\nshouldn't have this .cpp file in the first place, it should be generated\nautomatically, or the documentation comments should be added to the\ngenearated headers by mojo. We can't do so now so we need this kind of\nhack, but I'd rather keep the scope of the hack as small as possible.\n\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 7bc59b84..61b5fe21 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -67,6 +67,7 @@ includes = [\n> > libcamera_includes,\n> > ]\n> > \n> > +subdir('ipa')\n> > subdir('pipeline')\n> > subdir('proxy')\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 3A824C31FF\n\tfor ;\n\tWed, 19 May 2021 15:55:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D80A6891A;\n\tWed, 19 May 2021 17:55:15 +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 9360C68915\n\tfor ;\n\tWed, 19 May 2021 17:55:13 +0200 (CEST)","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 07034BA7;\n\tWed, 19 May 2021 17:55:12 +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=\"G0Wvhfv+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621439713;\n\tbh=p4ixIFjbK3Swvug+aHwki3ykXyTvxcW+P0VjQURI9rg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G0Wvhfv+9uvoTT8EQgQvtUD5NfK81uExrdugKgM3pnXVVKXzC+dR7gEsMBJTGwy1B\n\tcfqWG0sSVg2nOVz+g/LYkC4wa1Sgg6OQ3QfzPe1SU7HeA7CpwO4rPgRm4b9YmVvqyG\n\tb8JutaiW7HsZMpVOb5CCToIf3OH+uljtECFyVqRU=","Date":"Wed, 19 May 2021 18:55:11 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17032,"web_url":"https://patchwork.libcamera.org/comment/17032/","msgid":"<20210520022810.GC902042@pyrite.rasen.tech>","date":"2021-05-20T02:28:10","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n> > On 19/05/2021 11:19, Umang Jain wrote:\n> > > Moving the core.mojom documentation to its corresponding .cpp file\n> > > (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> > > documentation for IPABuffer, IPASettings and IPAStream structures.\n> > > Since the .mojom files are placed in include/ directory, the .cpp file\n> > > will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> > > documentation for other mojom generated IPA interfaces in subsequent\n> > > commit.\n> > > \n> > > Also, fix the Doxygen warning for the above IPA structs regarding their\n> > > constructors not being documented.\n> > > \n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > Documentation/Doxyfile.in | 8 +-\n> > > Documentation/meson.build | 1 +\n> > > include/libcamera/ipa/core.mojom | 72 ----------------\n> > > src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> > > src/libcamera/ipa/meson.build | 5 ++\n> > > src/libcamera/meson.build | 1 +\n> > > 6 files changed, 118 insertions(+), 74 deletions(-)\n> > > create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> > > create mode 100644 src/libcamera/ipa/meson.build\n> > > \n> > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > > index af006724..f4d578fa 100644\n> > > --- a/Documentation/Doxyfile.in\n> > > +++ b/Documentation/Doxyfile.in\n> > > @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> > > \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> > > \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> > > \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> > > -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> > > \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> > > \n> > > # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> > > @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> > > # Note that the wildcards are matched against the file with absolute path, so to\n> > > # exclude all test directories for example use the pattern */test/*\n> > > \n> > > -EXCLUDE_PATTERNS =\n> > > +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> > > + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> > > + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> > > + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> > > + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> > > + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> > \n> > I was going to suggest that perhaps this should be\n> > \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n> > \n> > But that would exclude the core_ipa_interface.h too, which I presume we\n> > need to include for Doxygen to map the documentation to.\n> > \n> > I wonder if there's a way to include only the files we need under\n> > include/libcamera/ipa if that list is smaller, rather than trying to\n> > exclude lots of combinations which would need to be updated with every\n> > new platform supported.\n> \n> I'd rather go for a method that would scale indeed :-) One option is to\n> modify the naming scheme of generated files to be able to match them by\n> pattern more easily.\n\n{raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\nanyway, so we could add their documentation (in\n{raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\nit in doxygen and meson.\n\nThen the exclude pattern would just be the first two lines.\n\n\nPaul\n\n> \n> > > # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> > > # (namespaces, classes, functions, etc.) that should be excluded from the\n> > > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > > index c8521574..9ecf4dfc 100644\n> > > --- a/Documentation/meson.build\n> > > +++ b/Documentation/meson.build\n> > > @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> > > doxyfile,\n> > > libcamera_internal_headers,\n> > > libcamera_ipa_headers,\n> > > + libcamera_ipa_interfaces,\n> > > libcamera_public_headers,\n> > > libcamera_sources,\n> > > libipa_headers,\n> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > > index 6caaa63e..e49815d8 100644\n> > > --- a/include/libcamera/ipa/core.mojom\n> > > +++ b/include/libcamera/ipa/core.mojom\n> > > @@ -94,88 +94,16 @@ module libcamera;\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> > > -\n> > > - /**\n> > > - * \\var IPASettings::sensorModel\n> > > - * \\brief The sensor model name\n> > > - *\n> > > - * Provides the sensor model name to the IPA.\n> > > - */\n> > > struct IPASettings {\n> > > \tstring configurationFile;\n> > > \tstring sensorModel;\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> > > diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> > > new file mode 100644\n> > > index 00000000..fe1ecce6\n> > > --- /dev/null\n> > > +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> > > @@ -0,0 +1,105 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> > > + */\n> > > +\n> > > +namespace libcamera {\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> > > + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> > > + * \\param[in] id\n> > > + * \\param[in] planes\n> > > + * \\sa id and planes\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> > > +/**\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> > > + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> > > + * \\param[in] configurationFile\n> > > + * \\param[in] sensorModel\n> > > + * \\sa configurationFile and sensorModel\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> > > +\n> > > +/**\n> > > + * \\var IPASettings::sensorModel\n> > > + * \\brief The sensor model name\n> > > + *\n> > > + * Provides the sensor model name to the IPA.\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> > > + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> > > + * \\param[in] pixelFormat\n> > > + * \\param[in] size\n> > > + * \\sa pixelFormat and size\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> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> > > new file mode 100644\n> > > index 00000000..0a16d197\n> > > --- /dev/null\n> > > +++ b/src/libcamera/ipa/meson.build\n> > > @@ -0,0 +1,5 @@\n> > > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > > +\n> > > +libcamera_ipa_interfaces = files([\n> > > + 'core_ipa_interface.cpp',\n> > > +])\n> > \n> > I'm sure someone will disagree with me, but I almost feel like this .cpp\n> > file should get run through a compiler, and therefore just added to the\n> > libcamera sources as normal (which I think would then already get picked\n> > up by doxygen).\n> > \n> > That way the syntax of the file is maintained too, given that it is a\n> > C++ file.\n> \n> What's the point if it only contains comments though ? Ideally we\n> shouldn't have this .cpp file in the first place, it should be generated\n> automatically, or the documentation comments should be added to the\n> genearated headers by mojo. We can't do so now so we need this kind of\n> hack, but I'd rather keep the scope of the hack as small as possible.\n> \n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 7bc59b84..61b5fe21 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -67,6 +67,7 @@ includes = [\n> > > libcamera_includes,\n> > > ]\n> > > \n> > > +subdir('ipa')\n> > > subdir('pipeline')\n> > > subdir('proxy')\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 12AC2C31FB\n\tfor ;\n\tThu, 20 May 2021 02:28:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40F486891F;\n\tThu, 20 May 2021 04:28:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1FFF602B3\n\tfor ;\n\tThu, 20 May 2021 04:28:17 +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 D05468DF;\n\tThu, 20 May 2021 04:28:15 +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=\"aYlkuBRI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621477697;\n\tbh=MxfbVyObwedNnMcf7UNmj5JXQmEJ45a/ohL8P8PeNms=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aYlkuBRIeriH2aTLuR0FL7Xme0KFo42DScqnqxFPMT/CbrgW27hQ/+rRPIasp4FnE\n\tfCL+NrR8oGfS0IkGLtHemHX2tTl8pJsKGzs5m8c13WJdb7jTcBVhO7AsgySFt5aKW6\n\tVxRhRA2bofcd7JfiLCfSl1PT/mcEWTkk2UqpCH6A=","Date":"Thu, 20 May 2021 11:28:10 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210520022810.GC902042@pyrite.rasen.tech>","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17036,"web_url":"https://patchwork.libcamera.org/comment/17036/","msgid":"","date":"2021-05-20T02:56:02","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul [& others]\n\nOn 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n>>> On 19/05/2021 11:19, Umang Jain wrote:\n>>>> Moving the core.mojom documentation to its corresponding .cpp file\n>>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n>>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n>>>> Since the .mojom files are placed in include/ directory, the .cpp file\n>>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n>>>> documentation for other mojom generated IPA interfaces in subsequent\n>>>> commit.\n>>>>\n>>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n>>>> constructors not being documented.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> Documentation/Doxyfile.in | 8 +-\n>>>> Documentation/meson.build | 1 +\n>>>> include/libcamera/ipa/core.mojom | 72 ----------------\n>>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n>>>> src/libcamera/ipa/meson.build | 5 ++\n>>>> src/libcamera/meson.build | 1 +\n>>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n>>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n>>>> create mode 100644 src/libcamera/ipa/meson.build\n>>>>\n>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>>>> index af006724..f4d578fa 100644\n>>>> --- a/Documentation/Doxyfile.in\n>>>> +++ b/Documentation/Doxyfile.in\n>>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n>>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n>>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n>>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n>>>> \n>>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n>>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n>>>> # Note that the wildcards are matched against the file with absolute path, so to\n>>>> # exclude all test directories for example use the pattern */test/*\n>>>> \n>>>> -EXCLUDE_PATTERNS =\n>>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n>>> I was going to suggest that perhaps this should be\n>>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n>>>\n>>> But that would exclude the core_ipa_interface.h too, which I presume we\n>>> need to include for Doxygen to map the documentation to.\n>>>\n>>> I wonder if there's a way to include only the files we need under\n>>> include/libcamera/ipa if that list is smaller, rather than trying to\n>>> exclude lots of combinations which would need to be updated with every\n>>> new platform supported.\n>> I'd rather go for a method that would scale indeed :-) One option is to\n>> modify the naming scheme of generated files to be able to match them by\n>> pattern more easily.\n> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n> anyway, so we could add their documentation (in\n> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n> it in doxygen and meson.\nYes, documenting all those IPA modules first might be ideal but not sure \nif may happen all at once. The way I saw, how would this work would be \naddressed is, document one of the modules & exclude it from the \nEXCLUDE_PATTERNS for it's generation.\n>\n> Then the exclude pattern would just be the first two lines.\nWhen all modules are documented we will have this state of exclude \npattern again. For new IPA (in-tree) modules appearing meanwhile, I \nthink we should only merge them, if they are documented as per the [2/7] \nprecedence.\n\nAlso want to mention that we can file it as a task and thing it's a good \nfirst-comers task to work on.\n>\n>\n> Paul\n>\n>>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n>>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>>>> index c8521574..9ecf4dfc 100644\n>>>> --- a/Documentation/meson.build\n>>>> +++ b/Documentation/meson.build\n>>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n>>>> doxyfile,\n>>>> libcamera_internal_headers,\n>>>> libcamera_ipa_headers,\n>>>> + libcamera_ipa_interfaces,\n>>>> libcamera_public_headers,\n>>>> libcamera_sources,\n>>>> libipa_headers,\n>>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n>>>> index 6caaa63e..e49815d8 100644\n>>>> --- a/include/libcamera/ipa/core.mojom\n>>>> +++ b/include/libcamera/ipa/core.mojom\n>>>> @@ -94,88 +94,16 @@ module libcamera;\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>>>> -\n>>>> - /**\n>>>> - * \\var IPASettings::sensorModel\n>>>> - * \\brief The sensor model name\n>>>> - *\n>>>> - * Provides the sensor model name to the IPA.\n>>>> - */\n>>>> struct IPASettings {\n>>>> \tstring configurationFile;\n>>>> \tstring sensorModel;\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>>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>> new file mode 100644\n>>>> index 00000000..fe1ecce6\n>>>> --- /dev/null\n>>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>> @@ -0,0 +1,105 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2021, Google Inc.\n>>>> + *\n>>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n>>>> + */\n>>>> +\n>>>> +namespace libcamera {\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>>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n>>>> + * \\param[in] id\n>>>> + * \\param[in] planes\n>>>> + * \\sa id and planes\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>>>> +/**\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>>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n>>>> + * \\param[in] configurationFile\n>>>> + * \\param[in] sensorModel\n>>>> + * \\sa configurationFile and sensorModel\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>>>> +\n>>>> +/**\n>>>> + * \\var IPASettings::sensorModel\n>>>> + * \\brief The sensor model name\n>>>> + *\n>>>> + * Provides the sensor model name to the IPA.\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>>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n>>>> + * \\param[in] pixelFormat\n>>>> + * \\param[in] size\n>>>> + * \\sa pixelFormat and size\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>>>> +\n>>>> +} /* namespace libcamera */\n>>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n>>>> new file mode 100644\n>>>> index 00000000..0a16d197\n>>>> --- /dev/null\n>>>> +++ b/src/libcamera/ipa/meson.build\n>>>> @@ -0,0 +1,5 @@\n>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n>>>> +\n>>>> +libcamera_ipa_interfaces = files([\n>>>> + 'core_ipa_interface.cpp',\n>>>> +])\n>>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n>>> file should get run through a compiler, and therefore just added to the\n>>> libcamera sources as normal (which I think would then already get picked\n>>> up by doxygen).\n>>>\n>>> That way the syntax of the file is maintained too, given that it is a\n>>> C++ file.\n>> What's the point if it only contains comments though ? Ideally we\n>> shouldn't have this .cpp file in the first place, it should be generated\n>> automatically, or the documentation comments should be added to the\n>> genearated headers by mojo. We can't do so now so we need this kind of\n>> hack, but I'd rather keep the scope of the hack as small as possible.\n>>\n>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>> index 7bc59b84..61b5fe21 100644\n>>>> --- a/src/libcamera/meson.build\n>>>> +++ b/src/libcamera/meson.build\n>>>> @@ -67,6 +67,7 @@ includes = [\n>>>> libcamera_includes,\n>>>> ]\n>>>> \n>>>> +subdir('ipa')\n>>>> subdir('pipeline')\n>>>> subdir('proxy')\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 28469C31FF\n\tfor ;\n\tThu, 20 May 2021 02:56:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E080F6891F;\n\tThu, 20 May 2021 04:56:08 +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 239716050F\n\tfor ;\n\tThu, 20 May 2021 04:56:08 +0200 (CEST)","from localhost.localdomain (unknown [103.251.226.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D8BE38DF;\n\tThu, 20 May 2021 04:56:06 +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=\"nAFQRof4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621479367;\n\tbh=YiCdMjDvVENiiikcX9a6s5z5PP/WjdO9cMyqaoxbL44=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=nAFQRof4af7pln2ulS+LhEtKfk7MyMzfPpQwzDoS/u9/gmLBaCP+rY8X7mdUsz4xj\n\tJCRuMFLte0qSzyz1QBcSzy/UpCtOWbumU2ve1TSizIsO9WbuwVU+2C8SFdvMTKA+yw\n\tHfuU4MnphqJya8nnbbi6BvLbZtbEjNpQIw8XoXfM=","To":"paul.elder@ideasonboard.com,\n\tLaurent Pinchart ","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>","From":"Umang Jain ","Message-ID":"","Date":"Thu, 20 May 2021 08:26:02 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"<20210520022810.GC902042@pyrite.rasen.tech>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17038,"web_url":"https://patchwork.libcamera.org/comment/17038/","msgid":"<20210520031415.GH902042@pyrite.rasen.tech>","date":"2021-05-20T03:14:15","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:\n> Hi Paul [& others]\n> \n> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n> > On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n> > > On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n> > > > On 19/05/2021 11:19, Umang Jain wrote:\n> > > > > Moving the core.mojom documentation to its corresponding .cpp file\n> > > > > (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> > > > > documentation for IPABuffer, IPASettings and IPAStream structures.\n> > > > > Since the .mojom files are placed in include/ directory, the .cpp file\n> > > > > will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> > > > > documentation for other mojom generated IPA interfaces in subsequent\n> > > > > commit.\n> > > > > \n> > > > > Also, fix the Doxygen warning for the above IPA structs regarding their\n> > > > > constructors not being documented.\n> > > > > \n> > > > > Signed-off-by: Umang Jain \n> > > > > ---\n> > > > > Documentation/Doxyfile.in | 8 +-\n> > > > > Documentation/meson.build | 1 +\n> > > > > include/libcamera/ipa/core.mojom | 72 ----------------\n> > > > > src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> > > > > src/libcamera/ipa/meson.build | 5 ++\n> > > > > src/libcamera/meson.build | 1 +\n> > > > > 6 files changed, 118 insertions(+), 74 deletions(-)\n> > > > > create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> > > > > create mode 100644 src/libcamera/ipa/meson.build\n> > > > > \n> > > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > > > > index af006724..f4d578fa 100644\n> > > > > --- a/Documentation/Doxyfile.in\n> > > > > +++ b/Documentation/Doxyfile.in\n> > > > > @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> > > > > \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> > > > > \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> > > > > \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> > > > > -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> > > > > \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> > > > > # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> > > > > @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> > > > > # Note that the wildcards are matched against the file with absolute path, so to\n> > > > > # exclude all test directories for example use the pattern */test/*\n> > > > > -EXCLUDE_PATTERNS =\n> > > > > +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> > > > > + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> > > > > + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> > > > > + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> > > > > + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> > > > > + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> > > > I was going to suggest that perhaps this should be\n> > > > \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n> > > > \n> > > > But that would exclude the core_ipa_interface.h too, which I presume we\n> > > > need to include for Doxygen to map the documentation to.\n> > > > \n> > > > I wonder if there's a way to include only the files we need under\n> > > > include/libcamera/ipa if that list is smaller, rather than trying to\n> > > > exclude lots of combinations which would need to be updated with every\n> > > > new platform supported.\n> > > I'd rather go for a method that would scale indeed :-) One option is to\n> > > modify the naming scheme of generated files to be able to match them by\n> > > pattern more easily.\n> > {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n> > anyway, so we could add their documentation (in\n> > {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n> > it in doxygen and meson.\n> Yes, documenting all those IPA modules first might be ideal but not sure if\n> may happen all at once. The way I saw, how would this work would be\n> addressed is, document one of the modules & exclude it from the\n> EXCLUDE_PATTERNS for it's generation.\n> > \n> > Then the exclude pattern would just be the first two lines.\n> When all modules are documented we will have this state of exclude pattern\n> again. For new IPA (in-tree) modules appearing meanwhile, I think we should\n> only merge them, if they are documented as per the [2/7] precedence.\n> \n> Also want to mention that we can file it as a task and thing it's a good\n> first-comers task to work on.\n\nAh, I see, remove them from the exclude patterns as they're added. Yeah\nthat would be a good first-comer task too.\n\nIn that case I think a todo comment (either here or in each mojom file?\nin the mojo file it won't be picked up by doxygen though... what about\nin dummy interface cpp files?) would be useful.\n\n\nPaul\n\n> > \n> > > > > # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> > > > > # (namespaces, classes, functions, etc.) that should be excluded from the\n> > > > > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > > > > index c8521574..9ecf4dfc 100644\n> > > > > --- a/Documentation/meson.build\n> > > > > +++ b/Documentation/meson.build\n> > > > > @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> > > > > doxyfile,\n> > > > > libcamera_internal_headers,\n> > > > > libcamera_ipa_headers,\n> > > > > + libcamera_ipa_interfaces,\n> > > > > libcamera_public_headers,\n> > > > > libcamera_sources,\n> > > > > libipa_headers,\n> > > > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > > > > index 6caaa63e..e49815d8 100644\n> > > > > --- a/include/libcamera/ipa/core.mojom\n> > > > > +++ b/include/libcamera/ipa/core.mojom\n> > > > > @@ -94,88 +94,16 @@ module libcamera;\n> > > > > \tuint32 maxFrameLength;\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> > > > > - * \\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> > > > > -\n> > > > > - /**\n> > > > > - * \\var IPASettings::sensorModel\n> > > > > - * \\brief The sensor model name\n> > > > > - *\n> > > > > - * Provides the sensor model name to the IPA.\n> > > > > - */\n> > > > > struct IPASettings {\n> > > > > \tstring configurationFile;\n> > > > > \tstring sensorModel;\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> > > > > diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..fe1ecce6\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> > > > > @@ -0,0 +1,105 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera {\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> > > > > + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> > > > > + * \\param[in] id\n> > > > > + * \\param[in] planes\n> > > > > + * \\sa id and planes\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> > > > > +/**\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> > > > > + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> > > > > + * \\param[in] configurationFile\n> > > > > + * \\param[in] sensorModel\n> > > > > + * \\sa configurationFile and sensorModel\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> > > > > +\n> > > > > +/**\n> > > > > + * \\var IPASettings::sensorModel\n> > > > > + * \\brief The sensor model name\n> > > > > + *\n> > > > > + * Provides the sensor model name to the IPA.\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> > > > > + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> > > > > + * \\param[in] pixelFormat\n> > > > > + * \\param[in] size\n> > > > > + * \\sa pixelFormat and size\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> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> > > > > new file mode 100644\n> > > > > index 00000000..0a16d197\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/ipa/meson.build\n> > > > > @@ -0,0 +1,5 @@\n> > > > > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > > > > +\n> > > > > +libcamera_ipa_interfaces = files([\n> > > > > + 'core_ipa_interface.cpp',\n> > > > > +])\n> > > > I'm sure someone will disagree with me, but I almost feel like this .cpp\n> > > > file should get run through a compiler, and therefore just added to the\n> > > > libcamera sources as normal (which I think would then already get picked\n> > > > up by doxygen).\n> > > > \n> > > > That way the syntax of the file is maintained too, given that it is a\n> > > > C++ file.\n> > > What's the point if it only contains comments though ? Ideally we\n> > > shouldn't have this .cpp file in the first place, it should be generated\n> > > automatically, or the documentation comments should be added to the\n> > > genearated headers by mojo. We can't do so now so we need this kind of\n> > > hack, but I'd rather keep the scope of the hack as small as possible.\n> > > \n> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > > index 7bc59b84..61b5fe21 100644\n> > > > > --- a/src/libcamera/meson.build\n> > > > > +++ b/src/libcamera/meson.build\n> > > > > @@ -67,6 +67,7 @@ includes = [\n> > > > > libcamera_includes,\n> > > > > ]\n> > > > > +subdir('ipa')\n> > > > > subdir('pipeline')\n> > > > > subdir('proxy')\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 E29BBC31FF\n\tfor ;\n\tThu, 20 May 2021 03:14:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40EC36891A;\n\tThu, 20 May 2021 05:14:24 +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 6537C6050F\n\tfor ;\n\tThu, 20 May 2021 05:14:23 +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 7A6BFD31;\n\tThu, 20 May 2021 05:14:21 +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=\"UvrqikZC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621480463;\n\tbh=0yfhxtb37vl5wYgT70K788n6IZksQ1oQhRnpQ4HqKoQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UvrqikZCsbCn2Plf6mJg9gUmoj/T1EjWHJn37sTbFBqCfDqANeHzupsAwNe31+E/N\n\thA2lhB+K3W3Rs4sJj1PGPg6dPLZd9W9iF20YjukUA8sVgvArpWfCY8iWkf2TaU4IAO\n\t5A7YDgjmcRBc9Je+VROeppPZKqwwRCIhhfZVlzto=","Date":"Thu, 20 May 2021 12:14:15 +0900","From":"paul.elder@ideasonboard.com","To":"Umang Jain ","Message-ID":"<20210520031415.GH902042@pyrite.rasen.tech>","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17045,"web_url":"https://patchwork.libcamera.org/comment/17045/","msgid":"","date":"2021-05-20T08:59:50","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, May 20, 2021 at 11:28:10AM +0900, paul.elder@ideasonboard.com wrote:\n> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n> > On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n> > > On 19/05/2021 11:19, Umang Jain wrote:\n> > > > Moving the core.mojom documentation to its corresponding .cpp file\n> > > > (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> > > > documentation for IPABuffer, IPASettings and IPAStream structures.\n> > > > Since the .mojom files are placed in include/ directory, the .cpp file\n> > > > will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> > > > documentation for other mojom generated IPA interfaces in subsequent\n> > > > commit.\n> > > > \n> > > > Also, fix the Doxygen warning for the above IPA structs regarding their\n> > > > constructors not being documented.\n> > > > \n> > > > Signed-off-by: Umang Jain \n> > > > ---\n> > > > Documentation/Doxyfile.in | 8 +-\n> > > > Documentation/meson.build | 1 +\n> > > > include/libcamera/ipa/core.mojom | 72 ----------------\n> > > > src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> > > > src/libcamera/ipa/meson.build | 5 ++\n> > > > src/libcamera/meson.build | 1 +\n> > > > 6 files changed, 118 insertions(+), 74 deletions(-)\n> > > > create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> > > > create mode 100644 src/libcamera/ipa/meson.build\n> > > > \n> > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > > > index af006724..f4d578fa 100644\n> > > > --- a/Documentation/Doxyfile.in\n> > > > +++ b/Documentation/Doxyfile.in\n> > > > @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> > > > \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> > > > \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> > > > \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> > > > -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> > > > \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> > > > \n> > > > # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> > > > @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> > > > # Note that the wildcards are matched against the file with absolute path, so to\n> > > > # exclude all test directories for example use the pattern */test/*\n> > > > \n> > > > -EXCLUDE_PATTERNS =\n> > > > +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> > > \n> > > I was going to suggest that perhaps this should be\n> > > \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n> > > \n> > > But that would exclude the core_ipa_interface.h too, which I presume we\n> > > need to include for Doxygen to map the documentation to.\n> > > \n> > > I wonder if there's a way to include only the files we need under\n> > > include/libcamera/ipa if that list is smaller, rather than trying to\n> > > exclude lots of combinations which would need to be updated with every\n> > > new platform supported.\n> > \n> > I'd rather go for a method that would scale indeed :-) One option is to\n> > modify the naming scheme of generated files to be able to match them by\n> > pattern more easily.\n> \n> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n> anyway, so we could add their documentation (in\n> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n> it in doxygen and meson.\n\nI initially thought that documenting device-specific structures was a\nbit overkill, but given that those are APIs, I'm beginning to reconsider\nthis. I'm fine going in that direction.\n\n> Then the exclude pattern would just be the first two lines.\n> \n> > > > # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> > > > # (namespaces, classes, functions, etc.) that should be excluded from the\n> > > > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > > > index c8521574..9ecf4dfc 100644\n> > > > --- a/Documentation/meson.build\n> > > > +++ b/Documentation/meson.build\n> > > > @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> > > > doxyfile,\n> > > > libcamera_internal_headers,\n> > > > libcamera_ipa_headers,\n> > > > + libcamera_ipa_interfaces,\n> > > > libcamera_public_headers,\n> > > > libcamera_sources,\n> > > > libipa_headers,\n> > > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > > > index 6caaa63e..e49815d8 100644\n> > > > --- a/include/libcamera/ipa/core.mojom\n> > > > +++ b/include/libcamera/ipa/core.mojom\n> > > > @@ -94,88 +94,16 @@ module libcamera;\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> > > > -\n> > > > - /**\n> > > > - * \\var IPASettings::sensorModel\n> > > > - * \\brief The sensor model name\n> > > > - *\n> > > > - * Provides the sensor model name to the IPA.\n> > > > - */\n> > > > struct IPASettings {\n> > > > \tstring configurationFile;\n> > > > \tstring sensorModel;\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> > > > diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> > > > new file mode 100644\n> > > > index 00000000..fe1ecce6\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> > > > @@ -0,0 +1,105 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\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> > > > + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> > > > + * \\param[in] id\n> > > > + * \\param[in] planes\n> > > > + * \\sa id and planes\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> > > > +/**\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> > > > + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> > > > + * \\param[in] configurationFile\n> > > > + * \\param[in] sensorModel\n> > > > + * \\sa configurationFile and sensorModel\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> > > > +\n> > > > +/**\n> > > > + * \\var IPASettings::sensorModel\n> > > > + * \\brief The sensor model name\n> > > > + *\n> > > > + * Provides the sensor model name to the IPA.\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> > > > + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> > > > + * \\param[in] pixelFormat\n> > > > + * \\param[in] size\n> > > > + * \\sa pixelFormat and size\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> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> > > > new file mode 100644\n> > > > index 00000000..0a16d197\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/ipa/meson.build\n> > > > @@ -0,0 +1,5 @@\n> > > > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > > > +\n> > > > +libcamera_ipa_interfaces = files([\n> > > > + 'core_ipa_interface.cpp',\n> > > > +])\n> > > \n> > > I'm sure someone will disagree with me, but I almost feel like this .cpp\n> > > file should get run through a compiler, and therefore just added to the\n> > > libcamera sources as normal (which I think would then already get picked\n> > > up by doxygen).\n> > > \n> > > That way the syntax of the file is maintained too, given that it is a\n> > > C++ file.\n> > \n> > What's the point if it only contains comments though ? Ideally we\n> > shouldn't have this .cpp file in the first place, it should be generated\n> > automatically, or the documentation comments should be added to the\n> > genearated headers by mojo. We can't do so now so we need this kind of\n> > hack, but I'd rather keep the scope of the hack as small as possible.\n> > \n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index 7bc59b84..61b5fe21 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -67,6 +67,7 @@ includes = [\n> > > > libcamera_includes,\n> > > > ]\n> > > > \n> > > > +subdir('ipa')\n> > > > subdir('pipeline')\n> > > > subdir('proxy')\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 24FC2C31FB\n\tfor ;\n\tThu, 20 May 2021 08:59:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D52586891A;\n\tThu, 20 May 2021 10:59:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC59868919\n\tfor ;\n\tThu, 20 May 2021 10:59:52 +0200 (CEST)","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 58DF4D41;\n\tThu, 20 May 2021 10:59:52 +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=\"HeKZU3JC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621501192;\n\tbh=g7K9urexFGo9vk6dtP46hsF3znDY1LN3Ujt/p9ImBM4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HeKZU3JCRCYH7b0hO9Ppt6BCmxTB/jyCAsUD6JnNxwIJVg+2nEN4hw04U4/lRTF0H\n\tKrvv5LJvHlagnhFQ+Xh3QRPi+9xkBIsu/QVOspbAIn10o2z9UCejQXk+GsBPl3Bd9g\n\t8fxl+9KdZ5fEeiL0hESvPsXjnh7lemNkza8yG9/Y=","Date":"Thu, 20 May 2021 11:59:50 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210520022810.GC902042@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17065,"web_url":"https://patchwork.libcamera.org/comment/17065/","msgid":"<942e027d-392d-30ee-e07a-750af1a157ab@ideasonboard.com>","date":"2021-05-21T09:07:15","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi All,\n\nOn 19/05/2021 16:55, Laurent Pinchart wrote:\n> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n>> On 19/05/2021 11:19, Umang Jain wrote:\n>>> Moving the core.mojom documentation to its corresponding .cpp file\n>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n>>> Since the .mojom files are placed in include/ directory, the .cpp file\n>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n>>> documentation for other mojom generated IPA interfaces in subsequent\n>>> commit.\n>>>\n>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n>>> constructors not being documented.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> Documentation/Doxyfile.in | 8 +-\n>>> Documentation/meson.build | 1 +\n>>> include/libcamera/ipa/core.mojom | 72 ----------------\n>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n>>> src/libcamera/ipa/meson.build | 5 ++\n>>> src/libcamera/meson.build | 1 +\n>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n>>> create mode 100644 src/libcamera/ipa/meson.build\n>>>\n>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>>> index af006724..f4d578fa 100644\n>>> --- a/Documentation/Doxyfile.in\n>>> +++ b/Documentation/Doxyfile.in\n>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n>>> \n>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n>>> # Note that the wildcards are matched against the file with absolute path, so to\n>>> # exclude all test directories for example use the pattern */test/*\n>>> \n>>> -EXCLUDE_PATTERNS =\n>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n>>\n>> I was going to suggest that perhaps this should be\n>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n>>\n>> But that would exclude the core_ipa_interface.h too, which I presume we\n>> need to include for Doxygen to map the documentation to.\n>>\n>> I wonder if there's a way to include only the files we need under\n>> include/libcamera/ipa if that list is smaller, rather than trying to\n>> exclude lots of combinations which would need to be updated with every\n>> new platform supported.\n> \n> I'd rather go for a method that would scale indeed :-) One option is to\n> modify the naming scheme of generated files to be able to match them by\n> pattern more easily.\n> \n>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>>> index c8521574..9ecf4dfc 100644\n>>> --- a/Documentation/meson.build\n>>> +++ b/Documentation/meson.build\n>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n>>> doxyfile,\n>>> libcamera_internal_headers,\n>>> libcamera_ipa_headers,\n>>> + libcamera_ipa_interfaces,\n>>> libcamera_public_headers,\n>>> libcamera_sources,\n>>> libipa_headers,\n>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n>>> index 6caaa63e..e49815d8 100644\n>>> --- a/include/libcamera/ipa/core.mojom\n>>> +++ b/include/libcamera/ipa/core.mojom\n>>> @@ -94,88 +94,16 @@ module libcamera;\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>>> -\n>>> - /**\n>>> - * \\var IPASettings::sensorModel\n>>> - * \\brief The sensor model name\n>>> - *\n>>> - * Provides the sensor model name to the IPA.\n>>> - */\n>>> struct IPASettings {\n>>> \tstring configurationFile;\n>>> \tstring sensorModel;\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>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n>>> new file mode 100644\n>>> index 00000000..fe1ecce6\n>>> --- /dev/null\n>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n>>> @@ -0,0 +1,105 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Google Inc.\n>>> + *\n>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n>>> + */\n>>> +\n>>> +namespace libcamera {\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>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n>>> + * \\param[in] id\n>>> + * \\param[in] planes\n>>> + * \\sa id and planes\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>>> +/**\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>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n>>> + * \\param[in] configurationFile\n>>> + * \\param[in] sensorModel\n>>> + * \\sa configurationFile and sensorModel\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>>> +\n>>> +/**\n>>> + * \\var IPASettings::sensorModel\n>>> + * \\brief The sensor model name\n>>> + *\n>>> + * Provides the sensor model name to the IPA.\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>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n>>> + * \\param[in] pixelFormat\n>>> + * \\param[in] size\n>>> + * \\sa pixelFormat and size\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>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n>>> new file mode 100644\n>>> index 00000000..0a16d197\n>>> --- /dev/null\n>>> +++ b/src/libcamera/ipa/meson.build\n>>> @@ -0,0 +1,5 @@\n>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n>>> +\n>>> +libcamera_ipa_interfaces = files([\n>>> + 'core_ipa_interface.cpp',\n>>> +])\n>>\n>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n>> file should get run through a compiler, and therefore just added to the\n>> libcamera sources as normal (which I think would then already get picked\n>> up by doxygen).\n>>\n>> That way the syntax of the file is maintained too, given that it is a\n>> C++ file.\n> \n> What's the point if it only contains comments though ? Ideally we\n> shouldn't have this .cpp file in the first place, it should be generated\n> automatically, or the documentation comments should be added to the\n> genearated headers by mojo. We can't do so now so we need this kind of\n> hack, but I'd rather keep the scope of the hack as small as possible.\n\n\nSee I knew it ;-) But that's my point - I won't object to this. But it\nhighlights that we're doing a hack (or a hack on a hack).\n\na .cpp file to me should be parsed by the compiler.\n\nBut I suppose we can't name these files .doxygen ;-)\n\nBut this is fine for now.\n\n\n> \n>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>> index 7bc59b84..61b5fe21 100644\n>>> --- a/src/libcamera/meson.build\n>>> +++ b/src/libcamera/meson.build\n>>> @@ -67,6 +67,7 @@ includes = [\n>>> libcamera_includes,\n>>> ]\n>>> \n>>> +subdir('ipa')\n>>> subdir('pipeline')\n>>> subdir('proxy')\n>>> \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 75988C31FB\n\tfor ;\n\tFri, 21 May 2021 09:07:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF4E768920;\n\tFri, 21 May 2021 11:07:19 +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 62FBE68918\n\tfor ;\n\tFri, 21 May 2021 11:07:18 +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 AFA778D8;\n\tFri, 21 May 2021 11:07:17 +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=\"i7I9HfwA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621588037;\n\tbh=KsuARvusIObt1ziF4BekcZFLc7BnZXpBxqSRN5pd8lM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=i7I9HfwAyJ1Kcn/SaRUpBnttCOiMJz2qA2GATZykKznQ8/r95YHpRNi5Ck28LYS4v\n\tRVoyBNZpKTzDQ/KZRJmHg9+ZsWibutq7sSrB5PUsRN6AU6hWnzQhOq/qlkE4WSx/+L\n\tnFPiJH3bnh75V+tJk/BakxZQZndLYeFHT2xbgb6U=","To":"Laurent Pinchart ","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<942e027d-392d-30ee-e07a-750af1a157ab@ideasonboard.com>","Date":"Fri, 21 May 2021 10:07:15 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17066,"web_url":"https://patchwork.libcamera.org/comment/17066/","msgid":"<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>","date":"2021-05-21T09:10:41","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang, Paul,\n\nOn 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:\n> Hi Umang,\n> \n> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:\n>> Hi Paul [& others]\n>>\n>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n>>>>> On 19/05/2021 11:19, Umang Jain wrote:\n>>>>>> Moving the core.mojom documentation to its corresponding .cpp file\n>>>>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n>>>>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n>>>>>> Since the .mojom files are placed in include/ directory, the .cpp file\n>>>>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n>>>>>> documentation for other mojom generated IPA interfaces in subsequent\n>>>>>> commit.\n>>>>>>\n>>>>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n>>>>>> constructors not being documented.\n>>>>>>\n>>>>>> Signed-off-by: Umang Jain \n>>>>>> ---\n>>>>>> Documentation/Doxyfile.in | 8 +-\n>>>>>> Documentation/meson.build | 1 +\n>>>>>> include/libcamera/ipa/core.mojom | 72 ----------------\n>>>>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n>>>>>> src/libcamera/ipa/meson.build | 5 ++\n>>>>>> src/libcamera/meson.build | 1 +\n>>>>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n>>>>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>> create mode 100644 src/libcamera/ipa/meson.build\n>>>>>>\n>>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>>>>>> index af006724..f4d578fa 100644\n>>>>>> --- a/Documentation/Doxyfile.in\n>>>>>> +++ b/Documentation/Doxyfile.in\n>>>>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n>>>>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n>>>>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n>>>>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n>>>>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n>>>>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n>>>>>> # Note that the wildcards are matched against the file with absolute path, so to\n>>>>>> # exclude all test directories for example use the pattern */test/*\n>>>>>> -EXCLUDE_PATTERNS =\n>>>>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n>>>>> I was going to suggest that perhaps this should be\n>>>>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n>>>>>\n>>>>> But that would exclude the core_ipa_interface.h too, which I presume we\n>>>>> need to include for Doxygen to map the documentation to.\n>>>>>\n>>>>> I wonder if there's a way to include only the files we need under\n>>>>> include/libcamera/ipa if that list is smaller, rather than trying to\n>>>>> exclude lots of combinations which would need to be updated with every\n>>>>> new platform supported.\n>>>> I'd rather go for a method that would scale indeed :-) One option is to\n>>>> modify the naming scheme of generated files to be able to match them by\n>>>> pattern more easily.\n>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n>>> anyway, so we could add their documentation (in\n>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n>>> it in doxygen and meson.\n>> Yes, documenting all those IPA modules first might be ideal but not sure if\n>> may happen all at once. The way I saw, how would this work would be\n>> addressed is, document one of the modules & exclude it from the\n>> EXCLUDE_PATTERNS for it's generation.\n>>>\n>>> Then the exclude pattern would just be the first two lines.\n>> When all modules are documented we will have this state of exclude pattern\n>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should\n>> only merge them, if they are documented as per the [2/7] precedence.\n>>\n>> Also want to mention that we can file it as a task and thing it's a good\n>> first-comers task to work on.\n> \n> Ah, I see, remove them from the exclude patterns as they're added. Yeah\n> that would be a good first-comer task too.\n> \n> In that case I think a todo comment (either here or in each mojom file?\n> in the mojo file it won't be picked up by doxygen though... what about\n> in dummy interface cpp files?) would be useful.\n\nI'm curious - do we have faults if we don't exclude these now?\nWhat is needed to be able to simply remove these platform specifics as\nthey are?\n\nPresumably it would introduce some doxygen warnings?\n\nIf that's what would happen - I would actually prefer to remove the\nlines - and have the doxygen warnigns printed - providing clear\nmotivation to get them fixed as soon as possible (rather even than a todo)\n\nDoxygen warnings aren't fatal - but they are annoying, and that annoying\nwould help drive getting it fixed.\n\n\nAnyway, With a solution chosen..., for getting this into the .cpp file\nnow to progress this series...\n\nAcked-by: Kieran Bingham \n\n\n> Paul\n> \n>>>\n>>>>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n>>>>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n>>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>>>>>> index c8521574..9ecf4dfc 100644\n>>>>>> --- a/Documentation/meson.build\n>>>>>> +++ b/Documentation/meson.build\n>>>>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n>>>>>> doxyfile,\n>>>>>> libcamera_internal_headers,\n>>>>>> libcamera_ipa_headers,\n>>>>>> + libcamera_ipa_interfaces,\n>>>>>> libcamera_public_headers,\n>>>>>> libcamera_sources,\n>>>>>> libipa_headers,\n>>>>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n>>>>>> index 6caaa63e..e49815d8 100644\n>>>>>> --- a/include/libcamera/ipa/core.mojom\n>>>>>> +++ b/include/libcamera/ipa/core.mojom\n>>>>>> @@ -94,88 +94,16 @@ module libcamera;\n>>>>>> \tuint32 maxFrameLength;\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>>>>>> - * \\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>>>>>> -\n>>>>>> - /**\n>>>>>> - * \\var IPASettings::sensorModel\n>>>>>> - * \\brief The sensor model name\n>>>>>> - *\n>>>>>> - * Provides the sensor model name to the IPA.\n>>>>>> - */\n>>>>>> struct IPASettings {\n>>>>>> \tstring configurationFile;\n>>>>>> \tstring sensorModel;\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>>>>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>> new file mode 100644\n>>>>>> index 00000000..fe1ecce6\n>>>>>> --- /dev/null\n>>>>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>> @@ -0,0 +1,105 @@\n>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>>>> +/*\n>>>>>> + * Copyright (C) 2021, Google Inc.\n>>>>>> + *\n>>>>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n>>>>>> + */\n>>>>>> +\n>>>>>> +namespace libcamera {\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>>>>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n>>>>>> + * \\param[in] id\n>>>>>> + * \\param[in] planes\n>>>>>> + * \\sa id and planes\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>>>>>> +/**\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>>>>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n>>>>>> + * \\param[in] configurationFile\n>>>>>> + * \\param[in] sensorModel\n>>>>>> + * \\sa configurationFile and sensorModel\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>>>>>> +\n>>>>>> +/**\n>>>>>> + * \\var IPASettings::sensorModel\n>>>>>> + * \\brief The sensor model name\n>>>>>> + *\n>>>>>> + * Provides the sensor model name to the IPA.\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>>>>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n>>>>>> + * \\param[in] pixelFormat\n>>>>>> + * \\param[in] size\n>>>>>> + * \\sa pixelFormat and size\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>>>>>> +\n>>>>>> +} /* namespace libcamera */\n>>>>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n>>>>>> new file mode 100644\n>>>>>> index 00000000..0a16d197\n>>>>>> --- /dev/null\n>>>>>> +++ b/src/libcamera/ipa/meson.build\n>>>>>> @@ -0,0 +1,5 @@\n>>>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n>>>>>> +\n>>>>>> +libcamera_ipa_interfaces = files([\n>>>>>> + 'core_ipa_interface.cpp',\n>>>>>> +])\n>>>>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n>>>>> file should get run through a compiler, and therefore just added to the\n>>>>> libcamera sources as normal (which I think would then already get picked\n>>>>> up by doxygen).\n>>>>>\n>>>>> That way the syntax of the file is maintained too, given that it is a\n>>>>> C++ file.\n>>>> What's the point if it only contains comments though ? Ideally we\n>>>> shouldn't have this .cpp file in the first place, it should be generated\n>>>> automatically, or the documentation comments should be added to the\n>>>> genearated headers by mojo. We can't do so now so we need this kind of\n>>>> hack, but I'd rather keep the scope of the hack as small as possible.\n>>>>\n>>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>>>> index 7bc59b84..61b5fe21 100644\n>>>>>> --- a/src/libcamera/meson.build\n>>>>>> +++ b/src/libcamera/meson.build\n>>>>>> @@ -67,6 +67,7 @@ includes = [\n>>>>>> libcamera_includes,\n>>>>>> ]\n>>>>>> +subdir('ipa')\n>>>>>> subdir('pipeline')\n>>>>>> subdir('proxy')\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 A6F62C31FB\n\tfor ;\n\tFri, 21 May 2021 09:10:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 194116891B;\n\tFri, 21 May 2021 11:10:47 +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 BA74168918\n\tfor ;\n\tFri, 21 May 2021 11:10:45 +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 06E37ACC;\n\tFri, 21 May 2021 11:10:44 +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=\"etCrIcth\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621588245;\n\tbh=QOq9luJG964uv5FwuHs1+nCxU5ddSijZfI9W1dzs4ww=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=etCrIcth7psSCrewnARWgSL2qV0iTKOQoIvNrXCVru3Fwbxau3bJiuWXXqLZoSTRl\n\tr7x5es7tJvyQdYcvXtMJUPJasdv+RgV4IqMJL77siteMbPv4XoqN0k7DlRcyqmxInb\n\t2ywKiEYTuWf1i4dkeZtwWngsTqzrQZezzaoplXZA=","To":"paul.elder@ideasonboard.com, Umang Jain ","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>\n\t\n\t<20210520031415.GH902042@pyrite.rasen.tech>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>","Date":"Fri, 21 May 2021 10:10:41 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<20210520031415.GH902042@pyrite.rasen.tech>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17068,"web_url":"https://patchwork.libcamera.org/comment/17068/","msgid":"","date":"2021-05-21T09:15:49","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:\n> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:\n> > On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:\n> >> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n> >>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n> >>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n> >>>>> On 19/05/2021 11:19, Umang Jain wrote:\n> >>>>>> Moving the core.mojom documentation to its corresponding .cpp file\n> >>>>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> >>>>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n> >>>>>> Since the .mojom files are placed in include/ directory, the .cpp file\n> >>>>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> >>>>>> documentation for other mojom generated IPA interfaces in subsequent\n> >>>>>> commit.\n> >>>>>>\n> >>>>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n> >>>>>> constructors not being documented.\n> >>>>>>\n> >>>>>> Signed-off-by: Umang Jain \n> >>>>>> ---\n> >>>>>> Documentation/Doxyfile.in | 8 +-\n> >>>>>> Documentation/meson.build | 1 +\n> >>>>>> include/libcamera/ipa/core.mojom | 72 ----------------\n> >>>>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> >>>>>> src/libcamera/ipa/meson.build | 5 ++\n> >>>>>> src/libcamera/meson.build | 1 +\n> >>>>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n> >>>>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>> create mode 100644 src/libcamera/ipa/meson.build\n> >>>>>>\n> >>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> >>>>>> index af006724..f4d578fa 100644\n> >>>>>> --- a/Documentation/Doxyfile.in\n> >>>>>> +++ b/Documentation/Doxyfile.in\n> >>>>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> >>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> >>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> >>>>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> >>>>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> >>>>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> >>>>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> >>>>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> >>>>>> # Note that the wildcards are matched against the file with absolute path, so to\n> >>>>>> # exclude all test directories for example use the pattern */test/*\n> >>>>>> -EXCLUDE_PATTERNS =\n> >>>>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> >>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> >>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> >>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> >>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> >>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> >>>>> I was going to suggest that perhaps this should be\n> >>>>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n> >>>>>\n> >>>>> But that would exclude the core_ipa_interface.h too, which I presume we\n> >>>>> need to include for Doxygen to map the documentation to.\n> >>>>>\n> >>>>> I wonder if there's a way to include only the files we need under\n> >>>>> include/libcamera/ipa if that list is smaller, rather than trying to\n> >>>>> exclude lots of combinations which would need to be updated with every\n> >>>>> new platform supported.\n> >>>> I'd rather go for a method that would scale indeed :-) One option is to\n> >>>> modify the naming scheme of generated files to be able to match them by\n> >>>> pattern more easily.\n> >>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n> >>> anyway, so we could add their documentation (in\n> >>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n> >>> it in doxygen and meson.\n> >> Yes, documenting all those IPA modules first might be ideal but not sure if\n> >> may happen all at once. The way I saw, how would this work would be\n> >> addressed is, document one of the modules & exclude it from the\n> >> EXCLUDE_PATTERNS for it's generation.\n> >>>\n> >>> Then the exclude pattern would just be the first two lines.\n> >> When all modules are documented we will have this state of exclude pattern\n> >> again. For new IPA (in-tree) modules appearing meanwhile, I think we should\n> >> only merge them, if they are documented as per the [2/7] precedence.\n> >>\n> >> Also want to mention that we can file it as a task and thing it's a good\n> >> first-comers task to work on.\n> > \n> > Ah, I see, remove them from the exclude patterns as they're added. Yeah\n> > that would be a good first-comer task too.\n> > \n> > In that case I think a todo comment (either here or in each mojom file?\n> > in the mojo file it won't be picked up by doxygen though... what about\n> > in dummy interface cpp files?) would be useful.\n> \n> I'm curious - do we have faults if we don't exclude these now?\n> What is needed to be able to simply remove these platform specifics as\n> they are?\n> \n> Presumably it would introduce some doxygen warnings?\n> \n> If that's what would happen - I would actually prefer to remove the\n> lines - and have the doxygen warnigns printed - providing clear\n> motivation to get them fixed as soon as possible (rather even than a todo)\n> \n> Doxygen warnings aren't fatal - but they are annoying, and that annoying\n> would help drive getting it fixed.\n\nWe currently build the documentation without warnings, so I'd consider\nany warning as a regression that would need to be fixed with the highest\npriority, as any other build breakage.\n\n> Anyway, With a solution chosen..., for getting this into the .cpp file\n> now to progress this series...\n> \n> Acked-by: Kieran Bingham \n> \n> >>>>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> >>>>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n> >>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> >>>>>> index c8521574..9ecf4dfc 100644\n> >>>>>> --- a/Documentation/meson.build\n> >>>>>> +++ b/Documentation/meson.build\n> >>>>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> >>>>>> doxyfile,\n> >>>>>> libcamera_internal_headers,\n> >>>>>> libcamera_ipa_headers,\n> >>>>>> + libcamera_ipa_interfaces,\n> >>>>>> libcamera_public_headers,\n> >>>>>> libcamera_sources,\n> >>>>>> libipa_headers,\n> >>>>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> >>>>>> index 6caaa63e..e49815d8 100644\n> >>>>>> --- a/include/libcamera/ipa/core.mojom\n> >>>>>> +++ b/include/libcamera/ipa/core.mojom\n> >>>>>> @@ -94,88 +94,16 @@ module libcamera;\n> >>>>>> \tuint32 maxFrameLength;\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> >>>>>> - * \\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> >>>>>> -\n> >>>>>> - /**\n> >>>>>> - * \\var IPASettings::sensorModel\n> >>>>>> - * \\brief The sensor model name\n> >>>>>> - *\n> >>>>>> - * Provides the sensor model name to the IPA.\n> >>>>>> - */\n> >>>>>> struct IPASettings {\n> >>>>>> \tstring configurationFile;\n> >>>>>> \tstring sensorModel;\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> >>>>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>> new file mode 100644\n> >>>>>> index 00000000..fe1ecce6\n> >>>>>> --- /dev/null\n> >>>>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>> @@ -0,0 +1,105 @@\n> >>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>>>> +/*\n> >>>>>> + * Copyright (C) 2021, Google Inc.\n> >>>>>> + *\n> >>>>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> >>>>>> + */\n> >>>>>> +\n> >>>>>> +namespace libcamera {\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> >>>>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> >>>>>> + * \\param[in] id\n> >>>>>> + * \\param[in] planes\n> >>>>>> + * \\sa id and planes\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> >>>>>> +/**\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> >>>>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> >>>>>> + * \\param[in] configurationFile\n> >>>>>> + * \\param[in] sensorModel\n> >>>>>> + * \\sa configurationFile and sensorModel\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> >>>>>> +\n> >>>>>> +/**\n> >>>>>> + * \\var IPASettings::sensorModel\n> >>>>>> + * \\brief The sensor model name\n> >>>>>> + *\n> >>>>>> + * Provides the sensor model name to the IPA.\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> >>>>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> >>>>>> + * \\param[in] pixelFormat\n> >>>>>> + * \\param[in] size\n> >>>>>> + * \\sa pixelFormat and size\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> >>>>>> +\n> >>>>>> +} /* namespace libcamera */\n> >>>>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> >>>>>> new file mode 100644\n> >>>>>> index 00000000..0a16d197\n> >>>>>> --- /dev/null\n> >>>>>> +++ b/src/libcamera/ipa/meson.build\n> >>>>>> @@ -0,0 +1,5 @@\n> >>>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> >>>>>> +\n> >>>>>> +libcamera_ipa_interfaces = files([\n> >>>>>> + 'core_ipa_interface.cpp',\n> >>>>>> +])\n> >>>>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n> >>>>> file should get run through a compiler, and therefore just added to the\n> >>>>> libcamera sources as normal (which I think would then already get picked\n> >>>>> up by doxygen).\n> >>>>>\n> >>>>> That way the syntax of the file is maintained too, given that it is a\n> >>>>> C++ file.\n> >>>> What's the point if it only contains comments though ? Ideally we\n> >>>> shouldn't have this .cpp file in the first place, it should be generated\n> >>>> automatically, or the documentation comments should be added to the\n> >>>> genearated headers by mojo. We can't do so now so we need this kind of\n> >>>> hack, but I'd rather keep the scope of the hack as small as possible.\n> >>>>\n> >>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>>>>> index 7bc59b84..61b5fe21 100644\n> >>>>>> --- a/src/libcamera/meson.build\n> >>>>>> +++ b/src/libcamera/meson.build\n> >>>>>> @@ -67,6 +67,7 @@ includes = [\n> >>>>>> libcamera_includes,\n> >>>>>> ]\n> >>>>>> +subdir('ipa')\n> >>>>>> subdir('pipeline')\n> >>>>>> subdir('proxy')","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 7D94BC31FF\n\tfor ;\n\tFri, 21 May 2021 09:15:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3A6E6891D;\n\tFri, 21 May 2021 11:15:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1389668918\n\tfor ;\n\tFri, 21 May 2021 11:15:52 +0200 (CEST)","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 7F1038D8;\n\tFri, 21 May 2021 11:15:51 +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=\"pTEvNW2G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621588551;\n\tbh=SRWC9M0c7W9ejiCNtJCzVjDFdCBzc8d7W93pNjiZYxs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pTEvNW2GkahG3YKJzI+4pab8P9xiUbswjZ1cv678FnGHOyDq4E2fP0KiNMe+FwcuV\n\tHV1+h3cqtSSSOFauF7+ptAse+z89KfgA1eoreGsoHI/gVMT6hEA9ZD+2n16ONq5GAf\n\t8DPh8eGbQptuP+NnTLRM51EloDfABPxtFOgf1rdM=","Date":"Fri, 21 May 2021 12:15:49 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>\n\t\n\t<20210520031415.GH902042@pyrite.rasen.tech>\n\t<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17070,"web_url":"https://patchwork.libcamera.org/comment/17070/","msgid":"<6c20de85-dd98-5fe9-5bb0-8ed9cc50959c@ideasonboard.com>","date":"2021-05-21T09:19:59","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 21/05/2021 10:15, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:\n>> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:\n>>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:\n>>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n>>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n>>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n>>>>>>> On 19/05/2021 11:19, Umang Jain wrote:\n>>>>>>>> Moving the core.mojom documentation to its corresponding .cpp file\n>>>>>>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n>>>>>>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n>>>>>>>> Since the .mojom files are placed in include/ directory, the .cpp file\n>>>>>>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n>>>>>>>> documentation for other mojom generated IPA interfaces in subsequent\n>>>>>>>> commit.\n>>>>>>>>\n>>>>>>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n>>>>>>>> constructors not being documented.\n>>>>>>>>\n>>>>>>>> Signed-off-by: Umang Jain \n>>>>>>>> ---\n>>>>>>>> Documentation/Doxyfile.in | 8 +-\n>>>>>>>> Documentation/meson.build | 1 +\n>>>>>>>> include/libcamera/ipa/core.mojom | 72 ----------------\n>>>>>>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n>>>>>>>> src/libcamera/ipa/meson.build | 5 ++\n>>>>>>>> src/libcamera/meson.build | 1 +\n>>>>>>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n>>>>>>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>>>> create mode 100644 src/libcamera/ipa/meson.build\n>>>>>>>>\n>>>>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>>>>>>>> index af006724..f4d578fa 100644\n>>>>>>>> --- a/Documentation/Doxyfile.in\n>>>>>>>> +++ b/Documentation/Doxyfile.in\n>>>>>>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n>>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n>>>>>>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n>>>>>>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n>>>>>>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n>>>>>>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n>>>>>>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n>>>>>>>> # Note that the wildcards are matched against the file with absolute path, so to\n>>>>>>>> # exclude all test directories for example use the pattern */test/*\n>>>>>>>> -EXCLUDE_PATTERNS =\n>>>>>>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n>>>>>>> I was going to suggest that perhaps this should be\n>>>>>>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n>>>>>>>\n>>>>>>> But that would exclude the core_ipa_interface.h too, which I presume we\n>>>>>>> need to include for Doxygen to map the documentation to.\n>>>>>>>\n>>>>>>> I wonder if there's a way to include only the files we need under\n>>>>>>> include/libcamera/ipa if that list is smaller, rather than trying to\n>>>>>>> exclude lots of combinations which would need to be updated with every\n>>>>>>> new platform supported.\n>>>>>> I'd rather go for a method that would scale indeed :-) One option is to\n>>>>>> modify the naming scheme of generated files to be able to match them by\n>>>>>> pattern more easily.\n>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n>>>>> anyway, so we could add their documentation (in\n>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n>>>>> it in doxygen and meson.\n>>>> Yes, documenting all those IPA modules first might be ideal but not sure if\n>>>> may happen all at once. The way I saw, how would this work would be\n>>>> addressed is, document one of the modules & exclude it from the\n>>>> EXCLUDE_PATTERNS for it's generation.\n>>>>>\n>>>>> Then the exclude pattern would just be the first two lines.\n>>>> When all modules are documented we will have this state of exclude pattern\n>>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should\n>>>> only merge them, if they are documented as per the [2/7] precedence.\n>>>>\n>>>> Also want to mention that we can file it as a task and thing it's a good\n>>>> first-comers task to work on.\n>>>\n>>> Ah, I see, remove them from the exclude patterns as they're added. Yeah\n>>> that would be a good first-comer task too.\n>>>\n>>> In that case I think a todo comment (either here or in each mojom file?\n>>> in the mojo file it won't be picked up by doxygen though... what about\n>>> in dummy interface cpp files?) would be useful.\n>>\n>> I'm curious - do we have faults if we don't exclude these now?\n>> What is needed to be able to simply remove these platform specifics as\n>> they are?\n>>\n>> Presumably it would introduce some doxygen warnings?\n>>\n>> If that's what would happen - I would actually prefer to remove the\n>> lines - and have the doxygen warnigns printed - providing clear\n>> motivation to get them fixed as soon as possible (rather even than a todo)\n>>\n>> Doxygen warnings aren't fatal - but they are annoying, and that annoying\n>> would help drive getting it fixed.\n> \n> We currently build the documentation without warnings, so I'd consider\n> any warning as a regression that would need to be fixed with the highest\n> priority, as any other build breakage.\n\nThat's sort of my point - the question is ... can we knowingly introduce\nthose warnings, or should we paper over them by excluding the files\nuntil the documentation is moved over as is done in this patch.\n\nIf we remove the lines:\n @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n\nfrom above, I think what will happen is some warnings will be\nintroduced, which will mean we must then handle the documentation for\nRPi/Vimc/RKISP/IPU3 as a priority.\n\nEither way, as long as it all gets done is what matters.\n\n--\nKieran\n\n\n\n> \n>> Anyway, With a solution chosen..., for getting this into the .cpp file\n>> now to progress this series...\n>>\n>> Acked-by: Kieran Bingham \n>>\n>>>>>>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n>>>>>>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n>>>>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>>>>>>>> index c8521574..9ecf4dfc 100644\n>>>>>>>> --- a/Documentation/meson.build\n>>>>>>>> +++ b/Documentation/meson.build\n>>>>>>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n>>>>>>>> doxyfile,\n>>>>>>>> libcamera_internal_headers,\n>>>>>>>> libcamera_ipa_headers,\n>>>>>>>> + libcamera_ipa_interfaces,\n>>>>>>>> libcamera_public_headers,\n>>>>>>>> libcamera_sources,\n>>>>>>>> libipa_headers,\n>>>>>>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n>>>>>>>> index 6caaa63e..e49815d8 100644\n>>>>>>>> --- a/include/libcamera/ipa/core.mojom\n>>>>>>>> +++ b/include/libcamera/ipa/core.mojom\n>>>>>>>> @@ -94,88 +94,16 @@ module libcamera;\n>>>>>>>> \tuint32 maxFrameLength;\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>>>>>>>> - * \\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>>>>>>>> -\n>>>>>>>> - /**\n>>>>>>>> - * \\var IPASettings::sensorModel\n>>>>>>>> - * \\brief The sensor model name\n>>>>>>>> - *\n>>>>>>>> - * Provides the sensor model name to the IPA.\n>>>>>>>> - */\n>>>>>>>> struct IPASettings {\n>>>>>>>> \tstring configurationFile;\n>>>>>>>> \tstring sensorModel;\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>>>>>>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>>>> new file mode 100644\n>>>>>>>> index 00000000..fe1ecce6\n>>>>>>>> --- /dev/null\n>>>>>>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>>>> @@ -0,0 +1,105 @@\n>>>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>>>>>> +/*\n>>>>>>>> + * Copyright (C) 2021, Google Inc.\n>>>>>>>> + *\n>>>>>>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n>>>>>>>> + */\n>>>>>>>> +\n>>>>>>>> +namespace libcamera {\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>>>>>>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n>>>>>>>> + * \\param[in] id\n>>>>>>>> + * \\param[in] planes\n>>>>>>>> + * \\sa id and planes\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>>>>>>>> +/**\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>>>>>>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n>>>>>>>> + * \\param[in] configurationFile\n>>>>>>>> + * \\param[in] sensorModel\n>>>>>>>> + * \\sa configurationFile and sensorModel\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>>>>>>>> +\n>>>>>>>> +/**\n>>>>>>>> + * \\var IPASettings::sensorModel\n>>>>>>>> + * \\brief The sensor model name\n>>>>>>>> + *\n>>>>>>>> + * Provides the sensor model name to the IPA.\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>>>>>>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n>>>>>>>> + * \\param[in] pixelFormat\n>>>>>>>> + * \\param[in] size\n>>>>>>>> + * \\sa pixelFormat and size\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>>>>>>>> +\n>>>>>>>> +} /* namespace libcamera */\n>>>>>>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n>>>>>>>> new file mode 100644\n>>>>>>>> index 00000000..0a16d197\n>>>>>>>> --- /dev/null\n>>>>>>>> +++ b/src/libcamera/ipa/meson.build\n>>>>>>>> @@ -0,0 +1,5 @@\n>>>>>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n>>>>>>>> +\n>>>>>>>> +libcamera_ipa_interfaces = files([\n>>>>>>>> + 'core_ipa_interface.cpp',\n>>>>>>>> +])\n>>>>>>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n>>>>>>> file should get run through a compiler, and therefore just added to the\n>>>>>>> libcamera sources as normal (which I think would then already get picked\n>>>>>>> up by doxygen).\n>>>>>>>\n>>>>>>> That way the syntax of the file is maintained too, given that it is a\n>>>>>>> C++ file.\n>>>>>> What's the point if it only contains comments though ? Ideally we\n>>>>>> shouldn't have this .cpp file in the first place, it should be generated\n>>>>>> automatically, or the documentation comments should be added to the\n>>>>>> genearated headers by mojo. We can't do so now so we need this kind of\n>>>>>> hack, but I'd rather keep the scope of the hack as small as possible.\n>>>>>>\n>>>>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>>>>>> index 7bc59b84..61b5fe21 100644\n>>>>>>>> --- a/src/libcamera/meson.build\n>>>>>>>> +++ b/src/libcamera/meson.build\n>>>>>>>> @@ -67,6 +67,7 @@ includes = [\n>>>>>>>> libcamera_includes,\n>>>>>>>> ]\n>>>>>>>> +subdir('ipa')\n>>>>>>>> subdir('pipeline')\n>>>>>>>> subdir('proxy')\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 B1EF9C31FB\n\tfor ;\n\tFri, 21 May 2021 09:20:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 013F76891D;\n\tFri, 21 May 2021 11:20:05 +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 0C13768918\n\tfor ;\n\tFri, 21 May 2021 11:20:03 +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 47BA7ACC;\n\tFri, 21 May 2021 11:20:02 +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=\"KLXYYxt2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621588802;\n\tbh=+14txnYveQPbqSAONX109VCnMMHyGeazD36S/7L/5JI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=KLXYYxt2cfrm9pjCMYeSyk4HEUdtmW9EYhpkKygvK2wOY1Jr4WMV1FrXmnFuMC1x5\n\tBtVjz36sP6vTimoopzWfhc8XgrTSMY8/KwUwFIWH8iEHYXEYj/UY2MXYN7OD9eZPK1\n\tDXBmu63v9kv8OLjH/EWuTZ1QBtDDMpt6WhKlr26E=","To":"Laurent Pinchart ","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>\n\t\n\t<20210520031415.GH902042@pyrite.rasen.tech>\n\t<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<6c20de85-dd98-5fe9-5bb0-8ed9cc50959c@ideasonboard.com>","Date":"Fri, 21 May 2021 10:19:59 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17072,"web_url":"https://patchwork.libcamera.org/comment/17072/","msgid":"","date":"2021-05-21T09:27:10","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, May 21, 2021 at 10:19:59AM +0100, Kieran Bingham wrote:\n> On 21/05/2021 10:15, Laurent Pinchart wrote:\n> > On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:\n> >> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:\n> >>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:\n> >>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n> >>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n> >>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n> >>>>>>> On 19/05/2021 11:19, Umang Jain wrote:\n> >>>>>>>> Moving the core.mojom documentation to its corresponding .cpp file\n> >>>>>>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> >>>>>>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n> >>>>>>>> Since the .mojom files are placed in include/ directory, the .cpp file\n> >>>>>>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> >>>>>>>> documentation for other mojom generated IPA interfaces in subsequent\n> >>>>>>>> commit.\n> >>>>>>>>\n> >>>>>>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n> >>>>>>>> constructors not being documented.\n> >>>>>>>>\n> >>>>>>>> Signed-off-by: Umang Jain \n> >>>>>>>> ---\n> >>>>>>>> Documentation/Doxyfile.in | 8 +-\n> >>>>>>>> Documentation/meson.build | 1 +\n> >>>>>>>> include/libcamera/ipa/core.mojom | 72 ----------------\n> >>>>>>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> >>>>>>>> src/libcamera/ipa/meson.build | 5 ++\n> >>>>>>>> src/libcamera/meson.build | 1 +\n> >>>>>>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n> >>>>>>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>>>> create mode 100644 src/libcamera/ipa/meson.build\n> >>>>>>>>\n> >>>>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> >>>>>>>> index af006724..f4d578fa 100644\n> >>>>>>>> --- a/Documentation/Doxyfile.in\n> >>>>>>>> +++ b/Documentation/Doxyfile.in\n> >>>>>>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> >>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> >>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> >>>>>>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> >>>>>>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> >>>>>>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> >>>>>>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> >>>>>>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> >>>>>>>> # Note that the wildcards are matched against the file with absolute path, so to\n> >>>>>>>> # exclude all test directories for example use the pattern */test/*\n> >>>>>>>> -EXCLUDE_PATTERNS =\n> >>>>>>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> >>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> >>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> >>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> >>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> >>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> >>>>>>> I was going to suggest that perhaps this should be\n> >>>>>>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n> >>>>>>>\n> >>>>>>> But that would exclude the core_ipa_interface.h too, which I presume we\n> >>>>>>> need to include for Doxygen to map the documentation to.\n> >>>>>>>\n> >>>>>>> I wonder if there's a way to include only the files we need under\n> >>>>>>> include/libcamera/ipa if that list is smaller, rather than trying to\n> >>>>>>> exclude lots of combinations which would need to be updated with every\n> >>>>>>> new platform supported.\n> >>>>>> I'd rather go for a method that would scale indeed :-) One option is to\n> >>>>>> modify the naming scheme of generated files to be able to match them by\n> >>>>>> pattern more easily.\n> >>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n> >>>>> anyway, so we could add their documentation (in\n> >>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n> >>>>> it in doxygen and meson.\n> >>>> Yes, documenting all those IPA modules first might be ideal but not sure if\n> >>>> may happen all at once. The way I saw, how would this work would be\n> >>>> addressed is, document one of the modules & exclude it from the\n> >>>> EXCLUDE_PATTERNS for it's generation.\n> >>>>>\n> >>>>> Then the exclude pattern would just be the first two lines.\n> >>>> When all modules are documented we will have this state of exclude pattern\n> >>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should\n> >>>> only merge them, if they are documented as per the [2/7] precedence.\n> >>>>\n> >>>> Also want to mention that we can file it as a task and thing it's a good\n> >>>> first-comers task to work on.\n> >>>\n> >>> Ah, I see, remove them from the exclude patterns as they're added. Yeah\n> >>> that would be a good first-comer task too.\n> >>>\n> >>> In that case I think a todo comment (either here or in each mojom file?\n> >>> in the mojo file it won't be picked up by doxygen though... what about\n> >>> in dummy interface cpp files?) would be useful.\n> >>\n> >> I'm curious - do we have faults if we don't exclude these now?\n> >> What is needed to be able to simply remove these platform specifics as\n> >> they are?\n> >>\n> >> Presumably it would introduce some doxygen warnings?\n> >>\n> >> If that's what would happen - I would actually prefer to remove the\n> >> lines - and have the doxygen warnigns printed - providing clear\n> >> motivation to get them fixed as soon as possible (rather even than a todo)\n> >>\n> >> Doxygen warnings aren't fatal - but they are annoying, and that annoying\n> >> would help drive getting it fixed.\n> > \n> > We currently build the documentation without warnings, so I'd consider\n> > any warning as a regression that would need to be fixed with the highest\n> > priority, as any other build breakage.\n> \n> That's sort of my point - the question is ... can we knowingly introduce\n> those warnings, or should we paper over them by excluding the files\n> until the documentation is moved over as is done in this patch.\n\nSame answer as for knowingly introducing any other build breakage :-) If\nit has to be fixed with the highest priority, it can as well be done as\npart of the series.\n\n> If we remove the lines:\n> @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> \n> from above, I think what will happen is some warnings will be\n> introduced, which will mean we must then handle the documentation for\n> RPi/Vimc/RKISP/IPU3 as a priority.\n> \n> Either way, as long as it all gets done is what matters.\n> \n> >> Anyway, With a solution chosen..., for getting this into the .cpp file\n> >> now to progress this series...\n> >>\n> >> Acked-by: Kieran Bingham \n> >>\n> >>>>>>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> >>>>>>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n> >>>>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> >>>>>>>> index c8521574..9ecf4dfc 100644\n> >>>>>>>> --- a/Documentation/meson.build\n> >>>>>>>> +++ b/Documentation/meson.build\n> >>>>>>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> >>>>>>>> doxyfile,\n> >>>>>>>> libcamera_internal_headers,\n> >>>>>>>> libcamera_ipa_headers,\n> >>>>>>>> + libcamera_ipa_interfaces,\n> >>>>>>>> libcamera_public_headers,\n> >>>>>>>> libcamera_sources,\n> >>>>>>>> libipa_headers,\n> >>>>>>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> >>>>>>>> index 6caaa63e..e49815d8 100644\n> >>>>>>>> --- a/include/libcamera/ipa/core.mojom\n> >>>>>>>> +++ b/include/libcamera/ipa/core.mojom\n> >>>>>>>> @@ -94,88 +94,16 @@ module libcamera;\n> >>>>>>>> \tuint32 maxFrameLength;\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> >>>>>>>> - * \\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> >>>>>>>> -\n> >>>>>>>> - /**\n> >>>>>>>> - * \\var IPASettings::sensorModel\n> >>>>>>>> - * \\brief The sensor model name\n> >>>>>>>> - *\n> >>>>>>>> - * Provides the sensor model name to the IPA.\n> >>>>>>>> - */\n> >>>>>>>> struct IPASettings {\n> >>>>>>>> \tstring configurationFile;\n> >>>>>>>> \tstring sensorModel;\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> >>>>>>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>>>> new file mode 100644\n> >>>>>>>> index 00000000..fe1ecce6\n> >>>>>>>> --- /dev/null\n> >>>>>>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>>>> @@ -0,0 +1,105 @@\n> >>>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>>>>>> +/*\n> >>>>>>>> + * Copyright (C) 2021, Google Inc.\n> >>>>>>>> + *\n> >>>>>>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> >>>>>>>> + */\n> >>>>>>>> +\n> >>>>>>>> +namespace libcamera {\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> >>>>>>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> >>>>>>>> + * \\param[in] id\n> >>>>>>>> + * \\param[in] planes\n> >>>>>>>> + * \\sa id and planes\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> >>>>>>>> +/**\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> >>>>>>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> >>>>>>>> + * \\param[in] configurationFile\n> >>>>>>>> + * \\param[in] sensorModel\n> >>>>>>>> + * \\sa configurationFile and sensorModel\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> >>>>>>>> +\n> >>>>>>>> +/**\n> >>>>>>>> + * \\var IPASettings::sensorModel\n> >>>>>>>> + * \\brief The sensor model name\n> >>>>>>>> + *\n> >>>>>>>> + * Provides the sensor model name to the IPA.\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> >>>>>>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> >>>>>>>> + * \\param[in] pixelFormat\n> >>>>>>>> + * \\param[in] size\n> >>>>>>>> + * \\sa pixelFormat and size\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> >>>>>>>> +\n> >>>>>>>> +} /* namespace libcamera */\n> >>>>>>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> >>>>>>>> new file mode 100644\n> >>>>>>>> index 00000000..0a16d197\n> >>>>>>>> --- /dev/null\n> >>>>>>>> +++ b/src/libcamera/ipa/meson.build\n> >>>>>>>> @@ -0,0 +1,5 @@\n> >>>>>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> >>>>>>>> +\n> >>>>>>>> +libcamera_ipa_interfaces = files([\n> >>>>>>>> + 'core_ipa_interface.cpp',\n> >>>>>>>> +])\n> >>>>>>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n> >>>>>>> file should get run through a compiler, and therefore just added to the\n> >>>>>>> libcamera sources as normal (which I think would then already get picked\n> >>>>>>> up by doxygen).\n> >>>>>>>\n> >>>>>>> That way the syntax of the file is maintained too, given that it is a\n> >>>>>>> C++ file.\n> >>>>>> What's the point if it only contains comments though ? Ideally we\n> >>>>>> shouldn't have this .cpp file in the first place, it should be generated\n> >>>>>> automatically, or the documentation comments should be added to the\n> >>>>>> genearated headers by mojo. We can't do so now so we need this kind of\n> >>>>>> hack, but I'd rather keep the scope of the hack as small as possible.\n> >>>>>>\n> >>>>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>>>>>>> index 7bc59b84..61b5fe21 100644\n> >>>>>>>> --- a/src/libcamera/meson.build\n> >>>>>>>> +++ b/src/libcamera/meson.build\n> >>>>>>>> @@ -67,6 +67,7 @@ includes = [\n> >>>>>>>> libcamera_includes,\n> >>>>>>>> ]\n> >>>>>>>> +subdir('ipa')\n> >>>>>>>> subdir('pipeline')\n> >>>>>>>> subdir('proxy')","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 01598C31FB\n\tfor ;\n\tFri, 21 May 2021 09:27:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2DF16891D;\n\tFri, 21 May 2021 11:27:14 +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 ED40B68918\n\tfor ;\n\tFri, 21 May 2021 11:27:12 +0200 (CEST)","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 7E965ACC;\n\tFri, 21 May 2021 11:27:12 +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=\"U79Dfp7Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621589232;\n\tbh=vVMn5f2moVXnEWG7XBNFkJ5r8Q0hfJPIm3kkkAdYiLg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U79Dfp7YM0lxpkGrCi8yA7UoNK0D4DD3wHEugNYRwr/KabaikDvdqapmWTFQn3BDV\n\tqap7xwV10THzsqpF5PW9KwoGeAzcPNMVPEshEndO2f09fGnnIhabR0qGFM8NpWbmCo\n\tfUMtTGgrKPBEe2EygOSdDHD4YlDPhNoHcXoilJhI=","Date":"Fri, 21 May 2021 12:27:10 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>\n\t\n\t<20210520031415.GH902042@pyrite.rasen.tech>\n\t<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>\n\t\n\t<6c20de85-dd98-5fe9-5bb0-8ed9cc50959c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6c20de85-dd98-5fe9-5bb0-8ed9cc50959c@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17088,"web_url":"https://patchwork.libcamera.org/comment/17088/","msgid":"","date":"2021-05-21T11:33:27","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"hi All,\n\nOn 5/21/21 2:57 PM, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> On Fri, May 21, 2021 at 10:19:59AM +0100, Kieran Bingham wrote:\n>> On 21/05/2021 10:15, Laurent Pinchart wrote:\n>>> On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:\n>>>> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:\n>>>>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:\n>>>>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n>>>>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n>>>>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n>>>>>>>>> On 19/05/2021 11:19, Umang Jain wrote:\n>>>>>>>>>> Moving the core.mojom documentation to its corresponding .cpp file\n>>>>>>>>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n>>>>>>>>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n>>>>>>>>>> Since the .mojom files are placed in include/ directory, the .cpp file\n>>>>>>>>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n>>>>>>>>>> documentation for other mojom generated IPA interfaces in subsequent\n>>>>>>>>>> commit.\n>>>>>>>>>>\n>>>>>>>>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n>>>>>>>>>> constructors not being documented.\n>>>>>>>>>>\n>>>>>>>>>> Signed-off-by: Umang Jain \n>>>>>>>>>> ---\n>>>>>>>>>> Documentation/Doxyfile.in | 8 +-\n>>>>>>>>>> Documentation/meson.build | 1 +\n>>>>>>>>>> include/libcamera/ipa/core.mojom | 72 ----------------\n>>>>>>>>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n>>>>>>>>>> src/libcamera/ipa/meson.build | 5 ++\n>>>>>>>>>> src/libcamera/meson.build | 1 +\n>>>>>>>>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n>>>>>>>>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>>>>>> create mode 100644 src/libcamera/ipa/meson.build\n>>>>>>>>>>\n>>>>>>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>>>>>>>>>> index af006724..f4d578fa 100644\n>>>>>>>>>> --- a/Documentation/Doxyfile.in\n>>>>>>>>>> +++ b/Documentation/Doxyfile.in\n>>>>>>>>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n>>>>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>>>>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n>>>>>>>>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n>>>>>>>>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n>>>>>>>>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n>>>>>>>>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n>>>>>>>>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n>>>>>>>>>> # Note that the wildcards are matched against the file with absolute path, so to\n>>>>>>>>>> # exclude all test directories for example use the pattern */test/*\n>>>>>>>>>> -EXCLUDE_PATTERNS =\n>>>>>>>>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n>>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n>>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n>>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n>>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n>>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n>>>>>>>>> I was going to suggest that perhaps this should be\n>>>>>>>>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n>>>>>>>>>\n>>>>>>>>> But that would exclude the core_ipa_interface.h too, which I presume we\n>>>>>>>>> need to include for Doxygen to map the documentation to.\n>>>>>>>>>\n>>>>>>>>> I wonder if there's a way to include only the files we need under\n>>>>>>>>> include/libcamera/ipa if that list is smaller, rather than trying to\n>>>>>>>>> exclude lots of combinations which would need to be updated with every\n>>>>>>>>> new platform supported.\n>>>>>>>> I'd rather go for a method that would scale indeed :-) One option is to\n>>>>>>>> modify the naming scheme of generated files to be able to match them by\n>>>>>>>> pattern more easily.\n>>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n>>>>>>> anyway, so we could add their documentation (in\n>>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n>>>>>>> it in doxygen and meson.\n>>>>>> Yes, documenting all those IPA modules first might be ideal but not sure if\n>>>>>> may happen all at once. The way I saw, how would this work would be\n>>>>>> addressed is, document one of the modules & exclude it from the\n>>>>>> EXCLUDE_PATTERNS for it's generation.\n>>>>>>> Then the exclude pattern would just be the first two lines.\n>>>>>> When all modules are documented we will have this state of exclude pattern\n>>>>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should\n>>>>>> only merge them, if they are documented as per the [2/7] precedence.\n>>>>>>\n>>>>>> Also want to mention that we can file it as a task and thing it's a good\n>>>>>> first-comers task to work on.\n>>>>> Ah, I see, remove them from the exclude patterns as they're added. Yeah\n>>>>> that would be a good first-comer task too.\n>>>>>\n>>>>> In that case I think a todo comment (either here or in each mojom file?\n>>>>> in the mojo file it won't be picked up by doxygen though... what about\n>>>>> in dummy interface cpp files?) would be useful.\n>>>> I'm curious - do we have faults if we don't exclude these now?\n>>>> What is needed to be able to simply remove these platform specifics as\n>>>> they are?\n>>>>\n>>>> Presumably it would introduce some doxygen warnings?\n>>>>\n>>>> If that's what would happen - I would actually prefer to remove the\n>>>> lines - and have the doxygen warnigns printed - providing clear\n>>>> motivation to get them fixed as soon as possible (rather even than a todo)\n>>>>\n>>>> Doxygen warnings aren't fatal - but they are annoying, and that annoying\n>>>> would help drive getting it fixed.\n>>> We currently build the documentation without warnings, so I'd consider\n>>> any warning as a regression that would need to be fixed with the highest\n>>> priority, as any other build breakage.\n>> That's sort of my point - the question is ... can we knowingly introduce\n>> those warnings, or should we paper over them by excluding the files\n>> until the documentation is moved over as is done in this patch.\n> Same answer as for knowingly introducing any other build breakage :-) If\n> it has to be fixed with the highest priority, it can as well be done as\n> part of the series.\nIt's not highest priority for this series at-least, given that we were \njust testing waters on how to keep up on documentation for .mojom files. \nI know it's a kind of hack but it's a good stopping point to not get \nahead of yourselves. I, would really like if we were able to pass in \nmojom files directly to doxygen (even if going through a intermediatary \nstep, like extracting comments out first) - but it would be a quite a \nwork, I think, to get the plumbing in place. I suspected, that \ndocumenting all IPAs interfaces will also be take some - hence I have \nleft it at a point where it can be done in phases/per-IPA basis, if \ndecided to carry on with this precedence of documenting mojom.\n\nAlso, Maybe we need a \"doc\" sprint week :P\n>\n>> If we remove the lines:\n>> @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n>> @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n>> @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n>> @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n>>\n>> from above, I think what will happen is some warnings will be\n>> introduced, which will mean we must then handle the documentation for\n>> RPi/Vimc/RKISP/IPU3 as a priority.\n>>\n>> Either way, as long as it all gets done is what matters.\n>>\n>>>> Anyway, With a solution chosen..., for getting this into the .cpp file\n>>>> now to progress this series...\n>>>>\n>>>> Acked-by: Kieran Bingham \n>>>>\n>>>>>>>>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n>>>>>>>>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n>>>>>>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>>>>>>>>>> index c8521574..9ecf4dfc 100644\n>>>>>>>>>> --- a/Documentation/meson.build\n>>>>>>>>>> +++ b/Documentation/meson.build\n>>>>>>>>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n>>>>>>>>>> doxyfile,\n>>>>>>>>>> libcamera_internal_headers,\n>>>>>>>>>> libcamera_ipa_headers,\n>>>>>>>>>> + libcamera_ipa_interfaces,\n>>>>>>>>>> libcamera_public_headers,\n>>>>>>>>>> libcamera_sources,\n>>>>>>>>>> libipa_headers,\n>>>>>>>>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n>>>>>>>>>> index 6caaa63e..e49815d8 100644\n>>>>>>>>>> --- a/include/libcamera/ipa/core.mojom\n>>>>>>>>>> +++ b/include/libcamera/ipa/core.mojom\n>>>>>>>>>> @@ -94,88 +94,16 @@ module libcamera;\n>>>>>>>>>> \tuint32 maxFrameLength;\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>>>>>>>>>> - * \\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>>>>>>>>>> -\n>>>>>>>>>> - /**\n>>>>>>>>>> - * \\var IPASettings::sensorModel\n>>>>>>>>>> - * \\brief The sensor model name\n>>>>>>>>>> - *\n>>>>>>>>>> - * Provides the sensor model name to the IPA.\n>>>>>>>>>> - */\n>>>>>>>>>> struct IPASettings {\n>>>>>>>>>> \tstring configurationFile;\n>>>>>>>>>> \tstring sensorModel;\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>>>>>>>>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>>>>>> new file mode 100644\n>>>>>>>>>> index 00000000..fe1ecce6\n>>>>>>>>>> --- /dev/null\n>>>>>>>>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n>>>>>>>>>> @@ -0,0 +1,105 @@\n>>>>>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>>>>>>>> +/*\n>>>>>>>>>> + * Copyright (C) 2021, Google Inc.\n>>>>>>>>>> + *\n>>>>>>>>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n>>>>>>>>>> + */\n>>>>>>>>>> +\n>>>>>>>>>> +namespace libcamera {\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>>>>>>>>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n>>>>>>>>>> + * \\param[in] id\n>>>>>>>>>> + * \\param[in] planes\n>>>>>>>>>> + * \\sa id and planes\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>>>>>>>>>> +/**\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>>>>>>>>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n>>>>>>>>>> + * \\param[in] configurationFile\n>>>>>>>>>> + * \\param[in] sensorModel\n>>>>>>>>>> + * \\sa configurationFile and sensorModel\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>>>>>>>>>> +\n>>>>>>>>>> +/**\n>>>>>>>>>> + * \\var IPASettings::sensorModel\n>>>>>>>>>> + * \\brief The sensor model name\n>>>>>>>>>> + *\n>>>>>>>>>> + * Provides the sensor model name to the IPA.\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>>>>>>>>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n>>>>>>>>>> + * \\param[in] pixelFormat\n>>>>>>>>>> + * \\param[in] size\n>>>>>>>>>> + * \\sa pixelFormat and size\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>>>>>>>>>> +\n>>>>>>>>>> +} /* namespace libcamera */\n>>>>>>>>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n>>>>>>>>>> new file mode 100644\n>>>>>>>>>> index 00000000..0a16d197\n>>>>>>>>>> --- /dev/null\n>>>>>>>>>> +++ b/src/libcamera/ipa/meson.build\n>>>>>>>>>> @@ -0,0 +1,5 @@\n>>>>>>>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n>>>>>>>>>> +\n>>>>>>>>>> +libcamera_ipa_interfaces = files([\n>>>>>>>>>> + 'core_ipa_interface.cpp',\n>>>>>>>>>> +])\n>>>>>>>>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n>>>>>>>>> file should get run through a compiler, and therefore just added to the\n>>>>>>>>> libcamera sources as normal (which I think would then already get picked\n>>>>>>>>> up by doxygen).\n>>>>>>>>>\n>>>>>>>>> That way the syntax of the file is maintained too, given that it is a\n>>>>>>>>> C++ file.\n>>>>>>>> What's the point if it only contains comments though ? Ideally we\n>>>>>>>> shouldn't have this .cpp file in the first place, it should be generated\n>>>>>>>> automatically, or the documentation comments should be added to the\n>>>>>>>> genearated headers by mojo. We can't do so now so we need this kind of\n>>>>>>>> hack, but I'd rather keep the scope of the hack as small as possible.\n>>>>>>>>\n>>>>>>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>>>>>>>> index 7bc59b84..61b5fe21 100644\n>>>>>>>>>> --- a/src/libcamera/meson.build\n>>>>>>>>>> +++ b/src/libcamera/meson.build\n>>>>>>>>>> @@ -67,6 +67,7 @@ includes = [\n>>>>>>>>>> libcamera_includes,\n>>>>>>>>>> ]\n>>>>>>>>>> +subdir('ipa')\n>>>>>>>>>> subdir('pipeline')\n>>>>>>>>>> subdir('proxy')","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 0346EC3200\n\tfor ;\n\tFri, 21 May 2021 11:33:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B280B6891E;\n\tFri, 21 May 2021 13:33:34 +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 8CF6F68911\n\tfor ;\n\tFri, 21 May 2021 13:33:33 +0200 (CEST)","from localhost.localdomain (unknown [103.251.226.162])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0086E8D8;\n\tFri, 21 May 2021 13:33:31 +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=\"mBrO4fc3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621596813;\n\tbh=Bf3BzyXMick/tEJjWYHLk4DjQFwu95U4SWOqzDYdX1Y=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=mBrO4fc3myJylWVfUxqlxXpDsZ2n+Hy/x6I7RnjX9r5HQ6xYmxXQnjAgF/pnjj9i/\n\tNOvn1XU8n0NvAz5vFlT67dOUcADu05GQjgXfxBNMwxz3UvwBPILJyaNEjood1I72pA\n\tZGXdokTKYyx/K0Fg6ZL3X0wAOYmXThXgxNeusOgY=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210519101954.77711-1-umang.jain@ideasonboard.com>\n\t<20210519101954.77711-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>\n\t\n\t<20210520031415.GH902042@pyrite.rasen.tech>\n\t<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>\n\t\n\t<6c20de85-dd98-5fe9-5bb0-8ed9cc50959c@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Fri, 21 May 2021 17:03:27 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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":17147,"web_url":"https://patchwork.libcamera.org/comment/17147/","msgid":"","date":"2021-05-23T22:19:05","subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, May 21, 2021 at 05:03:27PM +0530, Umang Jain wrote:\n> On 5/21/21 2:57 PM, Laurent Pinchart wrote:\n> > On Fri, May 21, 2021 at 10:19:59AM +0100, Kieran Bingham wrote:\n> >> On 21/05/2021 10:15, Laurent Pinchart wrote:\n> >>> On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:\n> >>>> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:\n> >>>>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:\n> >>>>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:\n> >>>>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:\n> >>>>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:\n> >>>>>>>>> On 19/05/2021 11:19, Umang Jain wrote:\n> >>>>>>>>>> Moving the core.mojom documentation to its corresponding .cpp file\n> >>>>>>>>>> (core_ipa_interface.cpp). This will allow Doxygen to generate the\n> >>>>>>>>>> documentation for IPABuffer, IPASettings and IPAStream structures.\n> >>>>>>>>>> Since the .mojom files are placed in include/ directory, the .cpp file\n> >>>>>>>>>> will live in $sourcedir/src/libcamera/ipa/ - which can also contain\n> >>>>>>>>>> documentation for other mojom generated IPA interfaces in subsequent\n> >>>>>>>>>> commit.\n> >>>>>>>>>>\n> >>>>>>>>>> Also, fix the Doxygen warning for the above IPA structs regarding their\n> >>>>>>>>>> constructors not being documented.\n> >>>>>>>>>>\n> >>>>>>>>>> Signed-off-by: Umang Jain \n> >>>>>>>>>> ---\n> >>>>>>>>>> Documentation/Doxyfile.in | 8 +-\n> >>>>>>>>>> Documentation/meson.build | 1 +\n> >>>>>>>>>> include/libcamera/ipa/core.mojom | 72 ----------------\n> >>>>>>>>>> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> >>>>>>>>>> src/libcamera/ipa/meson.build | 5 ++\n> >>>>>>>>>> src/libcamera/meson.build | 1 +\n> >>>>>>>>>> 6 files changed, 118 insertions(+), 74 deletions(-)\n> >>>>>>>>>> create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>>>>>> create mode 100644 src/libcamera/ipa/meson.build\n> >>>>>>>>>>\n> >>>>>>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> >>>>>>>>>> index af006724..f4d578fa 100644\n> >>>>>>>>>> --- a/Documentation/Doxyfile.in\n> >>>>>>>>>> +++ b/Documentation/Doxyfile.in\n> >>>>>>>>>> @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> >>>>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> >>>>>>>>>> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> >>>>>>>>>> \t\t\t @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> >>>>>>>>>> -\t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> >>>>>>>>>> \t\t\t @TOP_BUILDDIR@/src/libcamera/proxy/\n> >>>>>>>>>> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> >>>>>>>>>> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO\n> >>>>>>>>>> # Note that the wildcards are matched against the file with absolute path, so to\n> >>>>>>>>>> # exclude all test directories for example use the pattern */test/*\n> >>>>>>>>>> -EXCLUDE_PATTERNS =\n> >>>>>>>>>> +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> >>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \\\n> >>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> >>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> >>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> >>>>>>>>>> + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> >>>>>>>>>\n> >>>>>>>>> I was going to suggest that perhaps this should be\n> >>>>>>>>> \t@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?\n> >>>>>>>>>\n> >>>>>>>>> But that would exclude the core_ipa_interface.h too, which I presume we\n> >>>>>>>>> need to include for Doxygen to map the documentation to.\n> >>>>>>>>>\n> >>>>>>>>> I wonder if there's a way to include only the files we need under\n> >>>>>>>>> include/libcamera/ipa if that list is smaller, rather than trying to\n> >>>>>>>>> exclude lots of combinations which would need to be updated with every\n> >>>>>>>>> new platform supported.\n> >>>>>>>>\n> >>>>>>>> I'd rather go for a method that would scale indeed :-) One option is to\n> >>>>>>>> modify the naming scheme of generated files to be able to match them by\n> >>>>>>>> pattern more easily.\n> >>>>>>>\n> >>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented\n> >>>>>>> anyway, so we could add their documentation (in\n> >>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable\n> >>>>>>> it in doxygen and meson.\n> >>>>>>\n> >>>>>> Yes, documenting all those IPA modules first might be ideal but not sure if\n> >>>>>> may happen all at once. The way I saw, how would this work would be\n> >>>>>> addressed is, document one of the modules & exclude it from the\n> >>>>>> EXCLUDE_PATTERNS for it's generation.\n> >>>>>>\n> >>>>>>> Then the exclude pattern would just be the first two lines.\n> >>>>>>\n> >>>>>> When all modules are documented we will have this state of exclude pattern\n> >>>>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should\n> >>>>>> only merge them, if they are documented as per the [2/7] precedence.\n> >>>>>>\n> >>>>>> Also want to mention that we can file it as a task and thing it's a good\n> >>>>>> first-comers task to work on.\n> >>>>>\n> >>>>> Ah, I see, remove them from the exclude patterns as they're added. Yeah\n> >>>>> that would be a good first-comer task too.\n> >>>>>\n> >>>>> In that case I think a todo comment (either here or in each mojom file?\n> >>>>> in the mojo file it won't be picked up by doxygen though... what about\n> >>>>> in dummy interface cpp files?) would be useful.\n> >>>>\n> >>>> I'm curious - do we have faults if we don't exclude these now?\n> >>>> What is needed to be able to simply remove these platform specifics as\n> >>>> they are?\n> >>>>\n> >>>> Presumably it would introduce some doxygen warnings?\n> >>>>\n> >>>> If that's what would happen - I would actually prefer to remove the\n> >>>> lines - and have the doxygen warnigns printed - providing clear\n> >>>> motivation to get them fixed as soon as possible (rather even than a todo)\n> >>>>\n> >>>> Doxygen warnings aren't fatal - but they are annoying, and that annoying\n> >>>> would help drive getting it fixed.\n> >>>\n> >>> We currently build the documentation without warnings, so I'd consider\n> >>> any warning as a regression that would need to be fixed with the highest\n> >>> priority, as any other build breakage.\n> >>\n> >> That's sort of my point - the question is ... can we knowingly introduce\n> >> those warnings, or should we paper over them by excluding the files\n> >> until the documentation is moved over as is done in this patch.\n> >\n> > Same answer as for knowingly introducing any other build breakage :-) If\n> > it has to be fixed with the highest priority, it can as well be done as\n> > part of the series.\n>\n> It's not highest priority for this series at-least, given that we were \n> just testing waters on how to keep up on documentation for .mojom files. \n> I know it's a kind of hack but it's a good stopping point to not get \n> ahead of yourselves. I, would really like if we were able to pass in \n> mojom files directly to doxygen (even if going through a intermediatary \n> step, like extracting comments out first) - but it would be a quite a \n> work, I think, to get the plumbing in place. I suspected, that \n> documenting all IPAs interfaces will also be take some - hence I have \n> left it at a point where it can be done in phases/per-IPA basis, if \n> decided to carry on with this precedence of documenting mojom.\n\nI'm fine with that if we exclude those files for now. I don't want to\ncarry build warning regressions for any extensive period of time.\n\n> Also, Maybe we need a \"doc\" sprint week :P\n>\n> >> If we remove the lines:\n> >> @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \\\n> >> @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \\\n> >> @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \\\n> >> @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \\\n> >>\n> >> from above, I think what will happen is some warnings will be\n> >> introduced, which will mean we must then handle the documentation for\n> >> RPi/Vimc/RKISP/IPU3 as a priority.\n> >>\n> >> Either way, as long as it all gets done is what matters.\n> >>\n> >>>> Anyway, With a solution chosen..., for getting this into the .cpp file\n> >>>> now to progress this series...\n> >>>>\n> >>>> Acked-by: Kieran Bingham \n> >>>>\n> >>>>>>>>>> # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names\n> >>>>>>>>>> # (namespaces, classes, functions, etc.) that should be excluded from the\n> >>>>>>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> >>>>>>>>>> index c8521574..9ecf4dfc 100644\n> >>>>>>>>>> --- a/Documentation/meson.build\n> >>>>>>>>>> +++ b/Documentation/meson.build\n> >>>>>>>>>> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()\n> >>>>>>>>>> doxyfile,\n> >>>>>>>>>> libcamera_internal_headers,\n> >>>>>>>>>> libcamera_ipa_headers,\n> >>>>>>>>>> + libcamera_ipa_interfaces,\n> >>>>>>>>>> libcamera_public_headers,\n> >>>>>>>>>> libcamera_sources,\n> >>>>>>>>>> libipa_headers,\n> >>>>>>>>>> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> >>>>>>>>>> index 6caaa63e..e49815d8 100644\n> >>>>>>>>>> --- a/include/libcamera/ipa/core.mojom\n> >>>>>>>>>> +++ b/include/libcamera/ipa/core.mojom\n> >>>>>>>>>> @@ -94,88 +94,16 @@ module libcamera;\n> >>>>>>>>>> \tuint32 maxFrameLength;\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> >>>>>>>>>> - * \\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> >>>>>>>>>> -\n> >>>>>>>>>> - /**\n> >>>>>>>>>> - * \\var IPASettings::sensorModel\n> >>>>>>>>>> - * \\brief The sensor model name\n> >>>>>>>>>> - *\n> >>>>>>>>>> - * Provides the sensor model name to the IPA.\n> >>>>>>>>>> - */\n> >>>>>>>>>> struct IPASettings {\n> >>>>>>>>>> \tstring configurationFile;\n> >>>>>>>>>> \tstring sensorModel;\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> >>>>>>>>>> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>>>>>> new file mode 100644\n> >>>>>>>>>> index 00000000..fe1ecce6\n> >>>>>>>>>> --- /dev/null\n> >>>>>>>>>> +++ b/src/libcamera/ipa/core_ipa_interface.cpp\n> >>>>>>>>>> @@ -0,0 +1,105 @@\n> >>>>>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>>>>>>>> +/*\n> >>>>>>>>>> + * Copyright (C) 2021, Google Inc.\n> >>>>>>>>>> + *\n> >>>>>>>>>> + * core_ipa_interface.cpp - Docs file for core.mojom generated header\n> >>>>>>>>>> + */\n> >>>>>>>>>> +\n> >>>>>>>>>> +namespace libcamera {\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> >>>>>>>>>> + * \\fn IPABuffer::IPABuffer(uint32_t id, const std::vector &planes)\n> >>>>>>>>>> + * \\param[in] id\n> >>>>>>>>>> + * \\param[in] planes\n> >>>>>>>>>> + * \\sa id and planes\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> >>>>>>>>>> +/**\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> >>>>>>>>>> + * \\fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)\n> >>>>>>>>>> + * \\param[in] configurationFile\n> >>>>>>>>>> + * \\param[in] sensorModel\n> >>>>>>>>>> + * \\sa configurationFile and sensorModel\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> >>>>>>>>>> +\n> >>>>>>>>>> +/**\n> >>>>>>>>>> + * \\var IPASettings::sensorModel\n> >>>>>>>>>> + * \\brief The sensor model name\n> >>>>>>>>>> + *\n> >>>>>>>>>> + * Provides the sensor model name to the IPA.\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> >>>>>>>>>> + * \\fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)\n> >>>>>>>>>> + * \\param[in] pixelFormat\n> >>>>>>>>>> + * \\param[in] size\n> >>>>>>>>>> + * \\sa pixelFormat and size\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> >>>>>>>>>> +\n> >>>>>>>>>> +} /* namespace libcamera */\n> >>>>>>>>>> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build\n> >>>>>>>>>> new file mode 100644\n> >>>>>>>>>> index 00000000..0a16d197\n> >>>>>>>>>> --- /dev/null\n> >>>>>>>>>> +++ b/src/libcamera/ipa/meson.build\n> >>>>>>>>>> @@ -0,0 +1,5 @@\n> >>>>>>>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> >>>>>>>>>> +\n> >>>>>>>>>> +libcamera_ipa_interfaces = files([\n> >>>>>>>>>> + 'core_ipa_interface.cpp',\n> >>>>>>>>>> +])\n> >>>>>>>>> I'm sure someone will disagree with me, but I almost feel like this .cpp\n> >>>>>>>>> file should get run through a compiler, and therefore just added to the\n> >>>>>>>>> libcamera sources as normal (which I think would then already get picked\n> >>>>>>>>> up by doxygen).\n> >>>>>>>>>\n> >>>>>>>>> That way the syntax of the file is maintained too, given that it is a\n> >>>>>>>>> C++ file.\n> >>>>>>>> What's the point if it only contains comments though ? Ideally we\n> >>>>>>>> shouldn't have this .cpp file in the first place, it should be generated\n> >>>>>>>> automatically, or the documentation comments should be added to the\n> >>>>>>>> genearated headers by mojo. We can't do so now so we need this kind of\n> >>>>>>>> hack, but I'd rather keep the scope of the hack as small as possible.\n> >>>>>>>>\n> >>>>>>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>>>>>>>>> index 7bc59b84..61b5fe21 100644\n> >>>>>>>>>> --- a/src/libcamera/meson.build\n> >>>>>>>>>> +++ b/src/libcamera/meson.build\n> >>>>>>>>>> @@ -67,6 +67,7 @@ includes = [\n> >>>>>>>>>> libcamera_includes,\n> >>>>>>>>>> ]\n> >>>>>>>>>> +subdir('ipa')\n> >>>>>>>>>> subdir('pipeline')\n> >>>>>>>>>> subdir('proxy')","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 6C00BC3201\n\tfor ;\n\tSun, 23 May 2021 22:19:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0CB86891E;\n\tMon, 24 May 2021 00:19:10 +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 DDBB76050F\n\tfor ;\n\tMon, 24 May 2021 00:19:09 +0200 (CEST)","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 2F5F6476;\n\tMon, 24 May 2021 00:19:09 +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=\"sn4eidJN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621808349;\n\tbh=xCUPXoxZAtKssRITfWUYgbBGqfOtSbI7dMv12bLoo6c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sn4eidJNoEwoTFcx8PO00QuwWZ3YQDqJP6+V/JuAwkE647B4x+568zRKt3GSq4b/N\n\tMltlbNnWXyo0ANStRt94WctnZj8/CoR/ZQ4zjZcQpJw5hUlE/mg+eQQWMrc9qefEjw\n\thvcCnS/8EbUX9qluGEZlO6Xc/+8Lxb1S+iD048n0=","Date":"Mon, 24 May 2021 01:19:05 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"\n\t\n\t<20210520022810.GC902042@pyrite.rasen.tech>\n\t\n\t<20210520031415.GH902042@pyrite.rasen.tech>\n\t<46a61c79-5a8b-7b85-38fb-144da58914cd@ideasonboard.com>\n\t\n\t<6c20de85-dd98-5fe9-5bb0-8ed9cc50959c@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface\n\tdocumentation to a .cpp file","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\" "}}]