[libcamera-devel] [PATCH v3 09/17] py: Switch to non-blocking eventfd

[Tomi Valkeinen]

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 at 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