[libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add capture script button
Utkarsh Tiwari
utkarsh02t at gmail.com
Thu Sep 1 07:48:42 CEST 2022
Hi Kieran, thanks for the review.
On Wed, Aug 31, 2022 at 10:35:31AM +0100, Kieran Bingham wrote:
> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-31 06:49:37)
> > Display a QLabel which is readonly, and displays the currently
> > 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_.
>
> "from the script" ?
Noted.
>
> > ---
> > 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>
> > <file>play-circle.svg</file>
> > + <file>upload.svg</file>
>
> Indentations are wrong here.
Ah should have not set the expandtabs. Would fix.
>
>
> > <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>
> > +#include <QWidget>
> >
> > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> > - QWidget *parent)
> > - : QDialog(parent), cm_(cameraManager)
> > + std::string scriptPath, QWidget *parent)
> > + : 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);
> > + QHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);
> > +
> > + scriptFileLine_ = new QLabel;
> > + scriptFileLine_->setFrameStyle(QFrame::Sunken | QFrame::StyledPanel);
> > +
> > + chooseCaptureScriptButton_ = new QToolButton;
> > + chooseCaptureScriptButton_->setIcon(QIcon::fromTheme("document-open",
> > + QIcon(":upload.svg")));
> > + chooseCaptureScriptButton_->setStyleSheet("border:none");
> > + 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();
>
> Where does checkstyle recommend to place this? It seems to be 'floating'
> too.
>
> It's tricky, but I'm not sure there is much better options anyway, so
> I'll move on.
>
> > +
> > + if (!selectedScriptPath_.empty()) {
> > + scriptFileInfo_.setFile(QString::fromStdString(selectedScriptPath_));
> > + scriptFileLine_->setText(scriptFileInfo_.fileName());
> > + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());
> > + } else {
> > + selectedScriptPath_ = scriptPath_;
> > + }
> > +}
> > +
> > +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());
> > + }
> > + 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();
> > +
> > + void accept() override;
> > + void reject() override;
> > +
> > +Q_SIGNALS:
> > + void stopCaptureScript();
> > +
> > private:
> > libcamera::CameraManager *cm_;
> >
> > + std::string scriptPath_;
> > + std::string selectedScriptPath_;
> > + QFileInfo scriptFileInfo_;
> > /* 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();
> > +
> > 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
>
> Perhaps:
> /*
> * If the camera has not changed, We still need to
> * handle any update to the capture script. This will
> * cause the script to restart if it was already
> * selected.
> */
>
>
> Or something like that (and correctly formatted).
Sure.
>
>
> > + 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();
> > +}
>
> This needs a blank line.
Sure. I thought I have fixed these in a rebase but seems not. anyway
seems like this would need v10.
>
> > +/**
> > + * \brief Loads and validates the current capture script
> > + *
>
> Can we describe here that the current capture session will be restarted
> if it was active?
Yes I think.
>
> > + * returns -EINVAL on failure and 0 on success
> > + */
> > +int MainWindow::loadCaptureScript()
> > +{
> > + if (scriptPath_.empty() || camera_ == nullptr)
> > + return -EINVAL;
>
> Perhaps -ENODEV? But I expect this can't happen. I think it's fine to be
> defensive here though.
Yes.
if(scriptPath_.empty())
return -EINVAL;
if(!camera_)
return -ENODEV;
>
> > +
> > + 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
>
> "have a stuck image"
>
> > + * in viewfinder.
>
> "in the viewfinder"
Noted.
>
> > + */
> > + bool wasCapturing = isCapturing_;
> > + if (isCapturing_)
> > + toggleCapture(false);
> > +
> > + script_ = std::move(script);
> > +
> > + /* Start capture again if we were capturing before. */
> > + if (wasCapturing)
> > + toggleCapture(true);
>
> New line here.
>
>
> All of those comments seem quite minor, so with those resolved:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > + return 0;
> > +}
> > +
> > std::string MainWindow::chooseCamera()
> > {
> > if (cameraSelectorDialog_->exec() != QDialog::Accepted)
> > return std::string();
> >
> > + scriptPath_ = cameraSelectorDialog_->getCaptureScript();
> > +
> > return cameraSelectorDialog_->getCameraId();
> > }
> >
> > @@ -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,
> > ]
> >
> > --
> > 2.34.1
> >
More information about the libcamera-devel
mailing list