[libcamera-devel] [PATCH 3/6] qcam: Introduce a toolbar and camera switching
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Feb 7 11:39:16 CET 2020
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 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.
>
> 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_;
> >>>> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list