[libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new platform library
Hirokazu Honda
hiroh at chromium.org
Thu Jun 17 10:15:38 CEST 2021
Hi Laurent,
On Thu, Jun 17, 2021 at 5:07 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Hiro,
>
> On Thu, Jun 17, 2021 at 02:15:56PM +0900, Hirokazu Honda wrote:
> > On Thu, Jun 17, 2021 at 11:21 AM 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
> > > 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,
> >
> > Noob question: I think libcamera-platform can be built stand-alone.
> > Why is libcamera_include needed; I think code in libcamera-platform
> doesn't
> > include headers built as libcamera.so?
>
> libcamera_includes is just a reference to the include/ directory of the
> source tree. The public, internal and platform headers are all located
> there, that's why it's needed.
>
> It could be nice to have an entirely different directory for headers, to
> avoid accidental dependencies from libcamera-platform to libcamera, but
> I'm not sure what the directory hierachy would be, and whether it could
> be clean or would need to be horrible :-)
>
>
Ah, it specifies the include path. Thanks I got it. I am still newbie of
the meson build config. :p
-Hiro
> > > > + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210617/aa0601e9/attachment-0001.htm>
More information about the libcamera-devel
mailing list