| Message ID | 20211209092906.37303-2-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Tomi Valkeinen (2021-12-09 09:29:02) > pybind11 needs a public destructor for Camera to be able to manage the > shared_ptr. It's not clear why this is needed, and can it be fixed in > pybind11. > :-( The 'HACK' here seems to be the only thing that would count as a blocker to integration. I wonder if this is really a HACK or if we just need to explain that the destructor needs to be public to support the python bindings, but I myself don't understand the reasoning itself. Is the python code the only instance where we create a shared_ptr for the Camera? If that's true - then perhaps this patch should actually be squashed with the one that returns the Camera as a shared_ptr... -- Kieran > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > include/libcamera/camera.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index a7759ccb..88a61ff5 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -104,12 +104,12 @@ public: > int start(const ControlList *controls = nullptr); > int stop(); > > + ~Camera(); > private: > LIBCAMERA_DISABLE_COPY(Camera) > > Camera(std::unique_ptr<Private> d, const std::string &id, > const std::set<Stream *> &streams); > - ~Camera(); > > friend class PipelineHandler; > void disconnect(); > -- > 2.25.1 >
Quoting Kieran Bingham (2021-12-09 10:05:56) > Quoting Tomi Valkeinen (2021-12-09 09:29:02) > > pybind11 needs a public destructor for Camera to be able to manage the > > shared_ptr. It's not clear why this is needed, and can it be fixed in > > pybind11. Ok - so now I thought about it, it's clear why it's needed. The shared_ptr means that it holds the reference until the 'end' and therefore might be the cause of calling the destructor, therefore it needs to be public... So this really is part of the 'returning a shared_ptr to Camera in the public API', and should be in the next patch. Either that, or the Camera just gets returned as a non-shared pointer. -- Kieran > > > > :-( The 'HACK' here seems to be the only thing that would count as a > blocker to integration. > > I wonder if this is really a HACK or if we just need to explain that the > destructor needs to be public to support the python bindings, but I > myself don't understand the reasoning itself. > > Is the python code the only instance where we create a shared_ptr for > the Camera? If that's true - then perhaps this patch should actually be > squashed with the one that returns the Camera as a shared_ptr... > > -- > Kieran > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > --- > > include/libcamera/camera.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index a7759ccb..88a61ff5 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -104,12 +104,12 @@ public: > > int start(const ControlList *controls = nullptr); > > int stop(); > > > > + ~Camera(); > > private: > > LIBCAMERA_DISABLE_COPY(Camera) > > > > Camera(std::unique_ptr<Private> d, const std::string &id, > > const std::set<Stream *> &streams); > > - ~Camera(); > > > > friend class PipelineHandler; > > void disconnect(); > > -- > > 2.25.1 > >
On 09/12/2021 12:05, Kieran Bingham wrote: > Quoting Tomi Valkeinen (2021-12-09 09:29:02) >> pybind11 needs a public destructor for Camera to be able to manage the >> shared_ptr. It's not clear why this is needed, and can it be fixed in >> pybind11. >> > > :-( The 'HACK' here seems to be the only thing that would count as a > blocker to integration. > > I wonder if this is really a HACK or if we just need to explain that the > destructor needs to be public to support the python bindings, but I > myself don't understand the reasoning itself. > > Is the python code the only instance where we create a shared_ptr for > the Camera? If that's true - then perhaps this patch should actually be > squashed with the one that returns the Camera as a shared_ptr... This is not related to the patch 2. pybind11 needs a holder type that is used to manage the references to the C++ instances. By default it's unique_ptr, but can be defined. We use shared_ptr for many classes (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr). I think normally it makes sense that the destructor must be public: if pybind11 wrapper code can create the instances, it can also destruct them. Here we don't create Cameras, nor destruct them, but the related code is still created, causing the build error. I believe it should be possible to fix this in pybind11, by making the template code to detect these kind of classes and skip the destructor code. Maybe. Somehow. I have no idea how =). Tomi
Quoting Tomi Valkeinen (2021-12-09 10:22:16) > On 09/12/2021 12:05, Kieran Bingham wrote: > > Quoting Tomi Valkeinen (2021-12-09 09:29:02) > >> pybind11 needs a public destructor for Camera to be able to manage the > >> shared_ptr. It's not clear why this is needed, and can it be fixed in > >> pybind11. > >> > > > > :-( The 'HACK' here seems to be the only thing that would count as a > > blocker to integration. > > > > I wonder if this is really a HACK or if we just need to explain that the > > destructor needs to be public to support the python bindings, but I > > myself don't understand the reasoning itself. > > > > Is the python code the only instance where we create a shared_ptr for > > the Camera? If that's true - then perhaps this patch should actually be > > squashed with the one that returns the Camera as a shared_ptr... > > This is not related to the patch 2. > > pybind11 needs a holder type that is used to manage the references to > the C++ instances. By default it's unique_ptr, but can be defined. We > use shared_ptr for many classes > (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr). > > I think normally it makes sense that the destructor must be public: if > pybind11 wrapper code can create the instances, it can also destruct > them. Here we don't create Cameras, nor destruct them, but the related > code is still created, causing the build error. > > I believe it should be possible to fix this in pybind11, by making the > template code to detect these kind of classes and skip the destructor > code. Maybe. Somehow. I have no idea how =). But how could a shared_ptr skip calling the destructor? Isn't the purpose of a shared_ptr to keep the object 'alive' until the last user is removed? Therefore the shared_ptr has to be able to destruct the object? Is there some magic that means the destructor can still be private for a shared_ptr in that instance? A quick google on shared_ptr private destructor brings up: https://stackoverflow.com/a/8202667 Saying it can be solved by making a custom deleter for the shared pointer. Is that what we're missing perhaps? -- Kieran
Hi Kieran, On Thu, Dec 09, 2021 at 10:34:51AM +0000, Kieran Bingham wrote: > Quoting Tomi Valkeinen (2021-12-09 10:22:16) > > On 09/12/2021 12:05, Kieran Bingham wrote: > > > Quoting Tomi Valkeinen (2021-12-09 09:29:02) > > >> pybind11 needs a public destructor for Camera to be able to manage the > > >> shared_ptr. It's not clear why this is needed, and can it be fixed in > > >> pybind11. > > > > > > :-( The 'HACK' here seems to be the only thing that would count as a > > > blocker to integration. > > > > > > I wonder if this is really a HACK or if we just need to explain that the > > > destructor needs to be public to support the python bindings, but I > > > myself don't understand the reasoning itself. > > > > > > Is the python code the only instance where we create a shared_ptr for > > > the Camera? If that's true - then perhaps this patch should actually be > > > squashed with the one that returns the Camera as a shared_ptr... > > > > This is not related to the patch 2. > > > > pybind11 needs a holder type that is used to manage the references to > > the C++ instances. By default it's unique_ptr, but can be defined. We > > use shared_ptr for many classes > > (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr). > > > > I think normally it makes sense that the destructor must be public: if > > pybind11 wrapper code can create the instances, it can also destruct > > them. Here we don't create Cameras, nor destruct them, but the related > > code is still created, causing the build error. > > > > I believe it should be possible to fix this in pybind11, by making the > > template code to detect these kind of classes and skip the destructor > > code. Maybe. Somehow. I have no idea how =). > > But how could a shared_ptr skip calling the destructor? Isn't the > purpose of a shared_ptr to keep the object 'alive' until the last user > is removed? Therefore the shared_ptr has to be able to destruct the > object? > > Is there some magic that means the destructor can still be private for a > shared_ptr in that instance? > > A quick google on shared_ptr private destructor brings up: > https://stackoverflow.com/a/8202667 > > Saying it can be solved by making a custom deleter for the shared > pointer. Is that what we're missing perhaps? We already have a deleter, see Camera::create().
On 09/12/2021 12:34, Kieran Bingham wrote: > Quoting Tomi Valkeinen (2021-12-09 10:22:16) >> On 09/12/2021 12:05, Kieran Bingham wrote: >>> Quoting Tomi Valkeinen (2021-12-09 09:29:02) >>>> pybind11 needs a public destructor for Camera to be able to manage the >>>> shared_ptr. It's not clear why this is needed, and can it be fixed in >>>> pybind11. >>>> >>> >>> :-( The 'HACK' here seems to be the only thing that would count as a >>> blocker to integration. >>> >>> I wonder if this is really a HACK or if we just need to explain that the >>> destructor needs to be public to support the python bindings, but I >>> myself don't understand the reasoning itself. >>> >>> Is the python code the only instance where we create a shared_ptr for >>> the Camera? If that's true - then perhaps this patch should actually be >>> squashed with the one that returns the Camera as a shared_ptr... >> >> This is not related to the patch 2. >> >> pybind11 needs a holder type that is used to manage the references to >> the C++ instances. By default it's unique_ptr, but can be defined. We >> use shared_ptr for many classes >> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr). >> >> I think normally it makes sense that the destructor must be public: if >> pybind11 wrapper code can create the instances, it can also destruct >> them. Here we don't create Cameras, nor destruct them, but the related >> code is still created, causing the build error. >> >> I believe it should be possible to fix this in pybind11, by making the >> template code to detect these kind of classes and skip the destructor >> code. Maybe. Somehow. I have no idea how =). > > But how could a shared_ptr skip calling the destructor? Isn't the > purpose of a shared_ptr to keep the object 'alive' until the last user > is removed? Therefore the shared_ptr has to be able to destruct the > object? > > Is there some magic that means the destructor can still be private for a > shared_ptr in that instance? > > A quick google on shared_ptr private destructor brings up: > https://stackoverflow.com/a/8202667 > > Saying it can be solved by making a custom deleter for the shared > pointer. Is that what we're missing perhaps? That is what is done in Camera.cpp, Camera::create(). Afaik that is the only place where cameras are instantiated, and it, of course, has access to the destructor. The pybind11 template code generates code to manage the wrapped class, and I think that code includes constructing and destructing of the instance. Or maybe only destructing, as otherwise we should get a similar error for the constructor if that is required. Tomi
On Thu, Dec 09, 2021 at 12:54:07PM +0200, Tomi Valkeinen wrote: > On 09/12/2021 12:34, Kieran Bingham wrote: > > Quoting Tomi Valkeinen (2021-12-09 10:22:16) > >> On 09/12/2021 12:05, Kieran Bingham wrote: > >>> Quoting Tomi Valkeinen (2021-12-09 09:29:02) > >>>> pybind11 needs a public destructor for Camera to be able to manage the > >>>> shared_ptr. It's not clear why this is needed, and can it be fixed in > >>>> pybind11. > >>>> > >>> > >>> :-( The 'HACK' here seems to be the only thing that would count as a > >>> blocker to integration. > >>> > >>> I wonder if this is really a HACK or if we just need to explain that the > >>> destructor needs to be public to support the python bindings, but I > >>> myself don't understand the reasoning itself. > >>> > >>> Is the python code the only instance where we create a shared_ptr for > >>> the Camera? If that's true - then perhaps this patch should actually be > >>> squashed with the one that returns the Camera as a shared_ptr... > >> > >> This is not related to the patch 2. > >> > >> pybind11 needs a holder type that is used to manage the references to > >> the C++ instances. By default it's unique_ptr, but can be defined. We > >> use shared_ptr for many classes > >> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr). > >> > >> I think normally it makes sense that the destructor must be public: if > >> pybind11 wrapper code can create the instances, it can also destruct > >> them. Here we don't create Cameras, nor destruct them, but the related > >> code is still created, causing the build error. > >> > >> I believe it should be possible to fix this in pybind11, by making the > >> template code to detect these kind of classes and skip the destructor > >> code. Maybe. Somehow. I have no idea how =). > > > > But how could a shared_ptr skip calling the destructor? Isn't the > > purpose of a shared_ptr to keep the object 'alive' until the last user > > is removed? Therefore the shared_ptr has to be able to destruct the > > object? > > > > Is there some magic that means the destructor can still be private for a > > shared_ptr in that instance? > > > > A quick google on shared_ptr private destructor brings up: > > https://stackoverflow.com/a/8202667 > > > > Saying it can be solved by making a custom deleter for the shared > > pointer. Is that what we're missing perhaps? > > That is what is done in Camera.cpp, Camera::create(). Afaik that is the > only place where cameras are instantiated, and it, of course, has access > to the destructor. > > The pybind11 template code generates code to manage the wrapped class, > and I think that code includes constructing and destructing of the > instance. Or maybe only destructing, as otherwise we should get a > similar error for the constructor if that is required. If std::shared_ptr<> causes issue, we could manually implement a PyCamera (name to be bikeshedded, could be just Camera in a py:: namespace) C++ class that would wrap std::shared_ptr<Camera>.
On 09/12/2021 18:46, Laurent Pinchart wrote: > On Thu, Dec 09, 2021 at 12:54:07PM +0200, Tomi Valkeinen wrote: >> On 09/12/2021 12:34, Kieran Bingham wrote: >>> Quoting Tomi Valkeinen (2021-12-09 10:22:16) >>>> On 09/12/2021 12:05, Kieran Bingham wrote: >>>>> Quoting Tomi Valkeinen (2021-12-09 09:29:02) >>>>>> pybind11 needs a public destructor for Camera to be able to manage the >>>>>> shared_ptr. It's not clear why this is needed, and can it be fixed in >>>>>> pybind11. >>>>>> >>>>> >>>>> :-( The 'HACK' here seems to be the only thing that would count as a >>>>> blocker to integration. >>>>> >>>>> I wonder if this is really a HACK or if we just need to explain that the >>>>> destructor needs to be public to support the python bindings, but I >>>>> myself don't understand the reasoning itself. >>>>> >>>>> Is the python code the only instance where we create a shared_ptr for >>>>> the Camera? If that's true - then perhaps this patch should actually be >>>>> squashed with the one that returns the Camera as a shared_ptr... >>>> >>>> This is not related to the patch 2. >>>> >>>> pybind11 needs a holder type that is used to manage the references to >>>> the C++ instances. By default it's unique_ptr, but can be defined. We >>>> use shared_ptr for many classes >>>> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr). >>>> >>>> I think normally it makes sense that the destructor must be public: if >>>> pybind11 wrapper code can create the instances, it can also destruct >>>> them. Here we don't create Cameras, nor destruct them, but the related >>>> code is still created, causing the build error. >>>> >>>> I believe it should be possible to fix this in pybind11, by making the >>>> template code to detect these kind of classes and skip the destructor >>>> code. Maybe. Somehow. I have no idea how =). >>> >>> But how could a shared_ptr skip calling the destructor? Isn't the >>> purpose of a shared_ptr to keep the object 'alive' until the last user >>> is removed? Therefore the shared_ptr has to be able to destruct the >>> object? >>> >>> Is there some magic that means the destructor can still be private for a >>> shared_ptr in that instance? >>> >>> A quick google on shared_ptr private destructor brings up: >>> https://stackoverflow.com/a/8202667 >>> >>> Saying it can be solved by making a custom deleter for the shared >>> pointer. Is that what we're missing perhaps? >> >> That is what is done in Camera.cpp, Camera::create(). Afaik that is the >> only place where cameras are instantiated, and it, of course, has access >> to the destructor. >> >> The pybind11 template code generates code to manage the wrapped class, >> and I think that code includes constructing and destructing of the >> instance. Or maybe only destructing, as otherwise we should get a >> similar error for the constructor if that is required. > > If std::shared_ptr<> causes issue, we could manually implement a > PyCamera (name to be bikeshedded, could be just Camera in a py:: > namespace) C++ class that would wrap std::shared_ptr<Camera>. Looks like there's a proposed fix: https://github.com/jagerman/pybind11/commit/53be81931f35313f70affc9826bdfed9820cce2c but the pull-req hasn't been approved: https://github.com/pybind/pybind11/pull/2067 There also seems to be a fork (?) or pybind11 which implements something related, which would possibly help here (or not): https://github.com/pybind/pybind11/blob/49f8f60ec4254e0f55db3c095c945210bcb43ad2/README_smart_holder.rst I'm a bit reluctant going to the wrapper class just to avoid this issue, as I fear it will create a bit of a mess in the code. Tomi
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index a7759ccb..88a61ff5 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -104,12 +104,12 @@ public: int start(const ControlList *controls = nullptr); int stop(); + ~Camera(); private: LIBCAMERA_DISABLE_COPY(Camera) Camera(std::unique_ptr<Private> d, const std::string &id, const std::set<Stream *> &streams); - ~Camera(); friend class PipelineHandler; void disconnect();
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- include/libcamera/camera.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)