[{"id":17204,"web_url":"https://patchwork.libcamera.org/comment/17204/","msgid":"","date":"2021-05-24T13:19:51","subject":"Re: [libcamera-devel] [RFC PATCH 4/6] ipa: core: Move documentation\n\tfrom cpp file back into the mojom file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, May 24, 2021 at 05:40:27PM +0900, Paul Elder wrote:\n> Move the documentation back to the mojom file from the cpp file. Also\n> remove the constructor documentation as it is not needed anymore. While\n> at it, move the documentation for IPAInterface::init() and\n> IPAInterface::stop() to the IPA guide.\n> \n> Signed-off-by: Paul Elder \n\nAfter a rebase, and a corresponding update of the commit message,\n\nReviewed-by: Laurent Pinchart \n\n> ---\n> Documentation/guides/ipa.rst | 6 +\n> include/libcamera/ipa/core.mojom | 200 +++++++++++++++++--\n> src/libcamera/ipa/core_ipa_interface.cpp | 237 -----------------------\n> 3 files changed, 190 insertions(+), 253 deletions(-)\n> delete mode 100644 src/libcamera/ipa/core_ipa_interface.cpp\n> \n> diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst\n> index 6d460e13..9213c220 100644\n> --- a/Documentation/guides/ipa.rst\n> +++ b/Documentation/guides/ipa.rst\n> @@ -166,6 +166,12 @@ At a minimum, the following three functions must be present (and implemented):\n> All three of these functions are synchronous. The parameters for start() and\n> init() may be customized.\n> \n> +init() initializes the IPA interface. It shall be called before any other\n> +function of the IPAInterface.\n> +\n> +stop() informs the IPA module that the camera is stopped. The IPA module shall\n> +release resources prepared in start().\n> +\n> A configure() method is recommended. Any ControlInfoMap instances that will be\n> used by the IPA must be sent to the IPA from the pipeline handler, at configure\n> time, for example.\n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index 34a799c2..b32f3093 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -2,6 +2,11 @@\n> \n> module libcamera;\n> \n> +/**\n> + * \\file core_ipa_interface.h\n> + * \\brief libcamera structs for IPAs\n> + */\n> +\n> /*\n> * Things that can be defined here (and in other mojom files):\n> * - consts\n> @@ -78,6 +83,116 @@ module libcamera;\n> \tuint32 height;\n> };\n> \n> +/**\n> + * \\struct IPACameraSensorInfo\n> + * \\brief Report the image sensor characteristics\n> + *\n> + * The structure reports image sensor characteristics used by IPA modules to\n> + * tune their algorithms based on the image sensor model currently in use and\n> + * its configuration.\n> + *\n> + * The reported information describes the sensor's intrinsics characteristics,\n> + * such as its pixel array size and the sensor model name, as well as\n> + * information relative to the currently configured mode, such as the produced\n> + * image size and the bit depth of the requested image format.\n> + *\n> + * Instances of this structure are meant to be assembled by the CameraSensor\n> + * class by inspecting the sensor static properties as well as information\n> + * relative to the current configuration.\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::model\n> + * \\brief The image sensor model name\n> + *\n> + * The sensor model name is a free-formed string that uniquely identifies the\n> + * sensor model.\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::bitsPerPixel\n> + * \\brief The number of bits per pixel of the image format produced by the\n> + * image sensor\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::activeAreaSize\n> + * \\brief The size of the pixel array active area of the sensor\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::analogCrop\n> + * \\brief The portion of the pixel array active area which is read-out and\n> + * processed\n> + *\n> + * The analog crop rectangle top-left corner is defined as the displacement\n> + * from the top-left corner of the pixel array active area. The rectangle\n> + * horizontal and vertical sizes define the portion of the pixel array which\n> + * is read-out and provided to the sensor's internal processing pipeline, before\n> + * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> + * take place.\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::outputSize\n> + * \\brief The size of the images produced by the camera sensor\n> + *\n> + * The output image size defines the horizontal and vertical sizes of the images\n> + * produced by the image sensor. The output image size is defined as the end\n> + * result of the sensor's internal image processing pipeline stages, applied on\n> + * the pixel array portion defined by the analog crop rectangle. Each image\n> + * processing stage that performs pixel sub-sampling techniques, such as pixel\n> + * binning or skipping, or perform any additional digital scaling concur in the\n> + * definition of the output image size.\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::pixelRate\n> + * \\brief The number of pixels produced in a second\n> + *\n> + * To obtain the read-out time in seconds of a full line:\n> + *\n> + * \\verbatim\n> + lineDuration(s) = lineLength(pixels) / pixelRate(pixels per second)\n> + \\endverbatim\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::lineLength\n> + * \\brief Total line length in pixels\n> + *\n> + * The total line length in pixel clock periods, including blanking.\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::minFrameLength\n> + * \\brief The minimum allowable frame length in units of lines\n> + *\n> + * The sensor frame length comprises of active output lines and blanking lines\n> + * in a frame. The minimum frame length value dictates the minimum allowable\n> + * frame duration of the sensor mode.\n> + *\n> + * To obtain the minimum frame duration:\n> + *\n> + * \\verbatim\n> + frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> + \\endverbatim\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::maxFrameLength\n> + * \\brief The maximum allowable frame length in units of lines\n> + *\n> + * The sensor frame length comprises of active output lines and blanking lines\n> + * in a frame. The maximum frame length value dictates the maximum allowable\n> + * frame duration of the sensor mode.\n> + *\n> + * To obtain the maximum frame duration:\n> + *\n> + * \\verbatim\n> + frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> + \\endverbatim\n> + */\n> struct IPACameraSensorInfo {\n> \tstring model;\n> \n> @@ -94,35 +209,88 @@ struct IPACameraSensorInfo {\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> 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> -struct IPAStream {\n> -\tuint32 pixelFormat;\n> -\tSize size;\n> -};\n> -\n> /**\n> - * \\fn IPAInterface::init()\n> - * \\brief Initialise the IPAInterface\n> - * \\param[in] settings The IPA initialization settings\n> + * \\struct IPAStream\n> + * \\brief Stream configuration for the IPA interface\n> *\n> - * This function initializes the IPA interface. It shall be called before any\n> - * other function of the IPAInterface. The \\a settings carry initialization\n> - * parameters that are valid for the whole life time of the IPA interface.\n> + * 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 IPAInterface::stop()\n> - * \\brief Stop the IPA\n> - *\n> - * This method informs the IPA module that the camera is stopped. The IPA module\n> - * shall release resources prepared in start().\n> + * \\var IPAStream::pixelFormat\n> + * \\brief The stream pixel format\n> + */\n> +\n> +/**\n> + * \\var IPAStream::size\n> + * \\brief The stream size in pixels\n> */\n> +struct IPAStream {\n> +\tuint32 pixelFormat;\n> +\tSize size;\n> +};\n> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp\n> deleted file mode 100644\n> index 47c8e93d..00000000\n> --- a/src/libcamera/ipa/core_ipa_interface.cpp\n> +++ /dev/null\n> @@ -1,237 +0,0 @@\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> -/**\n> - * \\struct IPACameraSensorInfo\n> - * \\brief Report the image sensor characteristics\n> - *\n> - * The structure reports image sensor characteristics used by IPA modules to\n> - * tune their algorithms based on the image sensor model currently in use and\n> - * its configuration.\n> - *\n> - * The reported information describes the sensor's intrinsics characteristics,\n> - * such as its pixel array size and the sensor model name, as well as\n> - * information relative to the currently configured mode, such as the produced\n> - * image size and the bit depth of the requested image format.\n> - *\n> - * Instances of this structure are meant to be assembled by the CameraSensor\n> - * class by inspecting the sensor static properties as well as information\n> - * relative to the current configuration.\n> - */\n> -\n> -/**\n> - * \\fn IPACameraSensorInfo::IPACameraSensorInfo(const std::string &model,\n> - * uint32_t bitsPerPixel,\n> - * const Size &activeAreaSize,\n> - * const Rectangle &analogCrop,\n> - * const Size &outputSize,\n> - * uint64_t pixelRate,\n> - * uint32_t lineLength,\n> - * uint32_t minFrameLength,\n> - * uint32_t maxFrameLength)\n> - *\n> - * \\param[in] model\n> - * \\param[in] bitsPerPixel\n> - * \\param[in] activeAreaSize\n> - * \\param[in] analogCrop\n> - * \\param[in] outputSize\n> - * \\param[in] pixelRate\n> - * \\param[in] lineLength\n> - * \\param[in] minFrameLength\n> - * \\param[in] maxFrameLength\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::model\n> - * \\brief The image sensor model name\n> - *\n> - * The sensor model name is a free-formed string that uniquely identifies the\n> - * sensor model.\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::bitsPerPixel\n> - * \\brief The number of bits per pixel of the image format produced by the\n> - * image sensor\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::activeAreaSize\n> - * \\brief The size of the pixel array active area of the sensor\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::analogCrop\n> - * \\brief The portion of the pixel array active area which is read-out and\n> - * processed\n> - *\n> - * The analog crop rectangle top-left corner is defined as the displacement\n> - * from the top-left corner of the pixel array active area. The rectangle\n> - * horizontal and vertical sizes define the portion of the pixel array which\n> - * is read-out and provided to the sensor's internal processing pipeline, before\n> - * any pixel sub-sampling method, such as pixel binning, skipping and averaging\n> - * take place.\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::outputSize\n> - * \\brief The size of the images produced by the camera sensor\n> - *\n> - * The output image size defines the horizontal and vertical sizes of the images\n> - * produced by the image sensor. The output image size is defined as the end\n> - * result of the sensor's internal image processing pipeline stages, applied on\n> - * the pixel array portion defined by the analog crop rectangle. Each image\n> - * processing stage that performs pixel sub-sampling techniques, such as pixel\n> - * binning or skipping, or perform any additional digital scaling concur in the\n> - * definition of the output image size.\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::pixelRate\n> - * \\brief The number of pixels produced in a second\n> - *\n> - * To obtain the read-out time in seconds of a full line:\n> - *\n> - * \\verbatim\n> - lineDuration(s) = lineLength(pixels) / pixelRate(pixels per second)\n> - \\endverbatim\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::lineLength\n> - * \\brief Total line length in pixels\n> - *\n> - * The total line length in pixel clock periods, including blanking.\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::minFrameLength\n> - * \\brief The minimum allowable frame length in units of lines\n> - *\n> - * The sensor frame length comprises of active output lines and blanking lines\n> - * in a frame. The minimum frame length value dictates the minimum allowable\n> - * frame duration of the sensor mode.\n> - *\n> - * To obtain the minimum frame duration:\n> - *\n> - * \\verbatim\n> - frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> - \\endverbatim\n> - */\n> -\n> -/**\n> - * \\var IPACameraSensorInfo::maxFrameLength\n> - * \\brief The maximum allowable frame length in units of lines\n> - *\n> - * The sensor frame length comprises of active output lines and blanking lines\n> - * in a frame. The maximum frame length value dictates the maximum allowable\n> - * frame duration of the sensor mode.\n> - *\n> - * To obtain the maximum frame duration:\n> - *\n> - * \\verbatim\n> - frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> - \\endverbatim\n> - */\n> -} /* namespace libcamera */","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 A7AB0C3201\n\tfor ;\n\tMon, 24 May 2021 13:19:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 140276891C;\n\tMon, 24 May 2021 15:19:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8035E601AA\n\tfor ;\n\tMon, 24 May 2021 15:19:55 +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 12AF1ED;\n\tMon, 24 May 2021 15:19:55 +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=\"wIGfUpF5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621862395;\n\tbh=9VsjcRFfr1pw0ANPf/+raUTt5gO480osZgRyw+ggkXU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wIGfUpF5xX91oS2bQy9Dv1mT+ZmZdxT2AMYvkJ79xO0JSk7Wq+o45/FHh+q/ienYI\n\tpNRdkEa/7WXCfA5PmrPOpki/wBaCmP6Gv07tXLUmtt5qFFEmz96IkCLGfPjieuarQ+\n\tQxUefxdS90aIMKAXhZfnKYlDoYOxUIm6ioKFyrOE=","Date":"Mon, 24 May 2021 16:19:51 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210524084029.1179881-1-paul.elder@ideasonboard.com>\n\t<20210524084029.1179881-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210524084029.1179881-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 4/6] ipa: core: Move documentation\n\tfrom cpp file back into the mojom 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\" "}}]