[{"id":2213,"web_url":"https://patchwork.libcamera.org/comment/2213/","msgid":"<20190711053949.GJ1557@wyvern>","date":"2019-07-11T05:39:49","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: signal: Support\n\tcross-thread signals","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your patch.\n\nOn 2019-07-10 22:17:05 +0300, Laurent Pinchart wrote:\n> Allow signals to cross thread boundaries by posting them to the\n> recipient through messages instead of calling the slot directly when the\n> recipient lives in a different thread.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/signal.h | 70 +++++++++++++++++++++++++++++----\n> src/libcamera/include/message.h | 14 +++++++\n> src/libcamera/message.cpp | 22 +++++++++++\n> src/libcamera/object.cpp | 15 +++++++\n> src/libcamera/signal.cpp | 26 ++++++++++++\n> 5 files changed, 139 insertions(+), 8 deletions(-)\n> \n> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\n> index c8f3243eaf5a..04e8fe7eebe3 100644\n> --- a/include/libcamera/signal.h\n> +++ b/include/libcamera/signal.h\n> @@ -8,6 +8,8 @@\n> #define __LIBCAMERA_SIGNAL_H__\n> \n> #include \n> +#include \n> +#include \n> #include \n> \n> #include \n> @@ -16,6 +18,9 @@ namespace libcamera {\n> \n> template\n> class Signal;\n> +class SlotBase;\n> +template\n> +class SlotArgs;\n\nThese forward declarations don't seems to be needed right?\n\nWith this addressed,\n\nReviewed-by: Niklas Söderlund \n\n> \n> class SlotBase\n> {\n> @@ -27,6 +32,9 @@ public:\n> \tvoid *obj() { return obj_; }\n> \tbool isObject() const { return isObject_; }\n> \n> +\tvoid activatePack(void *pack);\n> +\tvirtual void invokePack(void *pack) = 0;\n> +\n> protected:\n> \tvoid *obj_;\n> \tbool isObject_;\n> @@ -35,24 +43,70 @@ protected:\n> template\n> class SlotArgs : public SlotBase\n> {\n> +private:\n> +#ifndef __DOXYGEN__\n> +\t/*\n> +\t * This is a cheap partial implementation of std::integer_sequence<>\n> +\t * from C++14.\n> +\t */\n> +\ttemplate\n> +\tstruct sequence {\n> +\t};\n> +\n> +\ttemplate\n> +\tstruct generator : generator {\n> +\t};\n> +\n> +\ttemplate\n> +\tstruct generator<0, S...> {\n> +\t\ttypedef sequence type;\n> +\t};\n> +#endif\n> +\n> public:\n> +\tusing PackType = std::tuple::type...>;\n> +\n> \tSlotArgs(void *obj, bool isObject)\n> \t\t: SlotBase(obj, isObject) {}\n> \n> +\tvoid invokePack(void *pack) override\n> +\t{\n> +\t\tinvokePack(pack, typename generator::type());\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid invokePack(void *pack, sequence)\n> +\t{\n> +\t\tPackType *args = static_cast(pack);\n> +\t\tinvoke(std::get(*args)...);\n> +\t\tdelete args;\n> +\t}\n> +\n> +\tvirtual void activate(Args... args) = 0;\n> \tvirtual void invoke(Args... args) = 0;\n> -\n> -protected:\n> -\tfriend class Signal;\n> };\n> \n> template\n> class SlotMember : public SlotArgs\n> {\n> public:\n> +\tusing PackType = std::tuple::type...>;\n> +\n> \tSlotMember(T *obj, bool isObject, void (T::*func)(Args...))\n> \t\t: SlotArgs(obj, isObject), func_(func) {}\n> \n> -\tvoid invoke(Args... args) { (static_cast(this->obj_)->*func_)(args...); }\n> +\tvoid activate(Args... args)\n> +\t{\n> +\t\tif (this->isObject_)\n> +\t\t\tSlotBase::activatePack(new PackType{ args... });\n> +\t\telse\n> +\t\t\t(static_cast(this->obj_)->*func_)(args...);\n> +\t}\n> +\n> +\tvoid invoke(Args... args)\n> +\t{\n> +\t\t(static_cast(this->obj_)->*func_)(args...);\n> +\t}\n> \n> private:\n> \tfriend class Signal;\n> @@ -66,7 +120,8 @@ public:\n> \tSlotStatic(void (*func)(Args...))\n> \t\t: SlotArgs(nullptr, false), func_(func) {}\n> \n> -\tvoid invoke(Args... args) { (*func_)(args...); }\n> +\tvoid activate(Args... args) { (*func_)(args...); }\n> +\tvoid invoke(Args... args) {}\n> \n> private:\n> \tfriend class Signal;\n> @@ -186,9 +241,8 @@ public:\n> \t\t * disconnect operation, invalidating the iterator.\n> \t\t */\n> \t\tstd::vector slots{ slots_.begin(), slots_.end() };\n> -\t\tfor (SlotBase *slot : slots) {\n> -\t\t\tstatic_cast *>(slot)->invoke(args...);\n> -\t\t}\n> +\t\tfor (SlotBase *slot : slots)\n> +\t\t\tstatic_cast *>(slot)->activate(args...);\n> \t}\n> };\n> \n> diff --git a/src/libcamera/include/message.h b/src/libcamera/include/message.h\n> index 97c9b80ec0e0..db17d647c280 100644\n> --- a/src/libcamera/include/message.h\n> +++ b/src/libcamera/include/message.h\n> @@ -10,6 +10,7 @@\n> namespace libcamera {\n> \n> class Object;\n> +class SlotBase;\n> class Thread;\n> \n> class Message\n> @@ -17,6 +18,7 @@ class Message\n> public:\n> \tenum Type {\n> \t\tNone = 0,\n> +\t\tSignalMessage = 1,\n> \t};\n> \n> \tMessage(Type type);\n> @@ -32,6 +34,18 @@ private:\n> \tObject *receiver_;\n> };\n> \n> +class SignalMessage : public Message\n> +{\n> +public:\n> +\tSignalMessage(SlotBase *slot, void *pack)\n> +\t\t: Message(Message::SignalMessage), slot_(slot), pack_(pack)\n> +\t{\n> +\t}\n> +\n> +\tSlotBase *slot_;\n> +\tvoid *pack_;\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_MESSAGE_H__ */\n> diff --git a/src/libcamera/message.cpp b/src/libcamera/message.cpp\n> index 47caf44dc82d..66dd1e8bd618 100644\n> --- a/src/libcamera/message.cpp\n> +++ b/src/libcamera/message.cpp\n> @@ -68,4 +68,26 @@ Message::~Message()\n> * \\return The message receiver\n> */\n> \n> +/**\n> + * \\class SignalMessage\n> + * \\brief A message carrying a Signal across threads\n> + */\n> +\n> +/**\n> + * \\fn SignalMessage::SignalMessage()\n> + * \\brief Construct a SignalMessage\n> + * \\param[in] slot The slot that the signal targets\n> + * \\param[in] pack The signal arguments\n> + */\n> +\n> +/**\n> + * \\var SignalMessage::slot_\n> + * \\brief The slot that the signal targets\n> + */\n> +\n> +/**\n> + * \\var SignalMessage::pack_\n> + * \\brief The signal arguments\n> + */\n> +\n> }; /* namespace libcamera */\n> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> index 695e6c11b3a4..1dfa159267fe 100644\n> --- a/src/libcamera/object.cpp\n> +++ b/src/libcamera/object.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> \n> #include \"log.h\"\n> +#include \"message.h\"\n> #include \"thread.h\"\n> \n> /**\n> @@ -32,6 +33,10 @@ namespace libcamera {\n> * This allows implementing easy message passing between threads by inheriting\n> * from the Object class.\n> *\n> + * Object slots connected to signals will also run in the context of the\n> + * object's thread, regardless of whether the signal is emitted in the same or\n> + * in another thread.\n> + *\n> * \\sa Message, Signal, Thread\n> */\n> \n> @@ -82,6 +87,16 @@ void Object::postMessage(std::unique_ptr msg)\n> */\n> void Object::message(Message *msg)\n> {\n> +\tswitch (msg->type()) {\n> +\tcase Message::SignalMessage: {\n> +\t\tSignalMessage *smsg = static_cast(msg);\n> +\t\tsmsg->slot_->invokePack(smsg->pack_);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tdefault:\n> +\t\tbreak;\n> +\t}\n> }\n> \n> /**\n> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\n> index 4cb85ecb0686..53c18535fee3 100644\n> --- a/src/libcamera/signal.cpp\n> +++ b/src/libcamera/signal.cpp\n> @@ -7,6 +7,10 @@\n> \n> #include \n> \n> +#include \"message.h\"\n> +#include \"thread.h\"\n> +#include \"utils.h\"\n> +\n> /**\n> * \\file signal.h\n> * \\brief Signal & slot implementation\n> @@ -42,8 +46,30 @@ namespace libcamera {\n> * to the same slot. Duplicate connections between a signal and a slot are\n> * allowed and result in the slot being called multiple times for the same\n> * signal emission.\n> + *\n> + * When a slot belongs to an instance of the Object class, the slot is called\n> + * in the context of the thread that the object is bound to. If the signal is\n> + * emitted from the same thread, the slot will be called synchronously, before\n> + * Signal::emit() returns. If the signal is emitted from a different thread,\n> + * the slot will be called asynchronously from the object's thread's event\n> + * loop, after the Signal::emit() method returns, with a copy of the signal's\n> + * arguments. The emitter shall thus ensure that any pointer or reference\n> + * passed through the signal will remain valid after the signal is emitted.\n> */\n> \n> +void SlotBase::activatePack(void *pack)\n> +{\n> +\tObject *obj = static_cast(obj_);\n> +\n> +\tif (Thread::current() == obj->thread()) {\n> +\t\tinvokePack(pack);\n> +\t} else {\n> +\t\tstd::unique_ptr msg =\n> +\t\t\tutils::make_unique(this, pack);\n> +\t\tobj->postMessage(std::move(msg));\n> +\t}\n> +}\n> +\n> /**\n> * \\fn Signal::connect(T *object, void(T::*func)(Args...))\n> * \\brief Connect the signal to a member function slot\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-pg1-x544.google.com (mail-pg1-x544.google.com\n\t[IPv6:2607:f8b0:4864:20::544])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CFD4F60C23\n\tfor ;\n\tThu, 11 Jul 2019 07:39:54 +0200 (CEST)","by mail-pg1-x544.google.com with SMTP id o13so2335551pgp.12\n\tfor ;\n\tWed, 10 Jul 2019 22:39:54 -0700 (PDT)","from localhost (softbank126163157105.bbtec.net. [126.163.157.105])\n\tby smtp.gmail.com with ESMTPSA id\n\t201sm5522574pfz.24.2019.07.10.22.39.51\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tWed, 10 Jul 2019 22:39:52 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=yqwV7YLgUr/OgdOhM3u/YFMsPvvoNAAq3glr4S+RbWc=;\n\tb=ANmdEUcqzVG0T+8paFZGurmbmWxTgi8BH74rFOowkYXhaHONXaDpHzmsnDDMZC/zBf\n\tMMQx2/O8wbp2/d6d9ymaIKvTLPl10nd0d/1SKBZfYlORkwyf2k3mWE/g5yp/zlJHvBkW\n\tgOeOF+Yjo16meJSBv95jAS1zaSVxb0QRaDLfsvuxz9+R2I5deVhznVKMmSYDCoaK3Jv8\n\t6xlkNlfIKQfgT1lG3bbEE5qovu2dQUoBIdDUaNlUMvIJ42lZrxDc+hMtet5ad/rB8D0d\n\t/kfqD0/SvISuANCcB7esTPk6A7SNUgBDhcdJvca8OrNpz0tB9v7qRISnvmAMQFVEi+3a\n\tgH6w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=yqwV7YLgUr/OgdOhM3u/YFMsPvvoNAAq3glr4S+RbWc=;\n\tb=Rl4VBHFz1MGpJx9OblrpSpjpl+Alwe70jPDSO4ne2I46W5FuUrO4nPFUFZsEiiESZR\n\thpd9y7QBCtTOmExcIiEnvwExANSUb0P/4mDV8dYm2gY2AknMkrT4q8WSnNWo9lf1S9QB\n\t1dhoVUpESTvuULSN6sNLHqrhd8d7nheBVfKVwEXp1wy8h84faqjDLAtRKbo/KmpSN+EY\n\tOe70xDxNCJ8KZd+x7C2VoWGdNO59U5EjqL66vAHN8dx+xoK03p1lcUuUR4tDeKWbn2kR\n\tYmIRclbpo07q4rukoAHb6inoTkWlGtku2HpBXioSdYd03YO8vfQx7qMg6KFYhYo0K4Q8\n\tmHPA==","X-Gm-Message-State":"APjAAAWOZ9YjAcq8RHUDgkpeRjn19QCOPja2+UEBU3pwYYn7fpuSVnsy\n\tfQj+Xc/ArzMmFl+2LFuhC6pJfYfZ/Gw=","X-Google-Smtp-Source":"APXvYqzkE7jZbY3bPr8xP6PyV8rbNXQhoX1gQXKWgFRjEP87pw8Ah1rHZ9ST2mDkPaXhJCsmrlHXVA==","X-Received":"by 2002:a17:90a:fe5:: with SMTP id\n\t92mr2722660pjz.35.1562823593445; \n\tWed, 10 Jul 2019 22:39:53 -0700 (PDT)","Date":"Thu, 11 Jul 2019 14:39:49 +0900","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190711053949.GJ1557@wyvern>","References":"<20190710191708.13049-1-laurent.pinchart@ideasonboard.com>\n\t<20190710191708.13049-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190710191708.13049-3-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: signal: Support\n\tcross-thread signals","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 11 Jul 2019 05:39:55 -0000"}}]