[libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new platform library
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 17 04:20:51 CEST 2021
Hi Kieran,
Thank you for the patch.
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
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).
> 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.
> 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.
> # 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.
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.
> + 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.
> + [libcamera_platform_sources, libcamera_platform_headers],
You're missing one space in the indentation.
> + name_prefix : '',
Any reason why you can't drop this line and use 'camera_platform' as the
library name ?
> + 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 ;-)
> + 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,
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