[{"id":12334,"web_url":"https://patchwork.libcamera.org/comment/12334/","msgid":"<20200906015330.GG25630@pendragon.ideasonboard.com>","date":"2020-09-06T01:53:30","subject":"Re: [libcamera-devel] [PATCH v5 4/4] qcam: add additional command\n\tline option to select the render type","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Show,\n\nThank you for the patch.\n\nOn Fri, Sep 04, 2020 at 04:43:16PM +0800, Show Liu wrote:\n> Add new option \"--render=qt|gles\" to select the render type,\n> \"--render=gles\" to accelerate format conversion and rendering\n> \"--render=qt\" is the original Qt rendering\n> \n> Signed-off-by: Show Liu \n> ---\n> src/qcam/main.cpp | 3 +++\n> src/qcam/main_window.cpp | 29 ++++++++++++++++++++++++-----\n> src/qcam/main_window.h | 6 ++++++\n> 3 files changed, 33 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp\n> index bae358d..19d0559 100644\n> --- a/src/qcam/main.cpp\n> +++ b/src/qcam/main.cpp\n> @@ -35,6 +35,9 @@ OptionsParser::Options parseOptions(int argc, char *argv[])\n> \t\t\t \"help\");\n> \tparser.addOption(OptStream, &streamKeyValue,\n> \t\t\t \"Set configuration of a camera stream\", \"stream\", true);\n> +\tparser.addOption(OptRender, OptionString,\n\nShould this be named OptRendered as we're selecting a renderer ?\n\n> +\t\t\t \"Choose the render type use qt|gles\", \"render\",\n\n\"Choose the renderer type {qt,gles} (default: qt)\", \"renderer\"\n\n> +\t\t\t ArgumentRequired, \"render\");\n\nLet's keep options sorted alphabetically.\n\n> \n> \tOptionsParser::Options options = parser.parse(argc, argv);\n> \tif (options.isSet(OptHelp))\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 612d978..467cc74 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -91,7 +91,7 @@ private:\n> \n> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> \t: saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr),\n> -\t isCapturing_(false), captureRaw_(false)\n> +\t isCapturing_(false), captureRaw_(false), renderType_(\"qt\")\n> {\n> \tint ret;\n> \n> @@ -105,10 +105,29 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> \tsetWindowTitle(title_);\n> \tconnect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));\n> \n> -\tviewfinder_ = new ViewFinder(this);\n> -\tconnect(viewfinder_, &ViewFinder::renderComplete,\n> -\t\tthis, &MainWindow::queueRequest);\n> -\tsetCentralWidget(viewfinder_);\n> +\tif (options_.isSet(OptRender))\n> +\t\trenderType_ = options_[OptRender].toString().c_str();\n\nNo need to convert the option to a C string to then convert it back to a\nstd::string :-)\n\nA note mostly for myself, it would be nice to add support for default\nvalues to the OptionParser class, so that we could specify the default\nas \"qt\" when creating the parser, and just do\n\n\tstd::string renderType = options_[OptRender].toString();\n\nhere. And adding support for pre-defined choices would be good too, so\nthat the parser would catch invalid values.\n\n> +\n> +\tif (renderType_ == \"qt\") {\n> +\t\tviewfinder_ = new ViewFinderQt(this);\n> +\t\tViewFinderQt *vf = dynamic_cast(viewfinder_);\n\nI'd do it the other way around,\n\n\t\tViewFinderQt *vf = new ViewFinderQt(this);\n\t\tviewfinder_ = vf;\n\nso you can avoid the dynamic_cast.\n\n> +\t\tconnect(vf, &ViewFinderQt::renderComplete,\n> +\t\t\tthis, &MainWindow::queueRequest);\n> +\t\tsetCentralWidget(vf);\n> +\t} else if (renderType_ == \"gles\") {\n> +\t\tviewfinder_ = new ViewFinderGL(this);\n> +\t\tViewFinderGL *vf = dynamic_cast(viewfinder_);\n> +\t\tconnect(vf, &ViewFinderGL::renderComplete,\n> +\t\t\tthis, &MainWindow::queueRequest);\n> +\t\tsetCentralWidget(vf);\n> +\t} else {\n> +\t\tqWarning() << \"Render Type:\"\n\ns/Type:/type/\n\n> +\t\t\t << QString::fromStdString(renderType_)\n> +\t\t\t << \" is not support.\";\n\ns/ is/is/ (as Qt inserts spaces automatically)\ns/support/supported/\n\nOr maybe\n\n\t\tqWarning() << \"Invalid render type\"\n\t\t\t << QString::fromStdString(renderType_);\n\n?\n\n> +\t\tquit();\n> +\t\treturn;\n> +\t}\n> +\n> \tadjustSize();\n> \n> \t/* Hotplug/unplug support */\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 3d21779..a69b399 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -26,6 +26,8 @@\n> \n> #include \"../cam/stream_options.h\"\n> #include \"viewfinder.h\"\n> +#include \"viewfinder_gl.h\"\n> +#include \"viewfinder_qt.h\"\n\nThis is not needed in main_window.h, you can move it to main_window.cpp.\n\n> \n> using namespace libcamera;\n> \n> @@ -38,6 +40,7 @@ enum {\n> \tOptCamera = 'c',\n> \tOptHelp = 'h',\n> \tOptStream = 's',\n> +\tOptRender = 'r',\n\nLet's keep options sorted alphabetically here too.\n\n> };\n> \n> class CaptureRequest\n> @@ -134,6 +137,9 @@ private:\n> \tQElapsedTimer frameRateInterval_;\n> \tuint32_t previousFrames_;\n> \tuint32_t framesCaptured_;\n> +\n> +\t/* Render Type Qt or GLES */\n> +\tstd::string renderType_;\n\nThis is only used in MainWindow::MainWindow(), it can be a local\nvariable.\n\nWith these small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> };\n> \n> #endif /* __QCAM_MAIN_WINDOW__ */","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 2CEDBBDB1C\n\tfor ;\n\tSun, 6 Sep 2020 01:53:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC9F5629B6;\n\tSun, 6 Sep 2020 03:53:56 +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 6249761EA0\n\tfor ;\n\tSun, 6 Sep 2020 03:53:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 899DF277;\n\tSun, 6 Sep 2020 03:53:54 +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=\"fNCWtBWg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599357234;\n\tbh=vjDjNVu+4lCv2wlm7wApDFtK+iQl1E824Rd5tniKlhg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fNCWtBWgWcB8LaN+Tp/+g3fAaP+srb+y2e9lXlxRZPdyG5ABGcj6IzJEupyaheUPi\n\to2J+xTOSpUFGKINZg39zvwXwRsZHhuvvyHbW4vmhDx4aGDz7vixI8Yy6xsyHxbPPuv\n\t24VWQWj8/Zv/Bxs952QdIDzs1TmsOUmuV5fhycuI=","Date":"Sun, 6 Sep 2020 04:53:30 +0300","From":"Laurent Pinchart ","To":"Show Liu ","Message-ID":"<20200906015330.GG25630@pendragon.ideasonboard.com>","References":"<20200904084316.7319-1-show.liu@linaro.org>\n\t<20200904084316.7319-6-show.liu@linaro.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200904084316.7319-6-show.liu@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] qcam: add additional command\n\tline option to select the render type","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]