[{"id":31970,"web_url":"https://patchwork.libcamera.org/comment/31970/","msgid":"<20241030114233.GD17733@pendragon.ideasonboard.com>","date":"2024-10-30T11:42:33","subject":"Re: [PATCH] qcam: Use pointer when choosing camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stanislaw,\n\nThank you for the patch.\n\nOn Wed, Oct 30, 2024 at 08:17:39AM +0100, Stanislaw Gruszka wrote:\n> In order to remove redundant camera ID lookups and comparisons switch\n> to pointer-based checks when choosing and switching cameras.\n> \n> Co-developed-by: Laurent Pinchart \n> Signed-off-by: Laurent Pinchart \n> Signed-off-by: Stanislaw Gruszka \n> ---\n> src/apps/qcam/main_window.cpp | 30 +++++++++++++-----------------\n> src/apps/qcam/main_window.h | 2 +-\n> 2 files changed, 14 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp\n> index de487672..e0bbcc75 100644\n> --- a/src/apps/qcam/main_window.cpp\n> +++ b/src/apps/qcam/main_window.cpp\n> @@ -251,16 +251,14 @@ void MainWindow::updateTitle()\n> void MainWindow::switchCamera()\n> {\n> \t/* Get and acquire the new camera. */\n> -\tstd::string newCameraId = chooseCamera();\n> +\tstd::shared_ptr cam = chooseCamera();\n> \n> -\tif (newCameraId.empty())\n> +\tif (!cam)\n> \t\treturn;\n> \n> -\tif (camera_ && newCameraId == camera_->id())\n> +\tif (camera_ && cam == camera_)\n> \t\treturn;\n> \n> -\tconst std::shared_ptr &cam = cm_->get(newCameraId);\n> -\n> \tif (cam->acquire()) {\n> \t\tqInfo() << \"Failed to acquire camera\" << cam->id().c_str();\n> \t\treturn;\n> @@ -282,20 +280,21 @@ void MainWindow::switchCamera()\n> \tstartStopAction_->setChecked(true);\n> \n> \t/* Display the current cameraId in the toolbar .*/\n> -\tcameraSelectButton_->setText(QString::fromStdString(newCameraId));\n> +\tcameraSelectButton_->setText(QString::fromStdString(cam->id()));\n> }\n> \n> -std::string MainWindow::chooseCamera()\n> +std::shared_ptr MainWindow::chooseCamera()\n> {\n> \tif (cameraSelectorDialog_->exec() != QDialog::Accepted)\n> -\t\treturn std::string();\n> +\t\treturn {};\n> \n> -\treturn cameraSelectorDialog_->getCameraId();\n> +\tstd::string id = cameraSelectorDialog_->getCameraId();\n> +\treturn cm_->get(id);\n> }\n> \n> int MainWindow::openCamera()\n> {\n> -\tstd::string cameraName;\n> +\tstd::string cameraName = \"\";\n> \n> \t/*\n> \t * If a camera is specified on the command line, get it. Otherwise, if\n> @@ -304,24 +303,21 @@ int MainWindow::openCamera()\n> \t */\n> \tif (options_.isSet(OptCamera)) {\n> \t\tcameraName = static_cast(options_[OptCamera]);\n> +\t\tcamera_ = cm_->get(cameraName);\n> \t} else {\n> \t\tstd::vector> cameras = cm_->cameras();\n> \t\tif (cameras.size() == 1)\n> -\t\t\tcameraName = cameras[0]->id();\n> +\t\t\tcamera_ = cameras[0];\n> \t\telse\n> -\t\t\tcameraName = chooseCamera();\n> +\t\t\tcamera_ = chooseCamera();\n> \t}\n> \n> -\tif (cameraName == \"\")\n> -\t\treturn -EINVAL;\n> -\n> -\t/* Get and acquire the camera. */\n> -\tcamera_ = cm_->get(cameraName);\n> \tif (!camera_) {\n> \t\tqInfo() << \"Camera\" << cameraName.c_str() << \"not found\";\n\nThis will print \"Camera not found\" if the user presses the cancel\nbutton in the selector dialog. Should we restrict printing the message\nto the case where the camera name is specified on the command line ?\nSomething like\n\n\tif (options_.isSet(OptCamera)) {\n\t\tstd::string name = static_cast(options_[OptCamera]);\n\t\tcamera_ = cm_->get(name);\n\t\tif (!camera_)\n\t\t\tqInfo() << \"Camera\" << name.c_str() << \"not found\";\n\t} else {\n\t\tstd::vector> cameras = cm_->cameras();\n\t\tif (cameras.size() == 1)\n\t\t\tcamera_ = cameras[0];\n\t\telse\n\t\t\tcamera_ = chooseCamera();\n\t}\n\n\tif (!camera_)\n\t\treturn -ENODEV;\n\n> \t\treturn -ENODEV;\n> \t}\n> \n> +\t/* Acquire the camera. */\n> \tif (camera_->acquire()) {\n> \t\tqInfo() << \"Failed to acquire camera\";\n> \t\tcamera_.reset();\n> diff --git a/src/apps/qcam/main_window.h b/src/apps/qcam/main_window.h\n> index 4cead734..81fcf915 100644\n> --- a/src/apps/qcam/main_window.h\n> +++ b/src/apps/qcam/main_window.h\n> @@ -73,7 +73,7 @@ private Q_SLOTS:\n> private:\n> \tint createToolbars();\n> \n> -\tstd::string chooseCamera();\n> +\tstd::shared_ptr chooseCamera();\n> \tint openCamera();\n> \n> \tint startCapture();","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 4CCB1C31E9\n\tfor ;\n\tWed, 30 Oct 2024 11:42:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62ED46539E;\n\tWed, 30 Oct 2024 12:42:42 +0100 (CET)","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 314DF60360\n\tfor ;\n\tWed, 30 Oct 2024 12:42:40 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC454B7E;\n\tWed, 30 Oct 2024 12:42:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BqFFD+50\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730288557;\n\tbh=ed8v+qylujwIG2nvU88veNBafbLetaAEY+DaQPGbbmY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BqFFD+50iY3AnqJ2BxKef5izMlama6/+ZIw/ptWr+ONZtjcUIWYykfs/AnIO2XN7t\n\tryRRQ+oRu1/b4K6CdmQKmNfJfrGwz55hcZ3YA93El6VR5mxDB3gPwdASQZ8cZFv9+x\n\tVMEqR+B0QlDIZz7CqXe8BESRah3r+xDLfyJ68jdQ=","Date":"Wed, 30 Oct 2024 13:42:33 +0200","From":"Laurent Pinchart ","To":"Stanislaw Gruszka ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] qcam: Use pointer when choosing camera","Message-ID":"<20241030114233.GD17733@pendragon.ideasonboard.com>","References":"<20241030071739.271004-1-stanislaw.gruszka@linux.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241030071739.271004-1-stanislaw.gruszka@linux.intel.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31971,"web_url":"https://patchwork.libcamera.org/comment/31971/","msgid":"","date":"2024-10-30T16:20:07","subject":"Re: [PATCH] qcam: Use pointer when choosing camera","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"Hi Laurent,\n\nOn Wed, Oct 30, 2024 at 01:42:33PM +0200, Laurent Pinchart wrote:\n> Hi Stanislaw,\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 30, 2024 at 08:17:39AM +0100, Stanislaw Gruszka wrote:\n> > In order to remove redundant camera ID lookups and comparisons switch\n> > to pointer-based checks when choosing and switching cameras.\n> > \n> > Co-developed-by: Laurent Pinchart \n> > Signed-off-by: Laurent Pinchart \n> > Signed-off-by: Stanislaw Gruszka \n> > ---\n> > src/apps/qcam/main_window.cpp | 30 +++++++++++++-----------------\n> > src/apps/qcam/main_window.h | 2 +-\n> > 2 files changed, 14 insertions(+), 18 deletions(-)\n> > \n> > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp\n> > index de487672..e0bbcc75 100644\n> > --- a/src/apps/qcam/main_window.cpp\n> > +++ b/src/apps/qcam/main_window.cpp\n> > @@ -251,16 +251,14 @@ void MainWindow::updateTitle()\n> > void MainWindow::switchCamera()\n> > {\n> > \t/* Get and acquire the new camera. */\n> > -\tstd::string newCameraId = chooseCamera();\n> > +\tstd::shared_ptr cam = chooseCamera();\n> > \n> > -\tif (newCameraId.empty())\n> > +\tif (!cam)\n> > \t\treturn;\n> > \n> > -\tif (camera_ && newCameraId == camera_->id())\n> > +\tif (camera_ && cam == camera_)\n> > \t\treturn;\n> > \n> > -\tconst std::shared_ptr &cam = cm_->get(newCameraId);\n> > -\n> > \tif (cam->acquire()) {\n> > \t\tqInfo() << \"Failed to acquire camera\" << cam->id().c_str();\n> > \t\treturn;\n> > @@ -282,20 +280,21 @@ void MainWindow::switchCamera()\n> > \tstartStopAction_->setChecked(true);\n> > \n> > \t/* Display the current cameraId in the toolbar .*/\n> > -\tcameraSelectButton_->setText(QString::fromStdString(newCameraId));\n> > +\tcameraSelectButton_->setText(QString::fromStdString(cam->id()));\n> > }\n> > \n> > -std::string MainWindow::chooseCamera()\n> > +std::shared_ptr MainWindow::chooseCamera()\n> > {\n> > \tif (cameraSelectorDialog_->exec() != QDialog::Accepted)\n> > -\t\treturn std::string();\n> > +\t\treturn {};\n> > \n> > -\treturn cameraSelectorDialog_->getCameraId();\n> > +\tstd::string id = cameraSelectorDialog_->getCameraId();\n> > +\treturn cm_->get(id);\n> > }\n> > \n> > int MainWindow::openCamera()\n> > {\n> > -\tstd::string cameraName;\n> > +\tstd::string cameraName = \"\";\n> > \n> > \t/*\n> > \t * If a camera is specified on the command line, get it. Otherwise, if\n> > @@ -304,24 +303,21 @@ int MainWindow::openCamera()\n> > \t */\n> > \tif (options_.isSet(OptCamera)) {\n> > \t\tcameraName = static_cast(options_[OptCamera]);\n> > +\t\tcamera_ = cm_->get(cameraName);\n> > \t} else {\n> > \t\tstd::vector> cameras = cm_->cameras();\n> > \t\tif (cameras.size() == 1)\n> > -\t\t\tcameraName = cameras[0]->id();\n> > +\t\t\tcamera_ = cameras[0];\n> > \t\telse\n> > -\t\t\tcameraName = chooseCamera();\n> > +\t\t\tcamera_ = chooseCamera();\n> > \t}\n> > \n> > -\tif (cameraName == \"\")\n> > -\t\treturn -EINVAL;\n> > -\n> > -\t/* Get and acquire the camera. */\n> > -\tcamera_ = cm_->get(cameraName);\n> > \tif (!camera_) {\n> > \t\tqInfo() << \"Camera\" << cameraName.c_str() << \"not found\";\n> \n> This will print \"Camera not found\" if the user presses the cancel\n\nYes, it's not nice, but I wanted some message when there is no camera ...\n\n> button in the selector dialog. Should we restrict printing the message\n> to the case where the camera name is specified on the command line ?\n> Something like\n> \n> \tif (options_.isSet(OptCamera)) {\n> \t\tstd::string name = static_cast(options_[OptCamera]);\n> \t\tcamera_ = cm_->get(name);\n> \t\tif (!camera_)\n> \t\t\tqInfo() << \"Camera\" << name.c_str() << \"not found\";\n> \t} else {\n> \t\tstd::vector> cameras = cm_->cameras();\n> \t\tif (cameras.size() == 1)\n> \t\t\tcamera_ = cameras[0];\n> \t\telse\n> \t\t\tcamera_ = chooseCamera();\n\n.. so I think here we can add message like below, to cover both branches:\n\n\t if (!camara_)\n \t\t\tqInfo() << \"No camera detected\";\n\nRegards\nStanislaw","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 827CFC31E9\n\tfor ;\n\tWed, 30 Oct 2024 16:20:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C6FF653AA;\n\tWed, 30 Oct 2024 17:20:16 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [192.198.163.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 369B965392\n\tfor ;\n\tWed, 30 Oct 2024 17:20:13 +0100 (CET)","from orviesa001.jf.intel.com ([10.64.159.141])\n\tby fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t30 Oct 2024 09:20:12 -0700","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.245.118.67])\n\tby smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t30 Oct 2024 09:20:10 -0700"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"DFKZ4n87\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1730305214; x=1761841214;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=x1rp1EaKxz4h5PD85vPUMy0hmHZtJBOJ7DJdclse9aQ=;\n\tb=DFKZ4n87G0av9d+/FCN1dCcGfwxnO061N+rIBW0dr8+ipJgyG+p0g3+4\n\tN3dSNFt+LPepMwxD1WcKcS/PlguSVSlseserdSOLo9yOhJl++UqqBe8lh\n\t6kurw9KfOZc0Tlot1U0p3a18CVAJD3LEJaKsEq1+3l+qooVZayAPSj1/Y\n\tTGHvsSWvje7qnRi4y3ww3YpLyKeZ+n9dP9Wj4S372tNc7J0TVJzskjLuU\n\t8u+EngifJ1ni/C7kSaRKWO9SuyyWt/ilfgfxtXKAl9n9D7GqL/gh0AIDb\n\tIhhwgSHrg6Y5hWgkJlUFeyYCzcUOGNZmsAZMSCl+CdVrAXd4S8m7aHIst A==;","X-CSE-ConnectionGUID":["i3aPdK9KSwifr7a9UOv0iw==","74GUyUDmRM+m7NFE3VMNtA=="],"X-CSE-MsgGUID":["/c9bn/r6TMCt3e5pd9fQwA==","tI0QjrCRQCmNbPLDbTby9g=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11241\"; a=\"17660096\"","E=Sophos;i=\"6.11,245,1725346800\"; d=\"scan'208\";a=\"17660096\"","E=Sophos;i=\"6.11,245,1725346800\"; d=\"scan'208\";a=\"119821734\""],"X-ExtLoop1":"1","Date":"Wed, 30 Oct 2024 17:20:07 +0100","From":"Stanislaw Gruszka ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] qcam: Use pointer when choosing camera","Message-ID":"","References":"<20241030071739.271004-1-stanislaw.gruszka@linux.intel.com>\n\t<20241030114233.GD17733@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241030114233.GD17733@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]