[libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new platform library
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jun 17 11:10:10 CEST 2021
Hi Laurent,
On 17/06/2021 03:20, Laurent Pinchart wrote:
> 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
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.
>
>> 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.
>
>> # 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).
> 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 ...
>> + 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?
>
>> + 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.
> 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
--
Kieran
More information about the libcamera-devel
mailing list