[libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface documentation to a .cpp file
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri May 21 11:07:15 CEST 2021
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 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.
>
>>> # 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')
>>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list