[libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new platform library
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 18 11:11:33 CEST 2021
Hi Kieran,
On Fri, Jun 18, 2021 at 10:00:22AM +0100, Kieran Bingham wrote:
> On 17/06/2021 10:33, Laurent Pinchart wrote:
> > On Thu, Jun 17, 2021 at 10:10:10AM +0100, Kieran Bingham wrote:
> >> On 17/06/2021 03:20, Laurent Pinchart wrote:
> >>> On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:
> >>>> The libcamera-platform.so will feature internal support functionality
> >>>> that is utilised by libcamera, and can be shared in other places.
> >>>
> >>> I'm sure we'll bikeshed the "platform" name :-) We had mentioned
> >>> "support" previously I believe, and I thought that's what you would use,
> >>> but "platform" doesn't sound too bad. The only downside I can see is
> >>
> >> support was really grating on me, in particular when I realised there
> >> are likely to be libraries beneath libcamera, and libraries 'above'.
> >>
> >> Platform to me feels more like the 'base' ... (because you can stand on
> >> a platform).
> >>
> >>> that it could be construed as a library aimed at abstracting differences
> >>> between platforms. "base" may be another option, although I'm not sure
> >>> to like it (the name comes from Chromium, where it has a similar role as
> >>> the library you're creating here).
> >>
> >> As mentioned in the cover letter, it could be that in the future indeed
> >> we might need some platform abstraction. So that's again why I preferred
> >> platform from the beginning.
> >
> > There would likely be a big overlap between this library and the
> > platform abstraction, but I'm a bit concerned that some platform
> > abstraction may need to go elsewhere. I see the goal for this library to
> > be more of a foundation than a platform abstraction.
> >
> > Maybe libcamera-foundation is a possible name :-)
>
> Hrm ... that sounds like something else entirely ...
:-)
> >>>> However - the libcamera-platform library does not constitute a part
> >>>> of the public libcamera API directly. It is a layer beneath libcamera
> >>>> which provides common abstractions to internal objects.
> >>>
> >>> That's partly true only I'm afraid, looking at patch 4/6,
> >>> bound_method.h, object.h and signal.h are moved to this library, and
> >>> they're part of the libcamera public API. It's not a problem in itself.
> >>
> >> Argh - yes - that's annoying - and I had somewhat mis-considered this.
> >>
> >> This commit was written before I ended up pulling in libcamera public
> >> headers, which I only hit deeper down the rabbit-hole ...
> >>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>> ---
> >>>> Documentation/Doxyfile.in | 4 +++-
> >>>> Documentation/meson.build | 2 ++
> >>>> include/libcamera/meson.build | 1 +
> >>>> include/libcamera/platform/meson.build | 9 ++++++++
> >>>> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++
> >>>> src/libcamera/meson.build | 2 ++
> >>>> src/meson.build | 1 +
> >>>> 7 files changed, 47 insertions(+), 1 deletion(-)
> >>>> create mode 100644 include/libcamera/platform/meson.build
> >>>> create mode 100644 src/libcamera-platform/meson.build
> >>>>
> >>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> >>>> index 8305f56af7a8..c1b395bf0b83 100644
> >>>> --- a/Documentation/Doxyfile.in
> >>>> +++ b/Documentation/Doxyfile.in
> >>>> @@ -791,8 +791,10 @@ WARN_LOGFILE =
> >>>> INPUT = "@TOP_SRCDIR@/include/libcamera" \
> >>>> "@TOP_SRCDIR@/src/ipa/libipa" \
> >>>> "@TOP_SRCDIR@/src/libcamera" \
> >>>> + "@TOP_SRCDIR@/src/libcamera-platform" \
> >>>> "@TOP_BUILDDIR@/include/libcamera" \
> >>>> - "@TOP_BUILDDIR@/src/libcamera"
> >>>> + "@TOP_BUILDDIR@/src/libcamera" \
> >>>> + "@TOP_BUILDDIR@/src/libcamera-platform"
> >>>>
> >>>
> >>> Drive-by comment, I've been toying with the idea of splitting the
> >>> Doxygen documentation between the public and internal API.
> >>
> >> I think that's likely a good idea.
> >>
> >> Hopefully we can get some more time to work on documentation
> >> specifically as I think we need some more introduction type pages and
> >> something to actually guide through the doxygen generated pages.
> >
> > Agreed.
> >
> >>>> # This tag can be used to specify the character encoding of the source files
> >>>> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses
> >>>> diff --git a/Documentation/meson.build b/Documentation/meson.build
> >>>> index 9ecf4dfcf79f..01b753f07fb6 100644
> >>>> --- a/Documentation/meson.build
> >>>> +++ b/Documentation/meson.build
> >>>> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()
> >>>> libcamera_ipa_interfaces,
> >>>> libcamera_public_headers,
> >>>> libcamera_sources,
> >>>> + libcamera_platform_headers,
> >>>
> >>> Following up on the comment in the commit message, do we need
> >>> libcamera_platform_internal_headers and
> >>> libcamera_platform_public_headers ? The distinction between the two
> >>> would be different than for libcamera. We currently don't install the
> >>> internal headers, while we'll need to install the "internal platform"
> >>> headers as they can be used by IPA modules. What I'd like to avoid is
> >>> giving an ABI stability guarantee for the whole platform library.
> >>
> >> I'm concerned that would be a mistake.
> >> How can you have public-public headers and public-private-headers?
> >>
> >> All of the headers in this library are publicly exposed.
> >>
> >> I want to be able to say I think the risk of having ABI breakage on this
> >> library component is lower though - because these are mostly just helper
> >> objects, but I know if I were to actually say that, then we'd change the
> >> ABI on say File or ... something ...
> >>
> >> I guess that begs the question of how will libcamera-platform be
> >> versioned. I think as long as it's in the same repo as libcamera itself
> >> - it would share the same versions, and I now have the
> >> abi-compliance-checker which will identify if we do cause an ABI breakage.
> >>
> >> (I hope to see the ABI compliance checker as a pre-integration
> >> validation stage).
> >
> > I think it should be versioned exactly as libcamera. That part doesn't
> > worry me. What bothers me is that the platform headers will contain both
> > stable and non-stable APIs (and ABIs), which will not be easy to convey.
> > Applications will use signal.h, but we don't want them to use thread.h.
> > Even if we document that they shouldn't, some will, and there will be
> > breakages.
> >
> >>> In any case, it's possibly something we can handle later, but as this
> >>> series makes quite a few internal headers public in libcamera-platform,
> >>> I'm a bit worried we will start using them in the public libcamera API.
> >>> Currently there's no risk of doing so by mistake, as the headers are
> >>> clearly marked as internal by their location, and we would immediately
> >>> spot during review an attempt to move an internal header to the public
> >>> directory.
> >>
> >> Yes, having 'you can use this header' ... ' you can not use this header'
> >> would become a bit awkward ...
> >
> > Random idea, how about adding a
> >
> > #ifndef LIBCAMERA_PLATFORM_PRIVATE
> > #error ...
> > #endif
> >
> > at the beginning of all private headers ? Both libcamera and IPA modules
> > would define LIBCAMERA_PLATFORM_PRIVATE. cam and qcam wouldn't. We can
> > bikeshed the LIBCAMERA_PLATFORM_PRIVATE macro name of course :-)
> >
> > This may not be the long term solution we want, but it would already
> > avoid mistakes going unnoticed, and it's a simple change.
>
> That can prevent inclusion of headers which are not permitted indeed,
> and if an application defines LIBCAMERA_PLATFORM_PRIVATE - then they're
> on their own.
>
> It also gives the opportunity to present an appropriate error message
> which I like too.
>
>
> So we have two options:
>
> A)
> Split platform headers into two locations, even though they are
> 'public' to convey that they are not guaranteed to be stable ABI (I
> fear how much we're shooting ourselves in the foot over this)
>
> #include <libcamera/request.h> Public API
> #include <libcamera/platform/signal.h> Public Platform API
> #include <libcamera/platform/internal/file.h> Public but unstable ABI
>
>
> B)
> Keep platform headers together, but define a preprocessor guard
>
> #include <libcamera/request.h> Public API
> #include <libcamera/platform/signal.h> Public Platform API
> #include <libcamera/platform/file.h> Requires '#define LC_P_PRIV'
>
> with s/LC_P_PRIV/LIBCAMERA_PLATFORM_PRIVATE/ of course. Shortened for
> line length.
>
>
> It's worth noting that both A and B can both be done together in fact,
> or perhaps even maybe they should?
I'd start with B as A will be another large patch. We can then see how
we like it, and apply A later if needed. How does that sound ?
> >>>> + libcamera_platform_sources,
> >>>> libipa_headers,
> >>>> libipa_sources,
> >>>> ],
> >>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> >>>> index 086c958b0a53..2c3bbeb8df36 100644
> >>>> --- a/include/libcamera/meson.build
> >>>> +++ b/include/libcamera/meson.build
> >>>> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'
> >>>>
> >>>> subdir('internal')
> >>>> subdir('ipa')
> >>>> +subdir('platform')
> >>>>
> >>>> install_headers(libcamera_public_headers,
> >>>> subdir : include_dir)
> >>>> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build
> >>>> new file mode 100644
> >>>> index 000000000000..c8e0d0c5ba12
> >>>> --- /dev/null
> >>>> +++ b/include/libcamera/platform/meson.build
> >>>> @@ -0,0 +1,9 @@
> >>>> +# SPDX-License-Identifier: CC0-1.0
> >>>> +
> >>>> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'
> >>>> +
> >>>> +libcamera_platform_headers = files([
> >>>> +])
> >>>> +
> >>>> +install_headers(libcamera_platform_headers,
> >>>> + subdir: libcamera_platform_include_dir)
> >>>> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build
> >>>> new file mode 100644
> >>>> index 000000000000..64d0dfee2731
> >>>> --- /dev/null
> >>>> +++ b/src/libcamera-platform/meson.build
> >>>> @@ -0,0 +1,29 @@
> >>>> +# SPDX-License-Identifier: CC0-1.0
> >>>> +
> >>>> +libcamera_platform_sources = files([
> >>>> +])
> >>>> +
> >>>> +libcamera_platform_deps = [
> >>>> +]
> >>>> +
> >>>> +libcamera_platform_lib = shared_library('libcamera_platform',
> >>>
> >>> $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l
> >>> 664
> >>> $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l
> >>> 619
> >>>
> >>> Nearly a draw :-) I've checked because I would have sworn '-' was way
> >>> more common than '_', but it seems it's a personal preference.
> >>
> >> This should be '-', I suspect the _ was a copy paste from the meson
> >> variable name ;-(
> >>
> >>>> + [libcamera_platform_sources, libcamera_platform_headers],
> >>>
> >>> You're missing one space in the indentation.
> >>
> >> ack.
> >>
> >>>> + name_prefix : '',
> >>>
> >>> Any reason why you can't drop this line and use 'camera_platform' as the
> >>> library name ?
> >>
> >> To be consistent with our libcamera meson file?
> >
> > We have
> >
> > libcamera = shared_library('camera',
> > ...
> >
> > with no name_prefix.
>
> That's confused me ;-) Hrm:
>
> gg name_prefix
> src/android/meson.build: name_prefix : '',
> src/ipa/ipu3/meson.build: name_prefix : '',
> src/ipa/raspberrypi/meson.build: name_prefix : '',
> src/ipa/rkisp1/meson.build: name_prefix : '',
> src/ipa/vimc/meson.build: name_prefix : '',
> src/v4l2/meson.build: name_prefix : '',
That's because all of those produce .so files that don't start with
'lib'.
> I must have got the template from src/ipa instead or such then.
>
> I'll update to be the same as the main libcamera one indeed.
>
> >>>> + install : true,
> >>>> + cpp_args : libcamera_cpp_args,
> >>>> + include_directories : libcamera_includes,
> >>>> + dependencies : libcamera_platform_deps)
> >>>> +
> >>>> +libcamera_platform = declare_dependency(sources : [
> >>>> + libcamera_platform_headers,
> >>>
> >>> One space missing here too.
> >>>
> >>>> + ],
> >>>> + include_directories : libcamera_includes,
> >>>> + link_with : libcamera_platform_lib)
> >>>
> >>> Do we actually need this ? Wouldn't it be enough to just link libcamera
> >>> (and the IPA modules) to libcamera_platform_lib ? The reason we have a
> >>> libcamera_dep is because libcamera generates some headers, which needs
> >>> to be done before anything compiling against those headers gets built.
> >>> That's not needed for the platform library (at least not yet :-)).
> >>>
> >>>> +
> >>>> +pkg_mod = import('pkgconfig')
> >>>> +pkg_mod.generate(libraries : libcamera_platform_lib,
> >>>> + version : '1.0',
> >>>> + name : 'libcamera-platform',
> >>>
> >>> One more reason to name the binary libcamera-platform.so ;-)
> >>
> >> Yes, that was my intended name.
> >>
> >>>> + filebase : 'camera-platform',
> >>>> + description : 'Complex Camera Support Library',
> >>>
> >>> This should be updated.
> >>>
> >>>> + subdirs : 'libcamera')
> >>>
> >>> I wonder if we should have a separate pkgconfig file for
> >>> libcamera-platform, or include the library in the libcamera pkgconfig.
> >>> It's an internal split really, and it wouldn't be nice to force
> >>> applications to deal with the libcamera-platform pkgconfig explicitly.
> >>>
> >>> On the other hand, IPA modules will need this. Maybe we should do both,
> >>
> >> Yes - the point is that IPA's want to explicitly pull this in.
> >
> > Would it then make sense to have libcamera-platform in both the
> > libcamera and libcamera-platform pkgconfig ?
>
> I think I'm missing something. libcamera already links against
> libcamera-platform. Applications don't need to...
If an application connects to a signal, it will use a symbol from
libcamera-platform, and should thus link to it.
> What 'part' of libcamera-platform do you want to put into the pkgconfig?
>
> The linker will already pull it in from the link required by libcamera.so.
Are you sure about that ? At least for static linking (which we don't
test yet), an application needs to explicitly link to all the libraries
providing symbols it uses.
> And the headers are already on a common path
> #include <libcamera/platform/signal.h>
>
> Applications shouldn't need to know nor care about libcamera-platform.
Exactly, that's why libcamera.pc should include a -lcamera-platform. No
need for separate -L or -I entries.
> >>> and a libcamera-platform.pc for IPA modules, and include
> >>> libcamera-platform.so in the libraries of libcamera.pc ?
> >>>
> >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >>>> index 54512652272c..6ba59e4006cb 100644
> >>>> --- a/src/libcamera/meson.build
> >>>> +++ b/src/libcamera/meson.build
> >>>> @@ -127,6 +127,7 @@ libcamera_deps = [
> >>>> libgnutls,
> >>>> liblttng,
> >>>> libudev,
> >>>> + libcamera_platform,
> >>>> dependency('threads'),
> >>>> ]
> >>>>
> >>>> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [
> >>>> libcamera_generated_ipa_headers,
> >>>> ],
> >>>> include_directories : libcamera_includes,
> >>>> + dependencies: libcamera_platform,
> >>>> link_with : libcamera)
> >>>>
> >>>> subdir('proxy/worker')
> >>>> diff --git a/src/meson.build b/src/meson.build
> >>>> index a4e96ecd728a..70e1a4618a0f 100644
> >>>> --- a/src/meson.build
> >>>> +++ b/src/meson.build
> >>>> @@ -29,6 +29,7 @@ libcamera_cpp_args = []
> >>>> libcamera_objects = []
> >>>>
> >>>> # libcamera must be built first as a dependency to the other components.
> >>>> +subdir('libcamera-platform')
> >>>> subdir('libcamera')
> >>>>
> >>>> subdir('android')
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list