[{"id":1687,"web_url":"https://patchwork.libcamera.org/comment/1687/","msgid":"<20190523153024.GP4745@pendragon.ideasonboard.com>","date":"2019-05-23T15:30:24","subject":"Re: [libcamera-devel] [PATCH v2 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 05:13:06PM +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 of the\n> organically grown global variables.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/cam/main.cpp | 184 ++++++++++++++++++++++++++++++++---------------\n>  1 file changed, 125 insertions(+), 59 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index fe7d4f90dbf14ffd..f7fe83aafd81b633 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -18,17 +18,117 @@\n>  \n>  using namespace libcamera;\n>  \n> -OptionsParser::Options options;\n> -std::shared_ptr<Camera> camera;\n> -EventLoop *loop;\n> +class CamApp\n> +{\n> +public:\n> +\tCamApp();\n> +\n> +\tstatic CamApp *instance();\n> +\n> +\tint init(int argc, char **argv);\n> +\tvoid cleanup();\n> +\n> +\tint exec();\n> +\tvoid quit();\n> +\n> +private:\n> +\tint parseOptions(int argc, char *argv[]);\n> +\n> +\tOptionsParser::Options options_;\n> +\tCameraManager *cm_;\n> +\tstd::shared_ptr<Camera> camera_;\n> +\tEventLoop *loop_;\n> +};\n> +\n> +CamApp::CamApp()\n> +\t: cm_(nullptr), camera_(nullptr), loop_(nullptr)\n> +{\n> +}\n> +\n> +CamApp *CamApp::instance()\n> +{\n> +\tstatic CamApp app;\n> +\treturn &app;\n\nI think it's best to make someone responsible for constructing the\ninstance, as that's where init() will need to be called. For that\nreason, I think you should add a static CamApp *app_ = nullptr member to\nCamApp, set it to this in the constructor, and return it here. The\nmain() function would then create the CamApp instance (either on the\nstack, or through new) and initialise it.\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> +\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> +\tdelete loop_;\n> +\tloop_ = nullptr;\n> +\n> +\tif (camera_) {\n> +\t\tcamera_->release();\n> +\t\tcamera_.reset();\n> +\t}\n> +\n> +\tcm_->stop();\n> +}\n> +\n> +int CamApp::exec()\n> +{\n> +\tif (options_.isSet(OptList)) {\n> +\t\tstd::cout << \"Available cameras:\" << std::endl;\n> +\t\tfor (const std::shared_ptr<Camera> &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\nHow about renaming this to run(), and implementing exec() as\n\nint CamApp::exec()\n{\n\tint ret;\n\n\tret = run();\n\tcleanup();\n\n\treturn ret;\n}\n\nWith these two comments addressed,\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \n> -void signalHandler(int signal)\n> +void CamApp::quit()\n>  {\n> -\tstd::cout << \"Exiting\" << std::endl;\n> -\tloop->exit();\n> +\tif (loop_)\n> +\t\tloop_->exit();\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 +158,43 @@ 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> +void signalHandler(int signal)\n> +{\n> +\tstd::cout << \"Exiting\" << std::endl;\n> +\tCamApp::instance()->quit();\n> +}\n> +\n>  int main(int argc, char **argv)\n>  {\n> +\tCamApp *app = CamApp::instance();\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<Camera> &cam : cm->cameras())\n> -\t\t\tstd::cout << \"- \" << cam->name() << std::endl;\n> -\t}\n> +\tret = app->exec();\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>  }","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33D0F60B1B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 23 May 2019 17:30:42 +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 9141E583;\n\tThu, 23 May 2019 17:30:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558625441;\n\tbh=ZDmZ0tuQwZYy7ydnJqDz3Lad5+SMXxLmmJEc60CCw8w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IG3RHyyROf7OAjdFIjydhA+0o/bGImt+PvMgcd6rqgHZGpU6U+/pW+sSPEqSF2eWH\n\t1CB475YbwZX/6mGcMfhkkjmOD01OiS9hHo3wkU6cGPaumWPmfk9Ra4wTiKVXAL5uTT\n\tZz99271bgEJeZnwULwZXFm3kLmfHM+TlfzHK3sqg=","Date":"Thu, 23 May 2019 18:30:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190523153024.GP4745@pendragon.ideasonboard.com>","References":"<20190523151306.22007-1-niklas.soderlund@ragnatech.se>\n\t<20190523151306.22007-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":"<20190523151306.22007-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] cam: Add CamApp class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 23 May 2019 15:30:42 -0000"}}]