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

[Laurent Pinchart]

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 at 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_;
> >>  };

-- 
Regards,

Laurent Pinchart