[{"id":11755,"web_url":"https://patchwork.libcamera.org/comment/11755/","msgid":"<20200731203828.GA24315@pendragon.ideasonboard.com>","date":"2020-07-31T20:38:28","subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:\n> Add --monitor to monitor new hotplug and unplug camera events from\n> the CameraManager.\n> \n> Signed-off-by: Umang Jain \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Kieran Bingham \n> ---\n> src/cam/main.cpp | 26 ++++++++++++++++++++++++++\n> src/cam/main.h | 1 +\n> 2 files changed, 27 insertions(+)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index f5aba04..b03437e 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -36,6 +36,8 @@ public:\n> \tvoid quit();\n> \n> private:\n> +\tvoid cameraAdded(std::shared_ptr cam);\n> +\tvoid cameraRemoved(std::shared_ptr cam);\n> \tint parseOptions(int argc, char *argv[]);\n> \tint prepareConfig();\n> \tint listControls();\n> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)\n> \t\tret = prepareConfig();\n> \t\tif (ret)\n> \t\t\treturn ret;\n> +\t} else if (options_.isSet(OptMonitor)) {\n> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> +\t\tstd::cout << \"Monitoring new hotplug or unplug camera events…\" << std::endl;\n> \t}\n> \n> \tloop_ = new EventLoop(cm_->eventDispatcher());\n> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \tparser.addOption(OptStrictFormats, OptionNone,\n> \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n> \t\t\t \"strict-formats\");\n> +\tparser.addOption(OptMonitor, OptionNone, \"Monitor for hotplug and unplug camera events\",\n\nI'd move the message to the next line to avoid too long lines.\n\n> +\t\t\t \"monitor\");\n> \n> \toptions_ = parser.parse(argc, argv);\n> \tif (!options_.valid())\n> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()\n> \treturn 0;\n> }\n> \n> +void CamApp::cameraAdded(std::shared_ptr cam)\n> +{\n> +\tstd::cout << \"Camera Added: \" << cam->name() << std::endl;\n> +}\n> +\n> +void CamApp::cameraRemoved(std::shared_ptr cam)\n> +{\n> +\tstd::cout << \"Camera Removed: \" << cam->name() << std::endl;\n> +}\n> +\n> int CamApp::run()\n> {\n> \tint ret;\n> @@ -342,10 +360,18 @@ int CamApp::run()\n> \t}\n> \n> \tif (options_.isSet(OptCapture)) {\n> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n\nThis is done in CamApp::init() already, do we need it here ? One can\nspecific both --monitor and --capture if they want to monitor for\nhotplug during capture, I wouldn't make it automatic.\n\n> \t\tCapture capture(camera_, config_.get(), loop_);\n> \t\treturn capture.run(options_);\n> \t}\n> \n> +\tif (options_.isSet(OptMonitor)) {\n> +\t\tret = loop_->exec();\n> +\t\tif (ret)\n> +\t\t\tstd::cout << \"Failed to run monitor loop\" << std::endl;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/src/cam/main.h b/src/cam/main.h\n> index 6f95add..ea8dfd3 100644\n> --- a/src/cam/main.h\n> +++ b/src/cam/main.h\n> @@ -15,6 +15,7 @@ enum {\n> \tOptInfo = 'I',\n> \tOptList = 'l',\n> \tOptListProperties = 'p',\n> +\tOptMonitor = 'm',\n> \tOptStream = 's',\n> \tOptListControls = 256,\n> \tOptStrictFormats = 257,","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 65AF9BD86F\n\tfor ;\n\tFri, 31 Jul 2020 20:38:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE43161EAB;\n\tFri, 31 Jul 2020 22:38:38 +0200 (CEST)","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 541DB60398\n\tfor ;\n\tFri, 31 Jul 2020 22:38:38 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BFEE953C;\n\tFri, 31 Jul 2020 22:38:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NV4+zitR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596227917;\n\tbh=TFVI9W8mPuNmeW/OpydK3w8/QiEtrmNIakkoy4QpyuY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NV4+zitRwyAyKdTFlx6z0O6igrl+2jCidNEzolO6UYl7QgJYhHKfDsHizgFKkp1ye\n\t+mpiCQMQgIDDp4nW88jNCzk8oND3nXwyVqIzgPI1zijurOiJIFunDLPNzsxAepfOsg\n\tU3n7IjarFGXQbSZIv30RNIc7DWA6o46u1hszod7I=","Date":"Fri, 31 Jul 2020 23:38:28 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200731203828.GA24315@pendragon.ideasonboard.com>","References":"<20200731194649.110184-1-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200731194649.110184-1-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11756,"web_url":"https://patchwork.libcamera.org/comment/11756/","msgid":"<20200731203937.GB24315@pendragon.ideasonboard.com>","date":"2020-07-31T20:39:37","subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jul 31, 2020 at 11:38:28PM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:\n> > Add --monitor to monitor new hotplug and unplug camera events from\n> > the CameraManager.\n> > \n> > Signed-off-by: Umang Jain \n> > Reviewed-by: Niklas Söderlund \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > src/cam/main.cpp | 26 ++++++++++++++++++++++++++\n> > src/cam/main.h | 1 +\n> > 2 files changed, 27 insertions(+)\n> > \n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index f5aba04..b03437e 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -36,6 +36,8 @@ public:\n> > \tvoid quit();\n> > \n> > private:\n> > +\tvoid cameraAdded(std::shared_ptr cam);\n> > +\tvoid cameraRemoved(std::shared_ptr cam);\n> > \tint parseOptions(int argc, char *argv[]);\n> > \tint prepareConfig();\n> > \tint listControls();\n> > @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)\n> > \t\tret = prepareConfig();\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > +\t} else if (options_.isSet(OptMonitor)) {\n> > +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> > +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> > +\t\tstd::cout << \"Monitoring new hotplug or unplug camera events…\" << std::endl;\n> > \t}\n> > \n> > \tloop_ = new EventLoop(cm_->eventDispatcher());\n> > @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \tparser.addOption(OptStrictFormats, OptionNone,\n> > \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n> > \t\t\t \"strict-formats\");\n> > +\tparser.addOption(OptMonitor, OptionNone, \"Monitor for hotplug and unplug camera events\",\n> \n> I'd move the message to the next line to avoid too long lines.\n> \n> > +\t\t\t \"monitor\");\n> > \n> > \toptions_ = parser.parse(argc, argv);\n> > \tif (!options_.valid())\n> > @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()\n> > \treturn 0;\n> > }\n> > \n> > +void CamApp::cameraAdded(std::shared_ptr cam)\n> > +{\n> > +\tstd::cout << \"Camera Added: \" << cam->name() << std::endl;\n> > +}\n> > +\n> > +void CamApp::cameraRemoved(std::shared_ptr cam)\n> > +{\n> > +\tstd::cout << \"Camera Removed: \" << cam->name() << std::endl;\n> > +}\n> > +\n> > int CamApp::run()\n> > {\n> > \tint ret;\n> > @@ -342,10 +360,18 @@ int CamApp::run()\n> > \t}\n> > \n> > \tif (options_.isSet(OptCapture)) {\n> > +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> > +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> \n> This is done in CamApp::init() already, do we need it here ? One can\n> specific both --monitor and --capture if they want to monitor for\n> hotplug during capture, I wouldn't make it automatic.\n\nAnd for this to work of course, the else if should be turned into an if\nin CamApp::init().\n\n> > \t\tCapture capture(camera_, config_.get(), loop_);\n> > \t\treturn capture.run(options_);\n> > \t}\n> > \n> > +\tif (options_.isSet(OptMonitor)) {\n> > +\t\tret = loop_->exec();\n> > +\t\tif (ret)\n> > +\t\t\tstd::cout << \"Failed to run monitor loop\" << std::endl;\n> > +\t}\n> > +\n> > \treturn 0;\n> > }\n> > \n> > diff --git a/src/cam/main.h b/src/cam/main.h\n> > index 6f95add..ea8dfd3 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -15,6 +15,7 @@ enum {\n> > \tOptInfo = 'I',\n> > \tOptList = 'l',\n> > \tOptListProperties = 'p',\n> > +\tOptMonitor = 'm',\n> > \tOptStream = 's',\n> > \tOptListControls = 256,\n> > \tOptStrictFormats = 257,","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C06A3BD86F\n\tfor ;\n\tFri, 31 Jul 2020 20:39:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C829616FF;\n\tFri, 31 Jul 2020 22:39:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2CA5E60398\n\tfor ;\n\tFri, 31 Jul 2020 22:39:47 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E04253C;\n\tFri, 31 Jul 2020 22:39:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"a/ajGiJI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596227986;\n\tbh=xXAAEkcrjSKnqsYoT/AFrBPVPgs5RuO2+1wDiToJFQ4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a/ajGiJI4SIBo3br4XLMJWqWaX5XTDkXtQABD1LoWXEctasMc3UfMQoFdGGXtPRd8\n\tXqFvP6cujdslvF8JBmlEYzas51HRs/xRaZTSIVZPCjYpKVfixNF9BB1bnu0YpU/HRK\n\txV0ZTZ2vP0/JI0Di0ddGmSKfza/54priK5UjKtkk=","Date":"Fri, 31 Jul 2020 23:39:37 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200731203937.GB24315@pendragon.ideasonboard.com>","References":"<20200731194649.110184-1-email@uajain.com>\n\t<20200731203828.GA24315@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200731203828.GA24315@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11765,"web_url":"https://patchwork.libcamera.org/comment/11765/","msgid":"","date":"2020-08-01T10:19:11","subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nOn 8/1/20 2:08 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:\n>> Add --monitor to monitor new hotplug and unplug camera events from\n>> the CameraManager.\n>>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Niklas Söderlund \n>> Reviewed-by: Kieran Bingham \n>> ---\n>> src/cam/main.cpp | 26 ++++++++++++++++++++++++++\n>> src/cam/main.h | 1 +\n>> 2 files changed, 27 insertions(+)\n>>\n>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n>> index f5aba04..b03437e 100644\n>> --- a/src/cam/main.cpp\n>> +++ b/src/cam/main.cpp\n>> @@ -36,6 +36,8 @@ public:\n>> \tvoid quit();\n>> \n>> private:\n>> +\tvoid cameraAdded(std::shared_ptr cam);\n>> +\tvoid cameraRemoved(std::shared_ptr cam);\n>> \tint parseOptions(int argc, char *argv[]);\n>> \tint prepareConfig();\n>> \tint listControls();\n>> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)\n>> \t\tret = prepareConfig();\n>> \t\tif (ret)\n>> \t\t\treturn ret;\n>> +\t} else if (options_.isSet(OptMonitor)) {\n>> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n>> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n>> +\t\tstd::cout << \"Monitoring new hotplug or unplug camera events…\" << std::endl;\n>> \t}\n>> \n>> \tloop_ = new EventLoop(cm_->eventDispatcher());\n>> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n>> \tparser.addOption(OptStrictFormats, OptionNone,\n>> \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n>> \t\t\t \"strict-formats\");\n>> +\tparser.addOption(OptMonitor, OptionNone, \"Monitor for hotplug and unplug camera events\",\n> I'd move the message to the next line to avoid too long lines.\n>\n>> +\t\t\t \"monitor\");\n>> \n>> \toptions_ = parser.parse(argc, argv);\n>> \tif (!options_.valid())\n>> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()\n>> \treturn 0;\n>> }\n>> \n>> +void CamApp::cameraAdded(std::shared_ptr cam)\n>> +{\n>> +\tstd::cout << \"Camera Added: \" << cam->name() << std::endl;\n>> +}\n>> +\n>> +void CamApp::cameraRemoved(std::shared_ptr cam)\n>> +{\n>> +\tstd::cout << \"Camera Removed: \" << cam->name() << std::endl;\n>> +}\n>> +\n>> int CamApp::run()\n>> {\n>> \tint ret;\n>> @@ -342,10 +360,18 @@ int CamApp::run()\n>> \t}\n>> \n>> \tif (options_.isSet(OptCapture)) {\n>> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n>> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> This is done in CamApp::init() already, do we need it here ? One can\n> specific both --monitor and --capture if they want to monitor for\n> hotplug during capture, I wouldn't make it automatic.\nhmm, one of reviews from Kieran in v1 was to make it automatic, if we \nrun any\nkind of loop (like Capture), hence the patch got driven in that direction.\n\nI think specifying --monitor (shorthand -m) explicitly shouldn't be that \nbig a deal.\n\n\n>\n>> \t\tCapture capture(camera_, config_.get(), loop_);\n>> \t\treturn capture.run(options_);\n>> \t}\n>> \n>> +\tif (options_.isSet(OptMonitor)) {\n>> +\t\tret = loop_->exec();\n>> +\t\tif (ret)\n>> +\t\t\tstd::cout << \"Failed to run monitor loop\" << std::endl;\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>> \n>> diff --git a/src/cam/main.h b/src/cam/main.h\n>> index 6f95add..ea8dfd3 100644\n>> --- a/src/cam/main.h\n>> +++ b/src/cam/main.h\n>> @@ -15,6 +15,7 @@ enum {\n>> \tOptInfo = 'I',\n>> \tOptList = 'l',\n>> \tOptListProperties = 'p',\n>> +\tOptMonitor = 'm',\n>> \tOptStream = 's',\n>> \tOptListControls = 256,\n>> \tOptStrictFormats = 257,","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1720ABD878\n\tfor ;\n\tSat, 1 Aug 2020 10:19:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B79461F24;\n\tSat, 1 Aug 2020 12:19:15 +0200 (CEST)","from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 179EC61970\n\tfor ;\n\tSat, 1 Aug 2020 12:19:12 +0200 (CEST)","by filterdrecv-p3iad2-d8cc6d457-6kjsg with SMTP id\n\tfilterdrecv-p3iad2-d8cc6d457-6kjsg-18-5F25419F-21\n\t2020-08-01 10:19:11.423207041 +0000 UTC m=+231048.234965292","from mail.uajain.com (unknown)\n\tby ismtpd0008p1hnd1.sendgrid.net (SG) with ESMTP\n\tid Rykyn55lSGO62igiADulaQ Sat, 01 Aug 2020 10:19:10.914 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"XkAaccZk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=Cmp0wOksW5tWELKm01WdRwssl5MwR0ZZ7L+Wlh5lSWc=;\n\tb=XkAaccZkhm3b2yuadzVBzXVVJFRo7fmvTowOl9Qo47aHzy/VJYUEWcaaOqIyURqrLFLc\n\tHzknBYQlm0eXDwloRRYWV1mqY68RjM1bKrdu2XABLNGHD/VmzujTXcR/UqdQ8tNw0TaVCQ\n\tJNVOaUcXSHGlaxHNXBduQEUogSEoCEpj0=","References":"<20200731194649.110184-1-email@uajain.com>\n\t<20200731203828.GA24315@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Sat, 01 Aug 2020 10:19:11 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200731203828.GA24315@pendragon.ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcYnCu86f3L0M7Nw13AAh1QnDijcA8aBcqO3Ks65lImDZ72nXUQSnJ6j1Tw+VvRnUoDvgzKg2rd/sD9wPYq00quAopBO2R0S+60gn+ufjM7V0CdMWnr/rkP3LohLOJfPR1W/vNhfxvKKI8jSS9M0zvP/athu+hIewk1uAuugpbEupVs90P2lH0VkRzX2/g5/V62bcQW9E623FFNOo2jA6jWA==","To":"Laurent Pinchart ","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11769,"web_url":"https://patchwork.libcamera.org/comment/11769/","msgid":"<20200801125307.GC11820@pendragon.ideasonboard.com>","date":"2020-08-01T12:53:07","subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nCC'ing Kieran,\n\nOn Sat, Aug 01, 2020 at 10:19:11AM +0000, Umang Jain wrote:\n> On 8/1/20 2:08 AM, Laurent Pinchart wrote:\n> > On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:\n> >> Add --monitor to monitor new hotplug and unplug camera events from\n> >> the CameraManager.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> Reviewed-by: Niklas Söderlund \n> >> Reviewed-by: Kieran Bingham \n> >> ---\n> >> src/cam/main.cpp | 26 ++++++++++++++++++++++++++\n> >> src/cam/main.h | 1 +\n> >> 2 files changed, 27 insertions(+)\n> >>\n> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> >> index f5aba04..b03437e 100644\n> >> --- a/src/cam/main.cpp\n> >> +++ b/src/cam/main.cpp\n> >> @@ -36,6 +36,8 @@ public:\n> >> \tvoid quit();\n> >> \n> >> private:\n> >> +\tvoid cameraAdded(std::shared_ptr cam);\n> >> +\tvoid cameraRemoved(std::shared_ptr cam);\n> >> \tint parseOptions(int argc, char *argv[]);\n> >> \tint prepareConfig();\n> >> \tint listControls();\n> >> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)\n> >> \t\tret = prepareConfig();\n> >> \t\tif (ret)\n> >> \t\t\treturn ret;\n> >> +\t} else if (options_.isSet(OptMonitor)) {\n> >> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> >> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> >> +\t\tstd::cout << \"Monitoring new hotplug or unplug camera events…\" << std::endl;\n> >> \t}\n> >> \n> >> \tloop_ = new EventLoop(cm_->eventDispatcher());\n> >> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n> >> \tparser.addOption(OptStrictFormats, OptionNone,\n> >> \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n> >> \t\t\t \"strict-formats\");\n> >> +\tparser.addOption(OptMonitor, OptionNone, \"Monitor for hotplug and unplug camera events\",\n> >\n> > I'd move the message to the next line to avoid too long lines.\n> >\n> >> +\t\t\t \"monitor\");\n> >> \n> >> \toptions_ = parser.parse(argc, argv);\n> >> \tif (!options_.valid())\n> >> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()\n> >> \treturn 0;\n> >> }\n> >> \n> >> +void CamApp::cameraAdded(std::shared_ptr cam)\n> >> +{\n> >> +\tstd::cout << \"Camera Added: \" << cam->name() << std::endl;\n> >> +}\n> >> +\n> >> +void CamApp::cameraRemoved(std::shared_ptr cam)\n> >> +{\n> >> +\tstd::cout << \"Camera Removed: \" << cam->name() << std::endl;\n> >> +}\n> >> +\n> >> int CamApp::run()\n> >> {\n> >> \tint ret;\n> >> @@ -342,10 +360,18 @@ int CamApp::run()\n> >> \t}\n> >> \n> >> \tif (options_.isSet(OptCapture)) {\n> >> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> >> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> >\n> > This is done in CamApp::init() already, do we need it here ? One can\n> > specific both --monitor and --capture if they want to monitor for\n> > hotplug during capture, I wouldn't make it automatic.\n>\n> hmm, one of reviews from Kieran in v1 was to make it automatic, if we \n> run any\n> kind of loop (like Capture), hence the patch got driven in that direction.\n> \n> I think specifying --monitor (shorthand -m) explicitly shouldn't be that \n> big a deal.\n\nI'll let Kieran comment on this before merging the patch.\n\n> >> \t\tCapture capture(camera_, config_.get(), loop_);\n> >> \t\treturn capture.run(options_);\n> >> \t}\n> >> \n> >> +\tif (options_.isSet(OptMonitor)) {\n> >> +\t\tret = loop_->exec();\n> >> +\t\tif (ret)\n> >> +\t\t\tstd::cout << \"Failed to run monitor loop\" << std::endl;\n> >> +\t}\n> >> +\n> >> \treturn 0;\n> >> }\n> >> \n> >> diff --git a/src/cam/main.h b/src/cam/main.h\n> >> index 6f95add..ea8dfd3 100644\n> >> --- a/src/cam/main.h\n> >> +++ b/src/cam/main.h\n> >> @@ -15,6 +15,7 @@ enum {\n> >> \tOptInfo = 'I',\n> >> \tOptList = 'l',\n> >> \tOptListProperties = 'p',\n> >> +\tOptMonitor = 'm',\n> >> \tOptStream = 's',\n> >> \tOptListControls = 256,\n> >> \tOptStrictFormats = 257,","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 97FE3BD86F\n\tfor ;\n\tSat, 1 Aug 2020 12:53:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62EB161F7D;\n\tSat, 1 Aug 2020 14:53:19 +0200 (CEST)","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 AF4E061F24\n\tfor ;\n\tSat, 1 Aug 2020 14:53:17 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D00A55E;\n\tSat, 1 Aug 2020 14:53:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ieJ8Qvk+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596286397;\n\tbh=RtkYh/e6zoRSH25M0goMiMvgiT5FpkLs4WhM542mswk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ieJ8Qvk+sjcbO8IxU2w4axYqmfxLkinT3Lbd/PeQ1ehj8BnhFKxcJSSF9unZUMUh0\n\tCUTUZ/F2V9Z2QiGFrWfdho4SilmCu2U497kdvP+PYubZQLgKtNLr1uIw+elqTDzFQX\n\tXmk3U1P2nnBhs91qBYqWnCmjv4QjFgJsnc/ZgszY=","Date":"Sat, 1 Aug 2020 15:53:07 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200801125307.GC11820@pendragon.ideasonboard.com>","References":"<20200731194649.110184-1-email@uajain.com>\n\t<20200731203828.GA24315@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11787,"web_url":"https://patchwork.libcamera.org/comment/11787/","msgid":"<7cbbc4d2-4548-2719-de51-ef05e0ecd961@ideasonboard.com>","date":"2020-08-03T09:01:09","subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang, Laurent,\n\nOn 01/08/2020 13:53, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> CC'ing Kieran,\n> \n> On Sat, Aug 01, 2020 at 10:19:11AM +0000, Umang Jain wrote:\n>> On 8/1/20 2:08 AM, Laurent Pinchart wrote:\n>>> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:\n>>>> Add --monitor to monitor new hotplug and unplug camera events from\n>>>> the CameraManager.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> Reviewed-by: Niklas Söderlund \n>>>> Reviewed-by: Kieran Bingham \n>>>> ---\n>>>> src/cam/main.cpp | 26 ++++++++++++++++++++++++++\n>>>> src/cam/main.h | 1 +\n>>>> 2 files changed, 27 insertions(+)\n>>>>\n>>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n>>>> index f5aba04..b03437e 100644\n>>>> --- a/src/cam/main.cpp\n>>>> +++ b/src/cam/main.cpp\n>>>> @@ -36,6 +36,8 @@ public:\n>>>> \tvoid quit();\n>>>> \n>>>> private:\n>>>> +\tvoid cameraAdded(std::shared_ptr cam);\n>>>> +\tvoid cameraRemoved(std::shared_ptr cam);\n>>>> \tint parseOptions(int argc, char *argv[]);\n>>>> \tint prepareConfig();\n>>>> \tint listControls();\n>>>> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)\n>>>> \t\tret = prepareConfig();\n>>>> \t\tif (ret)\n>>>> \t\t\treturn ret;\n>>>> +\t} else if (options_.isSet(OptMonitor)) {\n>>>> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n>>>> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n>>>> +\t\tstd::cout << \"Monitoring new hotplug or unplug camera events…\" << std::endl;\n>>>> \t}\n\nMy comment previously was that I think we should always connect these\nsignals in this function (without an 'if isSet(OptMonitor)') so that any\ntime cam is running, if there is an event which makes it to the\napplication the user is notified.\n\n\n>>>> \n>>>> \tloop_ = new EventLoop(cm_->eventDispatcher());\n>>>> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n>>>> \tparser.addOption(OptStrictFormats, OptionNone,\n>>>> \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n>>>> \t\t\t \"strict-formats\");\n>>>> +\tparser.addOption(OptMonitor, OptionNone, \"Monitor for hotplug and unplug camera events\",\n>>>\n>>> I'd move the message to the next line to avoid too long lines.\n>>>\n>>>> +\t\t\t \"monitor\");\n>>>> \n>>>> \toptions_ = parser.parse(argc, argv);\n>>>> \tif (!options_.valid())\n>>>> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> +void CamApp::cameraAdded(std::shared_ptr cam)\n>>>> +{\n>>>> +\tstd::cout << \"Camera Added: \" << cam->name() << std::endl;\n>>>> +}\n>>>> +\n>>>> +void CamApp::cameraRemoved(std::shared_ptr cam)\n>>>> +{\n>>>> +\tstd::cout << \"Camera Removed: \" << cam->name() << std::endl;\n>>>> +}\n>>>> +\n>>>> int CamApp::run()\n>>>> {\n>>>> \tint ret;\n>>>> @@ -342,10 +360,18 @@ int CamApp::run()\n>>>> \t}\n>>>> \n>>>> \tif (options_.isSet(OptCapture)) {\n>>>> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n>>>> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n>>>\n>>> This is done in CamApp::init() already, do we need it here ? One can\n>>> specific both --monitor and --capture if they want to monitor for\n>>> hotplug during capture, I wouldn't make it automatic.\n>>\n>> hmm, one of reviews from Kieran in v1 was to make it automatic, if we \n>> run any\n>> kind of loop (like Capture), hence the patch got driven in that direction.\n>>\n>> I think specifying --monitor (shorthand -m) explicitly shouldn't be that \n>> big a deal.\n> \n> I'll let Kieran comment on this before merging the patch.\n\nThus, if the connections are done in CamApp::init() (always, regardless\nof the -m) then we don't need to reconnect here ...\n\n\n> \n>>>> \t\tCapture capture(camera_, config_.get(), loop_);\n>>>> \t\treturn capture.run(options_);\n>>>> \t}\n>>>> \n>>>> +\tif (options_.isSet(OptMonitor)) {\n>>>> +\t\tret = loop_->exec();\n>>>> +\t\tif (ret)\n>>>> +\t\t\tstd::cout << \"Failed to run monitor loop\" << std::endl;\n>>>> +\t}\n\nBut we do still need to provide a loop.\n\nI'm not sure I'm keen on the idea of mixing --monitor and --capture as a\nrequirement to obtain notifications during a capture. I think it should\nalways notify. The '--monitor' is just a convenience to provide an event\nloop which will do nothing else except wait for events...\n\nAny thoughts anyone?\n\nI don't mind if the consensus is that to see notifications while doing a\ncapture you should still specify the '-m', because the connection will\nalready be done (conditionally in init), and we will be running in an\nevent loop in 'return capture.run(options_);' which will return when it\ncompletes.\n\n\nSo if consensus is \"to get events, then the -m option should be\nprovided\", then v1 of this patch already does that.\n\nOtherwise, if we should always report hotplug events, then v1 just needs\nto be adjusted to unconditionally connect the signal/slots during\nCamApp::init.\n\n\nPersonally, I would choose to always display the events. They are cheap,\nand the monitor loop becomes just a convenience.\n\n\n--\nKieran\n\n\n\n>>>> +\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> diff --git a/src/cam/main.h b/src/cam/main.h\n>>>> index 6f95add..ea8dfd3 100644\n>>>> --- a/src/cam/main.h\n>>>> +++ b/src/cam/main.h\n>>>> @@ -15,6 +15,7 @@ enum {\n>>>> \tOptInfo = 'I',\n>>>> \tOptList = 'l',\n>>>> \tOptListProperties = 'p',\n>>>> +\tOptMonitor = 'm',\n>>>> \tOptStream = 's',\n>>>> \tOptListControls = 256,\n>>>> \tOptStrictFormats = 257,\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AB115BD87A\n\tfor ;\n\tMon, 3 Aug 2020 09:01:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47E7060398;\n\tMon, 3 Aug 2020 11:01:13 +0200 (CEST)","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 1314060398\n\tfor ;\n\tMon, 3 Aug 2020 11:01:12 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 583C2543;\n\tMon, 3 Aug 2020 11:01:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"s+v8iP/Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596445271;\n\tbh=xvP522WLo0Vxb9sbRNH+2VY6GPhoe0ak8JScN/bh3Rc=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=s+v8iP/Yu5eatnqfTuzMZAKiXlP4v/zEivyJZO0tfhbshchfZYni/iEVy30BS4q3x\n\tQnzicZrmA/ZkiJOfQAwhz+lJ50IX5JiGGEiNKGDM/gFhQ2rt2OQdIrncIVJuYJ1HKs\n\thsdJAixq2lSt1WHdxhcHqTwprC6Q+4G5k2V4lAWs=","To":"Laurent Pinchart ,\n\tUmang Jain ","References":"<20200731194649.110184-1-email@uajain.com>\n\t<20200731203828.GA24315@pendragon.ideasonboard.com>\n\t\n\t<20200801125307.GC11820@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<7cbbc4d2-4548-2719-de51-ef05e0ecd961@ideasonboard.com>","Date":"Mon, 3 Aug 2020 10:01:09 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200801125307.GC11820@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11790,"web_url":"https://patchwork.libcamera.org/comment/11790/","msgid":"<20200803113124.GB5926@pendragon.ideasonboard.com>","date":"2020-08-03T11:31:24","subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Aug 03, 2020 at 10:01:09AM +0100, Kieran Bingham wrote:\n> On 01/08/2020 13:53, Laurent Pinchart wrote:\n> > On Sat, Aug 01, 2020 at 10:19:11AM +0000, Umang Jain wrote:\n> >> On 8/1/20 2:08 AM, Laurent Pinchart wrote:\n> >>> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:\n> >>>> Add --monitor to monitor new hotplug and unplug camera events from\n> >>>> the CameraManager.\n> >>>>\n> >>>> Signed-off-by: Umang Jain \n> >>>> Reviewed-by: Niklas Söderlund \n> >>>> Reviewed-by: Kieran Bingham \n> >>>> ---\n> >>>> src/cam/main.cpp | 26 ++++++++++++++++++++++++++\n> >>>> src/cam/main.h | 1 +\n> >>>> 2 files changed, 27 insertions(+)\n> >>>>\n> >>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> >>>> index f5aba04..b03437e 100644\n> >>>> --- a/src/cam/main.cpp\n> >>>> +++ b/src/cam/main.cpp\n> >>>> @@ -36,6 +36,8 @@ public:\n> >>>> \tvoid quit();\n> >>>> \n> >>>> private:\n> >>>> +\tvoid cameraAdded(std::shared_ptr cam);\n> >>>> +\tvoid cameraRemoved(std::shared_ptr cam);\n> >>>> \tint parseOptions(int argc, char *argv[]);\n> >>>> \tint prepareConfig();\n> >>>> \tint listControls();\n> >>>> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)\n> >>>> \t\tret = prepareConfig();\n> >>>> \t\tif (ret)\n> >>>> \t\t\treturn ret;\n> >>>> +\t} else if (options_.isSet(OptMonitor)) {\n> >>>> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> >>>> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> >>>> +\t\tstd::cout << \"Monitoring new hotplug or unplug camera events…\" << std::endl;\n> >>>> \t}\n> \n> My comment previously was that I think we should always connect these\n> signals in this function (without an 'if isSet(OptMonitor)') so that any\n> time cam is running, if there is an event which makes it to the\n> application the user is notified.\n> \n> >>>> \n> >>>> \tloop_ = new EventLoop(cm_->eventDispatcher());\n> >>>> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])\n> >>>> \tparser.addOption(OptStrictFormats, OptionNone,\n> >>>> \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n> >>>> \t\t\t \"strict-formats\");\n> >>>> +\tparser.addOption(OptMonitor, OptionNone, \"Monitor for hotplug and unplug camera events\",\n> >>>\n> >>> I'd move the message to the next line to avoid too long lines.\n> >>>\n> >>>> +\t\t\t \"monitor\");\n> >>>> \n> >>>> \toptions_ = parser.parse(argc, argv);\n> >>>> \tif (!options_.valid())\n> >>>> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()\n> >>>> \treturn 0;\n> >>>> }\n> >>>> \n> >>>> +void CamApp::cameraAdded(std::shared_ptr cam)\n> >>>> +{\n> >>>> +\tstd::cout << \"Camera Added: \" << cam->name() << std::endl;\n> >>>> +}\n> >>>> +\n> >>>> +void CamApp::cameraRemoved(std::shared_ptr cam)\n> >>>> +{\n> >>>> +\tstd::cout << \"Camera Removed: \" << cam->name() << std::endl;\n> >>>> +}\n> >>>> +\n> >>>> int CamApp::run()\n> >>>> {\n> >>>> \tint ret;\n> >>>> @@ -342,10 +360,18 @@ int CamApp::run()\n> >>>> \t}\n> >>>> \n> >>>> \tif (options_.isSet(OptCapture)) {\n> >>>> +\t\tcm_->cameraAdded.connect(this, &CamApp::cameraAdded);\n> >>>> +\t\tcm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);\n> >>>\n> >>> This is done in CamApp::init() already, do we need it here ? One can\n> >>> specific both --monitor and --capture if they want to monitor for\n> >>> hotplug during capture, I wouldn't make it automatic.\n> >>\n> >> hmm, one of reviews from Kieran in v1 was to make it automatic, if we \n> >> run any\n> >> kind of loop (like Capture), hence the patch got driven in that direction.\n> >>\n> >> I think specifying --monitor (shorthand -m) explicitly shouldn't be that \n> >> big a deal.\n> > \n> > I'll let Kieran comment on this before merging the patch.\n> \n> Thus, if the connections are done in CamApp::init() (always, regardless\n> of the -m) then we don't need to reconnect here ...\n> \n> >>>> \t\tCapture capture(camera_, config_.get(), loop_);\n> >>>> \t\treturn capture.run(options_);\n> >>>> \t}\n> >>>> \n> >>>> +\tif (options_.isSet(OptMonitor)) {\n> >>>> +\t\tret = loop_->exec();\n> >>>> +\t\tif (ret)\n> >>>> +\t\t\tstd::cout << \"Failed to run monitor loop\" << std::endl;\n> >>>> +\t}\n> \n> But we do still need to provide a loop.\n> \n> I'm not sure I'm keen on the idea of mixing --monitor and --capture as a\n> requirement to obtain notifications during a capture. I think it should\n> always notify. The '--monitor' is just a convenience to provide an event\n> loop which will do nothing else except wait for events...\n> \n> Any thoughts anyone?\n> \n> I don't mind if the consensus is that to see notifications while doing a\n> capture you should still specify the '-m', because the connection will\n> already be done (conditionally in init), and we will be running in an\n> event loop in 'return capture.run(options_);' which will return when it\n> completes.\n> \n> \n> So if consensus is \"to get events, then the -m option should be\n> provided\", then v1 of this patch already does that.\n\nThe reason why I think -m should be mandatory to listen to hotplug\nevents is that the cam application is meant to be a low-level test\napplication that can exercise the whole libcamera API. Having the\nability to capture without listening to hotplug events could be useful.\nIt would be a different story in a more specialized application, but for\ncam I think it's best to let the user decide what they need.\n\n> Otherwise, if we should always report hotplug events, then v1 just needs\n> to be adjusted to unconditionally connect the signal/slots during\n> CamApp::init.\n> \n> \n> Personally, I would choose to always display the events. They are cheap,\n> and the monitor loop becomes just a convenience.\n> \n> >>>> +\n> >>>> \treturn 0;\n> >>>> }\n> >>>> \n> >>>> diff --git a/src/cam/main.h b/src/cam/main.h\n> >>>> index 6f95add..ea8dfd3 100644\n> >>>> --- a/src/cam/main.h\n> >>>> +++ b/src/cam/main.h\n> >>>> @@ -15,6 +15,7 @@ enum {\n> >>>> \tOptInfo = 'I',\n> >>>> \tOptList = 'l',\n> >>>> \tOptListProperties = 'p',\n> >>>> +\tOptMonitor = 'm',\n> >>>> \tOptStream = 's',\n> >>>> \tOptListControls = 256,\n> >>>> \tOptStrictFormats = 257,","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 15C84BD86F\n\tfor ;\n\tMon, 3 Aug 2020 11:31:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A68C61946;\n\tMon, 3 Aug 2020 13:31:37 +0200 (CEST)","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 2D5FA60391\n\tfor ;\n\tMon, 3 Aug 2020 13:31:36 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8953D543;\n\tMon, 3 Aug 2020 13:31:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bqSYwQrX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596454295;\n\tbh=DqItHx/5qjurBOPSI0Dq1YhTioTvhW2PjCXbEu87qXI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bqSYwQrXcPRl9Tctce/GwT/NQ8hJ3FjxGW/bH0OTOo3RUtSbHBJnXLa4il1SRxaE7\n\tU32EzIvkxMCymQMMzSRWVhyX3HT5LIh+Q2Pd/bE0Jiy/VLwP0IA+Y8F1GrNrN2kO9t\n\tTkRkKh45RbOi76hTDjGUw7FbJ5c/gx8O9CNtzLbs=","Date":"Mon, 3 Aug 2020 14:31:24 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200803113124.GB5926@pendragon.ideasonboard.com>","References":"<20200731194649.110184-1-email@uajain.com>\n\t<20200731203828.GA24315@pendragon.ideasonboard.com>\n\t\n\t<20200801125307.GC11820@pendragon.ideasonboard.com>\n\t<7cbbc4d2-4548-2719-de51-ef05e0ecd961@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<7cbbc4d2-4548-2719-de51-ef05e0ecd961@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] cam: Add --monitor option","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]