[libcamera-devel] [PATCH 2/2] libcamera: Documentation: Split public/private documentation
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jan 5 12:37:28 CET 2024
Quoting Dan Scally via libcamera-devel (2024-01-05 10:52:01)
>
> On 26/12/2023 22:59, Laurent Pinchart via libcamera-devel wrote:
> > On Tue, Dec 26, 2023 at 05:24:37PM +0200, Laurent Pinchart via libcamera-devel wrote:
> >> 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(-)
> > [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.
> > For the record, I initially tried the set the list of input files
> > explicitly. Doxyfile contains an INPUT configuration parameter that we
> > currently set to a list of directories, and I thought I could pass it a
> > list of files instead. We have a partial list of files corresponding to
> > the public API, and it wasn't difficult to split libcamera_sources into
> > public and internal sources.
> >
> > I hit a blocker because the list of files is stored in an array of File
> > objects. I couldn't find a way to convert that to the real path of the
> > files to pass to cdata.set() prior to meson 1.4.0 that introduced
> > file.full_path(). File objects get converted automatically to file paths
> > when passed to lots of functions, so I suppose it would be possible to
> > use a custom_target() to convert the array of Files to strings, but
> > that's becoming quite a bit of a hack.
> >
> > The other option is to include/exclude directories, which we already do
> > actually. I'll see if that can be expanded.
>
>
> So having picked up this experiment, I think sticking with the list of specific files would be the
> way to go. I can generate that without using file.full_path() using a method available to Meson 0.54
> and later, and that seems to work ok. There are two main problems I hit from doing things this way:
>
>
> 1. Definitions of functions for the Private counterpart class in the public class' source file -
> this throws warnings complaining that the functions are undeclared.
>
> 2. Myriad references to the thread-safe anchor in src/libcamera/base/thread.cpp are unresolveable
> without including that source file.
>
>
> To solve the first problem the simplest solution is likely to be to include the internal headers -
> as the classes are Private no documentation is generated for them anyway, though the headers do then
> appear in the Files list in the generated documentation. For the second we can include thread.h/cpp
> and simply flag as \internal everything except the \page block...or alternatively break that comment
> out into a separate file in Documentation/ instead.
Is the Threading model internal or public API? I suspect we don't want
to docuemnt the thread functions in the public API - but we do want to
include the threading model documentation, so breaking out to a separate
.dox file might be appropriate here.
I think it depends how much of thread.cpp is part of the public API
anyway.
> Either way, this route reduces the files the commit touches significantly to basically just the
> meson.builds, and additionally removes the burden of remembering to flag \internal any new functions
> or objects we add, so I do think it's better. Any thoughts on the solutions proposed above?
I think maintaining updates to meson.build should be easier already and
I think we have a checkstyle rule that already tests to see if updates
are made to meson.build in the event of adding new files. So this does
sound like an easier route.
> >>> 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]
More information about the libcamera-devel
mailing list