[libcamera-devel] [PATCH v3.1] ipa: core: Move documentation from cpp file back into the mojom file

Laurent Pinchart laurent.pinchart at ideasonboard.com
Thu May 27 11:03:51 CEST 2021


Hi Paul,

Thank you for the patch.

On Thu, May 27, 2021 at 05:04:18PM +0900, Paul Elder wrote:
> Move the documentation back to the mojom file from the cpp file. While
> at it, move the documentation for IPAInterface::init() and
> IPAInterface::stop() to the IPA guide.
> 
> While at it, update the todo comment in all of the mojom files
> accordingly.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> [Update todos] Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> [Update todos] Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

Let's keep tags starting on column 1, otherwise it will be difficult to
parse them should we ever need to.

The kernel style would be

[umang.jain at ideasonboard.com: Update todos]
Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

But I'd simply write

Update todos

Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> ---
> Changes in v3.1:
> - squash Umang's todo update
> 
> Changes in v3:
> - remove core_ipa_interface.cpp
> ---
>  Documentation/guides/ipa.rst             |   6 +
>  include/libcamera/ipa/core.mojom         | 200 +++++++++++++++++++++--
>  include/libcamera/ipa/ipu3.mojom         |   3 +-
>  include/libcamera/ipa/raspberrypi.mojom  |   3 +-
>  include/libcamera/ipa/rkisp1.mojom       |   3 +-
>  include/libcamera/ipa/vimc.mojom         |   3 +-
>  src/libcamera/ipa/core_ipa_interface.cpp | 199 ----------------------
>  7 files changed, 194 insertions(+), 223 deletions(-)
>  delete mode 100644 src/libcamera/ipa/core_ipa_interface.cpp
> 
> diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst
> index 6195b2b7..c612470f 100644
> --- a/Documentation/guides/ipa.rst
> +++ b/Documentation/guides/ipa.rst
> @@ -166,6 +166,12 @@ At a minimum, the following three functions must be present (and implemented):
>  All three of these functions are synchronous. The parameters for start() and
>  init() may be customized.
>  
> +init() initializes the IPA interface. It shall be called before any other
> +function of the IPAInterface.
> +
> +stop() informs the IPA module that the camera is stopped. The IPA module shall
> +release resources prepared in start().
> +
>  A configure() method is recommended. Any ControlInfoMap instances that will be
>  used by the IPA must be sent to the IPA from the pipeline handler, at configure
>  time, for example.
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> index 34a799c2..b32f3093 100644
> --- a/include/libcamera/ipa/core.mojom
> +++ b/include/libcamera/ipa/core.mojom
> @@ -2,6 +2,11 @@
>  
>  module libcamera;
>  
> +/**
> + * \file core_ipa_interface.h
> + * \brief libcamera structs for IPAs
> + */
> +
>  /*
>   * Things that can be defined here (and in other mojom files):
>   * - consts
> @@ -78,6 +83,116 @@ module libcamera;
>  	uint32 height;
>  };
>  
> +/**
> + * \struct IPACameraSensorInfo
> + * \brief Report the image sensor characteristics
> + *
> + * The structure reports image sensor characteristics used by IPA modules to
> + * tune their algorithms based on the image sensor model currently in use and
> + * its configuration.
> + *
> + * The reported information describes the sensor's intrinsics characteristics,
> + * such as its pixel array size and the sensor model name, as well as
> + * information relative to the currently configured mode, such as the produced
> + * image size and the bit depth of the requested image format.
> + *
> + * Instances of this structure are meant to be assembled by the CameraSensor
> + * class by inspecting the sensor static properties as well as information
> + * relative to the current configuration.
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::model
> + * \brief The image sensor model name
> + *
> + * The sensor model name is a free-formed string that uniquely identifies the
> + * sensor model.
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::bitsPerPixel
> + * \brief The number of bits per pixel of the image format produced by the
> + * image sensor
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::activeAreaSize
> + * \brief The size of the pixel array active area of the sensor
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::analogCrop
> + * \brief The portion of the pixel array active area which is read-out and
> + * processed
> + *
> + * The analog crop rectangle top-left corner is defined as the displacement
> + * from the top-left corner of the pixel array active area. The rectangle
> + * horizontal and vertical sizes define the portion of the pixel array which
> + * is read-out and provided to the sensor's internal processing pipeline, before
> + * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> + * take place.
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::outputSize
> + * \brief The size of the images produced by the camera sensor
> + *
> + * The output image size defines the horizontal and vertical sizes of the images
> + * produced by the image sensor. The output image size is defined as the end
> + * result of the sensor's internal image processing pipeline stages, applied on
> + * the pixel array portion defined by the analog crop rectangle. Each image
> + * processing stage that performs pixel sub-sampling techniques, such as pixel
> + * binning or skipping, or perform any additional digital scaling concur in the
> + * definition of the output image size.
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::pixelRate
> + * \brief The number of pixels produced in a second
> + *
> + * To obtain the read-out time in seconds of a full line:
> + *
> + * \verbatim
> +       lineDuration(s) = lineLength(pixels) / pixelRate(pixels per second)
> +   \endverbatim
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::lineLength
> + * \brief Total line length in pixels
> + *
> + * The total line length in pixel clock periods, including blanking.
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::minFrameLength
> + * \brief The minimum allowable frame length in units of lines
> + *
> + * The sensor frame length comprises of active output lines and blanking lines
> + * in a frame. The minimum frame length value dictates the minimum allowable
> + * frame duration of the sensor mode.
> + *
> + * To obtain the minimum frame duration:
> + *
> + * \verbatim
> +       frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> +   \endverbatim
> + */
> +
> +/**
> + * \var IPACameraSensorInfo::maxFrameLength
> + * \brief The maximum allowable frame length in units of lines
> + *
> + * The sensor frame length comprises of active output lines and blanking lines
> + * in a frame. The maximum frame length value dictates the maximum allowable
> + * frame duration of the sensor mode.
> + *
> + * To obtain the maximum frame duration:
> + *
> + * \verbatim
> +       frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> +   \endverbatim
> + */
>  struct IPACameraSensorInfo {
>  	string model;
>  
> @@ -94,35 +209,88 @@ struct IPACameraSensorInfo {
>  	uint32 maxFrameLength;
>  };
>  
> +/**
> + * \struct IPABuffer
> + * \brief Buffer information for the IPA interface
> + *
> + * The IPABuffer structure associates buffer memory with a unique ID. It is
> + * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which
> + * buffers will be identified by their ID in the IPA interface.
> + */
> +
> +/**
> + * \var IPABuffer::id
> + * \brief The buffer unique ID
> + *
> + * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs
> + * are chosen by the pipeline handler to fulfil the following constraints:
> + *
> + * - IDs shall be positive integers different than zero
> + * - IDs shall be unique among all mapped buffers
> + *
> + * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are
> + * freed and may be reused for new buffer mappings.
> + */
> +
> +/**
> + * \var IPABuffer::planes
> + * \brief The buffer planes description
> + *
> + * Stores the dmabuf handle and length for each plane of the buffer.
> + */
>  struct IPABuffer {
>  	uint32 id;
>  	[hasFd] array<FrameBuffer.Plane> planes;
>  };
>  
> +/**
> + * \struct IPASettings
> + * \brief IPA interface initialization settings
> + *
> + * The IPASettings structure stores data passed to the IPAInterface::init()
> + * function. The data contains settings that don't depend on a particular camera
> + * or pipeline configuration and are valid for the whole life time of the IPA
> + * interface.
> + */
> +
> +/**
> + * \var IPASettings::configurationFile
> + * \brief The name of the IPA configuration file
> + *
> + * This field may be an empty string if the IPA doesn't require a configuration
> + * file.
> + */
> +
> +/**
> + * \var IPASettings::sensorModel
> + * \brief The sensor model name
> + *
> + * Provides the sensor model name to the IPA.
> + */
>  struct IPASettings {
>  	string configurationFile;
>  	string sensorModel;
>  };
>  
> -struct IPAStream {
> -	uint32 pixelFormat;
> -	Size size;
> -};
> -
>  /**
> - * \fn IPAInterface::init()
> - * \brief Initialise the IPAInterface
> - * \param[in] settings The IPA initialization settings
> + * \struct IPAStream
> + * \brief Stream configuration for the IPA interface
>   *
> - * This function initializes the IPA interface. It shall be called before any
> - * other function of the IPAInterface. The \a settings carry initialization
> - * parameters that are valid for the whole life time of the IPA interface.
> + * The IPAStream structure stores stream configuration parameters needed by the
> + * IPAInterface::configure() method. It mirrors the StreamConfiguration class
> + * that is not suitable for this purpose due to not being serializable.
>   */
>  
>  /**
> - * \fn IPAInterface::stop()
> - * \brief Stop the IPA
> - *
> - * This method informs the IPA module that the camera is stopped. The IPA module
> - * shall release resources prepared in start().
> + * \var IPAStream::pixelFormat
> + * \brief The stream pixel format
> + */
> +
> +/**
> + * \var IPAStream::size
> + * \brief The stream size in pixels
>   */
> +struct IPAStream {
> +	uint32 pixelFormat;
> +	Size size;
> +};
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 29b4c805..000c494d 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -1,8 +1,7 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  
>  /*
> - * \todo Document the interface as src/libcamera/ipa/ipu3_ipa_interface.cpp
> - * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.
> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
>   */
>  
>  module ipa.ipu3;
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index db1f689f..606d1de4 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -1,8 +1,7 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  
>  /*
> - * \todo Document the interface as src/libcamera/ipa/raspberrypi_ipa_interface.cpp
> - * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.
> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
>   */
>  
>  module ipa.RPi;
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 55fa43be..cae757ea 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -1,8 +1,7 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  
>  /*
> - * \todo Document the interface as src/libcamera/ipa/rkisp1_ipa_interface.cpp
> - * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.
> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
>   */
>  
>  module ipa.rkisp1;
> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> index 9ffd93bb..86bc318a 100644
> --- a/include/libcamera/ipa/vimc.mojom
> +++ b/include/libcamera/ipa/vimc.mojom
> @@ -1,8 +1,7 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  
>  /*
> - * \todo Document the interface as src/libcamera/ipa/vimc_ipa_interface.cpp
> - * and remove the EXCLUDE_PATTERNS entry in Doxygen.in for its generation.
> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
>   */
>  
>  module ipa.vimc;
> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp
> deleted file mode 100644
> index 6ab689cd..00000000
> --- a/src/libcamera/ipa/core_ipa_interface.cpp
> +++ /dev/null
> @@ -1,199 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2021, Google Inc.
> - *
> - * core_ipa_interface.cpp - Docs file for core.mojom generated header
> - */
> -
> -namespace libcamera {
> -
> -/**
> - * \file core_ipa_interface.h
> - * \brief Core IPA inteface
> - */
> -
> -/**
> - * \struct IPABuffer
> - * \brief Buffer information for the IPA interface
> - *
> - * The IPABuffer structure associates buffer memory with a unique ID. It is
> - * used to map buffers to the IPA with IPAInterface::mapBuffers(), after which
> - * buffers will be identified by their ID in the IPA interface.
> - */
> -
> -/**
> - * \var IPABuffer::id
> - * \brief The buffer unique ID
> - *
> - * Buffers mapped to the IPA are identified by numerical unique IDs. The IDs
> - * are chosen by the pipeline handler to fulfil the following constraints:
> - *
> - * - IDs shall be positive integers different than zero
> - * - IDs shall be unique among all mapped buffers
> - *
> - * When buffers are unmapped with IPAInterface::unmapBuffers() their IDs are
> - * freed and may be reused for new buffer mappings.
> - */
> -
> -/**
> - * \var IPABuffer::planes
> - * \brief The buffer planes description
> - *
> - * Stores the dmabuf handle and length for each plane of the buffer.
> - */
> -
> -/**
> - * \struct IPASettings
> - * \brief IPA interface initialization settings
> - *
> - * The IPASettings structure stores data passed to the IPAInterface::init()
> - * function. The data contains settings that don't depend on a particular camera
> - * or pipeline configuration and are valid for the whole life time of the IPA
> - * interface.
> - */
> -
> -/**
> - * \var IPASettings::configurationFile
> - * \brief The name of the IPA configuration file
> - *
> - * This field may be an empty string if the IPA doesn't require a configuration
> - * file.
> - */
> -
> -/**
> - * \var IPASettings::sensorModel
> - * \brief The sensor model name
> - *
> - * Provides the sensor model name to the IPA.
> - */
> -
> -/**
> - * \struct IPAStream
> - * \brief Stream configuration for the IPA interface
> - *
> - * The IPAStream structure stores stream configuration parameters needed by the
> - * IPAInterface::configure() method. It mirrors the StreamConfiguration class
> - * that is not suitable for this purpose due to not being serializable.
> - */
> -
> -/**
> - * \var IPAStream::pixelFormat
> - * \brief The stream pixel format
> - */
> -
> -/**
> - * \var IPAStream::size
> - * \brief The stream size in pixels
> - */
> -
> -/**
> - * \struct IPACameraSensorInfo
> - * \brief Report the image sensor characteristics
> - *
> - * The structure reports image sensor characteristics used by IPA modules to
> - * tune their algorithms based on the image sensor model currently in use and
> - * its configuration.
> - *
> - * The reported information describes the sensor's intrinsics characteristics,
> - * such as its pixel array size and the sensor model name, as well as
> - * information relative to the currently configured mode, such as the produced
> - * image size and the bit depth of the requested image format.
> - *
> - * Instances of this structure are meant to be assembled by the CameraSensor
> - * class by inspecting the sensor static properties as well as information
> - * relative to the current configuration.
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::model
> - * \brief The image sensor model name
> - *
> - * The sensor model name is a free-formed string that uniquely identifies the
> - * sensor model.
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::bitsPerPixel
> - * \brief The number of bits per pixel of the image format produced by the
> - * image sensor
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::activeAreaSize
> - * \brief The size of the pixel array active area of the sensor
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::analogCrop
> - * \brief The portion of the pixel array active area which is read-out and
> - * processed
> - *
> - * The analog crop rectangle top-left corner is defined as the displacement
> - * from the top-left corner of the pixel array active area. The rectangle
> - * horizontal and vertical sizes define the portion of the pixel array which
> - * is read-out and provided to the sensor's internal processing pipeline, before
> - * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> - * take place.
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::outputSize
> - * \brief The size of the images produced by the camera sensor
> - *
> - * The output image size defines the horizontal and vertical sizes of the images
> - * produced by the image sensor. The output image size is defined as the end
> - * result of the sensor's internal image processing pipeline stages, applied on
> - * the pixel array portion defined by the analog crop rectangle. Each image
> - * processing stage that performs pixel sub-sampling techniques, such as pixel
> - * binning or skipping, or perform any additional digital scaling concur in the
> - * definition of the output image size.
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::pixelRate
> - * \brief The number of pixels produced in a second
> - *
> - * To obtain the read-out time in seconds of a full line:
> - *
> - * \verbatim
> -       lineDuration(s) = lineLength(pixels) / pixelRate(pixels per second)
> -   \endverbatim
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::lineLength
> - * \brief Total line length in pixels
> - *
> - * The total line length in pixel clock periods, including blanking.
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::minFrameLength
> - * \brief The minimum allowable frame length in units of lines
> - *
> - * The sensor frame length comprises of active output lines and blanking lines
> - * in a frame. The minimum frame length value dictates the minimum allowable
> - * frame duration of the sensor mode.
> - *
> - * To obtain the minimum frame duration:
> - *
> - * \verbatim
> -       frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> -   \endverbatim
> - */
> -
> -/**
> - * \var IPACameraSensorInfo::maxFrameLength
> - * \brief The maximum allowable frame length in units of lines
> - *
> - * The sensor frame length comprises of active output lines and blanking lines
> - * in a frame. The maximum frame length value dictates the maximum allowable
> - * frame duration of the sensor mode.
> - *
> - * To obtain the maximum frame duration:
> - *
> - * \verbatim
> -       frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> -   \endverbatim
> - */
> -} /* namespace libcamera */

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list