| Message ID | 20220701084521.31831-10-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote: > Blocking wait can be easily implemented on top in Python, so rather than > supporting only blocking reads, or supporting both non-blocking and > blocking reads, let's support only non-blocking reads. > I'm not sure why it is better to ask application to do so on out behalf, having to expose the camera manager eventfd... > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > src/py/examples/simple-cam.py | 5 +++-- > src/py/examples/simple-capture.py | 12 +++++++++-- > src/py/examples/simple-continuous-capture.py | 5 +++-- > src/py/libcamera/py_camera_manager.cpp | 22 +++++++++++++------- > src/py/libcamera/py_camera_manager.h | 2 +- > test/py/unittests.py | 7 +++++++ > 6 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py > index 2b81bb65..b3e97ca7 100755 > --- a/src/py/examples/simple-cam.py > +++ b/src/py/examples/simple-cam.py > @@ -19,8 +19,9 @@ TIMEOUT_SEC = 3 > > > def handle_camera_event(cm): > - # cm.get_ready_requests() will not block here, as we know there is an event > - # to read. > + # cm.get_ready_requests() returns the ready requests, which in our case > + # should almost always return a single Request, but in some cases there > + # could be multiple or none. > > reqs = cm.get_ready_requests() > > diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py > index a6a9b33e..5f93574f 100755 > --- a/src/py/examples/simple-capture.py > +++ b/src/py/examples/simple-capture.py > @@ -14,6 +14,7 @@ > > import argparse > import libcamera as libcam > +import selectors > import sys > > # Number of frames to capture > @@ -107,11 +108,18 @@ def main(): > # The main loop. Wait for the queued Requests to complete, process them, > # and re-queue them again. > > + sel = selectors.DefaultSelector() > + sel.register(cm.event_fd, selectors.EVENT_READ) > + > while frames_done < TOTAL_FRAMES: > - # cm.get_ready_requests() blocks until there is an event and returns > - # all the ready requests. Here we should almost always get a single > + # cm.get_ready_requests() does not block, so we use a Selector to wait > + # for a camera event. Here we should almost always get a single > # Request, but in some cases there could be multiple or none. > > + events = sel.select() > + if not events: > + continue > + > reqs = cm.get_ready_requests() > > for req in reqs: > diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py > index fe78a2dd..26a8060b 100755 > --- a/src/py/examples/simple-continuous-capture.py > +++ b/src/py/examples/simple-continuous-capture.py > @@ -88,8 +88,9 @@ class CaptureContext: > camera_contexts: list[CameraCaptureContext] = [] > > def handle_camera_event(self): > - # cm.get_ready_requests() will not block here, as we know there is an event > - # to read. > + # cm.get_ready_requests() returns the ready requests, which in our case > + # should almost always return a single Request, but in some cases there > + # could be multiple or none. > > reqs = self.cm.get_ready_requests() > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > index 5600f661..3dd8668e 100644 > --- a/src/py/libcamera/py_camera_manager.cpp > +++ b/src/py/libcamera/py_camera_manager.cpp > @@ -22,7 +22,7 @@ PyCameraManager::PyCameraManager() > > cameraManager_ = std::make_unique<CameraManager>(); > > - int fd = eventfd(0, EFD_CLOEXEC); > + int fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK); > if (fd == -1) > throw std::system_error(errno, std::generic_category(), > "Failed to create eventfd"); > @@ -60,18 +60,24 @@ py::list PyCameraManager::cameras() > > std::vector<py::object> PyCameraManager::getReadyRequests() > { > - readFd(); > + int ret = readFd(); > > - std::vector<py::object> ret; > + if (ret == EAGAIN) > + return std::vector<py::object>(); > + > + if (ret != 0) > + throw std::system_error(ret, std::generic_category()); > + > + std::vector<py::object> py_reqs; > > for (Request *request : getCompletedRequests()) { > py::object o = py::cast(request); > /* Decrease the ref increased in Camera.queue_request() */ > o.dec_ref(); > - ret.push_back(o); > + py_reqs.push_back(o); > } > > - return ret; > + return py_reqs; > } > > /* Note: Called from another thread */ > @@ -94,12 +100,14 @@ void PyCameraManager::writeFd() > LOG(Python, Fatal) << "Unable to write to eventfd"; > } > > -void PyCameraManager::readFd() > +int PyCameraManager::readFd() > { > uint8_t buf[8]; > > if (read(eventFd_.get(), buf, 8) != 8) > - throw std::system_error(errno, std::generic_category()); > + return errno; > + > + return 0; > } > > void PyCameraManager::pushRequest(Request *req) > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > index 56bea13d..3525057d 100644 > --- a/src/py/libcamera/py_camera_manager.h > +++ b/src/py/libcamera/py_camera_manager.h > @@ -39,7 +39,7 @@ private: > LIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_); > > void writeFd(); > - void readFd(); > + int readFd(); > void pushRequest(Request *req); > std::vector<Request *> getCompletedRequests(); > }; > diff --git a/test/py/unittests.py b/test/py/unittests.py > index 9adc4337..6dea80fc 100755 > --- a/test/py/unittests.py > +++ b/test/py/unittests.py > @@ -207,9 +207,16 @@ class SimpleCaptureMethods(CameraTesterBase): > reqs = None > gc.collect() > > + sel = selectors.DefaultSelector() > + sel.register(cm.event_fd, selectors.EVENT_READ) > + > reqs = [] > > while True: > + events = sel.select() > + if not events: > + continue > + > ready_reqs = cm.get_ready_requests() > > reqs += ready_reqs > -- > 2.34.1 >
On 18/08/2022 17:29, Jacopo Mondi wrote: > Hi Tomi > > On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote: >> Blocking wait can be easily implemented on top in Python, so rather than >> supporting only blocking reads, or supporting both non-blocking and >> blocking reads, let's support only non-blocking reads. >> > > I'm not sure why it is better to ask application to do so on out > behalf, having to expose the camera manager eventfd... We want to expose the fd anyway. Most, or at least all serious uses will need the fd to wait for the events. That's how it is even before this series. The point of this change is that instead of offering a function to get the ready events which blocks if there are no events, we offer a function which never blocks. With this change the users can either just call the function whenever to see if there are events, or they can wait for the events with the fd. Previously this was not possible. Tomi
> On 18/08/2022 17:29, Jacopo Mondi wrote: > > Hi Tomi > > > > On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote: > > > Blocking wait can be easily implemented on top in Python, so rather than > > > supporting only blocking reads, or supporting both non-blocking and > > > blocking reads, let's support only non-blocking reads. > > > > > > > I'm not sure why it is better to ask application to do so on out > > behalf, having to expose the camera manager eventfd... > > We want to expose the fd anyway. Most, or at least all serious uses will > need the fd to wait for the events. That's how it is even before this > series. > > The point of this change is that instead of offering a function to get the > ready events which blocks if there are no events, we offer a function which > never blocks. > > With this change the users can either just call the function whenever to see > if there are events, or they can wait for the events with the fd. Previously > this was not possible. I would have expected a non-blocking "eventReady()" and a blocking implementation inside the python wrapper instead of having apps to instrument a selector. Maybe that's typical for Python ? > > Tomi
On 18/08/2022 18:02, Jacopo Mondi wrote: >> On 18/08/2022 17:29, Jacopo Mondi wrote: >>> Hi Tomi >>> >>> On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote: >>>> Blocking wait can be easily implemented on top in Python, so rather than >>>> supporting only blocking reads, or supporting both non-blocking and >>>> blocking reads, let's support only non-blocking reads. >>>> >>> >>> I'm not sure why it is better to ask application to do so on out >>> behalf, having to expose the camera manager eventfd... >> >> We want to expose the fd anyway. Most, or at least all serious uses will >> need the fd to wait for the events. That's how it is even before this >> series. >> >> The point of this change is that instead of offering a function to get the >> ready events which blocks if there are no events, we offer a function which >> never blocks. >> >> With this change the users can either just call the function whenever to see >> if there are events, or they can wait for the events with the fd. Previously >> this was not possible. > > I would have expected a non-blocking "eventReady()" and a blocking > implementation inside the python wrapper instead of having apps to > instrument a selector. Maybe that's typical for Python ? There are, of course, many ways to implement this. This was discussed, and if I recall right, in my previous versions (and some I didn't send) I was going back and forth between different ways to manage this. If I recall right, Laurent was (strongly?) of the opinion that blocking mode is not needed by the helpers. I'm kind of in the middle, I guess. It's easy to implement on top, though. Tomi
On Thu, Aug 18, 2022 at 06:21:51PM +0300, Tomi Valkeinen wrote: > On 18/08/2022 18:02, Jacopo Mondi wrote: > >> On 18/08/2022 17:29, Jacopo Mondi wrote: > >>> On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote: > >>>> Blocking wait can be easily implemented on top in Python, so rather than > >>>> supporting only blocking reads, or supporting both non-blocking and > >>>> blocking reads, let's support only non-blocking reads. > >>>> > >>> > >>> I'm not sure why it is better to ask application to do so on out > >>> behalf, having to expose the camera manager eventfd... > >> > >> We want to expose the fd anyway. Most, or at least all serious uses will > >> need the fd to wait for the events. That's how it is even before this > >> series. > >> > >> The point of this change is that instead of offering a function to get the > >> ready events which blocks if there are no events, we offer a function which > >> never blocks. > >> > >> With this change the users can either just call the function whenever to see > >> if there are events, or they can wait for the events with the fd. Previously > >> this was not possible. > > > > I would have expected a non-blocking "eventReady()" and a blocking > > implementation inside the python wrapper instead of having apps to > > instrument a selector. Maybe that's typical for Python ? > > There are, of course, many ways to implement this. This was discussed, > and if I recall right, in my previous versions (and some I didn't send) > I was going back and forth between different ways to manage this. If I > recall right, Laurent was (strongly?) of the opinion that blocking mode > is not needed by the helpers. I'm kind of in the middle, I guess. It's > easy to implement on top, though. That matches my recollection of the discussions :-)
Hi Tomi, Thank you for the patch. On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote: > Blocking wait can be easily implemented on top in Python, so rather than > supporting only blocking reads, or supporting both non-blocking and > blocking reads, let's support only non-blocking reads. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > src/py/examples/simple-cam.py | 5 +++-- > src/py/examples/simple-capture.py | 12 +++++++++-- > src/py/examples/simple-continuous-capture.py | 5 +++-- > src/py/libcamera/py_camera_manager.cpp | 22 +++++++++++++------- > src/py/libcamera/py_camera_manager.h | 2 +- > test/py/unittests.py | 7 +++++++ > 6 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py > index 2b81bb65..b3e97ca7 100755 > --- a/src/py/examples/simple-cam.py > +++ b/src/py/examples/simple-cam.py > @@ -19,8 +19,9 @@ TIMEOUT_SEC = 3 > > > def handle_camera_event(cm): > - # cm.get_ready_requests() will not block here, as we know there is an event > - # to read. > + # cm.get_ready_requests() returns the ready requests, which in our case > + # should almost always return a single Request, but in some cases there > + # could be multiple or none. > > reqs = cm.get_ready_requests() > > diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py > index a6a9b33e..5f93574f 100755 > --- a/src/py/examples/simple-capture.py > +++ b/src/py/examples/simple-capture.py > @@ -14,6 +14,7 @@ > > import argparse > import libcamera as libcam > +import selectors > import sys > > # Number of frames to capture > @@ -107,11 +108,18 @@ def main(): > # The main loop. Wait for the queued Requests to complete, process them, > # and re-queue them again. > > + sel = selectors.DefaultSelector() > + sel.register(cm.event_fd, selectors.EVENT_READ) > + > while frames_done < TOTAL_FRAMES: > - # cm.get_ready_requests() blocks until there is an event and returns > - # all the ready requests. Here we should almost always get a single > + # cm.get_ready_requests() does not block, so we use a Selector to wait > + # for a camera event. Here we should almost always get a single > # Request, but in some cases there could be multiple or none. > > + events = sel.select() > + if not events: > + continue > + > reqs = cm.get_ready_requests() > > for req in reqs: > diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py > index fe78a2dd..26a8060b 100755 > --- a/src/py/examples/simple-continuous-capture.py > +++ b/src/py/examples/simple-continuous-capture.py > @@ -88,8 +88,9 @@ class CaptureContext: > camera_contexts: list[CameraCaptureContext] = [] > > def handle_camera_event(self): > - # cm.get_ready_requests() will not block here, as we know there is an event > - # to read. > + # cm.get_ready_requests() returns the ready requests, which in our case > + # should almost always return a single Request, but in some cases there > + # could be multiple or none. > > reqs = self.cm.get_ready_requests() > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > index 5600f661..3dd8668e 100644 > --- a/src/py/libcamera/py_camera_manager.cpp > +++ b/src/py/libcamera/py_camera_manager.cpp > @@ -22,7 +22,7 @@ PyCameraManager::PyCameraManager() > > cameraManager_ = std::make_unique<CameraManager>(); > > - int fd = eventfd(0, EFD_CLOEXEC); > + int fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK); > if (fd == -1) > throw std::system_error(errno, std::generic_category(), > "Failed to create eventfd"); > @@ -60,18 +60,24 @@ py::list PyCameraManager::cameras() > > std::vector<py::object> PyCameraManager::getReadyRequests() > { > - readFd(); > + int ret = readFd(); > > - std::vector<py::object> ret; > + if (ret == EAGAIN) > + return std::vector<py::object>(); > + > + if (ret != 0) > + throw std::system_error(ret, std::generic_category()); > + > + std::vector<py::object> py_reqs; You could possibly name the variable py_reqs already in the patch that introduces this function, up to you. > > for (Request *request : getCompletedRequests()) { > py::object o = py::cast(request); > /* Decrease the ref increased in Camera.queue_request() */ > o.dec_ref(); > - ret.push_back(o); > + py_reqs.push_back(o); > } > > - return ret; > + return py_reqs; > } > > /* Note: Called from another thread */ > @@ -94,12 +100,14 @@ void PyCameraManager::writeFd() > LOG(Python, Fatal) << "Unable to write to eventfd"; > } > > -void PyCameraManager::readFd() > +int PyCameraManager::readFd() > { > uint8_t buf[8]; > > if (read(eventFd_.get(), buf, 8) != 8) > - throw std::system_error(errno, std::generic_category()); > + return errno; When no error occurs, there's no guarantee that read() will set errno to 0, so if the return value of read() is 0, errno will be undefined. I would also return a negative error code as we usually do (don't forget to update the caller). ret = read(eventFd_.get(), buf, 8); if (ret == 8) return 0; else if (ret < 0) return -errno; else return -E???; Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + return 0; > } > > void PyCameraManager::pushRequest(Request *req) > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > index 56bea13d..3525057d 100644 > --- a/src/py/libcamera/py_camera_manager.h > +++ b/src/py/libcamera/py_camera_manager.h > @@ -39,7 +39,7 @@ private: > LIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_); > > void writeFd(); > - void readFd(); > + int readFd(); > void pushRequest(Request *req); > std::vector<Request *> getCompletedRequests(); > }; > diff --git a/test/py/unittests.py b/test/py/unittests.py > index 9adc4337..6dea80fc 100755 > --- a/test/py/unittests.py > +++ b/test/py/unittests.py > @@ -207,9 +207,16 @@ class SimpleCaptureMethods(CameraTesterBase): > reqs = None > gc.collect() > > + sel = selectors.DefaultSelector() > + sel.register(cm.event_fd, selectors.EVENT_READ) > + > reqs = [] > > while True: > + events = sel.select() > + if not events: > + continue > + > ready_reqs = cm.get_ready_requests() > > reqs += ready_reqs
On 18/08/2022 23:07, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Fri, Jul 01, 2022 at 11:45:13AM +0300, Tomi Valkeinen wrote: >> Blocking wait can be easily implemented on top in Python, so rather than >> supporting only blocking reads, or supporting both non-blocking and >> blocking reads, let's support only non-blocking reads. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> src/py/examples/simple-cam.py | 5 +++-- >> src/py/examples/simple-capture.py | 12 +++++++++-- >> src/py/examples/simple-continuous-capture.py | 5 +++-- >> src/py/libcamera/py_camera_manager.cpp | 22 +++++++++++++------- >> src/py/libcamera/py_camera_manager.h | 2 +- >> test/py/unittests.py | 7 +++++++ >> 6 files changed, 39 insertions(+), 14 deletions(-) >> >> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py >> index 2b81bb65..b3e97ca7 100755 >> --- a/src/py/examples/simple-cam.py >> +++ b/src/py/examples/simple-cam.py >> @@ -19,8 +19,9 @@ TIMEOUT_SEC = 3 >> >> >> def handle_camera_event(cm): >> - # cm.get_ready_requests() will not block here, as we know there is an event >> - # to read. >> + # cm.get_ready_requests() returns the ready requests, which in our case >> + # should almost always return a single Request, but in some cases there >> + # could be multiple or none. >> >> reqs = cm.get_ready_requests() >> >> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py >> index a6a9b33e..5f93574f 100755 >> --- a/src/py/examples/simple-capture.py >> +++ b/src/py/examples/simple-capture.py >> @@ -14,6 +14,7 @@ >> >> import argparse >> import libcamera as libcam >> +import selectors >> import sys >> >> # Number of frames to capture >> @@ -107,11 +108,18 @@ def main(): >> # The main loop. Wait for the queued Requests to complete, process them, >> # and re-queue them again. >> >> + sel = selectors.DefaultSelector() >> + sel.register(cm.event_fd, selectors.EVENT_READ) >> + >> while frames_done < TOTAL_FRAMES: >> - # cm.get_ready_requests() blocks until there is an event and returns >> - # all the ready requests. Here we should almost always get a single >> + # cm.get_ready_requests() does not block, so we use a Selector to wait >> + # for a camera event. Here we should almost always get a single >> # Request, but in some cases there could be multiple or none. >> >> + events = sel.select() >> + if not events: >> + continue >> + >> reqs = cm.get_ready_requests() >> >> for req in reqs: >> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py >> index fe78a2dd..26a8060b 100755 >> --- a/src/py/examples/simple-continuous-capture.py >> +++ b/src/py/examples/simple-continuous-capture.py >> @@ -88,8 +88,9 @@ class CaptureContext: >> camera_contexts: list[CameraCaptureContext] = [] >> >> def handle_camera_event(self): >> - # cm.get_ready_requests() will not block here, as we know there is an event >> - # to read. >> + # cm.get_ready_requests() returns the ready requests, which in our case >> + # should almost always return a single Request, but in some cases there >> + # could be multiple or none. >> >> reqs = self.cm.get_ready_requests() >> >> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp >> index 5600f661..3dd8668e 100644 >> --- a/src/py/libcamera/py_camera_manager.cpp >> +++ b/src/py/libcamera/py_camera_manager.cpp >> @@ -22,7 +22,7 @@ PyCameraManager::PyCameraManager() >> >> cameraManager_ = std::make_unique<CameraManager>(); >> >> - int fd = eventfd(0, EFD_CLOEXEC); >> + int fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK); >> if (fd == -1) >> throw std::system_error(errno, std::generic_category(), >> "Failed to create eventfd"); >> @@ -60,18 +60,24 @@ py::list PyCameraManager::cameras() >> >> std::vector<py::object> PyCameraManager::getReadyRequests() >> { >> - readFd(); >> + int ret = readFd(); >> >> - std::vector<py::object> ret; >> + if (ret == EAGAIN) >> + return std::vector<py::object>(); >> + >> + if (ret != 0) >> + throw std::system_error(ret, std::generic_category()); >> + >> + std::vector<py::object> py_reqs; > > You could possibly name the variable py_reqs already in the patch that > introduces this function, up to you. Ok. >> >> for (Request *request : getCompletedRequests()) { >> py::object o = py::cast(request); >> /* Decrease the ref increased in Camera.queue_request() */ >> o.dec_ref(); >> - ret.push_back(o); >> + py_reqs.push_back(o); >> } >> >> - return ret; >> + return py_reqs; >> } >> >> /* Note: Called from another thread */ >> @@ -94,12 +100,14 @@ void PyCameraManager::writeFd() >> LOG(Python, Fatal) << "Unable to write to eventfd"; >> } >> >> -void PyCameraManager::readFd() >> +int PyCameraManager::readFd() >> { >> uint8_t buf[8]; >> >> if (read(eventFd_.get(), buf, 8) != 8) >> - throw std::system_error(errno, std::generic_category()); >> + return errno; > > When no error occurs, there's no guarantee that read() will set errno to > 0, so if the return value of read() is 0, errno will be undefined. I > would also return a negative error code as we usually do (don't forget > to update the caller). I think you're right. I read man eventfd so that read always returns 8 bytes or an error. And I guess that's correct, but I don't think it's strictly described anywhere, so better to check the return value more thoroughly. Tomi
diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py index 2b81bb65..b3e97ca7 100755 --- a/src/py/examples/simple-cam.py +++ b/src/py/examples/simple-cam.py @@ -19,8 +19,9 @@ TIMEOUT_SEC = 3 def handle_camera_event(cm): - # cm.get_ready_requests() will not block here, as we know there is an event - # to read. + # cm.get_ready_requests() returns the ready requests, which in our case + # should almost always return a single Request, but in some cases there + # could be multiple or none. reqs = cm.get_ready_requests() diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py index a6a9b33e..5f93574f 100755 --- a/src/py/examples/simple-capture.py +++ b/src/py/examples/simple-capture.py @@ -14,6 +14,7 @@ import argparse import libcamera as libcam +import selectors import sys # Number of frames to capture @@ -107,11 +108,18 @@ def main(): # The main loop. Wait for the queued Requests to complete, process them, # and re-queue them again. + sel = selectors.DefaultSelector() + sel.register(cm.event_fd, selectors.EVENT_READ) + while frames_done < TOTAL_FRAMES: - # cm.get_ready_requests() blocks until there is an event and returns - # all the ready requests. Here we should almost always get a single + # cm.get_ready_requests() does not block, so we use a Selector to wait + # for a camera event. Here we should almost always get a single # Request, but in some cases there could be multiple or none. + events = sel.select() + if not events: + continue + reqs = cm.get_ready_requests() for req in reqs: diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py index fe78a2dd..26a8060b 100755 --- a/src/py/examples/simple-continuous-capture.py +++ b/src/py/examples/simple-continuous-capture.py @@ -88,8 +88,9 @@ class CaptureContext: camera_contexts: list[CameraCaptureContext] = [] def handle_camera_event(self): - # cm.get_ready_requests() will not block here, as we know there is an event - # to read. + # cm.get_ready_requests() returns the ready requests, which in our case + # should almost always return a single Request, but in some cases there + # could be multiple or none. reqs = self.cm.get_ready_requests() diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp index 5600f661..3dd8668e 100644 --- a/src/py/libcamera/py_camera_manager.cpp +++ b/src/py/libcamera/py_camera_manager.cpp @@ -22,7 +22,7 @@ PyCameraManager::PyCameraManager() cameraManager_ = std::make_unique<CameraManager>(); - int fd = eventfd(0, EFD_CLOEXEC); + int fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK); if (fd == -1) throw std::system_error(errno, std::generic_category(), "Failed to create eventfd"); @@ -60,18 +60,24 @@ py::list PyCameraManager::cameras() std::vector<py::object> PyCameraManager::getReadyRequests() { - readFd(); + int ret = readFd(); - std::vector<py::object> ret; + if (ret == EAGAIN) + return std::vector<py::object>(); + + if (ret != 0) + throw std::system_error(ret, std::generic_category()); + + std::vector<py::object> py_reqs; for (Request *request : getCompletedRequests()) { py::object o = py::cast(request); /* Decrease the ref increased in Camera.queue_request() */ o.dec_ref(); - ret.push_back(o); + py_reqs.push_back(o); } - return ret; + return py_reqs; } /* Note: Called from another thread */ @@ -94,12 +100,14 @@ void PyCameraManager::writeFd() LOG(Python, Fatal) << "Unable to write to eventfd"; } -void PyCameraManager::readFd() +int PyCameraManager::readFd() { uint8_t buf[8]; if (read(eventFd_.get(), buf, 8) != 8) - throw std::system_error(errno, std::generic_category()); + return errno; + + return 0; } void PyCameraManager::pushRequest(Request *req) diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h index 56bea13d..3525057d 100644 --- a/src/py/libcamera/py_camera_manager.h +++ b/src/py/libcamera/py_camera_manager.h @@ -39,7 +39,7 @@ private: LIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_); void writeFd(); - void readFd(); + int readFd(); void pushRequest(Request *req); std::vector<Request *> getCompletedRequests(); }; diff --git a/test/py/unittests.py b/test/py/unittests.py index 9adc4337..6dea80fc 100755 --- a/test/py/unittests.py +++ b/test/py/unittests.py @@ -207,9 +207,16 @@ class SimpleCaptureMethods(CameraTesterBase): reqs = None gc.collect() + sel = selectors.DefaultSelector() + sel.register(cm.event_fd, selectors.EVENT_READ) + reqs = [] while True: + events = sel.select() + if not events: + continue + ready_reqs = cm.get_ready_requests() reqs += ready_reqs
Blocking wait can be easily implemented on top in Python, so rather than supporting only blocking reads, or supporting both non-blocking and blocking reads, let's support only non-blocking reads. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- src/py/examples/simple-cam.py | 5 +++-- src/py/examples/simple-capture.py | 12 +++++++++-- src/py/examples/simple-continuous-capture.py | 5 +++-- src/py/libcamera/py_camera_manager.cpp | 22 +++++++++++++------- src/py/libcamera/py_camera_manager.h | 2 +- test/py/unittests.py | 7 +++++++ 6 files changed, 39 insertions(+), 14 deletions(-)