[libcamera-devel,v8.1,6/8] qcam: CamSelectDialog: Add capture script button

17094 diff mbox series

Implement an Capture Script in CamSelectDialog button which would allow
the user to open a Capture Script (*.yaml).
This button has three states :
    - Open Capture Script
    - Loaded
    - Stop the execution of current capture script

When clicked in an open state, present the user with a QFileDialog to
allow selecting a single file. When the script is loaded the button
displays "Loaded", the script has not been verified yet. Verifying the
script and executing it happens after user presses Ok.

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

Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
---
Difference from v8:
	1. selectedScriptPath_ is now cleared when informScriptReset() is invoked.
Difference from v7:
	1. Fix grammetical errors in the commit message
	2. Intialize the cameraSelectorDialog_ to nullptr in Construct
		so we construct the CameraSelectorDialog just once
 src/qcam/cam_select_dialog.cpp | 70 ++++++++++++++++++++++++++++++++--
 src/qcam/cam_select_dialog.h   | 20 +++++++++-
 src/qcam/main_window.cpp       | 59 +++++++++++++++++++++++++++-
 src/qcam/main_window.h         |  7 ++++
 src/qcam/meson.build           |  2 +
 5 files changed, 153 insertions(+), 5 deletions(-)

Quoting Utkarsh Tiwari via libcamera-devel (2022-08-12 13:15:39)
> Implement an Capture Script in CamSelectDialog button which would allow
> the user to open a Capture Script (*.yaml).
> This button has three states :
>     - Open Capture Script
>     - Loaded
>     - Stop the execution of current capture script
> 
> When clicked in an open state, present the user with a QFileDialog to
> allow selecting a single file. When the script is loaded the button
> displays "Loaded", the script has not been verified yet. Verifying the
> script and executing it happens after user presses Ok.
> 
> 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_.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> ---
> Difference from v8:
>         1. selectedScriptPath_ is now cleared when informScriptReset() is invoked.
> Difference from v7:
>         1. Fix grammetical errors in the commit message
>         2. Intialize the cameraSelectorDialog_ to nullptr in Construct
>                 so we construct the CameraSelectorDialog just once
>  src/qcam/cam_select_dialog.cpp | 70 ++++++++++++++++++++++++++++++++--
>  src/qcam/cam_select_dialog.h   | 20 +++++++++-
>  src/qcam/main_window.cpp       | 59 +++++++++++++++++++++++++++-
>  src/qcam/main_window.h         |  7 ++++
>  src/qcam/meson.build           |  2 +
>  5 files changed, 153 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp
> index f97ad6eb..58cbfc28 100644
> --- a/src/qcam/cam_select_dialog.cpp
> +++ b/src/qcam/cam_select_dialog.cpp
> @@ -16,12 +16,13 @@
>  #include <QComboBox>
>  #include <QDialog>
>  #include <QDialogButtonBox>
> +#include <QFileDialog>
>  #include <QFormLayout>
>  #include <QString>
>  
>  CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> -                                          QWidget *parent)
> -       : QDialog(parent), cm_(cameraManager)
> +                                          bool isScriptRunning, QWidget *parent)
> +       : QDialog(parent), cm_(cameraManager), isScriptRunning_(isScriptRunning)
>  {
>         /* Use a QFormLayout for the dialog. */
>         QFormLayout *layout = new QFormLayout(this);
> @@ -39,6 +40,16 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
>         connect(cameraIdComboBox_, &QComboBox::currentTextChanged,
>                 this, &CameraSelectorDialog::handleCameraChange);
>  
> +       captureScriptButton_ = new QPushButton;
> +       connect(captureScriptButton_, &QPushButton::clicked,
> +               this, &CameraSelectorDialog::handleCaptureScriptButton);
> +
> +       /* Display the action that would be performed when button is clicked. */
> +       if (isScriptRunning_)
> +               captureScriptButton_->setText("Stop");
> +       else
> +               captureScriptButton_->setText("Open");
> +

I feel a bit uneasy at how we're trying to cram so much state
information into this one button.

I don't think I know of any 'standard' application that puts so much
context into the label of a button.

Have you looked for any applications that might have a user interface
feature that is close to what we are doing here?

I can see we're trying to convey:

 - Whether a capture script is already specified and loaded
 - That the user can open select a file
 - That the user can 'deselect' a script.

It might help to try to break some of that out.

I know the camera selection dialog will increase in size, but I expect
that will have to happen when we expose camera configuration options at
startup time too...


>         /* Setup the QDialogButton Box */
>         QDialogButtonBox *buttonBox =
>                 new QDialogButtonBox(QDialogButtonBox::Ok |
> @@ -50,10 +61,10 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
>                 this, &QDialog::reject);
>  
>         /* Set the layout. */
> -
>         layout->addRow("Camera:", cameraIdComboBox_);
>         layout->addRow("Location:", cameraLocation_);
>         layout->addRow("Model:", cameraModel_);
> +       layout->addRow("Capture Script:", captureScriptButton_);
>         layout->addWidget(buttonBox);
>  }
>  
> @@ -62,6 +73,11 @@ std::string CameraSelectorDialog::getCameraId()
>         return cameraIdComboBox_->currentText().toStdString();
>  }
>  
> +std::string CameraSelectorDialog::getCaptureScript()
> +{
> +       return scriptPath_;
> +}

I think this one liner could be inlined in the header. That's what we
normally do for this sort of simple read accessor.

Like cookie() and status() in the Request for instance: 
 - https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/request.h#n61

> +
>  /* Hotplug / Unplug Support. */
>  void CameraSelectorDialog::cameraAdded(libcamera::Camera *camera)
>  {
> @@ -115,3 +131,51 @@ void CameraSelectorDialog::updateCamInfo(const std::shared_ptr<libcamera::Camera
>  
>         cameraModel_->setText(QString::fromStdString(model));
>  }
> +
> +/* Capture script support. */
> +void CameraSelectorDialog::handleCaptureScriptButton()
> +{
> +       if (isScriptRunning_) {
> +               Q_EMIT stopCaptureScript();

Is this really emitting the signal ? It's just calling the function
directly isn't it ?


> +               isScriptRunning_ = false;
> +               captureScriptButton_->setText("Open");

How do applications normally convey that files can be opened or closed?

> +       } else {
> +               selectedScriptPath_ = QFileDialog::getOpenFileName(this,
> +                                                                  "Run Capture Script", QDir::currentPath(),
> +                                                                  "Capture Script (*.yaml)")
> +                                             .toStdString();
> +
> +               if (!selectedScriptPath_.empty())
> +                       captureScriptButton_->setText("Loaded");
> +               else
> +                       captureScriptButton_->setText("Open");
> +       }
> +}
> +
> +void CameraSelectorDialog::accept()
> +{
> +       scriptPath_ = selectedScriptPath_;
> +       QDialog::accept();
> +}
> +
> +void CameraSelectorDialog::reject()
> +{
> +       if (isScriptRunning_)
> +               selectedScriptPath_ = scriptPath_;
> +       QDialog::reject();
> +}
> +
> +void CameraSelectorDialog::informScriptReset()
> +{
> +       isScriptRunning_ = false;
> +       scriptPath_.clear();
> +       selectedScriptPath_.clear();
> +       captureScriptButton_->setText("Open");
> +}
> +
> +void CameraSelectorDialog::informScriptRunning(std::string scriptPath)
> +{
> +       isScriptRunning_ = true;
> +       scriptPath_ = scriptPath;
> +       captureScriptButton_->setText("Stop");
> +}
> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> index 16475af6..bbdf897e 100644
> --- a/src/qcam/cam_select_dialog.h
> +++ b/src/qcam/cam_select_dialog.h
> @@ -17,18 +17,21 @@
>  #include <QComboBox>
>  #include <QDialog>
>  #include <QLabel>
> +#include <QPushButton>
>  
>  class CameraSelectorDialog : public QDialog
>  {
>         Q_OBJECT
>  public:
>         CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> -                            QWidget *parent);
> +                            bool isScriptRunning, QWidget *parent);
>  
>         ~CameraSelectorDialog() = default;
>  
>         std::string getCameraId();
>  
> +       std::string getCaptureScript();
> +
>         /* Hotplug / Unplug Support. */
>         void cameraAdded(libcamera::Camera *camera);
>  
> @@ -38,11 +41,26 @@ public:
>         void updateCamInfo(const std::shared_ptr<libcamera::Camera> &camera);
>         void handleCameraChange();
>  
> +       /* Capture script support. */
> +       void handleCaptureScriptButton();
> +       void informScriptReset();
> +       void informScriptRunning(std::string scriptPath);
> +       void accept() override;
> +       void reject() override;
> +
> +Q_SIGNALS:
> +       void stopCaptureScript();
> +
>  private:
>         libcamera::CameraManager *cm_;
>  
> +       bool isScriptRunning_;
> +       std::string scriptPath_;
> +       std::string selectedScriptPath_;
> +
>         /* UI elements. */
>         QComboBox *cameraIdComboBox_;
>         QLabel *cameraLocation_;
>         QLabel *cameraModel_;
> +       QPushButton *captureScriptButton_;
>  };
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index bf40572a..3c7c3173 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>
> @@ -19,6 +20,7 @@
>  #include <QFileDialog>
>  #include <QImage>
>  #include <QImageWriter>
> +#include <QMessageBox>
>  #include <QMutexLocker>
>  #include <QStandardPaths>
>  #include <QStringList>
> @@ -152,6 +154,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>                 return;
>         }
>  
> +       /* Start capture script. */
> +       loadCaptureScript();
> +
>         startStopAction_->setChecked(true);
>  }
>  
> @@ -290,11 +295,54 @@ void MainWindow::switchCamera()
>         startStopAction_->setChecked(true);
>  }
>  
> +void MainWindow::stopCaptureScript()
> +{
> +       if (script_) {
> +               script_.reset();
> +               cameraSelectorDialog_->informScriptReset();

All this 'informing' makes it feel like we're telling the
cameraSelectorDialog_ something about data it owns...



> +       }
> +}
> +
> +void MainWindow::loadCaptureScript()
> +{
> +       if (scriptPath_.empty() || camera_ == nullptr)
> +               return;
> +
> +       script_ = std::make_unique<CaptureScript>(camera_, scriptPath_);
> +
> +       /*
> +        * If we are already capturing, stop so we don't have stuck image
> +        * in viewfinder.
> +        */
> +       bool wasCapturing = isCapturing_;
> +       if (isCapturing_)
> +               toggleCapture(false);
> +
> +       if (!script_->valid()) {
> +               script_.reset();
> +               cameraSelectorDialog_->informScriptReset();
> +
> +               QMessageBox::critical(this, "Invalid Script",
> +                                     "Couldn't load the capture script");
> +
> +       } else
> +               cameraSelectorDialog_->informScriptRunning(scriptPath_);

Having to 'inform' the dialog makes me wonder who's really in control of
the capture script.

I haven't done enough UI design to know if there's better ways to model
this though ... It just feels a bit fragile having to code 'informer's

I haven't got a better solution right now though ...

> +
> +       /* Start capture again if we were capturing before. */
> +       if (wasCapturing)
> +               toggleCapture(true);
> +}
> +
>  std::string MainWindow::chooseCamera()
>  {
> +       bool scriptRunning = script_ != nullptr;
> +
>         /* Construct the selection dialog, only the first time. */
>         if (!cameraSelectorDialog_)
> -               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);
> +               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);
> +
> +       connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
> +               this, &MainWindow::stopCaptureScript);
>  
>         /*
>          * Use the camera specified on the command line, if any, or display the
> @@ -309,6 +357,9 @@ std::string MainWindow::chooseCamera()
>                 std::string cameraId = cameraSelectorDialog_->getCameraId();
>                 cameraSelectButton_->setText(QString::fromStdString(cameraId));
>  
> +               scriptPath_ = cameraSelectorDialog_->getCaptureScript();
> +               loadCaptureScript();
> +
>                 return cameraId;
>         } else
>                 return std::string();
> @@ -506,6 +557,7 @@ int MainWindow::startCapture()
>         previousFrames_ = 0;
>         framesCaptured_ = 0;
>         lastBufferTime_ = 0;
> +       queueCount_ = 0;
>  
>         ret = camera_->start();
>         if (ret) {
> @@ -783,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 d161365a..10994b67 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 "cam_select_dialog.h"
> @@ -89,6 +90,9 @@ private:
>         void processHotplug(HotplugEvent *e);
>         void processViewfinder(libcamera::FrameBuffer *buffer);
>  
> +       void loadCaptureScript();
> +       void stopCaptureScript();
> +
>         /* UI elements */
>         QToolBar *toolbar_;
>         QAction *startStopAction_;
> @@ -130,6 +134,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.25.1
>

On Mon, Aug 22, 2022 at 11:28:05PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-12 13:15:39)
> > Implement an Capture Script in CamSelectDialog button which would allow
> > the user to open a Capture Script (*.yaml).
> > This button has three states :
> >     - Open Capture Script
> >     - Loaded
> >     - Stop the execution of current capture script
> > 
> > When clicked in an open state, present the user with a QFileDialog to
> > allow selecting a single file. When the script is loaded the button
> > displays "Loaded", the script has not been verified yet. Verifying the
> > script and executing it happens after user presses Ok.
> > 
> > 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_.
> > 
> > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> > ---
> > Difference from v8:
> >         1. selectedScriptPath_ is now cleared when informScriptReset() is invoked.
> > Difference from v7:
> >         1. Fix grammetical errors in the commit message
> >         2. Intialize the cameraSelectorDialog_ to nullptr in Construct
> >                 so we construct the CameraSelectorDialog just once
> >  src/qcam/cam_select_dialog.cpp | 70 ++++++++++++++++++++++++++++++++--
> >  src/qcam/cam_select_dialog.h   | 20 +++++++++-
> >  src/qcam/main_window.cpp       | 59 +++++++++++++++++++++++++++-
> >  src/qcam/main_window.h         |  7 ++++
> >  src/qcam/meson.build           |  2 +
> >  5 files changed, 153 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp
> > index f97ad6eb..58cbfc28 100644
> > --- a/src/qcam/cam_select_dialog.cpp
> > +++ b/src/qcam/cam_select_dialog.cpp
> > @@ -16,12 +16,13 @@
> >  #include <QComboBox>
> >  #include <QDialog>
> >  #include <QDialogButtonBox>
> > +#include <QFileDialog>
> >  #include <QFormLayout>
> >  #include <QString>
> >  
> >  CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> > -                                          QWidget *parent)
> > -       : QDialog(parent), cm_(cameraManager)
> > +                                          bool isScriptRunning, QWidget *parent)
> > +       : QDialog(parent), cm_(cameraManager), isScriptRunning_(isScriptRunning)
> >  {
> >         /* Use a QFormLayout for the dialog. */
> >         QFormLayout *layout = new QFormLayout(this);
> > @@ -39,6 +40,16 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
> >         connect(cameraIdComboBox_, &QComboBox::currentTextChanged,
> >                 this, &CameraSelectorDialog::handleCameraChange);
> >  
> > +       captureScriptButton_ = new QPushButton;
> > +       connect(captureScriptButton_, &QPushButton::clicked,
> > +               this, &CameraSelectorDialog::handleCaptureScriptButton);

Rename the function to selectCaptureScript.

> > +
> > +       /* Display the action that would be performed when button is clicked. */
> > +       if (isScriptRunning_)
> > +               captureScriptButton_->setText("Stop");
> > +       else
> > +               captureScriptButton_->setText("Open");
> > +
> 
> I feel a bit uneasy at how we're trying to cram so much state
> information into this one button.
> 
> I don't think I know of any 'standard' application that puts so much
> context into the label of a button.
> 
> Have you looked for any applications that might have a user interface
> feature that is close to what we are doing here?
> 
> I can see we're trying to convey:
> 
>  - Whether a capture script is already specified and loaded
>  - That the user can open select a file
>  - That the user can 'deselect' a script.
> 
> It might help to try to break some of that out.
> 
> I know the camera selection dialog will increase in size, but I expect
> that will have to happen when we expose camera configuration options at
> startup time too...

One option would be something like this:

    Use Capture Script: [No Script Selected] [X]

The [No Script Selected] is a push button, with "No Script Selected"
being the initial text. When the button is clicked, a file selection
dialog opens, to select a script. If the dialog is accepted, the text of
the button changes to display the file name, without the full path to
save space. You could also add a tooltip for the button to display the
full name including the path.

The [X] is a button too, using the edit-clear icon (and delete.svg as a
fallback). When the button is clicked, the selector button text is set
back to "No Script Selected".

Another option would be

    Use Capture Script: [ ] [Select a Script ...]

[ ] is a check box, and [Select a Script ...] a push button. The push
button would be disabled to start with, and enabled when the check box
is checked. The behaviour of the push button would then be the same as
in the previous option.

I have a preference for the first option I think, as the second option
has three states:

- Check box disabled
- Check box enabled with no script selected
- Check box enabled with script selected

The second state would be implemented as not using a capture script, but
from a UI point of view I think it would be more confusing than the
first option.

> >         /* Setup the QDialogButton Box */
> >         QDialogButtonBox *buttonBox =
> >                 new QDialogButtonBox(QDialogButtonBox::Ok |
> > @@ -50,10 +61,10 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag
> >                 this, &QDialog::reject);
> >  
> >         /* Set the layout. */
> > -
> >         layout->addRow("Camera:", cameraIdComboBox_);
> >         layout->addRow("Location:", cameraLocation_);
> >         layout->addRow("Model:", cameraModel_);
> > +       layout->addRow("Capture Script:", captureScriptButton_);
> >         layout->addWidget(buttonBox);
> >  }
> >  
> > @@ -62,6 +73,11 @@ std::string CameraSelectorDialog::getCameraId()
> >         return cameraIdComboBox_->currentText().toStdString();
> >  }
> >  
> > +std::string CameraSelectorDialog::getCaptureScript()
> > +{
> > +       return scriptPath_;
> > +}
> 
> I think this one liner could be inlined in the header. That's what we
> normally do for this sort of simple read accessor.
> 
> Like cookie() and status() in the Request for instance: 
>  - https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/request.h#n61
> 
> > +
> >  /* Hotplug / Unplug Support. */
> >  void CameraSelectorDialog::cameraAdded(libcamera::Camera *camera)
> >  {
> > @@ -115,3 +131,51 @@ void CameraSelectorDialog::updateCamInfo(const std::shared_ptr<libcamera::Camera
> >  
> >         cameraModel_->setText(QString::fromStdString(model));
> >  }
> > +
> > +/* Capture script support. */
> > +void CameraSelectorDialog::handleCaptureScriptButton()
> > +{
> > +       if (isScriptRunning_) {
> > +               Q_EMIT stopCaptureScript();
> 
> Is this really emitting the signal ? It's just calling the function
> directly isn't it ?

That's how Qt handles signals, the meta object compiler generates a
function, and emitting a signal is just a matter of calling that
function. Normally one would write

		emit stopCaptureScript();

but as we compile qcam with -DQT_NO_KEYWORDS we have to use macros that
don't clash with the signal/slot names.

> > +               isScriptRunning_ = false;
> > +               captureScriptButton_->setText("Open");
> 
> How do applications normally convey that files can be opened or closed?
> 
> > +       } else {
> > +               selectedScriptPath_ = QFileDialog::getOpenFileName(this,
> > +                                                                  "Run Capture Script", QDir::currentPath(),
> > +                                                                  "Capture Script (*.yaml)")
> > +                                             .toStdString();
> > +
> > +               if (!selectedScriptPath_.empty())
> > +                       captureScriptButton_->setText("Loaded");
> > +               else
> > +                       captureScriptButton_->setText("Open");
> > +       }
> > +}
> > +
> > +void CameraSelectorDialog::accept()
> > +{
> > +       scriptPath_ = selectedScriptPath_;
> > +       QDialog::accept();
> > +}
> > +
> > +void CameraSelectorDialog::reject()
> > +{
> > +       if (isScriptRunning_)
> > +               selectedScriptPath_ = scriptPath_;
> > +       QDialog::reject();
> > +}
> > +
> > +void CameraSelectorDialog::informScriptReset()
> > +{
> > +       isScriptRunning_ = false;
> > +       scriptPath_.clear();
> > +       selectedScriptPath_.clear();
> > +       captureScriptButton_->setText("Open");
> > +}
> > +
> > +void CameraSelectorDialog::informScriptRunning(std::string scriptPath)
> > +{
> > +       isScriptRunning_ = true;
> > +       scriptPath_ = scriptPath;
> > +       captureScriptButton_->setText("Stop");
> > +}
> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> > index 16475af6..bbdf897e 100644
> > --- a/src/qcam/cam_select_dialog.h
> > +++ b/src/qcam/cam_select_dialog.h
> > @@ -17,18 +17,21 @@
> >  #include <QComboBox>
> >  #include <QDialog>
> >  #include <QLabel>
> > +#include <QPushButton>
> >  
> >  class CameraSelectorDialog : public QDialog
> >  {
> >         Q_OBJECT
> >  public:
> >         CameraSelectorDialog(libcamera::CameraManager *cameraManager,
> > -                            QWidget *parent);
> > +                            bool isScriptRunning, QWidget *parent);
> >  
> >         ~CameraSelectorDialog() = default;
> >  
> >         std::string getCameraId();
> >  
> > +       std::string getCaptureScript();
> > +
> >         /* Hotplug / Unplug Support. */
> >         void cameraAdded(libcamera::Camera *camera);
> >  
> > @@ -38,11 +41,26 @@ public:
> >         void updateCamInfo(const std::shared_ptr<libcamera::Camera> &camera);
> >         void handleCameraChange();
> >  
> > +       /* Capture script support. */
> > +       void handleCaptureScriptButton();
> > +       void informScriptReset();
> > +       void informScriptRunning(std::string scriptPath);
> > +       void accept() override;
> > +       void reject() override;
> > +
> > +Q_SIGNALS:
> > +       void stopCaptureScript();
> > +
> >  private:
> >         libcamera::CameraManager *cm_;
> >  
> > +       bool isScriptRunning_;
> > +       std::string scriptPath_;
> > +       std::string selectedScriptPath_;
> > +
> >         /* UI elements. */
> >         QComboBox *cameraIdComboBox_;
> >         QLabel *cameraLocation_;
> >         QLabel *cameraModel_;
> > +       QPushButton *captureScriptButton_;
> >  };
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index bf40572a..3c7c3173 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>
> > @@ -19,6 +20,7 @@
> >  #include <QFileDialog>
> >  #include <QImage>
> >  #include <QImageWriter>
> > +#include <QMessageBox>
> >  #include <QMutexLocker>
> >  #include <QStandardPaths>
> >  #include <QStringList>
> > @@ -152,6 +154,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >                 return;
> >         }
> >  
> > +       /* Start capture script. */
> > +       loadCaptureScript();
> > +
> >         startStopAction_->setChecked(true);
> >  }
> >  
> > @@ -290,11 +295,54 @@ void MainWindow::switchCamera()
> >         startStopAction_->setChecked(true);
> >  }
> >  
> > +void MainWindow::stopCaptureScript()
> > +{
> > +       if (script_) {
> > +               script_.reset();
> > +               cameraSelectorDialog_->informScriptReset();
> 
> All this 'informing' makes it feel like we're telling the
> cameraSelectorDialog_ something about data it owns...
> 
> > +       }
> > +}
> > +
> > +void MainWindow::loadCaptureScript()
> > +{
> > +       if (scriptPath_.empty() || camera_ == nullptr)
> > +               return;
> > +
> > +       script_ = std::make_unique<CaptureScript>(camera_, scriptPath_);
> > +
> > +       /*
> > +        * If we are already capturing, stop so we don't have stuck image
> > +        * in viewfinder.
> > +        */
> > +       bool wasCapturing = isCapturing_;
> > +       if (isCapturing_)
> > +               toggleCapture(false);
> > +
> > +       if (!script_->valid()) {
> > +               script_.reset();
> > +               cameraSelectorDialog_->informScriptReset();
> > +
> > +               QMessageBox::critical(this, "Invalid Script",
> > +                                     "Couldn't load the capture script");
> > +
> > +       } else
> > +               cameraSelectorDialog_->informScriptRunning(scriptPath_);
> 
> Having to 'inform' the dialog makes me wonder who's really in control of
> the capture script.
> 
> I haven't done enough UI design to know if there's better ways to model
> this though ... It just feels a bit fragile having to code 'informer's
> 
> I haven't got a better solution right now though ...

I don't like it either. The open dialog should select the capture
script, and that should be it. The MainWindow class should be
responsible for the rest. If the user clicks the camera selection push
button, the open dialox box will be displayed, and a different or no
capture script can be selected without changing the camera. The
MainWindow should react accordingly.

We may want to add the ability to interact with capture scripts while
the camera is running (such as selecting a different script, or pausing,
resuming, or restarting the current script from the beginning) without
going through the open dialog box, but I'd leave that out for now.

> > +
> > +       /* Start capture again if we were capturing before. */
> > +       if (wasCapturing)
> > +               toggleCapture(true);
> > +}
> > +
> >  std::string MainWindow::chooseCamera()
> >  {
> > +       bool scriptRunning = script_ != nullptr;
> > +
> >         /* Construct the selection dialog, only the first time. */
> >         if (!cameraSelectorDialog_)
> > -               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);
> > +               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);
> > +
> > +       connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
> > +               this, &MainWindow::stopCaptureScript);
> >  
> >         /*
> >          * Use the camera specified on the command line, if any, or display the
> > @@ -309,6 +357,9 @@ std::string MainWindow::chooseCamera()
> >                 std::string cameraId = cameraSelectorDialog_->getCameraId();
> >                 cameraSelectButton_->setText(QString::fromStdString(cameraId));
> >  
> > +               scriptPath_ = cameraSelectorDialog_->getCaptureScript();
> > +               loadCaptureScript();

chooseCamera() should only deal with camera selection, loading the
script should move to MainWindow::switchCamera(). This means that
MainWindow::chooseCamera() will need to return both the camera name and
the script name.

> > +
> >                 return cameraId;
> >         } else
> >                 return std::string();
> > @@ -506,6 +557,7 @@ int MainWindow::startCapture()
> >         previousFrames_ = 0;
> >         framesCaptured_ = 0;
> >         lastBufferTime_ = 0;
> > +       queueCount_ = 0;
> >  
> >         ret = camera_->start();
> >         if (ret) {
> > @@ -783,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 d161365a..10994b67 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 "cam_select_dialog.h"
> > @@ -89,6 +90,9 @@ private:
> >         void processHotplug(HotplugEvent *e);
> >         void processViewfinder(libcamera::FrameBuffer *buffer);
> >  
> > +       void loadCaptureScript();
> > +       void stopCaptureScript();
> > +
> >         /* UI elements */
> >         QToolBar *toolbar_;
> >         QAction *startStopAction_;
> > @@ -130,6 +134,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,
> >  ]
> >