[libcamera-devel] [PATCH v4 11/16] py: merge read_event() and get_ready_requests()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, May 30, 2022 at 05:27:17PM +0300, Tomi Valkeinen wrote:
> We always call CameraManager.read_event() and
> CameraManager.get_ready_requests(), so to simplify the use merge the
> read_event() into the get_ready_requests().
> 
> This has the side effect that get_ready_requests() will now block if
> there is no event ready. If we ever need to call get_ready_requests() in
> a polling manner we will need a new function which behaves differently.
> 
> However, afaics the only sensible way to manage the event loop is to use
> select/poll on the eventfd and then call get_ready_requests() once,
> which is the use case what the current merged function supports.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> ---
>  src/py/cam/cam.py            | 2 --
>  src/py/libcamera/py_main.cpp | 7 ++-----
>  test/py/unittests.py         | 4 ----
>  3 files changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
> index bf8529d9..2ae89fa8 100755
> --- a/src/py/cam/cam.py
> +++ b/src/py/cam/cam.py
> @@ -243,8 +243,6 @@ class CaptureState:
>      # Called from renderer when there is a libcamera event
>      def event_handler(self):
>          try:
> -            self.cm.read_event()
> -
>              reqs = self.cm.get_ready_requests()
>  
>              for req in reqs:
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index fcf009f0..505cc3dc 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)
>  			return gEventfd;
>  		})
>  
> -		.def("read_event", [](CameraManager &) {
> +		.def("get_ready_requests", [](CameraManager &) {
>  			uint8_t buf[8];
>  
> -			int ret = read(gEventfd, buf, 8);
> -			if (ret != 8)
> +			if (read(gEventfd, buf, 8) != 8)
>  				throw std::system_error(errno, std::generic_category());

We need a memory barrier here to prevent reordering. I'm quite sure
there's one somewhere due to the read() call and the lock below, but I
haven't been able to figure out the C++ rule that guarantees this. Does
anyone know ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> -		})
>  
> -		.def("get_ready_requests", [](CameraManager &) {
>  			std::vector<Request *> v;
>  
>  			{
> diff --git a/test/py/unittests.py b/test/py/unittests.py
> index 33b35a0a..9adc4337 100755
> --- a/test/py/unittests.py
> +++ b/test/py/unittests.py
> @@ -210,8 +210,6 @@ class SimpleCaptureMethods(CameraTesterBase):
>          reqs = []
>  
>          while True:
> -            cm.read_event()
> -
>              ready_reqs = cm.get_ready_requests()
>  
>              reqs += ready_reqs
> @@ -283,8 +281,6 @@ class SimpleCaptureMethods(CameraTesterBase):
>          while running:
>              events = sel.select()
>              for key, _ in events:
> -                cm.read_event()
> -
>                  ready_reqs = cm.get_ready_requests()
>  
>                  reqs += ready_reqs

-- 
Regards,

Laurent Pinchart