[libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new platform library
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jun 18 12:20:06 CEST 2021
Hi Laurent,
On 18/06/2021 10:11, Laurent Pinchart wrote:
> 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).
base is growing on me. Reminds me of 'gstreamer-plugins-base' which is
used for common code on gstreamer plugins. But that then would make me
fear users expecting it to do more as well.
There's never a perfect name ;-)
Before these get merged, it will be easy to rename this from platform to
another name by running sed on the patches themselves and reapplying.
So if you'd prefer another name, now's the time to shout, and we can
make it happen quicker now than later.
>>>> 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 ?
B is fine.
I will likely create a
#include <libcamera/platform/private.h> so that it's common.
Something like:
/* SPDX-License-Identifier: LGPL-2.1-or-later */
/*
* Copyright (C) 2021, Google Inc.
*
* private.h - Private Header Validation
*
* A selection of internal libcamera headers are installed as part
* of the libcamera package to allow sharing of a select subset of
* internal functionality with IPA developers only.
*
* This functionality is not considered part of the public libcamera
* API, and can therefore potentially face ABI instabilities which
* should not be exposed to applications. IPAs however should be
* versioned and more closely matched to the libcamera installation.
*
* Components which include this file can not be included in any file
* which forms part of the libcamera API.
*/
#ifndef LIBCAMERA_PLATFORM_PRIVATE
#error "Private headers must not be included in the libcamera API"
#endif
I've done some testing with this and it's quite effective, so I think
it's helpful.
It's really showing up where the private API's are used ;-)
>>>>>> + 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.
>
Hrm... indeed - I can see how it would be needed for static.
We don't even build statically yet do we?
>> 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
--
Kieran
More information about the libcamera-devel
mailing list