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

[Umang Jain]

Hi Laurent,

On 6/7/20 10:34 PM, Laurent Pinchart wrote:
> 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 ?

Yes, They were mostly around having one camera on the system.

Hot-unplugging the only camera present and plugging it back in, does not

start streaming it out of the box. Might need to handle this case a bit 
differently

in qcam, which honestly I was hoping to address after these patch series 
is landed,

in chunks of small improvements.

>
>> 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 */