[libcamera-devel,v2,5/5] py: Move to mainline pybind11 version
diff mbox series

Message ID 20230530120133.99033-6-tomi.valkeinen@ideasonboard.com
State Accepted
Headers show
Series
  • py: Misc changes & move to mainline pybind11
Related show

Commit Message

Tomi Valkeinen May 30, 2023, 12:01 p.m. UTC 
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@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

Comments

Tomi Valkeinen May 30, 2023, 12:06 p.m. UTC | #1 
On 30/05/2023 15:01, 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@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-*

Ops, the above line should be deleted.

  Tomi
Laurent Pinchart May 30, 2023, 2:18 p.m. UTC | #2 
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@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@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
Laurent Pinchart May 31, 2023, 4:31 p.m. UTC | #3 
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@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.

> +
>  /*
>   * 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-*
> 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
Tomi Valkeinen May 31, 2023, 5:31 p.m. UTC | #4 
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@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

Patch
diff mbox series 

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-*
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