[libcamera-devel,RFC,v2,3/4] libcamera python bindings
diff mbox series

Message ID 20201127133738.880859-4-tomi.valkeinen@iki.fi
State RFC
Headers show
Series
  • prototype python bindings
Related show

Commit Message

Tomi Valkeinen Nov. 27, 2020, 1:37 p.m. UTC
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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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

Comments

Laurent Pinchart Nov. 30, 2020, 12:16 a.m. UTC | #1
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@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
Tomi Valkeinen Nov. 30, 2020, 2:03 p.m. UTC | #2
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@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

Patch
diff mbox series

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/
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')
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')
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
+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' ]
+
+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)
+
+install_data(['__init__.py'], install_dir: destdir)
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 @@ 
+#include <chrono>
+#include <thread>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <sys/eventfd.h>
+#include <mutex>
+
+#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;
+
+static py::object ControlValueToPy(const ControlValue &cv)
+{
+	//assert(!cv.isArray());
+	//assert(cv.numElements() == 1);
+
+	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:
+	case ControlTypeNone:
+	default:
+		throw runtime_error("Unsupported ControlValue type");
+	}
+}
+
+static ControlValue PyToControlValue(py::object &ob, ControlType type)
+{
+	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:
+	case ControlTypeNone:
+	default:
+		throw runtime_error("Control type not implemented");
+	}
+}
+
+struct CameraEvent
+{
+	shared_ptr<Camera> camera;
+	Request::Status status;
+	map<const Stream *, FrameBuffer *> bufmap;
+	ControlList metadata;
+	uint64_t cookie;
+};
+
+static int g_eventfd;
+static mutex g_buflist_mutex;
+static vector<CameraEvent> g_buflist;
+
+static void handle_request_completed(Request *req)
+{
+	CameraEvent ev;
+	ev.camera = req->camera();
+	ev.status = req->status();
+	ev.bufmap = req->buffers();
+	ev.metadata = req->metadata();
+	ev.cookie = req->cookie();
+
+	{
+		lock_guard guard(g_buflist_mutex);
+		g_buflist.push_back(ev);
+	}
+
+	uint64_t v = 1;
+	write(g_eventfd, &v, 8);
+}
+
+PYBIND11_MODULE(pycamera, m)
+{
+	py::class_<CameraEvent>(m, "CameraEvent")
+		.def_readonly("camera", &CameraEvent::camera)
+		.def_readonly("status", &CameraEvent::status)
+		.def_readonly("buffers", &CameraEvent::bufmap)
+		.def_property_readonly("metadata", [](const CameraEvent& self) {
+			py::dict ret;
+
+			for (const auto &[key, cv] : self.metadata) {
+				const ControlId *id = properties::properties.at(key);
+				py::object ob = ControlValueToPy(cv);
+
+				ret[id->name().c_str()] = ob;
+			}
+
+			return ret;
+		})
+		.def_readonly("cookie", &CameraEvent::cookie)
+	;
+
+	py::class_<CameraManager>(m, "CameraManager")
+		/*
+		 * CameraManager::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, and call CameraManager::start() from
+		 * the constructor.
+		 */
+
+		.def(py::init([]() {
+			g_eventfd = eventfd(0, 0);
+
+			auto cm = make_unique<CameraManager>();
+			cm->start();
+			return cm;
+		}))
+
+		.def_property_readonly("efd", [](CameraManager &) {
+			return g_eventfd;
+		})
+
+		.def("get_ready_requests", [](CameraManager &) {
+			vector<CameraEvent> v;
+
+			{
+				lock_guard guard(g_buflist_mutex);
+				swap(v, g_buflist);
+			}
+
+			return v;
+		})
+
+		.def("get", py::overload_cast<const string &>(&CameraManager::get))
+		.def("find", [](CameraManager &self, string str) {
+			std::transform(str.begin(), str.end(), str.begin(), ::tolower);
+
+			for (auto c : self.cameras()) {
+				string id = c->id();
+
+				std::transform(id.begin(), id.end(), id.begin(), ::tolower);
+
+				if (id.find(str) != string::npos)
+					return c;
+			}
+
+			return shared_ptr<Camera>();
+		})
+		.def_property_readonly("version", &CameraManager::version)
+
+		// Create a list of Cameras, where each camera has a keep-alive to CameraManager
+		.def_property_readonly("cameras", [](CameraManager &self) {
+			py::list l;
+			for (auto &c : self.cameras()) {
+				py::object py_cm = py::cast(self);
+				py::object py_cam = py::cast(c);
+				py::detail::keep_alive_impl(py_cam, py_cm);
+				l.append(py_cam);
+			}
+			return l;
+		});
+
+	py::class_<Camera, shared_ptr<Camera>>(m, "Camera", py::dynamic_attr())
+		.def_property_readonly("id", &Camera::id)
+		.def("acquire", &Camera::acquire)
+		.def("release", &Camera::release)
+		.def("start", [](shared_ptr<Camera> &self) {
+			self->requestCompleted.connect(handle_request_completed);
+
+			self->start();
+		})
+
+		.def("stop", [](shared_ptr<Camera> &self) {
+			self->stop();
+
+			self->requestCompleted.disconnect(handle_request_completed);
+		})
+
+		.def("__repr__", [](shared_ptr<Camera> &self) {
+			return "<pycamera.Camera '" + self->id() + "'>";
+		})
+
+		// Keep the camera alive, as StreamConfiguration contains a Stream*
+		.def("generateConfiguration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
+		.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_readonly("streams", [](Camera &self) {
+			py::set set;
+			for (auto &s : self.streams()) {
+				py::object py_self = py::cast(self);
+				py::object py_s = py::cast(s);
+				py::detail::keep_alive_impl(py_s, py_self);
+				set.add(py_s);
+			}
+			return set;
+		})
+
+		.def_property_readonly("controls", [](Camera &self) {
+			py::dict ret;
+
+			for (const auto &[id, ci] : self.controls()) {
+				ret[id->name().c_str()] = make_tuple<py::object>(ControlValueToPy(ci.min()),
+										 ControlValueToPy(ci.max()),
+										 ControlValueToPy(ci.def()));
+			}
+
+			return ret;
+		})
+
+		.def_property_readonly("properties", [](Camera &self) {
+			py::dict ret;
+
+			for (const auto &[key, cv] : self.properties()) {
+				const ControlId *id = properties::properties.at(key);
+				py::object ob = ControlValueToPy(cv);
+
+				ret[id->name().c_str()] = ob;
+			}
+
+			return ret;
+		});
+
+	py::class_<CameraConfiguration>(m, "CameraConfiguration")
+		.def("at", py::overload_cast<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(
+			"size",
+			[](StreamConfiguration &self) { return make_tuple(self.size.width, self.size.height); },
+			[](StreamConfiguration &self, tuple<uint32_t, uint32_t> size) { self.size.width = get<0>(size); self.size.height = get<1>(size); })
+		.def_property(
+			"fmt",
+			[](StreamConfiguration &self) { return self.pixelFormat.toString(); },
+			[](StreamConfiguration &self, string fmt) { self.pixelFormat = PixelFormat::fromString(fmt); })
+		.def_readwrite("stride", &StreamConfiguration::stride)
+		.def_readwrite("frameSize", &StreamConfiguration::frameSize)
+		.def_readwrite("bufferCount", &StreamConfiguration::bufferCount)
+		.def_property_readonly("formats", &StreamConfiguration::formats, py::return_value_policy::reference_internal);
+	;
+
+	py::class_<StreamFormats>(m, "StreamFormats")
+		.def_property_readonly("pixelFormats", [](StreamFormats &self) {
+			vector<string> fmts;
+			for (auto &fmt : self.pixelformats())
+				fmts.push_back(fmt.toString());
+			return fmts;
+		})
+		.def("sizes", [](StreamFormats &self, const string &pixelFormat) {
+			auto fmt = PixelFormat::fromString(pixelFormat);
+			vector<tuple<uint32_t, uint32_t>> fmts;
+			for (const auto &s : self.sizes(fmt))
+				fmts.push_back(make_tuple(s.width, s.height));
+			return fmts;
+		})
+		.def("range", [](StreamFormats &self, const string &pixelFormat) {
+			auto fmt = PixelFormat::fromString(pixelFormat);
+			const auto &range = self.range(fmt);
+			return make_tuple(make_tuple(range.hStep, range.vStep),
+					  make_tuple(range.min.width, range.min.height),
+					  make_tuple(range.max.width, range.max.height));
+		});
+
+	py::enum_<StreamRole>(m, "StreamRole")
+		.value("StillCapture", StreamRole::StillCapture)
+		.value("StillCaptureRaw", StreamRole::Raw)
+		.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_property_readonly("allocated", &FrameBufferAllocator::allocated)
+		// Create a list of FrameBuffer, where each FrameBuffer has a keep-alive to FrameBufferAllocator
+		.def("buffers", [](FrameBufferAllocator &self, Stream *stream) {
+			py::list l;
+			for (auto &ub : self.buffers(stream)) {
+				py::object py_fa = py::cast(self);
+				py::object py_buf = py::cast(ub.get());
+				py::detail::keep_alive_impl(py_buf, py_fa);
+				l.append(py_buf);
+			}
+			return l;
+		});
+
+	py::class_<FrameBuffer, unique_ptr<FrameBuffer, py::nodelete>>(m, "FrameBuffer")
+		// XXX who frees this
+		.def(py::init([](vector<tuple<int, unsigned int>> planes, unsigned int cookie) {
+			vector<FrameBuffer::Plane> v;
+			for (const auto& t : planes)
+				v.push_back({FileDescriptor(get<0>(t)), get<1>(t)});
+			return new FrameBuffer(v, cookie);
+		}))
+		.def_property_readonly("metadata", &FrameBuffer::metadata, py::return_value_policy::reference_internal)
+		.def("length", [](FrameBuffer &self, uint32_t idx) {
+			const FrameBuffer::Plane &plane = self.planes()[idx];
+			return plane.length;
+		})
+		.def("fd", [](FrameBuffer &self, uint32_t idx) {
+			const FrameBuffer::Plane &plane = self.planes()[idx];
+			return plane.fd.fd();
+		})
+		.def_property("cookie", &FrameBuffer::cookie, &FrameBuffer::setCookie);
+
+	py::class_<Stream, unique_ptr<Stream, py::nodelete>>(m, "Stream")
+		.def_property_readonly("configuration", &Stream::configuration);
+
+	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)
+		.def_property_readonly("cookie", &Request::cookie)
+		.def_property_readonly("hasPendingBuffers", &Request::hasPendingBuffers)
+		.def("set_control", [](Request &self, string &control, py::object value) {
+			const auto &controls = self.camera()->controls();
+
+			auto it = find_if(controls.begin(), controls.end(),
+					  [&control](const auto &kvp) { return kvp.first->name() == control; });
+
+			if (it == controls.end())
+				throw runtime_error("Control not found");
+
+			const auto &id = it->first;
+
+			self.controls().set(id->id(), PyToControlValue(value, id->type()));
+		});
+
+	py::enum_<Request::Status>(m, "RequestStatus")
+		.value("Pending", Request::RequestPending)
+		.value("Complete", Request::RequestComplete)
+		.value("Cancelled", Request::RequestCancelled);
+
+	py::enum_<FrameMetadata::Status>(m, "FrameMetadataStatus")
+		.value("Success", FrameMetadata::FrameSuccess)
+		.value("Error", FrameMetadata::FrameError)
+		.value("Cancelled", FrameMetadata::FrameCancelled);
+
+	py::class_<FrameMetadata>(m, "FrameMetadata")
+		.def_readonly("status", &FrameMetadata::status)
+		.def_readonly("sequence", &FrameMetadata::sequence)
+		.def_readonly("timestamp", &FrameMetadata::timestamp)
+		.def_property_readonly("bytesused", [](FrameMetadata &self) {
+			vector<unsigned int> v;
+			v.resize(self.planes.size());
+			transform(self.planes.begin(), self.planes.end(), v.begin(), [](const auto &p) { return p.bytesused; });
+			return v;
+		});
+
+	py::enum_<CameraConfiguration::Status>(m, "ConfigurationStatus")
+		.value("Valid", CameraConfiguration::Valid)
+		.value("Adjusted", CameraConfiguration::Adjusted)
+		.value("Invalid", CameraConfiguration::Invalid);
+}
diff --git a/src/py/test/drmtest.py b/src/py/test/drmtest.py
new file mode 100755
index 00000000..f7a6cc48
--- /dev/null
+++ b/src/py/test/drmtest.py
@@ -0,0 +1,129 @@ 
+#!/usr/bin/python3
+
+from simplecamera import SimpleCameraManager, SimpleCamera
+import pykms
+import pycamera as pycam
+import time
+import argparse
+import selectors
+import sys
+
+card = pykms.Card()
+
+res = pykms.ResourceManager(card)
+conn = res.reserve_connector()
+crtc = res.reserve_crtc(conn)
+plane = res.reserve_generic_plane(crtc)
+mode = conn.get_default_mode()
+modeb = mode.to_blob(card)
+
+req = pykms.AtomicReq(card)
+req.add_connector(conn, crtc)
+req.add_crtc(crtc, modeb)
+req.commit_sync(allow_modeset = True)
+
+class ScreenHandler:
+	def __init__(self, card, crtc, plane):
+		self.card = card
+		self.crtc = crtc
+		self.plane = plane
+		self.bufqueue = []
+		self.current = None
+		self.next = None
+
+	def handle_page_flip(self, frame, time):
+		old = self.current
+		self.current = self.next
+
+		if len(self.bufqueue) > 0:
+			self.next = self.bufqueue.pop(0)
+		else:
+			self.next = None
+
+		if self.next:
+			req = pykms.AtomicReq(self.card)
+			req.add_plane(self.plane, fb, self.crtc, dst=(0, 0, fb.width, fb.height))
+			req.commit()
+
+		return old
+
+	def queue(self, fb):
+		if not self.next:
+			self.next = fb
+
+			req = pykms.AtomicReq(self.card)
+			req.add_plane(self.plane, fb, self.crtc, dst=(0, 0, fb.width, fb.height))
+			req.commit()
+		else:
+			self.bufqueue.append(fb)
+
+
+
+
+screen = ScreenHandler(card, crtc, plane)
+
+
+
+def handle_camera_frame(camera, stream, fb):
+	screen.queue(cam_2_drm_map[fb])
+
+cm = SimpleCameraManager()
+cam = cm.find("imx219")
+cam.open()
+
+cam.format = "ARGB8888"
+cam.resolution = (1920, 1080)
+
+cam.callback = lambda stream, fb, camera=cam: handle_camera_frame(camera, stream, fb)
+
+cam_2_drm_map = {}
+drm_2_cam_map = {}
+
+cam.xxx_config()
+
+drmbuffers = []
+stream_cfg = cam.stream_config
+for fb in cam.buffers:
+	w, h = stream_cfg.size
+	stride = stream_cfg.stride
+	drmfb = pykms.DmabufFramebuffer(card, w, h, pykms.PixelFormat.ARGB8888,
+									[fb.fd(0)], [stride], [0])
+	drmbuffers.append(drmfb)
+
+	cam_2_drm_map[fb] = drmfb
+	drm_2_cam_map[drmfb] = fb
+
+
+cam.start()
+
+def readdrm(fileobj, mask):
+	for ev in card.read_events():
+		if ev.type == pykms.DrmEventType.FLIP_COMPLETE:
+			old = screen.handle_page_flip(ev.seq, ev.time)
+
+			if old:
+				fb = drm_2_cam_map[old]
+				cam.queue_fb(fb)
+
+running = True
+
+def readkey(fileobj, mask):
+	global running
+	sys.stdin.readline()
+	running = False
+
+sel = selectors.DefaultSelector()
+sel.register(card.fd, selectors.EVENT_READ, readdrm)
+sel.register(sys.stdin, selectors.EVENT_READ, readkey)
+
+print("Press enter to exit")
+
+while running:
+	events = sel.select()
+	for key, mask in events:
+		callback = key.data
+		callback(key.fileobj, mask)
+
+cam.stop()
+
+print("Done")
diff --git a/src/py/test/icam.py b/src/py/test/icam.py
new file mode 100755
index 00000000..2a8205ed
--- /dev/null
+++ b/src/py/test/icam.py
@@ -0,0 +1,154 @@ 
+#!/usr/bin/python3 -i
+
+from simplecamera import SimpleCameraManager, SimpleCamera
+from PyQt5 import QtCore, QtGui, QtWidgets
+import pycamera as pycam
+import argparse
+
+parser = argparse.ArgumentParser()
+parser.add_argument("-c", "--cameras", type=str, default=None)
+args = parser.parse_args()
+
+format_map = {
+	"YUYV": QtGui.QImage.Format.Format_RGB16,
+	"BGR888": QtGui.QImage.Format.Format_RGB888,
+	"MJPEG": QtGui.QImage.Format.Format_RGB888,
+}
+
+
+class MainWindow(QtWidgets.QWidget):
+	requestDone = QtCore.pyqtSignal(pycam.Stream, pycam.FrameBuffer)
+
+	def __init__(self, camera):
+		super().__init__()
+
+		# Use signal to handle request, so that the execution is transferred to the main thread
+		self.requestDone.connect(self.handle_request)
+		camera.callback = lambda stream, fb: self.requestDone.emit(stream, fb)
+
+		camera.xxx_config()
+
+		self.camera = camera
+
+		self.label = QtWidgets.QLabel()
+
+		windowLayout = QtWidgets.QHBoxLayout()
+		self.setLayout(windowLayout)
+
+		windowLayout.addWidget(self.label)
+
+		controlsLayout = QtWidgets.QVBoxLayout()
+		windowLayout.addLayout(controlsLayout)
+
+		windowLayout.addStretch()
+
+		group = QtWidgets.QGroupBox("Info")
+		groupLayout = QtWidgets.QVBoxLayout()
+		group.setLayout(groupLayout)
+		controlsLayout.addWidget(group)
+
+		lab = QtWidgets.QLabel(camera.id)
+		groupLayout.addWidget(lab)
+
+		self.frameLabel = QtWidgets.QLabel()
+		groupLayout.addWidget(self.frameLabel)
+
+
+		group = QtWidgets.QGroupBox("Properties")
+		groupLayout = QtWidgets.QVBoxLayout()
+		group.setLayout(groupLayout)
+		controlsLayout.addWidget(group)
+
+		for k, v in camera.properties.items():
+			lab = QtWidgets.QLabel()
+			lab.setText(k + " = " + str(v))
+			groupLayout.addWidget(lab)
+
+		group = QtWidgets.QGroupBox("Controls")
+		groupLayout = QtWidgets.QVBoxLayout()
+		group.setLayout(groupLayout)
+		controlsLayout.addWidget(group)
+
+		for k, (min, max, default) in camera.controls.items():
+			lab = QtWidgets.QLabel()
+			lab.setText("{} = {}/{}/{}".format(k, min, max, default))
+			groupLayout.addWidget(lab)
+
+		controlsLayout.addStretch()
+
+		self.camera.start()
+
+	def closeEvent(self, event):
+		self.camera.stop()
+		super().closeEvent(event)
+
+	def handle_request(self, stream, fb):
+		global format_map
+
+		#meta = fb.metadata
+		#print("Buf seq {}, bytes {}".format(meta.sequence, meta.bytesused))
+
+		with fb.mmap(0) as b:
+			cfg = stream.configuration
+			qfmt = format_map[cfg.fmt]
+			w, h = cfg.size
+			pitch = cfg.stride
+			img = QtGui.QImage(b, w, h, pitch, qfmt)
+			self.label.setPixmap(QtGui.QPixmap.fromImage(img))
+
+		self.frameLabel.setText("Queued: {}\nDone: {}".format(camera.reqs_queued, camera.reqs_completed))
+
+		self.camera.queue_fb(fb)
+
+
+app = QtWidgets.QApplication([])
+cm = SimpleCameraManager()
+
+notif = QtCore.QSocketNotifier(cm.cm.efd, QtCore.QSocketNotifier.Read)
+notif.activated.connect(lambda x: cm.read_events())
+
+if not args.cameras:
+	cameras = cm.cameras
+else:
+	cameras = []
+	for name in args.cameras.split(","):
+		c = cm.find(name)
+		if not c:
+			print("Camera not found: ", name)
+			exit(-1)
+		cameras.append(c)
+
+windows = []
+
+i = 0
+for camera in cameras:
+	globals()["cam" + str(i)] = camera
+	i += 1
+
+	camera.open()
+
+	fmts = camera.formats
+	if "BGR888" in fmts:
+		camera.format = "BGR888"
+	elif "YUYV" in fmts:
+		camera.format = "YUYV"
+	else:
+		raise Exception("Unsupported pixel format")
+
+	camera.resolution = (640, 480)
+
+	window = MainWindow(camera)
+	window.setAttribute(QtCore.Qt.WA_ShowWithoutActivating)
+	window.show()
+	windows.append(window)
+
+def cleanup():
+	for w in windows:
+		w.close()
+
+	for camera in cameras:
+		camera.close()
+	print("Done")
+
+import atexit
+atexit.register(cleanup)
diff --git a/src/py/test/run-valgrind.sh b/src/py/test/run-valgrind.sh
new file mode 100755
index 00000000..7537e507
--- /dev/null
+++ b/src/py/test/run-valgrind.sh
@@ -0,0 +1,6 @@ 
+#!/bin/bash
+
+export PYTHONMALLOC=malloc
+export PYTHONPATH=../../../build/debug/src/py
+
+valgrind --suppressions=valgrind-pycamera.supp --leak-check=full --show-leak-kinds=definite --gen-suppressions=yes python3 test.py $*
diff --git a/src/py/test/run.sh b/src/py/test/run.sh
new file mode 100755
index 00000000..96f68dcb
--- /dev/null
+++ b/src/py/test/run.sh
@@ -0,0 +1,3 @@ 
+#!/bin/bash
+
+PYTHONPATH=../../../build/debug/src/py python3 test.py $*
diff --git a/src/py/test/simplecamera.py b/src/py/test/simplecamera.py
new file mode 100644
index 00000000..2051dbeb
--- /dev/null
+++ b/src/py/test/simplecamera.py
@@ -0,0 +1,198 @@ 
+import pycamera as pycam
+import os
+
+class SimpleCameraManager:
+	def __init__(self):
+		self.cm = pycam.CameraManager()
+
+		self.cameras = []
+		for c in self.cm.cameras:
+			self.cameras.append(SimpleCamera(c))
+
+	def find(self, name):
+		for c in self.cameras:
+			if name.lower() in c.id.lower():
+				return c
+
+		return None
+
+	def read_events(self):
+		data = os.read(self.cm.efd, 8)
+
+		reqs = self.cm.get_ready_requests()
+
+		for req in reqs:
+			for c in self.cameras:
+				if c.pycam == req.camera:
+					c.req_complete_cb(req)
+
+class SimpleCamera:
+	def __init__(self, camera):
+		self.pycam = camera
+
+		self.callback = None
+
+		self.control_values = {}
+		#for k, (min, max, default) in self.pycam.controls.items():
+		#	self.control_values[k] = default
+
+		self.running = False
+
+	def __repr__(self):
+		return "<SimpleCamera '" + self.pycam.id + "'>"
+
+	@property
+	def id(self):
+		return self.pycam.id
+
+	@property
+	def formats(self):
+		return self.stream_config.formats.pixelFormats
+
+	def open(self):
+		self.pycam.acquire()
+
+		self.camera_config = self.pycam.generateConfiguration([pycam.StreamRole.Viewfinder])
+		self.stream_config = self.camera_config.at(0)
+
+	def close(self):
+		self.pycam.release()
+
+	@property
+	def properties(self):
+		return self.pycam.properties
+
+	@property
+	def controls(self):
+		return self.pycam.controls
+
+	def xxx_config(self):
+		self.configure_camera()
+		self.alloc_buffers()
+
+	def start(self):
+		self.reqs_queued = 0
+		self.reqs_completed = 0
+
+		self.running = True
+		self.pycam.start()
+
+		self.queue_initial_fbs()
+
+	def stop(self):
+		self.running = False
+
+		self.pycam.stop()
+
+		self.buffers = None
+
+	@property
+	def resolution(self):
+		return self.stream_config.size
+
+	@resolution.setter
+	def resolution(self, val):
+		running = self.running
+		if running:
+			self.stop()
+
+		self.stream_config.size = val
+		self.camera_config.validate()
+
+		if running:
+			self.start()
+
+	@property
+	def format(self):
+		return self.stream_config.fmt
+
+	@format.setter
+	def format(self, val):
+		running = self.running
+		if running:
+			self.stop()
+
+		self.stream_config.fmt = val
+		self.camera_config.validate()
+
+		if running:
+			self.start()
+
+	def configure_camera(self):
+		camera = self.pycam
+
+		status = self.camera_config.validate()
+
+		if status == pycam.ConfigurationStatus.Invalid:
+			raise Exception("Invalid configuration")
+
+		print("Cam: config {}".format(self.stream_config.toString()))
+
+		camera.configure(self.camera_config);
+
+	def alloc_buffers(self):
+		camera = self.pycam
+		stream = self.stream_config.stream
+
+		allocator = pycam.FrameBufferAllocator(camera);
+		ret = allocator.allocate(stream)
+		if ret < 0:
+			raise Exception("Can't allocate buffers")
+
+		self.buffers = allocator.buffers(stream)
+
+		print("Cam: Allocated {} buffers for stream".format(len(self.buffers)))
+
+	def queue_initial_fbs(self):
+		buffers = self.buffers
+
+		for fb in buffers:
+			self.queue_fb(fb)
+
+	def queue_fb(self, fb):
+		camera = self.pycam
+		stream = self.stream_config.stream
+
+		request = camera.createRequest()
+
+		if request == None:
+			raise Exception("Can't create request")
+
+		ret = request.addBuffer(stream, fb)
+		if ret < 0:
+			raise Exception("Can't set buffer for request")
+
+		# XXX: ExposureTime cannot be set if AeEnable == True
+		skip_exp_time = "AeEnable" in self.control_values and self.control_values["AeEnable"] == True
+
+		for k, v in self.control_values.items():
+			if k == "ExposureTime" and skip_exp_time:
+				continue
+			request.set_control(k, v)
+
+		control_values = {}
+
+		camera.queueRequest(request)
+
+		self.reqs_queued += 1
+
+
+	def req_complete_cb(self, req):
+		camera = self.pycam
+
+		assert(len(req.buffers) == 1)
+
+		stream, fb = next(iter(req.buffers.items()))
+
+		self.reqs_completed += 1
+
+		if self.running and self.callback:
+			self.callback(stream, fb)
+
+	def set_control(self, control, value):
+		if not control in self.pycam.controls:
+			for k in self.pycam.controls:
+				if control.lower() == k.lower():
+					control = k
+
+		self.control_values[control] = value
diff --git a/src/py/test/test.py b/src/py/test/test.py
new file mode 100755
index 00000000..86e86043
--- /dev/null
+++ b/src/py/test/test.py
@@ -0,0 +1,210 @@ 
+#!/usr/bin/python3
+
+import pycamera as pycam
+import time
+import binascii
+import argparse
+import selectors
+import os
+
+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, default=1)
+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))
+	print("        Properties:", c.properties)
+	print("        Controls:", c.controls)
+
+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.size = (1920, 480)
+	#stream_config.fmt = "BGR888"
+
+	print("Cam {}: stream config {}".format(ctx["id"], stream_config.toString()))
+
+	camera.configure(config);
+
+	ctx["config"] = config
+
+def alloc_buffers(ctx):
+	camera = ctx["camera"]
+	stream = ctx["config"].at(0).stream
+
+	allocator = pycam.FrameBufferAllocator(camera);
+	ret = allocator.allocate(stream)
+	if ret < 0:
+		print("Can't allocate buffers")
+		exit(-1)
+
+	allocated = len(allocator.buffers(stream))
+	print("Cam {}: Allocated {} buffers for stream".format(ctx["id"], allocated))
+
+	ctx["allocator"] = allocator
+
+def create_requests(ctx):
+	camera = ctx["camera"]
+	stream = ctx["config"].at(0).stream
+	buffers = ctx["allocator"].buffers(stream)
+
+	requests = []
+
+	b = -1
+
+	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)
+
+		#request.set_control("Brightness", b)
+		b += 0.25
+
+		requests.append(request)
+
+	ctx["requests"] = requests
+
+
+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))
+
+		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["reqs_completed"]
+				filename = "frame-{}-{}.data".format(id, num)
+				with open(filename, "wb") as f:
+					f.write(b)
+				print("Cam {}:    Saved {}".format(ctx["id"], filename))
+
+	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
+
+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)
+	alloc_buffers(ctx)
+	create_requests(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)
+
+running = True
+
+def readcam(fileobj, mask):
+	global running
+	data = os.read(fileobj, 8)
+
+	reqs = cm.get_ready_requests()
+
+	ctx = contexts[0]
+	for req in reqs:
+		ctx = next(ctx for ctx in contexts if ctx["camera"] == req.camera)
+		req_complete_cb(ctx, req)
+
+	running =  any(ctx["reqs_completed"] < args.num_frames for ctx in contexts)
+
+
+sel = selectors.DefaultSelector()
+sel.register(cm.efd, selectors.EVENT_READ, readcam)
+
+print("Press enter to exit")
+
+while running:
+	events = sel.select()
+	for key, mask in events:
+		callback = key.data
+		callback(key.fileobj, mask)
+
+print("Exiting...")
+
+for ctx in contexts:
+	camera = ctx["camera"]
+	camera.stop()
+	camera.release()
+
+print("Done")
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>
+   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
+}
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