[libcamera-devel] [PATCH v3 4/5] qcam: main_window: Introduce initial hotplug support

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, May 21, 2020 at 01:54:26PM +0000, Umang Jain wrote:
> Hook up various QCam UI bits with hotplug support introduced
> in previous commits. This looks good-enough as first steps
> to see how the hotplugging functionality is turning out to be
> from application point-of-view.
> 
> One can still think of few edge case nuances not yet covered
> under this implementation hence, those are intentionally
> kept out of scope for now. It might require some thinking
> and/or additional time on hand.

Out of curiosity, do you have any such cases in mind already ?

> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/qcam/main_window.cpp | 83 ++++++++++++++++++++++++++++++++++++++++
>  src/qcam/main_window.h   |  6 +++
>  2 files changed, 89 insertions(+)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 7de0895..ba96c27 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -49,6 +49,43 @@ public:
>  	}
>  };
>  
> +/**
> + * \brief Custom QEvent to signal hotplug or unplug
> + */
> +class HotplugEvent : public QEvent
> +{
> +public:
> +	enum PLUGEVENT {

We Use CamelCase for types, this would be PlugEvent.

> +		HOTPLUG,
> +		UNPLUG

Same here,

		Hotplug,
		Unplug,

Kieran will hate me for asking if it should be

		Plug,
		Unplug,

,

		Hotplug,
		Hotunplug,

or

		HotPlug,
		HotUnplug,

My personal preference is (from most preferred to least preferred)
HotUnplug, Unplug, Hotunplug, but at the same time I wouldn't rename
HotplugEvent to HotPlugEvent. Maybe we could use "hotplug" (Hotplug in
CamelCase) as a generic term that would cover both plug and unplug, and
"hot-plug" and "hot-unplug" (respectively HotPlug and HotUnplug in
CamelCase) when talking about plug and unplug specifically ? Or maybe
that's overkill :-)

> +	};
> +
> +	HotplugEvent(std::shared_ptr<Camera> camera, PLUGEVENT event)
> +		: QEvent(type())

		: QEvent(type()), camera_(std::move(camera)), plugEvent_(event)
	{
	}

> +	{
> +		camera_ = camera;
> +		plugEvent_ = event;
> +	}
> +
> +	~HotplugEvent()
> +	{
> +		camera_.reset();

This isn't needed, the shared pointer is automatically reset when
destroyed. You can drop the HotplugEvent destructor.

> +	}
> +
> +	static Type type()
> +	{
> +		static int type = QEvent::registerEventType();
> +		return static_cast<Type>(type);
> +	}
> +
> +	PLUGEVENT getHotplugEvent() { return plugEvent_; }

We don't usually prefix getters with get,

	PlugEvent hotplugEvent() const { return plugEvent; }

(with an added const)

> +	Camera *getCamera() { return camera_.get(); }

Same here.

> +
> +private:
> +	std::shared_ptr<Camera> camera_;
> +	PLUGEVENT plugEvent_;
> +};
> +
>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  	: saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr),
>  	  isCapturing_(false), captureRaw_(false)
> @@ -71,6 +108,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  	setCentralWidget(viewfinder_);
>  	adjustSize();
>  
> +	/* Hotplug/unplug support */
> +	cm_->newCameraAdded.connect(this, &MainWindow::addNewCamera);
> +	cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);
> +
>  	/* Open the camera and start capture. */
>  	ret = openCamera();
>  	if (ret < 0) {
> @@ -95,6 +136,9 @@ bool MainWindow::event(QEvent *e)
>  	if (e->type() == CaptureEvent::type()) {
>  		processCapture();
>  		return true;
> +	} else if (e->type() == HotplugEvent::type()) {
> +		processHotplug(static_cast<HotplugEvent *>(e));
> +		return true;
>  	}
>  
>  	return QMainWindow::event(e);
> @@ -525,6 +569,45 @@ void MainWindow::stopCapture()
>  	setWindowTitle(title_);
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Camera hotplugging support
> + */
> +
> +void MainWindow::processHotplug(HotplugEvent *e)
> +{
> +	Camera *camera = e->getCamera();
> +	HotplugEvent::PLUGEVENT event = e->getHotplugEvent();
> +
> +	if (event == HotplugEvent::PLUGEVENT::HOTPLUG) {

This can be shortened to HotplugEvent::HOTPLUG (same for similar
occurences below).

> +		cameraCombo_->addItem(QString::fromStdString(camera->name()));
> +	} else if (event == HotplugEvent::PLUGEVENT::UNPLUG) {
> +		int camIndex = cameraCombo_->findText(QString::fromStdString(camera->name()));

I'd move this line right before cameraCombo_->removeItem() (with a blank
line above) to keep the variable declaration close to its usage.

> +
> +		/* Check if the currently-streaming camera is removed. */
> +		if (camera == camera_.get()) {
> +			toggleCapture(false);
> +			cameraCombo_->setCurrentIndex(0);
> +		}
> +		cameraCombo_->removeItem(camIndex);
> +	}
> +}
> +
> +void MainWindow::addNewCamera(std::shared_ptr<Camera> camera)

If we rename CameraManager::newCameraAdded this should be renamed to
addCamera().

That's quite a few comments, but nothing major. I believe the next
version will be the last. Thanks for your work !

> +{
> +	qInfo() << "Adding new camera:" << camera->name().c_str();
> +	QCoreApplication::postEvent(this,
> +				    new HotplugEvent(std::move(camera),
> +						     HotplugEvent::PLUGEVENT::HOTPLUG));
> +}
> +
> +void MainWindow::removeCamera(std::shared_ptr<Camera> camera)
> +{
> +	qInfo() << "Removing camera:" << camera->name().c_str();
> +	QCoreApplication::postEvent(this,
> +				    new HotplugEvent(std::move(camera),
> +						     HotplugEvent::PLUGEVENT::UNPLUG));
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Image Save
>   */
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 59fa2d9..9108780 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -32,6 +32,8 @@ using namespace libcamera;
>  class QAction;
>  class QComboBox;
>  
> +class HotplugEvent;
> +
>  enum {
>  	OptCamera = 'c',
>  	OptHelp = 'h',
> @@ -87,8 +89,12 @@ private:
>  	int startCapture();
>  	void stopCapture();
>  
> +	void addNewCamera(std::shared_ptr<Camera> camera);
> +	void removeCamera(std::shared_ptr<Camera> camera);
> +
>  	void requestComplete(Request *request);
>  	void processCapture();
> +	void processHotplug(HotplugEvent *e);
>  	void processViewfinder(FrameBuffer *buffer);
>  
>  	/* UI elements */

-- 
Regards,

Laurent Pinchart