[libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new platform library

[Laurent Pinchart]

Hi Kieran,

On Fri, Jun 18, 2021 at 11:20:06AM +0100, Kieran Bingham wrote:
> On 18/06/2021 10:11, Laurent Pinchart wrote:
> > 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.

I like base as well.

> >>>> 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.

s/IPA/IPA module/

>  *
>  * 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

Ditto, s/IPAs/IPA modules/

>  * 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 ;-)

Sounds good.

> >>>>>> +                      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?

No we don't. Adding libcamera-platform.so (or libcamera-base.so ? :-))
to the libcamera.pc shouldn't be a big deal though, so we could do so
already.

> >> 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