[{"id":24652,"web_url":"https://patchwork.libcamera.org/comment/24652/","msgid":"<20220818142911.4v5ovfkhreumfdbx@uno.localdomain>","date":"2022-08-18T14:29:11","subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Tomi\n\nOn Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote:\n> Blocking wait can be easily implemented on top in Python, so rather than\n> supporting only blocking reads, or supporting both non-blocking and\n> blocking reads, let's support only non-blocking reads.\n>\n\nI'm not sure why it is better to ask application to do so on out\nbehalf, having to expose the camera manager eventfd...\n\n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/examples/simple-cam.py | 5 +++--\n> src/py/examples/simple-capture.py | 12 +++++++++--\n> src/py/examples/simple-continuous-capture.py | 5 +++--\n> src/py/libcamera/py_camera_manager.cpp | 22 +++++++++++++-------\n> src/py/libcamera/py_camera_manager.h | 2 +-\n> test/py/unittests.py | 7 +++++++\n> 6 files changed, 39 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py\n> index 2b81bb65..b3e97ca7 100755\n> --- a/src/py/examples/simple-cam.py\n> +++ b/src/py/examples/simple-cam.py\n> @@ -19,8 +19,9 @@ TIMEOUT_SEC = 3\n>\n>\n> def handle_camera_event(cm):\n> - # cm.get_ready_requests() will not block here, as we know there is an event\n> - # to read.\n> + # cm.get_ready_requests() returns the ready requests, which in our case\n> + # should almost always return a single Request, but in some cases there\n> + # could be multiple or none.\n>\n> reqs = cm.get_ready_requests()\n>\n> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py\n> index a6a9b33e..5f93574f 100755\n> --- a/src/py/examples/simple-capture.py\n> +++ b/src/py/examples/simple-capture.py\n> @@ -14,6 +14,7 @@\n>\n> import argparse\n> import libcamera as libcam\n> +import selectors\n> import sys\n>\n> # Number of frames to capture\n> @@ -107,11 +108,18 @@ def main():\n> # The main loop. Wait for the queued Requests to complete, process them,\n> # and re-queue them again.\n>\n> + sel = selectors.DefaultSelector()\n> + sel.register(cm.event_fd, selectors.EVENT_READ)\n> +\n> while frames_done < TOTAL_FRAMES:\n> - # cm.get_ready_requests() blocks until there is an event and returns\n> - # all the ready requests. Here we should almost always get a single\n> + # cm.get_ready_requests() does not block, so we use a Selector to wait\n> + # for a camera event. Here we should almost always get a single\n> # Request, but in some cases there could be multiple or none.\n>\n> + events = sel.select()\n> + if not events:\n> + continue\n> +\n> reqs = cm.get_ready_requests()\n>\n> for req in reqs:\n> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py\n> index fe78a2dd..26a8060b 100755\n> --- a/src/py/examples/simple-continuous-capture.py\n> +++ b/src/py/examples/simple-continuous-capture.py\n> @@ -88,8 +88,9 @@ class CaptureContext:\n> camera_contexts: list[CameraCaptureContext] = []\n>\n> def handle_camera_event(self):\n> - # cm.get_ready_requests() will not block here, as we know there is an event\n> - # to read.\n> + # cm.get_ready_requests() returns the ready requests, which in our case\n> + # should almost always return a single Request, but in some cases there\n> + # could be multiple or none.\n>\n> reqs = self.cm.get_ready_requests()\n>\n> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> index 5600f661..3dd8668e 100644\n> --- a/src/py/libcamera/py_camera_manager.cpp\n> +++ b/src/py/libcamera/py_camera_manager.cpp\n> @@ -22,7 +22,7 @@ PyCameraManager::PyCameraManager()\n>\n> \tcameraManager_ = std::make_unique();\n>\n> -\tint fd = eventfd(0, EFD_CLOEXEC);\n> +\tint fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> \tif (fd == -1)\n> \t\tthrow std::system_error(errno, std::generic_category(),\n> \t\t\t\t\t\"Failed to create eventfd\");\n> @@ -60,18 +60,24 @@ py::list PyCameraManager::cameras()\n>\n> std::vector PyCameraManager::getReadyRequests()\n> {\n> -\treadFd();\n> +\tint ret = readFd();\n>\n> -\tstd::vector ret;\n> +\tif (ret == EAGAIN)\n> +\t\treturn std::vector();\n> +\n> +\tif (ret != 0)\n> +\t\tthrow std::system_error(ret, std::generic_category());\n> +\n> +\tstd::vector py_reqs;\n>\n> \tfor (Request *request : getCompletedRequests()) {\n> \t\tpy::object o = py::cast(request);\n> \t\t/* Decrease the ref increased in Camera.queue_request() */\n> \t\to.dec_ref();\n> -\t\tret.push_back(o);\n> +\t\tpy_reqs.push_back(o);\n> \t}\n>\n> -\treturn ret;\n> +\treturn py_reqs;\n> }\n>\n> /* Note: Called from another thread */\n> @@ -94,12 +100,14 @@ void PyCameraManager::writeFd()\n> \t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> }\n>\n> -void PyCameraManager::readFd()\n> +int PyCameraManager::readFd()\n> {\n> \tuint8_t buf[8];\n>\n> \tif (read(eventFd_.get(), buf, 8) != 8)\n> -\t\tthrow std::system_error(errno, std::generic_category());\n> +\t\treturn errno;\n> +\n> +\treturn 0;\n> }\n>\n> void PyCameraManager::pushRequest(Request *req)\n> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> index 56bea13d..3525057d 100644\n> --- a/src/py/libcamera/py_camera_manager.h\n> +++ b/src/py/libcamera/py_camera_manager.h\n> @@ -39,7 +39,7 @@ private:\n> \t\tLIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);\n>\n> \tvoid writeFd();\n> -\tvoid readFd();\n> +\tint readFd();\n> \tvoid pushRequest(Request *req);\n> \tstd::vector getCompletedRequests();\n> };\n> diff --git a/test/py/unittests.py b/test/py/unittests.py\n> index 9adc4337..6dea80fc 100755\n> --- a/test/py/unittests.py\n> +++ b/test/py/unittests.py\n> @@ -207,9 +207,16 @@ class SimpleCaptureMethods(CameraTesterBase):\n> reqs = None\n> gc.collect()\n>\n> + sel = selectors.DefaultSelector()\n> + sel.register(cm.event_fd, selectors.EVENT_READ)\n> +\n> reqs = []\n>\n> while True:\n> + events = sel.select()\n> + if not events:\n> + continue\n> +\n> ready_reqs = cm.get_ready_requests()\n>\n> reqs += ready_reqs\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","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 4E3AFBE173\n\tfor ;\n\tThu, 18 Aug 2022 14:29:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92E1A61FC0;\n\tThu, 18 Aug 2022 16:29:21 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F94D61FA7\n\tfor ;\n\tThu, 18 Aug 2022 16:29:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 61891100003;\n\tThu, 18 Aug 2022 14:29:18 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660832961;\n\tbh=1sZkWkLGRrQkRkm0ogK0z+WyOnCvTXC+AWbewPBvVNE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=JctUfJVSXnKuFpD7r4DCL33SX+3EXXVVfetPpuwK44PKeB9jl5IqmFZRN8V5BlXdX\n\tA6QNgcy6dBe02si29OQC4iUVWWacdRdNV5DjonytY0FPHjhpKjj3h6ajU2WckmtXm1\n\tTfN1a4Zthmgaf2DoKjbMsM/+clmTWF4Pw2rydXxs68016oxqejLPG+RZc/fLhTs3ur\n\tKtJAkc5awRM2zjHnfp48F8nySLCsnB7bcfm4n4wF/e6xqpInN0lXSss5VeSfK+OFTk\n\tr36xhBUQLpsNLcRF4yGFx6ypsRjCzVeRErR0K7UVS0wiBVnmLL2J3dKUkg0632McCp\n\tECVqBWN62mRgQ==","Date":"Thu, 18 Aug 2022 16:29:11 +0200","To":"Tomi Valkeinen ","Message-ID":"<20220818142911.4v5ovfkhreumfdbx@uno.localdomain>","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24657,"web_url":"https://patchwork.libcamera.org/comment/24657/","msgid":"","date":"2022-08-18T14:49:39","subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 18/08/2022 17:29, Jacopo Mondi wrote:\n> Hi Tomi\n> \n> On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote:\n>> Blocking wait can be easily implemented on top in Python, so rather than\n>> supporting only blocking reads, or supporting both non-blocking and\n>> blocking reads, let's support only non-blocking reads.\n>>\n> \n> I'm not sure why it is better to ask application to do so on out\n> behalf, having to expose the camera manager eventfd...\n\nWe want to expose the fd anyway. Most, or at least all serious uses will \nneed the fd to wait for the events. That's how it is even before this \nseries.\n\nThe point of this change is that instead of offering a function to get \nthe ready events which blocks if there are no events, we offer a \nfunction which never blocks.\n\nWith this change the users can either just call the function whenever to \nsee if there are events, or they can wait for the events with the fd. \nPreviously this was not possible.\n\n Tomi","headers":{"Return-Path":"","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 A3A30BE173\n\tfor ;\n\tThu, 18 Aug 2022 14:49:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99EB361FC0;\n\tThu, 18 Aug 2022 16:49:45 +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 10D5A61FA7\n\tfor ;\n\tThu, 18 Aug 2022 16:49:44 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18BA08B;\n\tThu, 18 Aug 2022 16:49:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660834185;\n\tbh=2eJnte5ZzbkUZO2kwdcltCrY5DfzpZJgnalZj5RyNXw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ah8TC4sLMQKtfp6HrlnYyKgGJ4lLZhVdCWFnHuDogUF8orPi5m+oKv6+sJvuS4gaT\n\tICTTFSzjA9E25jSi2k1TaKTFH1s3mCXppwW8Vs02BE7g6UsyZbGnfqMqOX7x6weH7Z\n\ttzb9daJg3CEIdc00B0uNH1xX2wXnDRSyU3nKSUTLlICkZuj1JBrhsCRghKVutj4inD\n\t+B59Z8shwRNGVAVYWt7qfuitwJ74FJZz1X3AWLZO8TceyQdgXygxe2Nu5MCDUxMGJ5\n\tkbvs17ytzfgbTM794sOtS/vjGThtKaERe+K2Cr7Pb2Ae9WEBvzXRiPd6bRQkndXmqs\n\tgMNUwm+X5tNkg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660834183;\n\tbh=2eJnte5ZzbkUZO2kwdcltCrY5DfzpZJgnalZj5RyNXw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=joTJApYxsRQuBHci6U3ankVHtreXXbD6g/eZCaKY2cwdP7GhpJmbFbWGJYz6GK1MK\n\t7tLceVv+CxP54GyVxrJKBa6QEGPz4jQ+sxzKcf+wuba/ptNA6br2kqOfNev1oGmq2a\n\t+GbPrAAWSctBNEsYbuGa3eYgzEGAS4tm5utdhHro="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"joTJApYx\"; dkim-atps=neutral","Message-ID":"","Date":"Thu, 18 Aug 2022 17:49:39 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>\n\t<20220818142911.4v5ovfkhreumfdbx@uno.localdomain>","In-Reply-To":"<20220818142911.4v5ovfkhreumfdbx@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24663,"web_url":"https://patchwork.libcamera.org/comment/24663/","msgid":"<20220818150244.wyy6qfeugaji6beo@uno.localdomain>","date":"2022-08-18T15:02:44","subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"> On 18/08/2022 17:29, Jacopo Mondi wrote:\n> > Hi Tomi\n> >\n> > On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote:\n> > > Blocking wait can be easily implemented on top in Python, so rather than\n> > > supporting only blocking reads, or supporting both non-blocking and\n> > > blocking reads, let's support only non-blocking reads.\n> > >\n> >\n> > I'm not sure why it is better to ask application to do so on out\n> > behalf, having to expose the camera manager eventfd...\n>\n> We want to expose the fd anyway. Most, or at least all serious uses will\n> need the fd to wait for the events. That's how it is even before this\n> series.\n>\n> The point of this change is that instead of offering a function to get the\n> ready events which blocks if there are no events, we offer a function which\n> never blocks.\n>\n> With this change the users can either just call the function whenever to see\n> if there are events, or they can wait for the events with the fd. Previously\n> this was not possible.\n\nI would have expected a non-blocking \"eventReady()\" and a blocking\nimplementation inside the python wrapper instead of having apps to\ninstrument a selector. Maybe that's typical for Python ?\n\n\n>\n> Tomi","headers":{"Return-Path":"","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 C40A6C3272\n\tfor ;\n\tThu, 18 Aug 2022 15:02:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7CA6461FC3;\n\tThu, 18 Aug 2022 17:02:50 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6FDA61FA7\n\tfor ;\n\tThu, 18 Aug 2022 17:02:48 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id C8A7210000A;\n\tThu, 18 Aug 2022 15:02:46 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660834970;\n\tbh=UdDB2TxEg2+o8Ex0iqeA+7p2e9w3jeKkqm5y4cJsZEw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=AEXTkCD8NCGlKWvqI21QhAwHP6szDFy55PqxeDrqzcoZOKOfujLaB3q88KlSVlQBZ\n\tdwzNe9fHfnGKFm/prMJ2AhSZPR8Ph6SuKfKSF1AUsMBcVT3o8pGSODHvdPHILxl4hD\n\tAqF9OquFSIoCpit/3eAa3KEstnR/nKGJX+HuxpX+v7+bM098h+gmWmbfIferurbxty\n\t0zMeW4M0TcxEQZwK27FMR90hBXTVbgvOso9RVBzp+jfLjmr3ZYJY1y+7XVMV2c7ZP8\n\tZ+VfF0ltzSI1+veidKzuiW42BhAk7ohToYpF+N/A/7uuvublcewzXzcx+LHvJD6yM8\n\tnhpBG+Opa9u8A==","Date":"Thu, 18 Aug 2022 17:02:44 +0200","To":"Tomi Valkeinen ","Message-ID":"<20220818150244.wyy6qfeugaji6beo@uno.localdomain>","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>\n\t<20220818142911.4v5ovfkhreumfdbx@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24668,"web_url":"https://patchwork.libcamera.org/comment/24668/","msgid":"<4654b520-d438-7814-981c-14b4a2cfc67b@ideasonboard.com>","date":"2022-08-18T15:21:51","subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 18/08/2022 18:02, Jacopo Mondi wrote:\n>> On 18/08/2022 17:29, Jacopo Mondi wrote:\n>>> Hi Tomi\n>>>\n>>> On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote:\n>>>> Blocking wait can be easily implemented on top in Python, so rather than\n>>>> supporting only blocking reads, or supporting both non-blocking and\n>>>> blocking reads, let's support only non-blocking reads.\n>>>>\n>>>\n>>> I'm not sure why it is better to ask application to do so on out\n>>> behalf, having to expose the camera manager eventfd...\n>>\n>> We want to expose the fd anyway. Most, or at least all serious uses will\n>> need the fd to wait for the events. That's how it is even before this\n>> series.\n>>\n>> The point of this change is that instead of offering a function to get the\n>> ready events which blocks if there are no events, we offer a function which\n>> never blocks.\n>>\n>> With this change the users can either just call the function whenever to see\n>> if there are events, or they can wait for the events with the fd. Previously\n>> this was not possible.\n> \n> I would have expected a non-blocking \"eventReady()\" and a blocking\n> implementation inside the python wrapper instead of having apps to\n> instrument a selector. Maybe that's typical for Python ?\n\nThere are, of course, many ways to implement this. This was discussed, \nand if I recall right, in my previous versions (and some I didn't send) \nI was going back and forth between different ways to manage this. If I \nrecall right, Laurent was (strongly?) of the opinion that blocking mode \nis not needed by the helpers. I'm kind of in the middle, I guess. It's \neasy to implement on top, though.\n\n Tomi","headers":{"Return-Path":"","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 58DE7BE173\n\tfor ;\n\tThu, 18 Aug 2022 15:21:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF59561FC0;\n\tThu, 18 Aug 2022 17:21:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A5E7F61FA7\n\tfor ;\n\tThu, 18 Aug 2022 17:21:55 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D0DA28B;\n\tThu, 18 Aug 2022 17:21:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660836116;\n\tbh=rCOKD88H2NiNy4nxbryaYAorsi0oPfqXaf9zedz9y1k=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=19rwiPu5LxtP3YhUX84RQS6naWgervj2YASQTY4yXGDUO/HIEaZ+rV/AEA9uGq3BR\n\tJqprAZuijCVJ+wt4mb2M64fBYmCxu5/4Ub63r44FhhWp+ixFsXtott99Z/csc70JcE\n\trBcbKvMpqQS5We7WV4ASpxzCzFOhqjnrHw+7vXEls8VaY1QaaF9tmmYHLRb6IhhA5q\n\tkBXDfgHXc5Rsu0/BcASHPkvzq3db7TpN/5E3ghxm6xDX7BSdH9FmVpzH75nH59ObhK\n\t+dPJO192ESR2X+fzs98EoPj7768EnIkStwjrPeVT6Vaq6vFFcs09++0TheJGFHDHl6\n\tMQqa5VAzCxWkQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660836115;\n\tbh=rCOKD88H2NiNy4nxbryaYAorsi0oPfqXaf9zedz9y1k=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=YRYi1JIZq6fA/e1GCxYkxpNkr4Bh4xBDy6+aOpZ0v8hi2jyVz5Wfe5FtwyM9tIk7X\n\tFyLWUoOZO2ZLCJt45xj7OkIrATziVdPWVF9kW/Qw4gpZCztFMPd47D6aPBdFzttN2m\n\t1d6oZ+VOVsaMYBIyhCeqCdI2epzc+cgPMDAbT93I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YRYi1JIZ\"; dkim-atps=neutral","Message-ID":"<4654b520-d438-7814-981c-14b4a2cfc67b@ideasonboard.com>","Date":"Thu, 18 Aug 2022 18:21:51 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>\n\t<20220818142911.4v5ovfkhreumfdbx@uno.localdomain>\n\t\n\t<20220818150244.wyy6qfeugaji6beo@uno.localdomain>","In-Reply-To":"<20220818150244.wyy6qfeugaji6beo@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24684,"web_url":"https://patchwork.libcamera.org/comment/24684/","msgid":"","date":"2022-08-18T19:24:55","subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 18, 2022 at 06:21:51PM +0300, Tomi Valkeinen wrote:\n> On 18/08/2022 18:02, Jacopo Mondi wrote:\n> >> On 18/08/2022 17:29, Jacopo Mondi wrote:\n> >>> On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote:\n> >>>> Blocking wait can be easily implemented on top in Python, so rather than\n> >>>> supporting only blocking reads, or supporting both non-blocking and\n> >>>> blocking reads, let's support only non-blocking reads.\n> >>>>\n> >>>\n> >>> I'm not sure why it is better to ask application to do so on out\n> >>> behalf, having to expose the camera manager eventfd...\n> >>\n> >> We want to expose the fd anyway. Most, or at least all serious uses will\n> >> need the fd to wait for the events. That's how it is even before this\n> >> series.\n> >>\n> >> The point of this change is that instead of offering a function to get the\n> >> ready events which blocks if there are no events, we offer a function which\n> >> never blocks.\n> >>\n> >> With this change the users can either just call the function whenever to see\n> >> if there are events, or they can wait for the events with the fd. Previously\n> >> this was not possible.\n> > \n> > I would have expected a non-blocking \"eventReady()\" and a blocking\n> > implementation inside the python wrapper instead of having apps to\n> > instrument a selector. Maybe that's typical for Python ?\n> \n> There are, of course, many ways to implement this. This was discussed, \n> and if I recall right, in my previous versions (and some I didn't send) \n> I was going back and forth between different ways to manage this. If I \n> recall right, Laurent was (strongly?) of the opinion that blocking mode \n> is not needed by the helpers. I'm kind of in the middle, I guess. It's \n> easy to implement on top, though.\n\nThat matches my recollection of the discussions :-)","headers":{"Return-Path":"","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 C1692C3272\n\tfor ;\n\tThu, 18 Aug 2022 19:25:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0909861FC0;\n\tThu, 18 Aug 2022 21:25:01 +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 26F4061FA7\n\tfor ;\n\tThu, 18 Aug 2022 21:24:59 +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 697AD8B;\n\tThu, 18 Aug 2022 21:24:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660850701;\n\tbh=dI+VEmjX3Nra43F+ZZM9ZJ4JsusPDQ7wz7T+pIO1gY8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=2LYMZJUXJ3VVrepANZY338fMzODli7JgzoUdMY5djTDvnAdYvMPd42iuGs+D8zMgR\n\t+UiaOfTaTXkR/tgmhJzjMemwzjV4PbOpTTP+LpSYLYAC1593eQ0V0xTJWqQKn50WFh\n\tOHrMxfo1SCJ+u+u5IYB05XILZoENNK0N0hy5t8WnG2GKETdQ98VZ26ivBl1EmfoyUs\n\tTzRvP2P818YxKlMvaa9oNnLvLWE77DUEp8RNcG4LGqWxt7Q8WK9xpFhxh8ZL4VHs+w\n\tpxXCEuJmVGAqOnsx5FcxQx7QyIKTlBam+5xkQiEKeD+B4dmCGaFoWRBHJOF3qk09gn\n\tly3m8AqT7FeSg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660850698;\n\tbh=dI+VEmjX3Nra43F+ZZM9ZJ4JsusPDQ7wz7T+pIO1gY8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uPb8PL8I/JyZ+7jFL7gyVLayzr0H5+pBr3/LdoRbdeh2SpY8EztgRkjzKWlU3Ptfp\n\t/b0kGyTo3p/fEisRtyP4RoCnzbT9pkUt77yQzw7OtyHchF8uA2h6T20l+S6J5tfkvJ\n\tGY602SDvDn41wW5h59OtzGWBnJXi34bRHGmzOCfU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uPb8PL8I\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 22:24:55 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>\n\t<20220818142911.4v5ovfkhreumfdbx@uno.localdomain>\n\t\n\t<20220818150244.wyy6qfeugaji6beo@uno.localdomain>\n\t<4654b520-d438-7814-981c-14b4a2cfc67b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<4654b520-d438-7814-981c-14b4a2cfc67b@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24685,"web_url":"https://patchwork.libcamera.org/comment/24685/","msgid":"","date":"2022-08-18T20:07:07","subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","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, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote:\n> Blocking wait can be easily implemented on top in Python, so rather than\n> supporting only blocking reads, or supporting both non-blocking and\n> blocking reads, let's support only non-blocking reads.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/examples/simple-cam.py | 5 +++--\n> src/py/examples/simple-capture.py | 12 +++++++++--\n> src/py/examples/simple-continuous-capture.py | 5 +++--\n> src/py/libcamera/py_camera_manager.cpp | 22 +++++++++++++-------\n> src/py/libcamera/py_camera_manager.h | 2 +-\n> test/py/unittests.py | 7 +++++++\n> 6 files changed, 39 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py\n> index 2b81bb65..b3e97ca7 100755\n> --- a/src/py/examples/simple-cam.py\n> +++ b/src/py/examples/simple-cam.py\n> @@ -19,8 +19,9 @@ TIMEOUT_SEC = 3\n> \n> \n> def handle_camera_event(cm):\n> - # cm.get_ready_requests() will not block here, as we know there is an event\n> - # to read.\n> + # cm.get_ready_requests() returns the ready requests, which in our case\n> + # should almost always return a single Request, but in some cases there\n> + # could be multiple or none.\n> \n> reqs = cm.get_ready_requests()\n> \n> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py\n> index a6a9b33e..5f93574f 100755\n> --- a/src/py/examples/simple-capture.py\n> +++ b/src/py/examples/simple-capture.py\n> @@ -14,6 +14,7 @@\n> \n> import argparse\n> import libcamera as libcam\n> +import selectors\n> import sys\n> \n> # Number of frames to capture\n> @@ -107,11 +108,18 @@ def main():\n> # The main loop. Wait for the queued Requests to complete, process them,\n> # and re-queue them again.\n> \n> + sel = selectors.DefaultSelector()\n> + sel.register(cm.event_fd, selectors.EVENT_READ)\n> +\n> while frames_done < TOTAL_FRAMES:\n> - # cm.get_ready_requests() blocks until there is an event and returns\n> - # all the ready requests. Here we should almost always get a single\n> + # cm.get_ready_requests() does not block, so we use a Selector to wait\n> + # for a camera event. Here we should almost always get a single\n> # Request, but in some cases there could be multiple or none.\n> \n> + events = sel.select()\n> + if not events:\n> + continue\n> +\n> reqs = cm.get_ready_requests()\n> \n> for req in reqs:\n> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py\n> index fe78a2dd..26a8060b 100755\n> --- a/src/py/examples/simple-continuous-capture.py\n> +++ b/src/py/examples/simple-continuous-capture.py\n> @@ -88,8 +88,9 @@ class CaptureContext:\n> camera_contexts: list[CameraCaptureContext] = []\n> \n> def handle_camera_event(self):\n> - # cm.get_ready_requests() will not block here, as we know there is an event\n> - # to read.\n> + # cm.get_ready_requests() returns the ready requests, which in our case\n> + # should almost always return a single Request, but in some cases there\n> + # could be multiple or none.\n> \n> reqs = self.cm.get_ready_requests()\n> \n> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> index 5600f661..3dd8668e 100644\n> --- a/src/py/libcamera/py_camera_manager.cpp\n> +++ b/src/py/libcamera/py_camera_manager.cpp\n> @@ -22,7 +22,7 @@ PyCameraManager::PyCameraManager()\n> \n> \tcameraManager_ = std::make_unique();\n> \n> -\tint fd = eventfd(0, EFD_CLOEXEC);\n> +\tint fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> \tif (fd == -1)\n> \t\tthrow std::system_error(errno, std::generic_category(),\n> \t\t\t\t\t\"Failed to create eventfd\");\n> @@ -60,18 +60,24 @@ py::list PyCameraManager::cameras()\n> \n> std::vector PyCameraManager::getReadyRequests()\n> {\n> -\treadFd();\n> +\tint ret = readFd();\n> \n> -\tstd::vector ret;\n> +\tif (ret == EAGAIN)\n> +\t\treturn std::vector();\n> +\n> +\tif (ret != 0)\n> +\t\tthrow std::system_error(ret, std::generic_category());\n> +\n> +\tstd::vector py_reqs;\n\nYou could possibly name the variable py_reqs already in the patch that\nintroduces this function, up to you.\n\n> \n> \tfor (Request *request : getCompletedRequests()) {\n> \t\tpy::object o = py::cast(request);\n> \t\t/* Decrease the ref increased in Camera.queue_request() */\n> \t\to.dec_ref();\n> -\t\tret.push_back(o);\n> +\t\tpy_reqs.push_back(o);\n> \t}\n> \n> -\treturn ret;\n> +\treturn py_reqs;\n> }\n> \n> /* Note: Called from another thread */\n> @@ -94,12 +100,14 @@ void PyCameraManager::writeFd()\n> \t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> }\n> \n> -void PyCameraManager::readFd()\n> +int PyCameraManager::readFd()\n> {\n> \tuint8_t buf[8];\n> \n> \tif (read(eventFd_.get(), buf, 8) != 8)\n> -\t\tthrow std::system_error(errno, std::generic_category());\n> +\t\treturn errno;\n\nWhen no error occurs, there's no guarantee that read() will set errno to\n0, so if the return value of read() is 0, errno will be undefined. I\nwould also return a negative error code as we usually do (don't forget\nto update the caller).\n\n\tret = read(eventFd_.get(), buf, 8);\n\tif (ret == 8)\n\t\treturn 0;\n\telse if (ret < 0)\n\t\treturn -errno;\n\telse\n\t\treturn -E???;\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\treturn 0;\n> }\n> \n> void PyCameraManager::pushRequest(Request *req)\n> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> index 56bea13d..3525057d 100644\n> --- a/src/py/libcamera/py_camera_manager.h\n> +++ b/src/py/libcamera/py_camera_manager.h\n> @@ -39,7 +39,7 @@ private:\n> \t\tLIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);\n> \n> \tvoid writeFd();\n> -\tvoid readFd();\n> +\tint readFd();\n> \tvoid pushRequest(Request *req);\n> \tstd::vector getCompletedRequests();\n> };\n> diff --git a/test/py/unittests.py b/test/py/unittests.py\n> index 9adc4337..6dea80fc 100755\n> --- a/test/py/unittests.py\n> +++ b/test/py/unittests.py\n> @@ -207,9 +207,16 @@ class SimpleCaptureMethods(CameraTesterBase):\n> reqs = None\n> gc.collect()\n> \n> + sel = selectors.DefaultSelector()\n> + sel.register(cm.event_fd, selectors.EVENT_READ)\n> +\n> reqs = []\n> \n> while True:\n> + events = sel.select()\n> + if not events:\n> + continue\n> +\n> ready_reqs = cm.get_ready_requests()\n> \n> reqs += ready_reqs","headers":{"Return-Path":"","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 5B820BE173\n\tfor ;\n\tThu, 18 Aug 2022 20:07:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B33F961FC0;\n\tThu, 18 Aug 2022 22:07:12 +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 729D061FA7\n\tfor ;\n\tThu, 18 Aug 2022 22:07:11 +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 B8FB28B;\n\tThu, 18 Aug 2022 22:07:10 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660853232;\n\tbh=6skPYIdYQJNIUjOF0jVkmeB5MnaTGbe/nIrDyT+JX/M=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zcBgYZ/QuA9ne65H0ry+22e+z8B/rmhV2eZqZnOqPYuvgwzhKaCVDEy5D8QJFa7AG\n\t1yVaVrE/VEuBfLZwKNDxuYJmhI3R9Wrf704mZavlVIIhpTTeeK4cJ6d1rafeYflRha\n\tsqzYATWU7MqP/0jSiFnkaJPpuSCmWAQa2AQpJz9dKuQIsUWSYILoDuHnqZn8GjOlMd\n\thT5E2f97hJJPJwxauY1tpC0OUTLqE7gcG32MrDLHs82e+Ww6BoD8ttmZuu8hHuej0e\n\tss9VALMu/NUZUynjrn7XASC6gNpf7WWlbJocjzzlCxVg/N0s6loaMsIsKoPXj/na/D\n\t3nCvnyOcPQ0Kw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660853230;\n\tbh=6skPYIdYQJNIUjOF0jVkmeB5MnaTGbe/nIrDyT+JX/M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ps07kLbxQ0HBFznr57SHQVQG54SkMPxdgXarsnSBws/veRhN+6sGzI1lXzlpxERM0\n\tUFBt80od6ogxxOfnB0E4iabnyYS7zOwpTRKq1DVG/kfNGmF5EVTIFkQZ0XOLs0SMVe\n\tW8ns2nyqWTQfko0dpkd8krYqZL2B23w5XmGfyHcg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ps07kLbx\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 23:07:07 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24702,"web_url":"https://patchwork.libcamera.org/comment/24702/","msgid":"<8fa86c78-43c5-7306-6bff-3bab60599047@ideasonboard.com>","date":"2022-08-19T06:10:48","subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 18/08/2022 23:07, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote:\n>> Blocking wait can be easily implemented on top in Python, so rather than\n>> supporting only blocking reads, or supporting both non-blocking and\n>> blocking reads, let's support only non-blocking reads.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/examples/simple-cam.py | 5 +++--\n>> src/py/examples/simple-capture.py | 12 +++++++++--\n>> src/py/examples/simple-continuous-capture.py | 5 +++--\n>> src/py/libcamera/py_camera_manager.cpp | 22 +++++++++++++-------\n>> src/py/libcamera/py_camera_manager.h | 2 +-\n>> test/py/unittests.py | 7 +++++++\n>> 6 files changed, 39 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py\n>> index 2b81bb65..b3e97ca7 100755\n>> --- a/src/py/examples/simple-cam.py\n>> +++ b/src/py/examples/simple-cam.py\n>> @@ -19,8 +19,9 @@ TIMEOUT_SEC = 3\n>> \n>> \n>> def handle_camera_event(cm):\n>> - # cm.get_ready_requests() will not block here, as we know there is an event\n>> - # to read.\n>> + # cm.get_ready_requests() returns the ready requests, which in our case\n>> + # should almost always return a single Request, but in some cases there\n>> + # could be multiple or none.\n>> \n>> reqs = cm.get_ready_requests()\n>> \n>> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py\n>> index a6a9b33e..5f93574f 100755\n>> --- a/src/py/examples/simple-capture.py\n>> +++ b/src/py/examples/simple-capture.py\n>> @@ -14,6 +14,7 @@\n>> \n>> import argparse\n>> import libcamera as libcam\n>> +import selectors\n>> import sys\n>> \n>> # Number of frames to capture\n>> @@ -107,11 +108,18 @@ def main():\n>> # The main loop. Wait for the queued Requests to complete, process them,\n>> # and re-queue them again.\n>> \n>> + sel = selectors.DefaultSelector()\n>> + sel.register(cm.event_fd, selectors.EVENT_READ)\n>> +\n>> while frames_done < TOTAL_FRAMES:\n>> - # cm.get_ready_requests() blocks until there is an event and returns\n>> - # all the ready requests. Here we should almost always get a single\n>> + # cm.get_ready_requests() does not block, so we use a Selector to wait\n>> + # for a camera event. Here we should almost always get a single\n>> # Request, but in some cases there could be multiple or none.\n>> \n>> + events = sel.select()\n>> + if not events:\n>> + continue\n>> +\n>> reqs = cm.get_ready_requests()\n>> \n>> for req in reqs:\n>> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py\n>> index fe78a2dd..26a8060b 100755\n>> --- a/src/py/examples/simple-continuous-capture.py\n>> +++ b/src/py/examples/simple-continuous-capture.py\n>> @@ -88,8 +88,9 @@ class CaptureContext:\n>> camera_contexts: list[CameraCaptureContext] = []\n>> \n>> def handle_camera_event(self):\n>> - # cm.get_ready_requests() will not block here, as we know there is an event\n>> - # to read.\n>> + # cm.get_ready_requests() returns the ready requests, which in our case\n>> + # should almost always return a single Request, but in some cases there\n>> + # could be multiple or none.\n>> \n>> reqs = self.cm.get_ready_requests()\n>> \n>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n>> index 5600f661..3dd8668e 100644\n>> --- a/src/py/libcamera/py_camera_manager.cpp\n>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>> @@ -22,7 +22,7 @@ PyCameraManager::PyCameraManager()\n>> \n>> \tcameraManager_ = std::make_unique();\n>> \n>> -\tint fd = eventfd(0, EFD_CLOEXEC);\n>> +\tint fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n>> \tif (fd == -1)\n>> \t\tthrow std::system_error(errno, std::generic_category(),\n>> \t\t\t\t\t\"Failed to create eventfd\");\n>> @@ -60,18 +60,24 @@ py::list PyCameraManager::cameras()\n>> \n>> std::vector PyCameraManager::getReadyRequests()\n>> {\n>> -\treadFd();\n>> +\tint ret = readFd();\n>> \n>> -\tstd::vector ret;\n>> +\tif (ret == EAGAIN)\n>> +\t\treturn std::vector();\n>> +\n>> +\tif (ret != 0)\n>> +\t\tthrow std::system_error(ret, std::generic_category());\n>> +\n>> +\tstd::vector py_reqs;\n> \n> You could possibly name the variable py_reqs already in the patch that\n> introduces this function, up to you.\n\nOk.\n\n>> \n>> \tfor (Request *request : getCompletedRequests()) {\n>> \t\tpy::object o = py::cast(request);\n>> \t\t/* Decrease the ref increased in Camera.queue_request() */\n>> \t\to.dec_ref();\n>> -\t\tret.push_back(o);\n>> +\t\tpy_reqs.push_back(o);\n>> \t}\n>> \n>> -\treturn ret;\n>> +\treturn py_reqs;\n>> }\n>> \n>> /* Note: Called from another thread */\n>> @@ -94,12 +100,14 @@ void PyCameraManager::writeFd()\n>> \t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n>> }\n>> \n>> -void PyCameraManager::readFd()\n>> +int PyCameraManager::readFd()\n>> {\n>> \tuint8_t buf[8];\n>> \n>> \tif (read(eventFd_.get(), buf, 8) != 8)\n>> -\t\tthrow std::system_error(errno, std::generic_category());\n>> +\t\treturn errno;\n> \n> When no error occurs, there's no guarantee that read() will set errno to\n> 0, so if the return value of read() is 0, errno will be undefined. I\n> would also return a negative error code as we usually do (don't forget\n> to update the caller).\n\nI think you're right. I read man eventfd so that read always returns 8 \nbytes or an error. And I guess that's correct, but I don't think it's \nstrictly described anywhere, so better to check the return value more \nthoroughly.\n\n Tomi","headers":{"Return-Path":"","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 91A0CC3272\n\tfor ;\n\tFri, 19 Aug 2022 06:10:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0329F61FC0;\n\tFri, 19 Aug 2022 08:10:54 +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 92CED603E2\n\tfor ;\n\tFri, 19 Aug 2022 08:10:52 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C6FD13F1;\n\tFri, 19 Aug 2022 08:10:51 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660889454;\n\tbh=ifIxkYa40Ovk679PXspnXnXYyubRfg5bJZOygFYUYRA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=A/qBHiit1yL3hF+b1on1IKRaY8NLao9XjUZC0GAANDPNNLii+kxADQbhMslQQjahq\n\tKzhF+/7+4lgFXKolzii25Zs0W3+aWcDWwZjKmj4tlt+VpyCDTu94L/Z6hTikH3Lt3x\n\tklAqqbD2qZUidhq10Bt60zByEA9mhz6XoexDjPfdbEICgkTsnR9Avd4xkxg1JsYgly\n\t+FmXaPCItSIG17DFfa8m0FMRgUVX43jPS/fI4BbGFaDyp2EOuTVVC1zdasv98THF4+\n\tARuXZDNKJypCg2b/dvE/9OaB9cCiXk28xc3rvBc9hBiyoEC6jQ1npoOJmowHGVtuVW\n\t1i2OTgfC+ljMw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660889452;\n\tbh=ifIxkYa40Ovk679PXspnXnXYyubRfg5bJZOygFYUYRA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Fso6gXMcTsubbKIPF3SMvlbO3gHLcgqQvI/MjwwoyMGYQQnWGhxFf5ojHON8BpZ2a\n\t9F+V7H7/gJkaWX3/FpEIV+A1zvYeXw5IkJ4wNsXySCKyXgXez610Ao0lGjrSW6ULED\n\tncMXI+Du/njLdnJguQFtCQDv1poXG5m6dGtnDY9Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Fso6gXMc\"; dkim-atps=neutral","Message-ID":"<8fa86c78-43c5-7306-6bff-3bab60599047@ideasonboard.com>","Date":"Fri, 19 Aug 2022 09:10:48 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-10-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking\n\teventfd","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]