[libcamera-devel] [RFC v2 3/4] libcamera python bindings
Tomi Valkeinen
tomi.valkeinen at iki.fi
Mon Nov 30 15:03:40 CET 2020
Hi Laurent,
On 30/11/2020 02:16, Laurent Pinchart wrote:
> 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.
Thanks, but note what I said in the cover letter: "This is not a request to review" =).
While all feedback is welcome, it's probably not worth the time to look at things like comment
styles, order of includes, commented out code, etc.
>> 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.
Ok.
>> 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 ?
Ok.
>> 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.
I can do that if you think the C bindings are not many years in the future. Would that directory be
at the top level?
Not exactly related, but I find the libcamera src & include directories a bit confusing. I find the
style used in kms++ better, where all the files for a project/component are under a single
directory, and no need for a "src" toplevel directory.
But I'm obviously biased =).
>> 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.
Yep.
>> +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 ?
Yes, there are a lot of such cases. Mostly, I think, they're cases where a class has a member field
names foo, and the constructor takes a parameter named foo. The constructor then initializes the
field with foo(foo).
>> +
>> +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.
Hmm yes, that's a good idea.
>> +
>> +install_data(['__init__.py'], install_dir: destdir)
>
> Missing space after ':'.
Ok.
>> 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.
Ok. Is the file name and the description required (e.g. "object.cpp - Base object") by some licence
guide? I don't quite see what benefit such a line provides.
>> +#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.
Ok, I'll have a look. I have never used the hooks, but I have the feeling that I won't like it. I
commit often, and maybe 90% of the times I commit, the commit is not in any way ready or meant for
publishing. It may not even compile cleanly (which is also why I hate Werror enabled by default, I
think it's just bad and annoying (but CI should enable it)).
I do check patches I sent for (proper) review. I didn't check these as these are just prototypes,
and the point was just to deprecate the old series so that no one uses it, and possibly to get ideas
on how to solve some of the existing problems.
Arranging includes alphabetically wasn't really on my list at all =).
>> +
>> +#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.
Ok. Have you had conflicts? Having "std::" all around uglifies the code in my opinion =)
>> +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.
These, are some other things, are just to help testing and debugging. I will clean them up when I
send a proper series.
>> +
>> + 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 ?
Sure, and many other things should be supported too =). I didn't add them as I didn't have any
platforms that had rectangles or sizes, so I couldn't test. And if I recall right, Rectangle and
Size weren't trivial to add.
>> + case ControlTypeNone:
>> + default:
>> + throw runtime_error("Unsupported ControlValue type");
>> + }
>> +}
>> +
>> +static ControlValue PyToControlValue(py::object &ob, ControlType type)
>
> Should the first parameter be const ?
Yes, I can constify it.
>> +{
>> + 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 ?
I can't recall, but it's possible I dropped topmost frames. The suppression was mostly for my
testing, not meant to be a "real" suppression file. A real suppression file needs studying the case
in detail, which possibly means looking at python interpreter sources...
In any case, I don't see this anymore. Possibly more recent pybind11 solved it. Or something else.
However, I see two new leaks...
>> + 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.
That's what valgrind gives with its generate suppression feature.
Tomi
More information about the libcamera-devel
mailing list