[{"id":27171,"web_url":"https://patchwork.libcamera.org/comment/27171/","msgid":"<20230530101222.GB19436@pendragon.ideasonboard.com>","date":"2023-05-30T10:12:22","subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Tue, May 30, 2023 at 12:20:16PM +0300, Tomi Valkeinen wrote:\n> We are using pybind11 'smart_holder' branch to solve the Camera\n> destructor issue (see the comment in this patch, or the commit\n> that originally added Python bindings support).\n> \n> As it would be very nice to use the mainline pybind11 (which is packaged\n> in distributions), this patch adds a workaround allowing us to move to\n> the mainline pybind11 version.\n> \n> The workaround is simply creating a custom holder class, used only for\n> the Camera, which wraps around the shared_ptr. This makes the compiler\n> happy.\n> \n> Moving to mainline pybdin11 is achieved with:\n\ns/pybdin11/pybind11/\n\n> \n> - Change the pybind11 wrap to point to the mainline pybdind11 version\n> \n> - Change the meson.build file to use a system-installed pybind11 if\n> available, and use the pybind11 subproject as a fallback. The fallback\n> is there as a convenience, as pybind11 may not be available in\n> pre-packaged form in all environments.\n\nGiven that Python bindings isn't a core feature of libcamera, do we\nreally need a subproject, or could we disable the Python bindings when\npybind11 isn't found ? We have subprojects for libyaml and libyuv, as\nthey're required by the libcamera core and the Android camera HAL and\nnot available on AOSP, but for optional features, I would prefer relying\non the build environment to provide the necessary dependencies,\nespecially given that both Debian and Fedora have a pybind11 package.\n\n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/meson.build | 11 ++--\n> src/py/libcamera/py_camera_manager.h | 2 +-\n> src/py/libcamera/py_color_space.cpp | 2 +-\n> src/py/libcamera/py_controls_generated.cpp.in | 2 +-\n> src/py/libcamera/py_enums.cpp | 2 +-\n> src/py/libcamera/py_formats_generated.cpp.in | 2 +-\n> src/py/libcamera/py_geometry.cpp | 2 +-\n> src/py/libcamera/py_helpers.h | 2 +-\n> src/py/libcamera/py_main.cpp | 52 +++++++++++++++++--\n> .../libcamera/py_properties_generated.cpp.in | 2 +-\n> src/py/libcamera/py_transform.cpp | 2 +-\n> subprojects/.gitignore | 2 +-\n> subprojects/pybind11.wrap | 18 ++++---\n> 13 files changed, 76 insertions(+), 25 deletions(-)\n> \n> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> index f87b1b4d..9d86d8bf 100644\n> --- a/src/py/libcamera/meson.build\n> +++ b/src/py/libcamera/meson.build\n> @@ -7,10 +7,15 @@ if not py3_dep.found()\n> subdir_done()\n> endif\n> \n> -pycamera_enabled = true\n> +pybind11_dep = dependency('pybind11', required : get_option('pycamera'),\n> + fallback : ['pybind11', 'pybind11_dep'])\n> +\n> +if not pybind11_dep.found()\n> + pycamera_enabled = false\n> + subdir_done()\n> +endif\n> \n> -pybind11_proj = subproject('pybind11')\n> -pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> +pycamera_enabled = true\n> \n> pycamera_sources = files([\n> 'py_camera_manager.cpp',\n> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> index 3525057d..3574db23 100644\n> --- a/src/py/libcamera/py_camera_manager.h\n> +++ b/src/py/libcamera/py_camera_manager.h\n> @@ -9,7 +9,7 @@\n> \n> #include \n> \n> -#include \n> +#include \n> \n> using namespace libcamera;\n> \n> diff --git a/src/py/libcamera/py_color_space.cpp b/src/py/libcamera/py_color_space.cpp\n> index a8301e3d..5201121a 100644\n> --- a/src/py/libcamera/py_color_space.cpp\n> +++ b/src/py/libcamera/py_color_space.cpp\n> @@ -9,7 +9,7 @@\n> #include \n> \n> #include \n> -#include \n> +#include \n> #include \n> \n> namespace py = pybind11;\n> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in\n> index cb8442ba..18fa57d9 100644\n> --- a/src/py/libcamera/py_controls_generated.cpp.in\n> +++ b/src/py/libcamera/py_controls_generated.cpp.in\n> @@ -9,7 +9,7 @@\n> \n> #include \n> \n> -#include \n> +#include \n> \n> namespace py = pybind11;\n> \n> diff --git a/src/py/libcamera/py_enums.cpp b/src/py/libcamera/py_enums.cpp\n> index 96d4beef..803c4e7e 100644\n> --- a/src/py/libcamera/py_enums.cpp\n> +++ b/src/py/libcamera/py_enums.cpp\n> @@ -7,7 +7,7 @@\n> \n> #include \n> \n> -#include \n> +#include \n> \n> namespace py = pybind11;\n> \n> diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in\n> index b88807f3..a3f7f94d 100644\n> --- a/src/py/libcamera/py_formats_generated.cpp.in\n> +++ b/src/py/libcamera/py_formats_generated.cpp.in\n> @@ -9,7 +9,7 @@\n> \n> #include \n> \n> -#include \n> +#include \n> \n> namespace py = pybind11;\n> \n> diff --git a/src/py/libcamera/py_geometry.cpp b/src/py/libcamera/py_geometry.cpp\n> index 84b0cb08..5c2aeac4 100644\n> --- a/src/py/libcamera/py_geometry.cpp\n> +++ b/src/py/libcamera/py_geometry.cpp\n> @@ -11,7 +11,7 @@\n> #include \n> \n> #include \n> -#include \n> +#include \n> #include \n> \n> namespace py = pybind11;\n> diff --git a/src/py/libcamera/py_helpers.h b/src/py/libcamera/py_helpers.h\n> index cd31e2cc..983969df 100644\n> --- a/src/py/libcamera/py_helpers.h\n> +++ b/src/py/libcamera/py_helpers.h\n> @@ -7,7 +7,7 @@\n> \n> #include \n> \n> -#include \n> +#include \n> \n> pybind11::object controlValueToPy(const libcamera::ControlValue &cv);\n> libcamera::ControlValue pyToControlValue(const pybind11::object &ob, libcamera::ControlType type);\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index c66b90cd..bbae7a99 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -17,7 +17,7 @@\n> #include \n> \n> #include \n> -#include \n> +#include \n> #include \n> #include \n> \n> @@ -33,6 +33,50 @@ namespace libcamera {\n> LOG_DEFINE_CATEGORY(Python)\n> \n> }\n\nMissing blank line.\n\n> +/*\n> + * This is a holder class used only for the Camera class, for the sole purpose\n> + * of avoiding the compilation issue with Camera's private destructor.\n> + *\n> + * pybind11 requires a public destructor for classes held with shared_ptrs, even\n> + * in cases where the public destructor is not strictly needed. The current\n> + * understanding is that there are the following options to solve the problem:\n> + *\n> + * - Use pybind11 'smart_holder' branch. The downside is that 'smart_holder'\n> + * is not the mainline branch, and not available in distributions.\n> + * - https://github.com/pybind/pybind11/pull/2067\n> + * - Make the Camera destructor public\n> + * - Something like the PyCameraSmartPtr here, which adds a layer, hiding the\n> + * issue.\n> + */\n> +template\n> +class PyCameraSmartPtr\n> +{\n> +public:\n> +\tusing element_type = T;\n> +\n> +\tPyCameraSmartPtr()\n> +\t{\n> +\t}\n> +\n> +\texplicit PyCameraSmartPtr(T *)\n> +\t{\n> +\t\tthrow std::runtime_error(\"invalid SmartPtr constructor call\");\n> +\t}\n> +\n> +\texplicit PyCameraSmartPtr(std::shared_ptr p)\n> +\t\t: ptr_(p)\n> +\t{\n> +\t}\n> +\n> +\tT *get() const { return ptr_.get(); }\n> +\n> +\toperator std::shared_ptr() const { return ptr_; }\n> +\n> +private:\n> +\tstd::shared_ptr ptr_;\n> +};\n> +\n> +PYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr);\n> \n> /*\n> * Note: global C++ destructors can be ran on this before the py module is\n> @@ -65,8 +109,8 @@ PYBIND11_MODULE(_libcamera, m)\n> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> \t */\n> \n> -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> -\tauto pyCamera = py::class_(m, \"Camera\");\n> +\tauto pyCameraManager = py::class_>(m, \"CameraManager\");\n\nWhy does PyCameraManager need to be changed ?\n\n> +\tauto pyCamera = py::class_>(m, \"Camera\");\n> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> \tauto pyStreamConfiguration = py::class_(m, \"StreamConfiguration\");\n> @@ -271,7 +315,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t.def(\"range\", &StreamFormats::range);\n> \n> \tpyFrameBufferAllocator\n> -\t\t.def(py::init>(), py::keep_alive<1, 2>())\n> +\t\t.def(py::init>(), py::keep_alive<1, 2>())\n> \t\t.def(\"allocate\", [](FrameBufferAllocator &self, Stream *stream) {\n> \t\t\tint ret = self.allocate(stream);\n> \t\t\tif (ret < 0)\n> diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in\n> index 044b2b2a..e49b6e91 100644\n> --- a/src/py/libcamera/py_properties_generated.cpp.in\n> +++ b/src/py/libcamera/py_properties_generated.cpp.in\n> @@ -9,7 +9,7 @@\n> \n> #include \n> \n> -#include \n> +#include \n> \n> namespace py = pybind11;\n> \n> diff --git a/src/py/libcamera/py_transform.cpp b/src/py/libcamera/py_transform.cpp\n> index 08783e29..f3a0bfaf 100644\n> --- a/src/py/libcamera/py_transform.cpp\n> +++ b/src/py/libcamera/py_transform.cpp\n> @@ -9,7 +9,7 @@\n> #include \n> \n> #include \n> -#include \n> +#include \n> #include \n> \n> namespace py = pybind11;\n> diff --git a/subprojects/.gitignore b/subprojects/.gitignore\n> index 0a8fd3a6..ebe59479 100644\n> --- a/subprojects/.gitignore\n> +++ b/subprojects/.gitignore\n> @@ -4,4 +4,4 @@\n> /libyaml\n> /libyuv\n> /packagecache\n> -/pybind11\n> +/pybind11-*\n> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap\n> index dd02687b..96ec57a1 100644\n> --- a/subprojects/pybind11.wrap\n> +++ b/subprojects/pybind11.wrap\n> @@ -1,11 +1,13 @@\n> -# SPDX-License-Identifier: CC0-1.0\n> -\n\nPlease specify a license.\n\n> -[wrap-git]\n> -url = https://github.com/pybind/pybind11.git\n> -# This is the head of 'smart_holder' branch\n> -revision = aebdf00cd060b871c5a1e0c2cf4a333503dd0431\n> -depth = 1\n> -patch_directory = pybind11\n> +[wrap-file]\n> +directory = pybind11-2.10.4\n> +source_url = https://github.com/pybind/pybind11/archive/refs/tags/v2.10.4.tar.gz\n> +source_filename = pybind11-2.10.4.tar.gz\n> +source_hash = 832e2f309c57da9c1e6d4542dedd34b24e4192ecb4d62f6f4866a737454c9970\n> +patch_filename = pybind11_2.10.4-1_patch.zip\n> +patch_url = https://wrapdb.mesonbuild.com/v2/pybind11_2.10.4-1/get_patch\n> +patch_hash = 9489d0cdc1244078a3108c52b4591a6f07f3dc30ca7299d3a3c42b84fa763396\n> +source_fallback_url = https://github.com/mesonbuild/wrapdb/releases/download/pybind11_2.10.4-1/pybind11-2.10.4.tar.gz\n> +wrapdb_version = 2.10.4-1\n> \n> [provide]\n> pybind11 = pybind11_dep","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 20B25C31E9\n\tfor ;\n\tTue, 30 May 2023 10:12:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 748F660599;\n\tTue, 30 May 2023 12:12: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 13B3760595\n\tfor ;\n\tTue, 30 May 2023 12:12:23 +0200 (CEST)","from pendragon.ideasonboard.com (om126205206011.34.openmobile.ne.jp\n\t[126.205.206.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 26EB2E4;\n\tTue, 30 May 2023 12:12:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685441544;\n\tbh=f2Y7YBNiln792vSQ0h6ND8qNath2Jli4zPJqyKmsL60=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=TcIhCTwlUWRTOuCxdzfCDPmVp9pAYYmo/3iFaDyzOvSkcNlLqPcpki0VzG/+ZeasY\n\tuGI1BxYrazEEmt9yJRnXXLgK6BIWFAxpuTgyFFBh+6fsqcGOLVFEfIvl5b0tiyQfX7\n\trCJ31NoUEOOpK6TMwvh0D3Qge8jL5qaYF6UBapIvULcBuulxMT8g0EEkiZB/XITgBg\n\tz5NLua4sNA2nOd+5Hljc0ZkAHOqAnsJg7qEX49GLFxy2mH21fmht8vPfWn2GRWfQMh\n\t1xeOhTBcsOPF344efUb04tMHbpqfk1w05QzOSIrzBuCT1jWAnhWTWL+Tp/b1fIcS/q\n\tLEXGr3pgixPCQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685441522;\n\tbh=f2Y7YBNiln792vSQ0h6ND8qNath2Jli4zPJqyKmsL60=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A9/EE0dz2Sc1AE5ZWImMznA7ivZLNo+6xx9oFrZ8b2eg4FumbgXM8XdHIf67weuEi\n\tKurSx6zIL/abqQJPH2EyY2WDE017fOJUVhi/fKMjP1Eo+C5/fpCMMOWHm7PuDnJCIl\n\tNxfdcciEuoUCWWR2rc4w+V26+8xG1oi7xC974Ygg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"A9/EE0dz\"; dkim-atps=neutral","Date":"Tue, 30 May 2023 13:12:22 +0300","To":"Tomi Valkeinen ","Message-ID":"<20230530101222.GB19436@pendragon.ideasonboard.com>","References":"<20230530092016.34953-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530092016.34953-6-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230530092016.34953-6-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27173,"web_url":"https://patchwork.libcamera.org/comment/27173/","msgid":"<139fd0f5-44aa-7671-45aa-dad91f03691e@ideasonboard.com>","date":"2023-05-30T11:16:23","subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/05/2023 13:12, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Tue, May 30, 2023 at 12:20:16PM +0300, Tomi Valkeinen wrote:\n>> We are using pybind11 'smart_holder' branch to solve the Camera\n>> destructor issue (see the comment in this patch, or the commit\n>> that originally added Python bindings support).\n>>\n>> As it would be very nice to use the mainline pybind11 (which is packaged\n>> in distributions), this patch adds a workaround allowing us to move to\n>> the mainline pybind11 version.\n>>\n>> The workaround is simply creating a custom holder class, used only for\n>> the Camera, which wraps around the shared_ptr. This makes the compiler\n>> happy.\n>>\n>> Moving to mainline pybdin11 is achieved with:\n> \n> s/pybdin11/pybind11/\n> \n>>\n>> - Change the pybind11 wrap to point to the mainline pybdind11 version\n>>\n>> - Change the meson.build file to use a system-installed pybind11 if\n>> available, and use the pybind11 subproject as a fallback. The fallback\n>> is there as a convenience, as pybind11 may not be available in\n>> pre-packaged form in all environments.\n> \n> Given that Python bindings isn't a core feature of libcamera, do we\n> really need a subproject, or could we disable the Python bindings when\n> pybind11 isn't found ? We have subprojects for libyaml and libyuv, as\n> they're required by the libcamera core and the Android camera HAL and\n> not available on AOSP, but for optional features, I would prefer relying\n> on the build environment to provide the necessary dependencies,\n> especially given that both Debian and Fedora have a pybind11 package.\n\nWell... It's for my convenience, at least =). Buildroot doesn't/didn't \nhave a properly working pybind11.\n\nMaybe we can drop it, but I'd rather do it later so that we don't mess \nup too many things at the same time.\n\nOr I can just add a patch to my local tree which adds the subproject, if \nthe consensus is to drop it from libcamera.\n\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/libcamera/meson.build | 11 ++--\n>> src/py/libcamera/py_camera_manager.h | 2 +-\n>> src/py/libcamera/py_color_space.cpp | 2 +-\n>> src/py/libcamera/py_controls_generated.cpp.in | 2 +-\n>> src/py/libcamera/py_enums.cpp | 2 +-\n>> src/py/libcamera/py_formats_generated.cpp.in | 2 +-\n>> src/py/libcamera/py_geometry.cpp | 2 +-\n>> src/py/libcamera/py_helpers.h | 2 +-\n>> src/py/libcamera/py_main.cpp | 52 +++++++++++++++++--\n>> .../libcamera/py_properties_generated.cpp.in | 2 +-\n>> src/py/libcamera/py_transform.cpp | 2 +-\n>> subprojects/.gitignore | 2 +-\n>> subprojects/pybind11.wrap | 18 ++++---\n>> 13 files changed, 76 insertions(+), 25 deletions(-)\n>>\n>> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n>> index f87b1b4d..9d86d8bf 100644\n>> --- a/src/py/libcamera/meson.build\n>> +++ b/src/py/libcamera/meson.build\n>> @@ -7,10 +7,15 @@ if not py3_dep.found()\n>> subdir_done()\n>> endif\n>> \n>> -pycamera_enabled = true\n>> +pybind11_dep = dependency('pybind11', required : get_option('pycamera'),\n>> + fallback : ['pybind11', 'pybind11_dep'])\n>> +\n>> +if not pybind11_dep.found()\n>> + pycamera_enabled = false\n>> + subdir_done()\n>> +endif\n>> \n>> -pybind11_proj = subproject('pybind11')\n>> -pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n>> +pycamera_enabled = true\n>> \n>> pycamera_sources = files([\n>> 'py_camera_manager.cpp',\n>> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n>> index 3525057d..3574db23 100644\n>> --- a/src/py/libcamera/py_camera_manager.h\n>> +++ b/src/py/libcamera/py_camera_manager.h\n>> @@ -9,7 +9,7 @@\n>> \n>> #include \n>> \n>> -#include \n>> +#include \n>> \n>> using namespace libcamera;\n>> \n>> diff --git a/src/py/libcamera/py_color_space.cpp b/src/py/libcamera/py_color_space.cpp\n>> index a8301e3d..5201121a 100644\n>> --- a/src/py/libcamera/py_color_space.cpp\n>> +++ b/src/py/libcamera/py_color_space.cpp\n>> @@ -9,7 +9,7 @@\n>> #include \n>> \n>> #include \n>> -#include \n>> +#include \n>> #include \n>> \n>> namespace py = pybind11;\n>> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in\n>> index cb8442ba..18fa57d9 100644\n>> --- a/src/py/libcamera/py_controls_generated.cpp.in\n>> +++ b/src/py/libcamera/py_controls_generated.cpp.in\n>> @@ -9,7 +9,7 @@\n>> \n>> #include \n>> \n>> -#include \n>> +#include \n>> \n>> namespace py = pybind11;\n>> \n>> diff --git a/src/py/libcamera/py_enums.cpp b/src/py/libcamera/py_enums.cpp\n>> index 96d4beef..803c4e7e 100644\n>> --- a/src/py/libcamera/py_enums.cpp\n>> +++ b/src/py/libcamera/py_enums.cpp\n>> @@ -7,7 +7,7 @@\n>> \n>> #include \n>> \n>> -#include \n>> +#include \n>> \n>> namespace py = pybind11;\n>> \n>> diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in\n>> index b88807f3..a3f7f94d 100644\n>> --- a/src/py/libcamera/py_formats_generated.cpp.in\n>> +++ b/src/py/libcamera/py_formats_generated.cpp.in\n>> @@ -9,7 +9,7 @@\n>> \n>> #include \n>> \n>> -#include \n>> +#include \n>> \n>> namespace py = pybind11;\n>> \n>> diff --git a/src/py/libcamera/py_geometry.cpp b/src/py/libcamera/py_geometry.cpp\n>> index 84b0cb08..5c2aeac4 100644\n>> --- a/src/py/libcamera/py_geometry.cpp\n>> +++ b/src/py/libcamera/py_geometry.cpp\n>> @@ -11,7 +11,7 @@\n>> #include \n>> \n>> #include \n>> -#include \n>> +#include \n>> #include \n>> \n>> namespace py = pybind11;\n>> diff --git a/src/py/libcamera/py_helpers.h b/src/py/libcamera/py_helpers.h\n>> index cd31e2cc..983969df 100644\n>> --- a/src/py/libcamera/py_helpers.h\n>> +++ b/src/py/libcamera/py_helpers.h\n>> @@ -7,7 +7,7 @@\n>> \n>> #include \n>> \n>> -#include \n>> +#include \n>> \n>> pybind11::object controlValueToPy(const libcamera::ControlValue &cv);\n>> libcamera::ControlValue pyToControlValue(const pybind11::object &ob, libcamera::ControlType type);\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index c66b90cd..bbae7a99 100644\n>> --- a/src/py/libcamera/py_main.cpp\n>> +++ b/src/py/libcamera/py_main.cpp\n>> @@ -17,7 +17,7 @@\n>> #include \n>> \n>> #include \n>> -#include \n>> +#include \n>> #include \n>> #include \n>> \n>> @@ -33,6 +33,50 @@ namespace libcamera {\n>> LOG_DEFINE_CATEGORY(Python)\n>> \n>> }\n> \n> Missing blank line.\n> \n>> +/*\n>> + * This is a holder class used only for the Camera class, for the sole purpose\n>> + * of avoiding the compilation issue with Camera's private destructor.\n>> + *\n>> + * pybind11 requires a public destructor for classes held with shared_ptrs, even\n>> + * in cases where the public destructor is not strictly needed. The current\n>> + * understanding is that there are the following options to solve the problem:\n>> + *\n>> + * - Use pybind11 'smart_holder' branch. The downside is that 'smart_holder'\n>> + * is not the mainline branch, and not available in distributions.\n>> + * - https://github.com/pybind/pybind11/pull/2067\n>> + * - Make the Camera destructor public\n>> + * - Something like the PyCameraSmartPtr here, which adds a layer, hiding the\n>> + * issue.\n>> + */\n>> +template\n>> +class PyCameraSmartPtr\n>> +{\n>> +public:\n>> +\tusing element_type = T;\n>> +\n>> +\tPyCameraSmartPtr()\n>> +\t{\n>> +\t}\n>> +\n>> +\texplicit PyCameraSmartPtr(T *)\n>> +\t{\n>> +\t\tthrow std::runtime_error(\"invalid SmartPtr constructor call\");\n>> +\t}\n>> +\n>> +\texplicit PyCameraSmartPtr(std::shared_ptr p)\n>> +\t\t: ptr_(p)\n>> +\t{\n>> +\t}\n>> +\n>> +\tT *get() const { return ptr_.get(); }\n>> +\n>> +\toperator std::shared_ptr() const { return ptr_; }\n>> +\n>> +private:\n>> +\tstd::shared_ptr ptr_;\n>> +};\n>> +\n>> +PYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr);\n>> \n>> /*\n>> * Note: global C++ destructors can be ran on this before the py module is\n>> @@ -65,8 +109,8 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n>> \t */\n>> \n>> -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n>> -\tauto pyCamera = py::class_(m, \"Camera\");\n>> +\tauto pyCameraManager = py::class_>(m, \"CameraManager\");\n> \n> Why does PyCameraManager need to be changed ?\n\nThe default holder is unique_ptr. We handle the camera manager with the \nsingleton pattern, using shared_ptr. So we always want to use shared_ptr.\n\nIf we don't do this, the camera manager gets freed during operation, \nleading to a crash.\n\nI have to say the exact details escape me, but the smart_holder version \nhandles this smartly, as we do return a shared_ptr from the singleton() \nfunction. But the mainline version doesn't cope with that, and I believe \nit converts the shared_ptr to a unique_ptr, unless we specifically tell \nit to use the shared_ptr as a holder class.\n\nI'll add a note about this to the commit message.\n\n>> +\tauto pyCamera = py::class_>(m, \"Camera\");\n>> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n>> \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n>> \tauto pyStreamConfiguration = py::class_(m, \"StreamConfiguration\");\n>> @@ -271,7 +315,7 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t.def(\"range\", &StreamFormats::range);\n>> \n>> \tpyFrameBufferAllocator\n>> -\t\t.def(py::init>(), py::keep_alive<1, 2>())\n>> +\t\t.def(py::init>(), py::keep_alive<1, 2>())\n>> \t\t.def(\"allocate\", [](FrameBufferAllocator &self, Stream *stream) {\n>> \t\t\tint ret = self.allocate(stream);\n>> \t\t\tif (ret < 0)\n>> diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in\n>> index 044b2b2a..e49b6e91 100644\n>> --- a/src/py/libcamera/py_properties_generated.cpp.in\n>> +++ b/src/py/libcamera/py_properties_generated.cpp.in\n>> @@ -9,7 +9,7 @@\n>> \n>> #include \n>> \n>> -#include \n>> +#include \n>> \n>> namespace py = pybind11;\n>> \n>> diff --git a/src/py/libcamera/py_transform.cpp b/src/py/libcamera/py_transform.cpp\n>> index 08783e29..f3a0bfaf 100644\n>> --- a/src/py/libcamera/py_transform.cpp\n>> +++ b/src/py/libcamera/py_transform.cpp\n>> @@ -9,7 +9,7 @@\n>> #include \n>> \n>> #include \n>> -#include \n>> +#include \n>> #include \n>> \n>> namespace py = pybind11;\n>> diff --git a/subprojects/.gitignore b/subprojects/.gitignore\n>> index 0a8fd3a6..ebe59479 100644\n>> --- a/subprojects/.gitignore\n>> +++ b/subprojects/.gitignore\n>> @@ -4,4 +4,4 @@\n>> /libyaml\n>> /libyuv\n>> /packagecache\n>> -/pybind11\n>> +/pybind11-*\n>> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap\n>> index dd02687b..96ec57a1 100644\n>> --- a/subprojects/pybind11.wrap\n>> +++ b/subprojects/pybind11.wrap\n>> @@ -1,11 +1,13 @@\n>> -# SPDX-License-Identifier: CC0-1.0\n>> -\n> \n> Please specify a license.\n\nHmm, I wonder what it is... This file is downloaded with the \"meson wrap \ninstall\" tool. https://github.com/mesonbuild/wrapdb says \"MIT\", so \nperhaps it's that. I hope it won't get overwritten every time we update \nthe wrap with \"meson wrap update\"...\n\n Tomi","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 F0D97C3213\n\tfor ;\n\tTue, 30 May 2023 11:16:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CD1A626F8;\n\tTue, 30 May 2023 13:16:29 +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 5D8D460595\n\tfor ;\n\tTue, 30 May 2023 13:16:27 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4DB427EC;\n\tTue, 30 May 2023 13:16:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685445389;\n\tbh=46kcMj5xqtwpfhDl2lAjQg5WlNkfrVW7WhlJj3zFFuA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=LHg9ZUHK3S5H6irS+veA5jQwgLC609+k6M1H+TrEJknGitquDSyGSa8aPhJC0ZY4r\n\tNzy3BR42xZdz8kiCiBWXKwQx7Gkm+thbt8AT4AyCobmN+XpUgDTfK8tncAWVsZ+1NF\n\tqs5qIt4JAUZzWbbVL7he/6URruMIrVH7/+DxmwbuDHcOsynR+KS/C0uYROdKY7asJ+\n\tMvcncA+SDBVJ3rmVPiQaGxNGbLqjEcmAhZS8J1AKRcfF5/9H7k9d0AT8Z+mikC6hTP\n\tDSB82LYygXS3emFqKHK1DVN0EFmatjWnuSW2+gTCulPHymYhsz/DVzyV3KnogbKLT6\n\tZVlXlJ348G3bg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685445366;\n\tbh=46kcMj5xqtwpfhDl2lAjQg5WlNkfrVW7WhlJj3zFFuA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=kAf/NUxlqj4waWz9it/xT72S6lXQ8G/stwSidzglhLcPvW9GcNFPSNUKmustUogb4\n\tCYW5rUP7ZGoBUVlKM8x3fqD01dN1bmPdxaGQ4wtjr05cea85x9fGdVy8JwTTiijUsw\n\teLhA2UQTyNAxNW+oaSWePIW07QAvSN6KGUosKALc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kAf/NUxl\"; dkim-atps=neutral","Message-ID":"<139fd0f5-44aa-7671-45aa-dad91f03691e@ideasonboard.com>","Date":"Tue, 30 May 2023 14:16:23 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.11.0","To":"Laurent Pinchart ","References":"<20230530092016.34953-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530092016.34953-6-tomi.valkeinen@ideasonboard.com>\n\t<20230530101222.GB19436@pendragon.ideasonboard.com>","Content-Language":"en-US","In-Reply-To":"<20230530101222.GB19436@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27174,"web_url":"https://patchwork.libcamera.org/comment/27174/","msgid":"<20230530112905.GA22516@pendragon.ideasonboard.com>","date":"2023-05-30T11:29:05","subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Tue, May 30, 2023 at 02:16:23PM +0300, Tomi Valkeinen wrote:\n> On 30/05/2023 13:12, Laurent Pinchart wrote:\n> > On Tue, May 30, 2023 at 12:20:16PM +0300, Tomi Valkeinen wrote:\n> >> We are using pybind11 'smart_holder' branch to solve the Camera\n> >> destructor issue (see the comment in this patch, or the commit\n> >> that originally added Python bindings support).\n> >>\n> >> As it would be very nice to use the mainline pybind11 (which is packaged\n> >> in distributions), this patch adds a workaround allowing us to move to\n> >> the mainline pybind11 version.\n> >>\n> >> The workaround is simply creating a custom holder class, used only for\n> >> the Camera, which wraps around the shared_ptr. This makes the compiler\n> >> happy.\n> >>\n> >> Moving to mainline pybdin11 is achieved with:\n> > \n> > s/pybdin11/pybind11/\n> > \n> >>\n> >> - Change the pybind11 wrap to point to the mainline pybdind11 version\n> >>\n> >> - Change the meson.build file to use a system-installed pybind11 if\n> >> available, and use the pybind11 subproject as a fallback. The fallback\n> >> is there as a convenience, as pybind11 may not be available in\n> >> pre-packaged form in all environments.\n> > \n> > Given that Python bindings isn't a core feature of libcamera, do we\n> > really need a subproject, or could we disable the Python bindings when\n> > pybind11 isn't found ? We have subprojects for libyaml and libyuv, as\n> > they're required by the libcamera core and the Android camera HAL and\n> > not available on AOSP, but for optional features, I would prefer relying\n> > on the build environment to provide the necessary dependencies,\n> > especially given that both Debian and Fedora have a pybind11 package.\n> \n> Well... It's for my convenience, at least =). Buildroot doesn't/didn't \n> have a properly working pybind11.\n\nI think the package is called python-pybind in buildroot.\n\n> Maybe we can drop it, but I'd rather do it later so that we don't mess \n> up too many things at the same time.\n> \n> Or I can just add a patch to my local tree which adds the subproject, if \n> the consensus is to drop it from libcamera.\n\nCould you test with the buildroot python-pybind package ? If that works,\nis there any blocker to dropping the subproject ?\n\nI don't mind dropping the subproject in a separate patch, to make it\neasy to revert it.\n\n> >> Signed-off-by: Tomi Valkeinen \n> >> ---\n> >> src/py/libcamera/meson.build | 11 ++--\n> >> src/py/libcamera/py_camera_manager.h | 2 +-\n> >> src/py/libcamera/py_color_space.cpp | 2 +-\n> >> src/py/libcamera/py_controls_generated.cpp.in | 2 +-\n> >> src/py/libcamera/py_enums.cpp | 2 +-\n> >> src/py/libcamera/py_formats_generated.cpp.in | 2 +-\n> >> src/py/libcamera/py_geometry.cpp | 2 +-\n> >> src/py/libcamera/py_helpers.h | 2 +-\n> >> src/py/libcamera/py_main.cpp | 52 +++++++++++++++++--\n> >> .../libcamera/py_properties_generated.cpp.in | 2 +-\n> >> src/py/libcamera/py_transform.cpp | 2 +-\n> >> subprojects/.gitignore | 2 +-\n> >> subprojects/pybind11.wrap | 18 ++++---\n> >> 13 files changed, 76 insertions(+), 25 deletions(-)\n> >>\n> >> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> >> index f87b1b4d..9d86d8bf 100644\n> >> --- a/src/py/libcamera/meson.build\n> >> +++ b/src/py/libcamera/meson.build\n> >> @@ -7,10 +7,15 @@ if not py3_dep.found()\n> >> subdir_done()\n> >> endif\n> >> \n> >> -pycamera_enabled = true\n> >> +pybind11_dep = dependency('pybind11', required : get_option('pycamera'),\n> >> + fallback : ['pybind11', 'pybind11_dep'])\n> >> +\n> >> +if not pybind11_dep.found()\n> >> + pycamera_enabled = false\n> >> + subdir_done()\n> >> +endif\n> >> \n> >> -pybind11_proj = subproject('pybind11')\n> >> -pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> >> +pycamera_enabled = true\n> >> \n> >> pycamera_sources = files([\n> >> 'py_camera_manager.cpp',\n> >> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> >> index 3525057d..3574db23 100644\n> >> --- a/src/py/libcamera/py_camera_manager.h\n> >> +++ b/src/py/libcamera/py_camera_manager.h\n> >> @@ -9,7 +9,7 @@\n> >> \n> >> #include \n> >> \n> >> -#include \n> >> +#include \n> >> \n> >> using namespace libcamera;\n> >> \n> >> diff --git a/src/py/libcamera/py_color_space.cpp b/src/py/libcamera/py_color_space.cpp\n> >> index a8301e3d..5201121a 100644\n> >> --- a/src/py/libcamera/py_color_space.cpp\n> >> +++ b/src/py/libcamera/py_color_space.cpp\n> >> @@ -9,7 +9,7 @@\n> >> #include \n> >> \n> >> #include \n> >> -#include \n> >> +#include \n> >> #include \n> >> \n> >> namespace py = pybind11;\n> >> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in\n> >> index cb8442ba..18fa57d9 100644\n> >> --- a/src/py/libcamera/py_controls_generated.cpp.in\n> >> +++ b/src/py/libcamera/py_controls_generated.cpp.in\n> >> @@ -9,7 +9,7 @@\n> >> \n> >> #include \n> >> \n> >> -#include \n> >> +#include \n> >> \n> >> namespace py = pybind11;\n> >> \n> >> diff --git a/src/py/libcamera/py_enums.cpp b/src/py/libcamera/py_enums.cpp\n> >> index 96d4beef..803c4e7e 100644\n> >> --- a/src/py/libcamera/py_enums.cpp\n> >> +++ b/src/py/libcamera/py_enums.cpp\n> >> @@ -7,7 +7,7 @@\n> >> \n> >> #include \n> >> \n> >> -#include \n> >> +#include \n> >> \n> >> namespace py = pybind11;\n> >> \n> >> diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in\n> >> index b88807f3..a3f7f94d 100644\n> >> --- a/src/py/libcamera/py_formats_generated.cpp.in\n> >> +++ b/src/py/libcamera/py_formats_generated.cpp.in\n> >> @@ -9,7 +9,7 @@\n> >> \n> >> #include \n> >> \n> >> -#include \n> >> +#include \n> >> \n> >> namespace py = pybind11;\n> >> \n> >> diff --git a/src/py/libcamera/py_geometry.cpp b/src/py/libcamera/py_geometry.cpp\n> >> index 84b0cb08..5c2aeac4 100644\n> >> --- a/src/py/libcamera/py_geometry.cpp\n> >> +++ b/src/py/libcamera/py_geometry.cpp\n> >> @@ -11,7 +11,7 @@\n> >> #include \n> >> \n> >> #include \n> >> -#include \n> >> +#include \n> >> #include \n> >> \n> >> namespace py = pybind11;\n> >> diff --git a/src/py/libcamera/py_helpers.h b/src/py/libcamera/py_helpers.h\n> >> index cd31e2cc..983969df 100644\n> >> --- a/src/py/libcamera/py_helpers.h\n> >> +++ b/src/py/libcamera/py_helpers.h\n> >> @@ -7,7 +7,7 @@\n> >> \n> >> #include \n> >> \n> >> -#include \n> >> +#include \n> >> \n> >> pybind11::object controlValueToPy(const libcamera::ControlValue &cv);\n> >> libcamera::ControlValue pyToControlValue(const pybind11::object &ob, libcamera::ControlType type);\n> >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> >> index c66b90cd..bbae7a99 100644\n> >> --- a/src/py/libcamera/py_main.cpp\n> >> +++ b/src/py/libcamera/py_main.cpp\n> >> @@ -17,7 +17,7 @@\n> >> #include \n> >> \n> >> #include \n> >> -#include \n> >> +#include \n> >> #include \n> >> #include \n> >> \n> >> @@ -33,6 +33,50 @@ namespace libcamera {\n> >> LOG_DEFINE_CATEGORY(Python)\n> >> \n> >> }\n> > \n> > Missing blank line.\n> > \n> >> +/*\n> >> + * This is a holder class used only for the Camera class, for the sole purpose\n> >> + * of avoiding the compilation issue with Camera's private destructor.\n> >> + *\n> >> + * pybind11 requires a public destructor for classes held with shared_ptrs, even\n> >> + * in cases where the public destructor is not strictly needed. The current\n> >> + * understanding is that there are the following options to solve the problem:\n> >> + *\n> >> + * - Use pybind11 'smart_holder' branch. The downside is that 'smart_holder'\n> >> + * is not the mainline branch, and not available in distributions.\n> >> + * - https://github.com/pybind/pybind11/pull/2067\n> >> + * - Make the Camera destructor public\n> >> + * - Something like the PyCameraSmartPtr here, which adds a layer, hiding the\n> >> + * issue.\n> >> + */\n> >> +template\n> >> +class PyCameraSmartPtr\n> >> +{\n> >> +public:\n> >> +\tusing element_type = T;\n> >> +\n> >> +\tPyCameraSmartPtr()\n> >> +\t{\n> >> +\t}\n> >> +\n> >> +\texplicit PyCameraSmartPtr(T *)\n> >> +\t{\n> >> +\t\tthrow std::runtime_error(\"invalid SmartPtr constructor call\");\n> >> +\t}\n> >> +\n> >> +\texplicit PyCameraSmartPtr(std::shared_ptr p)\n> >> +\t\t: ptr_(p)\n> >> +\t{\n> >> +\t}\n> >> +\n> >> +\tT *get() const { return ptr_.get(); }\n> >> +\n> >> +\toperator std::shared_ptr() const { return ptr_; }\n> >> +\n> >> +private:\n> >> +\tstd::shared_ptr ptr_;\n> >> +};\n> >> +\n> >> +PYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr);\n> >> \n> >> /*\n> >> * Note: global C++ destructors can be ran on this before the py module is\n> >> @@ -65,8 +109,8 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> >> \t */\n> >> \n> >> -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> >> -\tauto pyCamera = py::class_(m, \"Camera\");\n> >> +\tauto pyCameraManager = py::class_>(m, \"CameraManager\");\n> > \n> > Why does PyCameraManager need to be changed ?\n> \n> The default holder is unique_ptr. We handle the camera manager with the \n> singleton pattern, using shared_ptr. So we always want to use shared_ptr.\n> \n> If we don't do this, the camera manager gets freed during operation, \n> leading to a crash.\n> \n> I have to say the exact details escape me, but the smart_holder version \n> handles this smartly, as we do return a shared_ptr from the singleton() \n> function.\n\nThat's lots of magic :-)\n\n> But the mainline version doesn't cope with that, and I believe \n> it converts the shared_ptr to a unique_ptr, unless we specifically tell \n> it to use the shared_ptr as a holder class.\n> \n> I'll add a note about this to the commit message.\n\nThanks, that's good enough for me.\n\n> >> +\tauto pyCamera = py::class_>(m, \"Camera\");\n> >> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> >> \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> >> \tauto pyStreamConfiguration = py::class_(m, \"StreamConfiguration\");\n> >> @@ -271,7 +315,7 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t\t.def(\"range\", &StreamFormats::range);\n> >> \n> >> \tpyFrameBufferAllocator\n> >> -\t\t.def(py::init>(), py::keep_alive<1, 2>())\n> >> +\t\t.def(py::init>(), py::keep_alive<1, 2>())\n> >> \t\t.def(\"allocate\", [](FrameBufferAllocator &self, Stream *stream) {\n> >> \t\t\tint ret = self.allocate(stream);\n> >> \t\t\tif (ret < 0)\n> >> diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in\n> >> index 044b2b2a..e49b6e91 100644\n> >> --- a/src/py/libcamera/py_properties_generated.cpp.in\n> >> +++ b/src/py/libcamera/py_properties_generated.cpp.in\n> >> @@ -9,7 +9,7 @@\n> >> \n> >> #include \n> >> \n> >> -#include \n> >> +#include \n> >> \n> >> namespace py = pybind11;\n> >> \n> >> diff --git a/src/py/libcamera/py_transform.cpp b/src/py/libcamera/py_transform.cpp\n> >> index 08783e29..f3a0bfaf 100644\n> >> --- a/src/py/libcamera/py_transform.cpp\n> >> +++ b/src/py/libcamera/py_transform.cpp\n> >> @@ -9,7 +9,7 @@\n> >> #include \n> >> \n> >> #include \n> >> -#include \n> >> +#include \n> >> #include \n> >> \n> >> namespace py = pybind11;\n> >> diff --git a/subprojects/.gitignore b/subprojects/.gitignore\n> >> index 0a8fd3a6..ebe59479 100644\n> >> --- a/subprojects/.gitignore\n> >> +++ b/subprojects/.gitignore\n> >> @@ -4,4 +4,4 @@\n> >> /libyaml\n> >> /libyuv\n> >> /packagecache\n> >> -/pybind11\n> >> +/pybind11-*\n> >> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap\n> >> index dd02687b..96ec57a1 100644\n> >> --- a/subprojects/pybind11.wrap\n> >> +++ b/subprojects/pybind11.wrap\n> >> @@ -1,11 +1,13 @@\n> >> -# SPDX-License-Identifier: CC0-1.0\n> >> -\n> > \n> > Please specify a license.\n> \n> Hmm, I wonder what it is... This file is downloaded with the \"meson wrap \n> install\" tool. https://github.com/mesonbuild/wrapdb says \"MIT\", so \n> perhaps it's that. I hope it won't get overwritten every time we update \n> the wrap with \"meson wrap update\"...\n\nIf we drop the subproject that won't be a problem anymore ;-)","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 8F821C31E9\n\tfor ;\n\tTue, 30 May 2023 11:29:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13A0D60595;\n\tTue, 30 May 2023 13:29:08 +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 037C360595\n\tfor ;\n\tTue, 30 May 2023 13:29:07 +0200 (CEST)","from pendragon.ideasonboard.com (om126205198071.34.openmobile.ne.jp\n\t[126.205.198.71])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 357137EC;\n\tTue, 30 May 2023 13:28:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685446148;\n\tbh=34GCc3UI1hIb3KXMT032xg1XVRrBdrWFbNTt0UE3suI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vHqgMeL0b3x+O3VOskG0me7ZhVPCxlsjCRD9kPBSPAAK+uY/SxL8dYx//9WDzIsdd\n\tJ66iu2HPpO/FFBpvk0sTDuP13lkZIqliisCubeKTyLkcy6ENX4ZqSoZ7IWyVekkdQm\n\tL60WhA54VF8EgZ3/FnQI6Sz/AIgQ+X7KMzig6Ci9wn7w5GgTCNSLMWW4w3kbgimLhx\n\toPr1Mb8NCJkA2WEPQJtEXWLHvm9oMJtlDpzHXEDWJ1HH0EgcYEOsSop9+ucJ4pUduh\n\tHof0p/XYEJx1IfHmqpxeYuuixlMvp08Ebcc1bcUmDjq98irgqZNkkTVhVIoiNU3y4l\n\tLTBV6pZ8KkEPA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685446126;\n\tbh=34GCc3UI1hIb3KXMT032xg1XVRrBdrWFbNTt0UE3suI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D3Pa2vX+f5l2mZqFahKsKkrN2+Pl/h4LoYZjFSCqKzhSvPbUdyb/ZhzItN3i3SenU\n\tpTDp7ze/jm6/abLKRIhkVSw79Wh9FbLRiNbXfL4AxUfPbn8fqiWJhzXfsvciuoU7OD\n\t200HXZwDyLUT2a1FliedTqcsGSONNczLaaOMspc0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"D3Pa2vX+\"; dkim-atps=neutral","Date":"Tue, 30 May 2023 14:29:05 +0300","To":"Tomi Valkeinen ","Message-ID":"<20230530112905.GA22516@pendragon.ideasonboard.com>","References":"<20230530092016.34953-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530092016.34953-6-tomi.valkeinen@ideasonboard.com>\n\t<20230530101222.GB19436@pendragon.ideasonboard.com>\n\t<139fd0f5-44aa-7671-45aa-dad91f03691e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<139fd0f5-44aa-7671-45aa-dad91f03691e@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27175,"web_url":"https://patchwork.libcamera.org/comment/27175/","msgid":"<835e4f86-2789-deca-d206-14f5f7f7f2df@ideasonboard.com>","date":"2023-05-30T11:34:17","subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/05/2023 14:29, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Tue, May 30, 2023 at 02:16:23PM +0300, Tomi Valkeinen wrote:\n>> On 30/05/2023 13:12, Laurent Pinchart wrote:\n>>> On Tue, May 30, 2023 at 12:20:16PM +0300, Tomi Valkeinen wrote:\n>>>> We are using pybind11 'smart_holder' branch to solve the Camera\n>>>> destructor issue (see the comment in this patch, or the commit\n>>>> that originally added Python bindings support).\n>>>>\n>>>> As it would be very nice to use the mainline pybind11 (which is packaged\n>>>> in distributions), this patch adds a workaround allowing us to move to\n>>>> the mainline pybind11 version.\n>>>>\n>>>> The workaround is simply creating a custom holder class, used only for\n>>>> the Camera, which wraps around the shared_ptr. This makes the compiler\n>>>> happy.\n>>>>\n>>>> Moving to mainline pybdin11 is achieved with:\n>>>\n>>> s/pybdin11/pybind11/\n>>>\n>>>>\n>>>> - Change the pybind11 wrap to point to the mainline pybdind11 version\n>>>>\n>>>> - Change the meson.build file to use a system-installed pybind11 if\n>>>> available, and use the pybind11 subproject as a fallback. The fallback\n>>>> is there as a convenience, as pybind11 may not be available in\n>>>> pre-packaged form in all environments.\n>>>\n>>> Given that Python bindings isn't a core feature of libcamera, do we\n>>> really need a subproject, or could we disable the Python bindings when\n>>> pybind11 isn't found ? We have subprojects for libyaml and libyuv, as\n>>> they're required by the libcamera core and the Android camera HAL and\n>>> not available on AOSP, but for optional features, I would prefer relying\n>>> on the build environment to provide the necessary dependencies,\n>>> especially given that both Debian and Fedora have a pybind11 package.\n>>\n>> Well... It's for my convenience, at least =). Buildroot doesn't/didn't\n>> have a properly working pybind11.\n> \n> I think the package is called python-pybind in buildroot.\n> \n>> Maybe we can drop it, but I'd rather do it later so that we don't mess\n>> up too many things at the same time.\n>>\n>> Or I can just add a patch to my local tree which adds the subproject, if\n>> the consensus is to drop it from libcamera.\n> \n> Could you test with the buildroot python-pybind package ? If that works,\n> is there any blocker to dropping the subproject ?\n\nI will try with the latest buildroot. The python-pybind didn't work when \nI tried it the some time ago. The issues were the familiar \"what belongs \nto the host and what belongs to the target and how does the host get the \npython details about the target\".\n\n> I don't mind dropping the subproject in a separate patch, to make it\n> easy to revert it.\n> \n>>>> Signed-off-by: Tomi Valkeinen \n>>>> ---\n>>>> src/py/libcamera/meson.build | 11 ++--\n>>>> src/py/libcamera/py_camera_manager.h | 2 +-\n>>>> src/py/libcamera/py_color_space.cpp | 2 +-\n>>>> src/py/libcamera/py_controls_generated.cpp.in | 2 +-\n>>>> src/py/libcamera/py_enums.cpp | 2 +-\n>>>> src/py/libcamera/py_formats_generated.cpp.in | 2 +-\n>>>> src/py/libcamera/py_geometry.cpp | 2 +-\n>>>> src/py/libcamera/py_helpers.h | 2 +-\n>>>> src/py/libcamera/py_main.cpp | 52 +++++++++++++++++--\n>>>> .../libcamera/py_properties_generated.cpp.in | 2 +-\n>>>> src/py/libcamera/py_transform.cpp | 2 +-\n>>>> subprojects/.gitignore | 2 +-\n>>>> subprojects/pybind11.wrap | 18 ++++---\n>>>> 13 files changed, 76 insertions(+), 25 deletions(-)\n>>>>\n>>>> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n>>>> index f87b1b4d..9d86d8bf 100644\n>>>> --- a/src/py/libcamera/meson.build\n>>>> +++ b/src/py/libcamera/meson.build\n>>>> @@ -7,10 +7,15 @@ if not py3_dep.found()\n>>>> subdir_done()\n>>>> endif\n>>>> \n>>>> -pycamera_enabled = true\n>>>> +pybind11_dep = dependency('pybind11', required : get_option('pycamera'),\n>>>> + fallback : ['pybind11', 'pybind11_dep'])\n>>>> +\n>>>> +if not pybind11_dep.found()\n>>>> + pycamera_enabled = false\n>>>> + subdir_done()\n>>>> +endif\n>>>> \n>>>> -pybind11_proj = subproject('pybind11')\n>>>> -pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n>>>> +pycamera_enabled = true\n>>>> \n>>>> pycamera_sources = files([\n>>>> 'py_camera_manager.cpp',\n>>>> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n>>>> index 3525057d..3574db23 100644\n>>>> --- a/src/py/libcamera/py_camera_manager.h\n>>>> +++ b/src/py/libcamera/py_camera_manager.h\n>>>> @@ -9,7 +9,7 @@\n>>>> \n>>>> #include \n>>>> \n>>>> -#include \n>>>> +#include \n>>>> \n>>>> using namespace libcamera;\n>>>> \n>>>> diff --git a/src/py/libcamera/py_color_space.cpp b/src/py/libcamera/py_color_space.cpp\n>>>> index a8301e3d..5201121a 100644\n>>>> --- a/src/py/libcamera/py_color_space.cpp\n>>>> +++ b/src/py/libcamera/py_color_space.cpp\n>>>> @@ -9,7 +9,7 @@\n>>>> #include \n>>>> \n>>>> #include \n>>>> -#include \n>>>> +#include \n>>>> #include \n>>>> \n>>>> namespace py = pybind11;\n>>>> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in\n>>>> index cb8442ba..18fa57d9 100644\n>>>> --- a/src/py/libcamera/py_controls_generated.cpp.in\n>>>> +++ b/src/py/libcamera/py_controls_generated.cpp.in\n>>>> @@ -9,7 +9,7 @@\n>>>> \n>>>> #include \n>>>> \n>>>> -#include \n>>>> +#include \n>>>> \n>>>> namespace py = pybind11;\n>>>> \n>>>> diff --git a/src/py/libcamera/py_enums.cpp b/src/py/libcamera/py_enums.cpp\n>>>> index 96d4beef..803c4e7e 100644\n>>>> --- a/src/py/libcamera/py_enums.cpp\n>>>> +++ b/src/py/libcamera/py_enums.cpp\n>>>> @@ -7,7 +7,7 @@\n>>>> \n>>>> #include \n>>>> \n>>>> -#include \n>>>> +#include \n>>>> \n>>>> namespace py = pybind11;\n>>>> \n>>>> diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in\n>>>> index b88807f3..a3f7f94d 100644\n>>>> --- a/src/py/libcamera/py_formats_generated.cpp.in\n>>>> +++ b/src/py/libcamera/py_formats_generated.cpp.in\n>>>> @@ -9,7 +9,7 @@\n>>>> \n>>>> #include \n>>>> \n>>>> -#include \n>>>> +#include \n>>>> \n>>>> namespace py = pybind11;\n>>>> \n>>>> diff --git a/src/py/libcamera/py_geometry.cpp b/src/py/libcamera/py_geometry.cpp\n>>>> index 84b0cb08..5c2aeac4 100644\n>>>> --- a/src/py/libcamera/py_geometry.cpp\n>>>> +++ b/src/py/libcamera/py_geometry.cpp\n>>>> @@ -11,7 +11,7 @@\n>>>> #include \n>>>> \n>>>> #include \n>>>> -#include \n>>>> +#include \n>>>> #include \n>>>> \n>>>> namespace py = pybind11;\n>>>> diff --git a/src/py/libcamera/py_helpers.h b/src/py/libcamera/py_helpers.h\n>>>> index cd31e2cc..983969df 100644\n>>>> --- a/src/py/libcamera/py_helpers.h\n>>>> +++ b/src/py/libcamera/py_helpers.h\n>>>> @@ -7,7 +7,7 @@\n>>>> \n>>>> #include \n>>>> \n>>>> -#include \n>>>> +#include \n>>>> \n>>>> pybind11::object controlValueToPy(const libcamera::ControlValue &cv);\n>>>> libcamera::ControlValue pyToControlValue(const pybind11::object &ob, libcamera::ControlType type);\n>>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>>>> index c66b90cd..bbae7a99 100644\n>>>> --- a/src/py/libcamera/py_main.cpp\n>>>> +++ b/src/py/libcamera/py_main.cpp\n>>>> @@ -17,7 +17,7 @@\n>>>> #include \n>>>> \n>>>> #include \n>>>> -#include \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>> \n>>>> @@ -33,6 +33,50 @@ namespace libcamera {\n>>>> LOG_DEFINE_CATEGORY(Python)\n>>>> \n>>>> }\n>>>\n>>> Missing blank line.\n>>>\n>>>> +/*\n>>>> + * This is a holder class used only for the Camera class, for the sole purpose\n>>>> + * of avoiding the compilation issue with Camera's private destructor.\n>>>> + *\n>>>> + * pybind11 requires a public destructor for classes held with shared_ptrs, even\n>>>> + * in cases where the public destructor is not strictly needed. The current\n>>>> + * understanding is that there are the following options to solve the problem:\n>>>> + *\n>>>> + * - Use pybind11 'smart_holder' branch. The downside is that 'smart_holder'\n>>>> + * is not the mainline branch, and not available in distributions.\n>>>> + * - https://github.com/pybind/pybind11/pull/2067\n>>>> + * - Make the Camera destructor public\n>>>> + * - Something like the PyCameraSmartPtr here, which adds a layer, hiding the\n>>>> + * issue.\n>>>> + */\n>>>> +template\n>>>> +class PyCameraSmartPtr\n>>>> +{\n>>>> +public:\n>>>> +\tusing element_type = T;\n>>>> +\n>>>> +\tPyCameraSmartPtr()\n>>>> +\t{\n>>>> +\t}\n>>>> +\n>>>> +\texplicit PyCameraSmartPtr(T *)\n>>>> +\t{\n>>>> +\t\tthrow std::runtime_error(\"invalid SmartPtr constructor call\");\n>>>> +\t}\n>>>> +\n>>>> +\texplicit PyCameraSmartPtr(std::shared_ptr p)\n>>>> +\t\t: ptr_(p)\n>>>> +\t{\n>>>> +\t}\n>>>> +\n>>>> +\tT *get() const { return ptr_.get(); }\n>>>> +\n>>>> +\toperator std::shared_ptr() const { return ptr_; }\n>>>> +\n>>>> +private:\n>>>> +\tstd::shared_ptr ptr_;\n>>>> +};\n>>>> +\n>>>> +PYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr);\n>>>> \n>>>> /*\n>>>> * Note: global C++ destructors can be ran on this before the py module is\n>>>> @@ -65,8 +109,8 @@ PYBIND11_MODULE(_libcamera, m)\n>>>> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n>>>> \t */\n>>>> \n>>>> -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n>>>> -\tauto pyCamera = py::class_(m, \"Camera\");\n>>>> +\tauto pyCameraManager = py::class_>(m, \"CameraManager\");\n>>>\n>>> Why does PyCameraManager need to be changed ?\n>>\n>> The default holder is unique_ptr. We handle the camera manager with the\n>> singleton pattern, using shared_ptr. So we always want to use shared_ptr.\n>>\n>> If we don't do this, the camera manager gets freed during operation,\n>> leading to a crash.\n>>\n>> I have to say the exact details escape me, but the smart_holder version\n>> handles this smartly, as we do return a shared_ptr from the singleton()\n>> function.\n> \n> That's lots of magic :-)\n\nIndeed. I think the main thing/feature with pybind11 is the handling of \nthe memory between Python and C++. It's complex...\n\n Tomi","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 642C3C3213\n\tfor ;\n\tTue, 30 May 2023 11:34:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF02F60599;\n\tTue, 30 May 2023 13:34:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A7F960595\n\tfor ;\n\tTue, 30 May 2023 13:34:21 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2EB3E7EC;\n\tTue, 30 May 2023 13:34:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685446462;\n\tbh=02Qud/JZtIkZno48kLwRtaMTZxpaYriWFsS0BJXRh4w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kLUN1YV1Kin5ZkSmkX4L3roWW/mau0rf1YX3vkqth07S7S3dArtt5OabSyfn+pmCp\n\tSpdoIg6eBJSb9Ocv2kK42Ss9ctUYd7BDwN88+UyeQcZfhDUOr0jBO6boTmDVo4FFoN\n\tDPt2WAEeY01ZFJWHE7aFfZpUqco41ZkMWHthMAYoGQ5cNxwGMVvo5NzK44ZyQyT0Os\n\tyWaWBcr0qxDoaznlvY0DRSbkQbbnxes4S+5JDAvnBj+XOr0dtrMY1vjP8bLpuuavn0\n\tn6WUrxvU2/w2CCxLdYa5HmmtY6/FHhizGhNy3Ra/gB+Xl4wDm9JHBcqVhZwew5LCDq\n\tjCJK63SzNjXVg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685446440;\n\tbh=02Qud/JZtIkZno48kLwRtaMTZxpaYriWFsS0BJXRh4w=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=nH/XIg9hsmccA99psv2bxVQNbbJ9jxn7PYG842xt5OrHf2wzHjciNvunXZGOJCxkR\n\tA+Ryuw2I4vHpSdRsbD/7rHFrXJvVXmQLG/wcyAywMbHgs4qpzbmeWim+PVtzhpMyj7\n\tLa/qVDzFYCifOWKHBZbVxU5atwWSF+wHO4smkqCM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nH/XIg9h\"; dkim-atps=neutral","Message-ID":"<835e4f86-2789-deca-d206-14f5f7f7f2df@ideasonboard.com>","Date":"Tue, 30 May 2023 14:34:17 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.11.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20230530092016.34953-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530092016.34953-6-tomi.valkeinen@ideasonboard.com>\n\t<20230530101222.GB19436@pendragon.ideasonboard.com>\n\t<139fd0f5-44aa-7671-45aa-dad91f03691e@ideasonboard.com>\n\t<20230530112905.GA22516@pendragon.ideasonboard.com>","In-Reply-To":"<20230530112905.GA22516@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27176,"web_url":"https://patchwork.libcamera.org/comment/27176/","msgid":"<82f48ea1-2c57-166e-8588-355a9fbebf90@ideasonboard.com>","date":"2023-05-30T12:02:56","subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/05/2023 14:29, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Tue, May 30, 2023 at 02:16:23PM +0300, Tomi Valkeinen wrote:\n>> On 30/05/2023 13:12, Laurent Pinchart wrote:\n>>> On Tue, May 30, 2023 at 12:20:16PM +0300, Tomi Valkeinen wrote:\n>>>> We are using pybind11 'smart_holder' branch to solve the Camera\n>>>> destructor issue (see the comment in this patch, or the commit\n>>>> that originally added Python bindings support).\n>>>>\n>>>> As it would be very nice to use the mainline pybind11 (which is packaged\n>>>> in distributions), this patch adds a workaround allowing us to move to\n>>>> the mainline pybind11 version.\n>>>>\n>>>> The workaround is simply creating a custom holder class, used only for\n>>>> the Camera, which wraps around the shared_ptr. This makes the compiler\n>>>> happy.\n>>>>\n>>>> Moving to mainline pybdin11 is achieved with:\n>>>\n>>> s/pybdin11/pybind11/\n>>>\n>>>>\n>>>> - Change the pybind11 wrap to point to the mainline pybdind11 version\n>>>>\n>>>> - Change the meson.build file to use a system-installed pybind11 if\n>>>> available, and use the pybind11 subproject as a fallback. The fallback\n>>>> is there as a convenience, as pybind11 may not be available in\n>>>> pre-packaged form in all environments.\n>>>\n>>> Given that Python bindings isn't a core feature of libcamera, do we\n>>> really need a subproject, or could we disable the Python bindings when\n>>> pybind11 isn't found ? We have subprojects for libyaml and libyuv, as\n>>> they're required by the libcamera core and the Android camera HAL and\n>>> not available on AOSP, but for optional features, I would prefer relying\n>>> on the build environment to provide the necessary dependencies,\n>>> especially given that both Debian and Fedora have a pybind11 package.\n>>\n>> Well... It's for my convenience, at least =). Buildroot doesn't/didn't\n>> have a properly working pybind11.\n> \n> I think the package is called python-pybind in buildroot.\n> \n>> Maybe we can drop it, but I'd rather do it later so that we don't mess\n>> up too many things at the same time.\n>>\n>> Or I can just add a patch to my local tree which adds the subproject, if\n>> the consensus is to drop it from libcamera.\n> \n> Could you test with the buildroot python-pybind package ? If that works,\n> is there any blocker to dropping the subproject ?\n\nBuildroot seemed to work fine for me with a quick test, so let's drop \nthe subproject. If this turns out to be a problem, it's easy to add it back.\n\n Tomi","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 D3215C3213\n\tfor ;\n\tTue, 30 May 2023 12:03:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92B80626FA;\n\tTue, 30 May 2023 14:03:01 +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 A50E160595\n\tfor ;\n\tTue, 30 May 2023 14:02:59 +0200 (CEST)","from [192.168.88.20] (91-154-35-171.elisa-laajakaista.fi\n\t[91.154.35.171])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DF184E4;\n\tTue, 30 May 2023 14:02:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685448181;\n\tbh=4P6vmPsQZH4pmUGpspq7OOZv7PvCDeEYl9y82lItYic=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iteyuv1dx1RDVTlXreC6nOjIL0TScCTaFxfkTX+PxYiQvUD1CMEUlNvxsPR+cq2UR\n\tS2XRhT/c91dBDt0pF+NQUnPvvUpcP+y+B0RNlnoda8ExKAquIMtCkGrVfSU0JdxuJr\n\tTazMssPfECHdYUehbC9j0eMz36w1FmHDF/c9EcW8eLCDg0/c8trdTUESeIsDUrmY0+\n\taEh9svJXUJtKIxS+V+Ry/cIYHQi1oK0gVzIdDGZObjA6kqsiuP5cY3SVqEBP0OdWsx\n\t9vcaJX41lkG8iPwiyU0H2+9YsukPkr1wwu55iNIOdDU9xelwSz1AxUiIMkW5U8epFc\n\tO+3O8MwNDzTWg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685448159;\n\tbh=4P6vmPsQZH4pmUGpspq7OOZv7PvCDeEYl9y82lItYic=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=X5oBfq/KvZuXypHOlZbPhB3YKbb1+mNxVDOU0hbWUswlgPns8B80HXqKeHAeCOKST\n\tSeOZ8LO1B0ebJw1rj4TiJc4u18aIVeX3JlRUgFAjNqJ1mBgzesiopeDwdhFQ2zrBbo\n\tnJSvigj4+tPkDdmfUfiAv2MWfp/CZJHKyKEgYHgs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"X5oBfq/K\"; dkim-atps=neutral","Message-ID":"<82f48ea1-2c57-166e-8588-355a9fbebf90@ideasonboard.com>","Date":"Tue, 30 May 2023 15:02:56 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.11.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20230530092016.34953-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530092016.34953-6-tomi.valkeinen@ideasonboard.com>\n\t<20230530101222.GB19436@pendragon.ideasonboard.com>\n\t<139fd0f5-44aa-7671-45aa-dad91f03691e@ideasonboard.com>\n\t<20230530112905.GA22516@pendragon.ideasonboard.com>","In-Reply-To":"<20230530112905.GA22516@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11\n\tversion","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]