[{"id":4756,"web_url":"https://patchwork.libcamera.org/comment/4756/","msgid":"<20200506210710.GG15206@pendragon.ideasonboard.com>","date":"2020-05-06T21:07:10","subject":"Re: [libcamera-devel] [PATCH 3/4] qcam: main_window: Introduce\n\thotplug support","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 Wed, May 06, 2020 at 10:33:55AM +0000, Umang Jain wrote:\n> Signed-off-by: Umang Jain \n> ---\n> src/qcam/main_window.cpp | 31 +++++++++++++++++++++++++++++++\n> src/qcam/main_window.h | 3 +++\n> 2 files changed, 34 insertions(+)\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 7de0895..9db1647 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -71,6 +71,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> \tsetCentralWidget(viewfinder_);\n> \tadjustSize();\n> \n> +\t/* Hotplug/unplug support */\n> +\tcm_->cameraAdded.connect(this, &MainWindow::addNewCamera);\n> +\tcm_->cameraRemoved.connect(this, &MainWindow::removeCamera);\n> +\n> \t/* Open the camera and start capture. */\n> \tret = openCamera();\n> \tif (ret < 0) {\n> @@ -525,6 +529,33 @@ void MainWindow::stopCapture()\n> \tsetWindowTitle(title_);\n> }\n> \n> +/* -----------------------------------------------------------------------------\n> + * Camera hotplugging support\n> + */\n> +\n> +void MainWindow::addNewCamera(Camera *cam)\n> +{\n> +\tqInfo() << \"Adding new camera: \" << cam->name().c_str();\n\nQt will insert a space automatically, so you can write\n\n\tqInfo() << \"Adding new camera:\" << cam->name().c_str();\n\n> +\tcameraCombo_->addItem(QString::fromStdString(cam->name()));\n\nThis is called from the camera manager thread, and Qt doesn't allow GUI\nclasses to be accessed from any other thread than the GUI thread (see\nhttps://doc.qt.io/qt-5/thread-basics.html). I recommend posting an event\nto move processing of hotplug to the GUI thread. This is even more\nimportant for unplug, as you stop the stream then, which can take a\nlarge amount of time.\n\nYou can create a new HotplugEvent class modelled after CaptureEvent to\nhandle this, and store a pointer to the camera as well as a boolean flag\nto tell if the camera is plugged or unplugged (so you won't need two\ndifferent QEvent subclasses for hotplug and hotunplug).\n\nThis will however create race conditions, as if the camera being removed\nisn't the one currently streaming, no reference to it will be kept by\nqcam, and the Camera object will be deleted before the event handler\nwill have a chance to run. One option could be to store the camera name\nin addition to the camera pointer in the event, so unplug code could\ncheck the name only when the camera being removed isn't the active\ncamera. Same for hotplug actually, while it wouldn't be easy to trigger\nthat, a camera could be added and then removed before the hotplug event\nis processed. Maybe this calls for passing a std::shared_ptr\nthrough the cameraAdded and cameraRemoved signals ? I'm not sure what's\nbest at this point, feel free to think about it and share you opinion\n:-)\n\n> +}\n> +\n> +void MainWindow::removeCamera(Camera *cam)\n> +{\n> +\tint camIndex = cameraCombo_->findText(QString::fromStdString(cam->name()));\n> +\n> +\t/* Check if the currently-streaming camera is removed.\n> +\t *\n> +\t * \\todo Also analyse the edge-case where the only available\n> +\t * camera is removed. */\n\nWhat happens in that case ? :-)\n\nWe format comments with /* and */ on a line of their own.\n\n> +\tif (camIndex == cameraCombo_->currentIndex()) {\n\nWouldn't it be simpler to test\n\n\tif (cam == camera_) {\n\n?\n\n> +\t\ttoggleCapture(false);\n> +\t\tcameraCombo_->setCurrentIndex(0);\n> +\t}\n> +\n> +\tqInfo() << \"Removing camera: \" << cam->name().c_str();\n\nSame here regarding the space.\n\n> +\tcameraCombo_->removeItem(camIndex);\n> +}\n> +\n> /* -----------------------------------------------------------------------------\n> * Image Save\n> */\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 59fa2d9..cb2fa26 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -87,6 +87,9 @@ private:\n> \tint startCapture();\n> \tvoid stopCapture();\n> \n> +\tvoid addNewCamera(Camera *camera);\n> +\tvoid removeCamera(Camera *camera);\n> +\n> \tvoid requestComplete(Request *request);\n> \tvoid processCapture();\n> \tvoid processViewfinder(FrameBuffer *buffer);","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 62842603F2\n\tfor ;\n\tWed, 6 May 2020 23:07:16 +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 D1D9F542;\n\tWed, 6 May 2020 23:07:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"STgymt5U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588799236;\n\tbh=O/b7Eb5Z3dQJF7+kLDtpCmzY/frg3SrD1yFct6A9U1k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=STgymt5UnHc5wYDP8QQOns4l07odOvOnkLQ2rN/8nYfJ/a2LBml1qvX6ii7bcEz0e\n\t1+WxE4HU2BEjWY/N8/bgS49xnJLgAuOObAqlD8aUgHi8WfVq1wOek2wL5KhPT2t1Je\n\tANnwJ9hCqry2oPYdduGqCGh8fbCySoiPaGvGi0CU=","Date":"Thu, 7 May 2020 00:07:10 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel ","Message-ID":"<20200506210710.GG15206@pendragon.ideasonboard.com>","References":"<20200506103346.3433-1-email@uajain.com>\n\t<20200506103346.3433-4-email@uajain.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200506103346.3433-4-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH 3/4] qcam: main_window: Introduce\n\thotplug support","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","X-List-Received-Date":"Wed, 06 May 2020 21:07:16 -0000"}}]