[libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and camera switching
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Feb 7 11:02:23 CET 2020
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 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?
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_;
>> };
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list