[libcamera-devel] [RFC PATCH] ipa: Documentation: Enable documentation of mojom files

[Laurent Pinchart]

Hi Kieran,

On Mon, May 17, 2021 at 10:26:31AM +0100, Kieran Bingham wrote:
> On 15/05/2021 19:26, Laurent Pinchart wrote:
> > On Thu, May 13, 2021 at 04:53:46PM +0900, Paul Elder wrote:
> >> Enable generation of documentation for mojom files. As doxygen cannot
> >> parse mojom files directly, put the comments in a cpp file instead, and
> >> plumb meson and doxygen accordingly.
> > 
> > A really open question: would it make sense to write a small python
> > script that would extract the comments from the .mojom files and
> > generate .cpp files, to keep the documentation close to the definition
> > of the data types ? We don't do so for C++ data types, so maybe it's
> > fine to move the documentation to .cpp files manually. Conceptually we
> > could generate code in different languages, in which case it would make
> > more sense to have the documentation in the .mojom file, but I doubt
> > we'll ever do so.
> 
> If we're going to do that we may as well update doxygen to parse mojom
> files.
> 
> If we script generating some .cpp files to move documentation into, then
> we have to manually add those .cpp files into the meson.build and link
> those generated files to doxygen anyway.

We don't have to compile them though.

> At that point, it's probably better that we simply pass the mojom files
> to Doxygen and teach it how to parse it for comments (or more likely,
> how to ignore any mojom specifics).
> 
> But that then incurs a penalty of requiring to get that unstreamed to
> doxygen and requiring that version before it can be used by anyone
> building our documentation... :-(

And it would also be orders of magnitude more complex to develop. I
don't think it would be reasonable to do so ourselves.

> >> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >>
> >> ---
> >> Obviously we'll have to remove the documentation from core.mojom, and
> >> add documentation accordingly to the other mojom files. This is still
> >> and RFC just to see if this is the direction we want to go.
> >> ---
> >>  Documentation/Doxyfile.in                    |   4 +-
> >>  Documentation/meson.build                    |   1 +
> >>  include/libcamera/ipa/core_ipa_interface.cpp | 190 +++++++++++++++++++
> >>  include/libcamera/ipa/meson.build            |   4 +
> >>  4 files changed, 197 insertions(+), 2 deletions(-)
> >>  create mode 100644 include/libcamera/ipa/core_ipa_interface.cpp
> >>
> >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> >> index af006724..213adb9b 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,8 @@ 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
> >>  
> >>  # 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..86f1134b 100644
> >> --- a/Documentation/meson.build
> >> +++ b/Documentation/meson.build
> >> @@ -23,6 +23,7 @@ if doxygen.found() and dot.found()
> >>                    input : [
> >>                        doxyfile,
> >>                        libcamera_internal_headers,
> >> +                      libcamera_ipa_docs,
> >>                        libcamera_ipa_headers,
> >>                        libcamera_public_headers,
> >>                        libcamera_sources,
> >> diff --git a/include/libcamera/ipa/core_ipa_interface.cpp b/include/libcamera/ipa/core_ipa_interface.cpp
> >> new file mode 100644
> >> index 00000000..f9820b92
> >> --- /dev/null
> >> +++ b/include/libcamera/ipa/core_ipa_interface.cpp
> >> @@ -0,0 +1,190 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +
> >> +namespace libcamera {
> >> +
> >> +/**
> >> + * \struct CameraSensorInfo
> >> + * \brief Report the image sensor characteristics
> >> + *
> >> + * The structure reports image sensor characteristics used by IPA modules to
> >> + * tune their algorithms based on the image sensor model currently in use and
> >> + * its configuration.
> >> + *
> >> + * The reported information describes the sensor's intrinsics characteristics,
> >> + * such as its pixel array size and the sensor model name, as well as
> >> + * information relative to the currently configured mode, such as the produced
> >> + * image size and the bit depth of the requested image format.
> >> + *
> >> + * Instances of this structure are meant to be assembled by the CameraSensor
> >> + * class by inspecting the sensor static properties as well as information
> >> + * relative to the current configuration.
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::model
> >> + * \brief The image sensor model name
> >> + *
> >> + * The sensor model name is a free-formed string that uniquely identifies the
> >> + * sensor model.
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::bitsPerPixel
> >> + * \brief The number of bits per pixel of the image format produced by the
> >> + * image sensor
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::activeAreaSize
> >> + * \brief The size of the pixel array active area of the sensor
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::analogCrop
> >> + * \brief The portion of the pixel array active area which is read-out and
> >> + * processed
> >> + *
> >> + * The analog crop rectangle top-left corner is defined as the displacement
> >> + * from the top-left corner of the pixel array active area. The rectangle
> >> + * horizontal and vertical sizes define the portion of the pixel array which
> >> + * is read-out and provided to the sensor's internal processing pipeline, before
> >> + * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> >> + * take place.
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::outputSize
> >> + * \brief The size of the images produced by the camera sensor
> >> + *
> >> + * The output image size defines the horizontal and vertical sizes of the images
> >> + * produced by the image sensor. The output image size is defined as the end
> >> + * result of the sensor's internal image processing pipeline stages, applied on
> >> + * the pixel array portion defined by the analog crop rectangle. Each image
> >> + * processing stage that performs pixel sub-sampling techniques, such as pixel
> >> + * binning or skipping, or perform any additional digital scaling concur in the
> >> + * definition of the output image size.
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::pixelRate
> >> + * \brief The number of pixels produced in a second
> >> + *
> >> + * To obtain the read-out time in seconds of a full line:
> >> + *
> >> + * \verbatim
> >> +	lineDuration(s) = lineLength(pixels) / pixelRate(pixels per second)
> >> +   \endverbatim
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::lineLength
> >> + * \brief Total line length in pixels
> >> + *
> >> + * The total line length in pixel clock periods, including blanking.
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::minFrameLength
> >> + * \brief The minimum allowable frame length in units of lines
> >> + *
> >> + * The sensor frame length comprises of active output lines and blanking lines
> >> + * in a frame. The minimum frame length value dictates the minimum allowable
> >> + * frame duration of the sensor mode.
> >> + *
> >> + * To obtain the minimum frame duration:
> >> + *
> >> + * \verbatim
> >> +	frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> >> +   \endverbatim
> >> + */
> >> +
> >> +/**
> >> + * \var CameraSensorInfo::maxFrameLength
> >> + * \brief The maximum allowable frame length in units of lines
> >> + *
> >> + * The sensor frame length comprises of active output lines and blanking lines
> >> + * in a frame. The maximum frame length value dictates the maximum allowable
> >> + * frame duration of the sensor mode.
> >> + *
> >> + * To obtain the maximum frame duration:
> >> + *
> >> + * \verbatim
> >> +	frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)
> >> +   \endverbatim
> >> + */
> >> +
> >> +/**
> >> + * \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 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 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
> >> + */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> >> index 40c4e737..69bc855e 100644
> >> --- a/include/libcamera/ipa/meson.build
> >> +++ b/include/libcamera/ipa/meson.build
> >> @@ -6,6 +6,10 @@ libcamera_ipa_headers = files([
> >>      'ipa_module_info.h',
> >>  ])
> >>  
> >> +libcamera_ipa_docs = files([
> >> +    'core_ipa_interface.cpp',
> >> +])
> >> +
> >>  install_headers(libcamera_ipa_headers,
> >>                  subdir: libcamera_include_dir / 'ipa')
> >>  

-- 
Regards,

Laurent Pinchart