[libcamera-devel] [PATCH] libcamera: base: thread: Prevent messages to thread without an event loop

[Umang Jain]

Hi Laurent,

Thank you for the explanation.

Need few clarifications below

On 6/28/22 3:16 PM, Laurent Pinchart via libcamera-devel wrote:
> On Tue, Jun 28, 2022 at 11:09:21AM +0300, Tomi Valkeinen wrote:
>> On 27/06/2022 19:46, Laurent Pinchart wrote:
>>> A message posted to an object bound to a thread without an event loop
>>> will never get delivered. This indicates a bug in the caller, and
>>> materializes in ways that can be hard to debug, such as a signal never
>>> being delivered. Add an assertion in Thread::postMessage() to catch this
>>> at the source.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>    src/libcamera/base/thread.cpp | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
>>> index 6bda9d1462f5..6ead1d7c12d0 100644
>>> --- a/src/libcamera/base/thread.cpp
>>> +++ b/src/libcamera/base/thread.cpp
>>> @@ -530,6 +530,7 @@ void Thread::postMessage(std::unique_ptr<Message> msg, Object *receiver)
>>>    	msg->receiver_ = receiver;
>>>    
>>>    	ASSERT(data_ == receiver->thread()->data_);
>>> +	ASSERT(data_ != mainThread.data_);

Simply reading the commit message and inferring, the first change I 
expected was on the lines;
         ASSERT (data_->dispatcher_ != nullptr)

or for readability,

   	ASSERT(receiver->thread()->data_->dispatcher_ != nullptr);


that is, checking the receiver's thread event loop exists or not.
>>>    
>>>    	MutexLocker locker(data_->messages_.mutex_);
>>>    	data_->messages_.list_.push_back(std::move(msg));
>>>
>>> base-commit: 27cc0a6b58bcca32071cb6ab96e5ee79c75031e5
>> So what was the issue with the code I had? The problem is that
>> CameraManager is a libcamera Object, and was associated with the main
>> thread...? And a PyCameraManager which doesn't inherit CameraManager is
>> not an Object and not associated with any thread, so it works?
> That's right. Because PyCameraManager inherited from Object, it
> automatically got support for queued signal delivery, where a signal
> emitted in one thread would be processed in a different thread. This
> mechanism requires the receiving object to be bound to a thread that has
> an event loop managed by libcamera. By default an Object is bound to the
> thread it's created in.
>
> The CameraManager is created in the Python main thread, and the signal
> is emitted from an internal libcamera thread. The signal will thus be
> queued (the default connection type is Auto, which means that signals
> are delivered directly if the receiver object is bound to the thread
> that the signal is emitted from, or queued otherwise), which queues a
> message for the receiver's thread event loop. As the receiver thread is
> the Python main thread (the application thread from a libcamera point of
> view), which doesn't have a libcamera event loop, the message is never
> processed.

I understand the explanation above.

However I'm a bit lost with mainThread static variable. Am I right to 
infer that the mainThread is the Python's main thread in this case? If 
yes, the patch is correct, refraining the messages to be posted to a 
object whose threads has no event loop.

However, how about if the mainThread inherits from libcamera (hence 
Object, Thread, Signal delivery is in place) but tries to send a message 
to another Object bound to itself (i.e. mainThread)
Won't the assertion in this patch fail ?

> If the receiver doesn't inherit from the Object class, then the signal
> will be delivered directly, in the emitter's thread. You can achieve the
> same result with an Object receiver by specifying the direct connection
> type when connecting the signal.
>