[libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add capture script button
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 31 11:59:12 CEST 2022
Hi Utkarsh,
Thank you for the patch.
On Wed, Aug 31, 2022 at 11:19:37AM +0530, Utkarsh Tiwari via libcamera-devel wrote:
> Display a QLabel which is readonly, and displays the currently
Labels are always read-only :-)
> selected capture script. The tooltip of the QLabel displays the file
> path of the script.
>
> Implement a capture script button which is a QToolButton which when
> clicked opens a QFileDialog this allows to select a capture script
> (*.yaml) file.
>
> Next to the capture scipt button, show a QToolButton which stops the
> capture script.
>
> If an invalid script has been selected show a QMesssageBox::critical and
> continue with the capture's previous state.
>
> Introduce a queueCount_ to keep track of the requests queued.
>
> When stopping the execution of the capture script the queueCount_ is not
> reset and the capture continues as it is (i.e it is not stopped or
> restarted).
>
> Requests are queued with any controls the script matching the current
> queueCount_.
Missing SoB line.
> ---
> Differnce from v8:
> 1. Now display a QLabel with the fileName and filePath with button
> on the side.
> 2. infromScriptReset() informScriptRunning() are removed
> 3. Local script makes handling of invalid scripts easy.
> src/qcam/assets/feathericons/feathericons.qrc | 2 +
> src/qcam/cam_select_dialog.cpp | 88 ++++++++++++++++++-
> src/qcam/cam_select_dialog.h | 20 ++++-
> src/qcam/main_window.cpp | 67 +++++++++++++-
> src/qcam/main_window.h | 7 ++
> src/qcam/meson.build | 2 +
> 6 files changed, 181 insertions(+), 5 deletions(-)
>
> diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc
> index c5302040..0ea0c2d5 100644
> --- a/src/qcam/assets/feathericons/feathericons.qrc
> +++ b/src/qcam/assets/feathericons/feathericons.qrc
> @@ -3,7 +3,9 @@
> <qresource>
> <file>aperture.svg</file>
> <file>camera-off.svg</file>
> + <file>delete.svg</file>
Please indent with tabs, not spaces.
> <file>play-circle.svg</file>
> + <file>upload.svg</file>
Same here.
> <file>save.svg</file>
> <file>stop-circle.svg</file>
> <file>x-circle.svg</file>
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp
> index 6543228a..99405cc1 100644
> --- a/src/qcam/cam_select_dialog.cpp
> +++ b/src/qcam/cam_select_dialog.cpp
> @@ -14,13 +14,18 @@
>
> #include <QComboBox>
> #include <QDialogButtonBox>
> +#include <QFileDialog>
> #include <QFormLayout>
> +#include <QHBoxLayout>
> +#include <QIcon>
> #include <QLabel>
> #include <QString>
> +#include <QToolButton>
Why not a QPushButton ? QToolButton is usually meant for toolbars.
> +#include <QWidget>
>
> CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> - QWidget *parent)
> - : QDialog(parent), cm_(cameraManager)
> + std::string scriptPath, QWidget *parent)
Make it a const std::string & to avoid copies.
> + : QDialog(parent), cm_(cameraManager), scriptPath_(scriptPath)
> {
> /* Use a QFormLayout for the dialog. */
> QFormLayout *layout = new QFormLayout(this);
> @@ -38,6 +43,41 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
> connect(cameraIdComboBox_, &QComboBox::currentTextChanged,
> this, &CameraSelectorDialog::updateCamInfo);
>
> + /* Set capture script selection / removal button. */
> + QWidget *captureWidget = new QWidget(this);
Maybe captureScriptWidhet ?
> + QHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);
Ditto.
> +
> + scriptFileLine_ = new QLabel;
> + scriptFileLine_->setFrameStyle(QFrame::Sunken | QFrame::StyledPanel);
Generally speaking, changing styles manually should be avoided as much
as possible. It makes the overall style inconsistent and is confusing
for users. I'd drop this.
> +
> + chooseCaptureScriptButton_ = new QToolButton;
> + chooseCaptureScriptButton_->setIcon(QIcon::fromTheme("document-open",
> + QIcon(":upload.svg")));
> + chooseCaptureScriptButton_->setStyleSheet("border:none");
Same here, drop this. A button that doesn't look like a button gives no
visual clue that it can be clicked.
> + connect(chooseCaptureScriptButton_, &QToolButton::clicked,
> + this, &CameraSelectorDialog::selectCaptureScript);
> +
> + QToolButton *stopCaptureScriptButton = new QToolButton;
> + stopCaptureScriptButton->setIcon(QIcon::fromTheme("edit-clear",
> + QIcon(":delete.svg")));
> + stopCaptureScriptButton->setStyleSheet("border:node;");
> + connect(stopCaptureScriptButton, &QToolButton::clicked,
> + this, &CameraSelectorDialog::resetCaptureScript);
> +
> + captureLayout->addWidget(scriptFileLine_);
> + captureLayout->addWidget(chooseCaptureScriptButton_);
> + captureLayout->addWidget(stopCaptureScriptButton);
> + captureLayout->setMargin(0);
> +
> + /* Set the file name of the capture script. */
> + if (scriptPath_.empty()) {
> + scriptFileLine_->setText("No File Selected");
> + } else {
> + scriptFileInfo_.setFile(QString::fromStdString(scriptPath_));
> + scriptFileLine_->setText(scriptFileInfo_.fileName());
> + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());
> + }
> +
> /* Setup the QDialogButton Box */
> QDialogButtonBox *buttonBox =
> new QDialogButtonBox(QDialogButtonBox::Ok |
> @@ -52,6 +92,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
> layout->addRow("Camera:", cameraIdComboBox_);
> layout->addRow("Location:", cameraLocation_);
> layout->addRow("Model:", cameraModel_);
> + layout->addRow("Capture Script:", captureWidget);
> layout->addWidget(buttonBox);
> }
>
> @@ -110,3 +151,46 @@ void CameraSelectorDialog::updateCamInfo(QString cameraId)
>
> cameraModel_->setText(QString::fromStdString(model));
> }
> +
> +/* Capture script support. */
> +void CameraSelectorDialog::selectCaptureScript()
> +{
> + selectedScriptPath_ = QFileDialog::getOpenFileName(this,
> + "Run Capture Script", QDir::currentPath(),
> + "Capture Script (*.yaml)")
> + .toStdString();
> +
> + if (!selectedScriptPath_.empty()) {
> + scriptFileInfo_.setFile(QString::fromStdString(selectedScriptPath_));
You can make this a local variable:
QFileInfo fileInfo(QString::fromStdString(selectedScriptPath_));
Don't forget to move the #include <QFileInfo> from the header to this
file.
Furthermore, converting the QString to std::string to then convert it
back to QString here is not efficient. I would use QString everywhere in
this class and convert to std::string in the caller (MainWindow).
> + scriptFileLine_->setText(scriptFileInfo_.fileName());
> + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());
> + } else {
> + selectedScriptPath_ = scriptPath_;
> + }
This while logic looks weird, selectedScriptPath_ is modified when the
file dialog is rejected, to then be restored to the previous value.
Store the return value of getOpenFileName() in a local QString
scriptPath variable first, with then
if (scriptPath.isNull))
return;
QFileInfo fileInfo(scriptPath);
scriptFileLine_->setText(fileInfo.fileName());
scriptFileLine_->setToolTip(fileInfo.filePath());
selectedScriptPath_ = scriptPath.toStdString();
> +}
> +
> +void CameraSelectorDialog::resetCaptureScript()
> +{
> + Q_EMIT stopCaptureScript();
> + scriptPath_.clear();
> + selectedScriptPath_.clear();
> + scriptFileLine_->setText("No File Selected");
> +}
> +
> +void CameraSelectorDialog::accept()
> +{
> + scriptPath_ = selectedScriptPath_;
> + QDialog::accept();
> +}
> +
> +void CameraSelectorDialog::reject()
> +{
> + if (scriptPath_.empty()) {
> + scriptFileLine_->setText("No File Selected");
> + } else {
> + scriptFileInfo_.setFile(QString::fromStdString(scriptPath_));
> + scriptFileLine_->setText(scriptFileInfo_.fileName());
> + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());
> + }
No need for this. When the camera selector dialog is rejected it can
still remember which capture script has been selected. You can thus drop
the accept() and reject() overrides, and merge the two scriptPath_ and
selectedScriptPath_ variables into a single one.
> + QDialog::reject();
> +}
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> index c91b7ebe..377faebc 100644
> --- a/src/qcam/cam_select_dialog.h
> +++ b/src/qcam/cam_select_dialog.h
> @@ -15,21 +15,24 @@
> #include <libcamera/property_ids.h>
>
> #include <QDialog>
> +#include <QFileInfo>
> #include <QString>
>
> class QComboBox;
> class QLabel;
> +class QToolButton;
>
> class CameraSelectorDialog : public QDialog
> {
> Q_OBJECT
> public:
> CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> - QWidget *parent);
> + std::string scriptPath, QWidget *parent);
>
> ~CameraSelectorDialog();
>
> std::string getCameraId();
> + std::string getCaptureScript() { return scriptPath_; };
>
> /* Hotplug / Unplug Support. */
> void addCamera(QString cameraId);
> @@ -38,11 +41,26 @@ public:
> /* Camera Information */
> void updateCamInfo(QString cameraId);
>
> + /* Capture script support. */
> + void selectCaptureScript();
> + void resetCaptureScript();
These should be private Q_SLOTS.
> +
> + void accept() override;
> + void reject() override;
> +
> +Q_SIGNALS:
> + void stopCaptureScript();
Drop this signal, the capture script should be retrieved from the dialog
by the MainWindow class when the dialog is accepted, there's no need for
dynamic notification.
> +
> private:
> libcamera::CameraManager *cm_;
>
> + std::string scriptPath_;
> + std::string selectedScriptPath_;
> + QFileInfo scriptFileInfo_;
Add a blank line here.
> /* UI elements. */
> QComboBox *cameraIdComboBox_;
> QLabel *cameraLocation_;
> QLabel *cameraModel_;
> + QLabel *scriptFileLine_;
> + QToolButton *chooseCaptureScriptButton_;
> };
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 2a9ca830..af992b94 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -9,6 +9,7 @@
>
> #include <assert.h>
> #include <iomanip>
> +#include <memory>
> #include <string>
>
> #include <libcamera/camera_manager.h>
> @@ -18,6 +19,7 @@
> #include <QFileDialog>
> #include <QImage>
> #include <QImageWriter>
> +#include <QMessageBox>
> #include <QMutexLocker>
> #include <QStandardPaths>
> #include <QStringList>
> @@ -143,7 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> cm_->cameraAdded.connect(this, &MainWindow::addCamera);
> cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);
>
> - cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);
> + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);
> + connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
> + this, &MainWindow::stopCaptureScript);
>
> /* Open the camera and start capture. */
> ret = openCamera();
> @@ -152,6 +156,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> return;
> }
>
> + /* Start capture script. */
> + if (!scriptPath_.empty())
> + ret = loadCaptureScript();
ret isn't used.
> +
> startStopAction_->setChecked(true);
> }
>
> @@ -266,8 +274,11 @@ void MainWindow::switchCamera()
> if (newCameraId.empty())
> return;
>
> - if (camera_ && newCameraId == camera_->id())
> + if (camera_ && newCameraId == camera_->id()) {
> + // When user opens camera selection dialog for CaptureScript selection
C-style comments and line wrap.
> + loadCaptureScript();
> return;
> + }
>
> const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
>
> @@ -287,17 +298,63 @@ void MainWindow::switchCamera()
> camera_->release();
> camera_ = cam;
>
> + loadCaptureScript();
> +
> startStopAction_->setChecked(true);
>
> /* Display the current cameraId in the toolbar .*/
> cameraSelectButton_->setText(QString::fromStdString(newCameraId));
> }
>
> +void MainWindow::stopCaptureScript()
> +{
> + if (script_)
> + script_.reset();
> +}
Blank line.
> +/**
> + * \brief Loads and validates the current capture script
> + *
> + * returns -EINVAL on failure and 0 on success
> + */
We don't want to generate documentation from this file, so use /*
instead of /** and drop the \brief:
/*
* Load and validate the current capture script. Return -EINVAL on failure and 0
* on success.
*/
> +int MainWindow::loadCaptureScript()
> +{
> + if (scriptPath_.empty() || camera_ == nullptr)
> + return -EINVAL;
> +
> + auto script = std::make_unique<CaptureScript>(camera_, scriptPath_);
> +
> + if (!script->valid()) {
> + script.reset();
> +
> + QMessageBox::critical(this, "Invalid Script",
> + "Couldn't load the capture script");
> +
> + return -EINVAL;
> + }
> +
> + /*
> + * If we are already capturing, stop so we don't have stuck image
> + * in viewfinder.
> + */
> + bool wasCapturing = isCapturing_;
> + if (isCapturing_)
> + toggleCapture(false);
> +
> + script_ = std::move(script);
> +
> + /* Start capture again if we were capturing before. */
> + if (wasCapturing)
> + toggleCapture(true);
> + return 0;
> +}
> +
> std::string MainWindow::chooseCamera()
> {
> if (cameraSelectorDialog_->exec() != QDialog::Accepted)
> return std::string();
>
> + scriptPath_ = cameraSelectorDialog_->getCaptureScript();
> +
> return cameraSelectorDialog_->getCameraId();
I don't like much how the capture script is stored in a member variable
but the camera ID is returned. This isn't consistent, you should treat
both the same way (returning an std::tuple is an option if you want to
return them).
> }
>
> @@ -499,6 +556,7 @@ int MainWindow::startCapture()
> previousFrames_ = 0;
> framesCaptured_ = 0;
> lastBufferTime_ = 0;
> + queueCount_ = 0;
>
> ret = camera_->start();
> if (ret) {
> @@ -777,5 +835,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer)
>
> int MainWindow::queueRequest(Request *request)
> {
> + if (script_)
> + request->controls() = script_->frameControls(queueCount_);
> +
> + queueCount_++;
> +
> return camera_->queueRequest(request);
> }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 22c85247..7c877ae1 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -27,6 +27,7 @@
> #include <QQueue>
> #include <QTimer>
>
> +#include "../cam/capture_script.h"
> #include "../cam/stream_options.h"
>
> #include "viewfinder.h"
> @@ -89,6 +90,9 @@ private:
> void processHotplug(HotplugEvent *e);
> void processViewfinder(libcamera::FrameBuffer *buffer);
>
> + int loadCaptureScript();
> + void stopCaptureScript();
> +
> /* UI elements */
> QToolBar *toolbar_;
> QAction *startStopAction_;
> @@ -129,6 +133,9 @@ private:
> QElapsedTimer frameRateInterval_;
> uint32_t previousFrames_;
> uint32_t framesCaptured_;
> + uint32_t queueCount_;
>
> std::vector<std::unique_ptr<libcamera::Request>> requests_;
> + std::unique_ptr<CaptureScript> script_;
> + std::string scriptPath_;
> };
> diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> index 61861ea6..70a18d7e 100644
> --- a/src/qcam/meson.build
> +++ b/src/qcam/meson.build
> @@ -15,6 +15,7 @@ endif
> qcam_enabled = true
>
> qcam_sources = files([
> + '../cam/capture_script.cpp',
> '../cam/image.cpp',
> '../cam/options.cpp',
> '../cam/stream_options.cpp',
> @@ -39,6 +40,7 @@ qcam_resources = files([
> qcam_deps = [
> libatomic,
> libcamera_public,
> + libyaml,
> qt5_dep,
> ]
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list