[libcamera-devel] [PATCH 2/2] libcamera: Documentation: Split public/private documentation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 26 16:24:37 CET 2023
On Tue, Dec 26, 2023 at 11:41:35AM +0100, Jacopo Mondi via libcamera-devel wrote:
> Hi Dan
> thanks for this big chunk of work
>
> On Wed, Dec 20, 2023 at 04:31:44PM +0000, Daniel Scally via libcamera-devel wrote:
> > The API reference pages generated by Doxygen are comprehensive, but
> > therefore quite overwhelming for application developers who will
> > likely never need to use the majority of the library's objects. To
More than likely, it's outright forbidden :-) You can write
"who will only use the public API objects."
> > reduce the complexity of the documentation, split it into two runs of
> > doxygen. In the first run leverage doxygen's \internal and \if flags
> > to hide library objects which are not intended to be exposed through
> > the public headers.
"to hide internal library objects" is enough.
> >
> > In the second run allow doxygen to generate documentation for all of
> > the library's objects as it currently does. This set will now be
> > output into build/Documentation/internal-api-html.
> >
> > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > ---
> > This patch touches a lot of files, but almost exclusively adds doxygen's
> > \internal flag to docu-comments along with the necessary changes to Doxyfile.in
> > and meson.build to implement the hiding of those objects in the auto-generated
> > documentation.
> >
> > The deviation from that rule is in object.cpp, where reference is made to
> > the "internal" Thread class documentation. Because the Object class is retained
> > in the public documents we can't remove the whole thing with \internal which
> > means resorting to \if .. \endif. It's specifically a reference to the Stopping
> > Threads section, and an alternative means of resolving this might be to move
> > that whole section to the Thread Support _page_ instead of being part of the
> > Thread class.
I think you can use
* \internal
* ...
* \endinternal
instead of '\if internal' in object.cpp.
> > Documentation/Doxyfile.in | 9 +++-
> > Documentation/meson.build | 47 +++++++++++++++++--
> > include/libcamera/ipa/core.mojom | 5 ++
> > src/ipa/ipu3/algorithms/af.cpp | 2 +
> > src/ipa/ipu3/algorithms/agc.cpp | 2 +
> > src/ipa/ipu3/algorithms/awb.cpp | 4 ++
> > src/ipa/ipu3/algorithms/blc.cpp | 2 +
> > src/ipa/ipu3/algorithms/tone_mapping.cpp | 2 +
> > src/ipa/ipu3/ipa_context.cpp | 5 ++
> > src/ipa/ipu3/ipu3.cpp | 1 +
> > src/ipa/libipa/algorithm.cpp | 3 ++
> > src/ipa/libipa/camera_sensor_helper.cpp | 8 ++++
> > src/ipa/libipa/fc_queue.cpp | 3 ++
> > src/ipa/libipa/histogram.cpp | 2 +
> > src/ipa/libipa/module.cpp | 2 +
> > src/ipa/rkisp1/algorithms/agc.cpp | 2 +
> > src/ipa/rkisp1/algorithms/awb.cpp | 2 +
> > src/ipa/rkisp1/algorithms/blc.cpp | 2 +
> > src/ipa/rkisp1/algorithms/cproc.cpp | 2 +
> > src/ipa/rkisp1/algorithms/dpcc.cpp | 2 +
> > src/ipa/rkisp1/algorithms/dpf.cpp | 2 +
> > src/ipa/rkisp1/algorithms/filter.cpp | 2 +
> > src/ipa/rkisp1/algorithms/gsl.cpp | 2 +
> > src/ipa/rkisp1/algorithms/lsc.cpp | 2 +
> > src/ipa/rkisp1/ipa_context.cpp | 13 +++++
> > src/libcamera/base/backtrace.cpp | 2 +
> > src/libcamera/base/event_dispatcher.cpp | 2 +
> > src/libcamera/base/event_dispatcher_poll.cpp | 2 +
> > src/libcamera/base/event_notifier.cpp | 3 ++
> > src/libcamera/base/file.cpp | 4 ++
> > src/libcamera/base/log.cpp | 7 +++
> > src/libcamera/base/message.cpp | 4 ++
> > src/libcamera/base/mutex.cpp | 4 ++
> > src/libcamera/base/object.cpp | 13 +++--
> > src/libcamera/base/semaphore.cpp | 2 +
> > src/libcamera/base/thread.cpp | 5 ++
> > src/libcamera/base/timer.cpp | 2 +
> > src/libcamera/base/utils.cpp | 2 +
> > src/libcamera/bayer_format.cpp | 4 ++
> > src/libcamera/byte_stream_buffer.cpp | 2 +
> > src/libcamera/camera.cpp | 1 +
> > src/libcamera/camera_controls.cpp | 2 +
> > src/libcamera/camera_lens.cpp | 2 +
> > src/libcamera/camera_manager.cpp | 1 +
> > src/libcamera/camera_sensor.cpp | 2 +
> > src/libcamera/camera_sensor_properties.cpp | 2 +
> > src/libcamera/control_serializer.cpp | 3 ++
> > src/libcamera/control_validator.cpp | 2 +
> > src/libcamera/converter.cpp | 4 ++
> > .../converter/converter_v4l2_m2m.cpp | 2 +
> > src/libcamera/delayed_controls.cpp | 3 ++
> > src/libcamera/device_enumerator.cpp | 3 ++
> > src/libcamera/framebuffer.cpp | 2 +
> > src/libcamera/ipa_controls.cpp | 4 ++
> > src/libcamera/ipa_data_serializer.cpp | 2 +
> > src/libcamera/ipa_interface.cpp | 2 +
> > src/libcamera/ipa_manager.cpp | 2 +
> > src/libcamera/ipa_module.cpp | 4 ++
> > src/libcamera/ipa_proxy.cpp | 3 ++
> > src/libcamera/ipc_pipe.cpp | 4 ++
> > src/libcamera/ipc_unixsocket.cpp | 3 ++
> > src/libcamera/mapped_framebuffer.cpp | 4 ++
> > src/libcamera/media_device.cpp | 2 +
> > src/libcamera/media_object.cpp | 6 +++
> > src/libcamera/pipeline/ipu3/imgu.cpp | 2 +
> > .../pipeline/rpi/common/delayed_controls.cpp | 3 ++
> > src/libcamera/pipeline_handler.cpp | 4 ++
> > src/libcamera/process.cpp | 4 ++
> > src/libcamera/pub_key.cpp | 2 +
> > src/libcamera/request.cpp | 1 +
> > src/libcamera/source_paths.cpp | 1 +
> > src/libcamera/sysfs.cpp | 1 +
> > src/libcamera/v4l2_device.cpp | 2 +
> > src/libcamera/v4l2_pixelformat.cpp | 3 ++
> > src/libcamera/v4l2_subdevice.cpp | 7 +++
> > src/libcamera/v4l2_videodevice.cpp | 7 +++
> > src/libcamera/yaml_parser.cpp | 5 ++
> > 77 files changed, 285 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index a86ea6c1..6689ace1 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -68,7 +68,14 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \
> >
> > EXCLUDE_SYMLINKS = YES
> >
> > -HTML_OUTPUT = api-html
> > +INTERNAL_DOCS = @INTERNAL_DOCS@
> > +
> > +HIDE_UNDOC_CLASSES = @HIDE_UNDOC_CLASSES@
> > +HIDE_UNDOC_MEMBERS = @HIDE_UNDOC_MEMBERS@
>
> mmm, is this related to the internal/external doc split ?
>
> Documentation says:
> "If the HIDE_UNDOC_CLASSES tag is set to YES, doxygen will hide all
> undocumented classes that are normally visible in the class hierarchy"
>
> Do we want un-documented classes to show up ?
>
> > +
> > +HTML_OUTPUT = @HTML_OUTPUT@
> > +
> > +ENABLED_SECTIONS = @ENABLED_SECTIONS@
> >
> > GENERATE_LATEX = NO
> >
> > diff --git a/Documentation/meson.build b/Documentation/meson.build
> > index 7a58fec8..7d2b7e33 100644
> > --- a/Documentation/meson.build
> > +++ b/Documentation/meson.build
> > @@ -21,14 +21,25 @@ if doxygen.found() and dot.found()
> > doxygen_predefined += '@0@=@1@'.format(key, config_h.get(key))
> > endforeach
> >
> > + # We run doxygen twice - once to build a reduced documentation set that's
>
> More than a "reduced documentation set" I would explicitly say that
> the first run excludes the internal API as it is meant to document the
> public API only.
>
> > + # more appropriate for application developers, and a second run that covers
> > + # all of the library's objects for libcamera developers. To achieve this we
> > + # flag as \internal the comments for objects we wish to hide, and remove the
> > + # auto generated documents via HIDE_UNDOC_CLASSES and HIDE_UNDOC_MEMBERS.
>
> AH! as otherwise the internal classes which result as undocumented
> because flagged as 'internal' will show up anyway in the documentation
> of public API ?
>
> > +
>
> Just checking: 2 lines are intentional ?
One blank line seems enough.
> > +
> > cdata.set('PREDEFINED', ' \\\n\t\t\t '.join(doxygen_predefined))
This belongs before the comment I think.
I would then create a
cdata_public = configuration_data()
cdata_public.merge_from(cdata)
to mimic the cdata_internal below.
> > + cdata.set('INTERNAL_DOCS', 'NO')
> > + cdata.set('HIDE_UNDOC_CLASSES', 'YES')
> > + cdata.set('HIDE_UNDOC_MEMBERS', 'YES')
> > + cdata.set('HTML_OUTPUT', 'api-html')
> > + cdata.set('ENABLED_SECTIONS', '')
> >
> > doxyfile = configure_file(input : 'Doxyfile.in',
And rename doxyfile to doxyfile_public.
> > output : 'Doxyfile',
> > configuration : cdata)
> >
> > - doxygen_input = [
> > - doxyfile,
> > + global_doxygen_input = [
> > libcamera_base_headers,
> > libcamera_base_sources,
> > libcamera_internal_headers,
> > @@ -41,16 +52,44 @@ if doxygen.found() and dot.found()
> > ]
> >
> > if is_variable('ipu3_ipa_sources')
> > - doxygen_input += [ipu3_ipa_sources]
> > + global_doxygen_input += [ipu3_ipa_sources]
> > endif
> >
> > + public_doxygen_input = global_doxygen_input
> > + public_doxygen_input += doxyfile
> > +
> > custom_target('doxygen',
> > - input : doxygen_input,
> > + input : public_doxygen_input,
> > output : 'api-html',
> > command : [doxygen, doxyfile],
> > install : true,
> > install_dir : doc_install_dir,
> > install_tag : 'doc')
> > +
> > + # This is the internal documentation, so _don't_ hide undocumented classes
> > + # as we want everything to show up.
" and warnings to be generated if any documentation is missing."
> > + cdata_internal = configuration_data()
> > + cdata_internal.merge_from(cdata)
> > + cdata_internal.set('HTML_OUTPUT', 'internal-api-html')
> > + cdata_internal.set('INTERNAL_DOCS', 'YES')
> > + cdata_internal.set('HIDE_UNDOC_CLASSES', 'NO')
> > + cdata_internal.set('HIDE_UNDOC_MEMBERS', 'NO')
> > + cdata_internal.set('ENABLED_SECTIONS', 'internal')
> > +
> > + doxyfile_internal = configure_file(input : 'Doxyfile.in',
> > + output : 'Doxyfile-internal',
> > + configuration : cdata_internal)
> > +
> > + internal_doxygen_input = global_doxygen_input
> > + internal_doxygen_input += doxyfile_internal
> > +
> > + custom_target('doxygen-internal',
> > + input : internal_doxygen_input,
> > + output : 'internal-api-html',
> > + command : [doxygen, doxyfile_internal],
> > + install : true,
> > + install_dir : doc_install_dir,
> > + install_tag : 'doc-internal')
> > endif
> >
> > #
[snip]
> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> > index 12927eec..25109451 100644
> > --- a/src/ipa/ipu3/algorithms/af.cpp
>
> Adding an \internal tag is a bit of a tedious job, but since you've
> already done so.... I wonder if it wouldn't be better to exclude some
> directories to doxygen completly. The whole src/ipa/ directory in example
> doesn't contain anything that application developers should be
> concerned with, right ?
>
> > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > @@ -26,6 +26,7 @@
> > #include "libipa/histogram.h"
> >
> > /**
> > + * \internal
> > * \file af.h
> > */
> >
[snip]
> > diff --git a/src/libcamera/base/backtrace.cpp b/src/libcamera/base/backtrace.cpp
> > index be30589d..8c0acdfc 100644
> > --- a/src/libcamera/base/backtrace.cpp
> > +++ b/src/libcamera/base/backtrace.cpp
>
> All components in src/libcamera/base except BoundMethod are internal.
include/libcamera/base/meson.build contains
libcamera_base_public_headers = files([
'bound_method.h',
'class.h',
'compiler.h',
'flags.h',
'object.h',
'shared_fd.h',
'signal.h',
'span.h',
'unique_fd.h',
])
so there's more than just BoundMethod.
> Should we exclude the directory ?
>
> Same for src/libcamera/pipeline. Have you considered the option of not
> parsing the dir completely ?
I think excluding some directories would make sense. I'll experiment.
> Also, I see in the public documentation the Extensible::Private class
> being documented. Is this intentional ?
That class seems internal.
> I'll skip the rest as I have enough questions already.
>
> > @@ -33,6 +33,7 @@
> > #include <libcamera/base/utils.h>
> >
> > /**
> > + * \internal
> > * \file backtrace.h
> > * \brief Generate call stack backtraces
> > */
> > @@ -135,6 +136,7 @@ std::string DwflParser::stackEntry(const void *ip)
> > } /* namespace */
> >
> > /**
> > + * \internal
> > * \class Backtrace
> > * \brief Representation of a call stack backtrace
> > *
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list