[{"id":13962,"web_url":"https://patchwork.libcamera.org/comment/13962/","msgid":"<20201130001654.GE5893@pendragon.ideasonboard.com>","date":"2020-11-30T00:16:54","subject":"Re: [libcamera-devel] [RFC v2 3/4] libcamera python bindings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Fri, Nov 27, 2020 at 03:37:37PM +0200, Tomi Valkeinen wrote:\n> Main issues:\n> \n> - Memory management in general. Who owns the object, how to pass\n>   ownership, etc.\n> \n> - Specifically, Request is currently broken. We can't, afaik, pass\n>   ownership around. So currently Python never frees a Request, and if\n>   the Request is not given to Camera::queueRequest, it will leak.\n> \n> - Need public Camera destructor. It is not clear to me why C++ allows it\n>   to be private, but pybind11 doesn't.\n\nHere's a partial review, I'll try to give the rest a go in the near\nfuture.\n\n> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>\n> ---\n>  .gitignore                         |   2 +\n>  meson_options.txt                  |   2 +\n>  src/meson.build                    |   1 +\n>  src/py/meson.build                 |   1 +\n>  src/py/pycamera/__init__.py        |  11 +\n>  src/py/pycamera/meson.build        |  38 +++\n>  src/py/pycamera/pymain.cpp         | 382 +++++++++++++++++++++++++++++\n>  src/py/test/drmtest.py             | 129 ++++++++++\n>  src/py/test/icam.py                | 154 ++++++++++++\n>  src/py/test/run-valgrind.sh        |   6 +\n>  src/py/test/run.sh                 |   3 +\n>  src/py/test/simplecamera.py        | 198 +++++++++++++++\n>  src/py/test/test.py                | 210 ++++++++++++++++\n>  src/py/test/valgrind-pycamera.supp |  17 ++\n>  subprojects/pybind11.wrap          |  10 +\n>  15 files changed, 1164 insertions(+)\n>  create mode 100644 src/py/meson.build\n>  create mode 100644 src/py/pycamera/__init__.py\n>  create mode 100644 src/py/pycamera/meson.build\n>  create mode 100644 src/py/pycamera/pymain.cpp\n>  create mode 100755 src/py/test/drmtest.py\n>  create mode 100755 src/py/test/icam.py\n>  create mode 100755 src/py/test/run-valgrind.sh\n>  create mode 100755 src/py/test/run.sh\n>  create mode 100644 src/py/test/simplecamera.py\n>  create mode 100755 src/py/test/test.py\n>  create mode 100644 src/py/test/valgrind-pycamera.supp\n>  create mode 100644 subprojects/pybind11.wrap\n> \n> diff --git a/.gitignore b/.gitignore\n> index d3d73615..1f9dc7d1 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -5,3 +5,5 @@ build/\n>  patches/\n>  *.patch\n>  *.pyc\n> +subprojects/packagecache/\n> +subprojects/pybind11-2.3.0/\n\nLet's move directories before files, and keep them alphabetically\nsorted.\n\n> diff --git a/meson_options.txt b/meson_options.txt\n> index 53f2675e..ef995527 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -37,3 +37,5 @@ option('v4l2',\n>          type : 'boolean',\n>          value : false,\n>          description : 'Compile the V4L2 compatibility layer')\n> +\n> +option('pycamera', type : 'feature', value : 'auto')\n\nA description would be nice. Can you format the option similarly to the\nother ones ?\n\n> diff --git a/src/meson.build b/src/meson.build\n> index b9c7e759..61ec3991 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -23,3 +23,4 @@ if get_option('v4l2')\n>  endif\n>  \n>  subdir('gstreamer')\n> +subdir('py')\n\nWe could create a bindings directory, to store other language bindings\n(there should be a C binding in the future), but maybe that's overkill.\n\n> diff --git a/src/py/meson.build b/src/py/meson.build\n> new file mode 100644\n> index 00000000..42ffa221\n> --- /dev/null\n> +++ b/src/py/meson.build\n> @@ -0,0 +1 @@\n> +subdir('pycamera')\n> diff --git a/src/py/pycamera/__init__.py b/src/py/pycamera/__init__.py\n> new file mode 100644\n> index 00000000..ddb70096\n> --- /dev/null\n> +++ b/src/py/pycamera/__init__.py\n> @@ -0,0 +1,11 @@\n> +from .pycamera import *\n> +from enum import Enum\n> +import os\n> +import struct\n\nThese three imports are not used.\n\n> +import mmap\n> +\n> +\n> +def __FrameBuffer__mmap(self, plane):\n> +\treturn mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)\n> +\n> +FrameBuffer.mmap = __FrameBuffer__mmap\n> diff --git a/src/py/pycamera/meson.build b/src/py/pycamera/meson.build\n> new file mode 100644\n> index 00000000..9ff9b8ee\n> --- /dev/null\n> +++ b/src/py/pycamera/meson.build\n> @@ -0,0 +1,38 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +py3_dep = dependency('python3', required : get_option('pycamera'))\n> +\n> +if py3_dep.found() == false\n> +    subdir_done()\n> +endif\n> +\n> +pybind11_proj = subproject('pybind11')\n> +pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> +\n> +pycamera_sources = files([\n> +    'pymain.cpp',\n> +])\n> +\n> +pycamera_deps = [\n> +    libcamera_dep,\n> +    py3_dep,\n> +    pybind11_dep,\n> +]\n> +\n> +pycamera_args = [ '-fvisibility=hidden' ]\n> +pycamera_args += [ '-Wno-shadow' ]\n\nIs this because pybind11 shadows variables ?\n\n> +\n> +destdir = get_option('libdir') + '/python' + py3_dep.version() + '/site-packages/pycamera'\n> +\n> +pycamera = shared_module('pycamera',\n> +                         pycamera_sources,\n> +                         install : true,\n> +                         install_dir : destdir,\n> +                         name_prefix : '',\n> +                         dependencies : pycamera_deps,\n> +                         cpp_args : pycamera_args)\n> +\n> +# Copy __init__.py to build dir so that we can run without installing\n> +configure_file(input: '__init__.py', output: '__init__.py', copy: true)\n\nYou could also create a symlink. There's an example in the top-level\nmeson.build.\n\n> +\n> +install_data(['__init__.py'], install_dir: destdir)\n\nMissing space after ':'.\n\n> diff --git a/src/py/pycamera/pymain.cpp b/src/py/pycamera/pymain.cpp\n> new file mode 100644\n> index 00000000..bd1b9bdd\n> --- /dev/null\n> +++ b/src/py/pycamera/pymain.cpp\n> @@ -0,0 +1,382 @@\n\nMissing SPDX tag and initial comment block.\n\n> +#include <chrono>\n> +#include <thread>\n> +#include <fcntl.h>\n> +#include <unistd.h>\n> +#include <sys/mman.h>\n> +#include <sys/eventfd.h>\n> +#include <mutex>\n\nCan you copy utils/hooks/post-commit to .git/hooks/ to ensure\ncheckstyle.py is run ? It should warn about incorrect alphabetical\norder.\n\n> +\n> +#include <pybind11/pybind11.h>\n> +#include <pybind11/stl.h>\n> +#include <pybind11/stl_bind.h>\n> +#include <pybind11/functional.h>\n> +\n> +#include <libcamera/libcamera.h>\n> +\n> +namespace py = pybind11;\n> +\n> +using namespace std;\n\nWe usually use the namespace prefix explicitly for std.\n\n> +using namespace libcamera;\n> +\n> +static py::object ControlValueToPy(const ControlValue &cv)\n> +{\n> +\t//assert(!cv.isArray());\n> +\t//assert(cv.numElements() == 1);\n\nIf this isn't useful let's drop those two lines, otherwise let's\nuncomment them.\n\n> +\n> +\tswitch (cv.type()) {\n> +\tcase ControlTypeBool:\n> +\t\treturn py::cast(cv.get<bool>());\n> +\tcase ControlTypeByte:\n> +\t\treturn py::cast(cv.get<uint8_t>());\n> +\tcase ControlTypeInteger32:\n> +\t\treturn py::cast(cv.get<int32_t>());\n> +\tcase ControlTypeInteger64:\n> +\t\treturn py::cast(cv.get<int64_t>());\n> +\tcase ControlTypeFloat:\n> +\t\treturn py::cast(cv.get<float>());\n> +\tcase ControlTypeString:\n> +\t\treturn py::cast(cv.get<string>());\n> +\tcase ControlTypeRectangle:\n> +\tcase ControlTypeSize:\n\nShouldn't rectangle and size be supported ?\n\n> +\tcase ControlTypeNone:\n> +\tdefault:\n> +\t\tthrow runtime_error(\"Unsupported ControlValue type\");\n> +\t}\n> +}\n> +\n> +static ControlValue PyToControlValue(py::object &ob, ControlType type)\n\nShould the first parameter be const ?\n\n> +{\n> +\tswitch (type) {\n> +\tcase ControlTypeBool:\n> +\t\treturn ControlValue(ob.cast<bool>());\n> +\tcase ControlTypeByte:\n> +\t\treturn ControlValue(ob.cast<uint8_t>());\n> +\tcase ControlTypeInteger32:\n> +\t\treturn ControlValue(ob.cast<int32_t>());\n> +\tcase ControlTypeInteger64:\n> +\t\treturn ControlValue(ob.cast<int64_t>());\n> +\tcase ControlTypeFloat:\n> +\t\treturn ControlValue(ob.cast<float>());\n> +\tcase ControlTypeString:\n> +\t\treturn ControlValue(ob.cast<string>());\n> +\tcase ControlTypeRectangle:\n> +\tcase ControlTypeSize:\n\nSame here, shouldn't Rectangle and Size be supported ?\n\n> +\tcase ControlTypeNone:\n> +\tdefault:\n> +\t\tthrow runtime_error(\"Control type not implemented\");\n> +\t}\n> +}\n\n[snip]\n\n> diff --git a/src/py/test/valgrind-pycamera.supp b/src/py/test/valgrind-pycamera.supp\n> new file mode 100644\n> index 00000000..98c693f2\n> --- /dev/null\n> +++ b/src/py/test/valgrind-pycamera.supp\n> @@ -0,0 +1,17 @@\n> +{\n> +   <insert_a_suppression_name_here>\n\nCould you insert a suppression name here ? ;-)\n\n> +   Memcheck:Leak\n> +   match-leak-kinds: definite\n> +   fun:_Znwm\n\nYou mentioned that this is a one-time allocation with the callstack\nbelow. Does the valgrind suppression file express a complete callstack ?\n\n> +   fun:_ZN8pybind116moduleC1EPKcS2_\n> +   fun:PyInit_pycamera\n> +   fun:_PyImport_LoadDynamicModuleWithSpec\n> +   obj:/usr/bin/python3.8\n> +   obj:/usr/bin/python3.8\n> +   fun:PyVectorcall_Call\n> +   fun:_PyEval_EvalFrameDefault\n> +   fun:_PyEval_EvalCodeWithName\n> +   fun:_PyFunction_Vectorcall\n> +   fun:_PyEval_EvalFrameDefault\n> +   fun:_PyFunction_Vectorcall\n\nThree spaces is a really weird indentation.\n\n> +}\n> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap\n> new file mode 100644\n> index 00000000..a76ddb1b\n> --- /dev/null\n> +++ b/subprojects/pybind11.wrap\n> @@ -0,0 +1,10 @@\n> +[wrap-file]\n> +directory = pybind11-2.3.0\n> +\n> +source_url = https://github.com/pybind/pybind11/archive/v2.3.0.zip\n> +source_filename = pybind11-2.3.0.zip\n> +source_hash = 1f844c071d9d98f5bb08458f128baa0aa08a9e5939a6b42276adb1bacd8b483e\n> +\n> +patch_url = https://wrapdb.mesonbuild.com/v1/projects/pybind11/2.3.0/2/get_zip\n> +patch_filename = pybind11-2.3.0-2-wrap.zip\n> +patch_hash = f3bed4bfc8961b3b985ff1e63fc6e57c674f35b353f0d42adbc037f9416381fb","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D2E25BE176\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 30 Nov 2020 00:17:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 552F76347F;\n\tMon, 30 Nov 2020 01:17:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A03A6346E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 30 Nov 2020 01:17:03 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AB5BE97E;\n\tMon, 30 Nov 2020 01:17:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"b44UxG2w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606695422;\n\tbh=VO0Y7RGJJ8rMxTciMDdHM+3stmf1s0ScrFJPOi0s+2Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=b44UxG2wviySucBT+9Jx75YNz8KKFMNYvTkeLBWGkMS6fhoytUGtejjHHDaVlud0H\n\tpXSkP+T9rIwbfrRAXvs4DrKpZNL8lcgULxtJrU8APcFBVoLheI8qlXpAZtqZiiocL2\n\tzwDJMSYOJHdKf9DgHZx/WID/d7r/DCrY55IKpphc=","Date":"Mon, 30 Nov 2020 02:16:54 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@iki.fi>","Message-ID":"<20201130001654.GE5893@pendragon.ideasonboard.com>","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-4-tomi.valkeinen@iki.fi>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201127133738.880859-4-tomi.valkeinen@iki.fi>","Subject":"Re: [libcamera-devel] [RFC v2 3/4] libcamera python bindings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13979,"web_url":"https://patchwork.libcamera.org/comment/13979/","msgid":"<5f92ba45-193e-bd93-1bda-536855e4a72d@iki.fi>","date":"2020-11-30T14:03:40","subject":"Re: [libcamera-devel] [RFC v2 3/4] libcamera python bindings","submitter":{"id":70,"url":"https://patchwork.libcamera.org/api/people/70/","name":"Tomi Valkeinen","email":"tomi.valkeinen@iki.fi"},"content":"Hi Laurent,\n\nOn 30/11/2020 02:16, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 27, 2020 at 03:37:37PM +0200, Tomi Valkeinen wrote:\n>> Main issues:\n>>\n>> - Memory management in general. Who owns the object, how to pass\n>>   ownership, etc.\n>>\n>> - Specifically, Request is currently broken. We can't, afaik, pass\n>>   ownership around. So currently Python never frees a Request, and if\n>>   the Request is not given to Camera::queueRequest, it will leak.\n>>\n>> - Need public Camera destructor. It is not clear to me why C++ allows it\n>>   to be private, but pybind11 doesn't.\n> \n> Here's a partial review, I'll try to give the rest a go in the near\n> future.\n\nThanks, but note what I said in the cover letter: \"This is not a request to review\" =).\n\nWhile all feedback is welcome, it's probably not worth the time to look at things like comment\nstyles, order of includes, commented out code, etc.\n\n>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>\n>> ---\n>>  .gitignore                         |   2 +\n>>  meson_options.txt                  |   2 +\n>>  src/meson.build                    |   1 +\n>>  src/py/meson.build                 |   1 +\n>>  src/py/pycamera/__init__.py        |  11 +\n>>  src/py/pycamera/meson.build        |  38 +++\n>>  src/py/pycamera/pymain.cpp         | 382 +++++++++++++++++++++++++++++\n>>  src/py/test/drmtest.py             | 129 ++++++++++\n>>  src/py/test/icam.py                | 154 ++++++++++++\n>>  src/py/test/run-valgrind.sh        |   6 +\n>>  src/py/test/run.sh                 |   3 +\n>>  src/py/test/simplecamera.py        | 198 +++++++++++++++\n>>  src/py/test/test.py                | 210 ++++++++++++++++\n>>  src/py/test/valgrind-pycamera.supp |  17 ++\n>>  subprojects/pybind11.wrap          |  10 +\n>>  15 files changed, 1164 insertions(+)\n>>  create mode 100644 src/py/meson.build\n>>  create mode 100644 src/py/pycamera/__init__.py\n>>  create mode 100644 src/py/pycamera/meson.build\n>>  create mode 100644 src/py/pycamera/pymain.cpp\n>>  create mode 100755 src/py/test/drmtest.py\n>>  create mode 100755 src/py/test/icam.py\n>>  create mode 100755 src/py/test/run-valgrind.sh\n>>  create mode 100755 src/py/test/run.sh\n>>  create mode 100644 src/py/test/simplecamera.py\n>>  create mode 100755 src/py/test/test.py\n>>  create mode 100644 src/py/test/valgrind-pycamera.supp\n>>  create mode 100644 subprojects/pybind11.wrap\n>>\n>> diff --git a/.gitignore b/.gitignore\n>> index d3d73615..1f9dc7d1 100644\n>> --- a/.gitignore\n>> +++ b/.gitignore\n>> @@ -5,3 +5,5 @@ build/\n>>  patches/\n>>  *.patch\n>>  *.pyc\n>> +subprojects/packagecache/\n>> +subprojects/pybind11-2.3.0/\n> \n> Let's move directories before files, and keep them alphabetically\n> sorted.\n\nOk.\n\n>> diff --git a/meson_options.txt b/meson_options.txt\n>> index 53f2675e..ef995527 100644\n>> --- a/meson_options.txt\n>> +++ b/meson_options.txt\n>> @@ -37,3 +37,5 @@ option('v4l2',\n>>          type : 'boolean',\n>>          value : false,\n>>          description : 'Compile the V4L2 compatibility layer')\n>> +\n>> +option('pycamera', type : 'feature', value : 'auto')\n> \n> A description would be nice. Can you format the option similarly to the\n> other ones ?\n\nOk.\n\n>> diff --git a/src/meson.build b/src/meson.build\n>> index b9c7e759..61ec3991 100644\n>> --- a/src/meson.build\n>> +++ b/src/meson.build\n>> @@ -23,3 +23,4 @@ if get_option('v4l2')\n>>  endif\n>>  \n>>  subdir('gstreamer')\n>> +subdir('py')\n> \n> We could create a bindings directory, to store other language bindings\n> (there should be a C binding in the future), but maybe that's overkill.\n\nI can do that if you think the C bindings are not many years in the future. Would that directory be\nat the top level?\n\nNot exactly related, but I find the libcamera src & include directories a bit confusing. I find the\nstyle used in kms++ better, where all the files for a project/component are under a single\ndirectory, and no need for a \"src\" toplevel directory.\n\nBut I'm obviously biased =).\n\n>> diff --git a/src/py/meson.build b/src/py/meson.build\n>> new file mode 100644\n>> index 00000000..42ffa221\n>> --- /dev/null\n>> +++ b/src/py/meson.build\n>> @@ -0,0 +1 @@\n>> +subdir('pycamera')\n>> diff --git a/src/py/pycamera/__init__.py b/src/py/pycamera/__init__.py\n>> new file mode 100644\n>> index 00000000..ddb70096\n>> --- /dev/null\n>> +++ b/src/py/pycamera/__init__.py\n>> @@ -0,0 +1,11 @@\n>> +from .pycamera import *\n>> +from enum import Enum\n>> +import os\n>> +import struct\n> \n> These three imports are not used.\n\nYep.\n\n>> +import mmap\n>> +\n>> +\n>> +def __FrameBuffer__mmap(self, plane):\n>> +\treturn mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)\n>> +\n>> +FrameBuffer.mmap = __FrameBuffer__mmap\n>> diff --git a/src/py/pycamera/meson.build b/src/py/pycamera/meson.build\n>> new file mode 100644\n>> index 00000000..9ff9b8ee\n>> --- /dev/null\n>> +++ b/src/py/pycamera/meson.build\n>> @@ -0,0 +1,38 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +py3_dep = dependency('python3', required : get_option('pycamera'))\n>> +\n>> +if py3_dep.found() == false\n>> +    subdir_done()\n>> +endif\n>> +\n>> +pybind11_proj = subproject('pybind11')\n>> +pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n>> +\n>> +pycamera_sources = files([\n>> +    'pymain.cpp',\n>> +])\n>> +\n>> +pycamera_deps = [\n>> +    libcamera_dep,\n>> +    py3_dep,\n>> +    pybind11_dep,\n>> +]\n>> +\n>> +pycamera_args = [ '-fvisibility=hidden' ]\n>> +pycamera_args += [ '-Wno-shadow' ]\n> \n> Is this because pybind11 shadows variables ?\n\nYes, there are a lot of such cases. Mostly, I think, they're cases where a class has a member field\nnames foo, and the constructor takes a parameter named foo. The constructor then initializes the\nfield with foo(foo).\n\n>> +\n>> +destdir = get_option('libdir') + '/python' + py3_dep.version() + '/site-packages/pycamera'\n>> +\n>> +pycamera = shared_module('pycamera',\n>> +                         pycamera_sources,\n>> +                         install : true,\n>> +                         install_dir : destdir,\n>> +                         name_prefix : '',\n>> +                         dependencies : pycamera_deps,\n>> +                         cpp_args : pycamera_args)\n>> +\n>> +# Copy __init__.py to build dir so that we can run without installing\n>> +configure_file(input: '__init__.py', output: '__init__.py', copy: true)\n> \n> You could also create a symlink. There's an example in the top-level\n> meson.build.\n\nHmm yes, that's a good idea.\n\n>> +\n>> +install_data(['__init__.py'], install_dir: destdir)\n> \n> Missing space after ':'.\n\nOk.\n\n>> diff --git a/src/py/pycamera/pymain.cpp b/src/py/pycamera/pymain.cpp\n>> new file mode 100644\n>> index 00000000..bd1b9bdd\n>> --- /dev/null\n>> +++ b/src/py/pycamera/pymain.cpp\n>> @@ -0,0 +1,382 @@\n> \n> Missing SPDX tag and initial comment block.\n\nOk. Is the file name and the description required (e.g. \"object.cpp - Base object\") by some licence\nguide? I don't quite see what benefit such a line provides.\n\n>> +#include <chrono>\n>> +#include <thread>\n>> +#include <fcntl.h>\n>> +#include <unistd.h>\n>> +#include <sys/mman.h>\n>> +#include <sys/eventfd.h>\n>> +#include <mutex>\n> \n> Can you copy utils/hooks/post-commit to .git/hooks/ to ensure\n> checkstyle.py is run ? It should warn about incorrect alphabetical\n> order.\n\nOk, I'll have a look. I have never used the hooks, but I have the feeling that I won't like it. I\ncommit often, and maybe 90% of the times I commit, the commit is not in any way ready or meant for\npublishing. It may not even compile cleanly (which is also why I hate Werror enabled by default, I\nthink it's just bad and annoying (but CI should enable it)).\n\nI do check patches I sent for (proper) review. I didn't check these as these are just prototypes,\nand the point was just to deprecate the old series so that no one uses it, and possibly to get ideas\non how to solve some of the existing problems.\n\nArranging includes alphabetically wasn't really on my list at all =).\n\n>> +\n>> +#include <pybind11/pybind11.h>\n>> +#include <pybind11/stl.h>\n>> +#include <pybind11/stl_bind.h>\n>> +#include <pybind11/functional.h>\n>> +\n>> +#include <libcamera/libcamera.h>\n>> +\n>> +namespace py = pybind11;\n>> +\n>> +using namespace std;\n> \n> We usually use the namespace prefix explicitly for std.\n\nOk. Have you had conflicts? Having \"std::\" all around uglifies the code in my opinion =)\n\n>> +using namespace libcamera;\n>> +\n>> +static py::object ControlValueToPy(const ControlValue &cv)\n>> +{\n>> +\t//assert(!cv.isArray());\n>> +\t//assert(cv.numElements() == 1);\n> \n> If this isn't useful let's drop those two lines, otherwise let's\n> uncomment them.\n\nThese, are some other things, are just to help testing and debugging. I will clean them up when I\nsend a proper series.\n\n>> +\n>> +\tswitch (cv.type()) {\n>> +\tcase ControlTypeBool:\n>> +\t\treturn py::cast(cv.get<bool>());\n>> +\tcase ControlTypeByte:\n>> +\t\treturn py::cast(cv.get<uint8_t>());\n>> +\tcase ControlTypeInteger32:\n>> +\t\treturn py::cast(cv.get<int32_t>());\n>> +\tcase ControlTypeInteger64:\n>> +\t\treturn py::cast(cv.get<int64_t>());\n>> +\tcase ControlTypeFloat:\n>> +\t\treturn py::cast(cv.get<float>());\n>> +\tcase ControlTypeString:\n>> +\t\treturn py::cast(cv.get<string>());\n>> +\tcase ControlTypeRectangle:\n>> +\tcase ControlTypeSize:\n> \n> Shouldn't rectangle and size be supported ?\n\nSure, and many other things should be supported too =). I didn't add them as I didn't have any\nplatforms that had rectangles or sizes, so I couldn't test. And if I recall right, Rectangle and\nSize weren't trivial to add.\n\n>> +\tcase ControlTypeNone:\n>> +\tdefault:\n>> +\t\tthrow runtime_error(\"Unsupported ControlValue type\");\n>> +\t}\n>> +}\n>> +\n>> +static ControlValue PyToControlValue(py::object &ob, ControlType type)\n> \n> Should the first parameter be const ?\n\nYes, I can constify it.\n\n>> +{\n>> +\tswitch (type) {\n>> +\tcase ControlTypeBool:\n>> +\t\treturn ControlValue(ob.cast<bool>());\n>> +\tcase ControlTypeByte:\n>> +\t\treturn ControlValue(ob.cast<uint8_t>());\n>> +\tcase ControlTypeInteger32:\n>> +\t\treturn ControlValue(ob.cast<int32_t>());\n>> +\tcase ControlTypeInteger64:\n>> +\t\treturn ControlValue(ob.cast<int64_t>());\n>> +\tcase ControlTypeFloat:\n>> +\t\treturn ControlValue(ob.cast<float>());\n>> +\tcase ControlTypeString:\n>> +\t\treturn ControlValue(ob.cast<string>());\n>> +\tcase ControlTypeRectangle:\n>> +\tcase ControlTypeSize:\n> \n> Same here, shouldn't Rectangle and Size be supported ?\n> \n>> +\tcase ControlTypeNone:\n>> +\tdefault:\n>> +\t\tthrow runtime_error(\"Control type not implemented\");\n>> +\t}\n>> +}\n> \n> [snip]\n> \n>> diff --git a/src/py/test/valgrind-pycamera.supp b/src/py/test/valgrind-pycamera.supp\n>> new file mode 100644\n>> index 00000000..98c693f2\n>> --- /dev/null\n>> +++ b/src/py/test/valgrind-pycamera.supp\n>> @@ -0,0 +1,17 @@\n>> +{\n>> +   <insert_a_suppression_name_here>\n> \n> Could you insert a suppression name here ? ;-)\n> \n>> +   Memcheck:Leak\n>> +   match-leak-kinds: definite\n>> +   fun:_Znwm\n> \n> You mentioned that this is a one-time allocation with the callstack\n> below. Does the valgrind suppression file express a complete callstack ?\n\nI can't recall, but it's possible I dropped topmost frames. The suppression was mostly for my\ntesting, not meant to be a \"real\" suppression file. A real suppression file needs studying the case\nin detail, which possibly means looking at python interpreter sources...\n\nIn any case, I don't see this anymore. Possibly more recent pybind11 solved it. Or something else.\nHowever, I see two new leaks...\n\n>> +   fun:_ZN8pybind116moduleC1EPKcS2_\n>> +   fun:PyInit_pycamera\n>> +   fun:_PyImport_LoadDynamicModuleWithSpec\n>> +   obj:/usr/bin/python3.8\n>> +   obj:/usr/bin/python3.8\n>> +   fun:PyVectorcall_Call\n>> +   fun:_PyEval_EvalFrameDefault\n>> +   fun:_PyEval_EvalCodeWithName\n>> +   fun:_PyFunction_Vectorcall\n>> +   fun:_PyEval_EvalFrameDefault\n>> +   fun:_PyFunction_Vectorcall\n> \n> Three spaces is a really weird indentation.\n\nThat's what valgrind gives with its generate suppression feature.\n\n Tomi","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8E826BE177\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 30 Nov 2020 14:03:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E89F1634A3;\n\tMon, 30 Nov 2020 15:03:44 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0069163320\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 30 Nov 2020 15:03:42 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id t6so21806007lfl.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 30 Nov 2020 06:03:42 -0800 (PST)","from [192.168.1.111] (91-152-83-50.elisa-laajakaista.fi.\n\t[91.152.83.50]) by smtp.gmail.com with ESMTPSA id\n\ts22sm2444603lfi.187.2020.11.30.06.03.40\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 30 Nov 2020 06:03:41 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"aePkZnWi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=sender:subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=LtxhY8IQat/ZuqZjTNKt54V+VQAgKPN6wkJR2mHzhJg=;\n\tb=aePkZnWiiYoLtLWtMQ1JfRFy6jMndvcptvRsfvkoEW4uvqzsaRJjXGCS/+FTotsi77\n\t3RUGO06AlM91+wiryOrpMuzgdloOlfwZbKVINT4nNE+gAKw6cvF31BcLuLa2Wpxqwg70\n\tlc2s3coNNSrHWkOeTtDEFBmYBJ22/fgrtpLpzGWC/AiAS4GKnH63A2Qg3ICscgzxPw4H\n\tg/X808M9AW1i+NUyKpPgmbrLsjIQAI3eAab+3pmXMidqf2c+7Y1Je5s3iisGKCBvvt2m\n\tz3vW1DsCeww1JBDbjwET9uFa6nX/Oej0rhq76Fd0RhU+P3kALeiG9mF3+lp791Xz6ELy\n\tLeEQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:sender:subject:to:cc:references:from:message-id\n\t:date:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=LtxhY8IQat/ZuqZjTNKt54V+VQAgKPN6wkJR2mHzhJg=;\n\tb=blKu11KYI9ocDOmrDXYO3o23uaWzIcBBBaEAfIcfKz8KZq7ofg/fqiMfl/+YCkb2z/\n\tdF4Yp9VrZLDd4Jns0jC9y1mf545xT78XQCezAFVIhX+oKIn/HRJ9tvUuJyk1lYqdJSAv\n\t47t611yjCxyYa/e7IHrQ6OqGSnMmKK2U9L2JLph9yDT33sGHkeezhSMgBriS/4LKMKJ3\n\tlNEN2RwzXrCINeJfpVe+c6lIaXQFc8EYgxPkMzil5Q81exccEIN1rcQnJyQNCW/KU0jH\n\tQPe+FJoPwbgl92hJar4OP/RP5c32+0hjee6sqoiUJnmaRGakB3xC1+jchO6VawVf7CEY\n\tu1Rw==","X-Gm-Message-State":"AOAM531RwPF9cqI3aVbxcdNEFjAKVJ0jmEowjNnnOekc+A06Lnr9+Aed\n\t0UwCQ8dyDikIpx4ftRyPoh5MdpSEm7o=","X-Google-Smtp-Source":"ABdhPJxeLKy4humfq9WgrDJbIVP9ZNlDqHwaraHtNL99kUve1YvcCsN+tG/YhHz4xigjdWabTvkh5Q==","X-Received":"by 2002:a19:df54:: with SMTP id\n\tq20mr9966447lfj.300.1606745021979; \n\tMon, 30 Nov 2020 06:03:41 -0800 (PST)","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20201127133738.880859-1-tomi.valkeinen@iki.fi>\n\t<20201127133738.880859-4-tomi.valkeinen@iki.fi>\n\t<20201130001654.GE5893@pendragon.ideasonboard.com>","From":"Tomi Valkeinen <tomi.valkeinen@iki.fi>","Message-ID":"<5f92ba45-193e-bd93-1bda-536855e4a72d@iki.fi>","Date":"Mon, 30 Nov 2020 16:03:40 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201130001654.GE5893@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC v2 3/4] libcamera python bindings","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]