[libcamera-devel,3/4] qcam: main_window: Introduce hotplug support

3691 diff mbox series

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/qcam/main_window.cpp | 31 +++++++++++++++++++++++++++++++
 src/qcam/main_window.h   |  3 +++
 2 files changed, 34 insertions(+)

Hi Umang,

Thank you for the patch.

On Wed, May 06, 2020 at 10:33:55AM +0000, Umang Jain wrote:
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/qcam/main_window.cpp | 31 +++++++++++++++++++++++++++++++
>  src/qcam/main_window.h   |  3 +++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 7de0895..9db1647 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -71,6 +71,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  	setCentralWidget(viewfinder_);
>  	adjustSize();
>  
> +	/* Hotplug/unplug support */
> +	cm_->cameraAdded.connect(this, &MainWindow::addNewCamera);
> +	cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);
> +
>  	/* Open the camera and start capture. */
>  	ret = openCamera();
>  	if (ret < 0) {
> @@ -525,6 +529,33 @@ void MainWindow::stopCapture()
>  	setWindowTitle(title_);
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Camera hotplugging support
> + */
> +
> +void MainWindow::addNewCamera(Camera *cam)
> +{
> +	qInfo() << "Adding new camera: " << cam->name().c_str();

Qt will insert a space automatically, so you can write

	qInfo() << "Adding new camera:" << cam->name().c_str();

> +	cameraCombo_->addItem(QString::fromStdString(cam->name()));

This is called from the camera manager thread, and Qt doesn't allow GUI
classes to be accessed from any other thread than the GUI thread (see
https://doc.qt.io/qt-5/thread-basics.html). I recommend posting an event
to move processing of hotplug to the GUI thread. This is even more
important for unplug, as you stop the stream then, which can take a
large amount of time.

You can create a new HotplugEvent class modelled after CaptureEvent to
handle this, and store a pointer to the camera as well as a boolean flag
to tell if the camera is plugged or unplugged (so you won't need two
different QEvent subclasses for hotplug and hotunplug).

This will however create race conditions, as if the camera being removed
isn't the one currently streaming, no reference to it will be kept by
qcam, and the Camera object will be deleted before the event handler
will have a chance to run. One option could be to store the camera name
in addition to the camera pointer in the event, so unplug code could
check the name only when the camera being removed isn't the active
camera. Same for hotplug actually, while it wouldn't be easy to trigger
that, a camera could be added and then removed before the hotplug event
is processed. Maybe this calls for passing a std::shared_ptr<Camera>
through the cameraAdded and cameraRemoved signals ? I'm not sure what's
best at this point, feel free to think about it and share you opinion
:-)

> +}
> +
> +void MainWindow::removeCamera(Camera *cam)
> +{
> +	int camIndex = cameraCombo_->findText(QString::fromStdString(cam->name()));
> +
> +	/* Check if the currently-streaming camera is removed.
> +	 *
> +	 * \todo Also analyse the edge-case where the only available
> +	 * camera is removed. */

What happens in that case ? :-)

We format comments with /* and */ on a line of their own.

> +	if (camIndex == cameraCombo_->currentIndex()) {

Wouldn't it be simpler to test

	if (cam == camera_) {

?

> +		toggleCapture(false);
> +		cameraCombo_->setCurrentIndex(0);
> +	}
> +
> +	qInfo() << "Removing camera: " << cam->name().c_str();

Same here regarding the space.

> +	cameraCombo_->removeItem(camIndex);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Image Save
>   */
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 59fa2d9..cb2fa26 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -87,6 +87,9 @@ private:
>  	int startCapture();
>  	void stopCapture();
>  
> +	void addNewCamera(Camera *camera);
> +	void removeCamera(Camera *camera);
> +
>  	void requestComplete(Request *request);
>  	void processCapture();
>  	void processViewfinder(FrameBuffer *buffer);