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

12441 diff mbox series

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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
[Update todos] Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
[Update todos] Reviewed-by: Paul Elder <paul.elder@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

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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> [Update todos] Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> [Update todos] Reviewed-by: Paul Elder <paul.elder@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@ideasonboard.com: Update todos]
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

But I'd simply write

Update todos

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@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 */