| Message ID | 20210616151152.3856595-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 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@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, > + 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')
Hi Kieran, On Thu, Jun 17, 2021 at 11:21 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> 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 > 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@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? -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 >
Hi Kieran, On 6/16/21 8:41 PM, Kieran Bingham wrote: > The libcamera-platform.so will feature internal support functionality > that is utilised by libcamera, and can be shared in other places. this sounds similar to -dev / -devel packages listed in distributions. May we can use that suffix? :-) libcamera: A complex camera support library for Linux, Android, and ChromeOS libcamera-dev: Development files for libcamera > > 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. > > Signed-off-by: Kieran Bingham <kieran.bingham@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" > > # 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, > + 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', > + [libcamera_platform_sources, libcamera_platform_headers], > + name_prefix : '', > + install : true, > + cpp_args : libcamera_cpp_args, > + include_directories : libcamera_includes, isn't libcamera_includes an overkill here? Since we are splitting off the libraries, I think we should map the relevant include-directories for that component? I don't expect you to fix this in this series, but just wanted to see if this was the right way ahead? > + dependencies : libcamera_platform_deps) > + > +libcamera_platform = declare_dependency(sources : [ > + libcamera_platform_headers, > + ], > + include_directories : libcamera_includes, > + link_with : libcamera_platform_lib) > + > +pkg_mod = import('pkgconfig') > +pkg_mod.generate(libraries : libcamera_platform_lib, > + version : '1.0', > + name : 'libcamera-platform', > + filebase : 'camera-platform', > + description : 'Complex Camera Support Library', > + subdirs : 'libcamera') > 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')
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@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 :-) > > > + 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')
Hi Laurent, On Thu, Jun 17, 2021 at 5:07 PM Laurent Pinchart < laurent.pinchart@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@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 >
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@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') >
Hi Kieran, 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). > > 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 :-) > >> 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@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. > >> + 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. > >> + 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 ? > > 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')
Hi Laurent, On 17/06/2021 10:33, Laurent Pinchart wrote: > Hi Kieran, > > 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). >> >> 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@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? >>>> + 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 : '', 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... 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. 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. >>> 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') >
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). > >> > >> 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@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 ? > >>>> + 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. > 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')
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@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') >
Hi Umang, On 17/06/2021 06:32, Umang Jain wrote: > Hi Kieran, > > On 6/16/21 8:41 PM, Kieran Bingham wrote: >> The libcamera-platform.so will feature internal support functionality >> that is utilised by libcamera, and can be shared in other places. > > this sounds similar to -dev / -devel packages listed in distributions. > May we can use that suffix? :-) I don't think we can I'm afraid. the -dev packages are used to imply an application can use that package to build against libcamera. In this case it's exactly the opposite - this is for /libcamera/ (and permitted components, like IPAs, proxy-workers, even android-hal) to share 'private' libcamera interfaces. But they shouldn't be provided to application developers who would be the users expecting a libcamera-dev or libcamera-devel package to utilise libcamera. > libcamera: A complex camera support library for Linux, Android, and > ChromeOS > libcamera-dev: Development files for libcamera > > >> >> 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. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@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" >> # 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, >> + 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', >> + [libcamera_platform_sources, >> libcamera_platform_headers], >> + name_prefix : '', >> + install : true, >> + cpp_args : libcamera_cpp_args, >> + include_directories : >> libcamera_includes, > > > isn't libcamera_includes an overkill here? Since we are splitting off > the libraries, I think we should map the relevant include-directories > for that component? I don't expect you to fix this in this series, but > just wanted to see if this was the right way ahead? These platform headers share a common base with libcamera. #include <libcamera/request.h> #include <libcamera/platform/file.h> The difference is in who is allowed to use them ... >> + dependencies : >> libcamera_platform_deps) >> + >> +libcamera_platform = declare_dependency(sources : [ >> + libcamera_platform_headers, >> + ], >> + include_directories : >> libcamera_includes, >> + link_with : >> libcamera_platform_lib) >> + >> +pkg_mod = import('pkgconfig') >> +pkg_mod.generate(libraries : libcamera_platform_lib, >> + version : '1.0', >> + name : 'libcamera-platform', >> + filebase : 'camera-platform', >> + description : 'Complex Camera Support Library', >> + subdirs : 'libcamera') >> 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')
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@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')
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" # 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, + 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', + [libcamera_platform_sources, libcamera_platform_headers], + name_prefix : '', + install : true, + cpp_args : libcamera_cpp_args, + include_directories : libcamera_includes, + dependencies : libcamera_platform_deps) + +libcamera_platform = declare_dependency(sources : [ + libcamera_platform_headers, + ], + include_directories : libcamera_includes, + link_with : libcamera_platform_lib) + +pkg_mod = import('pkgconfig') +pkg_mod.generate(libraries : libcamera_platform_lib, + version : '1.0', + name : 'libcamera-platform', + filebase : 'camera-platform', + description : 'Complex Camera Support Library', + subdirs : 'libcamera') 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')
The libcamera-platform.so will feature internal support functionality that is utilised by libcamera, and can be shared in other places. 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. Signed-off-by: Kieran Bingham <kieran.bingham@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