<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 17, 2021 at 5:07 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Thu, Jun 17, 2021 at 02:15:56PM +0900, Hirokazu Honda wrote:<br>
> On Thu, Jun 17, 2021 at 11:21 AM Laurent Pinchart wrote:<br>
> > On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:<br>
> > > The libcamera-platform.so will feature internal support functionality<br>
> > > that is utilised by libcamera, and can be shared in other places.<br>
> ><br>
> > I'm sure we'll bikeshed the "platform" name :-) We had mentioned<br>
> > "support" previously I believe, and I thought that's what you would use,<br>
> > but "platform" doesn't sound too bad. The only downside I can see is<br>
> > that it could be construed as a library aimed at abstracting differences<br>
> > between platforms. "base" may be another option, although I'm not sure<br>
> > to like it (the name comes from Chromium, where it has a similar role as<br>
> > the library you're creating here).<br>
> ><br>
> > > However - the libcamera-platform library does not constitute a part<br>
> > > of the public libcamera API directly. It is a layer beneath libcamera<br>
> > > which provides common abstractions to internal objects.<br>
> ><br>
> > That's partly true only I'm afraid, looking at patch 4/6,<br>
> > bound_method.h, object.h and signal.h are moved to this library, and<br>
> > they're part of the libcamera public API. It's not a problem in itself.<br>
> ><br>
> > > Signed-off-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> > > ---<br>
> > > Documentation/Doxyfile.in | 4 +++-<br>
> > > Documentation/meson.build | 2 ++<br>
> > > include/libcamera/meson.build | 1 +<br>
> > > include/libcamera/platform/meson.build | 9 ++++++++<br>
> > > src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++<br>
> > > src/libcamera/meson.build | 2 ++<br>
> > > src/meson.build | 1 +<br>
> > > 7 files changed, 47 insertions(+), 1 deletion(-)<br>
> > > create mode 100644 include/libcamera/platform/meson.build<br>
> > > create mode 100644 src/libcamera-platform/meson.build<br>
> > ><br>
> > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in<br>
> > > index 8305f56af7a8..c1b395bf0b83 100644<br>
> > > --- a/Documentation/Doxyfile.in<br>
> > > +++ b/Documentation/Doxyfile.in<br>
> > > @@ -791,8 +791,10 @@ WARN_LOGFILE =<br>
> > > INPUT = "@TOP_SRCDIR@/include/libcamera" \<br>
> > > "@TOP_SRCDIR@/src/ipa/libipa" \<br>
> > > "@TOP_SRCDIR@/src/libcamera" \<br>
> > > + "@TOP_SRCDIR@/src/libcamera-platform" \<br>
> > > "@TOP_BUILDDIR@/include/libcamera" \<br>
> > > - "@TOP_BUILDDIR@/src/libcamera"<br>
> > > + "@TOP_BUILDDIR@/src/libcamera" \<br>
> > > + "@TOP_BUILDDIR@/src/libcamera-platform"<br>
> > ><br>
> ><br>
> > Drive-by comment, I've been toying with the idea of splitting the<br>
> > Doxygen documentation between the public and internal API.<br>
> ><br>
> > > # This tag can be used to specify the character encoding of the source files<br>
> > > # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses<br>
> > > diff --git a/Documentation/meson.build b/Documentation/meson.build<br>
> > > index 9ecf4dfcf79f..01b753f07fb6 100644<br>
> > > --- a/Documentation/meson.build<br>
> > > +++ b/Documentation/meson.build<br>
> > > @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()<br>
> > > libcamera_ipa_interfaces,<br>
> > > libcamera_public_headers,<br>
> > > libcamera_sources,<br>
> > > + libcamera_platform_headers,<br>
> ><br>
> > Following up on the comment in the commit message, do we need<br>
> > libcamera_platform_internal_headers and<br>
> > libcamera_platform_public_headers ? The distinction between the two<br>
> > would be different than for libcamera. We currently don't install the<br>
> > internal headers, while we'll need to install the "internal platform"<br>
> > headers as they can be used by IPA modules. What I'd like to avoid is<br>
> > giving an ABI stability guarantee for the whole platform library.<br>
> ><br>
> > In any case, it's possibly something we can handle later, but as this<br>
> > series makes quite a few internal headers public in libcamera-platform,<br>
> > I'm a bit worried we will start using them in the public libcamera API.<br>
> > Currently there's no risk of doing so by mistake, as the headers are<br>
> > clearly marked as internal by their location, and we would immediately<br>
> > spot during review an attempt to move an internal header to the public<br>
> > directory.<br>
> ><br>
> > > + libcamera_platform_sources,<br>
> > > libipa_headers,<br>
> > > libipa_sources,<br>
> > > ],<br>
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build<br>
> > > index 086c958b0a53..2c3bbeb8df36 100644<br>
> > > --- a/include/libcamera/meson.build<br>
> > > +++ b/include/libcamera/meson.build<br>
> > > @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'<br>
> > ><br>
> > > subdir('internal')<br>
> > > subdir('ipa')<br>
> > > +subdir('platform')<br>
> > ><br>
> > > install_headers(libcamera_public_headers,<br>
> > > subdir : include_dir)<br>
> > > diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build<br>
> > > new file mode 100644<br>
> > > index 000000000000..c8e0d0c5ba12<br>
> > > --- /dev/null<br>
> > > +++ b/include/libcamera/platform/meson.build<br>
> > > @@ -0,0 +1,9 @@<br>
> > > +# SPDX-License-Identifier: CC0-1.0<br>
> > > +<br>
> > > +libcamera_platform_include_dir = libcamera_include_dir / 'platform'<br>
> > > +<br>
> > > +libcamera_platform_headers = files([<br>
> > > +])<br>
> > > +<br>
> > > +install_headers(libcamera_platform_headers,<br>
> > > + subdir: libcamera_platform_include_dir)<br>
> > > diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build<br>
> > > new file mode 100644<br>
> > > index 000000000000..64d0dfee2731<br>
> > > --- /dev/null<br>
> > > +++ b/src/libcamera-platform/meson.build<br>
> > > @@ -0,0 +1,29 @@<br>
> > > +# SPDX-License-Identifier: CC0-1.0<br>
> > > +<br>
> > > +libcamera_platform_sources = files([<br>
> > > +])<br>
> > > +<br>
> > > +libcamera_platform_deps = [<br>
> > > +]<br>
> > > +<br>
> > > +libcamera_platform_lib = shared_library('libcamera_platform',<br>
> ><br>
> > $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l<br>
> > 664<br>
> > $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l<br>
> > 619<br>
> ><br>
> > Nearly a draw :-) I've checked because I would have sworn '-' was way<br>
> > more common than '_', but it seems it's a personal preference.<br>
> ><br>
> > > + [libcamera_platform_sources, libcamera_platform_headers],<br>
> ><br>
> > You're missing one space in the indentation.<br>
> ><br>
> > > + name_prefix : '',<br>
> ><br>
> > Any reason why you can't drop this line and use 'camera_platform' as the<br>
> > library name ?<br>
> ><br>
> > > + install : true,<br>
> > > + cpp_args : libcamera_cpp_args,<br>
> > > + include_directories : libcamera_includes,<br>
> <br>
> Noob question: I think libcamera-platform can be built stand-alone.<br>
> Why is libcamera_include needed; I think code in libcamera-platform doesn't<br>
> include headers built as libcamera.so?<br>
<br>
libcamera_includes is just a reference to the include/ directory of the<br>
source tree. The public, internal and platform headers are all located<br>
there, that's why it's needed.<br>
<br>
It could be nice to have an entirely different directory for headers, to<br>
avoid accidental dependencies from libcamera-platform to libcamera, but<br>
I'm not sure what the directory hierachy would be, and whether it could<br>
be clean or would need to be horrible :-)<br>
<br></blockquote><div><br></div><div>Ah, it specifies the include path. Thanks I got it. I am still newbie of the meson build config. :p</div><div>-Hiro</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > > + dependencies : libcamera_platform_deps)<br>
> > > +<br>
> > > +libcamera_platform = declare_dependency(sources : [<br>
> > > + libcamera_platform_headers,<br>
> ><br>
> > One space missing here too.<br>
> ><br>
> > > + ],<br>
> > > + include_directories : libcamera_includes,<br>
> > > + link_with : libcamera_platform_lib)<br>
> ><br>
> > Do we actually need this ? Wouldn't it be enough to just link libcamera<br>
> > (and the IPA modules) to libcamera_platform_lib ? The reason we have a<br>
> > libcamera_dep is because libcamera generates some headers, which needs<br>
> > to be done before anything compiling against those headers gets built.<br>
> > That's not needed for the platform library (at least not yet :-)).<br>
> ><br>
> > > +<br>
> > > +pkg_mod = import('pkgconfig')<br>
> > > +pkg_mod.generate(libraries : libcamera_platform_lib,<br>
> > > + version : '1.0',<br>
> > > + name : 'libcamera-platform',<br>
> ><br>
> > One more reason to name the binary libcamera-platform.so ;-)<br>
> ><br>
> > > + filebase : 'camera-platform',<br>
> > > + description : 'Complex Camera Support Library',<br>
> ><br>
> > This should be updated.<br>
> ><br>
> > > + subdirs : 'libcamera')<br>
> ><br>
> > I wonder if we should have a separate pkgconfig file for<br>
> > libcamera-platform, or include the library in the libcamera pkgconfig.<br>
> > It's an internal split really, and it wouldn't be nice to force<br>
> > applications to deal with the libcamera-platform pkgconfig explicitly.<br>
> ><br>
> > On the other hand, IPA modules will need this. Maybe we should do both,<br>
> > and a libcamera-platform.pc for IPA modules, and include<br>
> > libcamera-platform.so in the libraries of libcamera.pc ?<br>
> ><br>
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build<br>
> > > index 54512652272c..6ba59e4006cb 100644<br>
> > > --- a/src/libcamera/meson.build<br>
> > > +++ b/src/libcamera/meson.build<br>
> > > @@ -127,6 +127,7 @@ libcamera_deps = [<br>
> > > libgnutls,<br>
> > > liblttng,<br>
> > > libudev,<br>
> > > + libcamera_platform,<br>
> > > dependency('threads'),<br>
> > > ]<br>
> > ><br>
> > > @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [<br>
> > > libcamera_generated_ipa_headers,<br>
> > > ],<br>
> > > include_directories : libcamera_includes,<br>
> > > + dependencies: libcamera_platform,<br>
> > > link_with : libcamera)<br>
> > ><br>
> > > subdir('proxy/worker')<br>
> > > diff --git a/src/meson.build b/src/meson.build<br>
> > > index a4e96ecd728a..70e1a4618a0f 100644<br>
> > > --- a/src/meson.build<br>
> > > +++ b/src/meson.build<br>
> > > @@ -29,6 +29,7 @@ libcamera_cpp_args = []<br>
> > > libcamera_objects = []<br>
> > ><br>
> > > # libcamera must be built first as a dependency to the other components.<br>
> > > +subdir('libcamera-platform')<br>
> > > subdir('libcamera')<br>
> > ><br>
> > > subdir('android')<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>