[libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new platform library
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 17 11:33:27 CEST 2021
Hi Kieran,
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 :-)
> >> 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.
> >> + 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.
> >> + 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 ?
> > 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