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

[Laurent Pinchart]

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.

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

> 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 :-)

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