[{"id":17148,"web_url":"https://patchwork.libcamera.org/comment/17148/","msgid":"","date":"2021-05-24T00:43:29","subject":"Re: [libcamera-devel] [PATCH v3 1/6] 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\nThank you for the patch.\n\nOn Fri, May 21, 2021 at 06:58:18PM +0530, 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\nI wonder if we should store those files in src/libcamera/pipeline/. We\ncan think about this later.\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> Acked-by: Kieran Bingham \n> ---\n> Documentation/Doxyfile.in | 8 +-\n> Documentation/meson.build | 1 +\n> include/libcamera/ipa/core.mojom | 72 ----------------\n> include/libcamera/ipa/ipu3.mojom | 5 ++\n> include/libcamera/ipa/raspberrypi.mojom | 5 ++\n> include/libcamera/ipa/rkisp1.mojom | 5 ++\n> include/libcamera/ipa/vimc.mojom | 5 ++\n> src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++\n> src/libcamera/ipa/meson.build | 5 ++\n> src/libcamera/meson.build | 1 +\n> 10 files changed, 138 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\nAlphabetically sorted please :-)\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/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index a717b1e6..9e3cd885 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -1,5 +1,10 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> \n> +/*\n> + * \\todo Document the interface as src/libcamera/ipa/ipu3_ipa_interface.cpp\n> + * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.\n> + */\n> +\n> module ipa.ipu3;\n> \n> import \"include/libcamera/ipa/core.mojom\";\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 42321bee..770e3049 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -1,5 +1,10 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> \n> +/*\n> + * \\todo Document the interface as src/libcamera/ipa/raspberrypi_ipa_interface.cpp\n> + * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.\n> + */\n> +\n> module ipa.RPi;\n> \n> import \"include/libcamera/ipa/core.mojom\";\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index cca871a0..da6646df 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -1,5 +1,10 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> \n> +/*\n> + * \\todo Document the interface as src/libcamera/ipa/rkisp1_ipa_interface.cpp\n> + * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.\n> + */\n> +\n> module ipa.rkisp1;\n> \n> import \"include/libcamera/ipa/core.mojom\";\n> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom\n> index be4b85b8..9ffd93bb 100644\n> --- a/include/libcamera/ipa/vimc.mojom\n> +++ b/include/libcamera/ipa/vimc.mojom\n> @@ -1,5 +1,10 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> \n> +/*\n> + * \\todo Document the interface as src/libcamera/ipa/vimc_ipa_interface.cpp\n> + * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.\n> + */\n> +\n> module ipa.vimc;\n> \n> import \"include/libcamera/ipa/core.mojom\";\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\nYou're missing a \\file block here, Doxygen will not link the header in\nfiles.html without that. \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\nThe license for meson.build files should be CC0-1.0.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +libcamera_ipa_interfaces = files([\n> + 'core_ipa_interface.cpp',\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 5A05DC3200\n\tfor ;\n\tMon, 24 May 2021 00:43:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A87146891B;\n\tMon, 24 May 2021 02:43:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 719BD6050F\n\tfor ;\n\tMon, 24 May 2021 02:43:33 +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 E3B28476;\n\tMon, 24 May 2021 02:43:32 +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=\"fvV2KQxo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621817013;\n\tbh=SCiFMszV9wLBjarX/OzH78Z2sN5jWWMckUYJvRiKRQc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fvV2KQxoBQ8PdaUPRA5roaTCqc+kSqyZfIVJRFV+Oe0fRTTnexRQzeg/4BB46WOX8\n\t+7JdbtVnMjB32Zp5Lv3B5u94oyaI6YvR/4oZpiNbP7NcCqinz2NxnapyCdeOH9hbEN\n\tng/XqjY8Wtixm/KvGuP+I+FZiV2r2+1yF5cMXJHQ=","Date":"Mon, 24 May 2021 03:43:29 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210521132823.322076-1-umang.jain@ideasonboard.com>\n\t<20210521132823.322076-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210521132823.322076-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/6] 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\" "}}]