[{"id":17585,"web_url":"https://patchwork.libcamera.org/comment/17585/","msgid":"","date":"2021-06-17T02:20:51","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n> The libcamera-platform.so will feature internal support functionality\n> that is utilised by libcamera, and can be shared in other places.\n\nI'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n\"support\" previously I believe, and I thought that's what you would use,\nbut \"platform\" doesn't sound too bad. The only downside I can see is\nthat it could be construed as a library aimed at abstracting differences\nbetween platforms. \"base\" may be another option, although I'm not sure\nto like it (the name comes from Chromium, where it has a similar role as\nthe library you're creating here).\n\n> However - the libcamera-platform library does not constitute a part\n> of the public libcamera API directly. It is a layer beneath libcamera\n> which provides common abstractions to internal objects.\n\nThat's partly true only I'm afraid, looking at patch 4/6,\nbound_method.h, object.h and signal.h are moved to this library, and\nthey're part of the libcamera public API. It's not a problem in itself.\n\n> Signed-off-by: Kieran Bingham \n> ---\n> Documentation/Doxyfile.in | 4 +++-\n> Documentation/meson.build | 2 ++\n> include/libcamera/meson.build | 1 +\n> include/libcamera/platform/meson.build | 9 ++++++++\n> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n> src/libcamera/meson.build | 2 ++\n> src/meson.build | 1 +\n> 7 files changed, 47 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/platform/meson.build\n> create mode 100644 src/libcamera-platform/meson.build\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 8305f56af7a8..c1b395bf0b83 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> \n\nDrive-by comment, I've been toying with the idea of splitting the\nDoxygen documentation between the public and internal API.\n\n> # This tag can be used to specify the character encoding of the source files\n> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> index 9ecf4dfcf79f..01b753f07fb6 100644\n> --- a/Documentation/meson.build\n> +++ b/Documentation/meson.build\n> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> libcamera_ipa_interfaces,\n> libcamera_public_headers,\n> libcamera_sources,\n> + libcamera_platform_headers,\n\nFollowing up on the comment in the commit message, do we need\nlibcamera_platform_internal_headers and\nlibcamera_platform_public_headers ? The distinction between the two\nwould be different than for libcamera. We currently don't install the\ninternal headers, while we'll need to install the \"internal platform\"\nheaders as they can be used by IPA modules. What I'd like to avoid is\ngiving an ABI stability guarantee for the whole platform library.\n\nIn any case, it's possibly something we can handle later, but as this\nseries makes quite a few internal headers public in libcamera-platform,\nI'm a bit worried we will start using them in the public libcamera API.\nCurrently there's no risk of doing so by mistake, as the headers are\nclearly marked as internal by their location, and we would immediately\nspot during review an attempt to move an internal header to the public\ndirectory.\n\n> + libcamera_platform_sources,\n> libipa_headers,\n> libipa_sources,\n> ],\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 086c958b0a53..2c3bbeb8df36 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> \n> subdir('internal')\n> subdir('ipa')\n> +subdir('platform')\n> \n> install_headers(libcamera_public_headers,\n> subdir : include_dir)\n> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n> new file mode 100644\n> index 000000000000..c8e0d0c5ba12\n> --- /dev/null\n> +++ b/include/libcamera/platform/meson.build\n> @@ -0,0 +1,9 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> +\n> +libcamera_platform_headers = files([\n> +])\n> +\n> +install_headers(libcamera_platform_headers,\n> + subdir: libcamera_platform_include_dir)\n> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n> new file mode 100644\n> index 000000000000..64d0dfee2731\n> --- /dev/null\n> +++ b/src/libcamera-platform/meson.build\n> @@ -0,0 +1,29 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_platform_sources = files([\n> +])\n> +\n> +libcamera_platform_deps = [\n> +]\n> +\n> +libcamera_platform_lib = shared_library('libcamera_platform',\n\n$ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n664\n$ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n619\n\nNearly a draw :-) I've checked because I would have sworn '-' was way\nmore common than '_', but it seems it's a personal preference.\n\n> + [libcamera_platform_sources, libcamera_platform_headers],\n\nYou're missing one space in the indentation.\n\n> + name_prefix : '',\n\nAny reason why you can't drop this line and use 'camera_platform' as the\nlibrary name ?\n\n> + install : true,\n> + cpp_args : libcamera_cpp_args,\n> + include_directories : libcamera_includes,\n> + dependencies : libcamera_platform_deps)\n> +\n> +libcamera_platform = declare_dependency(sources : [\n> + libcamera_platform_headers,\n\nOne space missing here too.\n\n> + ],\n> + include_directories : libcamera_includes,\n> + link_with : libcamera_platform_lib)\n\nDo we actually need this ? Wouldn't it be enough to just link libcamera\n(and the IPA modules) to libcamera_platform_lib ? The reason we have a\nlibcamera_dep is because libcamera generates some headers, which needs\nto be done before anything compiling against those headers gets built.\nThat's not needed for the platform library (at least not yet :-)).\n\n> +\n> +pkg_mod = import('pkgconfig')\n> +pkg_mod.generate(libraries : libcamera_platform_lib,\n> + version : '1.0',\n> + name : 'libcamera-platform',\n\nOne more reason to name the binary libcamera-platform.so ;-)\n\n> + filebase : 'camera-platform',\n> + description : 'Complex Camera Support Library',\n\nThis should be updated.\n\n> + subdirs : 'libcamera')\n\nI wonder if we should have a separate pkgconfig file for\nlibcamera-platform, or include the library in the libcamera pkgconfig.\nIt's an internal split really, and it wouldn't be nice to force\napplications to deal with the libcamera-platform pkgconfig explicitly.\n\nOn the other hand, IPA modules will need this. Maybe we should do both,\nand a libcamera-platform.pc for IPA modules, and include\nlibcamera-platform.so in the libraries of libcamera.pc ?\n\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 54512652272c..6ba59e4006cb 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -127,6 +127,7 @@ libcamera_deps = [\n> libgnutls,\n> liblttng,\n> libudev,\n> + libcamera_platform,\n> dependency('threads'),\n> ]\n> \n> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> libcamera_generated_ipa_headers,\n> ],\n> include_directories : libcamera_includes,\n> + dependencies: libcamera_platform,\n> link_with : libcamera)\n> \n> subdir('proxy/worker')\n> diff --git a/src/meson.build b/src/meson.build\n> index a4e96ecd728a..70e1a4618a0f 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> libcamera_objects = []\n> \n> # libcamera must be built first as a dependency to the other components.\n> +subdir('libcamera-platform')\n> subdir('libcamera')\n> \n> subdir('android')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 17702C3218\n\tfor ;\n\tThu, 17 Jun 2021 02:21:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69BAD6892F;\n\tThu, 17 Jun 2021 04:21:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CE9046029D\n\tfor ;\n\tThu, 17 Jun 2021 04:21:13 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 408CFE53;\n\tThu, 17 Jun 2021 04:21:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qWRhqGbq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623896473;\n\tbh=TeoHYWTfszcY4SaIf6d43YAjU3wLBaxFP3/rMiGkGy4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qWRhqGbqMSIFrJcaEIz9Gpgh46nb91O10JgAS2ckAYB/FMXVJQPFWVLYUxHWbMOZ0\n\tLkU+OAbwx9wTeUQFasLyWPef8ta6oSGp3zBNpvFttC6sghR7VNxwGG9MUWBc4idLPx\n\tZEIsVsWK97gqHn6Ni2N3oFAxM9bN7Yb+HWROTUro=","Date":"Thu, 17 Jun 2021 05:20:51 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17594,"web_url":"https://patchwork.libcamera.org/comment/17594/","msgid":"","date":"2021-06-17T05:15:56","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran,\n\nOn Thu, Jun 17, 2021 at 11:21 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n> > The libcamera-platform.so will feature internal support functionality\n> > that is utilised by libcamera, and can be shared in other places.\n>\n> I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n> \"support\" previously I believe, and I thought that's what you would use,\n> but \"platform\" doesn't sound too bad. The only downside I can see is\n> that it could be construed as a library aimed at abstracting differences\n> between platforms. \"base\" may be another option, although I'm not sure\n> to like it (the name comes from Chromium, where it has a similar role as\n> the library you're creating here).\n>\n> > However - the libcamera-platform library does not constitute a part\n> > of the public libcamera API directly. It is a layer beneath libcamera\n> > which provides common abstractions to internal objects.\n>\n> That's partly true only I'm afraid, looking at patch 4/6,\n> bound_method.h, object.h and signal.h are moved to this library, and\n> they're part of the libcamera public API. It's not a problem in itself.\n>\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > Documentation/Doxyfile.in | 4 +++-\n> > Documentation/meson.build | 2 ++\n> > include/libcamera/meson.build | 1 +\n> > include/libcamera/platform/meson.build | 9 ++++++++\n> > src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 2 ++\n> > src/meson.build | 1 +\n> > 7 files changed, 47 insertions(+), 1 deletion(-)\n> > create mode 100644 include/libcamera/platform/meson.build\n> > create mode 100644 src/libcamera-platform/meson.build\n> >\n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index 8305f56af7a8..c1b395bf0b83 100644\n> > --- a/Documentation/Doxyfile.in\n> > +++ b/Documentation/Doxyfile.in\n> > @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> > INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> > \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> > \"@TOP_SRCDIR@/src/libcamera\" \\\n> > + \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> > \"@TOP_BUILDDIR@/include/libcamera\" \\\n> > - \"@TOP_BUILDDIR@/src/libcamera\"\n> > + \"@TOP_BUILDDIR@/src/libcamera\" \\\n> > + \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> >\n>\n> Drive-by comment, I've been toying with the idea of splitting the\n> Doxygen documentation between the public and internal API.\n>\n> > # This tag can be used to specify the character encoding of the source\n> files\n> > # that doxygen parses. Internally doxygen uses the UTF-8 encoding.\n> Doxygen uses\n> > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > index 9ecf4dfcf79f..01b753f07fb6 100644\n> > --- a/Documentation/meson.build\n> > +++ b/Documentation/meson.build\n> > @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> > libcamera_ipa_interfaces,\n> > libcamera_public_headers,\n> > libcamera_sources,\n> > + libcamera_platform_headers,\n>\n> Following up on the comment in the commit message, do we need\n> libcamera_platform_internal_headers and\n> libcamera_platform_public_headers ? The distinction between the two\n> would be different than for libcamera. We currently don't install the\n> internal headers, while we'll need to install the \"internal platform\"\n> headers as they can be used by IPA modules. What I'd like to avoid is\n> giving an ABI stability guarantee for the whole platform library.\n>\n> In any case, it's possibly something we can handle later, but as this\n> series makes quite a few internal headers public in libcamera-platform,\n> I'm a bit worried we will start using them in the public libcamera API.\n> Currently there's no risk of doing so by mistake, as the headers are\n> clearly marked as internal by their location, and we would immediately\n> spot during review an attempt to move an internal header to the public\n> directory.\n>\n> > + libcamera_platform_sources,\n> > libipa_headers,\n> > libipa_sources,\n> > ],\n> > diff --git a/include/libcamera/meson.build\n> b/include/libcamera/meson.build\n> > index 086c958b0a53..2c3bbeb8df36 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> >\n> > subdir('internal')\n> > subdir('ipa')\n> > +subdir('platform')\n> >\n> > install_headers(libcamera_public_headers,\n> > subdir : include_dir)\n> > diff --git a/include/libcamera/platform/meson.build\n> b/include/libcamera/platform/meson.build\n> > new file mode 100644\n> > index 000000000000..c8e0d0c5ba12\n> > --- /dev/null\n> > +++ b/include/libcamera/platform/meson.build\n> > @@ -0,0 +1,9 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> > +\n> > +libcamera_platform_headers = files([\n> > +])\n> > +\n> > +install_headers(libcamera_platform_headers,\n> > + subdir: libcamera_platform_include_dir)\n> > diff --git a/src/libcamera-platform/meson.build\n> b/src/libcamera-platform/meson.build\n> > new file mode 100644\n> > index 000000000000..64d0dfee2731\n> > --- /dev/null\n> > +++ b/src/libcamera-platform/meson.build\n> > @@ -0,0 +1,29 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +libcamera_platform_sources = files([\n> > +])\n> > +\n> > +libcamera_platform_deps = [\n> > +]\n> > +\n> > +libcamera_platform_lib = shared_library('libcamera_platform',\n>\n> $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n> 664\n> $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n> 619\n>\n> Nearly a draw :-) I've checked because I would have sworn '-' was way\n> more common than '_', but it seems it's a personal preference.\n>\n> > + [libcamera_platform_sources,\n> libcamera_platform_headers],\n>\n> You're missing one space in the indentation.\n>\n> > + name_prefix : '',\n>\n> Any reason why you can't drop this line and use 'camera_platform' as the\n> library name ?\n>\n> > + install : true,\n> > + cpp_args : libcamera_cpp_args,\n> > + include_directories :\n> libcamera_includes,\n>\n\nNoob question: I think libcamera-platform can be built stand-alone.\nWhy is libcamera_include needed; I think code in libcamera-platform doesn't\ninclude headers built as libcamera.so?\n\n-Hiro\n\n> > + dependencies :\n> libcamera_platform_deps)\n> > +\n> > +libcamera_platform = declare_dependency(sources : [\n> > + libcamera_platform_headers,\n>\n> One space missing here too.\n>\n> > + ],\n> > + include_directories :\n> libcamera_includes,\n> > + link_with :\n> libcamera_platform_lib)\n>\n> Do we actually need this ? Wouldn't it be enough to just link libcamera\n> (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n> libcamera_dep is because libcamera generates some headers, which needs\n> to be done before anything compiling against those headers gets built.\n> That's not needed for the platform library (at least not yet :-)).\n>\n> > +\n> > +pkg_mod = import('pkgconfig')\n> > +pkg_mod.generate(libraries : libcamera_platform_lib,\n> > + version : '1.0',\n> > + name : 'libcamera-platform',\n>\n> One more reason to name the binary libcamera-platform.so ;-)\n>\n> > + filebase : 'camera-platform',\n> > + description : 'Complex Camera Support Library',\n>\n> This should be updated.\n>\n> > + subdirs : 'libcamera')\n>\n> I wonder if we should have a separate pkgconfig file for\n> libcamera-platform, or include the library in the libcamera pkgconfig.\n> It's an internal split really, and it wouldn't be nice to force\n> applications to deal with the libcamera-platform pkgconfig explicitly.\n>\n> On the other hand, IPA modules will need this. Maybe we should do both,\n> and a libcamera-platform.pc for IPA modules, and include\n> libcamera-platform.so in the libraries of libcamera.pc ?\n>\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 54512652272c..6ba59e4006cb 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -127,6 +127,7 @@ libcamera_deps = [\n> > libgnutls,\n> > liblttng,\n> > libudev,\n> > + libcamera_platform,\n> > dependency('threads'),\n> > ]\n> >\n> > @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> > libcamera_generated_ipa_headers,\n> > ],\n> > include_directories :\n> libcamera_includes,\n> > + dependencies: libcamera_platform,\n> > link_with : libcamera)\n> >\n> > subdir('proxy/worker')\n> > diff --git a/src/meson.build b/src/meson.build\n> > index a4e96ecd728a..70e1a4618a0f 100644\n> > --- a/src/meson.build\n> > +++ b/src/meson.build\n> > @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> > libcamera_objects = []\n> >\n> > # libcamera must be built first as a dependency to the other components.\n> > +subdir('libcamera-platform')\n> > subdir('libcamera')\n> >\n> > subdir('android')\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DDA2FBD78E\n\tfor ;\n\tThu, 17 Jun 2021 05:16:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6046168941;\n\tThu, 17 Jun 2021 07:16:09 +0200 (CEST)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D66E60296\n\tfor ;\n\tThu, 17 Jun 2021 07:16:08 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id ji1so1445887ejc.4\n\tfor ;\n\tWed, 16 Jun 2021 22:16:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"PlXNqOKn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Xb5FZ4/wLDxQDhWTYH199tjSss4mqK/E2eTIVAwTRDo=;\n\tb=PlXNqOKn3X38Qy+4Ti0pWBGtwe+5Soxd2Xfl9WxDeoCdXhYYAY+VKrRh1uF0ezodj+\n\tEm6f+wTEzM6OWeOYnvVWyBvmOUCeYxK0jFZjaGPLY/S7AE7Q54v6bwzrrKvH8Me0pXjG\n\t8fysSQnefSYvh+0HimPM4tQb3rPLq/pL6H/HQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Xb5FZ4/wLDxQDhWTYH199tjSss4mqK/E2eTIVAwTRDo=;\n\tb=S4oO3XoNtaRgjgN723LJljs01nOcWpKWLp5qJ0odtnJDQFHD1Ws33EoSYc4Wum+tXP\n\tWpwmzc2TAJ7XnX2EyhO25yxfWCr+R/dpSjNYQU6ZLgxARU5Reli3ItXtPykUVqmQoyIR\n\tnh8VF/5j3w6W9SJF/GsAF3dOzX4Yp0ut8DomQfMAJl6CBDXnnhPdnHXmKqYuQbajspM5\n\t/S2RMhrhiB2nCOHswKabY0Gk+ZYP5xswpxsNZbpd5qMfbSpjZ1omIEPP6UOTWYxs1cCf\n\tc2uFHfWOojSfynIEL19xlek7CNpqPdJimmgoCrd8kWSzh4izYobFckb4yMx2Su1+PLah\n\tUqGw==","X-Gm-Message-State":"AOAM532gsH0TrunYJZgHF+p56SsgXpwDSsqZcAh1OP1W4/PFKLTMVV39\n\tt8JVd/W+ZFgQPjqDWCHQMUrJiw70p8v2/T6VBnuHZ2+8pszQ3w==","X-Google-Smtp-Source":"ABdhPJzkPynclv9mERWjqePrnXlzITASaUxpI7+vGoERZ8mu3Q4DsP+Kq2LD0pLUecFRhJHrkRz+8tBtEcYa+BqPvlA=","X-Received":"by 2002:a17:906:dbd8:: with SMTP id\n\tyc24mr3201359ejb.55.1623906967666; \n\tWed, 16 Jun 2021 22:16:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 17 Jun 2021 14:15:56 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000f4d2bd05c4ef4d7a\"","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17597,"web_url":"https://patchwork.libcamera.org/comment/17597/","msgid":"<9353eecf-be09-01af-8c06-ab10004ad1bf@ideasonboard.com>","date":"2021-06-17T05:32:39","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 6/16/21 8:41 PM, Kieran Bingham wrote:\n> The libcamera-platform.so will feature internal support functionality\n> that is utilised by libcamera, and can be shared in other places.\n\nthis sounds similar to -dev / -devel packages listed in distributions. \nMay we can use that suffix? :-)\n\nlibcamera: A complex camera support library for Linux, Android, and ChromeOS\nlibcamera-dev: Development files for libcamera\n\n\n>\n> However - the libcamera-platform library does not constitute a part\n> of the public libcamera API directly. It is a layer beneath libcamera\n> which provides common abstractions to internal objects.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> Documentation/Doxyfile.in | 4 +++-\n> Documentation/meson.build | 2 ++\n> include/libcamera/meson.build | 1 +\n> include/libcamera/platform/meson.build | 9 ++++++++\n> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n> src/libcamera/meson.build | 2 ++\n> src/meson.build | 1 +\n> 7 files changed, 47 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/platform/meson.build\n> create mode 100644 src/libcamera-platform/meson.build\n>\n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 8305f56af7a8..c1b395bf0b83 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> \n> # This tag can be used to specify the character encoding of the source files\n> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> index 9ecf4dfcf79f..01b753f07fb6 100644\n> --- a/Documentation/meson.build\n> +++ b/Documentation/meson.build\n> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> libcamera_ipa_interfaces,\n> libcamera_public_headers,\n> libcamera_sources,\n> + libcamera_platform_headers,\n> + libcamera_platform_sources,\n> libipa_headers,\n> libipa_sources,\n> ],\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 086c958b0a53..2c3bbeb8df36 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> \n> subdir('internal')\n> subdir('ipa')\n> +subdir('platform')\n> \n> install_headers(libcamera_public_headers,\n> subdir : include_dir)\n> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n> new file mode 100644\n> index 000000000000..c8e0d0c5ba12\n> --- /dev/null\n> +++ b/include/libcamera/platform/meson.build\n> @@ -0,0 +1,9 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> +\n> +libcamera_platform_headers = files([\n> +])\n> +\n> +install_headers(libcamera_platform_headers,\n> + subdir: libcamera_platform_include_dir)\n> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n> new file mode 100644\n> index 000000000000..64d0dfee2731\n> --- /dev/null\n> +++ b/src/libcamera-platform/meson.build\n> @@ -0,0 +1,29 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_platform_sources = files([\n> +])\n> +\n> +libcamera_platform_deps = [\n> +]\n> +\n> +libcamera_platform_lib = shared_library('libcamera_platform',\n> + [libcamera_platform_sources, libcamera_platform_headers],\n> + name_prefix : '',\n> + install : true,\n> + cpp_args : libcamera_cpp_args,\n> + include_directories : libcamera_includes,\n\n\nisn't libcamera_includes an overkill here? Since we are splitting off \nthe libraries, I think we should map the relevant include-directories \nfor that component? I don't expect you to fix this in this series, but \njust wanted to see if this was the right way ahead?\n\n\n> + dependencies : libcamera_platform_deps)\n> +\n> +libcamera_platform = declare_dependency(sources : [\n> + libcamera_platform_headers,\n> + ],\n> + include_directories : libcamera_includes,\n> + link_with : libcamera_platform_lib)\n> +\n> +pkg_mod = import('pkgconfig')\n> +pkg_mod.generate(libraries : libcamera_platform_lib,\n> + version : '1.0',\n> + name : 'libcamera-platform',\n> + filebase : 'camera-platform',\n> + description : 'Complex Camera Support Library',\n> + subdirs : 'libcamera')\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 54512652272c..6ba59e4006cb 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -127,6 +127,7 @@ libcamera_deps = [\n> libgnutls,\n> liblttng,\n> libudev,\n> + libcamera_platform,\n> dependency('threads'),\n> ]\n> \n> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> libcamera_generated_ipa_headers,\n> ],\n> include_directories : libcamera_includes,\n> + dependencies: libcamera_platform,\n> link_with : libcamera)\n> \n> subdir('proxy/worker')\n> diff --git a/src/meson.build b/src/meson.build\n> index a4e96ecd728a..70e1a4618a0f 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> libcamera_objects = []\n> \n> # libcamera must be built first as a dependency to the other components.\n> +subdir('libcamera-platform')\n> subdir('libcamera')\n> \n> subdir('android')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3CE7DBD78E\n\tfor ;\n\tThu, 17 Jun 2021 05:32:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2A8368947;\n\tThu, 17 Jun 2021 07:32:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6446260296\n\tfor ;\n\tThu, 17 Jun 2021 07:32:45 +0200 (CEST)","from [192.168.0.107] (unknown [103.238.109.26])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D4C9E7B;\n\tThu, 17 Jun 2021 07:32:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KrW1U50G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623907965;\n\tbh=YEVDKYlCIixb/3pGHVd1IdDfHrzBxLdd5D/UuNTtZv4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=KrW1U50GcCoWGltYx+uWFoo77mmneKvimZAZwPUs9bLOjWtq/uK+wJxhJ17pCR4t8\n\tj0zhzxqdaPRGqIzmj/25Jtphs5nO7YRDj41LigeWIH6QGyUVed3LWmcbR3M6BtHPt2\n\t1kLs9k+IeOYTpo8ENPjP7ttRbQbbPkeMVZF3XPAg=","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<9353eecf-be09-01af-8c06-ab10004ad1bf@ideasonboard.com>","Date":"Thu, 17 Jun 2021 11:02:39 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17609,"web_url":"https://patchwork.libcamera.org/comment/17609/","msgid":"","date":"2021-06-17T08:06:50","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Thu, Jun 17, 2021 at 02:15:56PM +0900, Hirokazu Honda wrote:\n> On Thu, Jun 17, 2021 at 11:21 AM Laurent Pinchart wrote:\n> > On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n> > > The libcamera-platform.so will feature internal support functionality\n> > > that is utilised by libcamera, and can be shared in other places.\n> >\n> > I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n> > \"support\" previously I believe, and I thought that's what you would use,\n> > but \"platform\" doesn't sound too bad. The only downside I can see is\n> > that it could be construed as a library aimed at abstracting differences\n> > between platforms. \"base\" may be another option, although I'm not sure\n> > to like it (the name comes from Chromium, where it has a similar role as\n> > the library you're creating here).\n> >\n> > > However - the libcamera-platform library does not constitute a part\n> > > of the public libcamera API directly. It is a layer beneath libcamera\n> > > which provides common abstractions to internal objects.\n> >\n> > That's partly true only I'm afraid, looking at patch 4/6,\n> > bound_method.h, object.h and signal.h are moved to this library, and\n> > they're part of the libcamera public API. It's not a problem in itself.\n> >\n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > Documentation/Doxyfile.in | 4 +++-\n> > > Documentation/meson.build | 2 ++\n> > > include/libcamera/meson.build | 1 +\n> > > include/libcamera/platform/meson.build | 9 ++++++++\n> > > src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 2 ++\n> > > src/meson.build | 1 +\n> > > 7 files changed, 47 insertions(+), 1 deletion(-)\n> > > create mode 100644 include/libcamera/platform/meson.build\n> > > create mode 100644 src/libcamera-platform/meson.build\n> > >\n> > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > > index 8305f56af7a8..c1b395bf0b83 100644\n> > > --- a/Documentation/Doxyfile.in\n> > > +++ b/Documentation/Doxyfile.in\n> > > @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> > > INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> > > \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> > > \"@TOP_SRCDIR@/src/libcamera\" \\\n> > > + \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> > > \"@TOP_BUILDDIR@/include/libcamera\" \\\n> > > - \"@TOP_BUILDDIR@/src/libcamera\"\n> > > + \"@TOP_BUILDDIR@/src/libcamera\" \\\n> > > + \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> > >\n> >\n> > Drive-by comment, I've been toying with the idea of splitting the\n> > Doxygen documentation between the public and internal API.\n> >\n> > > # This tag can be used to specify the character encoding of the source files\n> > > # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n> > > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > > index 9ecf4dfcf79f..01b753f07fb6 100644\n> > > --- a/Documentation/meson.build\n> > > +++ b/Documentation/meson.build\n> > > @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> > > libcamera_ipa_interfaces,\n> > > libcamera_public_headers,\n> > > libcamera_sources,\n> > > + libcamera_platform_headers,\n> >\n> > Following up on the comment in the commit message, do we need\n> > libcamera_platform_internal_headers and\n> > libcamera_platform_public_headers ? The distinction between the two\n> > would be different than for libcamera. We currently don't install the\n> > internal headers, while we'll need to install the \"internal platform\"\n> > headers as they can be used by IPA modules. What I'd like to avoid is\n> > giving an ABI stability guarantee for the whole platform library.\n> >\n> > In any case, it's possibly something we can handle later, but as this\n> > series makes quite a few internal headers public in libcamera-platform,\n> > I'm a bit worried we will start using them in the public libcamera API.\n> > Currently there's no risk of doing so by mistake, as the headers are\n> > clearly marked as internal by their location, and we would immediately\n> > spot during review an attempt to move an internal header to the public\n> > directory.\n> >\n> > > + libcamera_platform_sources,\n> > > libipa_headers,\n> > > libipa_sources,\n> > > ],\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index 086c958b0a53..2c3bbeb8df36 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> > >\n> > > subdir('internal')\n> > > subdir('ipa')\n> > > +subdir('platform')\n> > >\n> > > install_headers(libcamera_public_headers,\n> > > subdir : include_dir)\n> > > diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n> > > new file mode 100644\n> > > index 000000000000..c8e0d0c5ba12\n> > > --- /dev/null\n> > > +++ b/include/libcamera/platform/meson.build\n> > > @@ -0,0 +1,9 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> > > +\n> > > +libcamera_platform_headers = files([\n> > > +])\n> > > +\n> > > +install_headers(libcamera_platform_headers,\n> > > + subdir: libcamera_platform_include_dir)\n> > > diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n> > > new file mode 100644\n> > > index 000000000000..64d0dfee2731\n> > > --- /dev/null\n> > > +++ b/src/libcamera-platform/meson.build\n> > > @@ -0,0 +1,29 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +libcamera_platform_sources = files([\n> > > +])\n> > > +\n> > > +libcamera_platform_deps = [\n> > > +]\n> > > +\n> > > +libcamera_platform_lib = shared_library('libcamera_platform',\n> >\n> > $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n> > 664\n> > $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n> > 619\n> >\n> > Nearly a draw :-) I've checked because I would have sworn '-' was way\n> > more common than '_', but it seems it's a personal preference.\n> >\n> > > + [libcamera_platform_sources, libcamera_platform_headers],\n> >\n> > You're missing one space in the indentation.\n> >\n> > > + name_prefix : '',\n> >\n> > Any reason why you can't drop this line and use 'camera_platform' as the\n> > library name ?\n> >\n> > > + install : true,\n> > > + cpp_args : libcamera_cpp_args,\n> > > + include_directories : libcamera_includes,\n> \n> Noob question: I think libcamera-platform can be built stand-alone.\n> Why is libcamera_include needed; I think code in libcamera-platform doesn't\n> include headers built as libcamera.so?\n\nlibcamera_includes is just a reference to the include/ directory of the\nsource tree. The public, internal and platform headers are all located\nthere, that's why it's needed.\n\nIt could be nice to have an entirely different directory for headers, to\navoid accidental dependencies from libcamera-platform to libcamera, but\nI'm not sure what the directory hierachy would be, and whether it could\nbe clean or would need to be horrible :-)\n\n> > > + dependencies : libcamera_platform_deps)\n> > > +\n> > > +libcamera_platform = declare_dependency(sources : [\n> > > + libcamera_platform_headers,\n> >\n> > One space missing here too.\n> >\n> > > + ],\n> > > + include_directories : libcamera_includes,\n> > > + link_with : libcamera_platform_lib)\n> >\n> > Do we actually need this ? Wouldn't it be enough to just link libcamera\n> > (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n> > libcamera_dep is because libcamera generates some headers, which needs\n> > to be done before anything compiling against those headers gets built.\n> > That's not needed for the platform library (at least not yet :-)).\n> >\n> > > +\n> > > +pkg_mod = import('pkgconfig')\n> > > +pkg_mod.generate(libraries : libcamera_platform_lib,\n> > > + version : '1.0',\n> > > + name : 'libcamera-platform',\n> >\n> > One more reason to name the binary libcamera-platform.so ;-)\n> >\n> > > + filebase : 'camera-platform',\n> > > + description : 'Complex Camera Support Library',\n> >\n> > This should be updated.\n> >\n> > > + subdirs : 'libcamera')\n> >\n> > I wonder if we should have a separate pkgconfig file for\n> > libcamera-platform, or include the library in the libcamera pkgconfig.\n> > It's an internal split really, and it wouldn't be nice to force\n> > applications to deal with the libcamera-platform pkgconfig explicitly.\n> >\n> > On the other hand, IPA modules will need this. Maybe we should do both,\n> > and a libcamera-platform.pc for IPA modules, and include\n> > libcamera-platform.so in the libraries of libcamera.pc ?\n> >\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 54512652272c..6ba59e4006cb 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -127,6 +127,7 @@ libcamera_deps = [\n> > > libgnutls,\n> > > liblttng,\n> > > libudev,\n> > > + libcamera_platform,\n> > > dependency('threads'),\n> > > ]\n> > >\n> > > @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> > > libcamera_generated_ipa_headers,\n> > > ],\n> > > include_directories : libcamera_includes,\n> > > + dependencies: libcamera_platform,\n> > > link_with : libcamera)\n> > >\n> > > subdir('proxy/worker')\n> > > diff --git a/src/meson.build b/src/meson.build\n> > > index a4e96ecd728a..70e1a4618a0f 100644\n> > > --- a/src/meson.build\n> > > +++ b/src/meson.build\n> > > @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> > > libcamera_objects = []\n> > >\n> > > # libcamera must be built first as a dependency to the other components.\n> > > +subdir('libcamera-platform')\n> > > subdir('libcamera')\n> > >\n> > > subdir('android')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 56DD5BD78E\n\tfor ;\n\tThu, 17 Jun 2021 08:07:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB85E68946;\n\tThu, 17 Jun 2021 10:07:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 910CB60297\n\tfor ;\n\tThu, 17 Jun 2021 10:07:13 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F04DF58E;\n\tThu, 17 Jun 2021 10:07:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LdyQS3jx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623917233;\n\tbh=CA+id9ODUBitaxbsg2Ob5Ijsm5RKlnRJf+e4E1RZaGc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LdyQS3jx0iI6TE4AGjTK1DBQVFKAraj77oER0JzsV2oa4bzYMq0F3Dux8iS0rJr0/\n\t+E2rPPvDIpm4yqw+kiFChMHiZadxBFiBhG7hPQm3Wo4MQEGslnGVWPvX280/6AKwn2\n\tJ8Uh27WKebpOdGBELXY+JOxJTGjJvZ85vIbfWycE=","Date":"Thu, 17 Jun 2021 11:06:50 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17611,"web_url":"https://patchwork.libcamera.org/comment/17611/","msgid":"","date":"2021-06-17T08:15:38","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Thu, Jun 17, 2021 at 5:07 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Hiro,\n>\n> On Thu, Jun 17, 2021 at 02:15:56PM +0900, Hirokazu Honda wrote:\n> > On Thu, Jun 17, 2021 at 11:21 AM Laurent Pinchart wrote:\n> > > On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n> > > > The libcamera-platform.so will feature internal support functionality\n> > > > that is utilised by libcamera, and can be shared in other places.\n> > >\n> > > I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n> > > \"support\" previously I believe, and I thought that's what you would\n> use,\n> > > but \"platform\" doesn't sound too bad. The only downside I can see is\n> > > that it could be construed as a library aimed at abstracting\n> differences\n> > > between platforms. \"base\" may be another option, although I'm not sure\n> > > to like it (the name comes from Chromium, where it has a similar role\n> as\n> > > the library you're creating here).\n> > >\n> > > > However - the libcamera-platform library does not constitute a part\n> > > > of the public libcamera API directly. It is a layer beneath libcamera\n> > > > which provides common abstractions to internal objects.\n> > >\n> > > That's partly true only I'm afraid, looking at patch 4/6,\n> > > bound_method.h, object.h and signal.h are moved to this library, and\n> > > they're part of the libcamera public API. It's not a problem in itself.\n> > >\n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > > Documentation/Doxyfile.in | 4 +++-\n> > > > Documentation/meson.build | 2 ++\n> > > > include/libcamera/meson.build | 1 +\n> > > > include/libcamera/platform/meson.build | 9 ++++++++\n> > > > src/libcamera-platform/meson.build | 29\n> ++++++++++++++++++++++++++\n> > > > src/libcamera/meson.build | 2 ++\n> > > > src/meson.build | 1 +\n> > > > 7 files changed, 47 insertions(+), 1 deletion(-)\n> > > > create mode 100644 include/libcamera/platform/meson.build\n> > > > create mode 100644 src/libcamera-platform/meson.build\n> > > >\n> > > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > > > index 8305f56af7a8..c1b395bf0b83 100644\n> > > > --- a/Documentation/Doxyfile.in\n> > > > +++ b/Documentation/Doxyfile.in\n> > > > @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> > > > INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> > > > \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> > > > \"@TOP_SRCDIR@/src/libcamera\" \\\n> > > > + \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> > > > \"@TOP_BUILDDIR@/include/libcamera\" \\\n> > > > - \"@TOP_BUILDDIR@/src/libcamera\"\n> > > > + \"@TOP_BUILDDIR@/src/libcamera\" \\\n> > > > + \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> > > >\n> > >\n> > > Drive-by comment, I've been toying with the idea of splitting the\n> > > Doxygen documentation between the public and internal API.\n> > >\n> > > > # This tag can be used to specify the character encoding of the\n> source files\n> > > > # that doxygen parses. Internally doxygen uses the UTF-8 encoding.\n> Doxygen uses\n> > > > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > > > index 9ecf4dfcf79f..01b753f07fb6 100644\n> > > > --- a/Documentation/meson.build\n> > > > +++ b/Documentation/meson.build\n> > > > @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> > > > libcamera_ipa_interfaces,\n> > > > libcamera_public_headers,\n> > > > libcamera_sources,\n> > > > + libcamera_platform_headers,\n> > >\n> > > Following up on the comment in the commit message, do we need\n> > > libcamera_platform_internal_headers and\n> > > libcamera_platform_public_headers ? The distinction between the two\n> > > would be different than for libcamera. We currently don't install the\n> > > internal headers, while we'll need to install the \"internal platform\"\n> > > headers as they can be used by IPA modules. What I'd like to avoid is\n> > > giving an ABI stability guarantee for the whole platform library.\n> > >\n> > > In any case, it's possibly something we can handle later, but as this\n> > > series makes quite a few internal headers public in libcamera-platform,\n> > > I'm a bit worried we will start using them in the public libcamera API.\n> > > Currently there's no risk of doing so by mistake, as the headers are\n> > > clearly marked as internal by their location, and we would immediately\n> > > spot during review an attempt to move an internal header to the public\n> > > directory.\n> > >\n> > > > + libcamera_platform_sources,\n> > > > libipa_headers,\n> > > > libipa_sources,\n> > > > ],\n> > > > diff --git a/include/libcamera/meson.build\n> b/include/libcamera/meson.build\n> > > > index 086c958b0a53..2c3bbeb8df36 100644\n> > > > --- a/include/libcamera/meson.build\n> > > > +++ b/include/libcamera/meson.build\n> > > > @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> > > >\n> > > > subdir('internal')\n> > > > subdir('ipa')\n> > > > +subdir('platform')\n> > > >\n> > > > install_headers(libcamera_public_headers,\n> > > > subdir : include_dir)\n> > > > diff --git a/include/libcamera/platform/meson.build\n> b/include/libcamera/platform/meson.build\n> > > > new file mode 100644\n> > > > index 000000000000..c8e0d0c5ba12\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/platform/meson.build\n> > > > @@ -0,0 +1,9 @@\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > +\n> > > > +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> > > > +\n> > > > +libcamera_platform_headers = files([\n> > > > +])\n> > > > +\n> > > > +install_headers(libcamera_platform_headers,\n> > > > + subdir: libcamera_platform_include_dir)\n> > > > diff --git a/src/libcamera-platform/meson.build\n> b/src/libcamera-platform/meson.build\n> > > > new file mode 100644\n> > > > index 000000000000..64d0dfee2731\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera-platform/meson.build\n> > > > @@ -0,0 +1,29 @@\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > +\n> > > > +libcamera_platform_sources = files([\n> > > > +])\n> > > > +\n> > > > +libcamera_platform_deps = [\n> > > > +]\n> > > > +\n> > > > +libcamera_platform_lib = shared_library('libcamera_platform',\n> > >\n> > > $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n> > > 664\n> > > $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n> > > 619\n> > >\n> > > Nearly a draw :-) I've checked because I would have sworn '-' was way\n> > > more common than '_', but it seems it's a personal preference.\n> > >\n> > > > + [libcamera_platform_sources,\n> libcamera_platform_headers],\n> > >\n> > > You're missing one space in the indentation.\n> > >\n> > > > + name_prefix : '',\n> > >\n> > > Any reason why you can't drop this line and use 'camera_platform' as\n> the\n> > > library name ?\n> > >\n> > > > + install : true,\n> > > > + cpp_args :\n> libcamera_cpp_args,\n> > > > + include_directories :\n> libcamera_includes,\n> >\n> > Noob question: I think libcamera-platform can be built stand-alone.\n> > Why is libcamera_include needed; I think code in libcamera-platform\n> doesn't\n> > include headers built as libcamera.so?\n>\n> libcamera_includes is just a reference to the include/ directory of the\n> source tree. The public, internal and platform headers are all located\n> there, that's why it's needed.\n>\n> It could be nice to have an entirely different directory for headers, to\n> avoid accidental dependencies from libcamera-platform to libcamera, but\n> I'm not sure what the directory hierachy would be, and whether it could\n> be clean or would need to be horrible :-)\n>\n>\nAh, it specifies the include path. Thanks I got it. I am still newbie of\nthe meson build config. :p\n-Hiro\n\n\n> > > > + dependencies :\n> libcamera_platform_deps)\n> > > > +\n> > > > +libcamera_platform = declare_dependency(sources : [\n> > > > +\n> libcamera_platform_headers,\n> > >\n> > > One space missing here too.\n> > >\n> > > > + ],\n> > > > + include_directories :\n> libcamera_includes,\n> > > > + link_with :\n> libcamera_platform_lib)\n> > >\n> > > Do we actually need this ? Wouldn't it be enough to just link libcamera\n> > > (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n> > > libcamera_dep is because libcamera generates some headers, which needs\n> > > to be done before anything compiling against those headers gets built.\n> > > That's not needed for the platform library (at least not yet :-)).\n> > >\n> > > > +\n> > > > +pkg_mod = import('pkgconfig')\n> > > > +pkg_mod.generate(libraries : libcamera_platform_lib,\n> > > > + version : '1.0',\n> > > > + name : 'libcamera-platform',\n> > >\n> > > One more reason to name the binary libcamera-platform.so ;-)\n> > >\n> > > > + filebase : 'camera-platform',\n> > > > + description : 'Complex Camera Support Library',\n> > >\n> > > This should be updated.\n> > >\n> > > > + subdirs : 'libcamera')\n> > >\n> > > I wonder if we should have a separate pkgconfig file for\n> > > libcamera-platform, or include the library in the libcamera pkgconfig.\n> > > It's an internal split really, and it wouldn't be nice to force\n> > > applications to deal with the libcamera-platform pkgconfig explicitly.\n> > >\n> > > On the other hand, IPA modules will need this. Maybe we should do both,\n> > > and a libcamera-platform.pc for IPA modules, and include\n> > > libcamera-platform.so in the libraries of libcamera.pc ?\n> > >\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index 54512652272c..6ba59e4006cb 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -127,6 +127,7 @@ libcamera_deps = [\n> > > > libgnutls,\n> > > > liblttng,\n> > > > libudev,\n> > > > + libcamera_platform,\n> > > > dependency('threads'),\n> > > > ]\n> > > >\n> > > > @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> > > >\n> libcamera_generated_ipa_headers,\n> > > > ],\n> > > > include_directories :\n> libcamera_includes,\n> > > > + dependencies: libcamera_platform,\n> > > > link_with : libcamera)\n> > > >\n> > > > subdir('proxy/worker')\n> > > > diff --git a/src/meson.build b/src/meson.build\n> > > > index a4e96ecd728a..70e1a4618a0f 100644\n> > > > --- a/src/meson.build\n> > > > +++ b/src/meson.build\n> > > > @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> > > > libcamera_objects = []\n> > > >\n> > > > # libcamera must be built first as a dependency to the other\n> components.\n> > > > +subdir('libcamera-platform')\n> > > > subdir('libcamera')\n> > > >\n> > > > subdir('android')\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CEDE7C3218\n\tfor ;\n\tThu, 17 Jun 2021 08:15:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BD7368946;\n\tThu, 17 Jun 2021 10:15:52 +0200 (CEST)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D015B60297\n\tfor ;\n\tThu, 17 Jun 2021 10:15:50 +0200 (CEST)","by mail-ej1-x62c.google.com with SMTP id ho18so8357735ejc.8\n\tfor ;\n\tThu, 17 Jun 2021 01:15:50 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"dTVT58sL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=pb0FpsX2RYeu+5Ws78WDntu4VxP0b4D6B35T2uXNsy0=;\n\tb=dTVT58sLPBIk+6K2RRhZgTtnyc1vaEzuawTMjr1LIsOK5D/bAGz15Nam1/qHbe/YI5\n\t15NesT6n28WOe8/HH+d2eqSGqpFYzxUovVOWMMGZkSweVQrhkoGJdKsAeTjIEZPR7AxU\n\tNpPihEtQI3X/67n2TWq9sK0hv3cd6M9Ldrrvw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=pb0FpsX2RYeu+5Ws78WDntu4VxP0b4D6B35T2uXNsy0=;\n\tb=GNlXM4IQ6/5M+Muc2rA2c+3akoGWhE4UnXY3bJNpmG6EOOQtOWTwZ7eqXiZt7OgxNs\n\tr/ZelDEDzAQgNi55yQ3YXlTEVBNwja5aZRd12/epRzi9bIfun4wv2jXBUk9O0cLL3OyB\n\t9G3yI5QjG2sahhznddRFS9it5AK6E2bxZ1Txr7mm4jKzxqvluAMsu2AhcSVxZvt558fO\n\tti5ADKlfPBQwKG99Tpr2T+koPgaOi+p8xN0xwm/UzaZB4ibTBrNT0ak8ZoTfhvGGoX/q\n\tUHKBK2NN0sI0qGlrrTvpJPZhNGxJ3j1/Kud6QnnxwDNS6rHmE1OPfMfMobuLSOEWUxaV\n\tiQ3Q==","X-Gm-Message-State":"AOAM5315OiSdjsoV41IFJjPpfIw1HTxclnTB6nSKhj98qJsZ/sstj2iI\n\tZTHSieeBgLkDZDIs5UdOPmRosKHADQt4cCeHQ+HLaaWqNvdOOg==","X-Google-Smtp-Source":"ABdhPJzxWjtdO1iUxeIA0lZze8l50hW3Sq8ZI7UT26mXRK9N8udIrfeAj5zp5w6j5XYMA15EsSSPeCJAX+7KzaolG9A=","X-Received":"by 2002:a17:907:1009:: with SMTP id\n\tox9mr4032022ejb.292.1623917750331; \n\tThu, 17 Jun 2021 01:15:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 17 Jun 2021 17:15:38 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000a73e3a05c4f1d0f9\"","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17612,"web_url":"https://patchwork.libcamera.org/comment/17612/","msgid":"<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>","date":"2021-06-17T09:10:10","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 17/06/2021 03:20, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n>> The libcamera-platform.so will feature internal support functionality\n>> that is utilised by libcamera, and can be shared in other places.\n> \n> I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n> \"support\" previously I believe, and I thought that's what you would use,\n> but \"platform\" doesn't sound too bad. The only downside I can see is\n\nsupport was really grating on me, in particular when I realised there\nare likely to be libraries beneath libcamera, and libraries 'above'.\n\nPlatform to me feels more like the 'base' ... (because you can stand on\na platform).\n\n\n> that it could be construed as a library aimed at abstracting differences\n> between platforms. \"base\" may be another option, although I'm not sure\n> to like it (the name comes from Chromium, where it has a similar role as\n> the library you're creating here).\n\nAs mentioned in the cover letter, it could be that in the future indeed\nwe might need some platform abstraction. So that's again why I preferred\nplatform from the beginning.\n\n\n> \n>> However - the libcamera-platform library does not constitute a part\n>> of the public libcamera API directly. It is a layer beneath libcamera\n>> which provides common abstractions to internal objects.\n> \n> That's partly true only I'm afraid, looking at patch 4/6,\n> bound_method.h, object.h and signal.h are moved to this library, and\n> they're part of the libcamera public API. It's not a problem in itself.\n\n\nArgh - yes - that's annoying - and I had somewhat mis-considered this.\n\nThis commit was written before I ended up pulling in libcamera public\nheaders, which I only hit deeper down the rabbit-hole ...\n\n\n\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> Documentation/Doxyfile.in | 4 +++-\n>> Documentation/meson.build | 2 ++\n>> include/libcamera/meson.build | 1 +\n>> include/libcamera/platform/meson.build | 9 ++++++++\n>> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n>> src/libcamera/meson.build | 2 ++\n>> src/meson.build | 1 +\n>> 7 files changed, 47 insertions(+), 1 deletion(-)\n>> create mode 100644 include/libcamera/platform/meson.build\n>> create mode 100644 src/libcamera-platform/meson.build\n>>\n>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>> index 8305f56af7a8..c1b395bf0b83 100644\n>> --- a/Documentation/Doxyfile.in\n>> +++ b/Documentation/Doxyfile.in\n>> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n>> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n>> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n>> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n>> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n>> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n>> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n>> \n> \n> Drive-by comment, I've been toying with the idea of splitting the\n> Doxygen documentation between the public and internal API.\n\nI think that's likely a good idea.\n\nHopefully we can get some more time to work on documentation\nspecifically as I think we need some more introduction type pages and\nsomething to actually guide through the doxygen generated pages.\n\n> \n>> # This tag can be used to specify the character encoding of the source files\n>> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>> index 9ecf4dfcf79f..01b753f07fb6 100644\n>> --- a/Documentation/meson.build\n>> +++ b/Documentation/meson.build\n>> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n>> libcamera_ipa_interfaces,\n>> libcamera_public_headers,\n>> libcamera_sources,\n>> + libcamera_platform_headers,\n> \n> Following up on the comment in the commit message, do we need\n> libcamera_platform_internal_headers and\n> libcamera_platform_public_headers ? The distinction between the two\n> would be different than for libcamera. We currently don't install the\n> internal headers, while we'll need to install the \"internal platform\"\n> headers as they can be used by IPA modules. What I'd like to avoid is\n> giving an ABI stability guarantee for the whole platform library.\n\n\nI'm concerned that would be a mistake.\nHow can you have public-public headers and public-private-headers?\n\nAll of the headers in this library are publicly exposed.\n\nI want to be able to say I think the risk of having ABI breakage on this\nlibrary component is lower though - because these are mostly just helper\nobjects, but I know if I were to actually say that, then we'd change the\nABI on say File or ... something ...\n\nI guess that begs the question of how will libcamera-platform be\nversioned. I think as long as it's in the same repo as libcamera itself\n- it would share the same versions, and I now have the\nabi-compliance-checker which will identify if we do cause an ABI breakage.\n\n(I hope to see the ABI compliance checker as a pre-integration\nvalidation stage).\n\n\n> In any case, it's possibly something we can handle later, but as this\n> series makes quite a few internal headers public in libcamera-platform,\n> I'm a bit worried we will start using them in the public libcamera API.\n> Currently there's no risk of doing so by mistake, as the headers are\n> clearly marked as internal by their location, and we would immediately\n> spot during review an attempt to move an internal header to the public\n> directory.\n\nYes, having 'you can use this header' ... ' you can not use this header'\nwould become a bit awkward ...\n\n\n\n\n>> + libcamera_platform_sources,\n>> libipa_headers,\n>> libipa_sources,\n>> ],\n>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>> index 086c958b0a53..2c3bbeb8df36 100644\n>> --- a/include/libcamera/meson.build\n>> +++ b/include/libcamera/meson.build\n>> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n>> \n>> subdir('internal')\n>> subdir('ipa')\n>> +subdir('platform')\n>> \n>> install_headers(libcamera_public_headers,\n>> subdir : include_dir)\n>> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n>> new file mode 100644\n>> index 000000000000..c8e0d0c5ba12\n>> --- /dev/null\n>> +++ b/include/libcamera/platform/meson.build\n>> @@ -0,0 +1,9 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n>> +\n>> +libcamera_platform_headers = files([\n>> +])\n>> +\n>> +install_headers(libcamera_platform_headers,\n>> + subdir: libcamera_platform_include_dir)\n>> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n>> new file mode 100644\n>> index 000000000000..64d0dfee2731\n>> --- /dev/null\n>> +++ b/src/libcamera-platform/meson.build\n>> @@ -0,0 +1,29 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +libcamera_platform_sources = files([\n>> +])\n>> +\n>> +libcamera_platform_deps = [\n>> +]\n>> +\n>> +libcamera_platform_lib = shared_library('libcamera_platform',\n> \n> $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n> 664\n> $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n> 619\n> \n> Nearly a draw :-) I've checked because I would have sworn '-' was way\n> more common than '_', but it seems it's a personal preference.\n\nThis should be '-', I suspect the _ was a copy paste from the meson\nvariable name ;-(\n\n\n> \n>> + [libcamera_platform_sources, libcamera_platform_headers],\n> \n> You're missing one space in the indentation.\n\nack.\n\n\n> \n>> + name_prefix : '',\n> \n> Any reason why you can't drop this line and use 'camera_platform' as the\n> library name ?\n\nTo be consistent with our libcamera meson file?\n\n\n> \n>> + install : true,\n>> + cpp_args : libcamera_cpp_args,\n>> + include_directories : libcamera_includes,\n>> + dependencies : libcamera_platform_deps)\n>> +\n>> +libcamera_platform = declare_dependency(sources : [\n>> + libcamera_platform_headers,\n> \n> One space missing here too.\n> \n>> + ],\n>> + include_directories : libcamera_includes,\n>> + link_with : libcamera_platform_lib)\n> \n> Do we actually need this ? Wouldn't it be enough to just link libcamera\n> (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n> libcamera_dep is because libcamera generates some headers, which needs\n> to be done before anything compiling against those headers gets built.\n> That's not needed for the platform library (at least not yet :-)).\n> \n>> +\n>> +pkg_mod = import('pkgconfig')\n>> +pkg_mod.generate(libraries : libcamera_platform_lib,\n>> + version : '1.0',\n>> + name : 'libcamera-platform',\n> \n> One more reason to name the binary libcamera-platform.so ;-)\n\nYes, that was my intended name.\n\n\n> \n>> + filebase : 'camera-platform',\n>> + description : 'Complex Camera Support Library',\n> \n> This should be updated.\n> \n>> + subdirs : 'libcamera')\n> \n> I wonder if we should have a separate pkgconfig file for\n> libcamera-platform, or include the library in the libcamera pkgconfig.\n> It's an internal split really, and it wouldn't be nice to force\n> applications to deal with the libcamera-platform pkgconfig explicitly.\n> \n> On the other hand, IPA modules will need this. Maybe we should do both,\n\nYes - the point is that IPA's want to explicitly pull this in.\n\n\n> and a libcamera-platform.pc for IPA modules, and include\n> libcamera-platform.so in the libraries of libcamera.pc ?\n> \n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 54512652272c..6ba59e4006cb 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -127,6 +127,7 @@ libcamera_deps = [\n>> libgnutls,\n>> liblttng,\n>> libudev,\n>> + libcamera_platform,\n>> dependency('threads'),\n>> ]\n>> \n>> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n>> libcamera_generated_ipa_headers,\n>> ],\n>> include_directories : libcamera_includes,\n>> + dependencies: libcamera_platform,\n>> link_with : libcamera)\n>> \n>> subdir('proxy/worker')\n>> diff --git a/src/meson.build b/src/meson.build\n>> index a4e96ecd728a..70e1a4618a0f 100644\n>> --- a/src/meson.build\n>> +++ b/src/meson.build\n>> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n>> libcamera_objects = []\n>> \n>> # libcamera must be built first as a dependency to the other components.\n>> +subdir('libcamera-platform')\n>> subdir('libcamera')\n>> \n>> subdir('android')\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5E647BD78E\n\tfor ;\n\tThu, 17 Jun 2021 09:10:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AAC3F60298;\n\tThu, 17 Jun 2021 11:10:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D3E260297\n\tfor ;\n\tThu, 17 Jun 2021 11:10:13 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18F4BE7B;\n\tThu, 17 Jun 2021 11:10:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SlSH679B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623921013;\n\tbh=yWXrIxAcGYk6Auc0jT0U1y1FgWp55+PN8rsLgVZmVO0=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=SlSH679BkpbkgWxYyGVTryUa9PUfyFEhdZ68SHTQpyoJncgYMwqGHuipbIwRA/iqt\n\tD34EpP2Bm1FhO0bwVKYRGch3zOBWCsdxygW8xsUA52bQuZgOB+han73g+h5+TzLNPf\n\tZSiWLANm4bGcivzYHXItb+JvX3cWOzqB/lCoOjm0=","To":"Laurent Pinchart ","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>","Date":"Thu, 17 Jun 2021 10:10:10 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17615,"web_url":"https://patchwork.libcamera.org/comment/17615/","msgid":"","date":"2021-06-17T09:33:27","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jun 17, 2021 at 10:10:10AM +0100, Kieran Bingham wrote:\n> On 17/06/2021 03:20, Laurent Pinchart wrote:\n> > On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n> >> The libcamera-platform.so will feature internal support functionality\n> >> that is utilised by libcamera, and can be shared in other places.\n> > \n> > I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n> > \"support\" previously I believe, and I thought that's what you would use,\n> > but \"platform\" doesn't sound too bad. The only downside I can see is\n> \n> support was really grating on me, in particular when I realised there\n> are likely to be libraries beneath libcamera, and libraries 'above'.\n> \n> Platform to me feels more like the 'base' ... (because you can stand on\n> a platform).\n> \n> > that it could be construed as a library aimed at abstracting differences\n> > between platforms. \"base\" may be another option, although I'm not sure\n> > to like it (the name comes from Chromium, where it has a similar role as\n> > the library you're creating here).\n> \n> As mentioned in the cover letter, it could be that in the future indeed\n> we might need some platform abstraction. So that's again why I preferred\n> platform from the beginning.\n\nThere would likely be a big overlap between this library and the\nplatform abstraction, but I'm a bit concerned that some platform\nabstraction may need to go elsewhere. I see the goal for this library to\nbe more of a foundation than a platform abstraction.\n\nMaybe libcamera-foundation is a possible name :-)\n\n> >> However - the libcamera-platform library does not constitute a part\n> >> of the public libcamera API directly. It is a layer beneath libcamera\n> >> which provides common abstractions to internal objects.\n> > \n> > That's partly true only I'm afraid, looking at patch 4/6,\n> > bound_method.h, object.h and signal.h are moved to this library, and\n> > they're part of the libcamera public API. It's not a problem in itself.\n> \n> Argh - yes - that's annoying - and I had somewhat mis-considered this.\n> \n> This commit was written before I ended up pulling in libcamera public\n> headers, which I only hit deeper down the rabbit-hole ...\n> \n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> Documentation/Doxyfile.in | 4 +++-\n> >> Documentation/meson.build | 2 ++\n> >> include/libcamera/meson.build | 1 +\n> >> include/libcamera/platform/meson.build | 9 ++++++++\n> >> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n> >> src/libcamera/meson.build | 2 ++\n> >> src/meson.build | 1 +\n> >> 7 files changed, 47 insertions(+), 1 deletion(-)\n> >> create mode 100644 include/libcamera/platform/meson.build\n> >> create mode 100644 src/libcamera-platform/meson.build\n> >>\n> >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> >> index 8305f56af7a8..c1b395bf0b83 100644\n> >> --- a/Documentation/Doxyfile.in\n> >> +++ b/Documentation/Doxyfile.in\n> >> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> >> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> >> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> >> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n> >> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> >> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n> >> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n> >> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n> >> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> >> \n> > \n> > Drive-by comment, I've been toying with the idea of splitting the\n> > Doxygen documentation between the public and internal API.\n> \n> I think that's likely a good idea.\n> \n> Hopefully we can get some more time to work on documentation\n> specifically as I think we need some more introduction type pages and\n> something to actually guide through the doxygen generated pages.\n\nAgreed.\n\n> >> # This tag can be used to specify the character encoding of the source files\n> >> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n> >> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> >> index 9ecf4dfcf79f..01b753f07fb6 100644\n> >> --- a/Documentation/meson.build\n> >> +++ b/Documentation/meson.build\n> >> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> >> libcamera_ipa_interfaces,\n> >> libcamera_public_headers,\n> >> libcamera_sources,\n> >> + libcamera_platform_headers,\n> > \n> > Following up on the comment in the commit message, do we need\n> > libcamera_platform_internal_headers and\n> > libcamera_platform_public_headers ? The distinction between the two\n> > would be different than for libcamera. We currently don't install the\n> > internal headers, while we'll need to install the \"internal platform\"\n> > headers as they can be used by IPA modules. What I'd like to avoid is\n> > giving an ABI stability guarantee for the whole platform library.\n> \n> I'm concerned that would be a mistake.\n> How can you have public-public headers and public-private-headers?\n> \n> All of the headers in this library are publicly exposed.\n> \n> I want to be able to say I think the risk of having ABI breakage on this\n> library component is lower though - because these are mostly just helper\n> objects, but I know if I were to actually say that, then we'd change the\n> ABI on say File or ... something ...\n> \n> I guess that begs the question of how will libcamera-platform be\n> versioned. I think as long as it's in the same repo as libcamera itself\n> - it would share the same versions, and I now have the\n> abi-compliance-checker which will identify if we do cause an ABI breakage.\n> \n> (I hope to see the ABI compliance checker as a pre-integration\n> validation stage).\n\nI think it should be versioned exactly as libcamera. That part doesn't\nworry me. What bothers me is that the platform headers will contain both\nstable and non-stable APIs (and ABIs), which will not be easy to convey.\nApplications will use signal.h, but we don't want them to use thread.h.\nEven if we document that they shouldn't, some will, and there will be\nbreakages.\n\n> > In any case, it's possibly something we can handle later, but as this\n> > series makes quite a few internal headers public in libcamera-platform,\n> > I'm a bit worried we will start using them in the public libcamera API.\n> > Currently there's no risk of doing so by mistake, as the headers are\n> > clearly marked as internal by their location, and we would immediately\n> > spot during review an attempt to move an internal header to the public\n> > directory.\n> \n> Yes, having 'you can use this header' ... ' you can not use this header'\n> would become a bit awkward ...\n\nRandom idea, how about adding a\n\n#ifndef LIBCAMERA_PLATFORM_PRIVATE\n#error ...\n#endif\n\nat the beginning of all private headers ? Both libcamera and IPA modules\nwould define LIBCAMERA_PLATFORM_PRIVATE. cam and qcam wouldn't. We can\nbikeshed the LIBCAMERA_PLATFORM_PRIVATE macro name of course :-)\n\nThis may not be the long term solution we want, but it would already\navoid mistakes going unnoticed, and it's a simple change.\n\n> >> + libcamera_platform_sources,\n> >> libipa_headers,\n> >> libipa_sources,\n> >> ],\n> >> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> >> index 086c958b0a53..2c3bbeb8df36 100644\n> >> --- a/include/libcamera/meson.build\n> >> +++ b/include/libcamera/meson.build\n> >> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> >> \n> >> subdir('internal')\n> >> subdir('ipa')\n> >> +subdir('platform')\n> >> \n> >> install_headers(libcamera_public_headers,\n> >> subdir : include_dir)\n> >> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n> >> new file mode 100644\n> >> index 000000000000..c8e0d0c5ba12\n> >> --- /dev/null\n> >> +++ b/include/libcamera/platform/meson.build\n> >> @@ -0,0 +1,9 @@\n> >> +# SPDX-License-Identifier: CC0-1.0\n> >> +\n> >> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> >> +\n> >> +libcamera_platform_headers = files([\n> >> +])\n> >> +\n> >> +install_headers(libcamera_platform_headers,\n> >> + subdir: libcamera_platform_include_dir)\n> >> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n> >> new file mode 100644\n> >> index 000000000000..64d0dfee2731\n> >> --- /dev/null\n> >> +++ b/src/libcamera-platform/meson.build\n> >> @@ -0,0 +1,29 @@\n> >> +# SPDX-License-Identifier: CC0-1.0\n> >> +\n> >> +libcamera_platform_sources = files([\n> >> +])\n> >> +\n> >> +libcamera_platform_deps = [\n> >> +]\n> >> +\n> >> +libcamera_platform_lib = shared_library('libcamera_platform',\n> > \n> > $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n> > 664\n> > $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n> > 619\n> > \n> > Nearly a draw :-) I've checked because I would have sworn '-' was way\n> > more common than '_', but it seems it's a personal preference.\n> \n> This should be '-', I suspect the _ was a copy paste from the meson\n> variable name ;-(\n> \n> >> + [libcamera_platform_sources, libcamera_platform_headers],\n> > \n> > You're missing one space in the indentation.\n> \n> ack.\n> \n> >> + name_prefix : '',\n> > \n> > Any reason why you can't drop this line and use 'camera_platform' as the\n> > library name ?\n> \n> To be consistent with our libcamera meson file?\n\nWe have\n\nlibcamera = shared_library('camera',\n...\n\nwith no name_prefix.\n\n> >> + install : true,\n> >> + cpp_args : libcamera_cpp_args,\n> >> + include_directories : libcamera_includes,\n> >> + dependencies : libcamera_platform_deps)\n> >> +\n> >> +libcamera_platform = declare_dependency(sources : [\n> >> + libcamera_platform_headers,\n> > \n> > One space missing here too.\n> > \n> >> + ],\n> >> + include_directories : libcamera_includes,\n> >> + link_with : libcamera_platform_lib)\n> > \n> > Do we actually need this ? Wouldn't it be enough to just link libcamera\n> > (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n> > libcamera_dep is because libcamera generates some headers, which needs\n> > to be done before anything compiling against those headers gets built.\n> > That's not needed for the platform library (at least not yet :-)).\n> > \n> >> +\n> >> +pkg_mod = import('pkgconfig')\n> >> +pkg_mod.generate(libraries : libcamera_platform_lib,\n> >> + version : '1.0',\n> >> + name : 'libcamera-platform',\n> > \n> > One more reason to name the binary libcamera-platform.so ;-)\n> \n> Yes, that was my intended name.\n> \n> >> + filebase : 'camera-platform',\n> >> + description : 'Complex Camera Support Library',\n> > \n> > This should be updated.\n> > \n> >> + subdirs : 'libcamera')\n> > \n> > I wonder if we should have a separate pkgconfig file for\n> > libcamera-platform, or include the library in the libcamera pkgconfig.\n> > It's an internal split really, and it wouldn't be nice to force\n> > applications to deal with the libcamera-platform pkgconfig explicitly.\n> > \n> > On the other hand, IPA modules will need this. Maybe we should do both,\n> \n> Yes - the point is that IPA's want to explicitly pull this in.\n\nWould it then make sense to have libcamera-platform in both the\nlibcamera and libcamera-platform pkgconfig ?\n\n> > and a libcamera-platform.pc for IPA modules, and include\n> > libcamera-platform.so in the libraries of libcamera.pc ?\n> > \n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index 54512652272c..6ba59e4006cb 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -127,6 +127,7 @@ libcamera_deps = [\n> >> libgnutls,\n> >> liblttng,\n> >> libudev,\n> >> + libcamera_platform,\n> >> dependency('threads'),\n> >> ]\n> >> \n> >> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> >> libcamera_generated_ipa_headers,\n> >> ],\n> >> include_directories : libcamera_includes,\n> >> + dependencies: libcamera_platform,\n> >> link_with : libcamera)\n> >> \n> >> subdir('proxy/worker')\n> >> diff --git a/src/meson.build b/src/meson.build\n> >> index a4e96ecd728a..70e1a4618a0f 100644\n> >> --- a/src/meson.build\n> >> +++ b/src/meson.build\n> >> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> >> libcamera_objects = []\n> >> \n> >> # libcamera must be built first as a dependency to the other components.\n> >> +subdir('libcamera-platform')\n> >> subdir('libcamera')\n> >> \n> >> subdir('android')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 02AB4BD78E\n\tfor ;\n\tThu, 17 Jun 2021 09:33:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BB1468940;\n\tThu, 17 Jun 2021 11:33:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 481B660297\n\tfor ;\n\tThu, 17 Jun 2021 11:33:49 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF05FE7B;\n\tThu, 17 Jun 2021 11:33:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KP+C+ifP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623922428;\n\tbh=nCzgu1LdvaKEHALHahnk4ZhGuTgV+Qf+tewBBDuQoLY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KP+C+ifP8qriNpla3RfKK5xqu0iAxcWP38DjZjqUvsqn4HJF3W1yp8oEDDs11uxRG\n\tnwNZX9/nRp/ux7w4ZhRX4QOb7wU3bAdQYCI3Zig1UXDqC/tYjyIQXYhrdBe+QQi56b\n\tnQAE0cMpyzHsNxcTFDHdj9P+gs/IpwHUpMtrErMo=","Date":"Thu, 17 Jun 2021 12:33:27 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t\n\t<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17621,"web_url":"https://patchwork.libcamera.org/comment/17621/","msgid":"<2b385b30-3fe6-353f-3712-eadea9684fca@ideasonboard.com>","date":"2021-06-18T09:00:22","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 17/06/2021 10:33, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Thu, Jun 17, 2021 at 10:10:10AM +0100, Kieran Bingham wrote:\n>> On 17/06/2021 03:20, Laurent Pinchart wrote:\n>>> On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n>>>> The libcamera-platform.so will feature internal support functionality\n>>>> that is utilised by libcamera, and can be shared in other places.\n>>>\n>>> I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n>>> \"support\" previously I believe, and I thought that's what you would use,\n>>> but \"platform\" doesn't sound too bad. The only downside I can see is\n>>\n>> support was really grating on me, in particular when I realised there\n>> are likely to be libraries beneath libcamera, and libraries 'above'.\n>>\n>> Platform to me feels more like the 'base' ... (because you can stand on\n>> a platform).\n>>\n>>> that it could be construed as a library aimed at abstracting differences\n>>> between platforms. \"base\" may be another option, although I'm not sure\n>>> to like it (the name comes from Chromium, where it has a similar role as\n>>> the library you're creating here).\n>>\n>> As mentioned in the cover letter, it could be that in the future indeed\n>> we might need some platform abstraction. So that's again why I preferred\n>> platform from the beginning.\n> \n> There would likely be a big overlap between this library and the\n> platform abstraction, but I'm a bit concerned that some platform\n> abstraction may need to go elsewhere. I see the goal for this library to\n> be more of a foundation than a platform abstraction.\n> \n> Maybe libcamera-foundation is a possible name :-)\n\nHrm ... that sounds like something else entirely ...\n\n\n>>>> However - the libcamera-platform library does not constitute a part\n>>>> of the public libcamera API directly. It is a layer beneath libcamera\n>>>> which provides common abstractions to internal objects.\n>>>\n>>> That's partly true only I'm afraid, looking at patch 4/6,\n>>> bound_method.h, object.h and signal.h are moved to this library, and\n>>> they're part of the libcamera public API. It's not a problem in itself.\n>>\n>> Argh - yes - that's annoying - and I had somewhat mis-considered this.\n>>\n>> This commit was written before I ended up pulling in libcamera public\n>> headers, which I only hit deeper down the rabbit-hole ...\n>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> Documentation/Doxyfile.in | 4 +++-\n>>>> Documentation/meson.build | 2 ++\n>>>> include/libcamera/meson.build | 1 +\n>>>> include/libcamera/platform/meson.build | 9 ++++++++\n>>>> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n>>>> src/libcamera/meson.build | 2 ++\n>>>> src/meson.build | 1 +\n>>>> 7 files changed, 47 insertions(+), 1 deletion(-)\n>>>> create mode 100644 include/libcamera/platform/meson.build\n>>>> create mode 100644 src/libcamera-platform/meson.build\n>>>>\n>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>>>> index 8305f56af7a8..c1b395bf0b83 100644\n>>>> --- a/Documentation/Doxyfile.in\n>>>> +++ b/Documentation/Doxyfile.in\n>>>> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n>>>> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n>>>> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n>>>> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n>>>> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n>>>> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n>>>> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n>>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n>>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n>>>> \n>>>\n>>> Drive-by comment, I've been toying with the idea of splitting the\n>>> Doxygen documentation between the public and internal API.\n>>\n>> I think that's likely a good idea.\n>>\n>> Hopefully we can get some more time to work on documentation\n>> specifically as I think we need some more introduction type pages and\n>> something to actually guide through the doxygen generated pages.\n> \n> Agreed.\n> \n>>>> # This tag can be used to specify the character encoding of the source files\n>>>> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>>>> index 9ecf4dfcf79f..01b753f07fb6 100644\n>>>> --- a/Documentation/meson.build\n>>>> +++ b/Documentation/meson.build\n>>>> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n>>>> libcamera_ipa_interfaces,\n>>>> libcamera_public_headers,\n>>>> libcamera_sources,\n>>>> + libcamera_platform_headers,\n>>>\n>>> Following up on the comment in the commit message, do we need\n>>> libcamera_platform_internal_headers and\n>>> libcamera_platform_public_headers ? The distinction between the two\n>>> would be different than for libcamera. We currently don't install the\n>>> internal headers, while we'll need to install the \"internal platform\"\n>>> headers as they can be used by IPA modules. What I'd like to avoid is\n>>> giving an ABI stability guarantee for the whole platform library.\n>>\n>> I'm concerned that would be a mistake.\n>> How can you have public-public headers and public-private-headers?\n>>\n>> All of the headers in this library are publicly exposed.\n>>\n>> I want to be able to say I think the risk of having ABI breakage on this\n>> library component is lower though - because these are mostly just helper\n>> objects, but I know if I were to actually say that, then we'd change the\n>> ABI on say File or ... something ...\n>>\n>> I guess that begs the question of how will libcamera-platform be\n>> versioned. I think as long as it's in the same repo as libcamera itself\n>> - it would share the same versions, and I now have the\n>> abi-compliance-checker which will identify if we do cause an ABI breakage.\n>>\n>> (I hope to see the ABI compliance checker as a pre-integration\n>> validation stage).\n> \n> I think it should be versioned exactly as libcamera. That part doesn't\n> worry me. What bothers me is that the platform headers will contain both\n> stable and non-stable APIs (and ABIs), which will not be easy to convey.\n> Applications will use signal.h, but we don't want them to use thread.h.\n> Even if we document that they shouldn't, some will, and there will be\n> breakages.\n> \n>>> In any case, it's possibly something we can handle later, but as this\n>>> series makes quite a few internal headers public in libcamera-platform,\n>>> I'm a bit worried we will start using them in the public libcamera API.\n>>> Currently there's no risk of doing so by mistake, as the headers are\n>>> clearly marked as internal by their location, and we would immediately\n>>> spot during review an attempt to move an internal header to the public\n>>> directory.\n>>\n>> Yes, having 'you can use this header' ... ' you can not use this header'\n>> would become a bit awkward ...\n> \n> Random idea, how about adding a\n> \n> #ifndef LIBCAMERA_PLATFORM_PRIVATE\n> #error ...\n> #endif\n> \n> at the beginning of all private headers ? Both libcamera and IPA modules\n> would define LIBCAMERA_PLATFORM_PRIVATE. cam and qcam wouldn't. We can\n> bikeshed the LIBCAMERA_PLATFORM_PRIVATE macro name of course :-)\n> \n> This may not be the long term solution we want, but it would already\n> avoid mistakes going unnoticed, and it's a simple change.\n\n\nThat can prevent inclusion of headers which are not permitted indeed,\nand if an application defines LIBCAMERA_PLATFORM_PRIVATE - then they're\non their own.\n\nIt also gives the opportunity to present an appropriate error message\nwhich I like too.\n\n\nSo we have two options:\n\nA)\n Split platform headers into two locations, even though they are\n 'public' to convey that they are not guaranteed to be stable ABI (I\n fear how much we're shooting ourselves in the foot over this)\n\n #include \t\tPublic API\n #include \tPublic Platform API\n #include \tPublic but unstable ABI\n\n\nB)\n Keep platform headers together, but define a preprocessor guard\n\n #include \t Public API\n #include Public Platform API\n #include \t Requires '#define LC_P_PRIV'\n\nwith s/LC_P_PRIV/LIBCAMERA_PLATFORM_PRIVATE/ of course. Shortened for\nline length.\n\n\nIt's worth noting that both A and B can both be done together in fact,\nor perhaps even maybe they should?\n\n\n\n>>>> + libcamera_platform_sources,\n>>>> libipa_headers,\n>>>> libipa_sources,\n>>>> ],\n>>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>>>> index 086c958b0a53..2c3bbeb8df36 100644\n>>>> --- a/include/libcamera/meson.build\n>>>> +++ b/include/libcamera/meson.build\n>>>> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n>>>> \n>>>> subdir('internal')\n>>>> subdir('ipa')\n>>>> +subdir('platform')\n>>>> \n>>>> install_headers(libcamera_public_headers,\n>>>> subdir : include_dir)\n>>>> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n>>>> new file mode 100644\n>>>> index 000000000000..c8e0d0c5ba12\n>>>> --- /dev/null\n>>>> +++ b/include/libcamera/platform/meson.build\n>>>> @@ -0,0 +1,9 @@\n>>>> +# SPDX-License-Identifier: CC0-1.0\n>>>> +\n>>>> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n>>>> +\n>>>> +libcamera_platform_headers = files([\n>>>> +])\n>>>> +\n>>>> +install_headers(libcamera_platform_headers,\n>>>> + subdir: libcamera_platform_include_dir)\n>>>> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n>>>> new file mode 100644\n>>>> index 000000000000..64d0dfee2731\n>>>> --- /dev/null\n>>>> +++ b/src/libcamera-platform/meson.build\n>>>> @@ -0,0 +1,29 @@\n>>>> +# SPDX-License-Identifier: CC0-1.0\n>>>> +\n>>>> +libcamera_platform_sources = files([\n>>>> +])\n>>>> +\n>>>> +libcamera_platform_deps = [\n>>>> +]\n>>>> +\n>>>> +libcamera_platform_lib = shared_library('libcamera_platform',\n>>>\n>>> $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n>>> 664\n>>> $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n>>> 619\n>>>\n>>> Nearly a draw :-) I've checked because I would have sworn '-' was way\n>>> more common than '_', but it seems it's a personal preference.\n>>\n>> This should be '-', I suspect the _ was a copy paste from the meson\n>> variable name ;-(\n>>\n>>>> + [libcamera_platform_sources, libcamera_platform_headers],\n>>>\n>>> You're missing one space in the indentation.\n>>\n>> ack.\n>>\n>>>> + name_prefix : '',\n>>>\n>>> Any reason why you can't drop this line and use 'camera_platform' as the\n>>> library name ?\n>>\n>> To be consistent with our libcamera meson file?\n> \n> We have\n> \n> libcamera = shared_library('camera',\n> ...\n> \n> with no name_prefix.\n\nThat's confused me ;-) Hrm:\n\ngg name_prefix\nsrc/android/meson.build: name_prefix : '',\nsrc/ipa/ipu3/meson.build: name_prefix : '',\nsrc/ipa/raspberrypi/meson.build: name_prefix : '',\nsrc/ipa/rkisp1/meson.build: name_prefix : '',\nsrc/ipa/vimc/meson.build: name_prefix : '',\nsrc/v4l2/meson.build: name_prefix : '',\n\nI must have got the template from src/ipa instead or such then.\n\nI'll update to be the same as the main libcamera one indeed.\n\n\n> \n>>>> + install : true,\n>>>> + cpp_args : libcamera_cpp_args,\n>>>> + include_directories : libcamera_includes,\n>>>> + dependencies : libcamera_platform_deps)\n>>>> +\n>>>> +libcamera_platform = declare_dependency(sources : [\n>>>> + libcamera_platform_headers,\n>>>\n>>> One space missing here too.\n>>>\n>>>> + ],\n>>>> + include_directories : libcamera_includes,\n>>>> + link_with : libcamera_platform_lib)\n>>>\n>>> Do we actually need this ? Wouldn't it be enough to just link libcamera\n>>> (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n>>> libcamera_dep is because libcamera generates some headers, which needs\n>>> to be done before anything compiling against those headers gets built.\n>>> That's not needed for the platform library (at least not yet :-)).\n>>>\n>>>> +\n>>>> +pkg_mod = import('pkgconfig')\n>>>> +pkg_mod.generate(libraries : libcamera_platform_lib,\n>>>> + version : '1.0',\n>>>> + name : 'libcamera-platform',\n>>>\n>>> One more reason to name the binary libcamera-platform.so ;-)\n>>\n>> Yes, that was my intended name.\n>>\n>>>> + filebase : 'camera-platform',\n>>>> + description : 'Complex Camera Support Library',\n>>>\n>>> This should be updated.\n>>>\n>>>> + subdirs : 'libcamera')\n>>>\n>>> I wonder if we should have a separate pkgconfig file for\n>>> libcamera-platform, or include the library in the libcamera pkgconfig.\n>>> It's an internal split really, and it wouldn't be nice to force\n>>> applications to deal with the libcamera-platform pkgconfig explicitly.\n>>>\n>>> On the other hand, IPA modules will need this. Maybe we should do both,\n>>\n>> Yes - the point is that IPA's want to explicitly pull this in.\n> \n> Would it then make sense to have libcamera-platform in both the\n> libcamera and libcamera-platform pkgconfig ?\n\nI think I'm missing something. libcamera already links against\nlibcamera-platform. Applications don't need to...\n\nWhat 'part' of libcamera-platform do you want to put into the pkgconfig?\n\nThe linker will already pull it in from the link required by libcamera.so.\n\nAnd the headers are already on a common path\n #include \n\nApplications shouldn't need to know nor care about libcamera-platform.\n\n\n>>> and a libcamera-platform.pc for IPA modules, and include\n>>> libcamera-platform.so in the libraries of libcamera.pc ?\n>>>\n>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>> index 54512652272c..6ba59e4006cb 100644\n>>>> --- a/src/libcamera/meson.build\n>>>> +++ b/src/libcamera/meson.build\n>>>> @@ -127,6 +127,7 @@ libcamera_deps = [\n>>>> libgnutls,\n>>>> liblttng,\n>>>> libudev,\n>>>> + libcamera_platform,\n>>>> dependency('threads'),\n>>>> ]\n>>>> \n>>>> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n>>>> libcamera_generated_ipa_headers,\n>>>> ],\n>>>> include_directories : libcamera_includes,\n>>>> + dependencies: libcamera_platform,\n>>>> link_with : libcamera)\n>>>> \n>>>> subdir('proxy/worker')\n>>>> diff --git a/src/meson.build b/src/meson.build\n>>>> index a4e96ecd728a..70e1a4618a0f 100644\n>>>> --- a/src/meson.build\n>>>> +++ b/src/meson.build\n>>>> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n>>>> libcamera_objects = []\n>>>> \n>>>> # libcamera must be built first as a dependency to the other components.\n>>>> +subdir('libcamera-platform')\n>>>> subdir('libcamera')\n>>>> \n>>>> subdir('android')\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 83609C3218\n\tfor ;\n\tFri, 18 Jun 2021 09:00:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95E8768945;\n\tFri, 18 Jun 2021 11:00:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 430B060296\n\tfor ;\n\tFri, 18 Jun 2021 11:00:25 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A583E9E2;\n\tFri, 18 Jun 2021 11:00:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Puy1i5XL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624006824;\n\tbh=o+CY+Cls02J2ETho5JvsRXGolzSAkd86L5SqBGV0pNs=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=Puy1i5XLHyguNtjdpo0M/RTDQLTzt1HSvyz/KNXXD3rXP2tdL91WrS0Sw33rfC7bX\n\tpe1BnZOEIVT8HwQVLPbUYNcYDaf286N6JW9aRO43ZiKAIMMSMZOAeya5Gij1l2+AXc\n\t4MIyBYszbiCJ6rxQ1u2hK7X8TOm5lJ/jx3862Hao=","To":"Laurent Pinchart ","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t\n\t<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<2b385b30-3fe6-353f-3712-eadea9684fca@ideasonboard.com>","Date":"Fri, 18 Jun 2021 10:00:22 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17622,"web_url":"https://patchwork.libcamera.org/comment/17622/","msgid":"","date":"2021-06-18T09:11:33","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Jun 18, 2021 at 10:00:22AM +0100, Kieran Bingham wrote:\n> On 17/06/2021 10:33, Laurent Pinchart wrote:\n> > On Thu, Jun 17, 2021 at 10:10:10AM +0100, Kieran Bingham wrote:\n> >> On 17/06/2021 03:20, Laurent Pinchart wrote:\n> >>> On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n> >>>> The libcamera-platform.so will feature internal support functionality\n> >>>> that is utilised by libcamera, and can be shared in other places.\n> >>>\n> >>> I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n> >>> \"support\" previously I believe, and I thought that's what you would use,\n> >>> but \"platform\" doesn't sound too bad. The only downside I can see is\n> >>\n> >> support was really grating on me, in particular when I realised there\n> >> are likely to be libraries beneath libcamera, and libraries 'above'.\n> >>\n> >> Platform to me feels more like the 'base' ... (because you can stand on\n> >> a platform).\n> >>\n> >>> that it could be construed as a library aimed at abstracting differences\n> >>> between platforms. \"base\" may be another option, although I'm not sure\n> >>> to like it (the name comes from Chromium, where it has a similar role as\n> >>> the library you're creating here).\n> >>\n> >> As mentioned in the cover letter, it could be that in the future indeed\n> >> we might need some platform abstraction. So that's again why I preferred\n> >> platform from the beginning.\n> > \n> > There would likely be a big overlap between this library and the\n> > platform abstraction, but I'm a bit concerned that some platform\n> > abstraction may need to go elsewhere. I see the goal for this library to\n> > be more of a foundation than a platform abstraction.\n> > \n> > Maybe libcamera-foundation is a possible name :-)\n> \n> Hrm ... that sounds like something else entirely ...\n\n:-)\n\n> >>>> However - the libcamera-platform library does not constitute a part\n> >>>> of the public libcamera API directly. It is a layer beneath libcamera\n> >>>> which provides common abstractions to internal objects.\n> >>>\n> >>> That's partly true only I'm afraid, looking at patch 4/6,\n> >>> bound_method.h, object.h and signal.h are moved to this library, and\n> >>> they're part of the libcamera public API. It's not a problem in itself.\n> >>\n> >> Argh - yes - that's annoying - and I had somewhat mis-considered this.\n> >>\n> >> This commit was written before I ended up pulling in libcamera public\n> >> headers, which I only hit deeper down the rabbit-hole ...\n> >>\n> >>>> Signed-off-by: Kieran Bingham \n> >>>> ---\n> >>>> Documentation/Doxyfile.in | 4 +++-\n> >>>> Documentation/meson.build | 2 ++\n> >>>> include/libcamera/meson.build | 1 +\n> >>>> include/libcamera/platform/meson.build | 9 ++++++++\n> >>>> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n> >>>> src/libcamera/meson.build | 2 ++\n> >>>> src/meson.build | 1 +\n> >>>> 7 files changed, 47 insertions(+), 1 deletion(-)\n> >>>> create mode 100644 include/libcamera/platform/meson.build\n> >>>> create mode 100644 src/libcamera-platform/meson.build\n> >>>>\n> >>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> >>>> index 8305f56af7a8..c1b395bf0b83 100644\n> >>>> --- a/Documentation/Doxyfile.in\n> >>>> +++ b/Documentation/Doxyfile.in\n> >>>> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> >>>> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> >>>> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> >>>> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n> >>>> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> >>>> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n> >>>> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n> >>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n> >>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> >>>> \n> >>>\n> >>> Drive-by comment, I've been toying with the idea of splitting the\n> >>> Doxygen documentation between the public and internal API.\n> >>\n> >> I think that's likely a good idea.\n> >>\n> >> Hopefully we can get some more time to work on documentation\n> >> specifically as I think we need some more introduction type pages and\n> >> something to actually guide through the doxygen generated pages.\n> > \n> > Agreed.\n> > \n> >>>> # This tag can be used to specify the character encoding of the source files\n> >>>> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n> >>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> >>>> index 9ecf4dfcf79f..01b753f07fb6 100644\n> >>>> --- a/Documentation/meson.build\n> >>>> +++ b/Documentation/meson.build\n> >>>> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> >>>> libcamera_ipa_interfaces,\n> >>>> libcamera_public_headers,\n> >>>> libcamera_sources,\n> >>>> + libcamera_platform_headers,\n> >>>\n> >>> Following up on the comment in the commit message, do we need\n> >>> libcamera_platform_internal_headers and\n> >>> libcamera_platform_public_headers ? The distinction between the two\n> >>> would be different than for libcamera. We currently don't install the\n> >>> internal headers, while we'll need to install the \"internal platform\"\n> >>> headers as they can be used by IPA modules. What I'd like to avoid is\n> >>> giving an ABI stability guarantee for the whole platform library.\n> >>\n> >> I'm concerned that would be a mistake.\n> >> How can you have public-public headers and public-private-headers?\n> >>\n> >> All of the headers in this library are publicly exposed.\n> >>\n> >> I want to be able to say I think the risk of having ABI breakage on this\n> >> library component is lower though - because these are mostly just helper\n> >> objects, but I know if I were to actually say that, then we'd change the\n> >> ABI on say File or ... something ...\n> >>\n> >> I guess that begs the question of how will libcamera-platform be\n> >> versioned. I think as long as it's in the same repo as libcamera itself\n> >> - it would share the same versions, and I now have the\n> >> abi-compliance-checker which will identify if we do cause an ABI breakage.\n> >>\n> >> (I hope to see the ABI compliance checker as a pre-integration\n> >> validation stage).\n> > \n> > I think it should be versioned exactly as libcamera. That part doesn't\n> > worry me. What bothers me is that the platform headers will contain both\n> > stable and non-stable APIs (and ABIs), which will not be easy to convey.\n> > Applications will use signal.h, but we don't want them to use thread.h.\n> > Even if we document that they shouldn't, some will, and there will be\n> > breakages.\n> > \n> >>> In any case, it's possibly something we can handle later, but as this\n> >>> series makes quite a few internal headers public in libcamera-platform,\n> >>> I'm a bit worried we will start using them in the public libcamera API.\n> >>> Currently there's no risk of doing so by mistake, as the headers are\n> >>> clearly marked as internal by their location, and we would immediately\n> >>> spot during review an attempt to move an internal header to the public\n> >>> directory.\n> >>\n> >> Yes, having 'you can use this header' ... ' you can not use this header'\n> >> would become a bit awkward ...\n> > \n> > Random idea, how about adding a\n> > \n> > #ifndef LIBCAMERA_PLATFORM_PRIVATE\n> > #error ...\n> > #endif\n> > \n> > at the beginning of all private headers ? Both libcamera and IPA modules\n> > would define LIBCAMERA_PLATFORM_PRIVATE. cam and qcam wouldn't. We can\n> > bikeshed the LIBCAMERA_PLATFORM_PRIVATE macro name of course :-)\n> > \n> > This may not be the long term solution we want, but it would already\n> > avoid mistakes going unnoticed, and it's a simple change.\n> \n> That can prevent inclusion of headers which are not permitted indeed,\n> and if an application defines LIBCAMERA_PLATFORM_PRIVATE - then they're\n> on their own.\n> \n> It also gives the opportunity to present an appropriate error message\n> which I like too.\n> \n> \n> So we have two options:\n> \n> A)\n> Split platform headers into two locations, even though they are\n> 'public' to convey that they are not guaranteed to be stable ABI (I\n> fear how much we're shooting ourselves in the foot over this)\n> \n> #include \t\tPublic API\n> #include \tPublic Platform API\n> #include \tPublic but unstable ABI\n> \n> \n> B)\n> Keep platform headers together, but define a preprocessor guard\n> \n> #include \t Public API\n> #include Public Platform API\n> #include \t Requires '#define LC_P_PRIV'\n> \n> with s/LC_P_PRIV/LIBCAMERA_PLATFORM_PRIVATE/ of course. Shortened for\n> line length.\n> \n> \n> It's worth noting that both A and B can both be done together in fact,\n> or perhaps even maybe they should?\n\nI'd start with B as A will be another large patch. We can then see how\nwe like it, and apply A later if needed. How does that sound ?\n\n> >>>> + libcamera_platform_sources,\n> >>>> libipa_headers,\n> >>>> libipa_sources,\n> >>>> ],\n> >>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> >>>> index 086c958b0a53..2c3bbeb8df36 100644\n> >>>> --- a/include/libcamera/meson.build\n> >>>> +++ b/include/libcamera/meson.build\n> >>>> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> >>>> \n> >>>> subdir('internal')\n> >>>> subdir('ipa')\n> >>>> +subdir('platform')\n> >>>> \n> >>>> install_headers(libcamera_public_headers,\n> >>>> subdir : include_dir)\n> >>>> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n> >>>> new file mode 100644\n> >>>> index 000000000000..c8e0d0c5ba12\n> >>>> --- /dev/null\n> >>>> +++ b/include/libcamera/platform/meson.build\n> >>>> @@ -0,0 +1,9 @@\n> >>>> +# SPDX-License-Identifier: CC0-1.0\n> >>>> +\n> >>>> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> >>>> +\n> >>>> +libcamera_platform_headers = files([\n> >>>> +])\n> >>>> +\n> >>>> +install_headers(libcamera_platform_headers,\n> >>>> + subdir: libcamera_platform_include_dir)\n> >>>> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n> >>>> new file mode 100644\n> >>>> index 000000000000..64d0dfee2731\n> >>>> --- /dev/null\n> >>>> +++ b/src/libcamera-platform/meson.build\n> >>>> @@ -0,0 +1,29 @@\n> >>>> +# SPDX-License-Identifier: CC0-1.0\n> >>>> +\n> >>>> +libcamera_platform_sources = files([\n> >>>> +])\n> >>>> +\n> >>>> +libcamera_platform_deps = [\n> >>>> +]\n> >>>> +\n> >>>> +libcamera_platform_lib = shared_library('libcamera_platform',\n> >>>\n> >>> $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n> >>> 664\n> >>> $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n> >>> 619\n> >>>\n> >>> Nearly a draw :-) I've checked because I would have sworn '-' was way\n> >>> more common than '_', but it seems it's a personal preference.\n> >>\n> >> This should be '-', I suspect the _ was a copy paste from the meson\n> >> variable name ;-(\n> >>\n> >>>> + [libcamera_platform_sources, libcamera_platform_headers],\n> >>>\n> >>> You're missing one space in the indentation.\n> >>\n> >> ack.\n> >>\n> >>>> + name_prefix : '',\n> >>>\n> >>> Any reason why you can't drop this line and use 'camera_platform' as the\n> >>> library name ?\n> >>\n> >> To be consistent with our libcamera meson file?\n> > \n> > We have\n> > \n> > libcamera = shared_library('camera',\n> > ...\n> > \n> > with no name_prefix.\n> \n> That's confused me ;-) Hrm:\n> \n> gg name_prefix\n> src/android/meson.build: name_prefix : '',\n> src/ipa/ipu3/meson.build: name_prefix : '',\n> src/ipa/raspberrypi/meson.build: name_prefix : '',\n> src/ipa/rkisp1/meson.build: name_prefix : '',\n> src/ipa/vimc/meson.build: name_prefix : '',\n> src/v4l2/meson.build: name_prefix : '',\n\nThat's because all of those produce .so files that don't start with\n'lib'.\n\n> I must have got the template from src/ipa instead or such then.\n> \n> I'll update to be the same as the main libcamera one indeed.\n> \n> >>>> + install : true,\n> >>>> + cpp_args : libcamera_cpp_args,\n> >>>> + include_directories : libcamera_includes,\n> >>>> + dependencies : libcamera_platform_deps)\n> >>>> +\n> >>>> +libcamera_platform = declare_dependency(sources : [\n> >>>> + libcamera_platform_headers,\n> >>>\n> >>> One space missing here too.\n> >>>\n> >>>> + ],\n> >>>> + include_directories : libcamera_includes,\n> >>>> + link_with : libcamera_platform_lib)\n> >>>\n> >>> Do we actually need this ? Wouldn't it be enough to just link libcamera\n> >>> (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n> >>> libcamera_dep is because libcamera generates some headers, which needs\n> >>> to be done before anything compiling against those headers gets built.\n> >>> That's not needed for the platform library (at least not yet :-)).\n> >>>\n> >>>> +\n> >>>> +pkg_mod = import('pkgconfig')\n> >>>> +pkg_mod.generate(libraries : libcamera_platform_lib,\n> >>>> + version : '1.0',\n> >>>> + name : 'libcamera-platform',\n> >>>\n> >>> One more reason to name the binary libcamera-platform.so ;-)\n> >>\n> >> Yes, that was my intended name.\n> >>\n> >>>> + filebase : 'camera-platform',\n> >>>> + description : 'Complex Camera Support Library',\n> >>>\n> >>> This should be updated.\n> >>>\n> >>>> + subdirs : 'libcamera')\n> >>>\n> >>> I wonder if we should have a separate pkgconfig file for\n> >>> libcamera-platform, or include the library in the libcamera pkgconfig.\n> >>> It's an internal split really, and it wouldn't be nice to force\n> >>> applications to deal with the libcamera-platform pkgconfig explicitly.\n> >>>\n> >>> On the other hand, IPA modules will need this. Maybe we should do both,\n> >>\n> >> Yes - the point is that IPA's want to explicitly pull this in.\n> > \n> > Would it then make sense to have libcamera-platform in both the\n> > libcamera and libcamera-platform pkgconfig ?\n> \n> I think I'm missing something. libcamera already links against\n> libcamera-platform. Applications don't need to...\n\nIf an application connects to a signal, it will use a symbol from\nlibcamera-platform, and should thus link to it.\n\n> What 'part' of libcamera-platform do you want to put into the pkgconfig?\n> \n> The linker will already pull it in from the link required by libcamera.so.\n\nAre you sure about that ? At least for static linking (which we don't\ntest yet), an application needs to explicitly link to all the libraries\nproviding symbols it uses.\n\n> And the headers are already on a common path\n> #include \n> \n> Applications shouldn't need to know nor care about libcamera-platform.\n\nExactly, that's why libcamera.pc should include a -lcamera-platform. No\nneed for separate -L or -I entries.\n\n> >>> and a libcamera-platform.pc for IPA modules, and include\n> >>> libcamera-platform.so in the libraries of libcamera.pc ?\n> >>>\n> >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>>> index 54512652272c..6ba59e4006cb 100644\n> >>>> --- a/src/libcamera/meson.build\n> >>>> +++ b/src/libcamera/meson.build\n> >>>> @@ -127,6 +127,7 @@ libcamera_deps = [\n> >>>> libgnutls,\n> >>>> liblttng,\n> >>>> libudev,\n> >>>> + libcamera_platform,\n> >>>> dependency('threads'),\n> >>>> ]\n> >>>> \n> >>>> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> >>>> libcamera_generated_ipa_headers,\n> >>>> ],\n> >>>> include_directories : libcamera_includes,\n> >>>> + dependencies: libcamera_platform,\n> >>>> link_with : libcamera)\n> >>>> \n> >>>> subdir('proxy/worker')\n> >>>> diff --git a/src/meson.build b/src/meson.build\n> >>>> index a4e96ecd728a..70e1a4618a0f 100644\n> >>>> --- a/src/meson.build\n> >>>> +++ b/src/meson.build\n> >>>> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> >>>> libcamera_objects = []\n> >>>> \n> >>>> # libcamera must be built first as a dependency to the other components.\n> >>>> +subdir('libcamera-platform')\n> >>>> subdir('libcamera')\n> >>>> \n> >>>> subdir('android')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 45EC4BD78E\n\tfor ;\n\tFri, 18 Jun 2021 09:11:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F3846892E;\n\tFri, 18 Jun 2021 11:11:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 099CF60296\n\tfor ;\n\tFri, 18 Jun 2021 11:11:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 72A149E2;\n\tFri, 18 Jun 2021 11:11:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aiQL5xNf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624007516;\n\tbh=HnB0K2ZV2WLJ9+jUX6fojZi3Nb9gW5s9BbdxrzUEo+4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aiQL5xNfVEM6zc+V7Z69o6G3rOf7RgytyiMRyPo7VdTDLs2AZrf15NA7nmLq+5+X5\n\tflouwkbXp6v7Pgl6JjOkotu5MM9cUpN5it6TKutPPm1C9pvrldiv1iH6UclpCGGFkD\n\tHX+xRjKUmnU8vC60nkTPxhsUt7c0FGx8/WwpEAg4=","Date":"Fri, 18 Jun 2021 12:11:33 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t\n\t<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>\n\t\n\t<2b385b30-3fe6-353f-3712-eadea9684fca@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2b385b30-3fe6-353f-3712-eadea9684fca@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17625,"web_url":"https://patchwork.libcamera.org/comment/17625/","msgid":"<078f4c29-d99d-a548-53d9-a918f28e6301@ideasonboard.com>","date":"2021-06-18T10:20:06","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 18/06/2021 10:11, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Fri, Jun 18, 2021 at 10:00:22AM +0100, Kieran Bingham wrote:\n>> On 17/06/2021 10:33, Laurent Pinchart wrote:\n>>> On Thu, Jun 17, 2021 at 10:10:10AM +0100, Kieran Bingham wrote:\n>>>> On 17/06/2021 03:20, Laurent Pinchart wrote:\n>>>>> On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n>>>>>> The libcamera-platform.so will feature internal support functionality\n>>>>>> that is utilised by libcamera, and can be shared in other places.\n>>>>>\n>>>>> I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n>>>>> \"support\" previously I believe, and I thought that's what you would use,\n>>>>> but \"platform\" doesn't sound too bad. The only downside I can see is\n>>>>\n>>>> support was really grating on me, in particular when I realised there\n>>>> are likely to be libraries beneath libcamera, and libraries 'above'.\n>>>>\n>>>> Platform to me feels more like the 'base' ... (because you can stand on\n>>>> a platform).\n>>>>\n>>>>> that it could be construed as a library aimed at abstracting differences\n>>>>> between platforms. \"base\" may be another option, although I'm not sure\n>>>>> to like it (the name comes from Chromium, where it has a similar role as\n>>>>> the library you're creating here).\n\nbase is growing on me. Reminds me of 'gstreamer-plugins-base' which is\nused for common code on gstreamer plugins. But that then would make me\nfear users expecting it to do more as well.\n\nThere's never a perfect name ;-)\n\nBefore these get merged, it will be easy to rename this from platform to\nanother name by running sed on the patches themselves and reapplying.\n\nSo if you'd prefer another name, now's the time to shout, and we can\nmake it happen quicker now than later.\n\n\n\n\n>>>> As mentioned in the cover letter, it could be that in the future indeed\n>>>> we might need some platform abstraction. So that's again why I preferred\n>>>> platform from the beginning.\n>>>\n>>> There would likely be a big overlap between this library and the\n>>> platform abstraction, but I'm a bit concerned that some platform\n>>> abstraction may need to go elsewhere. I see the goal for this library to\n>>> be more of a foundation than a platform abstraction.\n>>>\n>>> Maybe libcamera-foundation is a possible name :-)\n>>\n>> Hrm ... that sounds like something else entirely ...\n> \n> :-)\n> \n>>>>>> However - the libcamera-platform library does not constitute a part\n>>>>>> of the public libcamera API directly. It is a layer beneath libcamera\n>>>>>> which provides common abstractions to internal objects.\n>>>>>\n>>>>> That's partly true only I'm afraid, looking at patch 4/6,\n>>>>> bound_method.h, object.h and signal.h are moved to this library, and\n>>>>> they're part of the libcamera public API. It's not a problem in itself.\n>>>>\n>>>> Argh - yes - that's annoying - and I had somewhat mis-considered this.\n>>>>\n>>>> This commit was written before I ended up pulling in libcamera public\n>>>> headers, which I only hit deeper down the rabbit-hole ...\n>>>>\n>>>>>> Signed-off-by: Kieran Bingham \n>>>>>> ---\n>>>>>> Documentation/Doxyfile.in | 4 +++-\n>>>>>> Documentation/meson.build | 2 ++\n>>>>>> include/libcamera/meson.build | 1 +\n>>>>>> include/libcamera/platform/meson.build | 9 ++++++++\n>>>>>> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n>>>>>> src/libcamera/meson.build | 2 ++\n>>>>>> src/meson.build | 1 +\n>>>>>> 7 files changed, 47 insertions(+), 1 deletion(-)\n>>>>>> create mode 100644 include/libcamera/platform/meson.build\n>>>>>> create mode 100644 src/libcamera-platform/meson.build\n>>>>>>\n>>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>>>>>> index 8305f56af7a8..c1b395bf0b83 100644\n>>>>>> --- a/Documentation/Doxyfile.in\n>>>>>> +++ b/Documentation/Doxyfile.in\n>>>>>> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n>>>>>> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n>>>>>> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n>>>>>> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n>>>>>> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n>>>>>> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n>>>>>> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n>>>>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n>>>>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n>>>>>> \n>>>>>\n>>>>> Drive-by comment, I've been toying with the idea of splitting the\n>>>>> Doxygen documentation between the public and internal API.\n>>>>\n>>>> I think that's likely a good idea.\n>>>>\n>>>> Hopefully we can get some more time to work on documentation\n>>>> specifically as I think we need some more introduction type pages and\n>>>> something to actually guide through the doxygen generated pages.\n>>>\n>>> Agreed.\n>>>\n>>>>>> # This tag can be used to specify the character encoding of the source files\n>>>>>> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n>>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>>>>>> index 9ecf4dfcf79f..01b753f07fb6 100644\n>>>>>> --- a/Documentation/meson.build\n>>>>>> +++ b/Documentation/meson.build\n>>>>>> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n>>>>>> libcamera_ipa_interfaces,\n>>>>>> libcamera_public_headers,\n>>>>>> libcamera_sources,\n>>>>>> + libcamera_platform_headers,\n>>>>>\n>>>>> Following up on the comment in the commit message, do we need\n>>>>> libcamera_platform_internal_headers and\n>>>>> libcamera_platform_public_headers ? The distinction between the two\n>>>>> would be different than for libcamera. We currently don't install the\n>>>>> internal headers, while we'll need to install the \"internal platform\"\n>>>>> headers as they can be used by IPA modules. What I'd like to avoid is\n>>>>> giving an ABI stability guarantee for the whole platform library.\n>>>>\n>>>> I'm concerned that would be a mistake.\n>>>> How can you have public-public headers and public-private-headers?\n>>>>\n>>>> All of the headers in this library are publicly exposed.\n>>>>\n>>>> I want to be able to say I think the risk of having ABI breakage on this\n>>>> library component is lower though - because these are mostly just helper\n>>>> objects, but I know if I were to actually say that, then we'd change the\n>>>> ABI on say File or ... something ...\n>>>>\n>>>> I guess that begs the question of how will libcamera-platform be\n>>>> versioned. I think as long as it's in the same repo as libcamera itself\n>>>> - it would share the same versions, and I now have the\n>>>> abi-compliance-checker which will identify if we do cause an ABI breakage.\n>>>>\n>>>> (I hope to see the ABI compliance checker as a pre-integration\n>>>> validation stage).\n>>>\n>>> I think it should be versioned exactly as libcamera. That part doesn't\n>>> worry me. What bothers me is that the platform headers will contain both\n>>> stable and non-stable APIs (and ABIs), which will not be easy to convey.\n>>> Applications will use signal.h, but we don't want them to use thread.h.\n>>> Even if we document that they shouldn't, some will, and there will be\n>>> breakages.\n>>>\n>>>>> In any case, it's possibly something we can handle later, but as this\n>>>>> series makes quite a few internal headers public in libcamera-platform,\n>>>>> I'm a bit worried we will start using them in the public libcamera API.\n>>>>> Currently there's no risk of doing so by mistake, as the headers are\n>>>>> clearly marked as internal by their location, and we would immediately\n>>>>> spot during review an attempt to move an internal header to the public\n>>>>> directory.\n>>>>\n>>>> Yes, having 'you can use this header' ... ' you can not use this header'\n>>>> would become a bit awkward ...\n>>>\n>>> Random idea, how about adding a\n>>>\n>>> #ifndef LIBCAMERA_PLATFORM_PRIVATE\n>>> #error ...\n>>> #endif\n>>>\n>>> at the beginning of all private headers ? Both libcamera and IPA modules\n>>> would define LIBCAMERA_PLATFORM_PRIVATE. cam and qcam wouldn't. We can\n>>> bikeshed the LIBCAMERA_PLATFORM_PRIVATE macro name of course :-)\n>>>\n>>> This may not be the long term solution we want, but it would already\n>>> avoid mistakes going unnoticed, and it's a simple change.\n>>\n>> That can prevent inclusion of headers which are not permitted indeed,\n>> and if an application defines LIBCAMERA_PLATFORM_PRIVATE - then they're\n>> on their own.\n>>\n>> It also gives the opportunity to present an appropriate error message\n>> which I like too.\n>>\n>>\n>> So we have two options:\n>>\n>> A)\n>> Split platform headers into two locations, even though they are\n>> 'public' to convey that they are not guaranteed to be stable ABI (I\n>> fear how much we're shooting ourselves in the foot over this)\n>>\n>> #include \t\tPublic API\n>> #include \tPublic Platform API\n>> #include \tPublic but unstable ABI\n>>\n>>\n>> B)\n>> Keep platform headers together, but define a preprocessor guard\n>>\n>> #include \t Public API\n>> #include Public Platform API\n>> #include \t Requires '#define LC_P_PRIV'\n>>\n>> with s/LC_P_PRIV/LIBCAMERA_PLATFORM_PRIVATE/ of course. Shortened for\n>> line length.\n>>\n>>\n>> It's worth noting that both A and B can both be done together in fact,\n>> or perhaps even maybe they should?\n> \n> I'd start with B as A will be another large patch. We can then see how\n> we like it, and apply A later if needed. How does that sound ?\n\n\nB is fine.\n\nI will likely create a\n\n#include so that it's common.\nSomething like:\n\n\n/* SPDX-License-Identifier: LGPL-2.1-or-later */\n/*\n * Copyright (C) 2021, Google Inc.\n *\n * private.h - Private Header Validation\n *\n * A selection of internal libcamera headers are installed as part\n * of the libcamera package to allow sharing of a select subset of\n * internal functionality with IPA developers only.\n *\n * This functionality is not considered part of the public libcamera\n * API, and can therefore potentially face ABI instabilities which\n * should not be exposed to applications. IPAs however should be\n * versioned and more closely matched to the libcamera installation.\n *\n * Components which include this file can not be included in any file\n * which forms part of the libcamera API.\n */\n\n#ifndef LIBCAMERA_PLATFORM_PRIVATE\n#error \"Private headers must not be included in the libcamera API\"\n#endif\n\n\nI've done some testing with this and it's quite effective, so I think\nit's helpful.\n\nIt's really showing up where the private API's are used ;-)\n\n\n>>>>>> + libcamera_platform_sources,\n>>>>>> libipa_headers,\n>>>>>> libipa_sources,\n>>>>>> ],\n>>>>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>>>>>> index 086c958b0a53..2c3bbeb8df36 100644\n>>>>>> --- a/include/libcamera/meson.build\n>>>>>> +++ b/include/libcamera/meson.build\n>>>>>> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n>>>>>> \n>>>>>> subdir('internal')\n>>>>>> subdir('ipa')\n>>>>>> +subdir('platform')\n>>>>>> \n>>>>>> install_headers(libcamera_public_headers,\n>>>>>> subdir : include_dir)\n>>>>>> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n>>>>>> new file mode 100644\n>>>>>> index 000000000000..c8e0d0c5ba12\n>>>>>> --- /dev/null\n>>>>>> +++ b/include/libcamera/platform/meson.build\n>>>>>> @@ -0,0 +1,9 @@\n>>>>>> +# SPDX-License-Identifier: CC0-1.0\n>>>>>> +\n>>>>>> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n>>>>>> +\n>>>>>> +libcamera_platform_headers = files([\n>>>>>> +])\n>>>>>> +\n>>>>>> +install_headers(libcamera_platform_headers,\n>>>>>> + subdir: libcamera_platform_include_dir)\n>>>>>> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n>>>>>> new file mode 100644\n>>>>>> index 000000000000..64d0dfee2731\n>>>>>> --- /dev/null\n>>>>>> +++ b/src/libcamera-platform/meson.build\n>>>>>> @@ -0,0 +1,29 @@\n>>>>>> +# SPDX-License-Identifier: CC0-1.0\n>>>>>> +\n>>>>>> +libcamera_platform_sources = files([\n>>>>>> +])\n>>>>>> +\n>>>>>> +libcamera_platform_deps = [\n>>>>>> +]\n>>>>>> +\n>>>>>> +libcamera_platform_lib = shared_library('libcamera_platform',\n>>>>>\n>>>>> $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n>>>>> 664\n>>>>> $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n>>>>> 619\n>>>>>\n>>>>> Nearly a draw :-) I've checked because I would have sworn '-' was way\n>>>>> more common than '_', but it seems it's a personal preference.\n>>>>\n>>>> This should be '-', I suspect the _ was a copy paste from the meson\n>>>> variable name ;-(\n>>>>\n>>>>>> + [libcamera_platform_sources, libcamera_platform_headers],\n>>>>>\n>>>>> You're missing one space in the indentation.\n>>>>\n>>>> ack.\n>>>>\n>>>>>> + name_prefix : '',\n>>>>>\n>>>>> Any reason why you can't drop this line and use 'camera_platform' as the\n>>>>> library name ?\n>>>>\n>>>> To be consistent with our libcamera meson file?\n>>>\n>>> We have\n>>>\n>>> libcamera = shared_library('camera',\n>>> ...\n>>>\n>>> with no name_prefix.\n>>\n>> That's confused me ;-) Hrm:\n>>\n>> gg name_prefix\n>> src/android/meson.build: name_prefix : '',\n>> src/ipa/ipu3/meson.build: name_prefix : '',\n>> src/ipa/raspberrypi/meson.build: name_prefix : '',\n>> src/ipa/rkisp1/meson.build: name_prefix : '',\n>> src/ipa/vimc/meson.build: name_prefix : '',\n>> src/v4l2/meson.build: name_prefix : '',\n> \n> That's because all of those produce .so files that don't start with\n> 'lib'.\n> \n>> I must have got the template from src/ipa instead or such then.\n>>\n>> I'll update to be the same as the main libcamera one indeed.\n>>\n>>>>>> + install : true,\n>>>>>> + cpp_args : libcamera_cpp_args,\n>>>>>> + include_directories : libcamera_includes,\n>>>>>> + dependencies : libcamera_platform_deps)\n>>>>>> +\n>>>>>> +libcamera_platform = declare_dependency(sources : [\n>>>>>> + libcamera_platform_headers,\n>>>>>\n>>>>> One space missing here too.\n>>>>>\n>>>>>> + ],\n>>>>>> + include_directories : libcamera_includes,\n>>>>>> + link_with : libcamera_platform_lib)\n>>>>>\n>>>>> Do we actually need this ? Wouldn't it be enough to just link libcamera\n>>>>> (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n>>>>> libcamera_dep is because libcamera generates some headers, which needs\n>>>>> to be done before anything compiling against those headers gets built.\n>>>>> That's not needed for the platform library (at least not yet :-)).\n>>>>>\n>>>>>> +\n>>>>>> +pkg_mod = import('pkgconfig')\n>>>>>> +pkg_mod.generate(libraries : libcamera_platform_lib,\n>>>>>> + version : '1.0',\n>>>>>> + name : 'libcamera-platform',\n>>>>>\n>>>>> One more reason to name the binary libcamera-platform.so ;-)\n>>>>\n>>>> Yes, that was my intended name.\n>>>>\n>>>>>> + filebase : 'camera-platform',\n>>>>>> + description : 'Complex Camera Support Library',\n>>>>>\n>>>>> This should be updated.\n>>>>>\n>>>>>> + subdirs : 'libcamera')\n>>>>>\n>>>>> I wonder if we should have a separate pkgconfig file for\n>>>>> libcamera-platform, or include the library in the libcamera pkgconfig.\n>>>>> It's an internal split really, and it wouldn't be nice to force\n>>>>> applications to deal with the libcamera-platform pkgconfig explicitly.\n>>>>>\n>>>>> On the other hand, IPA modules will need this. Maybe we should do both,\n>>>>\n>>>> Yes - the point is that IPA's want to explicitly pull this in.\n>>>\n>>> Would it then make sense to have libcamera-platform in both the\n>>> libcamera and libcamera-platform pkgconfig ?\n>>\n>> I think I'm missing something. libcamera already links against\n>> libcamera-platform. Applications don't need to...\n> \n> If an application connects to a signal, it will use a symbol from\n> libcamera-platform, and should thus link to it.\n> \n>> What 'part' of libcamera-platform do you want to put into the pkgconfig?\n>>\n>> The linker will already pull it in from the link required by libcamera.so.\n> \n> Are you sure about that ? At least for static linking (which we don't\n> test yet), an application needs to explicitly link to all the libraries\n> providing symbols it uses.\n> \n\nHrm... indeed - I can see how it would be needed for static.\nWe don't even build statically yet do we?\n\n>> And the headers are already on a common path\n>> #include \n>>\n>> Applications shouldn't need to know nor care about libcamera-platform.\n> \n> Exactly, that's why libcamera.pc should include a -lcamera-platform. No\n> need for separate -L or -I entries.\n> \n>>>>> and a libcamera-platform.pc for IPA modules, and include\n>>>>> libcamera-platform.so in the libraries of libcamera.pc ?\n>>>>>\n>>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>>>> index 54512652272c..6ba59e4006cb 100644\n>>>>>> --- a/src/libcamera/meson.build\n>>>>>> +++ b/src/libcamera/meson.build\n>>>>>> @@ -127,6 +127,7 @@ libcamera_deps = [\n>>>>>> libgnutls,\n>>>>>> liblttng,\n>>>>>> libudev,\n>>>>>> + libcamera_platform,\n>>>>>> dependency('threads'),\n>>>>>> ]\n>>>>>> \n>>>>>> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n>>>>>> libcamera_generated_ipa_headers,\n>>>>>> ],\n>>>>>> include_directories : libcamera_includes,\n>>>>>> + dependencies: libcamera_platform,\n>>>>>> link_with : libcamera)\n>>>>>> \n>>>>>> subdir('proxy/worker')\n>>>>>> diff --git a/src/meson.build b/src/meson.build\n>>>>>> index a4e96ecd728a..70e1a4618a0f 100644\n>>>>>> --- a/src/meson.build\n>>>>>> +++ b/src/meson.build\n>>>>>> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n>>>>>> libcamera_objects = []\n>>>>>> \n>>>>>> # libcamera must be built first as a dependency to the other components.\n>>>>>> +subdir('libcamera-platform')\n>>>>>> subdir('libcamera')\n>>>>>> \n>>>>>> subdir('android')\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 88A9CBD78E\n\tfor ;\n\tFri, 18 Jun 2021 10:20:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4BC8968942;\n\tFri, 18 Jun 2021 12:20:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02CA460298\n\tfor ;\n\tFri, 18 Jun 2021 12:20:09 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E6E63E5;\n\tFri, 18 Jun 2021 12:20:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ELIxTd21\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624011608;\n\tbh=VkkoBSNLFs4XrlqncLs/Hq5HTeKfX9X+KT2W1naIlWw=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=ELIxTd21dU/y6WYTjBbIXbDMCSPh8HaCc49cByc/8mOThzYG0uQu+YFiUInGsVDzd\n\tokZGGrP4ehsJzUq3qTojojvqByn2bG5TLERcMTMTI99db6DEIOKNxjOx9/Gu8ci/Um\n\tPHI3YHEIaLUgVzDw8J8UI2yeap8ghpiPaKGQymUI=","To":"Laurent Pinchart ","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t\n\t<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>\n\t\n\t<2b385b30-3fe6-353f-3712-eadea9684fca@ideasonboard.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<078f4c29-d99d-a548-53d9-a918f28e6301@ideasonboard.com>","Date":"Fri, 18 Jun 2021 11:20:06 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17626,"web_url":"https://patchwork.libcamera.org/comment/17626/","msgid":"","date":"2021-06-18T10:24:46","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 17/06/2021 06:32, Umang Jain wrote:\n> Hi Kieran,\n> \n> On 6/16/21 8:41 PM, Kieran Bingham wrote:\n>> The libcamera-platform.so will feature internal support functionality\n>> that is utilised by libcamera, and can be shared in other places.\n> \n> this sounds similar to -dev / -devel packages listed in distributions.\n> May we can use that suffix? :-)\n\nI don't think we can I'm afraid.\nthe -dev packages are used to imply an application can use that package\nto build against libcamera.\n\nIn this case it's exactly the opposite - this is for /libcamera/ (and\npermitted components, like IPAs, proxy-workers, even android-hal) to\nshare 'private' libcamera interfaces.\n\nBut they shouldn't be provided to application developers who would be\nthe users expecting a libcamera-dev or libcamera-devel package to\nutilise libcamera.\n\n\n\n> libcamera: A complex camera support library for Linux, Android, and\n> ChromeOS\n> libcamera-dev: Development files for libcamera\n> \n> \n>>\n>> However - the libcamera-platform library does not constitute a part\n>> of the public libcamera API directly. It is a layer beneath libcamera\n>> which provides common abstractions to internal objects.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>>   Documentation/Doxyfile.in              |  4 +++-\n>>   Documentation/meson.build              |  2 ++\n>>   include/libcamera/meson.build          |  1 +\n>>   include/libcamera/platform/meson.build |  9 ++++++++\n>>   src/libcamera-platform/meson.build     | 29 ++++++++++++++++++++++++++\n>>   src/libcamera/meson.build              |  2 ++\n>>   src/meson.build                        |  1 +\n>>   7 files changed, 47 insertions(+), 1 deletion(-)\n>>   create mode 100644 include/libcamera/platform/meson.build\n>>   create mode 100644 src/libcamera-platform/meson.build\n>>\n>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>> index 8305f56af7a8..c1b395bf0b83 100644\n>> --- a/Documentation/Doxyfile.in\n>> +++ b/Documentation/Doxyfile.in\n>> @@ -791,8 +791,10 @@ WARN_LOGFILE           =\n>>   INPUT                  = \"@TOP_SRCDIR@/include/libcamera\" \\\n>>                \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n>>                \"@TOP_SRCDIR@/src/libcamera\" \\\n>> +             \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n>>                \"@TOP_BUILDDIR@/include/libcamera\" \\\n>> -             \"@TOP_BUILDDIR@/src/libcamera\"\n>> +             \"@TOP_BUILDDIR@/src/libcamera\" \\\n>> +             \"@TOP_BUILDDIR@/src/libcamera-platform\"\n>>     # This tag can be used to specify the character encoding of the\n>> source files\n>>   # that doxygen parses. Internally doxygen uses the UTF-8 encoding.\n>> Doxygen uses\n>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>> index 9ecf4dfcf79f..01b753f07fb6 100644\n>> --- a/Documentation/meson.build\n>> +++ b/Documentation/meson.build\n>> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n>>                         libcamera_ipa_interfaces,\n>>                         libcamera_public_headers,\n>>                         libcamera_sources,\n>> +                      libcamera_platform_headers,\n>> +                      libcamera_platform_sources,\n>>                         libipa_headers,\n>>                         libipa_sources,\n>>                     ],\n>> diff --git a/include/libcamera/meson.build\n>> b/include/libcamera/meson.build\n>> index 086c958b0a53..2c3bbeb8df36 100644\n>> --- a/include/libcamera/meson.build\n>> +++ b/include/libcamera/meson.build\n>> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n>>     subdir('internal')\n>>   subdir('ipa')\n>> +subdir('platform')\n>>     install_headers(libcamera_public_headers,\n>>                   subdir : include_dir)\n>> diff --git a/include/libcamera/platform/meson.build\n>> b/include/libcamera/platform/meson.build\n>> new file mode 100644\n>> index 000000000000..c8e0d0c5ba12\n>> --- /dev/null\n>> +++ b/include/libcamera/platform/meson.build\n>> @@ -0,0 +1,9 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n>> +\n>> +libcamera_platform_headers = files([\n>> +])\n>> +\n>> +install_headers(libcamera_platform_headers,\n>> +                subdir: libcamera_platform_include_dir)\n>> diff --git a/src/libcamera-platform/meson.build\n>> b/src/libcamera-platform/meson.build\n>> new file mode 100644\n>> index 000000000000..64d0dfee2731\n>> --- /dev/null\n>> +++ b/src/libcamera-platform/meson.build\n>> @@ -0,0 +1,29 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +libcamera_platform_sources = files([\n>> +])\n>> +\n>> +libcamera_platform_deps = [\n>> +]\n>> +\n>> +libcamera_platform_lib = shared_library('libcamera_platform',\n>> +                                       [libcamera_platform_sources,\n>> libcamera_platform_headers],\n>> +                                       name_prefix : '',\n>> +                                       install : true,\n>> +                                       cpp_args : libcamera_cpp_args,\n>> +                                       include_directories :\n>> libcamera_includes,\n> \n> \n> isn't libcamera_includes an overkill here? Since we are splitting off\n> the libraries, I think we should map the relevant include-directories\n> for that component? I don't expect you to fix this in this series, but\n> just wanted to see if this was the right way ahead?\n\n\nThese platform headers share a common base with libcamera.\n\n#include \n#include \n\nThe difference is in who is allowed to use them ...\n\n\n\n>> +                                       dependencies :\n>> libcamera_platform_deps)\n>> +\n>> +libcamera_platform = declare_dependency(sources : [\n>> +                                           libcamera_platform_headers,\n>> +                                       ],\n>> +                                       include_directories :\n>> libcamera_includes,\n>> +                                       link_with :\n>> libcamera_platform_lib)\n>> +\n>> +pkg_mod = import('pkgconfig')\n>> +pkg_mod.generate(libraries : libcamera_platform_lib,\n>> +                 version : '1.0',\n>> +                 name : 'libcamera-platform',\n>> +                 filebase : 'camera-platform',\n>> +                 description : 'Complex Camera Support Library',\n>> +                 subdirs : 'libcamera')\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 54512652272c..6ba59e4006cb 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -127,6 +127,7 @@ libcamera_deps = [\n>>       libgnutls,\n>>       liblttng,\n>>       libudev,\n>> +    libcamera_platform,\n>>       dependency('threads'),\n>>   ]\n>>   @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n>>                                          libcamera_generated_ipa_headers,\n>>                                      ],\n>>                                      include_directories :\n>> libcamera_includes,\n>> +                                   dependencies: libcamera_platform,\n>>                                      link_with : libcamera)\n>>     subdir('proxy/worker')\n>> diff --git a/src/meson.build b/src/meson.build\n>> index a4e96ecd728a..70e1a4618a0f 100644\n>> --- a/src/meson.build\n>> +++ b/src/meson.build\n>> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n>>   libcamera_objects = []\n>>     # libcamera must be built first as a dependency to the other\n>> components.\n>> +subdir('libcamera-platform')\n>>   subdir('libcamera')\n>>     subdir('android')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 00F39C3218\n\tfor ;\n\tFri, 18 Jun 2021 10:24:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4619368942;\n\tFri, 18 Jun 2021 12:24:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D360E60298\n\tfor ;\n\tFri, 18 Jun 2021 12:24:49 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5632F3E5;\n\tFri, 18 Jun 2021 12:24:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iMTCy61s\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624011889;\n\tbh=BxLDfvuF4ScryEIlCTkT19iU3yY176cLBOSB7I6JhIc=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=iMTCy61seCZQXRy6TEAgbxzzbZHtYDUbW8+bz5jl3kAxyGBQEcFXKphnQTdmVCFtL\n\toBMf0KNI5REbM3kFCj/Quhr8CNdSmSFYtoXV2LnynKs8OFee/mCbxK1QB8MYwfi/rR\n\tG7a7yhxgi3bRN+YfHRWMv+DktiUYa065c6x7Jh+Y=","To":"Umang Jain ,\n\tlibcamera devel ","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t<9353eecf-be09-01af-8c06-ab10004ad1bf@ideasonboard.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 18 Jun 2021 11:24:46 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<9353eecf-be09-01af-8c06-ab10004ad1bf@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17627,"web_url":"https://patchwork.libcamera.org/comment/17627/","msgid":"","date":"2021-06-18T10:31:59","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Jun 18, 2021 at 11:20:06AM +0100, Kieran Bingham wrote:\n> On 18/06/2021 10:11, Laurent Pinchart wrote:\n> > On Fri, Jun 18, 2021 at 10:00:22AM +0100, Kieran Bingham wrote:\n> >> On 17/06/2021 10:33, Laurent Pinchart wrote:\n> >>> On Thu, Jun 17, 2021 at 10:10:10AM +0100, Kieran Bingham wrote:\n> >>>> On 17/06/2021 03:20, Laurent Pinchart wrote:\n> >>>>> On Wed, Jun 16, 2021 at 04:11:48PM +0100, Kieran Bingham wrote:\n> >>>>>> The libcamera-platform.so will feature internal support functionality\n> >>>>>> that is utilised by libcamera, and can be shared in other places.\n> >>>>>\n> >>>>> I'm sure we'll bikeshed the \"platform\" name :-) We had mentioned\n> >>>>> \"support\" previously I believe, and I thought that's what you would use,\n> >>>>> but \"platform\" doesn't sound too bad. The only downside I can see is\n> >>>>\n> >>>> support was really grating on me, in particular when I realised there\n> >>>> are likely to be libraries beneath libcamera, and libraries 'above'.\n> >>>>\n> >>>> Platform to me feels more like the 'base' ... (because you can stand on\n> >>>> a platform).\n> >>>>\n> >>>>> that it could be construed as a library aimed at abstracting differences\n> >>>>> between platforms. \"base\" may be another option, although I'm not sure\n> >>>>> to like it (the name comes from Chromium, where it has a similar role as\n> >>>>> the library you're creating here).\n> \n> base is growing on me. Reminds me of 'gstreamer-plugins-base' which is\n> used for common code on gstreamer plugins. But that then would make me\n> fear users expecting it to do more as well.\n> \n> There's never a perfect name ;-)\n> \n> Before these get merged, it will be easy to rename this from platform to\n> another name by running sed on the patches themselves and reapplying.\n> \n> So if you'd prefer another name, now's the time to shout, and we can\n> make it happen quicker now than later.\n\nI like base as well.\n\n> >>>> As mentioned in the cover letter, it could be that in the future indeed\n> >>>> we might need some platform abstraction. So that's again why I preferred\n> >>>> platform from the beginning.\n> >>>\n> >>> There would likely be a big overlap between this library and the\n> >>> platform abstraction, but I'm a bit concerned that some platform\n> >>> abstraction may need to go elsewhere. I see the goal for this library to\n> >>> be more of a foundation than a platform abstraction.\n> >>>\n> >>> Maybe libcamera-foundation is a possible name :-)\n> >>\n> >> Hrm ... that sounds like something else entirely ...\n> > \n> > :-)\n> > \n> >>>>>> However - the libcamera-platform library does not constitute a part\n> >>>>>> of the public libcamera API directly. It is a layer beneath libcamera\n> >>>>>> which provides common abstractions to internal objects.\n> >>>>>\n> >>>>> That's partly true only I'm afraid, looking at patch 4/6,\n> >>>>> bound_method.h, object.h and signal.h are moved to this library, and\n> >>>>> they're part of the libcamera public API. It's not a problem in itself.\n> >>>>\n> >>>> Argh - yes - that's annoying - and I had somewhat mis-considered this.\n> >>>>\n> >>>> This commit was written before I ended up pulling in libcamera public\n> >>>> headers, which I only hit deeper down the rabbit-hole ...\n> >>>>\n> >>>>>> Signed-off-by: Kieran Bingham \n> >>>>>> ---\n> >>>>>> Documentation/Doxyfile.in | 4 +++-\n> >>>>>> Documentation/meson.build | 2 ++\n> >>>>>> include/libcamera/meson.build | 1 +\n> >>>>>> include/libcamera/platform/meson.build | 9 ++++++++\n> >>>>>> src/libcamera-platform/meson.build | 29 ++++++++++++++++++++++++++\n> >>>>>> src/libcamera/meson.build | 2 ++\n> >>>>>> src/meson.build | 1 +\n> >>>>>> 7 files changed, 47 insertions(+), 1 deletion(-)\n> >>>>>> create mode 100644 include/libcamera/platform/meson.build\n> >>>>>> create mode 100644 src/libcamera-platform/meson.build\n> >>>>>>\n> >>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> >>>>>> index 8305f56af7a8..c1b395bf0b83 100644\n> >>>>>> --- a/Documentation/Doxyfile.in\n> >>>>>> +++ b/Documentation/Doxyfile.in\n> >>>>>> @@ -791,8 +791,10 @@ WARN_LOGFILE =\n> >>>>>> INPUT = \"@TOP_SRCDIR@/include/libcamera\" \\\n> >>>>>> \t\t\t \"@TOP_SRCDIR@/src/ipa/libipa\" \\\n> >>>>>> \t\t\t \"@TOP_SRCDIR@/src/libcamera\" \\\n> >>>>>> +\t\t\t \"@TOP_SRCDIR@/src/libcamera-platform\" \\\n> >>>>>> \t\t\t \"@TOP_BUILDDIR@/include/libcamera\" \\\n> >>>>>> -\t\t\t \"@TOP_BUILDDIR@/src/libcamera\"\n> >>>>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera\" \\\n> >>>>>> +\t\t\t \"@TOP_BUILDDIR@/src/libcamera-platform\"\n> >>>>>> \n> >>>>>\n> >>>>> Drive-by comment, I've been toying with the idea of splitting the\n> >>>>> Doxygen documentation between the public and internal API.\n> >>>>\n> >>>> I think that's likely a good idea.\n> >>>>\n> >>>> Hopefully we can get some more time to work on documentation\n> >>>> specifically as I think we need some more introduction type pages and\n> >>>> something to actually guide through the doxygen generated pages.\n> >>>\n> >>> Agreed.\n> >>>\n> >>>>>> # This tag can be used to specify the character encoding of the source files\n> >>>>>> # that doxygen parses. Internally doxygen uses the UTF-8 encoding. Doxygen uses\n> >>>>>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> >>>>>> index 9ecf4dfcf79f..01b753f07fb6 100644\n> >>>>>> --- a/Documentation/meson.build\n> >>>>>> +++ b/Documentation/meson.build\n> >>>>>> @@ -27,6 +27,8 @@ if doxygen.found() and dot.found()\n> >>>>>> libcamera_ipa_interfaces,\n> >>>>>> libcamera_public_headers,\n> >>>>>> libcamera_sources,\n> >>>>>> + libcamera_platform_headers,\n> >>>>>\n> >>>>> Following up on the comment in the commit message, do we need\n> >>>>> libcamera_platform_internal_headers and\n> >>>>> libcamera_platform_public_headers ? The distinction between the two\n> >>>>> would be different than for libcamera. We currently don't install the\n> >>>>> internal headers, while we'll need to install the \"internal platform\"\n> >>>>> headers as they can be used by IPA modules. What I'd like to avoid is\n> >>>>> giving an ABI stability guarantee for the whole platform library.\n> >>>>\n> >>>> I'm concerned that would be a mistake.\n> >>>> How can you have public-public headers and public-private-headers?\n> >>>>\n> >>>> All of the headers in this library are publicly exposed.\n> >>>>\n> >>>> I want to be able to say I think the risk of having ABI breakage on this\n> >>>> library component is lower though - because these are mostly just helper\n> >>>> objects, but I know if I were to actually say that, then we'd change the\n> >>>> ABI on say File or ... something ...\n> >>>>\n> >>>> I guess that begs the question of how will libcamera-platform be\n> >>>> versioned. I think as long as it's in the same repo as libcamera itself\n> >>>> - it would share the same versions, and I now have the\n> >>>> abi-compliance-checker which will identify if we do cause an ABI breakage.\n> >>>>\n> >>>> (I hope to see the ABI compliance checker as a pre-integration\n> >>>> validation stage).\n> >>>\n> >>> I think it should be versioned exactly as libcamera. That part doesn't\n> >>> worry me. What bothers me is that the platform headers will contain both\n> >>> stable and non-stable APIs (and ABIs), which will not be easy to convey.\n> >>> Applications will use signal.h, but we don't want them to use thread.h.\n> >>> Even if we document that they shouldn't, some will, and there will be\n> >>> breakages.\n> >>>\n> >>>>> In any case, it's possibly something we can handle later, but as this\n> >>>>> series makes quite a few internal headers public in libcamera-platform,\n> >>>>> I'm a bit worried we will start using them in the public libcamera API.\n> >>>>> Currently there's no risk of doing so by mistake, as the headers are\n> >>>>> clearly marked as internal by their location, and we would immediately\n> >>>>> spot during review an attempt to move an internal header to the public\n> >>>>> directory.\n> >>>>\n> >>>> Yes, having 'you can use this header' ... ' you can not use this header'\n> >>>> would become a bit awkward ...\n> >>>\n> >>> Random idea, how about adding a\n> >>>\n> >>> #ifndef LIBCAMERA_PLATFORM_PRIVATE\n> >>> #error ...\n> >>> #endif\n> >>>\n> >>> at the beginning of all private headers ? Both libcamera and IPA modules\n> >>> would define LIBCAMERA_PLATFORM_PRIVATE. cam and qcam wouldn't. We can\n> >>> bikeshed the LIBCAMERA_PLATFORM_PRIVATE macro name of course :-)\n> >>>\n> >>> This may not be the long term solution we want, but it would already\n> >>> avoid mistakes going unnoticed, and it's a simple change.\n> >>\n> >> That can prevent inclusion of headers which are not permitted indeed,\n> >> and if an application defines LIBCAMERA_PLATFORM_PRIVATE - then they're\n> >> on their own.\n> >>\n> >> It also gives the opportunity to present an appropriate error message\n> >> which I like too.\n> >>\n> >>\n> >> So we have two options:\n> >>\n> >> A)\n> >> Split platform headers into two locations, even though they are\n> >> 'public' to convey that they are not guaranteed to be stable ABI (I\n> >> fear how much we're shooting ourselves in the foot over this)\n> >>\n> >> #include \t\tPublic API\n> >> #include \tPublic Platform API\n> >> #include \tPublic but unstable ABI\n> >>\n> >>\n> >> B)\n> >> Keep platform headers together, but define a preprocessor guard\n> >>\n> >> #include \t Public API\n> >> #include Public Platform API\n> >> #include \t Requires '#define LC_P_PRIV'\n> >>\n> >> with s/LC_P_PRIV/LIBCAMERA_PLATFORM_PRIVATE/ of course. Shortened for\n> >> line length.\n> >>\n> >>\n> >> It's worth noting that both A and B can both be done together in fact,\n> >> or perhaps even maybe they should?\n> > \n> > I'd start with B as A will be another large patch. We can then see how\n> > we like it, and apply A later if needed. How does that sound ?\n> \n> B is fine.\n> \n> I will likely create a\n> \n> #include so that it's common.\n> Something like:\n> \n> \n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> * Copyright (C) 2021, Google Inc.\n> *\n> * private.h - Private Header Validation\n> *\n> * A selection of internal libcamera headers are installed as part\n> * of the libcamera package to allow sharing of a select subset of\n> * internal functionality with IPA developers only.\n\ns/IPA/IPA module/\n\n> *\n> * This functionality is not considered part of the public libcamera\n> * API, and can therefore potentially face ABI instabilities which\n> * should not be exposed to applications. IPAs however should be\n\nDitto, s/IPAs/IPA modules/\n\n> * versioned and more closely matched to the libcamera installation.\n> *\n> * Components which include this file can not be included in any file\n> * which forms part of the libcamera API.\n> */\n> \n> #ifndef LIBCAMERA_PLATFORM_PRIVATE\n> #error \"Private headers must not be included in the libcamera API\"\n> #endif\n> \n> \n> I've done some testing with this and it's quite effective, so I think\n> it's helpful.\n> \n> It's really showing up where the private API's are used ;-)\n\nSounds good.\n\n> >>>>>> + libcamera_platform_sources,\n> >>>>>> libipa_headers,\n> >>>>>> libipa_sources,\n> >>>>>> ],\n> >>>>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> >>>>>> index 086c958b0a53..2c3bbeb8df36 100644\n> >>>>>> --- a/include/libcamera/meson.build\n> >>>>>> +++ b/include/libcamera/meson.build\n> >>>>>> @@ -25,6 +25,7 @@ include_dir = libcamera_include_dir / 'libcamera'\n> >>>>>> \n> >>>>>> subdir('internal')\n> >>>>>> subdir('ipa')\n> >>>>>> +subdir('platform')\n> >>>>>> \n> >>>>>> install_headers(libcamera_public_headers,\n> >>>>>> subdir : include_dir)\n> >>>>>> diff --git a/include/libcamera/platform/meson.build b/include/libcamera/platform/meson.build\n> >>>>>> new file mode 100644\n> >>>>>> index 000000000000..c8e0d0c5ba12\n> >>>>>> --- /dev/null\n> >>>>>> +++ b/include/libcamera/platform/meson.build\n> >>>>>> @@ -0,0 +1,9 @@\n> >>>>>> +# SPDX-License-Identifier: CC0-1.0\n> >>>>>> +\n> >>>>>> +libcamera_platform_include_dir = libcamera_include_dir / 'platform'\n> >>>>>> +\n> >>>>>> +libcamera_platform_headers = files([\n> >>>>>> +])\n> >>>>>> +\n> >>>>>> +install_headers(libcamera_platform_headers,\n> >>>>>> + subdir: libcamera_platform_include_dir)\n> >>>>>> diff --git a/src/libcamera-platform/meson.build b/src/libcamera-platform/meson.build\n> >>>>>> new file mode 100644\n> >>>>>> index 000000000000..64d0dfee2731\n> >>>>>> --- /dev/null\n> >>>>>> +++ b/src/libcamera-platform/meson.build\n> >>>>>> @@ -0,0 +1,29 @@\n> >>>>>> +# SPDX-License-Identifier: CC0-1.0\n> >>>>>> +\n> >>>>>> +libcamera_platform_sources = files([\n> >>>>>> +])\n> >>>>>> +\n> >>>>>> +libcamera_platform_deps = [\n> >>>>>> +]\n> >>>>>> +\n> >>>>>> +libcamera_platform_lib = shared_library('libcamera_platform',\n> >>>>>\n> >>>>> $ ls -d /usr/lib64/lib*-[a-zA-Z]* | wc -l\n> >>>>> 664\n> >>>>> $ ls -d /usr/lib64/lib*_[a-zA-Z]* | wc -l\n> >>>>> 619\n> >>>>>\n> >>>>> Nearly a draw :-) I've checked because I would have sworn '-' was way\n> >>>>> more common than '_', but it seems it's a personal preference.\n> >>>>\n> >>>> This should be '-', I suspect the _ was a copy paste from the meson\n> >>>> variable name ;-(\n> >>>>\n> >>>>>> + [libcamera_platform_sources, libcamera_platform_headers],\n> >>>>>\n> >>>>> You're missing one space in the indentation.\n> >>>>\n> >>>> ack.\n> >>>>\n> >>>>>> + name_prefix : '',\n> >>>>>\n> >>>>> Any reason why you can't drop this line and use 'camera_platform' as the\n> >>>>> library name ?\n> >>>>\n> >>>> To be consistent with our libcamera meson file?\n> >>>\n> >>> We have\n> >>>\n> >>> libcamera = shared_library('camera',\n> >>> ...\n> >>>\n> >>> with no name_prefix.\n> >>\n> >> That's confused me ;-) Hrm:\n> >>\n> >> gg name_prefix\n> >> src/android/meson.build: name_prefix : '',\n> >> src/ipa/ipu3/meson.build: name_prefix : '',\n> >> src/ipa/raspberrypi/meson.build: name_prefix : '',\n> >> src/ipa/rkisp1/meson.build: name_prefix : '',\n> >> src/ipa/vimc/meson.build: name_prefix : '',\n> >> src/v4l2/meson.build: name_prefix : '',\n> > \n> > That's because all of those produce .so files that don't start with\n> > 'lib'.\n> > \n> >> I must have got the template from src/ipa instead or such then.\n> >>\n> >> I'll update to be the same as the main libcamera one indeed.\n> >>\n> >>>>>> + install : true,\n> >>>>>> + cpp_args : libcamera_cpp_args,\n> >>>>>> + include_directories : libcamera_includes,\n> >>>>>> + dependencies : libcamera_platform_deps)\n> >>>>>> +\n> >>>>>> +libcamera_platform = declare_dependency(sources : [\n> >>>>>> + libcamera_platform_headers,\n> >>>>>\n> >>>>> One space missing here too.\n> >>>>>\n> >>>>>> + ],\n> >>>>>> + include_directories : libcamera_includes,\n> >>>>>> + link_with : libcamera_platform_lib)\n> >>>>>\n> >>>>> Do we actually need this ? Wouldn't it be enough to just link libcamera\n> >>>>> (and the IPA modules) to libcamera_platform_lib ? The reason we have a\n> >>>>> libcamera_dep is because libcamera generates some headers, which needs\n> >>>>> to be done before anything compiling against those headers gets built.\n> >>>>> That's not needed for the platform library (at least not yet :-)).\n> >>>>>\n> >>>>>> +\n> >>>>>> +pkg_mod = import('pkgconfig')\n> >>>>>> +pkg_mod.generate(libraries : libcamera_platform_lib,\n> >>>>>> + version : '1.0',\n> >>>>>> + name : 'libcamera-platform',\n> >>>>>\n> >>>>> One more reason to name the binary libcamera-platform.so ;-)\n> >>>>\n> >>>> Yes, that was my intended name.\n> >>>>\n> >>>>>> + filebase : 'camera-platform',\n> >>>>>> + description : 'Complex Camera Support Library',\n> >>>>>\n> >>>>> This should be updated.\n> >>>>>\n> >>>>>> + subdirs : 'libcamera')\n> >>>>>\n> >>>>> I wonder if we should have a separate pkgconfig file for\n> >>>>> libcamera-platform, or include the library in the libcamera pkgconfig.\n> >>>>> It's an internal split really, and it wouldn't be nice to force\n> >>>>> applications to deal with the libcamera-platform pkgconfig explicitly.\n> >>>>>\n> >>>>> On the other hand, IPA modules will need this. Maybe we should do both,\n> >>>>\n> >>>> Yes - the point is that IPA's want to explicitly pull this in.\n> >>>\n> >>> Would it then make sense to have libcamera-platform in both the\n> >>> libcamera and libcamera-platform pkgconfig ?\n> >>\n> >> I think I'm missing something. libcamera already links against\n> >> libcamera-platform. Applications don't need to...\n> > \n> > If an application connects to a signal, it will use a symbol from\n> > libcamera-platform, and should thus link to it.\n> > \n> >> What 'part' of libcamera-platform do you want to put into the pkgconfig?\n> >>\n> >> The linker will already pull it in from the link required by libcamera.so.\n> > \n> > Are you sure about that ? At least for static linking (which we don't\n> > test yet), an application needs to explicitly link to all the libraries\n> > providing symbols it uses.\n> \n> Hrm... indeed - I can see how it would be needed for static.\n> We don't even build statically yet do we?\n\nNo we don't. Adding libcamera-platform.so (or libcamera-base.so ? :-))\nto the libcamera.pc shouldn't be a big deal though, so we could do so\nalready.\n\n> >> And the headers are already on a common path\n> >> #include \n> >>\n> >> Applications shouldn't need to know nor care about libcamera-platform.\n> > \n> > Exactly, that's why libcamera.pc should include a -lcamera-platform. No\n> > need for separate -L or -I entries.\n> > \n> >>>>> and a libcamera-platform.pc for IPA modules, and include\n> >>>>> libcamera-platform.so in the libraries of libcamera.pc ?\n> >>>>>\n> >>>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>>>>> index 54512652272c..6ba59e4006cb 100644\n> >>>>>> --- a/src/libcamera/meson.build\n> >>>>>> +++ b/src/libcamera/meson.build\n> >>>>>> @@ -127,6 +127,7 @@ libcamera_deps = [\n> >>>>>> libgnutls,\n> >>>>>> liblttng,\n> >>>>>> libudev,\n> >>>>>> + libcamera_platform,\n> >>>>>> dependency('threads'),\n> >>>>>> ]\n> >>>>>> \n> >>>>>> @@ -156,6 +157,7 @@ libcamera_dep = declare_dependency(sources : [\n> >>>>>> libcamera_generated_ipa_headers,\n> >>>>>> ],\n> >>>>>> include_directories : libcamera_includes,\n> >>>>>> + dependencies: libcamera_platform,\n> >>>>>> link_with : libcamera)\n> >>>>>> \n> >>>>>> subdir('proxy/worker')\n> >>>>>> diff --git a/src/meson.build b/src/meson.build\n> >>>>>> index a4e96ecd728a..70e1a4618a0f 100644\n> >>>>>> --- a/src/meson.build\n> >>>>>> +++ b/src/meson.build\n> >>>>>> @@ -29,6 +29,7 @@ libcamera_cpp_args = []\n> >>>>>> libcamera_objects = []\n> >>>>>> \n> >>>>>> # libcamera must be built first as a dependency to the other components.\n> >>>>>> +subdir('libcamera-platform')\n> >>>>>> subdir('libcamera')\n> >>>>>> \n> >>>>>> subdir('android')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C792EBD78E\n\tfor ;\n\tFri, 18 Jun 2021 10:32:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 423BA68945;\n\tFri, 18 Jun 2021 12:32:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CFCE760298\n\tfor ;\n\tFri, 18 Jun 2021 12:32:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21B683E5;\n\tFri, 18 Jun 2021 12:32:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XefErxMJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624012342;\n\tbh=gObHMp9l4y/q5i1jwiyYZ5IxvIS861C0POFizB9KnlI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XefErxMJUFVZ16LFnuCv7MqkaUdyUmcFljgQR95AxAUBIRiJA1c7JIeAaiZeRfhdb\n\twIY4XU6Zw+NacVYsAlCebdyOoB982FlDE/Djtke4A0ofTACrawWtfNBGOxQNVbiqLP\n\tHle3b2UNWDM4NP61v6vUVHK6PKrPeN+VHYKNyErg=","Date":"Fri, 18 Jun 2021 13:31:59 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210616151152.3856595-1-kieran.bingham@ideasonboard.com>\n\t<20210616151152.3856595-3-kieran.bingham@ideasonboard.com>\n\t\n\t<8eddf1f0-beb1-018b-3f06-dbee5ebcabf1@ideasonboard.com>\n\t\n\t<2b385b30-3fe6-353f-3712-eadea9684fca@ideasonboard.com>\n\t\n\t<078f4c29-d99d-a548-53d9-a918f28e6301@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<078f4c29-d99d-a548-53d9-a918f28e6301@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera-platform: Introduce new\n\tplatform library","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]