[libcamera-devel] [PATCH 2/2] libcamera: Documentation: Split public/private documentation
Dan Scally
dan.scally at ideasonboard.com
Fri Jan 5 11:52:01 CET 2024
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.
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?
>>> 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