[{"id":12957,"web_url":"https://patchwork.libcamera.org/comment/12957/","msgid":"<20201004184902.GF8774@pendragon.ideasonboard.com>","date":"2020-10-04T18:49:02","subject":"Re: [libcamera-devel] [RFC 4/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, Sep 18, 2020 at 06:20:19PM +0300, 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> - The forced threading causes some headache. Need to take care to use\n>   gil_scoped_release when C++ context can invoke callbacks, and\n>   gil_scoped_acquire at the invoke wrapper.\n\nI've given this a thought, and I wonder if we could solve it by using an\nevent queue and an eventfd to move everything to the Python thread.\nSignals would be connected to internal functions that would push a new\ninstance of a message object that binds the signal name and its\narguments. An eventfd would be used to wake up the python side, which\nwould call a process() function that would then dispatch the messages to\nPython code.\n\n> - Callbacks. Difficult to attach context to the callbacks. I solved it\n>   with BoundMethodFunction and using lambda captures\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\nCamera is meant to be managed as a shared_ptr<>, so if Python tries to\ndelete is explicitly, there's probably something bad happening\nsomewhere.\n\nIn file included from ../../src/py/pycamera/pymain.cpp:2:\nIn file included from /usr/include/c++/v1/thread:90:\nIn file included from /usr/include/c++/v1/functional:504:\n/usr/include/c++/v1/memory:2363:12: error: calling a private destructor of class 'libcamera::Camera'\n    delete __ptr;\n           ^\n/usr/include/c++/v1/memory:2618:7: note: in instantiation of member function 'std::__1::default_delete<libcamera::Camera>::operator()' requested here\n      __ptr_.second()(__tmp);\n      ^\n/usr/include/c++/v1/memory:2572:19: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::reset' requested here\n  ~unique_ptr() { reset(); }\n                  ^\n/usr/include/c++/v1/memory:3932:21: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::~unique_ptr' requested here\n    unique_ptr<_Yp> __hold(__p);\n                    ^\n../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1309:61: note: in instantiation of function template specialization 'std::__1::shared_ptr<libcamera::Camera>::shared_ptr<libcamera::Camera>' requested here\n            new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());\n                                                            ^\n../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1346:9: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_holder<libcamera::Camera>' requested here\n        init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());\n        ^\n../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1080:32: note: in instantiation of member function 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_instance' requested here\n        record.init_instance = init_instance;\n                               ^\n../../src/py/pycamera/pymain.cpp:116:2: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::class_<>' requested here\n        py::class_<Camera, shared_ptr<Camera>>(m, \"Camera\")\n        ^\n../../include/libcamera/camera.h:108:2: note: declared private here\n        ~Camera();\n        ^\n1 error generated.\n\nThe issue may come from the fact that pybin11 tries to create a\nstd::shared_ptr<> manually to hold the Camera pointer, instead of\ncopying the existing std::shared_ptr<>, but I'm not entirely sure.\n\n> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>\n> ---\n>  meson.build                    |   1 +\n>  meson_options.txt              |   2 +\n>  py/meson.build                 |   1 +\n>  py/pycamera/__init__.py        |  29 ++++++\n>  py/pycamera/meson.build        |  35 +++++++\n>  py/pycamera/pymain.cpp         | 169 +++++++++++++++++++++++++++++++\n>  py/test/run-valgrind.sh        |   3 +\n>  py/test/run.sh                 |   3 +\n>  py/test/test.py                | 177 +++++++++++++++++++++++++++++++++\n>  py/test/valgrind-pycamera.supp |  17 ++++\n>  10 files changed, 437 insertions(+)\n>  create mode 100644 py/meson.build\n>  create mode 100644 py/pycamera/__init__.py\n>  create mode 100644 py/pycamera/meson.build\n>  create mode 100644 py/pycamera/pymain.cpp\n>  create mode 100755 py/test/run-valgrind.sh\n>  create mode 100755 py/test/run.sh\n>  create mode 100755 py/test/test.py\n>  create mode 100644 py/test/valgrind-pycamera.supp\n\n[snip]\n\n> diff --git a/py/test/valgrind-pycamera.supp b/py/test/valgrind-pycamera.supp\n> new file mode 100644\n> index 0000000..98c693f\n> --- /dev/null\n> +++ b/py/test/valgrind-pycamera.supp\n> @@ -0,0 +1,17 @@\n> +{\n> +   <insert_a_suppression_name_here>\n> +   Memcheck:Leak\n> +   match-leak-kinds: definite\n> +   fun:_Znwm\n\nThis is new(unsigned long). No wonder you don't have memory leaks\nanymore :-)\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> +}","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 97DDCC3B5D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  4 Oct 2020 18:49:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2232563B2E;\n\tSun,  4 Oct 2020 20:49:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 883D96035F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  4 Oct 2020 20:49:41 +0200 (CEST)","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 EA37C3B;\n\tSun,  4 Oct 2020 20:49:40 +0200 (CEST)"],"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=\"LJlij78O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601837381;\n\tbh=UZ9SSh6VQzff2PqxGJ2LYlbZRCg+NOwdzEx6lde3yI0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LJlij78OqAAkKOT9MGUkJ2LnbwaL7ZmfgM6Yl6jzVl2gVjJEBKK9KE3yxYUqspZO1\n\tMrTkcExlY2veP213/moy+YOyi4q+XbxZepAWkbjNqv+SmqYGRfXvvp7MYUrCcdEKNx\n\t9M01tp1YK+Pyu086OVRtoYFShSe7DK2nGYK8qNPs=","Date":"Sun, 4 Oct 2020 21:49:02 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@iki.fi>","Message-ID":"<20201004184902.GF8774@pendragon.ideasonboard.com>","References":"<20200918152019.784315-1-tomi.valkeinen@iki.fi>\n\t<20200918152019.784315-5-tomi.valkeinen@iki.fi>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200918152019.784315-5-tomi.valkeinen@iki.fi>","Subject":"Re: [libcamera-devel] [RFC 4/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":12959,"web_url":"https://patchwork.libcamera.org/comment/12959/","msgid":"<0c2c0e4a-f82f-c291-c76d-e29702c14d00@iki.fi>","date":"2020-10-04T19:42:33","subject":"Re: [libcamera-devel] [RFC 4/4] libcamera python bindings","submitter":{"id":70,"url":"https://patchwork.libcamera.org/api/people/70/","name":"Tomi Valkeinen","email":"tomi.valkeinen@iki.fi"},"content":"On 04/10/2020 21:49, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, Sep 18, 2020 at 06:20:19PM +0300, 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>> - The forced threading causes some headache. Need to take care to use\n>>    gil_scoped_release when C++ context can invoke callbacks, and\n>>    gil_scoped_acquire at the invoke wrapper.\n> \n> I've given this a thought, and I wonder if we could solve it by using an\n> event queue and an eventfd to move everything to the Python thread.\n> Signals would be connected to internal functions that would push a new\n> instance of a message object that binds the signal name and its\n> arguments. An eventfd would be used to wake up the python side, which\n> would call a process() function that would then dispatch the messages to\n> Python code.\n\nYeah, at least it would be an interesting comparison case. Curiously, \nthings seem to work very well. I guess CPython frees the lock quite \noften without me specifically doing anything. To be honest, I don't know \nwhy it works.\n\nThat said, I had had deadlock issues previously, and I recently got more \nof them when I tried things with a python thread (doing background work \nwhile running interactive python shell).\n\nI think it may not be worth spending time with eventfd until it's clear \nit will give some benefit, and at the moment it's not quite clear to me.\n\nI can always move the execution from libcamera's thread to my thread in \none way or another. The main question is, will the GIL be freed often \nenough to allow the callbacks to be called. Maybe it is freed often, as \nI guess that's what python's own threads need also.\n\nBut I need to understand this better until I find the answer to that.\n\n>> - Callbacks. Difficult to attach context to the callbacks. I solved it\n>>    with BoundMethodFunction and using lambda captures\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> Camera is meant to be managed as a shared_ptr<>, so if Python tries to\n> delete is explicitly, there's probably something bad happening\n> somewhere.\n> \n> In file included from ../../src/py/pycamera/pymain.cpp:2:\n> In file included from /usr/include/c++/v1/thread:90:\n> In file included from /usr/include/c++/v1/functional:504:\n> /usr/include/c++/v1/memory:2363:12: error: calling a private destructor of class 'libcamera::Camera'\n>      delete __ptr;\n>             ^\n> /usr/include/c++/v1/memory:2618:7: note: in instantiation of member function 'std::__1::default_delete<libcamera::Camera>::operator()' requested here\n>        __ptr_.second()(__tmp);\n>        ^\n> /usr/include/c++/v1/memory:2572:19: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::reset' requested here\n>    ~unique_ptr() { reset(); }\n>                    ^\n> /usr/include/c++/v1/memory:3932:21: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::~unique_ptr' requested here\n>      unique_ptr<_Yp> __hold(__p);\n>                      ^\n> ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1309:61: note: in instantiation of function template specialization 'std::__1::shared_ptr<libcamera::Camera>::shared_ptr<libcamera::Camera>' requested here\n>              new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());\n>                                                              ^\n> ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1346:9: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_holder<libcamera::Camera>' requested here\n>          init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());\n>          ^\n> ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1080:32: note: in instantiation of member function 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_instance' requested here\n>          record.init_instance = init_instance;\n>                                 ^\n> ../../src/py/pycamera/pymain.cpp:116:2: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::class_<>' requested here\n>          py::class_<Camera, shared_ptr<Camera>>(m, \"Camera\")\n>          ^\n> ../../include/libcamera/camera.h:108:2: note: declared private here\n>          ~Camera();\n>          ^\n> 1 error generated.\n> \n> The issue may come from the fact that pybin11 tries to create a\n> std::shared_ptr<> manually to hold the Camera pointer, instead of\n> copying the existing std::shared_ptr<>, but I'm not entirely sure.\n\nYep, no clear idea... \"init_instance\" there sounds like pybind11 is also \ncreating code to that allows creating Camera, which I guess then implies \nalso the need to see the destructor.\n\n>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>\n>> ---\n>>   meson.build                    |   1 +\n>>   meson_options.txt              |   2 +\n>>   py/meson.build                 |   1 +\n>>   py/pycamera/__init__.py        |  29 ++++++\n>>   py/pycamera/meson.build        |  35 +++++++\n>>   py/pycamera/pymain.cpp         | 169 +++++++++++++++++++++++++++++++\n>>   py/test/run-valgrind.sh        |   3 +\n>>   py/test/run.sh                 |   3 +\n>>   py/test/test.py                | 177 +++++++++++++++++++++++++++++++++\n>>   py/test/valgrind-pycamera.supp |  17 ++++\n>>   10 files changed, 437 insertions(+)\n>>   create mode 100644 py/meson.build\n>>   create mode 100644 py/pycamera/__init__.py\n>>   create mode 100644 py/pycamera/meson.build\n>>   create mode 100644 py/pycamera/pymain.cpp\n>>   create mode 100755 py/test/run-valgrind.sh\n>>   create mode 100755 py/test/run.sh\n>>   create mode 100755 py/test/test.py\n>>   create mode 100644 py/test/valgrind-pycamera.supp\n> \n> [snip]\n> \n>> diff --git a/py/test/valgrind-pycamera.supp b/py/test/valgrind-pycamera.supp\n>> new file mode 100644\n>> index 0000000..98c693f\n>> --- /dev/null\n>> +++ b/py/test/valgrind-pycamera.supp\n>> @@ -0,0 +1,17 @@\n>> +{\n>> +   <insert_a_suppression_name_here>\n>> +   Memcheck:Leak\n>> +   match-leak-kinds: definite\n>> +   fun:_Znwm\n> \n> This is new(unsigned long). No wonder you don't have memory leaks\n> anymore :-)\n\nThat's just one 104 byte allocation at init time, with the callstack \nbelow. I don't know what's it about, but look like one time alloc when \nthe pybind11 camera module is created.\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>","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 493EBC3B5D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  4 Oct 2020 19:42:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3A0D63B2E;\n\tSun,  4 Oct 2020 21:42:37 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D19346035F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  4 Oct 2020 21:42:35 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id b19so5439391lji.11\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 04 Oct 2020 12:42:35 -0700 (PDT)","from [192.168.1.136] (91-152-83-50.elisa-laajakaista.fi.\n\t[91.152.83.50]) by smtp.gmail.com with ESMTPSA id\n\tu9sm2306977lju.95.2020.10.04.12.42.34\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 04 Oct 2020 12:42:34 -0700 (PDT)"],"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=\"HDJ3Ij1F\"; 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=N2WMAMoy5US7fLRy/47q6Fn4W3PEOi1D9LF9x8usOhA=;\n\tb=HDJ3Ij1FvE8qBjxMRIdxVAihkxe1naaVnEuo5xyvhl1Tv/p33zPhcY3CTPbxwjJF9G\n\tzMFjsVUtTMgUsGiHdrLF9NQNNHr8KgmuLU5WGeRF6kpQ6g2kbdqSwqt+t5cq8jmT0+qj\n\tNAEtk8o4UeP3Vkd9FC0QLFiGLOtMwpcUlGcjbe4gLcTLMFxzROZORXzOHP5s1YNropB2\n\trkwp80yMhONS4OFxbrUuMi6PxAaYAHAImR3Ggxbx+D+5vg4kzbwEljcAwpZQAZDd/4PX\n\tpCHxjoFlvZI90M0eV2+IRiHxklsGLgL4PXt0u3Gb4XY39QyBBMEIslGCjQz8jOa7LdvZ\n\trF9w==","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=N2WMAMoy5US7fLRy/47q6Fn4W3PEOi1D9LF9x8usOhA=;\n\tb=rpzUEIi3Y0wXQ6uZQFN43d7cWS5rlMOUJ8MlN+0SiFIUCrM8P48hMHW0CNcIYAXoq6\n\tdTCDVk2ujx/lzNWXWhu9sCSR/UMAqL3Q5jlL4vCDImA9ir9iqDXP4kmSt6i/f21KhAKI\n\tdk9UMSBU1lM4CmpRMlWkoDcaJIQ73ipMO0Rk6IliSLM5jyrPMdq1+AJX0pBnNJI/YL7O\n\ta68JDnEztqEROwwnrlajPrLAVw9ILcRDqMm4yBk5/jNB9SYAIJfnyDpQOP3zNL2bB44u\n\tUMM2WmZUirOC4zijv6igJSHMdVtPB/hV0hG2Hf7HKaLgN0Z2skCXkPXfruTj3ABPE782\n\tE5ag==","X-Gm-Message-State":"AOAM532Sut7gcP5Ifp4Bh0u0aRUM6SRsshdVox0yfMlCgQsE85i2dGyM\n\tSHH8qP7KicA/kkxkxtJdEMUZhz4C2fjRAw==","X-Google-Smtp-Source":"ABdhPJxKINEcKWpP83UXn4526J7tubdT9W8awpbfNMgLPxlNlV3/OfNFGUyveKTaYq8uAxq/1Qp7OA==","X-Received":"by 2002:a05:651c:105b:: with SMTP id\n\tx27mr849636ljm.302.1601840554820; \n\tSun, 04 Oct 2020 12:42:34 -0700 (PDT)","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20200918152019.784315-1-tomi.valkeinen@iki.fi>\n\t<20200918152019.784315-5-tomi.valkeinen@iki.fi>\n\t<20201004184902.GF8774@pendragon.ideasonboard.com>","From":"Tomi Valkeinen <tomi.valkeinen@iki.fi>","Message-ID":"<0c2c0e4a-f82f-c291-c76d-e29702c14d00@iki.fi>","Date":"Sun, 4 Oct 2020 22:42:33 +0300","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":"<20201004184902.GF8774@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC 4/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-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12960,"web_url":"https://patchwork.libcamera.org/comment/12960/","msgid":"<20201004194630.GH8774@pendragon.ideasonboard.com>","date":"2020-10-04T19:46:30","subject":"Re: [libcamera-devel] [RFC 4/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\nOn Sun, Oct 04, 2020 at 10:42:33PM +0300, Tomi Valkeinen wrote:\n> On 04/10/2020 21:49, Laurent Pinchart wrote:\n> > On Fri, Sep 18, 2020 at 06:20:19PM +0300, 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> >> - The forced threading causes some headache. Need to take care to use\n> >>    gil_scoped_release when C++ context can invoke callbacks, and\n> >>    gil_scoped_acquire at the invoke wrapper.\n> > \n> > I've given this a thought, and I wonder if we could solve it by using an\n> > event queue and an eventfd to move everything to the Python thread.\n> > Signals would be connected to internal functions that would push a new\n> > instance of a message object that binds the signal name and its\n> > arguments. An eventfd would be used to wake up the python side, which\n> > would call a process() function that would then dispatch the messages to\n> > Python code.\n> \n> Yeah, at least it would be an interesting comparison case. Curiously, \n> things seem to work very well. I guess CPython frees the lock quite \n> often without me specifically doing anything. To be honest, I don't know \n> why it works.\n> \n> That said, I had had deadlock issues previously, and I recently got more \n> of them when I tried things with a python thread (doing background work \n> while running interactive python shell).\n> \n> I think it may not be worth spending time with eventfd until it's clear \n> it will give some benefit, and at the moment it's not quite clear to me.\n> \n> I can always move the execution from libcamera's thread to my thread in \n> one way or another. The main question is, will the GIL be freed often \n> enough to allow the callbacks to be called. Maybe it is freed often, as \n> I guess that's what python's own threads need also.\n\nBlocking the callbacks will not only delay their delivery to the\napplication, but also potentially break the realtime requirements of\npipeline handlers. I think we should really not be doing so.\n\n> But I need to understand this better until I find the answer to that.\n> \n> >> - Callbacks. Difficult to attach context to the callbacks. I solved it\n> >>    with BoundMethodFunction and using lambda captures\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> > Camera is meant to be managed as a shared_ptr<>, so if Python tries to\n> > delete is explicitly, there's probably something bad happening\n> > somewhere.\n> > \n> > In file included from ../../src/py/pycamera/pymain.cpp:2:\n> > In file included from /usr/include/c++/v1/thread:90:\n> > In file included from /usr/include/c++/v1/functional:504:\n> > /usr/include/c++/v1/memory:2363:12: error: calling a private destructor of class 'libcamera::Camera'\n> >      delete __ptr;\n> >             ^\n> > /usr/include/c++/v1/memory:2618:7: note: in instantiation of member function 'std::__1::default_delete<libcamera::Camera>::operator()' requested here\n> >        __ptr_.second()(__tmp);\n> >        ^\n> > /usr/include/c++/v1/memory:2572:19: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::reset' requested here\n> >    ~unique_ptr() { reset(); }\n> >                    ^\n> > /usr/include/c++/v1/memory:3932:21: note: in instantiation of member function 'std::__1::unique_ptr<libcamera::Camera, std::__1::default_delete<libcamera::Camera> >::~unique_ptr' requested here\n> >      unique_ptr<_Yp> __hold(__p);\n> >                      ^\n> > ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1309:61: note: in instantiation of function template specialization 'std::__1::shared_ptr<libcamera::Camera>::shared_ptr<libcamera::Camera>' requested here\n> >              new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());\n> >                                                              ^\n> > ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1346:9: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_holder<libcamera::Camera>' requested here\n> >          init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());\n> >          ^\n> > ../../subprojects/pybind11-2.3.0/include/pybind11/pybind11.h:1080:32: note: in instantiation of member function 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::init_instance' requested here\n> >          record.init_instance = init_instance;\n> >                                 ^\n> > ../../src/py/pycamera/pymain.cpp:116:2: note: in instantiation of function template specialization 'pybind11::class_<libcamera::Camera, std::__1::shared_ptr<libcamera::Camera> >::class_<>' requested here\n> >          py::class_<Camera, shared_ptr<Camera>>(m, \"Camera\")\n> >          ^\n> > ../../include/libcamera/camera.h:108:2: note: declared private here\n> >          ~Camera();\n> >          ^\n> > 1 error generated.\n> > \n> > The issue may come from the fact that pybin11 tries to create a\n> > std::shared_ptr<> manually to hold the Camera pointer, instead of\n> > copying the existing std::shared_ptr<>, but I'm not entirely sure.\n> \n> Yep, no clear idea... \"init_instance\" there sounds like pybind11 is also \n> creating code to that allows creating Camera, which I guess then implies \n> also the need to see the destructor.\n> \n> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>\n> >> ---\n> >>   meson.build                    |   1 +\n> >>   meson_options.txt              |   2 +\n> >>   py/meson.build                 |   1 +\n> >>   py/pycamera/__init__.py        |  29 ++++++\n> >>   py/pycamera/meson.build        |  35 +++++++\n> >>   py/pycamera/pymain.cpp         | 169 +++++++++++++++++++++++++++++++\n> >>   py/test/run-valgrind.sh        |   3 +\n> >>   py/test/run.sh                 |   3 +\n> >>   py/test/test.py                | 177 +++++++++++++++++++++++++++++++++\n> >>   py/test/valgrind-pycamera.supp |  17 ++++\n> >>   10 files changed, 437 insertions(+)\n> >>   create mode 100644 py/meson.build\n> >>   create mode 100644 py/pycamera/__init__.py\n> >>   create mode 100644 py/pycamera/meson.build\n> >>   create mode 100644 py/pycamera/pymain.cpp\n> >>   create mode 100755 py/test/run-valgrind.sh\n> >>   create mode 100755 py/test/run.sh\n> >>   create mode 100755 py/test/test.py\n> >>   create mode 100644 py/test/valgrind-pycamera.supp\n> > \n> > [snip]\n> > \n> >> diff --git a/py/test/valgrind-pycamera.supp b/py/test/valgrind-pycamera.supp\n> >> new file mode 100644\n> >> index 0000000..98c693f\n> >> --- /dev/null\n> >> +++ b/py/test/valgrind-pycamera.supp\n> >> @@ -0,0 +1,17 @@\n> >> +{\n> >> +   <insert_a_suppression_name_here>\n> >> +   Memcheck:Leak\n> >> +   match-leak-kinds: definite\n> >> +   fun:_Znwm\n> > \n> > This is new(unsigned long). No wonder you don't have memory leaks\n> > anymore :-)\n> \n> That's just one 104 byte allocation at init time, with the callstack \n> below. I don't know what's it about, but look like one time alloc when \n> the pybind11 camera module is created.\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> >> +}","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 12EC8C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  4 Oct 2020 19:47:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EBC763B2E;\n\tSun,  4 Oct 2020 21:47:10 +0200 (CEST)","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 CD7A66035F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  4 Oct 2020 21:47:09 +0200 (CEST)","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 4AF303B;\n\tSun,  4 Oct 2020 21:47:09 +0200 (CEST)"],"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=\"bNJvONuE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601840829;\n\tbh=TRjFuj33c+gXPcdvfsPNTgbf5eGdS1BZg9+H8f6N2vo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bNJvONuEcOJtmMuBWxASp6VW6uLZq0h7r8RHYio2l4BJ1aeovU3v5s0gLL/GvENjN\n\tN1ayPEzBoEEkD8bnkWOOcZ6hiNRT/vqg9d63RflJ16vKfObWFdcI41IhSsg+iZettT\n\tZ9FvrShDJu9wg7/TAyUM0973aJeIx3m1kYGtxXQ8=","Date":"Sun, 4 Oct 2020 22:46:30 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@iki.fi>","Message-ID":"<20201004194630.GH8774@pendragon.ideasonboard.com>","References":"<20200918152019.784315-1-tomi.valkeinen@iki.fi>\n\t<20200918152019.784315-5-tomi.valkeinen@iki.fi>\n\t<20201004184902.GF8774@pendragon.ideasonboard.com>\n\t<0c2c0e4a-f82f-c291-c76d-e29702c14d00@iki.fi>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<0c2c0e4a-f82f-c291-c76d-e29702c14d00@iki.fi>","Subject":"Re: [libcamera-devel] [RFC 4/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":12961,"web_url":"https://patchwork.libcamera.org/comment/12961/","msgid":"<07a571ab-7ad6-dd75-4817-bfff967900fb@iki.fi>","date":"2020-10-04T19:55:39","subject":"Re: [libcamera-devel] [RFC 4/4] libcamera python bindings","submitter":{"id":70,"url":"https://patchwork.libcamera.org/api/people/70/","name":"Tomi Valkeinen","email":"tomi.valkeinen@iki.fi"},"content":"On 04/10/2020 22:46, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Sun, Oct 04, 2020 at 10:42:33PM +0300, Tomi Valkeinen wrote:\n>> On 04/10/2020 21:49, Laurent Pinchart wrote:\n>>> On Fri, Sep 18, 2020 at 06:20:19PM +0300, 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>>>> - The forced threading causes some headache. Need to take care to use\n>>>>     gil_scoped_release when C++ context can invoke callbacks, and\n>>>>     gil_scoped_acquire at the invoke wrapper.\n>>>\n>>> I've given this a thought, and I wonder if we could solve it by using an\n>>> event queue and an eventfd to move everything to the Python thread.\n>>> Signals would be connected to internal functions that would push a new\n>>> instance of a message object that binds the signal name and its\n>>> arguments. An eventfd would be used to wake up the python side, which\n>>> would call a process() function that would then dispatch the messages to\n>>> Python code.\n>>\n>> Yeah, at least it would be an interesting comparison case. Curiously,\n>> things seem to work very well. I guess CPython frees the lock quite\n>> often without me specifically doing anything. To be honest, I don't know\n>> why it works.\n>>\n>> That said, I had had deadlock issues previously, and I recently got more\n>> of them when I tried things with a python thread (doing background work\n>> while running interactive python shell).\n>>\n>> I think it may not be worth spending time with eventfd until it's clear\n>> it will give some benefit, and at the moment it's not quite clear to me.\n>>\n>> I can always move the execution from libcamera's thread to my thread in\n>> one way or another. The main question is, will the GIL be freed often\n>> enough to allow the callbacks to be called. Maybe it is freed often, as\n>> I guess that's what python's own threads need also.\n> \n> Blocking the callbacks will not only delay their delivery to the\n> application, but also potentially break the realtime requirements of\n> pipeline handlers. I think we should really not be doing so.\n\nRealtime requirement sounds scary. Can you elaborate? Wouldn't a slow \napplication side normally just cause the capture side to re-use the \ncurrent buffer and overwrite it?\n\nIf it's a requirement that the callbacks may not block for any \nnoticeable time, then I agree, this needs some redesign. But I could \nprobably just add the eventfd on the pybind11 level, without need to \nchange it in the libcamera c++ side? Or maybe that's what you meant =).\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 18A84C3B5D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  4 Oct 2020 19:55:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9886F6229F;\n\tSun,  4 Oct 2020 21:55:42 +0200 (CEST)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FC0560A00\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  4 Oct 2020 21:55:41 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id s205so5472265lja.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 04 Oct 2020 12:55:41 -0700 (PDT)","from [192.168.1.136] (91-152-83-50.elisa-laajakaista.fi.\n\t[91.152.83.50]) by smtp.gmail.com with ESMTPSA id\n\tv11sm2489007lfg.39.2020.10.04.12.55.39\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 04 Oct 2020 12:55:40 -0700 (PDT)"],"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=\"RcB+eQg/\"; 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=3DGJxHI3daALLwmTCYEdYPbRdgs1dxZSMk3l4Ho/fFM=;\n\tb=RcB+eQg/MuVr3jaKJouc3SrQzdkFfhbR0ADty6w32t5XL09K17Mf57WlrICySCeFKW\n\t7EBeVuwy85JhZ5N9/RIne4YWtTJirFA/0AokOspqAS7oBUjwt4Nxc+cq+p7Y5k3wKqfZ\n\t3o5UVoMvv4AVVlKyWg4UFfmYj9cC2EWQ4ZTi8Q1OOGABsNTkHOaLU9ov5F707f/K2GAS\n\tZmy97qbX5niMt2LJC4wL764A2N8Z01hVE4Eg95MjuHNbT3Z/v8phamTLNMgBgn7Z/XyK\n\tHHGHsirbpu3NMFc/2nXRu3DB1T7y8FOYjfb5VR+cL1Z246w+uX4tKKvZ9ILLQs8g6AFR\n\tkGxw==","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=3DGJxHI3daALLwmTCYEdYPbRdgs1dxZSMk3l4Ho/fFM=;\n\tb=J12Yys7ntACgh14Y3mHDoKASfTzGuftGAa/4YF/xBEnG696IhYdTo33f5urjnJjgxa\n\taSLQ3lD8kRXYkfu+9ZWIh6GBnJYeZj+FTAjfH+pCrUwIciWXiq/DWu7y5vf5Id37SCat\n\tPFFV0ZHhYeHFf2Y03aHc138tt3HGDb8uCIXGd/AjOGyFju5/1iu81GSXv16lzmUzFJZw\n\tOlHbEetXQH5N5E5+RU79iOXuOOWYvDIVnKbsbyEn48FiqPbQfs3w3eCPOFTXsMY2Xg9k\n\ttVX2OPLOMzh7IntMaX9CUHb8D3XH14jA0KKM2PFvTg0nnzhC3r7+Com7KLKptP8h6aJ5\n\tjKKg==","X-Gm-Message-State":"AOAM532Uh66boaOszYcA+A0eLijjWzhr02sICeAFD9KsByReI6bbrnoy\n\tkYV4Qzc4drcLHQxgMANPY+bTntHdkjOzPA==","X-Google-Smtp-Source":"ABdhPJxxcHFepuOP67EPSLSehy/b4hudnRchG0tTQISwCjk6dEMWZ+i/Zhault6DyX4tGS6ylYwjVA==","X-Received":"by 2002:a2e:97c9:: with SMTP id m9mr3510829ljj.180.1601841340607;\n\tSun, 04 Oct 2020 12:55:40 -0700 (PDT)","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20200918152019.784315-1-tomi.valkeinen@iki.fi>\n\t<20200918152019.784315-5-tomi.valkeinen@iki.fi>\n\t<20201004184902.GF8774@pendragon.ideasonboard.com>\n\t<0c2c0e4a-f82f-c291-c76d-e29702c14d00@iki.fi>\n\t<20201004194630.GH8774@pendragon.ideasonboard.com>","From":"Tomi Valkeinen <tomi.valkeinen@iki.fi>","Message-ID":"<07a571ab-7ad6-dd75-4817-bfff967900fb@iki.fi>","Date":"Sun, 4 Oct 2020 22:55:39 +0300","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":"<20201004194630.GH8774@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC 4/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-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12962,"web_url":"https://patchwork.libcamera.org/comment/12962/","msgid":"<20201004200013.GI8774@pendragon.ideasonboard.com>","date":"2020-10-04T20:00:13","subject":"Re: [libcamera-devel] [RFC 4/4] libcamera python bindings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Oct 04, 2020 at 10:55:39PM +0300, Tomi Valkeinen wrote:\n> On 04/10/2020 22:46, Laurent Pinchart wrote:\n> > Hi Tomi,\n> > \n> > On Sun, Oct 04, 2020 at 10:42:33PM +0300, Tomi Valkeinen wrote:\n> >> On 04/10/2020 21:49, Laurent Pinchart wrote:\n> >>> On Fri, Sep 18, 2020 at 06:20:19PM +0300, 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> >>>> - The forced threading causes some headache. Need to take care to use\n> >>>>     gil_scoped_release when C++ context can invoke callbacks, and\n> >>>>     gil_scoped_acquire at the invoke wrapper.\n> >>>\n> >>> I've given this a thought, and I wonder if we could solve it by using an\n> >>> event queue and an eventfd to move everything to the Python thread.\n> >>> Signals would be connected to internal functions that would push a new\n> >>> instance of a message object that binds the signal name and its\n> >>> arguments. An eventfd would be used to wake up the python side, which\n> >>> would call a process() function that would then dispatch the messages to\n> >>> Python code.\n> >>\n> >> Yeah, at least it would be an interesting comparison case. Curiously,\n> >> things seem to work very well. I guess CPython frees the lock quite\n> >> often without me specifically doing anything. To be honest, I don't know\n> >> why it works.\n> >>\n> >> That said, I had had deadlock issues previously, and I recently got more\n> >> of them when I tried things with a python thread (doing background work\n> >> while running interactive python shell).\n> >>\n> >> I think it may not be worth spending time with eventfd until it's clear\n> >> it will give some benefit, and at the moment it's not quite clear to me.\n> >>\n> >> I can always move the execution from libcamera's thread to my thread in\n> >> one way or another. The main question is, will the GIL be freed often\n> >> enough to allow the callbacks to be called. Maybe it is freed often, as\n> >> I guess that's what python's own threads need also.\n> > \n> > Blocking the callbacks will not only delay their delivery to the\n> > application, but also potentially break the realtime requirements of\n> > pipeline handlers. I think we should really not be doing so.\n> \n> Realtime requirement sounds scary. Can you elaborate? Wouldn't a slow \n> application side normally just cause the capture side to re-use the \n> current buffer and overwrite it?\n\nIt's not just about image capture for applications, but all the\nalgorithms that run behind the scenes that consume statistics and\nproduce parameters for the sensor and ISP. Control loops don't like\ndelays.\n\n> If it's a requirement that the callbacks may not block for any \n> noticeable time, then I agree, this needs some redesign. But I could \n> probably just add the eventfd on the pybind11 level, without need to \n> change it in the libcamera c++ side? Or maybe that's what you meant =).\n\nThat's exactly what I meant :-)","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 BF678C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  4 Oct 2020 20:00:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55A7263B2E;\n\tSun,  4 Oct 2020 22:00:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B5D56229F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  4 Oct 2020 22:00:53 +0200 (CEST)","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 C370E3B;\n\tSun,  4 Oct 2020 22:00:52 +0200 (CEST)"],"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=\"RDSmtfX6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601841653;\n\tbh=lHDIGXVdLJZf8r8lkBp/cHXSiDsnBgbgNz5D6hfDihA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RDSmtfX6ioJlpqCAf/QyhRYWQ1b3HfZfQEFHF8f/nTNU6ihDmV9hG01VZ/TYLkld7\n\t1RYUsL6c4sv5iqYSLrNWqQqsJSmKpgIqlAJuU1tFkd+1n6idv8qFkRNnTOnzvmq3OB\n\tNrs2rPMPnMO6WN1cDhDR+3q3oU1kWcbNUqMBpyYg=","Date":"Sun, 4 Oct 2020 23:00:13 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Tomi Valkeinen <tomi.valkeinen@iki.fi>","Message-ID":"<20201004200013.GI8774@pendragon.ideasonboard.com>","References":"<20200918152019.784315-1-tomi.valkeinen@iki.fi>\n\t<20200918152019.784315-5-tomi.valkeinen@iki.fi>\n\t<20201004184902.GF8774@pendragon.ideasonboard.com>\n\t<0c2c0e4a-f82f-c291-c76d-e29702c14d00@iki.fi>\n\t<20201004194630.GH8774@pendragon.ideasonboard.com>\n\t<07a571ab-7ad6-dd75-4817-bfff967900fb@iki.fi>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<07a571ab-7ad6-dd75-4817-bfff967900fb@iki.fi>","Subject":"Re: [libcamera-devel] [RFC 4/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>"}}]