[libcamera-devel] [PATCH v2 5/5] py: Move to mainline pybind11 version
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 30 16:18:10 CEST 2023
Hi Tomi,
Thank you for the patch.
On Tue, May 30, 2023 at 03:01:33PM +0300, Tomi Valkeinen wrote:
> We are using pybind11 'smart_holder' branch to solve the Camera
> destructor issue (see the comment in this patch, or the commit
> that originally added Python bindings support).
>
> As it would be very nice to use the mainline pybind11 (which is packaged
> in distributions), this patch adds a workaround allowing us to move to
> the mainline pybind11 version.
>
> The workaround is simply creating a custom holder class
> (PyCameraSmartPtr), used only for the Camera, which wraps around the
> shared_ptr. This makes the compiler happy.
>
> Moving to mainline pybind11 is achieved with:
>
> - Change the pybind11 wrap to point to the mainline pybdind11 version
>
> - Tell pybind11 to always use shared_ptr<> as the holder for
> PyCameraManager, as we use the singleton pattern for the
> PyCameraManager, and using shared_ptr<> to manage it is a requirement
>
> - Tell pybind11 to always use PyCameraSmartPtr<> as the holder for Camera
>
> - Change the meson.build file to use a system-installed pybind11
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> ---
> src/py/libcamera/meson.build | 10 ++--
> src/py/libcamera/py_camera_manager.h | 2 +-
> src/py/libcamera/py_color_space.cpp | 2 +-
> src/py/libcamera/py_controls_generated.cpp.in | 2 +-
> src/py/libcamera/py_enums.cpp | 2 +-
> src/py/libcamera/py_formats_generated.cpp.in | 2 +-
> src/py/libcamera/py_geometry.cpp | 2 +-
> src/py/libcamera/py_helpers.h | 2 +-
> src/py/libcamera/py_main.cpp | 53 +++++++++++++++++--
> .../libcamera/py_properties_generated.cpp.in | 2 +-
> src/py/libcamera/py_transform.cpp | 2 +-
> subprojects/.gitignore | 2 +-
> subprojects/pybind11.wrap | 11 ----
> 13 files changed, 66 insertions(+), 28 deletions(-)
> delete mode 100644 subprojects/pybind11.wrap
>
> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build
> index f87b1b4d..b38a57d7 100644
> --- a/src/py/libcamera/meson.build
> +++ b/src/py/libcamera/meson.build
> @@ -7,10 +7,14 @@ if not py3_dep.found()
> subdir_done()
> endif
>
> -pycamera_enabled = true
> +pybind11_dep = dependency('pybind11', required : get_option('pycamera'))
> +
> +if not pybind11_dep.found()
> + pycamera_enabled = false
> + subdir_done()
> +endif
>
> -pybind11_proj = subproject('pybind11')
> -pybind11_dep = pybind11_proj.get_variable('pybind11_dep')
> +pycamera_enabled = true
>
> pycamera_sources = files([
> 'py_camera_manager.cpp',
> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
> index 3525057d..3574db23 100644
> --- a/src/py/libcamera/py_camera_manager.h
> +++ b/src/py/libcamera/py_camera_manager.h
> @@ -9,7 +9,7 @@
>
> #include <libcamera/libcamera.h>
>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
>
> using namespace libcamera;
>
> diff --git a/src/py/libcamera/py_color_space.cpp b/src/py/libcamera/py_color_space.cpp
> index a8301e3d..5201121a 100644
> --- a/src/py/libcamera/py_color_space.cpp
> +++ b/src/py/libcamera/py_color_space.cpp
> @@ -9,7 +9,7 @@
> #include <libcamera/libcamera.h>
>
> #include <pybind11/operators.h>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
> #include <pybind11/stl.h>
>
> namespace py = pybind11;
> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in
> index cb8442ba..18fa57d9 100644
> --- a/src/py/libcamera/py_controls_generated.cpp.in
> +++ b/src/py/libcamera/py_controls_generated.cpp.in
> @@ -9,7 +9,7 @@
>
> #include <libcamera/control_ids.h>
>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
>
> namespace py = pybind11;
>
> diff --git a/src/py/libcamera/py_enums.cpp b/src/py/libcamera/py_enums.cpp
> index 96d4beef..803c4e7e 100644
> --- a/src/py/libcamera/py_enums.cpp
> +++ b/src/py/libcamera/py_enums.cpp
> @@ -7,7 +7,7 @@
>
> #include <libcamera/libcamera.h>
>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
>
> namespace py = pybind11;
>
> diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in
> index b88807f3..a3f7f94d 100644
> --- a/src/py/libcamera/py_formats_generated.cpp.in
> +++ b/src/py/libcamera/py_formats_generated.cpp.in
> @@ -9,7 +9,7 @@
>
> #include <libcamera/formats.h>
>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
>
> namespace py = pybind11;
>
> diff --git a/src/py/libcamera/py_geometry.cpp b/src/py/libcamera/py_geometry.cpp
> index 84b0cb08..5c2aeac4 100644
> --- a/src/py/libcamera/py_geometry.cpp
> +++ b/src/py/libcamera/py_geometry.cpp
> @@ -11,7 +11,7 @@
> #include <libcamera/libcamera.h>
>
> #include <pybind11/operators.h>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
> #include <pybind11/stl.h>
>
> namespace py = pybind11;
> diff --git a/src/py/libcamera/py_helpers.h b/src/py/libcamera/py_helpers.h
> index cd31e2cc..983969df 100644
> --- a/src/py/libcamera/py_helpers.h
> +++ b/src/py/libcamera/py_helpers.h
> @@ -7,7 +7,7 @@
>
> #include <libcamera/libcamera.h>
>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
>
> pybind11::object controlValueToPy(const libcamera::ControlValue &cv);
> libcamera::ControlValue pyToControlValue(const pybind11::object &ob, libcamera::ControlType type);
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index c66b90cd..c41c43d6 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -17,7 +17,7 @@
> #include <libcamera/libcamera.h>
>
> #include <pybind11/functional.h>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
> #include <pybind11/stl.h>
> #include <pybind11/stl_bind.h>
>
> @@ -34,6 +34,51 @@ LOG_DEFINE_CATEGORY(Python)
>
> }
>
> +/*
> + * This is a holder class used only for the Camera class, for the sole purpose
> + * of avoiding the compilation issue with Camera's private destructor.
> + *
> + * pybind11 requires a public destructor for classes held with shared_ptrs, even
> + * in cases where the public destructor is not strictly needed. The current
> + * understanding is that there are the following options to solve the problem:
> + *
> + * - Use pybind11 'smart_holder' branch. The downside is that 'smart_holder'
> + * is not the mainline branch, and not available in distributions.
> + * - https://github.com/pybind/pybind11/pull/2067
> + * - Make the Camera destructor public
> + * - Something like the PyCameraSmartPtr here, which adds a layer, hiding the
> + * issue.
> + */
> +template<typename T>
> +class PyCameraSmartPtr
> +{
> +public:
> + using element_type = T;
> +
> + PyCameraSmartPtr()
> + {
> + }
> +
> + explicit PyCameraSmartPtr(T *)
> + {
> + throw std::runtime_error("invalid SmartPtr constructor call");
> + }
> +
> + explicit PyCameraSmartPtr(std::shared_ptr<T> p)
> + : ptr_(p)
> + {
> + }
> +
> + T *get() const { return ptr_.get(); }
> +
> + operator std::shared_ptr<T>() const { return ptr_; }
> +
> +private:
> + std::shared_ptr<T> ptr_;
> +};
> +
> +PYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr<T>);
> +
> /*
> * Note: global C++ destructors can be ran on this before the py module is
> * destructed.
> @@ -65,8 +110,8 @@ PYBIND11_MODULE(_libcamera, m)
> * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings
> */
>
> - auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager");
> - auto pyCamera = py::class_<Camera>(m, "Camera");
> + auto pyCameraManager = py::class_<PyCameraManager, std::shared_ptr<PyCameraManager>>(m, "CameraManager");
> + auto pyCamera = py::class_<Camera, PyCameraSmartPtr<Camera>>(m, "Camera");
> auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
> auto pyCameraConfigurationStatus = py::enum_<CameraConfiguration::Status>(pyCameraConfiguration, "Status");
> auto pyStreamConfiguration = py::class_<StreamConfiguration>(m, "StreamConfiguration");
> @@ -271,7 +316,7 @@ PYBIND11_MODULE(_libcamera, m)
> .def("range", &StreamFormats::range);
>
> pyFrameBufferAllocator
> - .def(py::init<std::shared_ptr<Camera>>(), py::keep_alive<1, 2>())
> + .def(py::init<PyCameraSmartPtr<Camera>>(), py::keep_alive<1, 2>())
> .def("allocate", [](FrameBufferAllocator &self, Stream *stream) {
> int ret = self.allocate(stream);
> if (ret < 0)
> diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in
> index 044b2b2a..e49b6e91 100644
> --- a/src/py/libcamera/py_properties_generated.cpp.in
> +++ b/src/py/libcamera/py_properties_generated.cpp.in
> @@ -9,7 +9,7 @@
>
> #include <libcamera/property_ids.h>
>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
>
> namespace py = pybind11;
>
> diff --git a/src/py/libcamera/py_transform.cpp b/src/py/libcamera/py_transform.cpp
> index 08783e29..f3a0bfaf 100644
> --- a/src/py/libcamera/py_transform.cpp
> +++ b/src/py/libcamera/py_transform.cpp
> @@ -9,7 +9,7 @@
> #include <libcamera/libcamera.h>
>
> #include <pybind11/operators.h>
> -#include <pybind11/smart_holder.h>
> +#include <pybind11/pybind11.h>
> #include <pybind11/stl.h>
>
> namespace py = pybind11;
> diff --git a/subprojects/.gitignore b/subprojects/.gitignore
> index 0a8fd3a6..ebe59479 100644
> --- a/subprojects/.gitignore
> +++ b/subprojects/.gitignore
> @@ -4,4 +4,4 @@
> /libyaml
> /libyuv
> /packagecache
> -/pybind11
> +/pybind11-*
I'll drop this when merging.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap
> deleted file mode 100644
> index dd02687b..00000000
> --- a/subprojects/pybind11.wrap
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -# SPDX-License-Identifier: CC0-1.0
> -
> -[wrap-git]
> -url = https://github.com/pybind/pybind11.git
> -# This is the head of 'smart_holder' branch
> -revision = aebdf00cd060b871c5a1e0c2cf4a333503dd0431
> -depth = 1
> -patch_directory = pybind11
> -
> -[provide]
> -pybind11 = pybind11_dep
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list