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