[{"id":2448,"web_url":"https://patchwork.libcamera.org/comment/2448/","msgid":"<20190817153837.GB21683@wyvern>","date":"2019-08-17T15:38:37","subject":"Re: [libcamera-devel] [PATCH v2 13/18] test: Add EventNotifier\n\tthread move test","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 work.\n\nOn 2019-08-17 18:20:59 +0300, Laurent Pinchart wrote:\n> The test verifies correct behaviour of an enabled event notifier moved\n> to a different thread.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Jacopo Mondi \n> ---\n> test/event-thread.cpp | 112 ++++++++++++++++++++++++++++++++++++++++++\n> test/meson.build | 1 +\n> 2 files changed, 113 insertions(+)\n> create mode 100644 test/event-thread.cpp\n> \n> diff --git a/test/event-thread.cpp b/test/event-thread.cpp\n> new file mode 100644\n> index 000000000000..4a82d49b94f1\n> --- /dev/null\n> +++ b/test/event-thread.cpp\n> @@ -0,0 +1,112 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * event-thread.cpp - Threaded event test\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"test.h\"\n> +#include \"thread.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class EventHandler : public Object\n> +{\n> +public:\n> +\tEventHandler()\n> +\t{\n\nI think you should set notified_ to false here.\n\n> +\t\tpipe(pipefd_);\n> +\n> +\t\tnotifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read);\n> +\t\tnotifier_->activated.connect(this, &EventHandler::readReady);\n> +\t}\n> +\n> +\t~EventHandler()\n> +\t{\n> +\t\tdelete notifier_;\n> +\n> +\t\tclose(pipefd_[0]);\n> +\t\tclose(pipefd_[1]);\n> +\t}\n> +\n> +\tint notify()\n> +\t{\n> +\t\tstd::string data(\"H2G2\");\n> +\t\tssize_t ret;\n> +\n> +\t\tmemset(data_, 0, sizeof(data_));\n> +\t\tsize_ = 0;\n> +\n> +\t\tret = write(pipefd_[1], data.data(), data.size());\n> +\t\tif (ret < 0) {\n> +\t\t\tcout << \"Pipe write failed\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tbool notified() const\n> +\t{\n> +\t\treturn notified_;\n> +\t}\n> +\n> +\tvoid moveToThread(Thread *thread)\n> +\t{\n> +\t\tObject::moveToThread(thread);\n> +\t\tnotifier_->moveToThread(thread);\n> +\t}\n> +\n> +private:\n> +\tvoid readReady(EventNotifier *notifier)\n> +\t{\n> +\t\tsize_ = read(notifier->fd(), data_, sizeof(data_));\n> +\t\tnotified_ = true;\n> +\t}\n> +\n> +\tEventNotifier *notifier_;\n> +\n> +\tint pipefd_[2];\n> +\n> +\tbool notified_;\n> +\tchar data_[16];\n> +\tssize_t size_;\n> +};\n> +\n> +class EventThreadTest : public Test\n> +{\n> +protected:\n> +\tint run()\n> +\t{\n> +\t\tThread thread;\n> +\t\tthread.start();\n> +\n> +\t\tEventHandler handler;\n> +\t\thandler.notify();\n> +\t\thandler.moveToThread(&thread);\n\nI had to think twice when reading this. Maybe add comment to clarify \nthat it's OK to generate the event first and then move it as the main \nthread don't process events it's only possible for the event to be \nprocessed in a different thread.\n\nOr maybe I just have to get used to this API to make it apparent that \nthis is the only possibility.\n\nWith the initialization of notified_ fixed and with or without a comment,\n\nReviewed-by: Niklas Söderlund \n\n> +\n> +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n> +\n> +\t\t/* Must stop thread before destroying the handler. */\n> +\t\tthread.exit(0);\n> +\t\tthread.wait();\n> +\n> +\t\tif (!handler.notified()) {\n> +\t\t\tcout << \"Thread event handling test failed\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(EventThreadTest)\n> diff --git a/test/meson.build b/test/meson.build\n> index c6601813db78..f695ffd7be44 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -23,6 +23,7 @@ public_tests = [\n> \n> internal_tests = [\n> ['camera-sensor', 'camera-sensor.cpp'],\n> + ['event-thread', 'event-thread.cpp'],\n> ['message', 'message.cpp'],\n> ['object', 'object.cpp'],\n> ['object-invoke', 'object-invoke.cpp'],\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-ed1-x541.google.com (mail-ed1-x541.google.com\n\t[IPv6:2a00:1450:4864:20::541])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C3983600F9\n\tfor ;\n\tSat, 17 Aug 2019 17:38:40 +0200 (CEST)","by mail-ed1-x541.google.com with SMTP id w20so7601121edd.2\n\tfor ;\n\tSat, 17 Aug 2019 08:38:40 -0700 (PDT)","from localhost ([185.224.57.161]) by smtp.gmail.com with ESMTPSA id\n\tay8sm1252847ejb.4.2019.08.17.08.38.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 17 Aug 2019 08:38:39 -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=qU481uv/hZIDUNWC80KD/hM/eCnCodWW8L7Vpmo9h3E=;\n\tb=q8Q9uhQnezqmZ/OHVFeyoTcvvwFj9gvvze+9ZAmQSzRCf2JMH5qeszoAdZby+7PDHV\n\tXrzZb2wM99DsUOssioBtUROFqUMvQugBNAv4EPPeJwK5tivEbfvyKraUuPTAJnlH5FKU\n\t2iv1gneqpV34fwArTb6aGC8X6OSvD4oiMvj+cy8yXw+BqEBl9ZEUIjcLFKPn2xutGr/l\n\tuKRZqaRrzlsKkpmVq2kkfiXrEUwg31Tt7jKsd1a1zqsDCbp22h0bBT0P3QblrIFja1Om\n\tjJeqrUjwNsrFaEz2NVa5XIEso/PpSHtePv8HmT9oGEyv5NamomWJpginzpZtryeImkHU\n\tB+CA==","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=qU481uv/hZIDUNWC80KD/hM/eCnCodWW8L7Vpmo9h3E=;\n\tb=Wb7sP1q+Mx+Uk8bF5lodzqzqsJ3v1JCpYo1UPSdBz45Bepf6jkSzOJlqgzHB/F8Ig5\n\tP30INbeT2TkxgT16ZlRbQMD7c/hOKOSjyzSFjWMFatPIUnQg2bl8v095mqDLhLuTtPTn\n\tME60xbkXglFUpNtQFeVKf97U3AISkQjQ5tGLkCnzRARArIjPCpFMwZBQwbMzsGB+fzQ9\n\tdHK11AGZv5PX/R8si3QJN+X3q6ACso9OnX37ks7hFOdvxbjmoL+YCASQvC2lI0xaBGLr\n\toGZ4ak+5b/jO61GCTBcjxCDLnpDme8soSmb5jn8Jb9Uruw7VAK/k2+0VsP7toDBT998H\n\t8ojA==","X-Gm-Message-State":"APjAAAUJmqoh6KcxHhx64he5aykKb7akBhaeIM0WCbhGvnwgC5JjtRNe\n\tiEXgB8PShJsyBSNrSxnMOQFgoyO7P4U=","X-Google-Smtp-Source":"APXvYqw/XKwcLNNun7+/RbPmUa6DQAfg1Dof3g4d3qXaMKsZ12kT1+VZoI8eh1zkGOQmIhXeovFv4g==","X-Received":"by 2002:aa7:c605:: with SMTP id h5mr14869006edq.8.1566056320437; \n\tSat, 17 Aug 2019 08:38:40 -0700 (PDT)","Date":"Sat, 17 Aug 2019 17:38:37 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190817153837.GB21683@wyvern>","References":"<20190817152104.10834-1-laurent.pinchart@ideasonboard.com>\n\t<20190817152104.10834-14-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":"<20190817152104.10834-14-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 13/18] test: Add EventNotifier\n\tthread move test","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":"Sat, 17 Aug 2019 15:38:40 -0000"}},{"id":2452,"web_url":"https://patchwork.libcamera.org/comment/2452/","msgid":"<20190817154648.GC15630@pendragon.ideasonboard.com>","date":"2019-08-17T15:46:48","subject":"Re: [libcamera-devel] [PATCH v2 13/18] test: Add EventNotifier\n\tthread move test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sat, Aug 17, 2019 at 05:38:37PM +0200, Niklas Söderlund wrote:\n> On 2019-08-17 18:20:59 +0300, Laurent Pinchart wrote:\n> > The test verifies correct behaviour of an enabled event notifier moved\n> > to a different thread.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > test/event-thread.cpp | 112 ++++++++++++++++++++++++++++++++++++++++++\n> > test/meson.build | 1 +\n> > 2 files changed, 113 insertions(+)\n> > create mode 100644 test/event-thread.cpp\n> > \n> > diff --git a/test/event-thread.cpp b/test/event-thread.cpp\n> > new file mode 100644\n> > index 000000000000..4a82d49b94f1\n> > --- /dev/null\n> > +++ b/test/event-thread.cpp\n> > @@ -0,0 +1,112 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * event-thread.cpp - Threaded event test\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"test.h\"\n> > +#include \"thread.h\"\n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n> > +\n> > +class EventHandler : public Object\n> > +{\n> > +public:\n> > +\tEventHandler()\n> > +\t{\n> \n> I think you should set notified_ to false here.\n\nGood point.\n\n> > +\t\tpipe(pipefd_);\n> > +\n> > +\t\tnotifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read);\n> > +\t\tnotifier_->activated.connect(this, &EventHandler::readReady);\n> > +\t}\n> > +\n> > +\t~EventHandler()\n> > +\t{\n> > +\t\tdelete notifier_;\n> > +\n> > +\t\tclose(pipefd_[0]);\n> > +\t\tclose(pipefd_[1]);\n> > +\t}\n> > +\n> > +\tint notify()\n> > +\t{\n> > +\t\tstd::string data(\"H2G2\");\n> > +\t\tssize_t ret;\n> > +\n> > +\t\tmemset(data_, 0, sizeof(data_));\n> > +\t\tsize_ = 0;\n> > +\n> > +\t\tret = write(pipefd_[1], data.data(), data.size());\n> > +\t\tif (ret < 0) {\n> > +\t\t\tcout << \"Pipe write failed\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tbool notified() const\n> > +\t{\n> > +\t\treturn notified_;\n> > +\t}\n> > +\n> > +\tvoid moveToThread(Thread *thread)\n> > +\t{\n> > +\t\tObject::moveToThread(thread);\n> > +\t\tnotifier_->moveToThread(thread);\n> > +\t}\n> > +\n> > +private:\n> > +\tvoid readReady(EventNotifier *notifier)\n> > +\t{\n> > +\t\tsize_ = read(notifier->fd(), data_, sizeof(data_));\n> > +\t\tnotified_ = true;\n> > +\t}\n> > +\n> > +\tEventNotifier *notifier_;\n> > +\n> > +\tint pipefd_[2];\n> > +\n> > +\tbool notified_;\n> > +\tchar data_[16];\n> > +\tssize_t size_;\n> > +};\n> > +\n> > +class EventThreadTest : public Test\n> > +{\n> > +protected:\n> > +\tint run()\n> > +\t{\n> > +\t\tThread thread;\n> > +\t\tthread.start();\n> > +\n> > +\t\tEventHandler handler;\n> > +\t\thandler.notify();\n> > +\t\thandler.moveToThread(&thread);\n> \n> I had to think twice when reading this. Maybe add comment to clarify \n> that it's OK to generate the event first and then move it as the main \n> thread don't process events it's only possible for the event to be \n> processed in a different thread.\n> \n> Or maybe I just have to get used to this API to make it apparent that \n> this is the only possibility.\n\nNo, you have a good point. I'll add this comment.\n\n /*\n * Fire the event notifier and then move the notifier to a\n * different thread. The notifier will not notice the event\n * immediately as there is no event dispatcher loop running in\n * the main thread. This tests that a notifier being moved to a\n * different thread will correctly process already pending\n * events in the new thread.\n */\n\n> With the initialization of notified_ fixed and with or without a comment,\n> \n> Reviewed-by: Niklas Söderlund \n> \n> > +\n> > +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n> > +\n> > +\t\t/* Must stop thread before destroying the handler. */\n> > +\t\tthread.exit(0);\n> > +\t\tthread.wait();\n> > +\n> > +\t\tif (!handler.notified()) {\n> > +\t\t\tcout << \"Thread event handling test failed\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +};\n> > +\n> > +TEST_REGISTER(EventThreadTest)\n> > diff --git a/test/meson.build b/test/meson.build\n> > index c6601813db78..f695ffd7be44 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -23,6 +23,7 @@ public_tests = [\n> > \n> > internal_tests = [\n> > ['camera-sensor', 'camera-sensor.cpp'],\n> > + ['event-thread', 'event-thread.cpp'],\n> > ['message', 'message.cpp'],\n> > ['object', 'object.cpp'],\n> > ['object-invoke', 'object-invoke.cpp'],","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83348600F9\n\tfor ;\n\tSat, 17 Aug 2019 17:46:53 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1C7E7556;\n\tSat, 17 Aug 2019 17:46:53 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1566056813;\n\tbh=FpXjzZZnrJ/qWJ0HySp1kU9mj6bPy5mMZDxE9hSP02o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nhlP+LCiUpszx1G+dj41rXExG5c4u2fF1/sx0XGyyWtAdONPqqlLvpVTtHHQszERx\n\tkK8Cwfnr4J0HsfIzUjbxI2EXfiSRIWC9zbh6wLiB9krO5zslTVxJsID3s09Sm2in2T\n\tTX7vp6kHstnQS9EoFNTwO5oEG/bvsUSYce1hTB6A=","Date":"Sat, 17 Aug 2019 18:46:48 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190817154648.GC15630@pendragon.ideasonboard.com>","References":"<20190817152104.10834-1-laurent.pinchart@ideasonboard.com>\n\t<20190817152104.10834-14-laurent.pinchart@ideasonboard.com>\n\t<20190817153837.GB21683@wyvern>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190817153837.GB21683@wyvern>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 13/18] test: Add EventNotifier\n\tthread move test","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":"Sat, 17 Aug 2019 15:46:53 -0000"}}]