[{"id":27177,"web_url":"https://patchwork.libcamera.org/comment/27177/","msgid":"","date":"2023-05-30T12:06:38","subject":"Re: [libcamera-devel] [PATCH v2 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 15:01, 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\n> (PyCameraSmartPtr), used only for the Camera, which wraps around the\n> shared_ptr. This makes the compiler happy.\n> \n> Moving to mainline pybind11 is achieved with:\n> \n> - Change the pybind11 wrap to point to the mainline pybdind11 version\n> \n> - Tell pybind11 to always use shared_ptr<> as the holder for\n> PyCameraManager, as we use the singleton pattern for the\n> PyCameraManager, and using shared_ptr<> to manage it is a requirement\n> \n> - Tell pybind11 to always use PyCameraSmartPtr<> as the holder for Camera\n> \n> - Change the meson.build file to use a system-installed pybind11\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/meson.build | 10 ++--\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 | 53 +++++++++++++++++--\n> .../libcamera/py_properties_generated.cpp.in | 2 +-\n> src/py/libcamera/py_transform.cpp | 2 +-\n> subprojects/.gitignore | 2 +-\n> subprojects/pybind11.wrap | 11 ----\n> 13 files changed, 66 insertions(+), 28 deletions(-)\n> delete mode 100644 subprojects/pybind11.wrap\n> \n> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> index f87b1b4d..b38a57d7 100644\n> --- a/src/py/libcamera/meson.build\n> +++ b/src/py/libcamera/meson.build\n> @@ -7,10 +7,14 @@ 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> +\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..c41c43d6 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> @@ -34,6 +34,51 @@ LOG_DEFINE_CATEGORY(Python)\n> \n> }\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> * destructed.\n> @@ -65,8 +110,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> +\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 +316,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\nOps, the above line should be deleted.\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 DE3E3C3213\n\tfor ;\n\tTue, 30 May 2023 12:06:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D7FC626FA;\n\tTue, 30 May 2023 14:06:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B379E60595\n\tfor ;\n\tTue, 30 May 2023 14:06:41 +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 B9AB7E4;\n\tTue, 30 May 2023 14:06:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685448403;\n\tbh=NkM9aWqTYvCIQxVI+oYJLU44tm+BKp4mgFYhX5R2j/c=;\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:\n\tFrom;\n\tb=wme6ThwHgasjrOb+WTSO28msqODtTvVempRSEezTsCvA9pH4DmPiKa6ZgFJfIog33\n\t6xm+Tpw5Ng23R30DPGG2eNNdR//689a5fhxnuf8NlihR4+TUNecaaW/DDHxJz9pzUd\n\tqWJqmdUvD6xpkuydByeacseexGZDS0+9rJyA/BVSlVq299u8KckQYU/TlgCSXU7B6B\n\tCpOsxg15S6CX9TyxxB1nTtijXP6iVh06j9JsSjOmmbS4Y7SVPyHCUGLeiBRKrzfEuJ\n\tq4zJ4xmfEEUk5g7Y5tOjELJbDs9ABBEsV3pMlkgh/FuJvRKhSHvb9Rw6xMXpootia5\n\tYeys9M1fp0spA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685448381;\n\tbh=NkM9aWqTYvCIQxVI+oYJLU44tm+BKp4mgFYhX5R2j/c=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=AyVFuxA/UOBRrEWkIz43A8Bco1rQ5qFXMbd4ErUQtlxMaVjGeQsPzWbdsFh/8Jx5x\n\th3qYFvTCa/HydyCUUDQDLuVOeohHFkzdHV4zgUNXG54+stxgyG1pzaweNMeVP1bpW2\n\tW4ZrldSSQyj6spL335s4IWdlQdU5gkRcQINLh4MM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AyVFuxA/\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 30 May 2023 15:06:38 +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":"libcamera-devel@lists.libcamera.org","References":"<20230530120133.99033-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530120133.99033-6-tomi.valkeinen@ideasonboard.com>","In-Reply-To":"<20230530120133.99033-6-tomi.valkeinen@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27179,"web_url":"https://patchwork.libcamera.org/comment/27179/","msgid":"<20230530141810.GA29260@pendragon.ideasonboard.com>","date":"2023-05-30T14:18:10","subject":"Re: [libcamera-devel] [PATCH v2 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 03:01:33PM +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\n> (PyCameraSmartPtr), used only for the Camera, which wraps around the\n> shared_ptr. This makes the compiler happy.\n> \n> Moving to mainline pybind11 is achieved with:\n> \n> - Change the pybind11 wrap to point to the mainline pybdind11 version\n> \n> - Tell pybind11 to always use shared_ptr<> as the holder for\n> PyCameraManager, as we use the singleton pattern for the\n> PyCameraManager, and using shared_ptr<> to manage it is a requirement\n> \n> - Tell pybind11 to always use PyCameraSmartPtr<> as the holder for Camera\n> \n> - Change the meson.build file to use a system-installed pybind11\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/meson.build | 10 ++--\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 | 53 +++++++++++++++++--\n> .../libcamera/py_properties_generated.cpp.in | 2 +-\n> src/py/libcamera/py_transform.cpp | 2 +-\n> subprojects/.gitignore | 2 +-\n> subprojects/pybind11.wrap | 11 ----\n> 13 files changed, 66 insertions(+), 28 deletions(-)\n> delete mode 100644 subprojects/pybind11.wrap\n> \n> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> index f87b1b4d..b38a57d7 100644\n> --- a/src/py/libcamera/meson.build\n> +++ b/src/py/libcamera/meson.build\n> @@ -7,10 +7,14 @@ 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> +\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..c41c43d6 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> @@ -34,6 +34,51 @@ LOG_DEFINE_CATEGORY(Python)\n> \n> }\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> * destructed.\n> @@ -65,8 +110,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> +\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 +316,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\nI'll drop this when merging.\n\nReviewed-by: Laurent Pinchart \n\n> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap\n> deleted file mode 100644\n> index dd02687b..00000000\n> --- a/subprojects/pybind11.wrap\n> +++ /dev/null\n> @@ -1,11 +0,0 @@\n> -# SPDX-License-Identifier: CC0-1.0\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> -\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 5FB78C31E9\n\tfor ;\n\tTue, 30 May 2023 14:18:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C067A626F8;\n\tTue, 30 May 2023 16:18:12 +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 73DFF60595\n\tfor ;\n\tTue, 30 May 2023 16:18:11 +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 767467EC;\n\tTue, 30 May 2023 16:17:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685456292;\n\tbh=2pABkLo19JTtq9OXpQ/iqMbuEiEMaHS/UDHJHMjvjpo=;\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=P2gbXHJ/pWILs1AULQjdOdtBME+6zg3VPFkPeb+S5hSKWDOORJ2h9p6PfoIa1AVjz\n\t33Xwcu+NaN9rWRyAxfli5r2WBNqBm8B9FLxD1E1YDwVoGvWWWhy4dZPvYmlovk93HR\n\tLTqY19oh6VeUxmqXVKWbKBSK24IMYzBI7ure00N6iP7j2vUXvGpY91tsPbOsyL5yJQ\n\tHbyxwGaaOUmz1ts+oir5DjA64nKdz6hnumYxDAewfb23rERKXhXleWJaQP6Yvxwkq2\n\tje9oxn9lQ7+JsOkH+K87F3WEVGjR1MLwpAFl1hdpEhppi4Oik8md8/6NE6VU/uxN3Q\n\tx1uvm/m+AXwJA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685456270;\n\tbh=2pABkLo19JTtq9OXpQ/iqMbuEiEMaHS/UDHJHMjvjpo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YCf5vyy0RsONmkBe7Qe6XtUnLMXactl4//3YpxIFEZg0RLZsruTyzXNr0sxxRehkM\n\tu6MKtfvZGCpTK01nsEwZvhchNxkDznf6MJphW/XlqlsEOg5kPZ+PttpABUCBv77xTg\n\twD+cNA465FHDaRd7wGIA8rbe4JSzN7yXJB6jFJ/s="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YCf5vyy0\"; dkim-atps=neutral","Date":"Tue, 30 May 2023 17:18:10 +0300","To":"Tomi Valkeinen ","Message-ID":"<20230530141810.GA29260@pendragon.ideasonboard.com>","References":"<20230530120133.99033-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530120133.99033-6-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230530120133.99033-6-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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":27197,"web_url":"https://patchwork.libcamera.org/comment/27197/","msgid":"<20230531163112.GA11887@pendragon.ideasonboard.com>","date":"2023-05-31T16:31:12","subject":"Re: [libcamera-devel] [PATCH v2 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 03:01:33PM +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\n> (PyCameraSmartPtr), used only for the Camera, which wraps around the\n> shared_ptr. This makes the compiler happy.\n> \n> Moving to mainline pybind11 is achieved with:\n> \n> - Change the pybind11 wrap to point to the mainline pybdind11 version\n> \n> - Tell pybind11 to always use shared_ptr<> as the holder for\n> PyCameraManager, as we use the singleton pattern for the\n> PyCameraManager, and using shared_ptr<> to manage it is a requirement\n> \n> - Tell pybind11 to always use PyCameraSmartPtr<> as the holder for Camera\n> \n> - Change the meson.build file to use a system-installed pybind11\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/meson.build | 10 ++--\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 | 53 +++++++++++++++++--\n> .../libcamera/py_properties_generated.cpp.in | 2 +-\n> src/py/libcamera/py_transform.cpp | 2 +-\n> subprojects/.gitignore | 2 +-\n> subprojects/pybind11.wrap | 11 ----\n> 13 files changed, 66 insertions(+), 28 deletions(-)\n> delete mode 100644 subprojects/pybind11.wrap\n> \n> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> index f87b1b4d..b38a57d7 100644\n> --- a/src/py/libcamera/meson.build\n> +++ b/src/py/libcamera/meson.build\n> @@ -7,10 +7,14 @@ 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> +\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..c41c43d6 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> @@ -34,6 +34,51 @@ LOG_DEFINE_CATEGORY(Python)\n> \n> }\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\nI'm getting this compilation error with clang:\n\n../../src/py/libcamera/py_main.cpp:80:53: error: extra ';' outside of a function is incompatible with C++98 [-Werror,-Wc++98-compat-extra-semi]\nPYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr);\n\nI'll drop the semicolon.\n\n> +\n> /*\n> * Note: global C++ destructors can be ran on this before the py module is\n> * destructed.\n> @@ -65,8 +110,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> +\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 +316,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> deleted file mode 100644\n> index dd02687b..00000000\n> --- a/subprojects/pybind11.wrap\n> +++ /dev/null\n> @@ -1,11 +0,0 @@\n> -# SPDX-License-Identifier: CC0-1.0\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> -\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 ACB3CC31E9\n\tfor ;\n\tWed, 31 May 2023 16:31:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E5AE662722;\n\tWed, 31 May 2023 18:31:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45675626F8\n\tfor ;\n\tWed, 31 May 2023 18:31:16 +0200 (CEST)","from pendragon.ideasonboard.com (om126205251136.34.openmobile.ne.jp\n\t[126.205.251.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 805ED7F5;\n\tWed, 31 May 2023 18:30:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685550678;\n\tbh=a5f8M1KPhT538VfBQ7YxZ8R+F+tSI4kW+gWdj3a7uxw=;\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=KpZXx9DGlxfsVuum5sM4HtHOpDZc4zOZhVoaolzwMIdjiTOueCMIYArNBhoudb9AG\n\teCS3zN3Ex9Fwp14izjesC80QZ1js2O8lwj1BPozuP21HuxkBS3aDWn9GwUiNWluyFk\n\tI3NYI7/k4kM+Xdd+6SOgQeYGgzARGwJPXPUQfSVBSIjcydUWcJC1UhyeTb2NATfyBK\n\tnVQoFAyEo9wQOIxBRA7xh8yWQh/JyOIwfYRqYzmP+tU3UVA1GfvR3v4LuyNi0Iuw4e\n\tXMBKkmDzGyCSHXcBwBUbztIc13JDoXN0DBDoQF54XMrP+pozYV+y4jUedFKQYugQ//\n\tbqr3/ULGOpevg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685550654;\n\tbh=a5f8M1KPhT538VfBQ7YxZ8R+F+tSI4kW+gWdj3a7uxw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Pj5FkvQ6IIewD0dEKWbnXbI7f4oGKEABqSemJAR6os3g3NCQVRulpJpsNM1qN4Chn\n\tpwNSHM3q8bnj60WK8qtBoOQ1ZjsnQCK8S6i0LfTqyl+dT7eELOrOJ5vTl19Mszk2wu\n\teG+TKmMZTB7ga1/DQBZ5tIXJ6SCkxJrxQQOFtl1I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Pj5FkvQ6\"; dkim-atps=neutral","Date":"Wed, 31 May 2023 19:31:12 +0300","To":"Tomi Valkeinen ","Message-ID":"<20230531163112.GA11887@pendragon.ideasonboard.com>","References":"<20230530120133.99033-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530120133.99033-6-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230530120133.99033-6-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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":27198,"web_url":"https://patchwork.libcamera.org/comment/27198/","msgid":"<65704fda-278f-dc9c-a477-f706a88ca966@ideasonboard.com>","date":"2023-05-31T17:31:26","subject":"Re: [libcamera-devel] [PATCH v2 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 31/05/2023 19:31, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Tue, May 30, 2023 at 03:01:33PM +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\n>> (PyCameraSmartPtr), used only for the Camera, which wraps around the\n>> shared_ptr. This makes the compiler happy.\n>>\n>> Moving to mainline pybind11 is achieved with:\n>>\n>> - Change the pybind11 wrap to point to the mainline pybdind11 version\n>>\n>> - Tell pybind11 to always use shared_ptr<> as the holder for\n>> PyCameraManager, as we use the singleton pattern for the\n>> PyCameraManager, and using shared_ptr<> to manage it is a requirement\n>>\n>> - Tell pybind11 to always use PyCameraSmartPtr<> as the holder for Camera\n>>\n>> - Change the meson.build file to use a system-installed pybind11\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/libcamera/meson.build | 10 ++--\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 | 53 +++++++++++++++++--\n>> .../libcamera/py_properties_generated.cpp.in | 2 +-\n>> src/py/libcamera/py_transform.cpp | 2 +-\n>> subprojects/.gitignore | 2 +-\n>> subprojects/pybind11.wrap | 11 ----\n>> 13 files changed, 66 insertions(+), 28 deletions(-)\n>> delete mode 100644 subprojects/pybind11.wrap\n>>\n>> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n>> index f87b1b4d..b38a57d7 100644\n>> --- a/src/py/libcamera/meson.build\n>> +++ b/src/py/libcamera/meson.build\n>> @@ -7,10 +7,14 @@ 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>> +\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..c41c43d6 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>> @@ -34,6 +34,51 @@ LOG_DEFINE_CATEGORY(Python)\n>> \n>> }\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> I'm getting this compilation error with clang:\n> \n> ../../src/py/libcamera/py_main.cpp:80:53: error: extra ';' outside of a function is incompatible with C++98 [-Werror,-Wc++98-compat-extra-semi]\n> PYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr);\n> \n> I'll drop the semicolon.\n\nOk. Yes, builds fine for me without the semicolon.\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 DAAA8C3213\n\tfor ;\n\tWed, 31 May 2023 17:31:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 295FA626FA;\n\tWed, 31 May 2023 19:31:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BACF5626F8\n\tfor ;\n\tWed, 31 May 2023 19:31:29 +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 E99DA844;\n\tWed, 31 May 2023 19:31:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685554292;\n\tbh=wQPXpAZ4fGU/P+2IpD/WI+SgYGqcCU7fQsNCcmexJcY=;\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=LyvXEIlBqkFOUDLb4dgE5IFZ8P9KlMB9asgtchg4OVLHXq8OTBqvNVtc0HbTSR5U1\n\tyjuEfcBLaMaZsTVjjz9k3XA0xdMRmdoooG15VWK7K9PEM08B6lBE+Esf1OpXOQooGs\n\tKqTD7FX6sl1JPfQbE8D1ffiSEVuTZeQ8FGfGbFkk50DJfGbw5A0MEOmYZlHp/fBK+R\n\tN2PRPHWVuE4Xm7vOqecVgU1I602iFNZSMkefQLILF/zZREvF3KQTmNuLGFbki1YIVK\n\tGI6LFB4fA9s39Ptz6ePUt8xyHmiebz4ieh/jAjPaEy6mF3voR0IYH9yyKsme6Si5cF\n\tbWzJGQNcOU+Ag==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685554268;\n\tbh=wQPXpAZ4fGU/P+2IpD/WI+SgYGqcCU7fQsNCcmexJcY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Mv4QI7OGndNS2Fnt38wgaqOKjMkR0JG+Oro2vY4+Bie4VfLghBmiMI6wNpdUr9r1h\n\tbHT+z5NEGcnEcNafOFRVOd3BjiRCGMOSXFlVdnd4jWQTtLPy/P/RYr+SkBGSpOiRYQ\n\tpOJwlJpUNswgXAwgiemRx1L08SONCOBl9GGWv/ow="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Mv4QI7OG\"; dkim-atps=neutral","Message-ID":"<65704fda-278f-dc9c-a477-f706a88ca966@ideasonboard.com>","Date":"Wed, 31 May 2023 20:31:26 +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":"<20230530120133.99033-1-tomi.valkeinen@ideasonboard.com>\n\t<20230530120133.99033-6-tomi.valkeinen@ideasonboard.com>\n\t<20230531163112.GA11887@pendragon.ideasonboard.com>","Content-Language":"en-US","In-Reply-To":"<20230531163112.GA11887@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}}]