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

[Tomi Valkeinen]

On 04/10/2020 21:49, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> 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.

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
>> +}
>