[libcamera-devel] [RFC 4/4] libcamera python bindings

Sun Oct 4 21:46:30 CEST 2020

Hi Tomi,

On Sun, Oct 04, 2020 at 10:42:33PM +0300, Tomi Valkeinen wrote:
> On 04/10/2020 21:49, Laurent Pinchart wrote:
> > On Fri, Sep 18, 2020 at 06:20:19PM +0300, Tomi Valkeinen wrote:
> >> Main issues:
> >>
> >> - Memory management in general. Who owns the object, how to pass
> >>    ownership, etc.
> >>
> >> - Specifically, Request is currently broken. We can't, afaik, pass
> >>    ownership around. So currently Python never frees a Request, and if
> >>    the Request is not given to Camera::queueRequest, it will leak.
> >>
> >> - The forced threading causes some headache. Need to take care to use
> >>    gil_scoped_release when C++ context can invoke callbacks, and
> >>    gil_scoped_acquire at the invoke wrapper.
> > 
> > I've given this a thought, and I wonder if we could solve it by using an
> > event queue and an eventfd to move everything to the Python thread.
> > Signals would be connected to internal functions that would push a new
> > instance of a message object that binds the signal name and its
> > arguments. An eventfd would be used to wake up the python side, which
> > would call a process() function that would then dispatch the messages to
> > Python code.
> 
> Yeah, at least it would be an interesting comparison case. Curiously, 
> things seem to work very well. I guess CPython frees the lock quite 
> often without me specifically doing anything. To be honest, I don't know 
> why it works.
> 
> That said, I had had deadlock issues previously, and I recently got more 
> of them when I tried things with a python thread (doing background work 
> while running interactive python shell).
> 
> I think it may not be worth spending time with eventfd until it's clear 
> it will give some benefit, and at the moment it's not quite clear to me.
> 
> I can always move the execution from libcamera's thread to my thread in 
> one way or another. The main question is, will the GIL be freed often 
> enough to allow the callbacks to be called. Maybe it is freed often, as 
> I guess that's what python's own threads need also.

Blocking the callbacks will not only delay their delivery to the
application, but also potentially break the realtime requirements of
pipeline handlers. I think we should really not be doing so.

> But I need to understand this better until I find the answer to that.
> 
> >> - Callbacks. Difficult to attach context to the callbacks. I solved it
> >>    with BoundMethodFunction and using lambda captures
> >>
> >> - Need public Camera destructor. It is not clear to me why C++ allows it
> >>    to be private, but pybind11 doesn't.
> > 
> > Camera is meant to be managed as a shared_ptr<>, so if Python tries to
> > delete is explicitly, there's probably something bad happening
> > somewhere.
> > 
> > In file included from ../../src/py/pycamera/pymain.cpp:2:
> > In file included from /usr/include/c++/v1/thread:90:
> > In file included from /usr/include/c++/v1/functional:504:
> > /usr/include/c++/v1/memory:2363:12: error: calling a private destructor of class 'libcamera::Camera'
> >      delete __ptr;
> >             ^
> > /usr/include/c++/v1/memory:2618:7: note: in instantiation of member function 'std::__1::default_delete<libcamera::Camera>::operator()' requested here
> >        __ptr_.second()(__tmp);
> >        ^
> > /usr/include/c++/v1/memory:2572:19: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::reset' requested here
> >    ~unique_ptr() { reset(); }
> >                    ^
> > /usr/include/c++/v1/memory:3932:21: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::~unique_ptr' requested here
> >      unique_ptr<_Yp> __hold(__p);
> >                      ^
> > ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1309:61: note: in instantiation of function template specialization 'std::__1::shared_ptr<libcamera::Camera>::shared_ptr<libcamera::Camera>' requested here
> >              new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());
> >                                                              ^
> > ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1346:9: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_holder<libcamera::Camera>' requested here
> >          init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
> >          ^
> > ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1080:32: note: in instantiation of member function 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_instance' requested here
> >          record.init_instance = init_instance;
> >                                 ^
> > ../../src/py/pycamera/pymain.cpp:116:2: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::class_<>' requested here
> >          py::class_<Camera, shared_ptr<Camera>>(m, "Camera")
> >          ^
> > ../../include/libcamera/camera.h:108:2: note: declared private here
> >          ~Camera();
> >          ^
> > 1 error generated.
> > 
> > The issue may come from the fact that pybin11 tries to create a
> > std::shared_ptr<> manually to hold the Camera pointer, instead of
> > copying the existing std::shared_ptr<>, but I'm not entirely sure.
> 
> Yep, no clear idea... "init_instance" there sounds like pybind11 is also 
> creating code to that allows creating Camera, which I guess then implies 
> also the need to see the destructor.
> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at iki.fi>
> >> ---
> >>   meson.build                    |   1 +
> >>   meson_options.txt              |   2 +
> >>   py/meson.build                 |   1 +
> >>   py/pycamera/__init__.py        |  29 ++++++
> >>   py/pycamera/meson.build        |  35 +++++++
> >>   py/pycamera/pymain.cpp         | 169 +++++++++++++++++++++++++++++++
> >>   py/test/run-valgrind.sh        |   3 +
> >>   py/test/run.sh                 |   3 +
> >>   py/test/test.py                | 177 +++++++++++++++++++++++++++++++++
> >>   py/test/valgrind-pycamera.supp |  17 ++++
> >>   10 files changed, 437 insertions(+)
> >>   create mode 100644 py/meson.build
> >>   create mode 100644 py/pycamera/__init__.py
> >>   create mode 100644 py/pycamera/meson.build
> >>   create mode 100644 py/pycamera/pymain.cpp
> >>   create mode 100755 py/test/run-valgrind.sh
> >>   create mode 100755 py/test/run.sh
> >>   create mode 100755 py/test/test.py
> >>   create mode 100644 py/test/valgrind-pycamera.supp
> > 
> > [snip]
> > 
> >> diff --git a/py/test/valgrind-pycamera.supp b/py/test/valgrind-pycamera.supp
> >> new file mode 100644
> >> index 0000000..98c693f
> >> --- /dev/null
> >> +++ b/py/test/valgrind-pycamera.supp
> >> @@ -0,0 +1,17 @@
> >> +{
> >> +   <insert_a_suppression_name_here>
> >> +   Memcheck:Leak
> >> +   match-leak-kinds: definite
> >> +   fun:_Znwm
> > 
> > This is new(unsigned long). No wonder you don't have memory leaks
> > anymore :-)
> 
> That's just one 104 byte allocation at init time, with the callstack 
> below. I don't know what's it about, but look like one time alloc when 
> the pybind11 camera module is created.
> 
> >> +   fun:_ZN8pybind116moduleC1EPKcS2_
> >> +   fun:PyInit_pycamera
> >> +   fun:_PyImport_LoadDynamicModuleWithSpec
> >> +   obj:/usr/bin/python3.8
> >> +   obj:/usr/bin/python3.8
> >> +   fun:PyVectorcall_Call
> >> +   fun:_PyEval_EvalFrameDefault
> >> +   fun:_PyEval_EvalCodeWithName
> >> +   fun:_PyFunction_Vectorcall
> >> +   fun:_PyEval_EvalFrameDefault
> >> +   fun:_PyFunction_Vectorcall
> >> +}

-- 
Regards,

Laurent Pinchart