| Message ID | 20201127133738.880859-2-tomi.valkeinen@iki.fi |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. On Fri, Nov 27, 2020 at 03:37:35PM +0200, Tomi Valkeinen wrote: > Not clear why we need this with pybind11... We'll have to figure it out :-) I've been considering a while ago handling camera instances a bit differently, with a facade object create by libcamera (and returned a unique pointer), with a reference-counted (through shared_ptr) backend object not visible to applications. We could implement this, either in the libcamera core, or in the python bindings, if it helps. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> > --- > 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 5c5f1a05..d8e0bdbc 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -106,10 +106,10 @@ public: > int start(); > int stop(); > > + ~Camera(); > private: > Camera(PipelineHandler *pipe, const std::string &id, > const std::set<Stream *> &streams); > - ~Camera(); > > friend class PipelineHandler; > void disconnect();
On 30/11/2020 01:27, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Fri, Nov 27, 2020 at 03:37:35PM +0200, Tomi Valkeinen wrote: >> Not clear why we need this with pybind11... > > We'll have to figure it out :-) My guess is that pybind11 generates code to create a shared_ptr<Camera>. You need a public destructor for that. Ah, no, Camera constructor is private too, so that cannot be the case. > I've been considering a while ago handling camera instances a bit > differently, with a facade object create by libcamera (and returned a > unique pointer), with a reference-counted (through shared_ptr) backend > object not visible to applications. We could implement this, either in > the libcamera core, or in the python bindings, if it helps. What would be the benefit of that approach in libcamera core? Isn't that just re-implementing the same feature with more complex code? Unless you want to hide something with the backend object that can't be hidden at the moment. I'm not sure, but we might have the same problem with unique_ptr too. If something in pybind11 requires the destructor to be visible with shared_ptr, that could also be the case with unique_ptr. I have considered adding facades/wrappers for some classes in the py bindings, so that I could store extra state in those objects. But it does get complex, as I need to re-use existing objects instead of just creating a fresh one every time I get an instance from c++ side. That's a bit different than what you're suggesting, but would also remove the use of shared_ptr<Camera> from the bindings itself. But if all this is just to avoid making the Camera destructor public, I have to ask if it's worth it =). Tomi
Hi Tomi, On Mon, Nov 30, 2020 at 04:26:48PM +0200, Tomi Valkeinen wrote: > On 30/11/2020 01:27, Laurent Pinchart wrote: > > Hi Tomi, > > > > Thank you for the patch. > > > > On Fri, Nov 27, 2020 at 03:37:35PM +0200, Tomi Valkeinen wrote: > >> Not clear why we need this with pybind11... > > > > We'll have to figure it out :-) > > My guess is that pybind11 generates code to create a shared_ptr<Camera>. You need a public > destructor for that. > > Ah, no, Camera constructor is private too, so that cannot be the case. In pybind11::class_::init_holder(), 1. /// Initialize holder object, variant 1: object derives from enable_shared_from_this 2. template <typename T> 3. static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, 4. const holder_type * /* unused */, const std::enable_shared_from_this<T> * /* dummy */) { 5. try { 6. auto sh = std::dynamic_pointer_cast<typename holder_type::element_type>( 7. v_h.value_ptr<type>()->shared_from_this()); 8. if (sh) { 9. new (std::addressof(v_h.holder<holder_type>())) holder_type(std::move(sh)); 10. v_h.set_holder_constructed(); 11. } 12. } catch (const std::bad_weak_ptr &) {} 13. 14. if (!v_h.holder_constructed() && inst->owned) { 15. new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>()); 16. v_h.set_holder_constructed(); 17. } 18. } On line 15 a holder_type is constructed. holder_type is std::shared_ptr<Camera>, given that the bindings use py::class_<Camera, shared_ptr<Camera>>(m, "Camera", py::dynamic_attr()) I think this code will never be executed in practice, as the holder should always be initialized from a std::shared_ptr<>, but the compiler can't know that. > > I've been considering a while ago handling camera instances a bit > > differently, with a facade object create by libcamera (and returned a > > unique pointer), with a reference-counted (through shared_ptr) backend > > object not visible to applications. We could implement this, either in > > the libcamera core, or in the python bindings, if it helps. > > What would be the benefit of that approach in libcamera core? Isn't that just re-implementing the > same feature with more complex code? Unless you want to hide something with the backend object that > can't be hidden at the moment. It was partly about hiding the Camera object private fields, but that has since been done through usage of the d-pointer design pattern. The other reason is to avoid proliferation of std::shared_ptr<> on the application side. It could help with other language bindings too. > I'm not sure, but we might have the same problem with unique_ptr too. If something in pybind11 > requires the destructor to be visible with shared_ptr, that could also be the case with unique_ptr. The destructor for the facade object would be visible. That's the whole point, it would be constructed by libcamera, but destroyed by applications. I'm however not sure how practical that would be, given that we wouldn't be able to expose a vector of cameras anymore (at least not in the same way). > I have considered adding facades/wrappers for some classes in the py bindings, so that I could store > extra state in those objects. But it does get complex, as I need to re-use existing objects instead > of just creating a fresh one every time I get an instance from c++ side. I have a feeling we'll have to do so at some point, for some of the objects, but have no proof for now :-) > That's a bit different than what you're suggesting, but would also remove the use of > shared_ptr<Camera> from the bindings itself. > > But if all this is just to avoid making the Camera destructor public, I have to ask if it's worth it =).
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 5c5f1a05..d8e0bdbc 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -106,10 +106,10 @@ public: int start(); int stop(); + ~Camera(); private: Camera(PipelineHandler *pipe, const std::string &id, const std::set<Stream *> &streams); - ~Camera(); friend class PipelineHandler; void disconnect();
Not clear why we need this with pybind11... Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> --- include/libcamera/camera.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)