[libcamera-devel] [PATCH 5/5] py: Move to mainline pybind11 version
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 30 13:29:05 CEST 2023
Hi Tomi,
On Tue, May 30, 2023 at 02:16:23PM +0300, Tomi Valkeinen wrote:
> On 30/05/2023 13:12, Laurent Pinchart wrote:
> > On Tue, May 30, 2023 at 12:20:16PM +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, used only for
> >> the Camera, which wraps around the shared_ptr. This makes the compiler
> >> happy.
> >>
> >> Moving to mainline pybdin11 is achieved with:
> >
> > s/pybdin11/pybind11/
> >
> >>
> >> - Change the pybind11 wrap to point to the mainline pybdind11 version
> >>
> >> - Change the meson.build file to use a system-installed pybind11 if
> >> available, and use the pybind11 subproject as a fallback. The fallback
> >> is there as a convenience, as pybind11 may not be available in
> >> pre-packaged form in all environments.
> >
> > Given that Python bindings isn't a core feature of libcamera, do we
> > really need a subproject, or could we disable the Python bindings when
> > pybind11 isn't found ? We have subprojects for libyaml and libyuv, as
> > they're required by the libcamera core and the Android camera HAL and
> > not available on AOSP, but for optional features, I would prefer relying
> > on the build environment to provide the necessary dependencies,
> > especially given that both Debian and Fedora have a pybind11 package.
>
> Well... It's for my convenience, at least =). Buildroot doesn't/didn't
> have a properly working pybind11.
I think the package is called python-pybind in buildroot.
> Maybe we can drop it, but I'd rather do it later so that we don't mess
> up too many things at the same time.
>
> Or I can just add a patch to my local tree which adds the subproject, if
> the consensus is to drop it from libcamera.
Could you test with the buildroot python-pybind package ? If that works,
is there any blocker to dropping the subproject ?
I don't mind dropping the subproject in a separate patch, to make it
easy to revert it.
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >> ---
> >> src/py/libcamera/meson.build | 11 ++--
> >> 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 | 52 +++++++++++++++++--
> >> .../libcamera/py_properties_generated.cpp.in | 2 +-
> >> src/py/libcamera/py_transform.cpp | 2 +-
> >> subprojects/.gitignore | 2 +-
> >> subprojects/pybind11.wrap | 18 ++++---
> >> 13 files changed, 76 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build
> >> index f87b1b4d..9d86d8bf 100644
> >> --- a/src/py/libcamera/meson.build
> >> +++ b/src/py/libcamera/meson.build
> >> @@ -7,10 +7,15 @@ if not py3_dep.found()
> >> subdir_done()
> >> endif
> >>
> >> -pycamera_enabled = true
> >> +pybind11_dep = dependency('pybind11', required : get_option('pycamera'),
> >> + fallback : ['pybind11', 'pybind11_dep'])
> >> +
> >> +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..bbae7a99 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>
> >>
> >> @@ -33,6 +33,50 @@ namespace libcamera {
> >> LOG_DEFINE_CATEGORY(Python)
> >>
> >> }
> >
> > Missing blank line.
> >
> >> +/*
> >> + * 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
> >> @@ -65,8 +109,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");
> >
> > Why does PyCameraManager need to be changed ?
>
> The default holder is unique_ptr. We handle the camera manager with the
> singleton pattern, using shared_ptr. So we always want to use shared_ptr.
>
> If we don't do this, the camera manager gets freed during operation,
> leading to a crash.
>
> I have to say the exact details escape me, but the smart_holder version
> handles this smartly, as we do return a shared_ptr from the singleton()
> function.
That's lots of magic :-)
> But the mainline version doesn't cope with that, and I believe
> it converts the shared_ptr to a unique_ptr, unless we specifically tell
> it to use the shared_ptr as a holder class.
>
> I'll add a note about this to the commit message.
Thanks, that's good enough for me.
> >> + 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 +315,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-*
> >> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap
> >> index dd02687b..96ec57a1 100644
> >> --- a/subprojects/pybind11.wrap
> >> +++ b/subprojects/pybind11.wrap
> >> @@ -1,11 +1,13 @@
> >> -# SPDX-License-Identifier: CC0-1.0
> >> -
> >
> > Please specify a license.
>
> Hmm, I wonder what it is... This file is downloaded with the "meson wrap
> install" tool. https://github.com/mesonbuild/wrapdb says "MIT", so
> perhaps it's that. I hope it won't get overwritten every time we update
> the wrap with "meson wrap update"...
If we drop the subproject that won't be a problem anymore ;-)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list