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

[Laurent Pinchart]

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.

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

-- 
Regards,

Laurent Pinchart