[libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface documentation to a .cpp file
Umang Jain
umang.jain at ideasonboard.com
Thu May 20 04:56:02 CEST 2021
Hi Paul [& others]
On 5/20/21 7:58 AM, 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.
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')
>>>>
More information about the libcamera-devel
mailing list