[libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add capture script button
Utkarsh Tiwari
utkarsh02t at gmail.com
Thu Sep 1 08:04:59 CEST 2022
Hi Laurent, thanks for the review.
On Wed, Aug 31, 2022 at 12:59:12PM +0300, Laurent Pinchart wrote:
> 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 :-)
:P would fix.
>
> > 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.
Oops would add that.
>
> > ---
> > 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.
Gotta learn to use <C-v><Tab> in vim.
>
> > <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.
QToolButton works much better in cases when we require only the icon.
>
> > +#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.
Sure.
>
> > + : 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 ?
Sure, maybe not in a Mike Tyson accept tho :P
>
> > + QHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);
>
> Ditto.
Noted.
>
> > +
> > + 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.
>
I think we the frame provides a border in which the user knows placing
the pointer would result in the tooltip showing up and displaying the
path. Provides more visual feedback.
> > +
> > + 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.
>
Ah, I think went overboard making it icon only, would do.
> > + connect(chooseCaptureScriptButton_, &QToolButton::clicked,
> > + this, &CameraSelectorDialog::selectCaptureScript);
> > +
> > + QToolButton *stopCaptureScriptButton = new QToolButton;
> > + stopCaptureScriptButton->setIcon(QIcon::fromTheme("edit-clear",
> > + QIcon(":delete.svg")));
> > + stopCaptureScriptButton->setStyleSheet("border:node;");
Same here.
> > + 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).
>
I agree, we only take scriptPath in std::string, we can convert that and
use QString everywhere here.
> > + 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();
>
Oops, makes much easier would use.
> > +}
> > +
> > +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.
>
I'm a bit doubtful on this,
1. I start a script.
2. Open the dialog select some different script.
3. Reject the dialog.
4. Few minutes pass
5. I open the CameraSelectorDialog I think I would expect the dialog
to show me the current script which is running.
In my opinion after doing any action, reject or accept the label should
show what has been already passed to loadCaptureScript().
Open to more suggetions.
> > + 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