[libcamera-devel] [PATCH v8.1 6/8] qcam: CamSelectDialog: Add capture script button
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Aug 23 00:28:05 CEST 2022
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 at 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
>
More information about the libcamera-devel
mailing list