[libcamera-devel,v9,6/7] qcam: CamSelectDialog: Add capture script button

17270 diff mbox series

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_.
---
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(-)

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" ?

> ---
> 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.


>         <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).


> +               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.

> +/**
> + * \brief Loads and validates the current capture script
> + *

Can we describe here that the current capture session will be restarted
if it was active?

> + * 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.

> +
> +       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"

> +        */
> +       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@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
>

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,
>  ]
>

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@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
> >

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

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,
> > >  ]
> > >