| Message ID | 20200918152019.784315-5-tomi.valkeinen@iki.fi |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 > +}
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@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 >> +} >
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@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 > >> +}
On 04/10/2020 22:46, Laurent Pinchart wrote: > 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. Realtime requirement sounds scary. Can you elaborate? Wouldn't a slow application side normally just cause the capture side to re-use the current buffer and overwrite it? If it's a requirement that the callbacks may not block for any noticeable time, then I agree, this needs some redesign. But I could probably just add the eventfd on the pybind11 level, without need to change it in the libcamera c++ side? Or maybe that's what you meant =). Tomi
On Sun, Oct 04, 2020 at 10:55:39PM +0300, Tomi Valkeinen wrote: > On 04/10/2020 22:46, Laurent Pinchart wrote: > > 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. > > Realtime requirement sounds scary. Can you elaborate? Wouldn't a slow > application side normally just cause the capture side to re-use the > current buffer and overwrite it? It's not just about image capture for applications, but all the algorithms that run behind the scenes that consume statistics and produce parameters for the sensor and ISP. Control loops don't like delays. > If it's a requirement that the callbacks may not block for any > noticeable time, then I agree, this needs some redesign. But I could > probably just add the eventfd on the pybind11 level, without need to > change it in the libcamera c++ side? Or maybe that's what you meant =). That's exactly what I meant :-)
diff --git a/meson.build b/meson.build index c58d458..3d1c797 100644 --- a/meson.build +++ b/meson.build @@ -104,6 +104,7 @@ libcamera_includes = include_directories('include') subdir('include') subdir('src') subdir('utils') +subdir('py') # The documentation and test components are optional and can be disabled # through configuration values. They are enabled by default. diff --git a/meson_options.txt b/meson_options.txt index d2e07ef..45b88b6 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -32,3 +32,5 @@ option('v4l2', type : 'boolean', value : false, description : 'Compile the V4L2 compatibility layer') + +option('pycamera', type : 'feature', value : 'auto') diff --git a/py/meson.build b/py/meson.build new file mode 100644 index 0000000..42ffa22 --- /dev/null +++ b/py/meson.build @@ -0,0 +1 @@ +subdir('pycamera') diff --git a/py/pycamera/__init__.py b/py/pycamera/__init__.py new file mode 100644 index 0000000..c37571b --- /dev/null +++ b/py/pycamera/__init__.py @@ -0,0 +1,29 @@ +from .pycamera import * +from enum import Enum +import os +import struct +import mmap + +# Add a wrapper which returns an array of Cameras, which have keep-alive to the CameraManager +def __CameraManager__cameras(self): + cameras = [] + for i in range(self.num_cameras): + cameras.append(self.at(i)) + return cameras + + +CameraManager.cameras = property(__CameraManager__cameras) + +# Add a wrapper which returns an array of buffers, which have keep-alive to the FB allocator +def __FrameBufferAllocator__buffers(self, stream): + buffers = [] + for i in range(self.num_buffers(stream)): + buffers.append(self.at(stream, i)) + return buffers + +FrameBufferAllocator.buffers = __FrameBufferAllocator__buffers + +def __FrameBuffer__mmap(self, plane): + return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ) + +FrameBuffer.mmap = __FrameBuffer__mmap diff --git a/py/pycamera/meson.build b/py/pycamera/meson.build new file mode 100644 index 0000000..50bdfb8 --- /dev/null +++ b/py/pycamera/meson.build @@ -0,0 +1,35 @@ +# SPDX-License-Identifier: CC0-1.0 + +py3_dep = dependency('python3', required : get_option('pycamera')) + +if py3_dep.found() == false + subdir_done() +endif + +pycamera_sources = files([ + 'pymain.cpp', +]) + +pycamera_deps = [ + libcamera_dep, + py3_dep, +] + +includes = [ + '../../ext/pybind11/include', +] + +destdir = get_option('libdir') + '/python' + py3_dep.version() + '/site-packages/pycamera' + +pycamera = shared_module('pycamera', + pycamera_sources, + install : true, + install_dir : destdir, + name_prefix : '', + include_directories : includes, + dependencies : pycamera_deps) + +# Copy __init__.py to build dir so that we can run without installing +configure_file(input: '__init__.py', output: '__init__.py', copy: true) + +install_data(['__init__.py'], install_dir: destdir) diff --git a/py/pycamera/pymain.cpp b/py/pycamera/pymain.cpp new file mode 100644 index 0000000..569423a --- /dev/null +++ b/py/pycamera/pymain.cpp @@ -0,0 +1,169 @@ +#include <chrono> +#include <thread> +#include <fcntl.h> +#include <unistd.h> +#include <sys/mman.h> + +#include <pybind11/pybind11.h> +#include <pybind11/stl.h> +#include <pybind11/stl_bind.h> +#include <pybind11/functional.h> + +#include <libcamera/libcamera.h> + +namespace py = pybind11; + +using namespace std; +using namespace libcamera; + +PYBIND11_MODULE(pycamera, m) { + m.def("sleep", [](double s) { + py::gil_scoped_release release; + this_thread::sleep_for(std::chrono::duration<double>(s)); + }); + + py::class_<CameraManager>(m, "CameraManager") + // Call cm->start implicitly, as we can't use stop() either + .def(py::init([]() { + auto cm = make_unique<CameraManager>(); + cm->start(); + return cm; + })) + + //.def("start", &CameraManager::start) + + // stop() cannot be called, as CameraManager expects all Camera instances to be released before calling stop + // and we can't have such requirement in python, especially as we have a keep-alive from Camera to CameraManager. + // So we rely on GC and the keep-alives. + //.def("stop", &CameraManager::stop) + + .def_property_readonly("num_cameras", [](CameraManager& cm) { return cm.cameras().size(); }) + .def("at", [](CameraManager& cm, unsigned int idx) { return cm.cameras()[idx]; }, py::keep_alive<0, 1>()) + ; + + py::class_<Camera, shared_ptr<Camera>>(m, "Camera") + .def_property_readonly("id", &Camera::id) + .def("acquire", &Camera::acquire) + .def("release", &Camera::release) + .def("start", &Camera::start) + .def("stop", [](shared_ptr<Camera>& self) { + // Camera::stop can cause callbacks to be invoked, so we must release GIL + py::gil_scoped_release release; + self->stop(); + }) + .def("generateConfiguration", &Camera::generateConfiguration) + .def("configure", &Camera::configure) + + // XXX created requests MUST be queued to be freed, python will not free them + .def("createRequest", &Camera::createRequest, py::arg("cookie") = 0, py::return_value_policy::reference_internal) + .def("queueRequest", &Camera::queueRequest) + + .def_property("requestCompleted", + nullptr, + [](shared_ptr<Camera>& self, function<void(Request*)> f) { + if (f) { + self->requestCompleted.connect(function<void(Request*)>([f = move(f)](Request* req) { + // Called from libcamera's internal thread, so need to get GIL + py::gil_scoped_acquire acquire; + f(req); + })); + } else { + // XXX Disconnects all, as we have no means to disconnect the specific std::function + self->requestCompleted.disconnect(); + } + } + ) + + .def_property("bufferCompleted", + nullptr, + [](shared_ptr<Camera>& self, function<void(Request*, FrameBuffer*)> f) { + if (f) { + self->bufferCompleted.connect(function<void(Request*, FrameBuffer* fb)>([f = move(f)](Request* req, FrameBuffer* fb) { + // Called from libcamera's internal thread, so need to get GIL + py::gil_scoped_acquire acquire; + f(req, fb); + })); + } else { + // XXX Disconnects all, as we have no means to disconnect the specific std::function + self->bufferCompleted.disconnect(); + } + } + ) + + ; + + py::class_<CameraConfiguration>(m, "CameraConfiguration") + .def("at", (StreamConfiguration& (CameraConfiguration::*)(unsigned int))&CameraConfiguration::at, + py::return_value_policy::reference_internal) + .def("validate", &CameraConfiguration::validate) + .def_property_readonly("size", &CameraConfiguration::size) + .def_property_readonly("empty", &CameraConfiguration::empty) + ; + + py::class_<StreamConfiguration>(m, "StreamConfiguration") + .def("toString", &StreamConfiguration::toString) + .def_property_readonly("stream", &StreamConfiguration::stream, + py::return_value_policy::reference_internal) + .def_property("width", + [](StreamConfiguration& c) { return c.size.width; }, + [](StreamConfiguration& c, unsigned int w) { c.size.width = w; } + ) + .def_property("height", + [](StreamConfiguration& c) { return c.size.height; }, + [](StreamConfiguration& c, unsigned int h) { c.size.height = h; } + ) + .def_property("fmt", + [](StreamConfiguration& c) { return c.pixelFormat.toString(); }, + [](StreamConfiguration& c, string fmt) { c.pixelFormat = PixelFormat::fromString(fmt); } + ) + ; + + py::enum_<StreamRole>(m, "StreamRole") + .value("StillCapture", StreamRole::StillCapture) + .value("StillCaptureRaw", StreamRole::StillCaptureRaw) + .value("VideoRecording", StreamRole::VideoRecording) + .value("Viewfinder", StreamRole::Viewfinder) + ; + + py::class_<FrameBufferAllocator>(m, "FrameBufferAllocator") + .def(py::init<shared_ptr<Camera>>(), py::keep_alive<1, 2>()) + .def("allocate", &FrameBufferAllocator::allocate) + .def("free", &FrameBufferAllocator::free) + .def("num_buffers", [](FrameBufferAllocator& fa, Stream* stream) { return fa.buffers(stream).size(); }) + .def("at", [](FrameBufferAllocator& fa, Stream* stream, unsigned int idx) { return fa.buffers(stream).at(idx).get(); }, + py::return_value_policy::reference_internal) + ; + + py::class_<FrameBuffer, unique_ptr<FrameBuffer, py::nodelete>>(m, "FrameBuffer") + .def_property_readonly("metadata", &FrameBuffer::metadata, py::return_value_policy::reference_internal) + .def("length", [](FrameBuffer& fb, uint32_t idx) { + const FrameBuffer::Plane &plane = fb.planes()[idx]; + return plane.length; + }) + .def("fd", [](FrameBuffer& fb, uint32_t idx) { + const FrameBuffer::Plane &plane = fb.planes()[idx]; + return plane.fd.fd(); + }) + ; + + py::class_<Stream, unique_ptr<Stream, py::nodelete>>(m, "Stream") + ; + + py::class_<Request, unique_ptr<Request, py::nodelete>>(m, "Request") + .def("addBuffer", &Request::addBuffer) + .def_property_readonly("status", &Request::status) + .def_property_readonly("buffers", &Request::buffers) + ; + + + py::enum_<Request::Status>(m, "RequestStatus") + .value("Pending", Request::RequestPending) + .value("Complete", Request::RequestComplete) + .value("Cancelled", Request::RequestCancelled) + ; + + py::class_<FrameMetadata>(m, "FrameMetadata") + .def_property_readonly("sequence", [](FrameMetadata& data) { return data.sequence; }) + .def("bytesused", [](FrameMetadata& data, uint32_t idx) { return data.planes[idx].bytesused; }) + ; +} diff --git a/py/test/run-valgrind.sh b/py/test/run-valgrind.sh new file mode 100755 index 0000000..623188e --- /dev/null +++ b/py/test/run-valgrind.sh @@ -0,0 +1,3 @@ +#!/bin/bash + +PYTHONMALLOC=malloc PYTHONPATH=../../build/debug/py valgrind --suppressions=valgrind-pycamera.supp --leak-check=full --show-leak-kinds=definite --gen-suppressions=yes python3 test.py $* diff --git a/py/test/run.sh b/py/test/run.sh new file mode 100755 index 0000000..035d3ea --- /dev/null +++ b/py/test/run.sh @@ -0,0 +1,3 @@ +#!/bin/bash + +PYTHONPATH=../../build/debug/py python3 test.py $* diff --git a/py/test/test.py b/py/test/test.py new file mode 100755 index 0000000..0f874d3 --- /dev/null +++ b/py/test/test.py @@ -0,0 +1,177 @@ +#!/usr/bin/python3 + +import pycamera as pycam +import time +import binascii +import argparse + +parser = argparse.ArgumentParser() +parser.add_argument("-n", "--num-frames", type=int, default=10) +parser.add_argument("-c", "--print-crc", action="store_true") +parser.add_argument("-s", "--save-frames", action="store_true") +parser.add_argument("-m", "--max-cameras", type=int) +args = parser.parse_args() + +cm = pycam.CameraManager() + +cameras = cm.cameras + +if len(cameras) == 0: + print("No cameras") + exit(0) + +print("Cameras:") +for c in cameras: + print(" {}".format(c.id)) + +contexts = [] + +for i in range(len(cameras)): + contexts.append({ "camera": cameras[i], "id": i }) + if args.max_cameras and args.max_cameras - 1 == i: + break + +for ctx in contexts: + ctx["camera"].acquire() + +def configure_camera(ctx): + camera = ctx["camera"] + + # Configure + + config = camera.generateConfiguration([pycam.StreamRole.Viewfinder]) + stream_config = config.at(0) + + #stream_config.width = 160; + #stream_config.height = 120; + #stream_config.fmt = "YUYV" + + print("Cam {}: stream config {}".format(ctx["id"], stream_config.toString())) + + camera.configure(config); + + # Allocate buffers + + stream = stream_config.stream + + allocator = pycam.FrameBufferAllocator(camera); + ret = allocator.allocate(stream) + if ret < 0: + print("Can't allocate buffers") + exit(-1) + + allocated = allocator.num_buffers(stream) + print("Cam {}: Allocated {} buffers for stream".format(ctx["id"], allocated)) + + # Create Requests + + requests = [] + buffers = allocator.buffers(stream) + + for buffer in buffers: + request = camera.createRequest() + if request == None: + print("Can't create request") + exit(-1) + + ret = request.addBuffer(stream, buffer) + if ret < 0: + print("Can't set buffer for request") + exit(-1) + + requests.append(request) + + ctx["allocator"] = allocator + ctx["requests"] = requests + + +def buffer_complete_cb(ctx, req, fb): + print("Cam {}: Buf {} Complete: {}".format(ctx["id"], ctx["bufs_completed"], req.status)) + + with fb.mmap(0) as b: + if args.print_crc: + crc = binascii.crc32(b) + print("Cam {}: CRC {:#x}".format(ctx["id"], crc)) + + if args.save_frames: + id = ctx["id"] + num = ctx["bufs_completed"] + filename = "frame-{}-{}.data".format(id, num) + with open(filename, "wb") as f: + f.write(b) + print("Cam {}: Saved {}".format(ctx["id"], filename)) + + ctx["bufs_completed"] += 1 + +def req_complete_cb(ctx, req): + camera = ctx["camera"] + + print("Cam {}: Req {} Complete: {}".format(ctx["id"], ctx["reqs_completed"], req.status)) + + bufs = req.buffers + for stream, fb in bufs.items(): + meta = fb.metadata + print("Cam {}: Buf seq {}, bytes {}".format(ctx["id"], meta.sequence, meta.bytesused(0))) + + ctx["reqs_completed"] += 1 + + if ctx["reqs_queued"] < args.num_frames: + request = camera.createRequest() + if request == None: + print("Can't create request") + exit(-1) + + for stream, fb in bufs.items(): + ret = request.addBuffer(stream, fb) + if ret < 0: + print("Can't set buffer for request") + exit(-1) + + camera.queueRequest(request) + ctx["reqs_queued"] += 1 + + +def setup_callbacks(ctx): + camera = ctx["camera"] + + ctx["reqs_queued"] = 0 + ctx["reqs_completed"] = 0 + ctx["bufs_completed"] = 0 + + camera.requestCompleted = lambda req, ctx = ctx: req_complete_cb(ctx, req) + camera.bufferCompleted = lambda req, fb, ctx = ctx: buffer_complete_cb(ctx, req, fb) + +def queue_requests(ctx): + camera = ctx["camera"] + requests = ctx["requests"] + + camera.start() + + for request in requests: + camera.queueRequest(request) + ctx["reqs_queued"] += 1 + + + +for ctx in contexts: + configure_camera(ctx) + setup_callbacks(ctx) + +for ctx in contexts: + queue_requests(ctx) + + +print("Processing...") + +# Need to release GIL here, so that callbacks can be called +while any(ctx["reqs_completed"] < args.num_frames for ctx in contexts): + pycam.sleep(0.1) + +print("Exiting...") + +for ctx in contexts: + camera = ctx["camera"] + camera.stop() + camera.release() + +print("Done") 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 + 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 +}
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. - 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. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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