[libcamera-devel] [PATCH v2 5/5] py: Move to mainline pybind11 version
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Wed May 31 19:31:26 CEST 2023
On 31/05/2023 19:31, Laurent Pinchart wrote:
> Hi Tomi,
>
> 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>);
>
> I'm getting this compilation error with clang:
>
> ../../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]
> PYBIND11_DECLARE_HOLDER_TYPE(T, PyCameraSmartPtr<T>);
>
> I'll drop the semicolon.
Ok. Yes, builds fine for me without the semicolon.
Tomi
More information about the libcamera-devel
mailing list