[{"id":1674,"web_url":"https://patchwork.libcamera.org/comment/1674/","msgid":"<20190523094809.GI4745@pendragon.ideasonboard.com>","date":"2019-05-23T09:48:09","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, May 23, 2019 at 02:55:34AM +0200, Niklas Söderlund wrote:\n> Add more structure to main.cpp by breaking up the logic into a CamApp\n> class. This makes the code easier to read and removes all but one of the\n> organically grown global variables.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/main.cpp | 171 +++++++++++++++++++++++++++++++----------------\n> 1 file changed, 112 insertions(+), 59 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index fe7d4f90dbf14ffd..5ca8356025a0c9f9 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -18,17 +18,101 @@\n> \n> using namespace libcamera;\n> \n> -OptionsParser::Options options;\n> -std::shared_ptr camera;\n> -EventLoop *loop;\n> +class CamApp\n> +{\n> +public:\n> +\tCamApp();\n> +\n> +\tint init(int argc, char **argv);\n\nI would pass the argc and argv to the constructor, following the\nhttps://doc.qt.io/qt-5/qapplication.html API. You can either store them\ninternally and parse the options in init(), or do part of the\ninitialisation in the constructor and stored a valid state that would\nmake init() return an error immediately. The global app variable would\nthen become a pointer, or possibly even better, you could add a static\nmethod to CamApp() named instance() that would return the instance\n(stored in a static member variable in the constructor), and remove the\nlast global variable :-) The signal handler would call\nCamApp::instance()->quit().\n\n> +\tvoid cleanup();\n> +\n> +\tint run();\n\nI would call this exec() to mimic Qt to.\n\n> +\n> +\tEventLoop *loop;\n\nMissing blank line ?\n\n> +private:\n> +\tint parseOptions(int argc, char *argv[]);\n> +\n> +\tOptionsParser::Options options_;\n> +\tCameraManager *cm_;\n> +\tstd::shared_ptr camera_;\n> +};\n> +\n> +CamApp::CamApp()\n> +\t: cm_(nullptr), camera_(nullptr)\n> +{\n> +}\n> +\n> +int CamApp::init(int argc, char **argv)\n> +{\n> +\tint ret;\n> +\n> +\tret = parseOptions(argc, argv);\n> +\tif (ret < 0)\n> +\t\treturn ret == -EINTR ? 0 : ret;\n> +\n> +\tcm_ = CameraManager::instance();\n> +\n> +\tret = cm_->start();\n> +\tif (ret) {\n> +\t\tstd::cout << \"Failed to start camera manager: \"\n> +\t\t\t << strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> \n> -void signalHandler(int signal)\n> +\tif (options_.isSet(OptCamera)) {\n> +\t\tcamera_ = cm_->get(options_[OptCamera]);\n> +\t\tif (!camera_) {\n> +\t\t\tstd::cout << \"Camera \"\n> +\t\t\t\t << std::string(options_[OptCamera])\n> +\t\t\t\t << \" not found\" << std::endl;\n> +\t\t\tcm_->stop();\n> +\t\t\treturn -ENODEV;\n> +\t\t}\n> +\n> +\t\tif (camera_->acquire()) {\n> +\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> +\t\t\tcamera_.reset();\n> +\t\t\tcm_->stop();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tstd::cout << \"Using camera \" << camera_->name() << std::endl;\n> +\t}\n> +\n> +\tloop = new EventLoop(cm_->eventDispatcher());\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CamApp::cleanup()\n> {\n> -\tstd::cout << \"Exiting\" << std::endl;\n> -\tloop->exit();\n> +\tdelete loop;\n> +\n> +\tif (camera_) {\n> +\t\tcamera_->release();\n> +\t\tcamera_.reset();\n> +\t}\n> +\n> +\tcm_->stop();\n> +}\n> +\n> +int CamApp::run()\n> +{\n> +\tif (options_.isSet(OptList)) {\n> +\t\tstd::cout << \"Available cameras:\" << std::endl;\n> +\t\tfor (const std::shared_ptr &cam : cm_->cameras())\n> +\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> +\t}\n> +\n> +\tif (options_.isSet(OptCapture)) {\n> +\t\tCapture capture;\n> +\t\treturn capture.run(camera_.get(), loop, options_);\n> +\t}\n> +\n> +\treturn 0;\n> }\n> \n> -static int parseOptions(int argc, char *argv[])\n> +int CamApp::parseOptions(int argc, char *argv[])\n> {\n> \tKeyValueParser streamKeyValue;\n> \tstreamKeyValue.addOption(\"role\", OptionString,\n> @@ -58,77 +142,46 @@ static int parseOptions(int argc, char *argv[])\n> \t\t\t \"help\");\n> \tparser.addOption(OptList, OptionNone, \"List all cameras\", \"list\");\n> \n> -\toptions = parser.parse(argc, argv);\n> -\tif (!options.valid())\n> +\toptions_ = parser.parse(argc, argv);\n> +\tif (!options_.valid())\n> \t\treturn -EINVAL;\n> \n> -\tif (options.empty() || options.isSet(OptHelp)) {\n> +\tif (options_.empty() || options_.isSet(OptHelp)) {\n> \t\tparser.usage();\n> -\t\treturn options.empty() ? -EINVAL : -EINTR;\n> +\t\treturn options_.empty() ? -EINVAL : -EINTR;\n> \t}\n> \n> \treturn 0;\n> }\n> \n> +CamApp app;\n> +\n> +void signalHandler(int signal)\n> +{\n> +\tstd::cout << \"Exiting\" << std::endl;\n> +\n> +\tif (app.loop)\n> +\t\tapp.loop->exit();\n\nInstead of exposing the loop member, how about adding a public quit()\nmethod that would call loop->exit() ? loop should then become loop_.\n\n> +}\n> +\n> int main(int argc, char **argv)\n> {\n> \tint ret;\n> \n> -\tret = parseOptions(argc, argv);\n> -\tif (ret < 0)\n> -\t\treturn ret == -EINTR ? 0 : EXIT_FAILURE;\n> -\n> -\tCameraManager *cm = CameraManager::instance();\n> -\n> -\tret = cm->start();\n> -\tif (ret) {\n> -\t\tstd::cout << \"Failed to start camera manager: \"\n> -\t\t\t << strerror(-ret) << std::endl;\n> +\tret = app.init(argc, argv);\n> +\tif (ret)\n> \t\treturn EXIT_FAILURE;\n> -\t}\n> -\n> -\tloop = new EventLoop(cm->eventDispatcher());\n> \n> \tstruct sigaction sa = {};\n> \tsa.sa_handler = &signalHandler;\n> \tsigaction(SIGINT, &sa, nullptr);\n> \n> -\tif (options.isSet(OptList)) {\n> -\t\tstd::cout << \"Available cameras:\" << std::endl;\n> -\t\tfor (const std::shared_ptr &cam : cm->cameras())\n> -\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> -\t}\n> +\tret = app.run();\n> \n> -\tif (options.isSet(OptCamera)) {\n> -\t\tcamera = cm->get(options[OptCamera]);\n> -\t\tif (!camera) {\n> -\t\t\tstd::cout << \"Camera \"\n> -\t\t\t\t << std::string(options[OptCamera])\n> -\t\t\t\t << \" not found\" << std::endl;\n> -\t\t\tgoto out;\n> -\t\t}\n> +\tapp.cleanup();\n\nShould cleanup() be called internally at the end of run() ?\n\n> \n> -\t\tif (camera->acquire()) {\n> -\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> -\t\t\tgoto out;\n> -\t\t}\n> +\tif (ret)\n> +\t\treturn EXIT_FAILURE;\n> \n> -\t\tstd::cout << \"Using camera \" << camera->name() << std::endl;\n> -\t}\n> -\n> -\tif (options.isSet(OptCapture)) {\n> -\t\tCapture capture;\n> -\t\tret = capture.run(camera.get(), loop, options);\n> -\t}\n> -\n> -\tif (camera) {\n> -\t\tcamera->release();\n> -\t\tcamera.reset();\n> -\t}\n> -out:\n> -\tdelete loop;\n> -\n> -\tcm->stop();\n> -\n> -\treturn ret;\n> +\treturn 0;\n> }","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 CDF4960003\n\tfor ;\n\tThu, 23 May 2019 11:48:31 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-106-214-nat.elisa-mobile.fi\n\t[85.76.106.214])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7419A334;\n\tThu, 23 May 2019 11:48:30 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558604911;\n\tbh=ScaKZR2vcA1iaWIh+xpNdOTPPHmO6W3U9yafN8j0y3k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pDs9bl+AZZREmLXobpD1HbrXaq+8mFOVuucNKLVVVtm6uhKEGSjV/7TIGy9R87ixi\n\taPI5Wh2ot4VL6X5/zCICU/rssPFPZ9RWGhp0rXTr+E/n+YHyzY8lG0qS0FrQo/zFKl\n\tzNNeJL3LBPPsoasqJ8e+YGtrM7gGTKIWSki/gYk0=","Date":"Thu, 23 May 2019 12:48:09 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190523094809.GI4745@pendragon.ideasonboard.com>","References":"<20190523005534.9631-1-niklas.soderlund@ragnatech.se>\n\t<20190523005534.9631-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190523005534.9631-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","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, 23 May 2019 09:48:32 -0000"}},{"id":1675,"web_url":"https://patchwork.libcamera.org/comment/1675/","msgid":"<65645956-6eb2-5d0f-355d-5a1cf4e13e50@ideasonboard.com>","date":"2019-05-23T09:52:13","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 23/05/2019 01:55, Niklas Söderlund wrote:\n> Add more structure to main.cpp by breaking up the logic into a CamApp\n> class. This makes the code easier to read and removes all but one of the\n> organically grown global variables.\n\nWhich one is left :-) (Perhaps I'll find it in the code below, but it\nmight be nice to reference it here separately for clarity)\n\nIn the diff I see -options, -camera, -loop, and +app.\nIs it the +app you are referring to?\n\n> \n> Signed-off-by: Niklas Söderlund \n\nI'm struggling to find comments for this patch... (so I'm sure that's a\ngood thing) I could bikeshed the CamApp class name, but I won't :-D\n\nThe rest is mostly code move which should stay as minimal change as\npossible IMO for now so:\n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/cam/main.cpp | 171 +++++++++++++++++++++++++++++++----------------\n> 1 file changed, 112 insertions(+), 59 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index fe7d4f90dbf14ffd..5ca8356025a0c9f9 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -18,17 +18,101 @@\n> \n> using namespace libcamera;\n> \n> -OptionsParser::Options options;\n> -std::shared_ptr camera;\n> -EventLoop *loop;\n> +class CamApp\n> +{\n> +public:\n> +\tCamApp();\n> +\n> +\tint init(int argc, char **argv);\n> +\tvoid cleanup();\n> +\n> +\tint run();\n> +\n> +\tEventLoop *loop;\n\nI see we're accessing the loop directly from the signal handler.\n\nWe could add an inline exitLoop() to make this private as the other\nvariables:\n\n\tvoid exitLoop() { if (loop_) loop_->exit(); }\n\nOr it could be CamApp::exit() too ...\nBut I won't object loudly if you just want to keep it public for ease?\n\n\nOtherwise could do with an empty line here to separate public/private\nsections?\n\n> +private:\n> +\tint parseOptions(int argc, char *argv[]);\n> +\n> +\tOptionsParser::Options options_;\n> +\tCameraManager *cm_;\n> +\tstd::shared_ptr camera_;\n> +};\n> +\n> +CamApp::CamApp()\n> +\t: cm_(nullptr), camera_(nullptr)\n> +{\n> +}\n> +\n> +int CamApp::init(int argc, char **argv)\n> +{\n> +\tint ret;\n> +\n> +\tret = parseOptions(argc, argv);\n> +\tif (ret < 0)\n> +\t\treturn ret == -EINTR ? 0 : ret;\n> +\n> +\tcm_ = CameraManager::instance();\n> +\n> +\tret = cm_->start();\n> +\tif (ret) {\n> +\t\tstd::cout << \"Failed to start camera manager: \"\n> +\t\t\t << strerror(-ret) << std::endl;\n\nNot for this patch, but it would be really nice to have some logging\nhelpers, that can easily be shared across our apps and tests...\n\ndo we have a task created for that anywhere I wonder ?\n\n> +\t\treturn ret;\n> +\t}\n> \n> -void signalHandler(int signal)\n> +\tif (options_.isSet(OptCamera)) {\n> +\t\tcamera_ = cm_->get(options_[OptCamera]);\n> +\t\tif (!camera_) {\n> +\t\t\tstd::cout << \"Camera \"\n> +\t\t\t\t << std::string(options_[OptCamera])\n> +\t\t\t\t << \" not found\" << std::endl;\n> +\t\t\tcm_->stop();\n> +\t\t\treturn -ENODEV;\n> +\t\t}\n> +\n> +\t\tif (camera_->acquire()) {\n> +\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> +\t\t\tcamera_.reset();\n> +\t\t\tcm_->stop();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tstd::cout << \"Using camera \" << camera_->name() << std::endl;\n> +\t}\n> +\n> +\tloop = new EventLoop(cm_->eventDispatcher());\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CamApp::cleanup()\n> {\n> -\tstd::cout << \"Exiting\" << std::endl;\n> -\tloop->exit();\n> +\tdelete loop;\n> +\n> +\tif (camera_) {\n> +\t\tcamera_->release();\n> +\t\tcamera_.reset();\n> +\t}\n> +\n> +\tcm_->stop();\n> +}\n> +\n> +int CamApp::run()\n> +{\n> +\tif (options_.isSet(OptList)) {\n> +\t\tstd::cout << \"Available cameras:\" << std::endl;\n> +\t\tfor (const std::shared_ptr &cam : cm_->cameras())\n> +\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> +\t}\n> +\n> +\tif (options_.isSet(OptCapture)) {\n> +\t\tCapture capture;\n> +\t\treturn capture.run(camera_.get(), loop, options_);\n> +\t}\n> +\n> +\treturn 0;\n> }\n> \n> -static int parseOptions(int argc, char *argv[])\n> +int CamApp::parseOptions(int argc, char *argv[])\n> {\n> \tKeyValueParser streamKeyValue;\n> \tstreamKeyValue.addOption(\"role\", OptionString,\n> @@ -58,77 +142,46 @@ static int parseOptions(int argc, char *argv[])\n> \t\t\t \"help\");\n> \tparser.addOption(OptList, OptionNone, \"List all cameras\", \"list\");\n> \n> -\toptions = parser.parse(argc, argv);\n> -\tif (!options.valid())\n> +\toptions_ = parser.parse(argc, argv);\n> +\tif (!options_.valid())\n> \t\treturn -EINVAL;\n> \n> -\tif (options.empty() || options.isSet(OptHelp)) {\n> +\tif (options_.empty() || options_.isSet(OptHelp)) {\n> \t\tparser.usage();\n> -\t\treturn options.empty() ? -EINVAL : -EINTR;\n> +\t\treturn options_.empty() ? -EINVAL : -EINTR;\n> \t}\n> \n> \treturn 0;\n> }\n> \n> +CamApp app;\n> +\n> +void signalHandler(int signal)\n> +{\n> +\tstd::cout << \"Exiting\" << std::endl;\n> +\n> +\tif (app.loop)\n> +\t\tapp.loop->exit();\n> +}\n> +\n> int main(int argc, char **argv)\n> {\n> \tint ret;\n> \n> -\tret = parseOptions(argc, argv);\n> -\tif (ret < 0)\n> -\t\treturn ret == -EINTR ? 0 : EXIT_FAILURE;\n> -\n> -\tCameraManager *cm = CameraManager::instance();\n> -\n> -\tret = cm->start();\n> -\tif (ret) {\n> -\t\tstd::cout << \"Failed to start camera manager: \"\n> -\t\t\t << strerror(-ret) << std::endl;\n> +\tret = app.init(argc, argv);\n> +\tif (ret)\n> \t\treturn EXIT_FAILURE;\n> -\t}\n> -\n> -\tloop = new EventLoop(cm->eventDispatcher());\n> \n> \tstruct sigaction sa = {};\n> \tsa.sa_handler = &signalHandler;\n> \tsigaction(SIGINT, &sa, nullptr);\n> \n> -\tif (options.isSet(OptList)) {\n> -\t\tstd::cout << \"Available cameras:\" << std::endl;\n> -\t\tfor (const std::shared_ptr &cam : cm->cameras())\n> -\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> -\t}\n> +\tret = app.run();\n> \n> -\tif (options.isSet(OptCamera)) {\n> -\t\tcamera = cm->get(options[OptCamera]);\n> -\t\tif (!camera) {\n> -\t\t\tstd::cout << \"Camera \"\n> -\t\t\t\t << std::string(options[OptCamera])\n> -\t\t\t\t << \" not found\" << std::endl;\n> -\t\t\tgoto out;\n> -\t\t}\n> +\tapp.cleanup();\n> \n> -\t\tif (camera->acquire()) {\n> -\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> -\t\t\tgoto out;\n> -\t\t}\n> +\tif (ret)\n> +\t\treturn EXIT_FAILURE;\n> \n> -\t\tstd::cout << \"Using camera \" << camera->name() << std::endl;\n> -\t}\n> -\n> -\tif (options.isSet(OptCapture)) {\n> -\t\tCapture capture;\n> -\t\tret = capture.run(camera.get(), loop, options);\n> -\t}\n> -\n> -\tif (camera) {\n> -\t\tcamera->release();\n> -\t\tcamera.reset();\n> -\t}\n> -out:\n> -\tdelete loop;\n> -\n> -\tcm->stop();\n> -\n> -\treturn ret;\n> +\treturn 0;\n> }\n>","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 34B9860003\n\tfor ;\n\tThu, 23 May 2019 11:52:18 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 989B7334;\n\tThu, 23 May 2019 11:52:16 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558605136;\n\tbh=TPr+r6Z+prqlY7h0KdjSBFLRrRe0RxOA7a6Q8HCDav4=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=rdA3/r2a4uZHHoBeB7bjd5kXJrw56GOMFJDSJ5DQtCJH6+uwwgONP6//3c8B+Qj95\n\t8GsnWJswy+imqlmlGvmtjoG/tq2i0TLLawvougdbnueOssCI48VNZMxleG8iviAZuE\n\tGMDTbvOfQf9KMIANq5gfXMkt4U+qYqEry9QVCvSA=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190523005534.9631-1-niklas.soderlund@ragnatech.se>\n\t<20190523005534.9631-3-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<65645956-6eb2-5d0f-355d-5a1cf4e13e50@ideasonboard.com>","Date":"Thu, 23 May 2019 10:52:13 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.6.1","MIME-Version":"1.0","In-Reply-To":"<20190523005534.9631-3-niklas.soderlund@ragnatech.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","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, 23 May 2019 09:52:18 -0000"}},{"id":1684,"web_url":"https://patchwork.libcamera.org/comment/1684/","msgid":"<20190523150714.GI1651@bigcity.dyn.berto.se>","date":"2019-05-23T15:07:14","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","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 feedback.\n\nOn 2019-05-23 12:48:09 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Thu, May 23, 2019 at 02:55:34AM +0200, Niklas Söderlund wrote:\n> > Add more structure to main.cpp by breaking up the logic into a CamApp\n> > class. This makes the code easier to read and removes all but one of the\n> > organically grown global variables.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/main.cpp | 171 +++++++++++++++++++++++++++++++----------------\n> > 1 file changed, 112 insertions(+), 59 deletions(-)\n> > \n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index fe7d4f90dbf14ffd..5ca8356025a0c9f9 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -18,17 +18,101 @@\n> > \n> > using namespace libcamera;\n> > \n> > -OptionsParser::Options options;\n> > -std::shared_ptr camera;\n> > -EventLoop *loop;\n> > +class CamApp\n> > +{\n> > +public:\n> > +\tCamApp();\n> > +\n> > +\tint init(int argc, char **argv);\n> \n> I would pass the argc and argv to the constructor, following the\n> https://doc.qt.io/qt-5/qapplication.html API. You can either store them\n> internally and parse the options in init(), or do part of the\n> initialisation in the constructor and stored a valid state that would\n> make init() return an error immediately. The global app variable would\n> then become a pointer, or possibly even better, you could add a static\n> method to CamApp() named instance() that would return the instance\n> (stored in a static member variable in the constructor), and remove the\n> last global variable :-) The signal handler would call\n> CamApp::instance()->quit().\n\nI like quit() and instance() and will do so in v2.\n\nI do not particularly like combining instance() with moving arg{v,c} to \nto the constructor and caching them, so I will try without this in v2 \n;-)\n\n> \n> > +\tvoid cleanup();\n> > +\n> > +\tint run();\n> \n> I would call this exec() to mimic Qt to.\n> \n> > +\n> > +\tEventLoop *loop;\n> \n> Missing blank line ?\n> \n> > +private:\n> > +\tint parseOptions(int argc, char *argv[]);\n> > +\n> > +\tOptionsParser::Options options_;\n> > +\tCameraManager *cm_;\n> > +\tstd::shared_ptr camera_;\n> > +};\n> > +\n> > +CamApp::CamApp()\n> > +\t: cm_(nullptr), camera_(nullptr)\n> > +{\n> > +}\n> > +\n> > +int CamApp::init(int argc, char **argv)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tret = parseOptions(argc, argv);\n> > +\tif (ret < 0)\n> > +\t\treturn ret == -EINTR ? 0 : ret;\n> > +\n> > +\tcm_ = CameraManager::instance();\n> > +\n> > +\tret = cm_->start();\n> > +\tif (ret) {\n> > +\t\tstd::cout << \"Failed to start camera manager: \"\n> > +\t\t\t << strerror(-ret) << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > \n> > -void signalHandler(int signal)\n> > +\tif (options_.isSet(OptCamera)) {\n> > +\t\tcamera_ = cm_->get(options_[OptCamera]);\n> > +\t\tif (!camera_) {\n> > +\t\t\tstd::cout << \"Camera \"\n> > +\t\t\t\t << std::string(options_[OptCamera])\n> > +\t\t\t\t << \" not found\" << std::endl;\n> > +\t\t\tcm_->stop();\n> > +\t\t\treturn -ENODEV;\n> > +\t\t}\n> > +\n> > +\t\tif (camera_->acquire()) {\n> > +\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > +\t\t\tcamera_.reset();\n> > +\t\t\tcm_->stop();\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tstd::cout << \"Using camera \" << camera_->name() << std::endl;\n> > +\t}\n> > +\n> > +\tloop = new EventLoop(cm_->eventDispatcher());\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void CamApp::cleanup()\n> > {\n> > -\tstd::cout << \"Exiting\" << std::endl;\n> > -\tloop->exit();\n> > +\tdelete loop;\n> > +\n> > +\tif (camera_) {\n> > +\t\tcamera_->release();\n> > +\t\tcamera_.reset();\n> > +\t}\n> > +\n> > +\tcm_->stop();\n> > +}\n> > +\n> > +int CamApp::run()\n> > +{\n> > +\tif (options_.isSet(OptList)) {\n> > +\t\tstd::cout << \"Available cameras:\" << std::endl;\n> > +\t\tfor (const std::shared_ptr &cam : cm_->cameras())\n> > +\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> > +\t}\n> > +\n> > +\tif (options_.isSet(OptCapture)) {\n> > +\t\tCapture capture;\n> > +\t\treturn capture.run(camera_.get(), loop, options_);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > }\n> > \n> > -static int parseOptions(int argc, char *argv[])\n> > +int CamApp::parseOptions(int argc, char *argv[])\n> > {\n> > \tKeyValueParser streamKeyValue;\n> > \tstreamKeyValue.addOption(\"role\", OptionString,\n> > @@ -58,77 +142,46 @@ static int parseOptions(int argc, char *argv[])\n> > \t\t\t \"help\");\n> > \tparser.addOption(OptList, OptionNone, \"List all cameras\", \"list\");\n> > \n> > -\toptions = parser.parse(argc, argv);\n> > -\tif (!options.valid())\n> > +\toptions_ = parser.parse(argc, argv);\n> > +\tif (!options_.valid())\n> > \t\treturn -EINVAL;\n> > \n> > -\tif (options.empty() || options.isSet(OptHelp)) {\n> > +\tif (options_.empty() || options_.isSet(OptHelp)) {\n> > \t\tparser.usage();\n> > -\t\treturn options.empty() ? -EINVAL : -EINTR;\n> > +\t\treturn options_.empty() ? -EINVAL : -EINTR;\n> > \t}\n> > \n> > \treturn 0;\n> > }\n> > \n> > +CamApp app;\n> > +\n> > +void signalHandler(int signal)\n> > +{\n> > +\tstd::cout << \"Exiting\" << std::endl;\n> > +\n> > +\tif (app.loop)\n> > +\t\tapp.loop->exit();\n> \n> Instead of exposing the loop member, how about adding a public quit()\n> method that would call loop->exit() ? loop should then become loop_.\n> \n> > +}\n> > +\n> > int main(int argc, char **argv)\n> > {\n> > \tint ret;\n> > \n> > -\tret = parseOptions(argc, argv);\n> > -\tif (ret < 0)\n> > -\t\treturn ret == -EINTR ? 0 : EXIT_FAILURE;\n> > -\n> > -\tCameraManager *cm = CameraManager::instance();\n> > -\n> > -\tret = cm->start();\n> > -\tif (ret) {\n> > -\t\tstd::cout << \"Failed to start camera manager: \"\n> > -\t\t\t << strerror(-ret) << std::endl;\n> > +\tret = app.init(argc, argv);\n> > +\tif (ret)\n> > \t\treturn EXIT_FAILURE;\n> > -\t}\n> > -\n> > -\tloop = new EventLoop(cm->eventDispatcher());\n> > \n> > \tstruct sigaction sa = {};\n> > \tsa.sa_handler = &signalHandler;\n> > \tsigaction(SIGINT, &sa, nullptr);\n> > \n> > -\tif (options.isSet(OptList)) {\n> > -\t\tstd::cout << \"Available cameras:\" << std::endl;\n> > -\t\tfor (const std::shared_ptr &cam : cm->cameras())\n> > -\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> > -\t}\n> > +\tret = app.run();\n> > \n> > -\tif (options.isSet(OptCamera)) {\n> > -\t\tcamera = cm->get(options[OptCamera]);\n> > -\t\tif (!camera) {\n> > -\t\t\tstd::cout << \"Camera \"\n> > -\t\t\t\t << std::string(options[OptCamera])\n> > -\t\t\t\t << \" not found\" << std::endl;\n> > -\t\t\tgoto out;\n> > -\t\t}\n> > +\tapp.cleanup();\n> \n> Should cleanup() be called internally at the end of run() ?\n\nNo, the reason for having it separate is to make error handling in run() \nsimpler\n\n> \n> > \n> > -\t\tif (camera->acquire()) {\n> > -\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > -\t\t\tgoto out;\n> > -\t\t}\n> > +\tif (ret)\n> > +\t\treturn EXIT_FAILURE;\n> > \n> > -\t\tstd::cout << \"Using camera \" << camera->name() << std::endl;\n> > -\t}\n> > -\n> > -\tif (options.isSet(OptCapture)) {\n> > -\t\tCapture capture;\n> > -\t\tret = capture.run(camera.get(), loop, options);\n> > -\t}\n> > -\n> > -\tif (camera) {\n> > -\t\tcamera->release();\n> > -\t\tcamera.reset();\n> > -\t}\n> > -out:\n> > -\tdelete loop;\n> > -\n> > -\tcm->stop();\n> > -\n> > -\treturn ret;\n> > +\treturn 0;\n> > }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17E0160B1B\n\tfor ;\n\tThu, 23 May 2019 17:07:16 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id j24so5826143ljg.1\n\tfor ;\n\tThu, 23 May 2019 08:07:16 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tq11sm6105783lfb.89.2019.05.23.08.07.14\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 23 May 2019 08:07:14 -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=m6VYywr6A3bbvywIHx8lc2QqA7q9FrjNnMNr3cODkwU=;\n\tb=sDe4YeziK8GltX+0BleObLH3mwnyHt4QKNDNJkou9v3I/7/2tSc+gfdxLK0QkozAAu\n\tGQRSItszFgXLAkVw8LhJscTYZJFjmBQKEXAnOZy/y2c4jNrikLQfTZaJDpX4i+wHJuM6\n\tQ2kTRXUdAQcdUBosLMoy5FYWNN3cu2Ph+7bp88AVlomqiRJuD+V9cMUvnUXmVq4LnT9x\n\tZ+e949CTdonnu+27uq9oi+OIb+GkQ0aRuBLkNYIeX04eQTznwvl9/KyDW4loFEbNkZE6\n\tJ/yCTyxGaA7bNiqDps2MqspzBtcSVJmxX8Fklc+pxV5JLUSdtLTtCQChQyvPQmu5rIUV\n\tbb0g==","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=m6VYywr6A3bbvywIHx8lc2QqA7q9FrjNnMNr3cODkwU=;\n\tb=da1XATIblrxg27rEYj86oaIq4NIVQzjQT/xy2ksM4sYPua+B2kcqGPyhFc6O2C2Cpi\n\tWLc6IUQu+o8HNIbcgbRxrYWwVTmOf12Bik2Aqrg64ZgCPH2dh2hFIGIWGOchXwWTEoOI\n\tOxcMi9dZu9FUdr992KbBwfQxpy6yZHY8ykx5lBedQqq5j57r2TNEXSNgIG/R05UAPI1/\n\tiyxiKAm3C/szacxwM7Mn+LAge7bNP/ZllJ/rV0aqqN0K6GrgGWuHbUgr5dM7ZISoWgaQ\n\t75ORba56Tlg+AZ+hiW1mslq4MJGv91Zn82XhJCCRGRqJBbPjFxQ2l5AAjbrvH7u+k7zY\n\tlX+Q==","X-Gm-Message-State":"APjAAAXcLAQ5SywdQ516cY4NtU3YxqdnPNhwINNPsWWHfsiIH9svcCQX\n\t8ZePV3W6KjSI0eqI29GFf3KoYA==","X-Google-Smtp-Source":"APXvYqx1uKVZzHGvplfCgZietKOuiqYKjlbTv7v6I6KahGJIAjqXf7aG2wSiEBf+VlFE1crxQ99NlQ==","X-Received":"by 2002:a2e:206:: with SMTP id 6mr46347662ljc.59.1558624035234; \n\tThu, 23 May 2019 08:07:15 -0700 (PDT)","Date":"Thu, 23 May 2019 17:07:14 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190523150714.GI1651@bigcity.dyn.berto.se>","References":"<20190523005534.9631-1-niklas.soderlund@ragnatech.se>\n\t<20190523005534.9631-3-niklas.soderlund@ragnatech.se>\n\t<20190523094809.GI4745@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190523094809.GI4745@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.4 (2019-03-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","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, 23 May 2019 15:07:16 -0000"}},{"id":1685,"web_url":"https://patchwork.libcamera.org/comment/1685/","msgid":"<20190523151346.GN4745@pendragon.ideasonboard.com>","date":"2019-05-23T15:13:46","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Thu, May 23, 2019 at 05:07:14PM +0200, Niklas Söderlund wrote:\n> On 2019-05-23 12:48:09 +0300, Laurent Pinchart wrote:\n> > On Thu, May 23, 2019 at 02:55:34AM +0200, Niklas Söderlund wrote:\n> > > Add more structure to main.cpp by breaking up the logic into a CamApp\n> > > class. This makes the code easier to read and removes all but one of the\n> > > organically grown global variables.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/cam/main.cpp | 171 +++++++++++++++++++++++++++++++----------------\n> > > 1 file changed, 112 insertions(+), 59 deletions(-)\n> > > \n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index fe7d4f90dbf14ffd..5ca8356025a0c9f9 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -18,17 +18,101 @@\n> > > \n> > > using namespace libcamera;\n> > > \n> > > -OptionsParser::Options options;\n> > > -std::shared_ptr camera;\n> > > -EventLoop *loop;\n> > > +class CamApp\n> > > +{\n> > > +public:\n> > > +\tCamApp();\n> > > +\n> > > +\tint init(int argc, char **argv);\n> > \n> > I would pass the argc and argv to the constructor, following the\n> > https://doc.qt.io/qt-5/qapplication.html API. You can either store them\n> > internally and parse the options in init(), or do part of the\n> > initialisation in the constructor and stored a valid state that would\n> > make init() return an error immediately. The global app variable would\n> > then become a pointer, or possibly even better, you could add a static\n> > method to CamApp() named instance() that would return the instance\n> > (stored in a static member variable in the constructor), and remove the\n> > last global variable :-) The signal handler would call\n> > CamApp::instance()->quit().\n> \n> I like quit() and instance() and will do so in v2.\n> \n> I do not particularly like combining instance() with moving arg{v,c} to \n> to the constructor and caching them, so I will try without this in v2 \n> ;-)\n> \n> > > +\tvoid cleanup();\n> > > +\n> > > +\tint run();\n> > \n> > I would call this exec() to mimic Qt to.\n> > \n> > > +\n> > > +\tEventLoop *loop;\n> > \n> > Missing blank line ?\n> > \n> > > +private:\n> > > +\tint parseOptions(int argc, char *argv[]);\n> > > +\n> > > +\tOptionsParser::Options options_;\n> > > +\tCameraManager *cm_;\n> > > +\tstd::shared_ptr camera_;\n> > > +};\n> > > +\n> > > +CamApp::CamApp()\n> > > +\t: cm_(nullptr), camera_(nullptr)\n> > > +{\n> > > +}\n> > > +\n> > > +int CamApp::init(int argc, char **argv)\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\tret = parseOptions(argc, argv);\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret == -EINTR ? 0 : ret;\n> > > +\n> > > +\tcm_ = CameraManager::instance();\n> > > +\n> > > +\tret = cm_->start();\n> > > +\tif (ret) {\n> > > +\t\tstd::cout << \"Failed to start camera manager: \"\n> > > +\t\t\t << strerror(-ret) << std::endl;\n> > > +\t\treturn ret;\n> > > +\t}\n> > > \n> > > -void signalHandler(int signal)\n> > > +\tif (options_.isSet(OptCamera)) {\n> > > +\t\tcamera_ = cm_->get(options_[OptCamera]);\n> > > +\t\tif (!camera_) {\n> > > +\t\t\tstd::cout << \"Camera \"\n> > > +\t\t\t\t << std::string(options_[OptCamera])\n> > > +\t\t\t\t << \" not found\" << std::endl;\n> > > +\t\t\tcm_->stop();\n> > > +\t\t\treturn -ENODEV;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (camera_->acquire()) {\n> > > +\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > > +\t\t\tcamera_.reset();\n> > > +\t\t\tcm_->stop();\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tstd::cout << \"Using camera \" << camera_->name() << std::endl;\n> > > +\t}\n> > > +\n> > > +\tloop = new EventLoop(cm_->eventDispatcher());\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void CamApp::cleanup()\n> > > {\n> > > -\tstd::cout << \"Exiting\" << std::endl;\n> > > -\tloop->exit();\n> > > +\tdelete loop;\n> > > +\n> > > +\tif (camera_) {\n> > > +\t\tcamera_->release();\n> > > +\t\tcamera_.reset();\n> > > +\t}\n> > > +\n> > > +\tcm_->stop();\n> > > +}\n> > > +\n> > > +int CamApp::run()\n> > > +{\n> > > +\tif (options_.isSet(OptList)) {\n> > > +\t\tstd::cout << \"Available cameras:\" << std::endl;\n> > > +\t\tfor (const std::shared_ptr &cam : cm_->cameras())\n> > > +\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> > > +\t}\n> > > +\n> > > +\tif (options_.isSet(OptCapture)) {\n> > > +\t\tCapture capture;\n> > > +\t\treturn capture.run(camera_.get(), loop, options_);\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > }\n> > > \n> > > -static int parseOptions(int argc, char *argv[])\n> > > +int CamApp::parseOptions(int argc, char *argv[])\n> > > {\n> > > \tKeyValueParser streamKeyValue;\n> > > \tstreamKeyValue.addOption(\"role\", OptionString,\n> > > @@ -58,77 +142,46 @@ static int parseOptions(int argc, char *argv[])\n> > > \t\t\t \"help\");\n> > > \tparser.addOption(OptList, OptionNone, \"List all cameras\", \"list\");\n> > > \n> > > -\toptions = parser.parse(argc, argv);\n> > > -\tif (!options.valid())\n> > > +\toptions_ = parser.parse(argc, argv);\n> > > +\tif (!options_.valid())\n> > > \t\treturn -EINVAL;\n> > > \n> > > -\tif (options.empty() || options.isSet(OptHelp)) {\n> > > +\tif (options_.empty() || options_.isSet(OptHelp)) {\n> > > \t\tparser.usage();\n> > > -\t\treturn options.empty() ? -EINVAL : -EINTR;\n> > > +\t\treturn options_.empty() ? -EINVAL : -EINTR;\n> > > \t}\n> > > \n> > > \treturn 0;\n> > > }\n> > > \n> > > +CamApp app;\n> > > +\n> > > +void signalHandler(int signal)\n> > > +{\n> > > +\tstd::cout << \"Exiting\" << std::endl;\n> > > +\n> > > +\tif (app.loop)\n> > > +\t\tapp.loop->exit();\n> > \n> > Instead of exposing the loop member, how about adding a public quit()\n> > method that would call loop->exit() ? loop should then become loop_.\n> > \n> > > +}\n> > > +\n> > > int main(int argc, char **argv)\n> > > {\n> > > \tint ret;\n> > > \n> > > -\tret = parseOptions(argc, argv);\n> > > -\tif (ret < 0)\n> > > -\t\treturn ret == -EINTR ? 0 : EXIT_FAILURE;\n> > > -\n> > > -\tCameraManager *cm = CameraManager::instance();\n> > > -\n> > > -\tret = cm->start();\n> > > -\tif (ret) {\n> > > -\t\tstd::cout << \"Failed to start camera manager: \"\n> > > -\t\t\t << strerror(-ret) << std::endl;\n> > > +\tret = app.init(argc, argv);\n> > > +\tif (ret)\n> > > \t\treturn EXIT_FAILURE;\n> > > -\t}\n> > > -\n> > > -\tloop = new EventLoop(cm->eventDispatcher());\n> > > \n> > > \tstruct sigaction sa = {};\n> > > \tsa.sa_handler = &signalHandler;\n> > > \tsigaction(SIGINT, &sa, nullptr);\n> > > \n> > > -\tif (options.isSet(OptList)) {\n> > > -\t\tstd::cout << \"Available cameras:\" << std::endl;\n> > > -\t\tfor (const std::shared_ptr &cam : cm->cameras())\n> > > -\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> > > -\t}\n> > > +\tret = app.run();\n> > > \n> > > -\tif (options.isSet(OptCamera)) {\n> > > -\t\tcamera = cm->get(options[OptCamera]);\n> > > -\t\tif (!camera) {\n> > > -\t\t\tstd::cout << \"Camera \"\n> > > -\t\t\t\t << std::string(options[OptCamera])\n> > > -\t\t\t\t << \" not found\" << std::endl;\n> > > -\t\t\tgoto out;\n> > > -\t\t}\n> > > +\tapp.cleanup();\n> > \n> > Should cleanup() be called internally at the end of run() ?\n> \n> No, the reason for having it separate is to make error handling in run() \n> simpler\n\nLet's make the caller simple too, and add an exec() that calls run() and\ncleanup() :-)\n\n> > > \n> > > -\t\tif (camera->acquire()) {\n> > > -\t\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> > > -\t\t\tgoto out;\n> > > -\t\t}\n> > > +\tif (ret)\n> > > +\t\treturn EXIT_FAILURE;\n> > > \n> > > -\t\tstd::cout << \"Using camera \" << camera->name() << std::endl;\n> > > -\t}\n> > > -\n> > > -\tif (options.isSet(OptCapture)) {\n> > > -\t\tCapture capture;\n> > > -\t\tret = capture.run(camera.get(), loop, options);\n> > > -\t}\n> > > -\n> > > -\tif (camera) {\n> > > -\t\tcamera->release();\n> > > -\t\tcamera.reset();\n> > > -\t}\n> > > -out:\n> > > -\tdelete loop;\n> > > -\n> > > -\tcm->stop();\n> > > -\n> > > -\treturn ret;\n> > > +\treturn 0;\n> > > }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41CE360B1B\n\tfor ;\n\tThu, 23 May 2019 17:14:04 +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 B79C1583;\n\tThu, 23 May 2019 17:14:03 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558624443;\n\tbh=DCzghXEpH+Cm1WI6jD9b+W1sYSkjOtqWgw8F3G1auUE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QpQUADm4lw5WM+WuLVW1f48BKvzvJopcx9mQmrj6e7CGW+ocV2ZCC0EJPojupoMi+\n\tntx8v0bR24VuqeeijG2eVkmtexMsoYREM1bXKMPUNViF9++C9Yb1+NvnZYOmjhjWvi\n\tbH34UmRIr6hzn41rOFLOgNjq7/gfPdO3FhqAyCgs=","Date":"Thu, 23 May 2019 18:13:46 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190523151346.GN4745@pendragon.ideasonboard.com>","References":"<20190523005534.9631-1-niklas.soderlund@ragnatech.se>\n\t<20190523005534.9631-3-niklas.soderlund@ragnatech.se>\n\t<20190523094809.GI4745@pendragon.ideasonboard.com>\n\t<20190523150714.GI1651@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190523150714.GI1651@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Add CamApp class","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, 23 May 2019 15:14:04 -0000"}}]