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

12322 diff mbox series

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@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

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@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')
>  
>

On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
> 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@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.

I'd rather go for a method that would scale indeed :-) One option is to
modify the naming scheme of generated files to be able to match them by
pattern more easily.

> >  # 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.

What's the point if it only contains comments though ? Ideally we
shouldn't have this .cpp file in the first place, it should be generated
automatically, or the documentation comments should be added to the
genearated headers by mojo. We can't do so now so we need this kind of
hack, but I'd rather keep the scope of the hack as small as possible.

> > 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')
> >

On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
> > 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@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.
> 
> I'd rather go for a method that would scale indeed :-) One option is to
> modify the naming scheme of generated files to be able to match them by
> pattern more easily.

{raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
anyway, so we could add their documentation (in
{raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
it in doxygen and meson.

Then the exclude pattern would just be the first two lines.


Paul

> 
> > >  # 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.
> 
> What's the point if it only contains comments though ? Ideally we
> shouldn't have this .cpp file in the first place, it should be generated
> automatically, or the documentation comments should be added to the
> genearated headers by mojo. We can't do so now so we need this kind of
> hack, but I'd rather keep the scope of the hack as small as possible.
> 
> > > 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')
> > >

Hi Paul [& others]

On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
>>> 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@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.
>> I'd rather go for a method that would scale indeed :-) One option is to
>> modify the naming scheme of generated files to be able to match them by
>> pattern more easily.
> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
> anyway, so we could add their documentation (in
> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
> it in doxygen and meson.
Yes, documenting all those IPA modules first might be ideal but not sure 
if may happen all at once. The way I saw, how would this work would be 
addressed is, document one of the modules & exclude it from the 
EXCLUDE_PATTERNS for it's generation.
>
> Then the exclude pattern would just be the first two lines.
When all modules are documented we will have this state of exclude 
pattern again. For new IPA (in-tree) modules appearing meanwhile, I 
think we should only merge them, if they are documented as per the [2/7] 
precedence.

Also want to mention that we can file it as a task and thing it's a good 
first-comers task to work on.
>
>
> Paul
>
>>>>   # 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.
>> What's the point if it only contains comments though ? Ideally we
>> shouldn't have this .cpp file in the first place, it should be generated
>> automatically, or the documentation comments should be added to the
>> genearated headers by mojo. We can't do so now so we need this kind of
>> hack, but I'd rather keep the scope of the hack as small as possible.
>>
>>>> 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')
>>>>

Hi Umang,

On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:
> Hi Paul [& others]
> 
> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
> > On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
> > > On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
> > > > 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@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.
> > > I'd rather go for a method that would scale indeed :-) One option is to
> > > modify the naming scheme of generated files to be able to match them by
> > > pattern more easily.
> > {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
> > anyway, so we could add their documentation (in
> > {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
> > it in doxygen and meson.
> Yes, documenting all those IPA modules first might be ideal but not sure if
> may happen all at once. The way I saw, how would this work would be
> addressed is, document one of the modules & exclude it from the
> EXCLUDE_PATTERNS for it's generation.
> > 
> > Then the exclude pattern would just be the first two lines.
> When all modules are documented we will have this state of exclude pattern
> again. For new IPA (in-tree) modules appearing meanwhile, I think we should
> only merge them, if they are documented as per the [2/7] precedence.
> 
> Also want to mention that we can file it as a task and thing it's a good
> first-comers task to work on.

Ah, I see, remove them from the exclude patterns as they're added. Yeah
that would be a good first-comer task too.

In that case I think a todo comment (either here or in each mojom file?
in the mojo file it won't be picked up by doxygen though... what about
in dummy interface cpp files?) would be useful.


Paul

> > 
> > > > >   # 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.
> > > What's the point if it only contains comments though ? Ideally we
> > > shouldn't have this .cpp file in the first place, it should be generated
> > > automatically, or the documentation comments should be added to the
> > > genearated headers by mojo. We can't do so now so we need this kind of
> > > hack, but I'd rather keep the scope of the hack as small as possible.
> > > 
> > > > > 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')
>

On Thu, May 20, 2021 at 11:28:10AM +0900, paul.elder@ideasonboard.com wrote:
> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
> > On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
> > > 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@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.
> > 
> > I'd rather go for a method that would scale indeed :-) One option is to
> > modify the naming scheme of generated files to be able to match them by
> > pattern more easily.
> 
> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
> anyway, so we could add their documentation (in
> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
> it in doxygen and meson.

I initially thought that documenting device-specific structures was a
bit overkill, but given that those are APIs, I'm beginning to reconsider
this. I'm fine going in that direction.

> Then the exclude pattern would just be the first two lines.
> 
> > > >  # 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.
> > 
> > What's the point if it only contains comments though ? Ideally we
> > shouldn't have this .cpp file in the first place, it should be generated
> > automatically, or the documentation comments should be added to the
> > genearated headers by mojo. We can't do so now so we need this kind of
> > hack, but I'd rather keep the scope of the hack as small as possible.
> > 
> > > > 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')
> > > >

Hi All,

On 19/05/2021 16:55, Laurent Pinchart wrote:
> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
>> 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@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.
> 
> I'd rather go for a method that would scale indeed :-) One option is to
> modify the naming scheme of generated files to be able to match them by
> pattern more easily.
> 
>>>  # 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.
> 
> What's the point if it only contains comments though ? Ideally we
> shouldn't have this .cpp file in the first place, it should be generated
> automatically, or the documentation comments should be added to the
> genearated headers by mojo. We can't do so now so we need this kind of
> hack, but I'd rather keep the scope of the hack as small as possible.


See I knew it ;-) But that's my point - I won't object to this. But it
highlights that we're doing a hack (or a hack on a hack).

a .cpp file to me should be parsed by the compiler.

But I suppose we can't name these files .doxygen ;-)

But this is fine for now.


> 
>>> 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')
>>>  
>

Hi Umang, Paul,

On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:
> Hi Umang,
> 
> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:
>> Hi Paul [& others]
>>
>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
>>>>> 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@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.
>>>> I'd rather go for a method that would scale indeed :-) One option is to
>>>> modify the naming scheme of generated files to be able to match them by
>>>> pattern more easily.
>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
>>> anyway, so we could add their documentation (in
>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
>>> it in doxygen and meson.
>> Yes, documenting all those IPA modules first might be ideal but not sure if
>> may happen all at once. The way I saw, how would this work would be
>> addressed is, document one of the modules & exclude it from the
>> EXCLUDE_PATTERNS for it's generation.
>>>
>>> Then the exclude pattern would just be the first two lines.
>> When all modules are documented we will have this state of exclude pattern
>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should
>> only merge them, if they are documented as per the [2/7] precedence.
>>
>> Also want to mention that we can file it as a task and thing it's a good
>> first-comers task to work on.
> 
> Ah, I see, remove them from the exclude patterns as they're added. Yeah
> that would be a good first-comer task too.
> 
> In that case I think a todo comment (either here or in each mojom file?
> in the mojo file it won't be picked up by doxygen though... what about
> in dummy interface cpp files?) would be useful.

I'm curious - do we have faults if we don't exclude these now?
What is needed to be able to simply remove these platform specifics as
they are?

Presumably it would introduce some doxygen warnings?

If that's what would happen - I would actually prefer to remove the
lines - and have the doxygen warnigns printed - providing clear
motivation to get them fixed as soon as possible (rather even than a todo)

Doxygen warnings aren't fatal - but they are annoying, and that annoying
would help drive getting it fixed.


Anyway, With a solution chosen..., for getting this into the .cpp file
now to progress this series...

Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Paul
> 
>>>
>>>>>>   # 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.
>>>> What's the point if it only contains comments though ? Ideally we
>>>> shouldn't have this .cpp file in the first place, it should be generated
>>>> automatically, or the documentation comments should be added to the
>>>> genearated headers by mojo. We can't do so now so we need this kind of
>>>> hack, but I'd rather keep the scope of the hack as small as possible.
>>>>
>>>>>> 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')
>>

Hi Kieran,

On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:
> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:
> > On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:
> >> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
> >>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
> >>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
> >>>>> 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@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.
> >>>> I'd rather go for a method that would scale indeed :-) One option is to
> >>>> modify the naming scheme of generated files to be able to match them by
> >>>> pattern more easily.
> >>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
> >>> anyway, so we could add their documentation (in
> >>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
> >>> it in doxygen and meson.
> >> Yes, documenting all those IPA modules first might be ideal but not sure if
> >> may happen all at once. The way I saw, how would this work would be
> >> addressed is, document one of the modules & exclude it from the
> >> EXCLUDE_PATTERNS for it's generation.
> >>>
> >>> Then the exclude pattern would just be the first two lines.
> >> When all modules are documented we will have this state of exclude pattern
> >> again. For new IPA (in-tree) modules appearing meanwhile, I think we should
> >> only merge them, if they are documented as per the [2/7] precedence.
> >>
> >> Also want to mention that we can file it as a task and thing it's a good
> >> first-comers task to work on.
> > 
> > Ah, I see, remove them from the exclude patterns as they're added. Yeah
> > that would be a good first-comer task too.
> > 
> > In that case I think a todo comment (either here or in each mojom file?
> > in the mojo file it won't be picked up by doxygen though... what about
> > in dummy interface cpp files?) would be useful.
> 
> I'm curious - do we have faults if we don't exclude these now?
> What is needed to be able to simply remove these platform specifics as
> they are?
> 
> Presumably it would introduce some doxygen warnings?
> 
> If that's what would happen - I would actually prefer to remove the
> lines - and have the doxygen warnigns printed - providing clear
> motivation to get them fixed as soon as possible (rather even than a todo)
> 
> Doxygen warnings aren't fatal - but they are annoying, and that annoying
> would help drive getting it fixed.

We currently build the documentation without warnings, so I'd consider
any warning as a regression that would need to be fixed with the highest
priority, as any other build breakage.

> Anyway, With a solution chosen..., for getting this into the .cpp file
> now to progress this series...
> 
> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >>>>>>   # 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.
> >>>> What's the point if it only contains comments though ? Ideally we
> >>>> shouldn't have this .cpp file in the first place, it should be generated
> >>>> automatically, or the documentation comments should be added to the
> >>>> genearated headers by mojo. We can't do so now so we need this kind of
> >>>> hack, but I'd rather keep the scope of the hack as small as possible.
> >>>>
> >>>>>> 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')

Hi Laurent,

On 21/05/2021 10:15, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:
>> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:
>>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:
>>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
>>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
>>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
>>>>>>> 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@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.
>>>>>> I'd rather go for a method that would scale indeed :-) One option is to
>>>>>> modify the naming scheme of generated files to be able to match them by
>>>>>> pattern more easily.
>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
>>>>> anyway, so we could add their documentation (in
>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
>>>>> it in doxygen and meson.
>>>> Yes, documenting all those IPA modules first might be ideal but not sure if
>>>> may happen all at once. The way I saw, how would this work would be
>>>> addressed is, document one of the modules & exclude it from the
>>>> EXCLUDE_PATTERNS for it's generation.
>>>>>
>>>>> Then the exclude pattern would just be the first two lines.
>>>> When all modules are documented we will have this state of exclude pattern
>>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should
>>>> only merge them, if they are documented as per the [2/7] precedence.
>>>>
>>>> Also want to mention that we can file it as a task and thing it's a good
>>>> first-comers task to work on.
>>>
>>> Ah, I see, remove them from the exclude patterns as they're added. Yeah
>>> that would be a good first-comer task too.
>>>
>>> In that case I think a todo comment (either here or in each mojom file?
>>> in the mojo file it won't be picked up by doxygen though... what about
>>> in dummy interface cpp files?) would be useful.
>>
>> I'm curious - do we have faults if we don't exclude these now?
>> What is needed to be able to simply remove these platform specifics as
>> they are?
>>
>> Presumably it would introduce some doxygen warnings?
>>
>> If that's what would happen - I would actually prefer to remove the
>> lines - and have the doxygen warnigns printed - providing clear
>> motivation to get them fixed as soon as possible (rather even than a todo)
>>
>> Doxygen warnings aren't fatal - but they are annoying, and that annoying
>> would help drive getting it fixed.
> 
> We currently build the documentation without warnings, so I'd consider
> any warning as a regression that would need to be fixed with the highest
> priority, as any other build breakage.

That's sort of my point - the question is ... can we knowingly introduce
those warnings, or should we paper over them by excluding the files
until the documentation is moved over as is done in this patch.

If we remove the lines:
               @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 \

from above, I think what will happen is some warnings will be
introduced, which will mean we must then handle the documentation for
RPi/Vimc/RKISP/IPU3 as a priority.

Either way, as long as it all gets done is what matters.

--
Kieran



> 
>> Anyway, With a solution chosen..., for getting this into the .cpp file
>> now to progress this series...
>>
>> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>>>>>>>   # 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.
>>>>>> What's the point if it only contains comments though ? Ideally we
>>>>>> shouldn't have this .cpp file in the first place, it should be generated
>>>>>> automatically, or the documentation comments should be added to the
>>>>>> genearated headers by mojo. We can't do so now so we need this kind of
>>>>>> hack, but I'd rather keep the scope of the hack as small as possible.
>>>>>>
>>>>>>>> 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')
>

Hi Kieran,

On Fri, May 21, 2021 at 10:19:59AM +0100, Kieran Bingham wrote:
> On 21/05/2021 10:15, Laurent Pinchart wrote:
> > On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:
> >> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:
> >>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:
> >>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
> >>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
> >>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
> >>>>>>> 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@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.
> >>>>>> I'd rather go for a method that would scale indeed :-) One option is to
> >>>>>> modify the naming scheme of generated files to be able to match them by
> >>>>>> pattern more easily.
> >>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
> >>>>> anyway, so we could add their documentation (in
> >>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
> >>>>> it in doxygen and meson.
> >>>> Yes, documenting all those IPA modules first might be ideal but not sure if
> >>>> may happen all at once. The way I saw, how would this work would be
> >>>> addressed is, document one of the modules & exclude it from the
> >>>> EXCLUDE_PATTERNS for it's generation.
> >>>>>
> >>>>> Then the exclude pattern would just be the first two lines.
> >>>> When all modules are documented we will have this state of exclude pattern
> >>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should
> >>>> only merge them, if they are documented as per the [2/7] precedence.
> >>>>
> >>>> Also want to mention that we can file it as a task and thing it's a good
> >>>> first-comers task to work on.
> >>>
> >>> Ah, I see, remove them from the exclude patterns as they're added. Yeah
> >>> that would be a good first-comer task too.
> >>>
> >>> In that case I think a todo comment (either here or in each mojom file?
> >>> in the mojo file it won't be picked up by doxygen though... what about
> >>> in dummy interface cpp files?) would be useful.
> >>
> >> I'm curious - do we have faults if we don't exclude these now?
> >> What is needed to be able to simply remove these platform specifics as
> >> they are?
> >>
> >> Presumably it would introduce some doxygen warnings?
> >>
> >> If that's what would happen - I would actually prefer to remove the
> >> lines - and have the doxygen warnigns printed - providing clear
> >> motivation to get them fixed as soon as possible (rather even than a todo)
> >>
> >> Doxygen warnings aren't fatal - but they are annoying, and that annoying
> >> would help drive getting it fixed.
> > 
> > We currently build the documentation without warnings, so I'd consider
> > any warning as a regression that would need to be fixed with the highest
> > priority, as any other build breakage.
> 
> That's sort of my point - the question is ... can we knowingly introduce
> those warnings, or should we paper over them by excluding the files
> until the documentation is moved over as is done in this patch.

Same answer as for knowingly introducing any other build breakage :-) If
it has to be fixed with the highest priority, it can as well be done as
part of the series.

> If we remove the lines:
>                @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 \
> 
> from above, I think what will happen is some warnings will be
> introduced, which will mean we must then handle the documentation for
> RPi/Vimc/RKISP/IPU3 as a priority.
> 
> Either way, as long as it all gets done is what matters.
> 
> >> Anyway, With a solution chosen..., for getting this into the .cpp file
> >> now to progress this series...
> >>
> >> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >>>>>>>>   # 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.
> >>>>>> What's the point if it only contains comments though ? Ideally we
> >>>>>> shouldn't have this .cpp file in the first place, it should be generated
> >>>>>> automatically, or the documentation comments should be added to the
> >>>>>> genearated headers by mojo. We can't do so now so we need this kind of
> >>>>>> hack, but I'd rather keep the scope of the hack as small as possible.
> >>>>>>
> >>>>>>>> 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')

hi All,

On 5/21/21 2:57 PM, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Fri, May 21, 2021 at 10:19:59AM +0100, Kieran Bingham wrote:
>> On 21/05/2021 10:15, Laurent Pinchart wrote:
>>> On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:
>>>> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:
>>>>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:
>>>>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
>>>>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
>>>>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
>>>>>>>>> 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@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.
>>>>>>>> I'd rather go for a method that would scale indeed :-) One option is to
>>>>>>>> modify the naming scheme of generated files to be able to match them by
>>>>>>>> pattern more easily.
>>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
>>>>>>> anyway, so we could add their documentation (in
>>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
>>>>>>> it in doxygen and meson.
>>>>>> Yes, documenting all those IPA modules first might be ideal but not sure if
>>>>>> may happen all at once. The way I saw, how would this work would be
>>>>>> addressed is, document one of the modules & exclude it from the
>>>>>> EXCLUDE_PATTERNS for it's generation.
>>>>>>> Then the exclude pattern would just be the first two lines.
>>>>>> When all modules are documented we will have this state of exclude pattern
>>>>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should
>>>>>> only merge them, if they are documented as per the [2/7] precedence.
>>>>>>
>>>>>> Also want to mention that we can file it as a task and thing it's a good
>>>>>> first-comers task to work on.
>>>>> Ah, I see, remove them from the exclude patterns as they're added. Yeah
>>>>> that would be a good first-comer task too.
>>>>>
>>>>> In that case I think a todo comment (either here or in each mojom file?
>>>>> in the mojo file it won't be picked up by doxygen though... what about
>>>>> in dummy interface cpp files?) would be useful.
>>>> I'm curious - do we have faults if we don't exclude these now?
>>>> What is needed to be able to simply remove these platform specifics as
>>>> they are?
>>>>
>>>> Presumably it would introduce some doxygen warnings?
>>>>
>>>> If that's what would happen - I would actually prefer to remove the
>>>> lines - and have the doxygen warnigns printed - providing clear
>>>> motivation to get them fixed as soon as possible (rather even than a todo)
>>>>
>>>> Doxygen warnings aren't fatal - but they are annoying, and that annoying
>>>> would help drive getting it fixed.
>>> We currently build the documentation without warnings, so I'd consider
>>> any warning as a regression that would need to be fixed with the highest
>>> priority, as any other build breakage.
>> That's sort of my point - the question is ... can we knowingly introduce
>> those warnings, or should we paper over them by excluding the files
>> until the documentation is moved over as is done in this patch.
> Same answer as for knowingly introducing any other build breakage :-) If
> it has to be fixed with the highest priority, it can as well be done as
> part of the series.
It's not highest priority for this series at-least, given that we were 
just testing waters on how to keep up on documentation for .mojom files. 
I know it's a kind of hack but it's a good stopping point to not get 
ahead of yourselves. I, would really like if we were able to pass in 
mojom files directly to doxygen (even if going through a intermediatary 
step, like extracting comments out first) - but it would be a quite a 
work, I think, to get the plumbing in place. I suspected, that 
documenting all IPAs interfaces will also be take some - hence I have 
left it at a point where it can be done in phases/per-IPA basis, if 
decided to carry on with this precedence of documenting mojom.

Also, Maybe we need a "doc" sprint week :P
>
>> If we remove the lines:
>>                 @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 \
>>
>> from above, I think what will happen is some warnings will be
>> introduced, which will mean we must then handle the documentation for
>> RPi/Vimc/RKISP/IPU3 as a priority.
>>
>> Either way, as long as it all gets done is what matters.
>>
>>>> Anyway, With a solution chosen..., for getting this into the .cpp file
>>>> now to progress this series...
>>>>
>>>> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>
>>>>>>>>>>    # 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.
>>>>>>>> What's the point if it only contains comments though ? Ideally we
>>>>>>>> shouldn't have this .cpp file in the first place, it should be generated
>>>>>>>> automatically, or the documentation comments should be added to the
>>>>>>>> genearated headers by mojo. We can't do so now so we need this kind of
>>>>>>>> hack, but I'd rather keep the scope of the hack as small as possible.
>>>>>>>>
>>>>>>>>>> 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')

Hi Umang,

On Fri, May 21, 2021 at 05:03:27PM +0530, Umang Jain wrote:
> On 5/21/21 2:57 PM, Laurent Pinchart wrote:
> > On Fri, May 21, 2021 at 10:19:59AM +0100, Kieran Bingham wrote:
> >> On 21/05/2021 10:15, Laurent Pinchart wrote:
> >>> On Fri, May 21, 2021 at 10:10:41AM +0100, Kieran Bingham wrote:
> >>>> On 20/05/2021 04:14, paul.elder@ideasonboard.com wrote:
> >>>>> On Thu, May 20, 2021 at 08:26:02AM +0530, Umang Jain wrote:
> >>>>>> On 5/20/21 7:58 AM, paul.elder@ideasonboard.com wrote:
> >>>>>>> On Wed, May 19, 2021 at 06:55:11PM +0300, Laurent Pinchart wrote:
> >>>>>>>> On Wed, May 19, 2021 at 12:11:53PM +0100, Kieran Bingham wrote:
> >>>>>>>>> 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@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.
> >>>>>>>>
> >>>>>>>> I'd rather go for a method that would scale indeed :-) One option is to
> >>>>>>>> modify the naming scheme of generated files to be able to match them by
> >>>>>>>> pattern more easily.
> >>>>>>>
> >>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.h should all be documented
> >>>>>>> anyway, so we could add their documentation (in
> >>>>>>> {raspberrypi,vimc,rkisp1,ipu3}_ipa_interface.cpp) first, and then enable
> >>>>>>> it in doxygen and meson.
> >>>>>>
> >>>>>> Yes, documenting all those IPA modules first might be ideal but not sure if
> >>>>>> may happen all at once. The way I saw, how would this work would be
> >>>>>> addressed is, document one of the modules & exclude it from the
> >>>>>> EXCLUDE_PATTERNS for it's generation.
> >>>>>>
> >>>>>>> Then the exclude pattern would just be the first two lines.
> >>>>>>
> >>>>>> When all modules are documented we will have this state of exclude pattern
> >>>>>> again. For new IPA (in-tree) modules appearing meanwhile, I think we should
> >>>>>> only merge them, if they are documented as per the [2/7] precedence.
> >>>>>>
> >>>>>> Also want to mention that we can file it as a task and thing it's a good
> >>>>>> first-comers task to work on.
> >>>>>
> >>>>> Ah, I see, remove them from the exclude patterns as they're added. Yeah
> >>>>> that would be a good first-comer task too.
> >>>>>
> >>>>> In that case I think a todo comment (either here or in each mojom file?
> >>>>> in the mojo file it won't be picked up by doxygen though... what about
> >>>>> in dummy interface cpp files?) would be useful.
> >>>>
> >>>> I'm curious - do we have faults if we don't exclude these now?
> >>>> What is needed to be able to simply remove these platform specifics as
> >>>> they are?
> >>>>
> >>>> Presumably it would introduce some doxygen warnings?
> >>>>
> >>>> If that's what would happen - I would actually prefer to remove the
> >>>> lines - and have the doxygen warnigns printed - providing clear
> >>>> motivation to get them fixed as soon as possible (rather even than a todo)
> >>>>
> >>>> Doxygen warnings aren't fatal - but they are annoying, and that annoying
> >>>> would help drive getting it fixed.
> >>>
> >>> We currently build the documentation without warnings, so I'd consider
> >>> any warning as a regression that would need to be fixed with the highest
> >>> priority, as any other build breakage.
> >>
> >> That's sort of my point - the question is ... can we knowingly introduce
> >> those warnings, or should we paper over them by excluding the files
> >> until the documentation is moved over as is done in this patch.
> >
> > Same answer as for knowingly introducing any other build breakage :-) If
> > it has to be fixed with the highest priority, it can as well be done as
> > part of the series.
>
> It's not highest priority for this series at-least, given that we were 
> just testing waters on how to keep up on documentation for .mojom files. 
> I know it's a kind of hack but it's a good stopping point to not get 
> ahead of yourselves. I, would really like if we were able to pass in 
> mojom files directly to doxygen (even if going through a intermediatary 
> step, like extracting comments out first) - but it would be a quite a 
> work, I think, to get the plumbing in place. I suspected, that 
> documenting all IPAs interfaces will also be take some - hence I have 
> left it at a point where it can be done in phases/per-IPA basis, if 
> decided to carry on with this precedence of documenting mojom.

I'm fine with that if we exclude those files for now. I don't want to
carry build warning regressions for any extensive period of time.

> Also, Maybe we need a "doc" sprint week :P
>
> >> If we remove the lines:
> >>                 @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 \
> >>
> >> from above, I think what will happen is some warnings will be
> >> introduced, which will mean we must then handle the documentation for
> >> RPi/Vimc/RKISP/IPU3 as a priority.
> >>
> >> Either way, as long as it all gets done is what matters.
> >>
> >>>> Anyway, With a solution chosen..., for getting this into the .cpp file
> >>>> now to progress this series...
> >>>>
> >>>> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>>>
> >>>>>>>>>>    # 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.
> >>>>>>>> What's the point if it only contains comments though ? Ideally we
> >>>>>>>> shouldn't have this .cpp file in the first place, it should be generated
> >>>>>>>> automatically, or the documentation comments should be added to the
> >>>>>>>> genearated headers by mojo. We can't do so now so we need this kind of
> >>>>>>>> hack, but I'd rather keep the scope of the hack as small as possible.
> >>>>>>>>
> >>>>>>>>>> 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')