[libcamera-devel] [RFC v2 3/4] libcamera python bindings
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 30 01:16:54 CET 2020
Hi Tomi,
Thank you for the patch.
On Fri, Nov 27, 2020 at 03:37:37PM +0200, 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.
>
> - Need public Camera destructor. It is not clear to me why C++ allows it
> to be private, but pybind11 doesn't.
Here's a partial review, I'll try to give the rest a go in the near
future.
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at iki.fi>
> ---
> .gitignore | 2 +
> meson_options.txt | 2 +
> src/meson.build | 1 +
> src/py/meson.build | 1 +
> src/py/pycamera/__init__.py | 11 +
> src/py/pycamera/meson.build | 38 +++
> src/py/pycamera/pymain.cpp | 382 +++++++++++++++++++++++++++++
> src/py/test/drmtest.py | 129 ++++++++++
> src/py/test/icam.py | 154 ++++++++++++
> src/py/test/run-valgrind.sh | 6 +
> src/py/test/run.sh | 3 +
> src/py/test/simplecamera.py | 198 +++++++++++++++
> src/py/test/test.py | 210 ++++++++++++++++
> src/py/test/valgrind-pycamera.supp | 17 ++
> subprojects/pybind11.wrap | 10 +
> 15 files changed, 1164 insertions(+)
> create mode 100644 src/py/meson.build
> create mode 100644 src/py/pycamera/__init__.py
> create mode 100644 src/py/pycamera/meson.build
> create mode 100644 src/py/pycamera/pymain.cpp
> create mode 100755 src/py/test/drmtest.py
> create mode 100755 src/py/test/icam.py
> create mode 100755 src/py/test/run-valgrind.sh
> create mode 100755 src/py/test/run.sh
> create mode 100644 src/py/test/simplecamera.py
> create mode 100755 src/py/test/test.py
> create mode 100644 src/py/test/valgrind-pycamera.supp
> create mode 100644 subprojects/pybind11.wrap
>
> diff --git a/.gitignore b/.gitignore
> index d3d73615..1f9dc7d1 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -5,3 +5,5 @@ build/
> patches/
> *.patch
> *.pyc
> +subprojects/packagecache/
> +subprojects/pybind11-2.3.0/
Let's move directories before files, and keep them alphabetically
sorted.
> diff --git a/meson_options.txt b/meson_options.txt
> index 53f2675e..ef995527 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -37,3 +37,5 @@ option('v4l2',
> type : 'boolean',
> value : false,
> description : 'Compile the V4L2 compatibility layer')
> +
> +option('pycamera', type : 'feature', value : 'auto')
A description would be nice. Can you format the option similarly to the
other ones ?
> diff --git a/src/meson.build b/src/meson.build
> index b9c7e759..61ec3991 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -23,3 +23,4 @@ if get_option('v4l2')
> endif
>
> subdir('gstreamer')
> +subdir('py')
We could create a bindings directory, to store other language bindings
(there should be a C binding in the future), but maybe that's overkill.
> diff --git a/src/py/meson.build b/src/py/meson.build
> new file mode 100644
> index 00000000..42ffa221
> --- /dev/null
> +++ b/src/py/meson.build
> @@ -0,0 +1 @@
> +subdir('pycamera')
> diff --git a/src/py/pycamera/__init__.py b/src/py/pycamera/__init__.py
> new file mode 100644
> index 00000000..ddb70096
> --- /dev/null
> +++ b/src/py/pycamera/__init__.py
> @@ -0,0 +1,11 @@
> +from .pycamera import *
> +from enum import Enum
> +import os
> +import struct
These three imports are not used.
> +import mmap
> +
> +
> +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/src/py/pycamera/meson.build b/src/py/pycamera/meson.build
> new file mode 100644
> index 00000000..9ff9b8ee
> --- /dev/null
> +++ b/src/py/pycamera/meson.build
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +py3_dep = dependency('python3', required : get_option('pycamera'))
> +
> +if py3_dep.found() == false
> + subdir_done()
> +endif
> +
> +pybind11_proj = subproject('pybind11')
> +pybind11_dep = pybind11_proj.get_variable('pybind11_dep')
> +
> +pycamera_sources = files([
> + 'pymain.cpp',
> +])
> +
> +pycamera_deps = [
> + libcamera_dep,
> + py3_dep,
> + pybind11_dep,
> +]
> +
> +pycamera_args = [ '-fvisibility=hidden' ]
> +pycamera_args += [ '-Wno-shadow' ]
Is this because pybind11 shadows variables ?
> +
> +destdir = get_option('libdir') + '/python' + py3_dep.version() + '/site-packages/pycamera'
> +
> +pycamera = shared_module('pycamera',
> + pycamera_sources,
> + install : true,
> + install_dir : destdir,
> + name_prefix : '',
> + dependencies : pycamera_deps,
> + cpp_args : pycamera_args)
> +
> +# Copy __init__.py to build dir so that we can run without installing
> +configure_file(input: '__init__.py', output: '__init__.py', copy: true)
You could also create a symlink. There's an example in the top-level
meson.build.
> +
> +install_data(['__init__.py'], install_dir: destdir)
Missing space after ':'.
> diff --git a/src/py/pycamera/pymain.cpp b/src/py/pycamera/pymain.cpp
> new file mode 100644
> index 00000000..bd1b9bdd
> --- /dev/null
> +++ b/src/py/pycamera/pymain.cpp
> @@ -0,0 +1,382 @@
Missing SPDX tag and initial comment block.
> +#include <chrono>
> +#include <thread>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <sys/eventfd.h>
> +#include <mutex>
Can you copy utils/hooks/post-commit to .git/hooks/ to ensure
checkstyle.py is run ? It should warn about incorrect alphabetical
order.
> +
> +#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;
We usually use the namespace prefix explicitly for std.
> +using namespace libcamera;
> +
> +static py::object ControlValueToPy(const ControlValue &cv)
> +{
> + //assert(!cv.isArray());
> + //assert(cv.numElements() == 1);
If this isn't useful let's drop those two lines, otherwise let's
uncomment them.
> +
> + switch (cv.type()) {
> + case ControlTypeBool:
> + return py::cast(cv.get<bool>());
> + case ControlTypeByte:
> + return py::cast(cv.get<uint8_t>());
> + case ControlTypeInteger32:
> + return py::cast(cv.get<int32_t>());
> + case ControlTypeInteger64:
> + return py::cast(cv.get<int64_t>());
> + case ControlTypeFloat:
> + return py::cast(cv.get<float>());
> + case ControlTypeString:
> + return py::cast(cv.get<string>());
> + case ControlTypeRectangle:
> + case ControlTypeSize:
Shouldn't rectangle and size be supported ?
> + case ControlTypeNone:
> + default:
> + throw runtime_error("Unsupported ControlValue type");
> + }
> +}
> +
> +static ControlValue PyToControlValue(py::object &ob, ControlType type)
Should the first parameter be const ?
> +{
> + switch (type) {
> + case ControlTypeBool:
> + return ControlValue(ob.cast<bool>());
> + case ControlTypeByte:
> + return ControlValue(ob.cast<uint8_t>());
> + case ControlTypeInteger32:
> + return ControlValue(ob.cast<int32_t>());
> + case ControlTypeInteger64:
> + return ControlValue(ob.cast<int64_t>());
> + case ControlTypeFloat:
> + return ControlValue(ob.cast<float>());
> + case ControlTypeString:
> + return ControlValue(ob.cast<string>());
> + case ControlTypeRectangle:
> + case ControlTypeSize:
Same here, shouldn't Rectangle and Size be supported ?
> + case ControlTypeNone:
> + default:
> + throw runtime_error("Control type not implemented");
> + }
> +}
[snip]
> diff --git a/src/py/test/valgrind-pycamera.supp b/src/py/test/valgrind-pycamera.supp
> new file mode 100644
> index 00000000..98c693f2
> --- /dev/null
> +++ b/src/py/test/valgrind-pycamera.supp
> @@ -0,0 +1,17 @@
> +{
> + <insert_a_suppression_name_here>
Could you insert a suppression name here ? ;-)
> + Memcheck:Leak
> + match-leak-kinds: definite
> + fun:_Znwm
You mentioned that this is a one-time allocation with the callstack
below. Does the valgrind suppression file express a complete callstack ?
> + 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
Three spaces is a really weird indentation.
> +}
> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap
> new file mode 100644
> index 00000000..a76ddb1b
> --- /dev/null
> +++ b/subprojects/pybind11.wrap
> @@ -0,0 +1,10 @@
> +[wrap-file]
> +directory = pybind11-2.3.0
> +
> +source_url = https://github.com/pybind/pybind11/archive/v2.3.0.zip
> +source_filename = pybind11-2.3.0.zip
> +source_hash = 1f844c071d9d98f5bb08458f128baa0aa08a9e5939a6b42276adb1bacd8b483e
> +
> +patch_url = https://wrapdb.mesonbuild.com/v1/projects/pybind11/2.3.0/2/get_zip
> +patch_filename = pybind11-2.3.0-2-wrap.zip
> +patch_hash = f3bed4bfc8961b3b985ff1e63fc6e57c674f35b353f0d42adbc037f9416381fb
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list