[libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface documentation to a .cpp file
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu May 20 10:59:50 CEST 2021
On Thu, May 20, 2021 at 11:28:10AM +0900, paul.elder at 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 at ideasonboard.com>
> > > > ---
> > > > Documentation/Doxyfile.in | 8 +-
> > > > Documentation/meson.build | 1 +
> > > > include/libcamera/ipa/core.mojom | 72 ----------------
> > > > src/libcamera/ipa/core_ipa_interface.cpp | 105 +++++++++++++++++++++++
> > > > src/libcamera/ipa/meson.build | 5 ++
> > > > src/libcamera/meson.build | 1 +
> > > > 6 files changed, 118 insertions(+), 74 deletions(-)
> > > > create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp
> > > > create mode 100644 src/libcamera/ipa/meson.build
> > > >
> > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > > > index af006724..f4d578fa 100644
> > > > --- a/Documentation/Doxyfile.in
> > > > +++ b/Documentation/Doxyfile.in
> > > > @@ -844,7 +844,6 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \
> > > > @TOP_SRCDIR@/src/libcamera/pipeline/ \
> > > > @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> > > > @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> > > > - @TOP_BUILDDIR@/include/libcamera/ipa/ \
> > > > @TOP_BUILDDIR@/src/libcamera/proxy/
> > > >
> > > > # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or
> > > > @@ -861,7 +860,12 @@ EXCLUDE_SYMLINKS = NO
> > > > # Note that the wildcards are matched against the file with absolute path, so to
> > > > # exclude all test directories for example use the pattern */test/*
> > > >
> > > > -EXCLUDE_PATTERNS =
> > > > +EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \
> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/raspberrypi_*.h \
> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/vimc_*.h \
> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/rkisp1_*.h \
> > > > + @TOP_BUILDDIR@/include/libcamera/ipa/ipu3_*.h \
> > >
> > > I was going to suggest that perhaps this should be
> > > @TOP_BUILDDIR@/include/libcamera/ipa/*_interface.h ?
> > >
> > > But that would exclude the core_ipa_interface.h too, which I presume we
> > > need to include for Doxygen to map the documentation to.
> > >
> > > I wonder if there's a way to include only the files we need under
> > > include/libcamera/ipa if that list is smaller, rather than trying to
> > > exclude lots of combinations which would need to be updated with every
> > > new platform supported.
> >
> > 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')
> > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list