[libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add capture script button
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 1 12:08:29 CEST 2022
Hi Utkarsh,
On Thu, Sep 01, 2022 at 11:34:59AM +0530, Utkarsh Tiwari wrote:
> On Wed, Aug 31, 2022 at 12:59:12PM +0300, Laurent Pinchart wrote:
> > 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.
Does it ? I've tried replacing QToolButton with QPushButton and didn't
notice any difference in the dialog box.
> > > +#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.
Maybe QLabel isn't the right widget then. What I dislike is manual
styling, as it breaks user assumptions.
> > > +
> > > + 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.
If you reject the dialog you won't have selected a camera, so you won't
start capture, right ? There's thus no script 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