[libcamera-devel] [PATCH v5 2/2] cam: event_loop: Add timed events to event loop

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Thu, Mar 31, 2022 at 02:06:10PM +0100, Eric Curtin via libcamera-devel wrote:
> If you add an event with EventLoop::Default, that event will be executed
> every 10ms or at a rate of 100fps. Used to handle resize of windows and
> quit events in SDL.

Could you move this patch first in the series ? It's best to first add
the infrastructure we need, and then make use of it in the SDL sink.
This patch will thus not touch sdl_sink.cpp.

The commit message could then be updated to something like the
following:

Extend the EventLoop class to support periodic timer events. This can be
used to run tasks periodically, such as handling the event loop of SDL.

> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> ---
>  src/cam/event_loop.cpp | 10 +++++++++-
>  src/cam/event_loop.h   |  1 +
>  src/cam/sdl_sink.cpp   |  2 +-
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index e25784c0..7a5ed4da 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -75,7 +75,15 @@ void EventLoop::addEvent(int fd, EventType type,
>  		return;
>  	}
>  
> -	int ret = event_add(event->event_, nullptr);
> +	struct timeval *tp = nullptr;
> +	struct timeval tv;
> +	if (events == EV_PERSIST) { /* EventType = Default */
> +		tp = &tv;
> +		tv.tv_sec = 0;
> +		tv.tv_usec = 10000; /* every 10 ms */
> +	}
> +
> +	int ret = event_add(event->event_, tp);
>  	if (ret < 0) {
>  		std::cerr << "Failed to add event for fd " << fd << std::endl;
>  		return;
> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> index a4613eb2..8fd9bd20 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -20,6 +20,7 @@ class EventLoop
>  {
>  public:
>  	enum EventType {
> +		Default = 0, /* the event can be triggered only by a timeout or by manual activation */
>  		Read = 1,
>  		Write = 2,
>  	};

I think it would be nicer to add a

	void addTimerEvent(std::chrono::microseconds period);

function instead of adding a new event type. This will allow selecting
the timer period in the caller instead of hardcoding it here, and will
avoid having to pass a random fd value to the function.

> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> index 28e59cbb..97a550b5 100644
> --- a/src/cam/sdl_sink.cpp
> +++ b/src/cam/sdl_sink.cpp
> @@ -117,7 +117,7 @@ int SDLSink::start()
>  		return -EINVAL;
>  	}
>  
> -	EventLoop::instance()->addEvent(-1, EventLoop::Read, std::bind(&SDLSink::processSDLEvents, this));
> +	EventLoop::instance()->addEvent(-1, EventLoop::Default, std::bind(&SDLSink::processSDLEvents, this));
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart