[libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface documentation to a .cpp file

Kieran Bingham kieran.bingham at ideasonboard.com
Wed May 19 13:11:53 CEST 2021


Hi Umang,

On 19/05/2021 11:19, Umang Jain wrote:
> Moving the core.mojom documentation to its corresponding .cpp file
> (core_ipa_interface.cpp). This will allow Doxygen to generate the
> documentation for IPABuffer, IPASettings and IPAStream structures.
> Since the .mojom files are placed in include/ directory, the .cpp file
> will live in $sourcedir/src/libcamera/ipa/ - which can also contain
> documentation for other mojom generated IPA interfaces in subsequent
> commit.
> 
> Also, fix the Doxygen warning for the above IPA structs regarding their
> constructors not being documented.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  Documentation/Doxyfile.in                |   8 +-
>  Documentation/meson.build                |   1 +
>  include/libcamera/ipa/core.mojom         |  72 ----------------
>  src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++
>  src/libcamera/ipa/meson.build            |   5 ++
>  src/libcamera/meson.build                |   1 +
>  6 files changed, 118 insertions(+), 74 deletions(-)
>  create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp
>  create mode 100644 src/libcamera/ipa/meson.build
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index af006724..f4d578fa 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -844,7 +844,6 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/span.h \
>  			 @TOP_SRCDIR@/src/libcamera/pipeline/ \
>  			 @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>  			 @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> -			 @TOP_BUILDDIR@/include/libcamera/ipa/ \
>  			 @TOP_BUILDDIR@/src/libcamera/proxy/
>  
>  # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or
> @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS       = NO
>  # Note that the wildcards are matched against the file with absolute path, so to
>  # exclude all test directories for example use the pattern */test/*
>  
> -EXCLUDE_PATTERNS       =
> +EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
> +                         @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \
> +                         @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \
> +                         @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \
> +                         @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \
> +                         @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \


I was going to suggest that perhaps this should be
	@TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?

But that would exclude the core_ipa_interface.h too, which I presume we
need to include for Doxygen to map the documentation to.

I wonder if there's a way to include only the files we need under
include/libcamera/ipa if that list is smaller, rather than trying to
exclude lots of combinations which would need to be updated with every
new platform supported.

>  
>  # The EXCLUDE_SYMBOLS tag can be used to specify one or more symbol names
>  # (namespaces, classes, functions, etc.) that should be excluded from the
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index c8521574..9ecf4dfc 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -24,6 +24,7 @@ if doxygen.found() and dot.found()
>                        doxyfile,
>                        libcamera_internal_headers,
>                        libcamera_ipa_headers,
> +                      libcamera_ipa_interfaces,
>                        libcamera_public_headers,
>                        libcamera_sources,
>                        libipa_headers,
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> index 6caaa63e..e49815d8 100644
> --- a/include/libcamera/ipa/core.mojom
> +++ b/include/libcamera/ipa/core.mojom
> @@ -94,88 +94,16 @@ module libcamera;
>  	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
> - * \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 IPAStream {
>  	uint32 pixelFormat;
>  	Size size;
> diff --git a/src/libcamera/ipa/core_ipa_interface.cpp b/src/libcamera/ipa/core_ipa_interface.cpp
> new file mode 100644
> index 00000000..fe1ecce6
> --- /dev/null
> +++ b/src/libcamera/ipa/core_ipa_interface.cpp
> @@ -0,0 +1,105 @@
> +/* 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 {
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \fn IPABuffer::IPABuffer(uint32_t id, const std::vector<FrameBuffer::Plane> &planes)
> + * \param[in] id
> + * \param[in] planes
> + * \sa id and planes
> + */
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \fn IPASettings::IPASettings(const std::string &configurationFile, const std::string &sensorModel)
> + * \param[in] configurationFile
> + * \param[in] sensorModel
> + * \sa configurationFile and sensorModel
> + */
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \fn IPAStream::IPAStream(uint32_t pixelFormat, const Size &size)
> + * \param[in] pixelFormat
> + * \param[in] size
> + * \sa pixelFormat and size
> + */
> +
> +/**
> + * \var IPAStream::pixelFormat
> + * \brief The stream pixel format
> + */
> +
> +/**
> + * \var IPAStream::size
> + * \brief The stream size in pixels
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/ipa/meson.build b/src/libcamera/ipa/meson.build
> new file mode 100644
> index 00000000..0a16d197
> --- /dev/null
> +++ b/src/libcamera/ipa/meson.build
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +
> +libcamera_ipa_interfaces = files([
> +    'core_ipa_interface.cpp',
> +])

I'm sure someone will disagree with me, but I almost feel like this .cpp
file should get run through a compiler, and therefore just added to the
libcamera sources as normal (which I think would then already get picked
up by doxygen).

That way the syntax of the file is maintained too, given that it is a
C++ file.



> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 7bc59b84..61b5fe21 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -67,6 +67,7 @@ includes = [
>      libcamera_includes,
>  ]
>  
> +subdir('ipa')
>  subdir('pipeline')
>  subdir('proxy')
>  
> 

-- 
Regards
--
Kieran


More information about the libcamera-devel mailing list