[libcamera-devel] [PATCH v2 1/7] ipa: Move core IPA interface documentation to a .cpp file
Umang Jain
umang.jain at ideasonboard.com
Fri May 21 13:33:27 CEST 2021
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 at 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 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.
>>>>> 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 at 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')
More information about the libcamera-devel
mailing list