[libcamera-devel] [PATCH] libcamera: thread: Remove the unused setEventDispatcher() function

[Kieran Bingham]

Hi Laurent,

On 28/03/2021 22:38, Laurent Pinchart wrote:
> Custom event dispatchers for threads was an API meant to provide a way
> to integrate libcamera in the application's event loop. This isn't used
> anymore, as libcamera now creates internal threads. Drop the unused
> Thread::setEventDispatcher() function, and update the documentation
> accordingly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/internal/thread.h |  1 -
>  src/libcamera/thread.cpp            | 71 ++++++++++++-----------------
>  2 files changed, 29 insertions(+), 43 deletions(-)
> 
> diff --git a/include/libcamera/internal/thread.h b/include/libcamera/internal/thread.h
> index f6367a8f448b..25d0308d05b4 100644
> --- a/include/libcamera/internal/thread.h
> +++ b/include/libcamera/internal/thread.h
> @@ -46,7 +46,6 @@ public:
>  	static pid_t currentId();
>  
>  	EventDispatcher *eventDispatcher();
> -	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
>  
>  	void dispatchMessages(Message::Type type = Message::Type::None);
>  
> diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp
> index f339dab165b6..8261307628f8 100644
> --- a/src/libcamera/thread.cpp
> +++ b/src/libcamera/thread.cpp
> @@ -28,6 +28,13 @@
>   * interactions with application threads. Careful compliance with the threading
>   * model will ensure avoidance of race conditions.
>   *
> + * Every thread created by libcamera is associated with an instance of the
> + * Thread class. Those threads run by default an internal event loop to

"Those threads run an internal event loop by default " ...

> + * dispatch events to objects. Additionally, the main thread of the application
> + * (defined as the thread that calls CameraManager::start()) is also associated
> + * with a Thread instance, but has no event loop accessible to libcamera. Other
> + * application threads are not visible to libcamera.
> + *
>   * \section thread-objects Threads and Objects
>   *
>   * Instances of the Object class and all its derived classes are thread-aware
> @@ -39,13 +46,12 @@
>   * explicitly connected with ConnectionTypeDirect, will also be delivered from
>   * the object thread's event loop.
>   *
> - * All Object instances created by libcamera are bound to an internal thread,
> - * and applications don't need to provide an event loop to support them. Object
> - * instances created by applications require an event loop. It is the
> - * responsibility of applications to provide that event loop, either explicitly
> - * through CameraManager::setEventDispatcher(), or by running the default event
> - * loop provided by CameraManager::eventDispatcher() in their main thread. The
> - * main thread of an application is the one that calls CameraManager::start().
> + * All Object instances created internally by libcamera are bound to internal
> + * threads. As objects interact with thread event loops for proper operation,
> + * creating an Object instance in a thread that has no internal event loop (such
> + * as the main application thread, or libcamera threads that have a custom main
> + * loop), prevents some features of the Object class from being used. See
> + * Thread::exec() for more details.

This sounds like a potential for bugs.

Do those features have assertions or checks required to ensure that if
they are used in an incorrect context it will be trapped?

>   *
>   * \section thread-signals Threads and Signals
>   *
> @@ -220,9 +226,9 @@ ThreadData *ThreadData::current()
>   * with the Object, Signal and EventDispatcher classes.
>   *
>   * Thread instances by default run an event loop until the exit() method is
> - * called. A custom event dispatcher may be installed with
> - * setEventDispatcher(), otherwise a poll-based event dispatcher is used. This
> - * behaviour can be overriden by overloading the run() method.

s/overriden/overridden/

> + * called. The event loop dispatches events (messages, notifiers and timers)
> + * sent to the objects living in the thread. This behaviour can be overriden by

s/overriden/overridden/

> + * overloading the run() function.
>   *
>   * \context This class is \threadsafe.
>   */
> @@ -317,9 +323,17 @@ int Thread::exec()
>   * \brief Main method of the thread
>   *
>   * When the thread is started with start(), it calls this method in the context
> - * of the new thread. The run() method can be overloaded to perform custom
> - * work. When this method returns the thread execution is stopped, and the \ref
> - * finished signal is emitted.
> + * of the new thread. The run() method can be overridden to perform custom
> + * work, either custom initialization and cleanup before and after calling the
> + * Thread::exec() function, or a custom thread loop altogether. When this
> + * method returns the thread execution is stopped, and the \ref finished signal
> + * is emitted.
> + *
> + * Note that if this function is overriden and doesn't call Thread::exec(), no

s/overriden/overridden/


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> + * events will be dispatched to the objects living in the thread. These objects
> + * will not be able to use the EventNotifier, Timer or Message facilities. This
> + * includes functions that rely on message dispatching, such as
> + * Object::deleteLater().
>   *
>   * The base implementation just calls exec().
>   */
> @@ -435,38 +449,11 @@ pid_t Thread::currentId()
>  	return data->tid_;
>  }
>  
> -/**
> - * \brief Set the event dispatcher
> - * \param[in] dispatcher Pointer to the event dispatcher
> - *
> - * Threads that run an event loop require an event dispatcher to integrate
> - * event notification and timers with the loop. Users that want to provide
> - * their own event dispatcher shall call this method once and only once before
> - * the thread is started with start(). If no event dispatcher is provided, a
> - * default poll-based implementation will be used.
> - *
> - * The Thread takes ownership of the event dispatcher and will delete it when
> - * the thread is destroyed.
> - */
> -void Thread::setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher)
> -{
> -	if (data_->dispatcher_.load(std::memory_order_relaxed)) {
> -		LOG(Thread, Warning) << "Event dispatcher is already set";
> -		return;
> -	}
> -
> -	data_->dispatcher_.store(dispatcher.release(),
> -				 std::memory_order_relaxed);
> -}
> -
>  /**
>   * \brief Retrieve the event dispatcher
>   *
> - * This method retrieves the event dispatcher set with setEventDispatcher().
> - * If no dispatcher has been set, a default poll-based implementation is created
> - * and returned, and no custom event dispatcher may be installed anymore.
> - *
> - * The returned event dispatcher is valid until the thread is destroyed.
> + * This function retrieves the internal event dispatcher for the thread. The
> + * returned event dispatcher is valid until the thread is destroyed.
>   *
>   * \return Pointer to the event dispatcher
>   */
> 

-- 
Regards
--
Kieran