[libcamera-devel,3/6] qcam: Introduce a toolbar and camera switching

2789 diff mbox series

Implement a quit button, and a list of cameras.

Selecting a different camera from the Toolbar will stop the current
stream, and start streaming the chosen camera device if it can be
acquired.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
 src/qcam/main_window.h   |  4 +++
 2 files changed, 64 insertions(+)

Hi Kieran,

Thank you for the patch.

On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:
> Implement a quit button, and a list of cameras.
> 
> Selecting a different camera from the Toolbar will stop the current
> stream, and start streaming the chosen camera device if it can be
> acquired.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
>  src/qcam/main_window.h   |  4 +++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index b51a16de199d..1c7260f32d0a 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -13,6 +13,8 @@
>  #include <QCoreApplication>
>  #include <QInputDialog>
>  #include <QTimer>
> +#include <QToolBar>
> +#include <QToolButton>
>  
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/version.h>
> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  {
>  	int ret;
>  
> +	createToolbars(cm);
> +
>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
>  	setWindowTitle(title_);
>  	connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()
>  	}
>  }
>  
> +int MainWindow::createToolbars(CameraManager *cm)
> +{
> +	QAction *action;
> +
> +	toolbar_ = addToolBar("");

You should give a name to the toolbar, otherwise the context menu will
have an empty entry. You can call it "Main" or anything similar to start
with.

> +
> +	action = toolbar_->addAction("Quit");
> +	connect(action, &QAction::triggered, this, &MainWindow::quit);
> +
> +	QAction *cameraAction = new QAction("&Cameras", this);
> +	toolbar_->addAction(cameraAction);
> +
> +	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
> +
> +	cameraButton->setPopupMode(QToolButton::InstantPopup);

Any way we could remove the "Camera" entry from the popup menu ? It
seems we may have to insert a manually-created QComboBox. My initial
expriment resulted in the following code. Feel free to fold it in,
modify it, or ignore it if you think it's not a good idea.

diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index 0e994b1e9197..f68b7abccda6 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -10,6 +10,7 @@
 #include <string>
 #include <sys/mman.h>

+#include <QComboBox>
 #include <QCoreApplication>
 #include <QFileDialog>
 #include <QIcon>
@@ -29,11 +30,11 @@
 using namespace libcamera;

 MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
-	: options_(options), allocator_(nullptr), isCapturing_(false)
+	: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)
 {
 	int ret;

-	createToolbars(cm);
+	createToolbars();

 	title_ = "QCam " + QString::fromStdString(CameraManager::version());
 	setWindowTitle(title_);
@@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
 	setCentralWidget(viewfinder_);
 	adjustSize();

-	ret = openCamera(cm);
+	ret = openCamera();
 	if (!ret) {
 		ret = startCapture();
 	}
@@ -61,7 +62,7 @@ MainWindow::~MainWindow()
 	}
 }

-int MainWindow::createToolbars(CameraManager *cm)
+int MainWindow::createToolbars()
 {
 	QAction *action;

@@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)
 	action = toolbar_->addAction(QIcon(":x-circle.svg"), "Quit");
 	connect(action, &QAction::triggered, this, &MainWindow::quit);

-	QAction *cameraAction = new QAction("&Cameras", this);
-	toolbar_->addAction(cameraAction);
+	QComboBox *cameraCombo = new QComboBox();
+	connect(cameraCombo, QOverload<int>::of(&QComboBox::activated),
+		this, &MainWindow::setCamera);

-	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
+	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
+		cameraCombo->addItem(QString::fromStdString(cam->name()));

-	cameraButton->setPopupMode(QToolButton::InstantPopup);
-
-	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
-		action = new QAction(QString::fromStdString(cam->name()));
-		cameraButton->addAction(action);
-		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
-	}
+	toolbar_->addWidget(cameraCombo);

 	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
 	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
@@ -117,13 +114,19 @@ void MainWindow::updateTitle()
 	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
 }

-int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
+void MainWindow::setCamera(int index)
 {
+	const auto &cameras = cm_->cameras();
+	if (static_cast<unsigned int>(index) >= cameras.size())
+		return;
+
+	std::shared_ptr<Camera> cam = cameras[index];
+
 	std::cout << "Chose " << cam->name() << std::endl;

 	if (cam->acquire()) {
 		std::cout << "Failed to acquire camera" << std::endl;
-		return -EBUSY;
+		return;
 	}

 	std::cout << "Switching to camera " << cam->name() << std::endl;
@@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
 	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);

 	startCapture();
-
-	return 0;
 }

-std::string MainWindow::chooseCamera(CameraManager *cm)
+std::string MainWindow::chooseCamera()
 {
 	QStringList cameras;
 	bool result;

-	if (cm->cameras().size() == 1)
-		return cm->cameras()[0]->name();
+	if (cm_->cameras().size() == 1)
+		return cm_->cameras()[0]->name();

-	for (const std::shared_ptr<Camera> &cam : cm->cameras())
+	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
 		cameras.append(QString::fromStdString(cam->name()));

 	QString name = QInputDialog::getItem(this, "Select Camera",
@@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)
 	return name.toStdString();
 }

-int MainWindow::openCamera(CameraManager *cm)
+int MainWindow::openCamera()
 {
 	std::string cameraName;

 	if (options_.isSet(OptCamera))
 		cameraName = static_cast<std::string>(options_[OptCamera]);
 	else
-		cameraName = chooseCamera(cm);
+		cameraName = chooseCamera();

 	if (cameraName == "")
 		return -EINVAL;

-	camera_ = cm->get(cameraName);
+	camera_ = cm_->get(cameraName);
 	if (!camera_) {
 		std::cout << "Camera " << cameraName << " not found"
 			  << std::endl;
diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
index fc85b6a46491..49af0f6ad934 100644
--- a/src/qcam/main_window.h
+++ b/src/qcam/main_window.h
@@ -44,19 +44,21 @@ private Q_SLOTS:
 	void quit();
 	void updateTitle();

-	int setCamera(const std::shared_ptr<Camera> &cam);
+	void setCamera(int index);

 	int startCapture();
 	void stopCapture();
 	void saveImage();

 private:
-	int createToolbars(CameraManager *cm);
-	std::string chooseCamera(CameraManager *cm);
-	int openCamera(CameraManager *cm);
+	int createToolbars();
+	std::string chooseCamera();
+	int openCamera();
 	void requestComplete(Request *request);
 	int display(FrameBuffer *buffer);

+	CameraManager *cm_;
+
 	QString title_;
 	QTimer titleTimer_;

> +
> +	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
> +		action = new QAction(QString::fromStdString(cam->name()));
> +		cameraButton->addAction(action);
> +		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });

Are you aware that this will create local copies of the
std::shared_ptr<Camera> for each camera ? Probably not a big deal.

> +	}
> +
> +	return 0;
> +}
> +
>  void MainWindow::quit()
>  {
>  	QTimer::singleShot(0, QCoreApplication::instance(),
> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()
>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
>  }
>  
> +int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)

You can make this a void function, the return value of slots is ignored
when connected to signals.

> +{
> +	std::cout << "Chose " << cam->name() << std::endl;
> +
> +	if (cam->acquire()) {
> +		std::cout << "Failed to acquire camera" << std::endl;
> +		return -EBUSY;
> +	}
> +
> +	std::cout << "Switching to camera " << cam->name() << std::endl;
> +
> +	stopCapture();
> +	camera_->release();
> +
> +	/*
> +	 * If we don't disconnect this signal, it will persist (and get
> +	 * re-added and thus duplicated later if we ever switch back to an
> +	 * previously streamed camera). This causes all sorts of pain.
> +	 *
> +	 * Perhaps releasing a camera should disconnect all (public?) connected
> +	 * signals forcefully!

I'm not sure that would be a good idea, implicit actions are usually
confusing.

> +	 */
> +	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
> +	camera_ = cam;
> +	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);

How about connecting the signal in startCapture() and disconnecting it
in stopCapture() ? It will avoid the duplicated connect in openCamera().

> +
> +	startCapture();
> +
> +	return 0;
> +}
> +
>  std::string MainWindow::chooseCamera(CameraManager *cm)
>  {
>  	QStringList cameras;
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index a11443b30b37..f7c96fdd5c30 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -44,7 +44,10 @@ private Q_SLOTS:
>  	void quit();
>  	void updateTitle();
>  
> +	int setCamera(const std::shared_ptr<Camera> &cam);
> +
>  private:
> +	int createToolbars(CameraManager *cm);
>  	std::string chooseCamera(CameraManager *cm);
>  	int openCamera(CameraManager *cm);
>  
> @@ -71,6 +74,7 @@ private:
>  	uint32_t previousFrames_;
>  	uint32_t framesCaptured_;
>  
> +	QToolBar *toolbar_;
>  	ViewFinder *viewfinder_;
>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
>  };

Hi Laurent,

On 06/02/2020 23:28, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:
>> Implement a quit button, and a list of cameras.
>>
>> Selecting a different camera from the Toolbar will stop the current
>> stream, and start streaming the chosen camera device if it can be
>> acquired.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
>>  src/qcam/main_window.h   |  4 +++
>>  2 files changed, 64 insertions(+)
>>
>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>> index b51a16de199d..1c7260f32d0a 100644
>> --- a/src/qcam/main_window.cpp
>> +++ b/src/qcam/main_window.cpp
>> @@ -13,6 +13,8 @@
>>  #include <QCoreApplication>
>>  #include <QInputDialog>
>>  #include <QTimer>
>> +#include <QToolBar>
>> +#include <QToolButton>
>>  
>>  #include <libcamera/camera_manager.h>
>>  #include <libcamera/version.h>
>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>  {
>>  	int ret;
>>  
>> +	createToolbars(cm);
>> +
>>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
>>  	setWindowTitle(title_);
>>  	connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()
>>  	}
>>  }
>>  
>> +int MainWindow::createToolbars(CameraManager *cm)
>> +{
>> +	QAction *action;
>> +
>> +	toolbar_ = addToolBar("");
> 
> You should give a name to the toolbar, otherwise the context menu will
> have an empty entry. You can call it "Main" or anything similar to start
> with.
> 

Which context menu?

I'm not sure I understand the need. I mean, I don't care, I can add the
name - I just can't see /why/ a toolbar needs a name :-)

Ugh ... I see right clicking on the toolbar lets you hide it and then
you can't get it back again. So /that's/ the context menu ...

Should the toolbar be 'permanant'? Or removable - and if removable, how
would we expect to get it back. Keyboard shortcut?


Perhaps removable is useful to be able to simplify the view - but as
this is just a test tool - I don't see the benefit yet.

I'll try to look at disabling the context menu or making the main
toolbar permanent.


>> +
>> +	action = toolbar_->addAction("Quit");
>> +	connect(action, &QAction::triggered, this, &MainWindow::quit);
>> +
>> +	QAction *cameraAction = new QAction("&Cameras", this);
>> +	toolbar_->addAction(cameraAction);
>> +
>> +	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
>> +
>> +	cameraButton->setPopupMode(QToolButton::InstantPopup);
> 
> Any way we could remove the "Camera" entry from the popup menu ? It
> seems we may have to insert a manually-created QComboBox. My initial
> expriment resulted in the following code. Feel free to fold it in,
> modify it, or ignore it if you think it's not a good idea.
> 

I would actually like this entry to show the current camera in the toolbar.

And yes the duplicated entry is annoying.

But I haven't got as far as dealing with such UX issues.
I was focussing on getting the core code to work.

I'll work through your code and see what how it integrates.


Hrm ... one part I don't like about the below is selecting a camera by
index. That seems quite fragile once we have hotplug support ?



> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 0e994b1e9197..f68b7abccda6 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -10,6 +10,7 @@
>  #include <string>
>  #include <sys/mman.h>
> 
> +#include <QComboBox>
>  #include <QCoreApplication>
>  #include <QFileDialog>
>  #include <QIcon>
> @@ -29,11 +30,11 @@
>  using namespace libcamera;
> 
>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> -	: options_(options), allocator_(nullptr), isCapturing_(false)
> +	: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)
>  {
>  	int ret;
> 
> -	createToolbars(cm);
> +	createToolbars();
> 
>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
>  	setWindowTitle(title_);
> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>  	setCentralWidget(viewfinder_);
>  	adjustSize();
> 
> -	ret = openCamera(cm);
> +	ret = openCamera();
>  	if (!ret) {
>  		ret = startCapture();
>  	}
> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()
>  	}
>  }
> 
> -int MainWindow::createToolbars(CameraManager *cm)
> +int MainWindow::createToolbars()
>  {
>  	QAction *action;
> 
> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)
>  	action = toolbar_->addAction(QIcon(":x-circle.svg"), "Quit");
>  	connect(action, &QAction::triggered, this, &MainWindow::quit);
> 
> -	QAction *cameraAction = new QAction("&Cameras", this);
> -	toolbar_->addAction(cameraAction);
> +	QComboBox *cameraCombo = new QComboBox();
> +	connect(cameraCombo, QOverload<int>::of(&QComboBox::activated),
> +		this, &MainWindow::setCamera);
> 
> -	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> +		cameraCombo->addItem(QString::fromStdString(cam->name()));
> 
> -	cameraButton->setPopupMode(QToolButton::InstantPopup);
> -
> -	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
> -		action = new QAction(QString::fromStdString(cam->name()));
> -		cameraButton->addAction(action);
> -		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
> -	}
> +	toolbar_->addWidget(cameraCombo);
> 
>  	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
>  	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()
>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
>  }
> 
> -int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> +void MainWindow::setCamera(int index)
>  {
> +	const auto &cameras = cm_->cameras();
> +	if (static_cast<unsigned int>(index) >= cameras.size())
> +		return;
> +
> +	std::shared_ptr<Camera> cam = cameras[index];
> +
>  	std::cout << "Chose " << cam->name() << std::endl;
> 

I'll remove this debug print and print the camera name if it fails to
acquire.


>  	if (cam->acquire()) {
>  		std::cout << "Failed to acquire camera" << std::endl;
> -		return -EBUSY;
> +		return;
>  	}
> 
>  	std::cout << "Switching to camera " << cam->name() << std::endl;
> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
>  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> 
>  	startCapture();
> -
> -	return 0;
>  }
> 
> -std::string MainWindow::chooseCamera(CameraManager *cm)
> +std::string MainWindow::chooseCamera()
>  {
>  	QStringList cameras;
>  	bool result;
> 
> -	if (cm->cameras().size() == 1)
> -		return cm->cameras()[0]->name();
> +	if (cm_->cameras().size() == 1)
> +		return cm_->cameras()[0]->name();
> 
> -	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
>  		cameras.append(QString::fromStdString(cam->name()));
> 
>  	QString name = QInputDialog::getItem(this, "Select Camera",
> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)
>  	return name.toStdString();
>  }
> 
> -int MainWindow::openCamera(CameraManager *cm)
> +int MainWindow::openCamera()
>  {
>  	std::string cameraName;
> 
>  	if (options_.isSet(OptCamera))
>  		cameraName = static_cast<std::string>(options_[OptCamera]);
>  	else
> -		cameraName = chooseCamera(cm);
> +		cameraName = chooseCamera();
> 
>  	if (cameraName == "")
>  		return -EINVAL;
> 
> -	camera_ = cm->get(cameraName);
> +	camera_ = cm_->get(cameraName);
>  	if (!camera_) {
>  		std::cout << "Camera " << cameraName << " not found"
>  			  << std::endl;
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index fc85b6a46491..49af0f6ad934 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -44,19 +44,21 @@ private Q_SLOTS:
>  	void quit();
>  	void updateTitle();
> 
> -	int setCamera(const std::shared_ptr<Camera> &cam);
> +	void setCamera(int index);
> 
>  	int startCapture();
>  	void stopCapture();
>  	void saveImage();
> 
>  private:
> -	int createToolbars(CameraManager *cm);
> -	std::string chooseCamera(CameraManager *cm);
> -	int openCamera(CameraManager *cm);
> +	int createToolbars();
> +	std::string chooseCamera();
> +	int openCamera();
>  	void requestComplete(Request *request);
>  	int display(FrameBuffer *buffer);
> 
> +	CameraManager *cm_;
> +
>  	QString title_;
>  	QTimer titleTimer_;
> 
>> +
>> +	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
>> +		action = new QAction(QString::fromStdString(cam->name()));
>> +		cameraButton->addAction(action);
>> +		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
> 
> Are you aware that this will create local copies of the
> std::shared_ptr<Camera> for each camera ? Probably not a big deal.

Yes, I thought that was needed to make sure they remain in scope. And as
they are shared_ptr that should be fine right?

The alternative is to pass the QAction, and get the camera name from
there, or pass the cam->name() itself and then get the camera by name.

I thought as we already have the 'Camera' this would be better - but I
can change if it's a problem.


I see in your implementation you move to an index, but I fear this would
cause problems with hotplug support. But maybe that will be painful
anyway, and we'll have to reconstruct the camera list to update anytime
the camera list changes regardless.


>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  void MainWindow::quit()
>>  {
>>  	QTimer::singleShot(0, QCoreApplication::instance(),
>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()
>>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
>>  }
>>  
>> +int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> 
> You can make this a void function, the return value of slots is ignored
> when connected to signals.

Ah indeed, I started out with this as a void, I must have changed it to
return int when I pulled over the code with the -EBUSY.

But we can simply ignore that return value indeed, no action will occur.

Later it would be nice if we had a status bar to report specific
messages through the UI. But that's a 'later' task.

Or maybe a workshop style activity ... ;-)


> 
>> +{
>> +	std::cout << "Chose " << cam->name() << std::endl;
>> +
>> +	if (cam->acquire()) {
>> +		std::cout << "Failed to acquire camera" << std::endl;
>> +		return -EBUSY;
>> +	}
>> +
>> +	std::cout << "Switching to camera " << cam->name() << std::endl;
>> +
>> +	stopCapture();
>> +	camera_->release();
>> +
>> +	/*
>> +	 * If we don't disconnect this signal, it will persist (and get
>> +	 * re-added and thus duplicated later if we ever switch back to an
>> +	 * previously streamed camera). This causes all sorts of pain.
>> +	 *
>> +	 * Perhaps releasing a camera should disconnect all (public?) connected
>> +	 * signals forcefully!
> 
> I'm not sure that would be a good idea, implicit actions are usually
> confusing.
> 
>> +	 */
>> +	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
>> +	camera_ = cam;
>> +	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> 
> How about connecting the signal in startCapture() and disconnecting it
> in stopCapture() ? It will avoid the duplicated connect in openCamera().

Aha - that's much better and clearly obvious :)

I'll update to do so.


>> +
>> +	startCapture();
>> +
>> +	return 0;
>> +}
>> +
>>  std::string MainWindow::chooseCamera(CameraManager *cm)
>>  {
>>  	QStringList cameras;
>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
>> index a11443b30b37..f7c96fdd5c30 100644
>> --- a/src/qcam/main_window.h
>> +++ b/src/qcam/main_window.h
>> @@ -44,7 +44,10 @@ private Q_SLOTS:
>>  	void quit();
>>  	void updateTitle();
>>  
>> +	int setCamera(const std::shared_ptr<Camera> &cam);
>> +
>>  private:
>> +	int createToolbars(CameraManager *cm);
>>  	std::string chooseCamera(CameraManager *cm);
>>  	int openCamera(CameraManager *cm);
>>  
>> @@ -71,6 +74,7 @@ private:
>>  	uint32_t previousFrames_;
>>  	uint32_t framesCaptured_;
>>  
>> +	QToolBar *toolbar_;
>>  	ViewFinder *viewfinder_;
>>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
>>  };
>

Hi Kieran,

On Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:
> On 06/02/2020 23:28, Laurent Pinchart wrote:
> > On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:
> >> Implement a quit button, and a list of cameras.
> >>
> >> Selecting a different camera from the Toolbar will stop the current
> >> stream, and start streaming the chosen camera device if it can be
> >> acquired.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
> >>  src/qcam/main_window.h   |  4 +++
> >>  2 files changed, 64 insertions(+)
> >>
> >> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> >> index b51a16de199d..1c7260f32d0a 100644
> >> --- a/src/qcam/main_window.cpp
> >> +++ b/src/qcam/main_window.cpp
> >> @@ -13,6 +13,8 @@
> >>  #include <QCoreApplication>
> >>  #include <QInputDialog>
> >>  #include <QTimer>
> >> +#include <QToolBar>
> >> +#include <QToolButton>
> >>  
> >>  #include <libcamera/camera_manager.h>
> >>  #include <libcamera/version.h>
> >> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >>  {
> >>  	int ret;
> >>  
> >> +	createToolbars(cm);
> >> +
> >>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
> >>  	setWindowTitle(title_);
> >>  	connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
> >> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()
> >>  	}
> >>  }
> >>  
> >> +int MainWindow::createToolbars(CameraManager *cm)
> >> +{
> >> +	QAction *action;
> >> +
> >> +	toolbar_ = addToolBar("");
> > 
> > You should give a name to the toolbar, otherwise the context menu will
> > have an empty entry. You can call it "Main" or anything similar to start
> > with.
> 
> Which context menu?
> 
> I'm not sure I understand the need. I mean, I don't care, I can add the
> name - I just can't see /why/ a toolbar needs a name :-)
> 
> Ugh ... I see right clicking on the toolbar lets you hide it and then
> you can't get it back again. So /that's/ the context menu ...
> 
> Should the toolbar be 'permanant'? Or removable - and if removable, how
> would we expect to get it back. Keyboard shortcut?

If you can make it permanent I think that would be best.

> Perhaps removable is useful to be able to simplify the view - but as
> this is just a test tool - I don't see the benefit yet.
> 
> I'll try to look at disabling the context menu or making the main
> toolbar permanent.
> 
> >> +
> >> +	action = toolbar_->addAction("Quit");
> >> +	connect(action, &QAction::triggered, this, &MainWindow::quit);
> >> +
> >> +	QAction *cameraAction = new QAction("&Cameras", this);
> >> +	toolbar_->addAction(cameraAction);
> >> +
> >> +	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
> >> +
> >> +	cameraButton->setPopupMode(QToolButton::InstantPopup);
> > 
> > Any way we could remove the "Camera" entry from the popup menu ? It
> > seems we may have to insert a manually-created QComboBox. My initial
> > expriment resulted in the following code. Feel free to fold it in,
> > modify it, or ignore it if you think it's not a good idea.
> 
> I would actually like this entry to show the current camera in the toolbar.
> 
> And yes the duplicated entry is annoying.
> 
> But I haven't got as far as dealing with such UX issues.
> I was focussing on getting the core code to work.
> 
> I'll work through your code and see what how it integrates.
> 
> 
> Hrm ... one part I don't like about the below is selecting a camera by
> index. That seems quite fragile once we have hotplug support ?

Agreed, but once we have hotplug support we'll have to remove and add
entries from the combo box or popup menu, so refactoring will be needed
anyway. I think hotplug support will also require stable names/IDs, so
we should then have the tool we need to do the job.

> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 0e994b1e9197..f68b7abccda6 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -10,6 +10,7 @@
> >  #include <string>
> >  #include <sys/mman.h>
> > 
> > +#include <QComboBox>
> >  #include <QCoreApplication>
> >  #include <QFileDialog>
> >  #include <QIcon>
> > @@ -29,11 +30,11 @@
> >  using namespace libcamera;
> > 
> >  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> > -	: options_(options), allocator_(nullptr), isCapturing_(false)
> > +	: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)
> >  {
> >  	int ret;
> > 
> > -	createToolbars(cm);
> > +	createToolbars();
> > 
> >  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
> >  	setWindowTitle(title_);
> > @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >  	setCentralWidget(viewfinder_);
> >  	adjustSize();
> > 
> > -	ret = openCamera(cm);
> > +	ret = openCamera();
> >  	if (!ret) {
> >  		ret = startCapture();
> >  	}
> > @@ -61,7 +62,7 @@ MainWindow::~MainWindow()
> >  	}
> >  }
> > 
> > -int MainWindow::createToolbars(CameraManager *cm)
> > +int MainWindow::createToolbars()
> >  {
> >  	QAction *action;
> > 
> > @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)
> >  	action = toolbar_->addAction(QIcon(":x-circle.svg"), "Quit");
> >  	connect(action, &QAction::triggered, this, &MainWindow::quit);
> > 
> > -	QAction *cameraAction = new QAction("&Cameras", this);
> > -	toolbar_->addAction(cameraAction);
> > +	QComboBox *cameraCombo = new QComboBox();
> > +	connect(cameraCombo, QOverload<int>::of(&QComboBox::activated),
> > +		this, &MainWindow::setCamera);
> > 
> > -	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
> > +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > +		cameraCombo->addItem(QString::fromStdString(cam->name()));
> > 
> > -	cameraButton->setPopupMode(QToolButton::InstantPopup);
> > -
> > -	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
> > -		action = new QAction(QString::fromStdString(cam->name()));
> > -		cameraButton->addAction(action);
> > -		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
> > -	}
> > +	toolbar_->addWidget(cameraCombo);
> > 
> >  	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
> >  	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
> > @@ -117,13 +114,19 @@ void MainWindow::updateTitle()
> >  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
> >  }
> > 
> > -int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> > +void MainWindow::setCamera(int index)
> >  {
> > +	const auto &cameras = cm_->cameras();
> > +	if (static_cast<unsigned int>(index) >= cameras.size())
> > +		return;
> > +
> > +	std::shared_ptr<Camera> cam = cameras[index];
> > +
> >  	std::cout << "Chose " << cam->name() << std::endl;
> 
> I'll remove this debug print and print the camera name if it fails to
> acquire.
> 
> >  	if (cam->acquire()) {
> >  		std::cout << "Failed to acquire camera" << std::endl;
> > -		return -EBUSY;
> > +		return;
> >  	}
> > 
> >  	std::cout << "Switching to camera " << cam->name() << std::endl;
> > @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> >  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> > 
> >  	startCapture();
> > -
> > -	return 0;
> >  }
> > 
> > -std::string MainWindow::chooseCamera(CameraManager *cm)
> > +std::string MainWindow::chooseCamera()
> >  {
> >  	QStringList cameras;
> >  	bool result;
> > 
> > -	if (cm->cameras().size() == 1)
> > -		return cm->cameras()[0]->name();
> > +	if (cm_->cameras().size() == 1)
> > +		return cm_->cameras()[0]->name();
> > 
> > -	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> > +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> >  		cameras.append(QString::fromStdString(cam->name()));
> > 
> >  	QString name = QInputDialog::getItem(this, "Select Camera",
> > @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)
> >  	return name.toStdString();
> >  }
> > 
> > -int MainWindow::openCamera(CameraManager *cm)
> > +int MainWindow::openCamera()
> >  {
> >  	std::string cameraName;
> > 
> >  	if (options_.isSet(OptCamera))
> >  		cameraName = static_cast<std::string>(options_[OptCamera]);
> >  	else
> > -		cameraName = chooseCamera(cm);
> > +		cameraName = chooseCamera();
> > 
> >  	if (cameraName == "")
> >  		return -EINVAL;
> > 
> > -	camera_ = cm->get(cameraName);
> > +	camera_ = cm_->get(cameraName);
> >  	if (!camera_) {
> >  		std::cout << "Camera " << cameraName << " not found"
> >  			  << std::endl;
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index fc85b6a46491..49af0f6ad934 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -44,19 +44,21 @@ private Q_SLOTS:
> >  	void quit();
> >  	void updateTitle();
> > 
> > -	int setCamera(const std::shared_ptr<Camera> &cam);
> > +	void setCamera(int index);
> > 
> >  	int startCapture();
> >  	void stopCapture();
> >  	void saveImage();
> > 
> >  private:
> > -	int createToolbars(CameraManager *cm);
> > -	std::string chooseCamera(CameraManager *cm);
> > -	int openCamera(CameraManager *cm);
> > +	int createToolbars();
> > +	std::string chooseCamera();
> > +	int openCamera();
> >  	void requestComplete(Request *request);
> >  	int display(FrameBuffer *buffer);
> > 
> > +	CameraManager *cm_;
> > +
> >  	QString title_;
> >  	QTimer titleTimer_;
> > 
> >> +
> >> +	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
> >> +		action = new QAction(QString::fromStdString(cam->name()));
> >> +		cameraButton->addAction(action);
> >> +		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
> > 
> > Are you aware that this will create local copies of the
> > std::shared_ptr<Camera> for each camera ? Probably not a big deal.
> 
> Yes, I thought that was needed to make sure they remain in scope. And as
> they are shared_ptr that should be fine right?

I think it's OK, I just wanted to point it out as lambda capture can
sometimes be confusing.

> The alternative is to pass the QAction, and get the camera name from
> there, or pass the cam->name() itself and then get the camera by name.
> 
> I thought as we already have the 'Camera' this would be better - but I
> can change if it's a problem.
> 
> I see in your implementation you move to an index, but I fear this would
> cause problems with hotplug support. But maybe that will be painful
> anyway, and we'll have to reconstruct the camera list to update anytime
> the camera list changes regardless.

The reason I move to an index is becase the activated signal only gives
an index. There's a way to associate a QVariant to a combo box entry,
maybe we can store the shared_ptr in the QVariant, but I haven't tried
that.

> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  void MainWindow::quit()
> >>  {
> >>  	QTimer::singleShot(0, QCoreApplication::instance(),
> >> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()
> >>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
> >>  }
> >>  
> >> +int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> > 
> > You can make this a void function, the return value of slots is ignored
> > when connected to signals.
> 
> Ah indeed, I started out with this as a void, I must have changed it to
> return int when I pulled over the code with the -EBUSY.
> 
> But we can simply ignore that return value indeed, no action will occur.
> 
> Later it would be nice if we had a status bar to report specific
> messages through the UI. But that's a 'later' task.
> 
> Or maybe a workshop style activity ... ;-)
> 
> >> +{
> >> +	std::cout << "Chose " << cam->name() << std::endl;
> >> +
> >> +	if (cam->acquire()) {
> >> +		std::cout << "Failed to acquire camera" << std::endl;
> >> +		return -EBUSY;
> >> +	}
> >> +
> >> +	std::cout << "Switching to camera " << cam->name() << std::endl;
> >> +
> >> +	stopCapture();
> >> +	camera_->release();
> >> +
> >> +	/*
> >> +	 * If we don't disconnect this signal, it will persist (and get
> >> +	 * re-added and thus duplicated later if we ever switch back to an
> >> +	 * previously streamed camera). This causes all sorts of pain.
> >> +	 *
> >> +	 * Perhaps releasing a camera should disconnect all (public?) connected
> >> +	 * signals forcefully!
> > 
> > I'm not sure that would be a good idea, implicit actions are usually
> > confusing.
> > 
> >> +	 */
> >> +	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
> >> +	camera_ = cam;
> >> +	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> > 
> > How about connecting the signal in startCapture() and disconnecting it
> > in stopCapture() ? It will avoid the duplicated connect in openCamera().
> 
> Aha - that's much better and clearly obvious :)
> 
> I'll update to do so.
> 
> >> +
> >> +	startCapture();
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  std::string MainWindow::chooseCamera(CameraManager *cm)
> >>  {
> >>  	QStringList cameras;
> >> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> >> index a11443b30b37..f7c96fdd5c30 100644
> >> --- a/src/qcam/main_window.h
> >> +++ b/src/qcam/main_window.h
> >> @@ -44,7 +44,10 @@ private Q_SLOTS:
> >>  	void quit();
> >>  	void updateTitle();
> >>  
> >> +	int setCamera(const std::shared_ptr<Camera> &cam);
> >> +
> >>  private:
> >> +	int createToolbars(CameraManager *cm);
> >>  	std::string chooseCamera(CameraManager *cm);
> >>  	int openCamera(CameraManager *cm);
> >>  
> >> @@ -71,6 +74,7 @@ private:
> >>  	uint32_t previousFrames_;
> >>  	uint32_t framesCaptured_;
> >>  
> >> +	QToolBar *toolbar_;
> >>  	ViewFinder *viewfinder_;
> >>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
> >>  };

Hi Laurent,

On 07/02/2020 10:13, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:
>> On 06/02/2020 23:28, Laurent Pinchart wrote:
>>> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:
>>>> Implement a quit button, and a list of cameras.
>>>>
>>>> Selecting a different camera from the Toolbar will stop the current
>>>> stream, and start streaming the chosen camera device if it can be
>>>> acquired.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>  src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
>>>>  src/qcam/main_window.h   |  4 +++
>>>>  2 files changed, 64 insertions(+)
>>>>
>>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>>>> index b51a16de199d..1c7260f32d0a 100644
>>>> --- a/src/qcam/main_window.cpp
>>>> +++ b/src/qcam/main_window.cpp
>>>> @@ -13,6 +13,8 @@
>>>>  #include <QCoreApplication>
>>>>  #include <QInputDialog>
>>>>  #include <QTimer>
>>>> +#include <QToolBar>
>>>> +#include <QToolButton>
>>>>  
>>>>  #include <libcamera/camera_manager.h>
>>>>  #include <libcamera/version.h>
>>>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>>>  {
>>>>  	int ret;
>>>>  
>>>> +	createToolbars(cm);
>>>> +
>>>>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
>>>>  	setWindowTitle(title_);
>>>>  	connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
>>>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()
>>>>  	}
>>>>  }
>>>>  
>>>> +int MainWindow::createToolbars(CameraManager *cm)
>>>> +{
>>>> +	QAction *action;
>>>> +
>>>> +	toolbar_ = addToolBar("");
>>>
>>> You should give a name to the toolbar, otherwise the context menu will
>>> have an empty entry. You can call it "Main" or anything similar to start
>>> with.
>>
>> Which context menu?
>>
>> I'm not sure I understand the need. I mean, I don't care, I can add the
>> name - I just can't see /why/ a toolbar needs a name :-)
>>
>> Ugh ... I see right clicking on the toolbar lets you hide it and then
>> you can't get it back again. So /that's/ the context menu ...
>>
>> Should the toolbar be 'permanant'? Or removable - and if removable, how
>> would we expect to get it back. Keyboard shortcut?
> 
> If you can make it permanent I think that would be best.

Agreed.

>> Perhaps removable is useful to be able to simplify the view - but as
>> this is just a test tool - I don't see the benefit yet.
>>
>> I'll try to look at disabling the context menu or making the main
>> toolbar permanent.
>>
>>>> +
>>>> +	action = toolbar_->addAction("Quit");
>>>> +	connect(action, &QAction::triggered, this, &MainWindow::quit);
>>>> +
>>>> +	QAction *cameraAction = new QAction("&Cameras", this);
>>>> +	toolbar_->addAction(cameraAction);
>>>> +
>>>> +	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
>>>> +
>>>> +	cameraButton->setPopupMode(QToolButton::InstantPopup);
>>>
>>> Any way we could remove the "Camera" entry from the popup menu ? It
>>> seems we may have to insert a manually-created QComboBox. My initial
>>> expriment resulted in the following code. Feel free to fold it in,
>>> modify it, or ignore it if you think it's not a good idea.
>>
>> I would actually like this entry to show the current camera in the toolbar.
>>
>> And yes the duplicated entry is annoying.
>>
>> But I haven't got as far as dealing with such UX issues.
>> I was focussing on getting the core code to work.
>>
>> I'll work through your code and see what how it integrates.
>>
>>
>> Hrm ... one part I don't like about the below is selecting a camera by
>> index. That seems quite fragile once we have hotplug support ?
> 
> Agreed, but once we have hotplug support we'll have to remove and add
> entries from the combo box or popup menu, so refactoring will be needed
> anyway. I think hotplug support will also require stable names/IDs, so
> we should then have the tool we need to do the job.

I think my point is - we already have one at this level. A shared_ptr to
the Camera instance. That will always point to the same camera, and
never change.

And in the event that the camera is removed, then that Camera will be
marked disconnected, so it will fail to stream - but the 'object' will
be safely preserved until there are no users left?


> 
>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>>> index 0e994b1e9197..f68b7abccda6 100644
>>> --- a/src/qcam/main_window.cpp
>>> +++ b/src/qcam/main_window.cpp
>>> @@ -10,6 +10,7 @@
>>>  #include <string>
>>>  #include <sys/mman.h>
>>>
>>> +#include <QComboBox>
>>>  #include <QCoreApplication>
>>>  #include <QFileDialog>
>>>  #include <QIcon>
>>> @@ -29,11 +30,11 @@
>>>  using namespace libcamera;
>>>
>>>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>> -	: options_(options), allocator_(nullptr), isCapturing_(false)
>>> +	: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)
>>>  {
>>>  	int ret;
>>>
>>> -	createToolbars(cm);
>>> +	createToolbars();
>>>
>>>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
>>>  	setWindowTitle(title_);
>>> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>>  	setCentralWidget(viewfinder_);
>>>  	adjustSize();
>>>
>>> -	ret = openCamera(cm);
>>> +	ret = openCamera();
>>>  	if (!ret) {
>>>  		ret = startCapture();
>>>  	}
>>> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()
>>>  	}
>>>  }
>>>
>>> -int MainWindow::createToolbars(CameraManager *cm)
>>> +int MainWindow::createToolbars()
>>>  {
>>>  	QAction *action;
>>>
>>> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)
>>>  	action = toolbar_->addAction(QIcon(":x-circle.svg"), "Quit");
>>>  	connect(action, &QAction::triggered, this, &MainWindow::quit);
>>>
>>> -	QAction *cameraAction = new QAction("&Cameras", this);
>>> -	toolbar_->addAction(cameraAction);
>>> +	QComboBox *cameraCombo = new QComboBox();
>>> +	connect(cameraCombo, QOverload<int>::of(&QComboBox::activated),
>>> +		this, &MainWindow::setCamera);
>>>
>>> -	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
>>> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
>>> +		cameraCombo->addItem(QString::fromStdString(cam->name()));
>>>
>>> -	cameraButton->setPopupMode(QToolButton::InstantPopup);
>>> -
>>> -	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
>>> -		action = new QAction(QString::fromStdString(cam->name()));
>>> -		cameraButton->addAction(action);
>>> -		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
>>> -	}
>>> +	toolbar_->addWidget(cameraCombo);
>>>
>>>  	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
>>>  	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
>>> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()
>>>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
>>>  }
>>>
>>> -int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
>>> +void MainWindow::setCamera(int index)
>>>  {
>>> +	const auto &cameras = cm_->cameras();
>>> +	if (static_cast<unsigned int>(index) >= cameras.size())
>>> +		return;
>>> +
>>> +	std::shared_ptr<Camera> cam = cameras[index];
>>> +
>>>  	std::cout << "Chose " << cam->name() << std::endl;
>>
>> I'll remove this debug print and print the camera name if it fails to
>> acquire.
>>
>>>  	if (cam->acquire()) {
>>>  		std::cout << "Failed to acquire camera" << std::endl;
>>> -		return -EBUSY;
>>> +		return;
>>>  	}
>>>
>>>  	std::cout << "Switching to camera " << cam->name() << std::endl;
>>> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
>>>  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
>>>
>>>  	startCapture();
>>> -
>>> -	return 0;
>>>  }
>>>
>>> -std::string MainWindow::chooseCamera(CameraManager *cm)
>>> +std::string MainWindow::chooseCamera()
>>>  {
>>>  	QStringList cameras;
>>>  	bool result;
>>>
>>> -	if (cm->cameras().size() == 1)
>>> -		return cm->cameras()[0]->name();
>>> +	if (cm_->cameras().size() == 1)
>>> +		return cm_->cameras()[0]->name();
>>>
>>> -	for (const std::shared_ptr<Camera> &cam : cm->cameras())
>>> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
>>>  		cameras.append(QString::fromStdString(cam->name()));
>>>
>>>  	QString name = QInputDialog::getItem(this, "Select Camera",
>>> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)
>>>  	return name.toStdString();
>>>  }
>>>
>>> -int MainWindow::openCamera(CameraManager *cm)
>>> +int MainWindow::openCamera()
>>>  {
>>>  	std::string cameraName;
>>>
>>>  	if (options_.isSet(OptCamera))
>>>  		cameraName = static_cast<std::string>(options_[OptCamera]);
>>>  	else
>>> -		cameraName = chooseCamera(cm);
>>> +		cameraName = chooseCamera();
>>>
>>>  	if (cameraName == "")
>>>  		return -EINVAL;
>>>
>>> -	camera_ = cm->get(cameraName);
>>> +	camera_ = cm_->get(cameraName);
>>>  	if (!camera_) {
>>>  		std::cout << "Camera " << cameraName << " not found"
>>>  			  << std::endl;
>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
>>> index fc85b6a46491..49af0f6ad934 100644
>>> --- a/src/qcam/main_window.h
>>> +++ b/src/qcam/main_window.h
>>> @@ -44,19 +44,21 @@ private Q_SLOTS:
>>>  	void quit();
>>>  	void updateTitle();
>>>
>>> -	int setCamera(const std::shared_ptr<Camera> &cam);
>>> +	void setCamera(int index);
>>>
>>>  	int startCapture();
>>>  	void stopCapture();
>>>  	void saveImage();
>>>
>>>  private:
>>> -	int createToolbars(CameraManager *cm);
>>> -	std::string chooseCamera(CameraManager *cm);
>>> -	int openCamera(CameraManager *cm);
>>> +	int createToolbars();
>>> +	std::string chooseCamera();
>>> +	int openCamera();
>>>  	void requestComplete(Request *request);
>>>  	int display(FrameBuffer *buffer);
>>>
>>> +	CameraManager *cm_;
>>> +
>>>  	QString title_;
>>>  	QTimer titleTimer_;
>>>
>>>> +
>>>> +	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
>>>> +		action = new QAction(QString::fromStdString(cam->name()));
>>>> +		cameraButton->addAction(action);
>>>> +		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
>>>
>>> Are you aware that this will create local copies of the
>>> std::shared_ptr<Camera> for each camera ? Probably not a big deal.
>>
>> Yes, I thought that was needed to make sure they remain in scope. And as
>> they are shared_ptr that should be fine right?
> 
> I think it's OK, I just wanted to point it out as lambda capture can
> sometimes be confusing.

Indeed, but the alternatives felt uglier to code.
Essentially I'm only using a lambda to give convenient means of passing
parameters through the signal event.

If there's a 'neat' way to do so natively in SIGNAL/SLOT I'm open to
more suggestions, but when I looked into it, it seemed required to just
make an extra function which dealt with it, and at that point - a lambda
does that inline.


>> The alternative is to pass the QAction, and get the camera name from
>> there, or pass the cam->name() itself and then get the camera by name.
>>
>> I thought as we already have the 'Camera' this would be better - but I
>> can change if it's a problem.
>>
>> I see in your implementation you move to an index, but I fear this would
>> cause problems with hotplug support. But maybe that will be painful
>> anyway, and we'll have to reconstruct the camera list to update anytime
>> the camera list changes regardless.
> 
> The reason I move to an index is becase the activated signal only gives
> an index. There's a way to associate a QVariant to a combo box entry,
> maybe we can store the shared_ptr in the QVariant, but I haven't tried
> that.

If you've got the QAction* you can get the camera name from
QAction->text() I think...

Oh - but perhaps you don't even have that. Perhaps I'm conflating what
was my first approach :-)


>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  void MainWindow::quit()
>>>>  {
>>>>  	QTimer::singleShot(0, QCoreApplication::instance(),
>>>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()
>>>>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
>>>>  }
>>>>  
>>>> +int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
>>>
>>> You can make this a void function, the return value of slots is ignored
>>> when connected to signals.
>>
>> Ah indeed, I started out with this as a void, I must have changed it to
>> return int when I pulled over the code with the -EBUSY.
>>
>> But we can simply ignore that return value indeed, no action will occur.
>>
>> Later it would be nice if we had a status bar to report specific
>> messages through the UI. But that's a 'later' task.
>>
>> Or maybe a workshop style activity ... ;-)
>>
>>>> +{
>>>> +	std::cout << "Chose " << cam->name() << std::endl;
>>>> +
>>>> +	if (cam->acquire()) {
>>>> +		std::cout << "Failed to acquire camera" << std::endl;
>>>> +		return -EBUSY;
>>>> +	}
>>>> +
>>>> +	std::cout << "Switching to camera " << cam->name() << std::endl;
>>>> +
>>>> +	stopCapture();
>>>> +	camera_->release();
>>>> +
>>>> +	/*
>>>> +	 * If we don't disconnect this signal, it will persist (and get
>>>> +	 * re-added and thus duplicated later if we ever switch back to an
>>>> +	 * previously streamed camera). This causes all sorts of pain.
>>>> +	 *
>>>> +	 * Perhaps releasing a camera should disconnect all (public?) connected
>>>> +	 * signals forcefully!
>>>
>>> I'm not sure that would be a good idea, implicit actions are usually
>>> confusing.
>>>
>>>> +	 */
>>>> +	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
>>>> +	camera_ = cam;
>>>> +	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
>>>
>>> How about connecting the signal in startCapture() and disconnecting it
>>> in stopCapture() ? It will avoid the duplicated connect in openCamera().
>>
>> Aha - that's much better and clearly obvious :)
>>
>> I'll update to do so.
>>
>>>> +
>>>> +	startCapture();
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  std::string MainWindow::chooseCamera(CameraManager *cm)
>>>>  {
>>>>  	QStringList cameras;
>>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
>>>> index a11443b30b37..f7c96fdd5c30 100644
>>>> --- a/src/qcam/main_window.h
>>>> +++ b/src/qcam/main_window.h
>>>> @@ -44,7 +44,10 @@ private Q_SLOTS:
>>>>  	void quit();
>>>>  	void updateTitle();
>>>>  
>>>> +	int setCamera(const std::shared_ptr<Camera> &cam);
>>>> +
>>>>  private:
>>>> +	int createToolbars(CameraManager *cm);
>>>>  	std::string chooseCamera(CameraManager *cm);
>>>>  	int openCamera(CameraManager *cm);
>>>>  
>>>> @@ -71,6 +74,7 @@ private:
>>>>  	uint32_t previousFrames_;
>>>>  	uint32_t framesCaptured_;
>>>>  
>>>> +	QToolBar *toolbar_;
>>>>  	ViewFinder *viewfinder_;
>>>>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
>>>>  };
>

On Fri, Feb 07, 2020 at 10:21:48AM +0000, Kieran Bingham wrote:
> On 07/02/2020 10:13, Laurent Pinchart wrote:
> > On Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:
> >> On 06/02/2020 23:28, Laurent Pinchart wrote:
> >>> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:
> >>>> Implement a quit button, and a list of cameras.
> >>>>
> >>>> Selecting a different camera from the Toolbar will stop the current
> >>>> stream, and start streaming the chosen camera device if it can be
> >>>> acquired.
> >>>>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>>> ---
> >>>>  src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
> >>>>  src/qcam/main_window.h   |  4 +++
> >>>>  2 files changed, 64 insertions(+)
> >>>>
> >>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> >>>> index b51a16de199d..1c7260f32d0a 100644
> >>>> --- a/src/qcam/main_window.cpp
> >>>> +++ b/src/qcam/main_window.cpp
> >>>> @@ -13,6 +13,8 @@
> >>>>  #include <QCoreApplication>
> >>>>  #include <QInputDialog>
> >>>>  #include <QTimer>
> >>>> +#include <QToolBar>
> >>>> +#include <QToolButton>
> >>>>  
> >>>>  #include <libcamera/camera_manager.h>
> >>>>  #include <libcamera/version.h>
> >>>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >>>>  {
> >>>>  	int ret;
> >>>>  
> >>>> +	createToolbars(cm);
> >>>> +
> >>>>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
> >>>>  	setWindowTitle(title_);
> >>>>  	connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
> >>>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> +int MainWindow::createToolbars(CameraManager *cm)
> >>>> +{
> >>>> +	QAction *action;
> >>>> +
> >>>> +	toolbar_ = addToolBar("");
> >>>
> >>> You should give a name to the toolbar, otherwise the context menu will
> >>> have an empty entry. You can call it "Main" or anything similar to start
> >>> with.
> >>
> >> Which context menu?
> >>
> >> I'm not sure I understand the need. I mean, I don't care, I can add the
> >> name - I just can't see /why/ a toolbar needs a name :-)
> >>
> >> Ugh ... I see right clicking on the toolbar lets you hide it and then
> >> you can't get it back again. So /that's/ the context menu ...
> >>
> >> Should the toolbar be 'permanant'? Or removable - and if removable, how
> >> would we expect to get it back. Keyboard shortcut?
> > 
> > If you can make it permanent I think that would be best.
> 
> Agreed.
> 
> >> Perhaps removable is useful to be able to simplify the view - but as
> >> this is just a test tool - I don't see the benefit yet.
> >>
> >> I'll try to look at disabling the context menu or making the main
> >> toolbar permanent.
> >>
> >>>> +
> >>>> +	action = toolbar_->addAction("Quit");
> >>>> +	connect(action, &QAction::triggered, this, &MainWindow::quit);
> >>>> +
> >>>> +	QAction *cameraAction = new QAction("&Cameras", this);
> >>>> +	toolbar_->addAction(cameraAction);
> >>>> +
> >>>> +	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
> >>>> +
> >>>> +	cameraButton->setPopupMode(QToolButton::InstantPopup);
> >>>
> >>> Any way we could remove the "Camera" entry from the popup menu ? It
> >>> seems we may have to insert a manually-created QComboBox. My initial
> >>> expriment resulted in the following code. Feel free to fold it in,
> >>> modify it, or ignore it if you think it's not a good idea.
> >>
> >> I would actually like this entry to show the current camera in the toolbar.
> >>
> >> And yes the duplicated entry is annoying.
> >>
> >> But I haven't got as far as dealing with such UX issues.
> >> I was focussing on getting the core code to work.
> >>
> >> I'll work through your code and see what how it integrates.
> >>
> >>
> >> Hrm ... one part I don't like about the below is selecting a camera by
> >> index. That seems quite fragile once we have hotplug support ?
> > 
> > Agreed, but once we have hotplug support we'll have to remove and add
> > entries from the combo box or popup menu, so refactoring will be needed
> > anyway. I think hotplug support will also require stable names/IDs, so
> > we should then have the tool we need to do the job.
> 
> I think my point is - we already have one at this level. A shared_ptr to
> the Camera instance. That will always point to the same camera, and
> never change.
> 
> And in the event that the camera is removed, then that Camera will be
> marked disconnected, so it will fail to stream - but the 'object' will
> be safely preserved until there are no users left?

Correct, but I think we'll have to grey it out in the menu to make sure
it can't be selected, remove it when appropriate, and handle all other
kinds of GUI niceties. As the position in the cameras array won't be a
stable key anymore, we'll have to lookup combo box entries through a
different mean, requiring some form of stable and unique ID. That will
result in quite a bit of refactoring, that's why I'm not concerned about
using the index of now.

> >>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> >>> index 0e994b1e9197..f68b7abccda6 100644
> >>> --- a/src/qcam/main_window.cpp
> >>> +++ b/src/qcam/main_window.cpp
> >>> @@ -10,6 +10,7 @@
> >>>  #include <string>
> >>>  #include <sys/mman.h>
> >>>
> >>> +#include <QComboBox>
> >>>  #include <QCoreApplication>
> >>>  #include <QFileDialog>
> >>>  #include <QIcon>
> >>> @@ -29,11 +30,11 @@
> >>>  using namespace libcamera;
> >>>
> >>>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >>> -	: options_(options), allocator_(nullptr), isCapturing_(false)
> >>> +	: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)
> >>>  {
> >>>  	int ret;
> >>>
> >>> -	createToolbars(cm);
> >>> +	createToolbars();
> >>>
> >>>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
> >>>  	setWindowTitle(title_);
> >>> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >>>  	setCentralWidget(viewfinder_);
> >>>  	adjustSize();
> >>>
> >>> -	ret = openCamera(cm);
> >>> +	ret = openCamera();
> >>>  	if (!ret) {
> >>>  		ret = startCapture();
> >>>  	}
> >>> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()
> >>>  	}
> >>>  }
> >>>
> >>> -int MainWindow::createToolbars(CameraManager *cm)
> >>> +int MainWindow::createToolbars()
> >>>  {
> >>>  	QAction *action;
> >>>
> >>> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)
> >>>  	action = toolbar_->addAction(QIcon(":x-circle.svg"), "Quit");
> >>>  	connect(action, &QAction::triggered, this, &MainWindow::quit);
> >>>
> >>> -	QAction *cameraAction = new QAction("&Cameras", this);
> >>> -	toolbar_->addAction(cameraAction);
> >>> +	QComboBox *cameraCombo = new QComboBox();
> >>> +	connect(cameraCombo, QOverload<int>::of(&QComboBox::activated),
> >>> +		this, &MainWindow::setCamera);
> >>>
> >>> -	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
> >>> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> >>> +		cameraCombo->addItem(QString::fromStdString(cam->name()));
> >>>
> >>> -	cameraButton->setPopupMode(QToolButton::InstantPopup);
> >>> -
> >>> -	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
> >>> -		action = new QAction(QString::fromStdString(cam->name()));
> >>> -		cameraButton->addAction(action);
> >>> -		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
> >>> -	}
> >>> +	toolbar_->addWidget(cameraCombo);
> >>>
> >>>  	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
> >>>  	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
> >>> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()
> >>>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
> >>>  }
> >>>
> >>> -int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> >>> +void MainWindow::setCamera(int index)
> >>>  {
> >>> +	const auto &cameras = cm_->cameras();
> >>> +	if (static_cast<unsigned int>(index) >= cameras.size())
> >>> +		return;
> >>> +
> >>> +	std::shared_ptr<Camera> cam = cameras[index];
> >>> +
> >>>  	std::cout << "Chose " << cam->name() << std::endl;
> >>
> >> I'll remove this debug print and print the camera name if it fails to
> >> acquire.
> >>
> >>>  	if (cam->acquire()) {
> >>>  		std::cout << "Failed to acquire camera" << std::endl;
> >>> -		return -EBUSY;
> >>> +		return;
> >>>  	}
> >>>
> >>>  	std::cout << "Switching to camera " << cam->name() << std::endl;
> >>> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> >>>  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> >>>
> >>>  	startCapture();
> >>> -
> >>> -	return 0;
> >>>  }
> >>>
> >>> -std::string MainWindow::chooseCamera(CameraManager *cm)
> >>> +std::string MainWindow::chooseCamera()
> >>>  {
> >>>  	QStringList cameras;
> >>>  	bool result;
> >>>
> >>> -	if (cm->cameras().size() == 1)
> >>> -		return cm->cameras()[0]->name();
> >>> +	if (cm_->cameras().size() == 1)
> >>> +		return cm_->cameras()[0]->name();
> >>>
> >>> -	for (const std::shared_ptr<Camera> &cam : cm->cameras())
> >>> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> >>>  		cameras.append(QString::fromStdString(cam->name()));
> >>>
> >>>  	QString name = QInputDialog::getItem(this, "Select Camera",
> >>> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)
> >>>  	return name.toStdString();
> >>>  }
> >>>
> >>> -int MainWindow::openCamera(CameraManager *cm)
> >>> +int MainWindow::openCamera()
> >>>  {
> >>>  	std::string cameraName;
> >>>
> >>>  	if (options_.isSet(OptCamera))
> >>>  		cameraName = static_cast<std::string>(options_[OptCamera]);
> >>>  	else
> >>> -		cameraName = chooseCamera(cm);
> >>> +		cameraName = chooseCamera();
> >>>
> >>>  	if (cameraName == "")
> >>>  		return -EINVAL;
> >>>
> >>> -	camera_ = cm->get(cameraName);
> >>> +	camera_ = cm_->get(cameraName);
> >>>  	if (!camera_) {
> >>>  		std::cout << "Camera " << cameraName << " not found"
> >>>  			  << std::endl;
> >>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> >>> index fc85b6a46491..49af0f6ad934 100644
> >>> --- a/src/qcam/main_window.h
> >>> +++ b/src/qcam/main_window.h
> >>> @@ -44,19 +44,21 @@ private Q_SLOTS:
> >>>  	void quit();
> >>>  	void updateTitle();
> >>>
> >>> -	int setCamera(const std::shared_ptr<Camera> &cam);
> >>> +	void setCamera(int index);
> >>>
> >>>  	int startCapture();
> >>>  	void stopCapture();
> >>>  	void saveImage();
> >>>
> >>>  private:
> >>> -	int createToolbars(CameraManager *cm);
> >>> -	std::string chooseCamera(CameraManager *cm);
> >>> -	int openCamera(CameraManager *cm);
> >>> +	int createToolbars();
> >>> +	std::string chooseCamera();
> >>> +	int openCamera();
> >>>  	void requestComplete(Request *request);
> >>>  	int display(FrameBuffer *buffer);
> >>>
> >>> +	CameraManager *cm_;
> >>> +
> >>>  	QString title_;
> >>>  	QTimer titleTimer_;
> >>>
> >>>> +
> >>>> +	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
> >>>> +		action = new QAction(QString::fromStdString(cam->name()));
> >>>> +		cameraButton->addAction(action);
> >>>> +		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
> >>>
> >>> Are you aware that this will create local copies of the
> >>> std::shared_ptr<Camera> for each camera ? Probably not a big deal.
> >>
> >> Yes, I thought that was needed to make sure they remain in scope. And as
> >> they are shared_ptr that should be fine right?
> > 
> > I think it's OK, I just wanted to point it out as lambda capture can
> > sometimes be confusing.
> 
> Indeed, but the alternatives felt uglier to code.
> Essentially I'm only using a lambda to give convenient means of passing
> parameters through the signal event.
> 
> If there's a 'neat' way to do so natively in SIGNAL/SLOT I'm open to
> more suggestions, but when I looked into it, it seemed required to just
> make an extra function which dealt with it, and at that point - a lambda
> does that inline.

It sure does the job :-)

> >> The alternative is to pass the QAction, and get the camera name from
> >> there, or pass the cam->name() itself and then get the camera by name.
> >>
> >> I thought as we already have the 'Camera' this would be better - but I
> >> can change if it's a problem.
> >>
> >> I see in your implementation you move to an index, but I fear this would
> >> cause problems with hotplug support. But maybe that will be painful
> >> anyway, and we'll have to reconstruct the camera list to update anytime
> >> the camera list changes regardless.
> > 
> > The reason I move to an index is becase the activated signal only gives
> > an index. There's a way to associate a QVariant to a combo box entry,
> > maybe we can store the shared_ptr in the QVariant, but I haven't tried
> > that.
> 
> If you've got the QAction* you can get the camera name from
> QAction->text() I think...
> 
> Oh - but perhaps you don't even have that. Perhaps I'm conflating what
> was my first approach :-)

And the camera name isn't unique :-)

> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  void MainWindow::quit()
> >>>>  {
> >>>>  	QTimer::singleShot(0, QCoreApplication::instance(),
> >>>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()
> >>>>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
> >>>>  }
> >>>>  
> >>>> +int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
> >>>
> >>> You can make this a void function, the return value of slots is ignored
> >>> when connected to signals.
> >>
> >> Ah indeed, I started out with this as a void, I must have changed it to
> >> return int when I pulled over the code with the -EBUSY.
> >>
> >> But we can simply ignore that return value indeed, no action will occur.
> >>
> >> Later it would be nice if we had a status bar to report specific
> >> messages through the UI. But that's a 'later' task.
> >>
> >> Or maybe a workshop style activity ... ;-)
> >>
> >>>> +{
> >>>> +	std::cout << "Chose " << cam->name() << std::endl;
> >>>> +
> >>>> +	if (cam->acquire()) {
> >>>> +		std::cout << "Failed to acquire camera" << std::endl;
> >>>> +		return -EBUSY;
> >>>> +	}
> >>>> +
> >>>> +	std::cout << "Switching to camera " << cam->name() << std::endl;
> >>>> +
> >>>> +	stopCapture();
> >>>> +	camera_->release();
> >>>> +
> >>>> +	/*
> >>>> +	 * If we don't disconnect this signal, it will persist (and get
> >>>> +	 * re-added and thus duplicated later if we ever switch back to an
> >>>> +	 * previously streamed camera). This causes all sorts of pain.
> >>>> +	 *
> >>>> +	 * Perhaps releasing a camera should disconnect all (public?) connected
> >>>> +	 * signals forcefully!
> >>>
> >>> I'm not sure that would be a good idea, implicit actions are usually
> >>> confusing.
> >>>
> >>>> +	 */
> >>>> +	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
> >>>> +	camera_ = cam;
> >>>> +	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> >>>
> >>> How about connecting the signal in startCapture() and disconnecting it
> >>> in stopCapture() ? It will avoid the duplicated connect in openCamera().
> >>
> >> Aha - that's much better and clearly obvious :)
> >>
> >> I'll update to do so.
> >>
> >>>> +
> >>>> +	startCapture();
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  std::string MainWindow::chooseCamera(CameraManager *cm)
> >>>>  {
> >>>>  	QStringList cameras;
> >>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> >>>> index a11443b30b37..f7c96fdd5c30 100644
> >>>> --- a/src/qcam/main_window.h
> >>>> +++ b/src/qcam/main_window.h
> >>>> @@ -44,7 +44,10 @@ private Q_SLOTS:
> >>>>  	void quit();
> >>>>  	void updateTitle();
> >>>>  
> >>>> +	int setCamera(const std::shared_ptr<Camera> &cam);
> >>>> +
> >>>>  private:
> >>>> +	int createToolbars(CameraManager *cm);
> >>>>  	std::string chooseCamera(CameraManager *cm);
> >>>>  	int openCamera(CameraManager *cm);
> >>>>  
> >>>> @@ -71,6 +74,7 @@ private:
> >>>>  	uint32_t previousFrames_;
> >>>>  	uint32_t framesCaptured_;
> >>>>  
> >>>> +	QToolBar *toolbar_;
> >>>>  	ViewFinder *viewfinder_;
> >>>>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
> >>>>  };

Hi Laurent,

On 07/02/2020 10:39, Laurent Pinchart wrote:
> On Fri, Feb 07, 2020 at 10:21:48AM +0000, Kieran Bingham wrote:
>> On 07/02/2020 10:13, Laurent Pinchart wrote:
>>> On Fri, Feb 07, 2020 at 10:02:23AM +0000, Kieran Bingham wrote:
>>>> On 06/02/2020 23:28, Laurent Pinchart wrote:
>>>>> On Thu, Feb 06, 2020 at 03:05:01PM +0000, Kieran Bingham wrote:
>>>>>> Implement a quit button, and a list of cameras.
>>>>>>
>>>>>> Selecting a different camera from the Toolbar will stop the current
>>>>>> stream, and start streaming the chosen camera device if it can be
>>>>>> acquired.
>>>>>>
>>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>>> ---
>>>>>>  src/qcam/main_window.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
>>>>>>  src/qcam/main_window.h   |  4 +++
>>>>>>  2 files changed, 64 insertions(+)
>>>>>>
>>>>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>>>>>> index b51a16de199d..1c7260f32d0a 100644
>>>>>> --- a/src/qcam/main_window.cpp
>>>>>> +++ b/src/qcam/main_window.cpp
>>>>>> @@ -13,6 +13,8 @@
>>>>>>  #include <QCoreApplication>
>>>>>>  #include <QInputDialog>
>>>>>>  #include <QTimer>
>>>>>> +#include <QToolBar>
>>>>>> +#include <QToolButton>
>>>>>>  
>>>>>>  #include <libcamera/camera_manager.h>
>>>>>>  #include <libcamera/version.h>
>>>>>> @@ -27,6 +29,8 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>>>>>  {
>>>>>>  	int ret;
>>>>>>  
>>>>>> +	createToolbars(cm);
>>>>>> +
>>>>>>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
>>>>>>  	setWindowTitle(title_);
>>>>>>  	connect(&titleTimer_, SIGNAL(timeout()), this, SLOT(updateTitle()));
>>>>>> @@ -53,6 +57,31 @@ MainWindow::~MainWindow()
>>>>>>  	}
>>>>>>  }
>>>>>>  
>>>>>> +int MainWindow::createToolbars(CameraManager *cm)
>>>>>> +{
>>>>>> +	QAction *action;
>>>>>> +
>>>>>> +	toolbar_ = addToolBar("");
>>>>>
>>>>> You should give a name to the toolbar, otherwise the context menu will
>>>>> have an empty entry. You can call it "Main" or anything similar to start
>>>>> with.
>>>>
>>>> Which context menu?
>>>>
>>>> I'm not sure I understand the need. I mean, I don't care, I can add the
>>>> name - I just can't see /why/ a toolbar needs a name :-)
>>>>
>>>> Ugh ... I see right clicking on the toolbar lets you hide it and then
>>>> you can't get it back again. So /that's/ the context menu ...
>>>>
>>>> Should the toolbar be 'permanant'? Or removable - and if removable, how
>>>> would we expect to get it back. Keyboard shortcut?
>>>
>>> If you can make it permanent I think that would be best.
>>
>> Agreed.


I've figured out how to make this permanent.

>>
>>>> Perhaps removable is useful to be able to simplify the view - but as
>>>> this is just a test tool - I don't see the benefit yet.
>>>>
>>>> I'll try to look at disabling the context menu or making the main
>>>> toolbar permanent.
>>>>
>>>>>> +
>>>>>> +	action = toolbar_->addAction("Quit");
>>>>>> +	connect(action, &QAction::triggered, this, &MainWindow::quit);
>>>>>> +
>>>>>> +	QAction *cameraAction = new QAction("&Cameras", this);
>>>>>> +	toolbar_->addAction(cameraAction);
>>>>>> +
>>>>>> +	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
>>>>>> +
>>>>>> +	cameraButton->setPopupMode(QToolButton::InstantPopup);
>>>>>
>>>>> Any way we could remove the "Camera" entry from the popup menu ? It
>>>>> seems we may have to insert a manually-created QComboBox. My initial
>>>>> expriment resulted in the following code. Feel free to fold it in,
>>>>> modify it, or ignore it if you think it's not a good idea.
>>>>
>>>> I would actually like this entry to show the current camera in the toolbar.
>>>>
>>>> And yes the duplicated entry is annoying.
>>>>
>>>> But I haven't got as far as dealing with such UX issues.
>>>> I was focussing on getting the core code to work.
>>>>
>>>> I'll work through your code and see what how it integrates.
>>>>
>>>>
>>>> Hrm ... one part I don't like about the below is selecting a camera by
>>>> index. That seems quite fragile once we have hotplug support ?
>>>
>>> Agreed, but once we have hotplug support we'll have to remove and add
>>> entries from the combo box or popup menu, so refactoring will be needed
>>> anyway. I think hotplug support will also require stable names/IDs, so
>>> we should then have the tool we need to do the job.
>>
>> I think my point is - we already have one at this level. A shared_ptr to
>> the Camera instance. That will always point to the same camera, and
>> never change.
>>
>> And in the event that the camera is removed, then that Camera will be
>> marked disconnected, so it will fail to stream - but the 'object' will
>> be safely preserved until there are no users left?
> 
> Correct, but I think we'll have to grey it out in the menu to make sure
> it can't be selected, remove it when appropriate, and handle all other
> kinds of GUI niceties. As the position in the cameras array won't be a
> stable key anymore, we'll have to lookup combo box entries through a
> different mean, requiring some form of stable and unique ID. That will
> result in quite a bit of refactoring, that's why I'm not concerned about
> using the index of now.

QComboBox from below looks nice.
Merged in to the patch.


>>>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>>>>> index 0e994b1e9197..f68b7abccda6 100644
>>>>> --- a/src/qcam/main_window.cpp
>>>>> +++ b/src/qcam/main_window.cpp
>>>>> @@ -10,6 +10,7 @@
>>>>>  #include <string>
>>>>>  #include <sys/mman.h>
>>>>>
>>>>> +#include <QComboBox>
>>>>>  #include <QCoreApplication>
>>>>>  #include <QFileDialog>
>>>>>  #include <QIcon>
>>>>> @@ -29,11 +30,11 @@
>>>>>  using namespace libcamera;
>>>>>
>>>>>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>>>> -	: options_(options), allocator_(nullptr), isCapturing_(false)
>>>>> +	: cm_(cm), options_(options), allocator_(nullptr), isCapturing_(false)
>>>>>  {
>>>>>  	int ret;
>>>>>
>>>>> -	createToolbars(cm);
>>>>> +	createToolbars();
>>>>>
>>>>>  	title_ = "QCam " + QString::fromStdString(CameraManager::version());
>>>>>  	setWindowTitle(title_);
>>>>> @@ -43,7 +44,7 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>>>>  	setCentralWidget(viewfinder_);
>>>>>  	adjustSize();
>>>>>
>>>>> -	ret = openCamera(cm);
>>>>> +	ret = openCamera();
>>>>>  	if (!ret) {
>>>>>  		ret = startCapture();
>>>>>  	}
>>>>> @@ -61,7 +62,7 @@ MainWindow::~MainWindow()
>>>>>  	}
>>>>>  }
>>>>>
>>>>> -int MainWindow::createToolbars(CameraManager *cm)
>>>>> +int MainWindow::createToolbars()
>>>>>  {
>>>>>  	QAction *action;
>>>>>
>>>>> @@ -70,18 +71,14 @@ int MainWindow::createToolbars(CameraManager *cm)
>>>>>  	action = toolbar_->addAction(QIcon(":x-circle.svg"), "Quit");
>>>>>  	connect(action, &QAction::triggered, this, &MainWindow::quit);
>>>>>
>>>>> -	QAction *cameraAction = new QAction("&Cameras", this);
>>>>> -	toolbar_->addAction(cameraAction);
>>>>> +	QComboBox *cameraCombo = new QComboBox();
>>>>> +	connect(cameraCombo, QOverload<int>::of(&QComboBox::activated),
>>>>> +		this, &MainWindow::setCamera);
>>>>>
>>>>> -	QToolButton *cameraButton = dynamic_cast<QToolButton *>(toolbar_->widgetForAction(cameraAction));
>>>>> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
>>>>> +		cameraCombo->addItem(QString::fromStdString(cam->name()));
>>>>>
>>>>> -	cameraButton->setPopupMode(QToolButton::InstantPopup);
>>>>> -
>>>>> -	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
>>>>> -		action = new QAction(QString::fromStdString(cam->name()));
>>>>> -		cameraButton->addAction(action);
>>>>> -		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
>>>>> -	}
>>>>> +	toolbar_->addWidget(cameraCombo);
>>>>>
>>>>>  	action = toolbar_->addAction(QIcon(":play-circle.svg"), "start");
>>>>>  	connect(action, &QAction::triggered, this, &MainWindow::startCapture);
>>>>> @@ -117,13 +114,19 @@ void MainWindow::updateTitle()
>>>>>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
>>>>>  }
>>>>>
>>>>> -int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
>>>>> +void MainWindow::setCamera(int index)
>>>>>  {
>>>>> +	const auto &cameras = cm_->cameras();
>>>>> +	if (static_cast<unsigned int>(index) >= cameras.size())
>>>>> +		return;
>>>>> +
>>>>> +	std::shared_ptr<Camera> cam = cameras[index];
>>>>> +
>>>>>  	std::cout << "Chose " << cam->name() << std::endl;
>>>>
>>>> I'll remove this debug print and print the camera name if it fails to
>>>> acquire.
>>>>
>>>>>  	if (cam->acquire()) {
>>>>>  		std::cout << "Failed to acquire camera" << std::endl;
>>>>> -		return -EBUSY;
>>>>> +		return;
>>>>>  	}
>>>>>
>>>>>  	std::cout << "Switching to camera " << cam->name() << std::endl;
>>>>> @@ -144,19 +147,17 @@ int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
>>>>>  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
>>>>>
>>>>>  	startCapture();
>>>>> -
>>>>> -	return 0;
>>>>>  }
>>>>>
>>>>> -std::string MainWindow::chooseCamera(CameraManager *cm)
>>>>> +std::string MainWindow::chooseCamera()
>>>>>  {
>>>>>  	QStringList cameras;
>>>>>  	bool result;
>>>>>
>>>>> -	if (cm->cameras().size() == 1)
>>>>> -		return cm->cameras()[0]->name();
>>>>> +	if (cm_->cameras().size() == 1)
>>>>> +		return cm_->cameras()[0]->name();
>>>>>
>>>>> -	for (const std::shared_ptr<Camera> &cam : cm->cameras())
>>>>> +	for (const std::shared_ptr<Camera> &cam : cm_->cameras())
>>>>>  		cameras.append(QString::fromStdString(cam->name()));
>>>>>
>>>>>  	QString name = QInputDialog::getItem(this, "Select Camera",
>>>>> @@ -168,19 +169,19 @@ std::string MainWindow::chooseCamera(CameraManager *cm)
>>>>>  	return name.toStdString();
>>>>>  }
>>>>>
>>>>> -int MainWindow::openCamera(CameraManager *cm)
>>>>> +int MainWindow::openCamera()
>>>>>  {
>>>>>  	std::string cameraName;
>>>>>
>>>>>  	if (options_.isSet(OptCamera))
>>>>>  		cameraName = static_cast<std::string>(options_[OptCamera]);
>>>>>  	else
>>>>> -		cameraName = chooseCamera(cm);
>>>>> +		cameraName = chooseCamera();
>>>>>
>>>>>  	if (cameraName == "")
>>>>>  		return -EINVAL;
>>>>>
>>>>> -	camera_ = cm->get(cameraName);
>>>>> +	camera_ = cm_->get(cameraName);
>>>>>  	if (!camera_) {
>>>>>  		std::cout << "Camera " << cameraName << " not found"
>>>>>  			  << std::endl;
>>>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
>>>>> index fc85b6a46491..49af0f6ad934 100644
>>>>> --- a/src/qcam/main_window.h
>>>>> +++ b/src/qcam/main_window.h
>>>>> @@ -44,19 +44,21 @@ private Q_SLOTS:
>>>>>  	void quit();
>>>>>  	void updateTitle();
>>>>>
>>>>> -	int setCamera(const std::shared_ptr<Camera> &cam);
>>>>> +	void setCamera(int index);
>>>>>
>>>>>  	int startCapture();
>>>>>  	void stopCapture();
>>>>>  	void saveImage();
>>>>>
>>>>>  private:
>>>>> -	int createToolbars(CameraManager *cm);
>>>>> -	std::string chooseCamera(CameraManager *cm);
>>>>> -	int openCamera(CameraManager *cm);
>>>>> +	int createToolbars();
>>>>> +	std::string chooseCamera();
>>>>> +	int openCamera();
>>>>>  	void requestComplete(Request *request);
>>>>>  	int display(FrameBuffer *buffer);
>>>>>
>>>>> +	CameraManager *cm_;
>>>>> +
>>>>>  	QString title_;
>>>>>  	QTimer titleTimer_;
>>>>>
>>>>>> +
>>>>>> +	for (const std::shared_ptr<Camera> &cam : cm->cameras()) {
>>>>>> +		action = new QAction(QString::fromStdString(cam->name()));
>>>>>> +		cameraButton->addAction(action);
>>>>>> +		connect(action, &QAction::triggered, this, [=]() { this->setCamera(cam); });
>>>>>
>>>>> Are you aware that this will create local copies of the
>>>>> std::shared_ptr<Camera> for each camera ? Probably not a big deal.
>>>>
>>>> Yes, I thought that was needed to make sure they remain in scope. And as
>>>> they are shared_ptr that should be fine right?
>>>
>>> I think it's OK, I just wanted to point it out as lambda capture can
>>> sometimes be confusing.
>>
>> Indeed, but the alternatives felt uglier to code.
>> Essentially I'm only using a lambda to give convenient means of passing
>> parameters through the signal event.
>>
>> If there's a 'neat' way to do so natively in SIGNAL/SLOT I'm open to
>> more suggestions, but when I looked into it, it seemed required to just
>> make an extra function which dealt with it, and at that point - a lambda
>> does that inline.
> 
> It sure does the job :-)
> 
>>>> The alternative is to pass the QAction, and get the camera name from
>>>> there, or pass the cam->name() itself and then get the camera by name.
>>>>
>>>> I thought as we already have the 'Camera' this would be better - but I
>>>> can change if it's a problem.
>>>>
>>>> I see in your implementation you move to an index, but I fear this would
>>>> cause problems with hotplug support. But maybe that will be painful
>>>> anyway, and we'll have to reconstruct the camera list to update anytime
>>>> the camera list changes regardless.
>>>
>>> The reason I move to an index is becase the activated signal only gives
>>> an index. There's a way to associate a QVariant to a combo box entry,
>>> maybe we can store the shared_ptr in the QVariant, but I haven't tried
>>> that.
>>
>> If you've got the QAction* you can get the camera name from
>> QAction->text() I think...
>>
>> Oh - but perhaps you don't even have that. Perhaps I'm conflating what
>> was my first approach :-)
> 
> And the camera name isn't unique :-)
> 
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>>  void MainWindow::quit()
>>>>>>  {
>>>>>>  	QTimer::singleShot(0, QCoreApplication::instance(),
>>>>>> @@ -72,6 +101,37 @@ void MainWindow::updateTitle()
>>>>>>  	setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps");
>>>>>>  }
>>>>>>  
>>>>>> +int MainWindow::setCamera(const std::shared_ptr<Camera> &cam)
>>>>>
>>>>> You can make this a void function, the return value of slots is ignored
>>>>> when connected to signals.
>>>>
>>>> Ah indeed, I started out with this as a void, I must have changed it to
>>>> return int when I pulled over the code with the -EBUSY.
>>>>
>>>> But we can simply ignore that return value indeed, no action will occur.
>>>>
>>>> Later it would be nice if we had a status bar to report specific
>>>> messages through the UI. But that's a 'later' task.
>>>>
>>>> Or maybe a workshop style activity ... ;-)
>>>>
>>>>>> +{
>>>>>> +	std::cout << "Chose " << cam->name() << std::endl;
>>>>>> +
>>>>>> +	if (cam->acquire()) {
>>>>>> +		std::cout << "Failed to acquire camera" << std::endl;
>>>>>> +		return -EBUSY;
>>>>>> +	}
>>>>>> +
>>>>>> +	std::cout << "Switching to camera " << cam->name() << std::endl;
>>>>>> +
>>>>>> +	stopCapture();
>>>>>> +	camera_->release();
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If we don't disconnect this signal, it will persist (and get
>>>>>> +	 * re-added and thus duplicated later if we ever switch back to an
>>>>>> +	 * previously streamed camera). This causes all sorts of pain.
>>>>>> +	 *
>>>>>> +	 * Perhaps releasing a camera should disconnect all (public?) connected
>>>>>> +	 * signals forcefully!
>>>>>
>>>>> I'm not sure that would be a good idea, implicit actions are usually
>>>>> confusing.
>>>>>>>>>>> +	 */
>>>>>> +	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
>>>>>> +	camera_ = cam;
>>>>>> +	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
>>>>>
>>>>> How about connecting the signal in startCapture() and disconnecting it
>>>>> in stopCapture() ? It will avoid the duplicated connect in openCamera().
>>>>
>>>> Aha - that's much better and clearly obvious :)
>>>>
>>>> I'll update to do so.

This does indeed work much better from startCapture and removing in
stopCapture.

>>>>
>>>>>> +
>>>>>> +	startCapture();
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>>  std::string MainWindow::chooseCamera(CameraManager *cm)
>>>>>>  {
>>>>>>  	QStringList cameras;
>>>>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
>>>>>> index a11443b30b37..f7c96fdd5c30 100644
>>>>>> --- a/src/qcam/main_window.h
>>>>>> +++ b/src/qcam/main_window.h
>>>>>> @@ -44,7 +44,10 @@ private Q_SLOTS:
>>>>>>  	void quit();
>>>>>>  	void updateTitle();
>>>>>>  
>>>>>> +	int setCamera(const std::shared_ptr<Camera> &cam);
>>>>>> +
>>>>>>  private:
>>>>>> +	int createToolbars(CameraManager *cm);
>>>>>>  	std::string chooseCamera(CameraManager *cm);
>>>>>>  	int openCamera(CameraManager *cm);
>>>>>>  
>>>>>> @@ -71,6 +74,7 @@ private:
>>>>>>  	uint32_t previousFrames_;
>>>>>>  	uint32_t framesCaptured_;
>>>>>>  
>>>>>> +	QToolBar *toolbar_;
>>>>>>  	ViewFinder *viewfinder_;
>>>>>>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
>>>>>>  };
>