[{"id":24732,"web_url":"https://patchwork.libcamera.org/comment/24732/","msgid":"<166120728523.3484129.5835280252799197205@Monstersaurus>","date":"2022-08-22T22:28:05","subject":"Re: [libcamera-devel] [PATCH v8.1 6/8] qcam: CamSelectDialog: Add\n\tcapture script button","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Utkarsh Tiwari via libcamera-devel (2022-08-12 13:15:39)\n> Implement an Capture Script in CamSelectDialog button which would allow\n> the user to open a Capture Script (*.yaml).\n> This button has three states :\n>     - Open Capture Script\n>     - Loaded\n>     - Stop the execution of current capture script\n> \n> When clicked in an open state, present the user with a QFileDialog to\n> allow selecting a single file. When the script is loaded the button\n> displays \"Loaded\", the script has not been verified yet. Verifying the\n> script and executing it happens after user presses Ok.\n> \n> Introduce a queueCount_ to keep track of the requests queued.\n> \n> When stopping the execution of the capture script the queueCount_ is not\n> reset and the capture continues as it is (i.e it is not stopped or\n> restarted).\n> \n> Requests are queued with any controls the script matching the current\n> queueCount_.\n> \n> Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>\n> ---\n> Difference from v8:\n>         1. selectedScriptPath_ is now cleared when informScriptReset() is invoked.\n> Difference from v7:\n>         1. Fix grammetical errors in the commit message\n>         2. Intialize the cameraSelectorDialog_ to nullptr in Construct\n>                 so we construct the CameraSelectorDialog just once\n>  src/qcam/cam_select_dialog.cpp | 70 ++++++++++++++++++++++++++++++++--\n>  src/qcam/cam_select_dialog.h   | 20 +++++++++-\n>  src/qcam/main_window.cpp       | 59 +++++++++++++++++++++++++++-\n>  src/qcam/main_window.h         |  7 ++++\n>  src/qcam/meson.build           |  2 +\n>  5 files changed, 153 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> index f97ad6eb..58cbfc28 100644\n> --- a/src/qcam/cam_select_dialog.cpp\n> +++ b/src/qcam/cam_select_dialog.cpp\n> @@ -16,12 +16,13 @@\n>  #include <QComboBox>\n>  #include <QDialog>\n>  #include <QDialogButtonBox>\n> +#include <QFileDialog>\n>  #include <QFormLayout>\n>  #include <QString>\n>  \n>  CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> -                                          QWidget *parent)\n> -       : QDialog(parent), cm_(cameraManager)\n> +                                          bool isScriptRunning, QWidget *parent)\n> +       : QDialog(parent), cm_(cameraManager), isScriptRunning_(isScriptRunning)\n>  {\n>         /* Use a QFormLayout for the dialog. */\n>         QFormLayout *layout = new QFormLayout(this);\n> @@ -39,6 +40,16 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n>         connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n>                 this, &CameraSelectorDialog::handleCameraChange);\n>  \n> +       captureScriptButton_ = new QPushButton;\n> +       connect(captureScriptButton_, &QPushButton::clicked,\n> +               this, &CameraSelectorDialog::handleCaptureScriptButton);\n> +\n> +       /* Display the action that would be performed when button is clicked. */\n> +       if (isScriptRunning_)\n> +               captureScriptButton_->setText(\"Stop\");\n> +       else\n> +               captureScriptButton_->setText(\"Open\");\n> +\n\nI feel a bit uneasy at how we're trying to cram so much state\ninformation into this one button.\n\nI don't think I know of any 'standard' application that puts so much\ncontext into the label of a button.\n\nHave you looked for any applications that might have a user interface\nfeature that is close to what we are doing here?\n\nI can see we're trying to convey:\n\n - Whether a capture script is already specified and loaded\n - That the user can open select a file\n - That the user can 'deselect' a script.\n\nIt might help to try to break some of that out.\n\nI know the camera selection dialog will increase in size, but I expect\nthat will have to happen when we expose camera configuration options at\nstartup time too...\n\n\n>         /* Setup the QDialogButton Box */\n>         QDialogButtonBox *buttonBox =\n>                 new QDialogButtonBox(QDialogButtonBox::Ok |\n> @@ -50,10 +61,10 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n>                 this, &QDialog::reject);\n>  \n>         /* Set the layout. */\n> -\n>         layout->addRow(\"Camera:\", cameraIdComboBox_);\n>         layout->addRow(\"Location:\", cameraLocation_);\n>         layout->addRow(\"Model:\", cameraModel_);\n> +       layout->addRow(\"Capture Script:\", captureScriptButton_);\n>         layout->addWidget(buttonBox);\n>  }\n>  \n> @@ -62,6 +73,11 @@ std::string CameraSelectorDialog::getCameraId()\n>         return cameraIdComboBox_->currentText().toStdString();\n>  }\n>  \n> +std::string CameraSelectorDialog::getCaptureScript()\n> +{\n> +       return scriptPath_;\n> +}\n\nI think this one liner could be inlined in the header. That's what we\nnormally do for this sort of simple read accessor.\n\nLike cookie() and status() in the Request for instance: \n - https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/request.h#n61\n\n> +\n>  /* Hotplug / Unplug Support. */\n>  void CameraSelectorDialog::cameraAdded(libcamera::Camera *camera)\n>  {\n> @@ -115,3 +131,51 @@ void CameraSelectorDialog::updateCamInfo(const std::shared_ptr<libcamera::Camera\n>  \n>         cameraModel_->setText(QString::fromStdString(model));\n>  }\n> +\n> +/* Capture script support. */\n> +void CameraSelectorDialog::handleCaptureScriptButton()\n> +{\n> +       if (isScriptRunning_) {\n> +               Q_EMIT stopCaptureScript();\n\nIs this really emitting the signal ? It's just calling the function\ndirectly isn't it ?\n\n\n> +               isScriptRunning_ = false;\n> +               captureScriptButton_->setText(\"Open\");\n\nHow do applications normally convey that files can be opened or closed?\n\n> +       } else {\n> +               selectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> +                                                                  \"Run Capture Script\", QDir::currentPath(),\n> +                                                                  \"Capture Script (*.yaml)\")\n> +                                             .toStdString();\n> +\n> +               if (!selectedScriptPath_.empty())\n> +                       captureScriptButton_->setText(\"Loaded\");\n> +               else\n> +                       captureScriptButton_->setText(\"Open\");\n> +       }\n> +}\n> +\n> +void CameraSelectorDialog::accept()\n> +{\n> +       scriptPath_ = selectedScriptPath_;\n> +       QDialog::accept();\n> +}\n> +\n> +void CameraSelectorDialog::reject()\n> +{\n> +       if (isScriptRunning_)\n> +               selectedScriptPath_ = scriptPath_;\n> +       QDialog::reject();\n> +}\n> +\n> +void CameraSelectorDialog::informScriptReset()\n> +{\n> +       isScriptRunning_ = false;\n> +       scriptPath_.clear();\n> +       selectedScriptPath_.clear();\n> +       captureScriptButton_->setText(\"Open\");\n> +}\n> +\n> +void CameraSelectorDialog::informScriptRunning(std::string scriptPath)\n> +{\n> +       isScriptRunning_ = true;\n> +       scriptPath_ = scriptPath;\n> +       captureScriptButton_->setText(\"Stop\");\n> +}\n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> index 16475af6..bbdf897e 100644\n> --- a/src/qcam/cam_select_dialog.h\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -17,18 +17,21 @@\n>  #include <QComboBox>\n>  #include <QDialog>\n>  #include <QLabel>\n> +#include <QPushButton>\n>  \n>  class CameraSelectorDialog : public QDialog\n>  {\n>         Q_OBJECT\n>  public:\n>         CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> -                            QWidget *parent);\n> +                            bool isScriptRunning, QWidget *parent);\n>  \n>         ~CameraSelectorDialog() = default;\n>  \n>         std::string getCameraId();\n>  \n> +       std::string getCaptureScript();\n> +\n>         /* Hotplug / Unplug Support. */\n>         void cameraAdded(libcamera::Camera *camera);\n>  \n> @@ -38,11 +41,26 @@ public:\n>         void updateCamInfo(const std::shared_ptr<libcamera::Camera> &camera);\n>         void handleCameraChange();\n>  \n> +       /* Capture script support. */\n> +       void handleCaptureScriptButton();\n> +       void informScriptReset();\n> +       void informScriptRunning(std::string scriptPath);\n> +       void accept() override;\n> +       void reject() override;\n> +\n> +Q_SIGNALS:\n> +       void stopCaptureScript();\n> +\n>  private:\n>         libcamera::CameraManager *cm_;\n>  \n> +       bool isScriptRunning_;\n> +       std::string scriptPath_;\n> +       std::string selectedScriptPath_;\n> +\n>         /* UI elements. */\n>         QComboBox *cameraIdComboBox_;\n>         QLabel *cameraLocation_;\n>         QLabel *cameraModel_;\n> +       QPushButton *captureScriptButton_;\n>  };\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index bf40572a..3c7c3173 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -9,6 +9,7 @@\n>  \n>  #include <assert.h>\n>  #include <iomanip>\n> +#include <memory>\n>  #include <string>\n>  \n>  #include <libcamera/camera_manager.h>\n> @@ -19,6 +20,7 @@\n>  #include <QFileDialog>\n>  #include <QImage>\n>  #include <QImageWriter>\n> +#include <QMessageBox>\n>  #include <QMutexLocker>\n>  #include <QStandardPaths>\n>  #include <QStringList>\n> @@ -152,6 +154,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>                 return;\n>         }\n>  \n> +       /* Start capture script. */\n> +       loadCaptureScript();\n> +\n>         startStopAction_->setChecked(true);\n>  }\n>  \n> @@ -290,11 +295,54 @@ void MainWindow::switchCamera()\n>         startStopAction_->setChecked(true);\n>  }\n>  \n> +void MainWindow::stopCaptureScript()\n> +{\n> +       if (script_) {\n> +               script_.reset();\n> +               cameraSelectorDialog_->informScriptReset();\n\nAll this 'informing' makes it feel like we're telling the\ncameraSelectorDialog_ something about data it owns...\n\n\n\n> +       }\n> +}\n> +\n> +void MainWindow::loadCaptureScript()\n> +{\n> +       if (scriptPath_.empty() || camera_ == nullptr)\n> +               return;\n> +\n> +       script_ = std::make_unique<CaptureScript>(camera_, scriptPath_);\n> +\n> +       /*\n> +        * If we are already capturing, stop so we don't have stuck image\n> +        * in viewfinder.\n> +        */\n> +       bool wasCapturing = isCapturing_;\n> +       if (isCapturing_)\n> +               toggleCapture(false);\n> +\n> +       if (!script_->valid()) {\n> +               script_.reset();\n> +               cameraSelectorDialog_->informScriptReset();\n> +\n> +               QMessageBox::critical(this, \"Invalid Script\",\n> +                                     \"Couldn't load the capture script\");\n> +\n> +       } else\n> +               cameraSelectorDialog_->informScriptRunning(scriptPath_);\n\nHaving to 'inform' the dialog makes me wonder who's really in control of\nthe capture script.\n\nI haven't done enough UI design to know if there's better ways to model\nthis though ... It just feels a bit fragile having to code 'informer's\n\nI haven't got a better solution right now though ...\n\n> +\n> +       /* Start capture again if we were capturing before. */\n> +       if (wasCapturing)\n> +               toggleCapture(true);\n> +}\n> +\n>  std::string MainWindow::chooseCamera()\n>  {\n> +       bool scriptRunning = script_ != nullptr;\n> +\n>         /* Construct the selection dialog, only the first time. */\n>         if (!cameraSelectorDialog_)\n> -               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);\n> +               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);\n> +\n> +       connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> +               this, &MainWindow::stopCaptureScript);\n>  \n>         /*\n>          * Use the camera specified on the command line, if any, or display the\n> @@ -309,6 +357,9 @@ std::string MainWindow::chooseCamera()\n>                 std::string cameraId = cameraSelectorDialog_->getCameraId();\n>                 cameraSelectButton_->setText(QString::fromStdString(cameraId));\n>  \n> +               scriptPath_ = cameraSelectorDialog_->getCaptureScript();\n> +               loadCaptureScript();\n> +\n>                 return cameraId;\n>         } else\n>                 return std::string();\n> @@ -506,6 +557,7 @@ int MainWindow::startCapture()\n>         previousFrames_ = 0;\n>         framesCaptured_ = 0;\n>         lastBufferTime_ = 0;\n> +       queueCount_ = 0;\n>  \n>         ret = camera_->start();\n>         if (ret) {\n> @@ -783,5 +835,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer)\n>  \n>  int MainWindow::queueRequest(Request *request)\n>  {\n> +       if (script_)\n> +               request->controls() = script_->frameControls(queueCount_);\n> +\n> +       queueCount_++;\n> +\n>         return camera_->queueRequest(request);\n>  }\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index d161365a..10994b67 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -27,6 +27,7 @@\n>  #include <QQueue>\n>  #include <QTimer>\n>  \n> +#include \"../cam/capture_script.h\"\n>  #include \"../cam/stream_options.h\"\n>  \n>  #include \"cam_select_dialog.h\"\n> @@ -89,6 +90,9 @@ private:\n>         void processHotplug(HotplugEvent *e);\n>         void processViewfinder(libcamera::FrameBuffer *buffer);\n>  \n> +       void loadCaptureScript();\n> +       void stopCaptureScript();\n> +\n>         /* UI elements */\n>         QToolBar *toolbar_;\n>         QAction *startStopAction_;\n> @@ -130,6 +134,9 @@ private:\n>         QElapsedTimer frameRateInterval_;\n>         uint32_t previousFrames_;\n>         uint32_t framesCaptured_;\n> +       uint32_t queueCount_;\n>  \n>         std::vector<std::unique_ptr<libcamera::Request>> requests_;\n> +       std::unique_ptr<CaptureScript> script_;\n> +       std::string scriptPath_;\n>  };\n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index 61861ea6..70a18d7e 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -15,6 +15,7 @@ endif\n>  qcam_enabled = true\n>  \n>  qcam_sources = files([\n> +    '../cam/capture_script.cpp',\n>      '../cam/image.cpp',\n>      '../cam/options.cpp',\n>      '../cam/stream_options.cpp',\n> @@ -39,6 +40,7 @@ qcam_resources = files([\n>  qcam_deps = [\n>      libatomic,\n>      libcamera_public,\n> +    libyaml,\n>      qt5_dep,\n>  ]\n>  \n> -- \n> 2.25.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EDF44BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 22 Aug 2022 22:28:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5247161FC0;\n\tTue, 23 Aug 2022 00:28:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9AE4361FA3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 23 Aug 2022 00:28:08 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2DF3E2B3;\n\tTue, 23 Aug 2022 00:28:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661207290;\n\tbh=K+raVweHMKlyw708j9+qG+1S5+w8YOVXyIGJuUJGSoU=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=mjsuiZxeJIz66ZJZTzvidQYetGqUWOws12Jugj0+v7Dzixf8nit44ByD4dw5bi8nZ\n\tzGgurV1FfHFONL/WQiDtCpd/8hChhD48k9SezMdpXiZzHGAYHMSO2t3dkL3WQ4XB9G\n\tvIS4w4kZFbXgUScEvw1T+YSg4hhEnW4WMzE9jnaPxTyBeQvFjxdpTK5ZQGIp8gtyU0\n\tcOYD/U0LyB3YY51pH3WwdX517vPS7QyTC6noscPbhh8nDn34OK10dlYtfe+B5P5GUv\n\totwFm6R7nSkxCqKdHi6wFYp4RIFL8S1MdcAqgETuySjM54/QuxBxQSO42ns+a90NsK\n\tgGKosvGeQopbg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661207288;\n\tbh=K+raVweHMKlyw708j9+qG+1S5+w8YOVXyIGJuUJGSoU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=CdrWsUkTbUdmlVshv8PVhvzFPuJS3PDWCYgacBrUlXxitAo3Z42iajr2bRd+Giuay\n\tWEaaBsZDtDv8+3KByw0IXCUU/7eS4wRFZhXZHW/ZqUj0rWnsg60JIBByLzum9MbVYj\n\tQQ/u+empBhzR7nNOb5+l6kCN7kLNq4lu54Wcu7LU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CdrWsUkT\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220812121539.21441-1-utkarsh02t@gmail.com>","References":"<20220810150349.414043-7-utkarsh02t@gmail.com>\n\t<20220812121539.21441-1-utkarsh02t@gmail.com>","To":"Utkarsh Tiwari <utkarsh02t@gmail.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 22 Aug 2022 23:28:05 +0100","Message-ID":"<166120728523.3484129.5835280252799197205@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v8.1 6/8] qcam: CamSelectDialog: Add\n\tcapture script button","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":24742,"web_url":"https://patchwork.libcamera.org/comment/24742/","msgid":"<YwQ9utRyia/kr9Me@pendragon.ideasonboard.com>","date":"2022-08-23T02:38:50","subject":"Re: [libcamera-devel] [PATCH v8.1 6/8] qcam: CamSelectDialog: Add\n\tcapture script button","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Aug 22, 2022 at 11:28:05PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-12 13:15:39)\n> > Implement an Capture Script in CamSelectDialog button which would allow\n> > the user to open a Capture Script (*.yaml).\n> > This button has three states :\n> >     - Open Capture Script\n> >     - Loaded\n> >     - Stop the execution of current capture script\n> > \n> > When clicked in an open state, present the user with a QFileDialog to\n> > allow selecting a single file. When the script is loaded the button\n> > displays \"Loaded\", the script has not been verified yet. Verifying the\n> > script and executing it happens after user presses Ok.\n> > \n> > Introduce a queueCount_ to keep track of the requests queued.\n> > \n> > When stopping the execution of the capture script the queueCount_ is not\n> > reset and the capture continues as it is (i.e it is not stopped or\n> > restarted).\n> > \n> > Requests are queued with any controls the script matching the current\n> > queueCount_.\n> > \n> > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>\n> > ---\n> > Difference from v8:\n> >         1. selectedScriptPath_ is now cleared when informScriptReset() is invoked.\n> > Difference from v7:\n> >         1. Fix grammetical errors in the commit message\n> >         2. Intialize the cameraSelectorDialog_ to nullptr in Construct\n> >                 so we construct the CameraSelectorDialog just once\n> >  src/qcam/cam_select_dialog.cpp | 70 ++++++++++++++++++++++++++++++++--\n> >  src/qcam/cam_select_dialog.h   | 20 +++++++++-\n> >  src/qcam/main_window.cpp       | 59 +++++++++++++++++++++++++++-\n> >  src/qcam/main_window.h         |  7 ++++\n> >  src/qcam/meson.build           |  2 +\n> >  5 files changed, 153 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> > index f97ad6eb..58cbfc28 100644\n> > --- a/src/qcam/cam_select_dialog.cpp\n> > +++ b/src/qcam/cam_select_dialog.cpp\n> > @@ -16,12 +16,13 @@\n> >  #include <QComboBox>\n> >  #include <QDialog>\n> >  #include <QDialogButtonBox>\n> > +#include <QFileDialog>\n> >  #include <QFormLayout>\n> >  #include <QString>\n> >  \n> >  CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > -                                          QWidget *parent)\n> > -       : QDialog(parent), cm_(cameraManager)\n> > +                                          bool isScriptRunning, QWidget *parent)\n> > +       : QDialog(parent), cm_(cameraManager), isScriptRunning_(isScriptRunning)\n> >  {\n> >         /* Use a QFormLayout for the dialog. */\n> >         QFormLayout *layout = new QFormLayout(this);\n> > @@ -39,6 +40,16 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> >         connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> >                 this, &CameraSelectorDialog::handleCameraChange);\n> >  \n> > +       captureScriptButton_ = new QPushButton;\n> > +       connect(captureScriptButton_, &QPushButton::clicked,\n> > +               this, &CameraSelectorDialog::handleCaptureScriptButton);\n\nRename the function to selectCaptureScript.\n\n> > +\n> > +       /* Display the action that would be performed when button is clicked. */\n> > +       if (isScriptRunning_)\n> > +               captureScriptButton_->setText(\"Stop\");\n> > +       else\n> > +               captureScriptButton_->setText(\"Open\");\n> > +\n> \n> I feel a bit uneasy at how we're trying to cram so much state\n> information into this one button.\n> \n> I don't think I know of any 'standard' application that puts so much\n> context into the label of a button.\n> \n> Have you looked for any applications that might have a user interface\n> feature that is close to what we are doing here?\n> \n> I can see we're trying to convey:\n> \n>  - Whether a capture script is already specified and loaded\n>  - That the user can open select a file\n>  - That the user can 'deselect' a script.\n> \n> It might help to try to break some of that out.\n> \n> I know the camera selection dialog will increase in size, but I expect\n> that will have to happen when we expose camera configuration options at\n> startup time too...\n\nOne option would be something like this:\n\n    Use Capture Script: [No Script Selected] [X]\n\nThe [No Script Selected] is a push button, with \"No Script Selected\"\nbeing the initial text. When the button is clicked, a file selection\ndialog opens, to select a script. If the dialog is accepted, the text of\nthe button changes to display the file name, without the full path to\nsave space. You could also add a tooltip for the button to display the\nfull name including the path.\n\nThe [X] is a button too, using the edit-clear icon (and delete.svg as a\nfallback). When the button is clicked, the selector button text is set\nback to \"No Script Selected\".\n\nAnother option would be\n\n    Use Capture Script: [ ] [Select a Script ...]\n\n[ ] is a check box, and [Select a Script ...] a push button. The push\nbutton would be disabled to start with, and enabled when the check box\nis checked. The behaviour of the push button would then be the same as\nin the previous option.\n\nI have a preference for the first option I think, as the second option\nhas three states:\n\n- Check box disabled\n- Check box enabled with no script selected\n- Check box enabled with script selected\n\nThe second state would be implemented as not using a capture script, but\nfrom a UI point of view I think it would be more confusing than the\nfirst option.\n\n> >         /* Setup the QDialogButton Box */\n> >         QDialogButtonBox *buttonBox =\n> >                 new QDialogButtonBox(QDialogButtonBox::Ok |\n> > @@ -50,10 +61,10 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> >                 this, &QDialog::reject);\n> >  \n> >         /* Set the layout. */\n> > -\n> >         layout->addRow(\"Camera:\", cameraIdComboBox_);\n> >         layout->addRow(\"Location:\", cameraLocation_);\n> >         layout->addRow(\"Model:\", cameraModel_);\n> > +       layout->addRow(\"Capture Script:\", captureScriptButton_);\n> >         layout->addWidget(buttonBox);\n> >  }\n> >  \n> > @@ -62,6 +73,11 @@ std::string CameraSelectorDialog::getCameraId()\n> >         return cameraIdComboBox_->currentText().toStdString();\n> >  }\n> >  \n> > +std::string CameraSelectorDialog::getCaptureScript()\n> > +{\n> > +       return scriptPath_;\n> > +}\n> \n> I think this one liner could be inlined in the header. That's what we\n> normally do for this sort of simple read accessor.\n> \n> Like cookie() and status() in the Request for instance: \n>  - https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/request.h#n61\n> \n> > +\n> >  /* Hotplug / Unplug Support. */\n> >  void CameraSelectorDialog::cameraAdded(libcamera::Camera *camera)\n> >  {\n> > @@ -115,3 +131,51 @@ void CameraSelectorDialog::updateCamInfo(const std::shared_ptr<libcamera::Camera\n> >  \n> >         cameraModel_->setText(QString::fromStdString(model));\n> >  }\n> > +\n> > +/* Capture script support. */\n> > +void CameraSelectorDialog::handleCaptureScriptButton()\n> > +{\n> > +       if (isScriptRunning_) {\n> > +               Q_EMIT stopCaptureScript();\n> \n> Is this really emitting the signal ? It's just calling the function\n> directly isn't it ?\n\nThat's how Qt handles signals, the meta object compiler generates a\nfunction, and emitting a signal is just a matter of calling that\nfunction. Normally one would write\n\n\t\temit stopCaptureScript();\n\nbut as we compile qcam with -DQT_NO_KEYWORDS we have to use macros that\ndon't clash with the signal/slot names.\n\n> > +               isScriptRunning_ = false;\n> > +               captureScriptButton_->setText(\"Open\");\n> \n> How do applications normally convey that files can be opened or closed?\n> \n> > +       } else {\n> > +               selectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> > +                                                                  \"Run Capture Script\", QDir::currentPath(),\n> > +                                                                  \"Capture Script (*.yaml)\")\n> > +                                             .toStdString();\n> > +\n> > +               if (!selectedScriptPath_.empty())\n> > +                       captureScriptButton_->setText(\"Loaded\");\n> > +               else\n> > +                       captureScriptButton_->setText(\"Open\");\n> > +       }\n> > +}\n> > +\n> > +void CameraSelectorDialog::accept()\n> > +{\n> > +       scriptPath_ = selectedScriptPath_;\n> > +       QDialog::accept();\n> > +}\n> > +\n> > +void CameraSelectorDialog::reject()\n> > +{\n> > +       if (isScriptRunning_)\n> > +               selectedScriptPath_ = scriptPath_;\n> > +       QDialog::reject();\n> > +}\n> > +\n> > +void CameraSelectorDialog::informScriptReset()\n> > +{\n> > +       isScriptRunning_ = false;\n> > +       scriptPath_.clear();\n> > +       selectedScriptPath_.clear();\n> > +       captureScriptButton_->setText(\"Open\");\n> > +}\n> > +\n> > +void CameraSelectorDialog::informScriptRunning(std::string scriptPath)\n> > +{\n> > +       isScriptRunning_ = true;\n> > +       scriptPath_ = scriptPath;\n> > +       captureScriptButton_->setText(\"Stop\");\n> > +}\n> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > index 16475af6..bbdf897e 100644\n> > --- a/src/qcam/cam_select_dialog.h\n> > +++ b/src/qcam/cam_select_dialog.h\n> > @@ -17,18 +17,21 @@\n> >  #include <QComboBox>\n> >  #include <QDialog>\n> >  #include <QLabel>\n> > +#include <QPushButton>\n> >  \n> >  class CameraSelectorDialog : public QDialog\n> >  {\n> >         Q_OBJECT\n> >  public:\n> >         CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > -                            QWidget *parent);\n> > +                            bool isScriptRunning, QWidget *parent);\n> >  \n> >         ~CameraSelectorDialog() = default;\n> >  \n> >         std::string getCameraId();\n> >  \n> > +       std::string getCaptureScript();\n> > +\n> >         /* Hotplug / Unplug Support. */\n> >         void cameraAdded(libcamera::Camera *camera);\n> >  \n> > @@ -38,11 +41,26 @@ public:\n> >         void updateCamInfo(const std::shared_ptr<libcamera::Camera> &camera);\n> >         void handleCameraChange();\n> >  \n> > +       /* Capture script support. */\n> > +       void handleCaptureScriptButton();\n> > +       void informScriptReset();\n> > +       void informScriptRunning(std::string scriptPath);\n> > +       void accept() override;\n> > +       void reject() override;\n> > +\n> > +Q_SIGNALS:\n> > +       void stopCaptureScript();\n> > +\n> >  private:\n> >         libcamera::CameraManager *cm_;\n> >  \n> > +       bool isScriptRunning_;\n> > +       std::string scriptPath_;\n> > +       std::string selectedScriptPath_;\n> > +\n> >         /* UI elements. */\n> >         QComboBox *cameraIdComboBox_;\n> >         QLabel *cameraLocation_;\n> >         QLabel *cameraModel_;\n> > +       QPushButton *captureScriptButton_;\n> >  };\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index bf40572a..3c7c3173 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -9,6 +9,7 @@\n> >  \n> >  #include <assert.h>\n> >  #include <iomanip>\n> > +#include <memory>\n> >  #include <string>\n> >  \n> >  #include <libcamera/camera_manager.h>\n> > @@ -19,6 +20,7 @@\n> >  #include <QFileDialog>\n> >  #include <QImage>\n> >  #include <QImageWriter>\n> > +#include <QMessageBox>\n> >  #include <QMutexLocker>\n> >  #include <QStandardPaths>\n> >  #include <QStringList>\n> > @@ -152,6 +154,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> >                 return;\n> >         }\n> >  \n> > +       /* Start capture script. */\n> > +       loadCaptureScript();\n> > +\n> >         startStopAction_->setChecked(true);\n> >  }\n> >  \n> > @@ -290,11 +295,54 @@ void MainWindow::switchCamera()\n> >         startStopAction_->setChecked(true);\n> >  }\n> >  \n> > +void MainWindow::stopCaptureScript()\n> > +{\n> > +       if (script_) {\n> > +               script_.reset();\n> > +               cameraSelectorDialog_->informScriptReset();\n> \n> All this 'informing' makes it feel like we're telling the\n> cameraSelectorDialog_ something about data it owns...\n> \n> > +       }\n> > +}\n> > +\n> > +void MainWindow::loadCaptureScript()\n> > +{\n> > +       if (scriptPath_.empty() || camera_ == nullptr)\n> > +               return;\n> > +\n> > +       script_ = std::make_unique<CaptureScript>(camera_, scriptPath_);\n> > +\n> > +       /*\n> > +        * If we are already capturing, stop so we don't have stuck image\n> > +        * in viewfinder.\n> > +        */\n> > +       bool wasCapturing = isCapturing_;\n> > +       if (isCapturing_)\n> > +               toggleCapture(false);\n> > +\n> > +       if (!script_->valid()) {\n> > +               script_.reset();\n> > +               cameraSelectorDialog_->informScriptReset();\n> > +\n> > +               QMessageBox::critical(this, \"Invalid Script\",\n> > +                                     \"Couldn't load the capture script\");\n> > +\n> > +       } else\n> > +               cameraSelectorDialog_->informScriptRunning(scriptPath_);\n> \n> Having to 'inform' the dialog makes me wonder who's really in control of\n> the capture script.\n> \n> I haven't done enough UI design to know if there's better ways to model\n> this though ... It just feels a bit fragile having to code 'informer's\n> \n> I haven't got a better solution right now though ...\n\nI don't like it either. The open dialog should select the capture\nscript, and that should be it. The MainWindow class should be\nresponsible for the rest. If the user clicks the camera selection push\nbutton, the open dialox box will be displayed, and a different or no\ncapture script can be selected without changing the camera. The\nMainWindow should react accordingly.\n\nWe may want to add the ability to interact with capture scripts while\nthe camera is running (such as selecting a different script, or pausing,\nresuming, or restarting the current script from the beginning) without\ngoing through the open dialog box, but I'd leave that out for now.\n\n> > +\n> > +       /* Start capture again if we were capturing before. */\n> > +       if (wasCapturing)\n> > +               toggleCapture(true);\n> > +}\n> > +\n> >  std::string MainWindow::chooseCamera()\n> >  {\n> > +       bool scriptRunning = script_ != nullptr;\n> > +\n> >         /* Construct the selection dialog, only the first time. */\n> >         if (!cameraSelectorDialog_)\n> > -               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);\n> > +               cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptRunning, this);\n> > +\n> > +       connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> > +               this, &MainWindow::stopCaptureScript);\n> >  \n> >         /*\n> >          * Use the camera specified on the command line, if any, or display the\n> > @@ -309,6 +357,9 @@ std::string MainWindow::chooseCamera()\n> >                 std::string cameraId = cameraSelectorDialog_->getCameraId();\n> >                 cameraSelectButton_->setText(QString::fromStdString(cameraId));\n> >  \n> > +               scriptPath_ = cameraSelectorDialog_->getCaptureScript();\n> > +               loadCaptureScript();\n\nchooseCamera() should only deal with camera selection, loading the\nscript should move to MainWindow::switchCamera(). This means that\nMainWindow::chooseCamera() will need to return both the camera name and\nthe script name.\n\n> > +\n> >                 return cameraId;\n> >         } else\n> >                 return std::string();\n> > @@ -506,6 +557,7 @@ int MainWindow::startCapture()\n> >         previousFrames_ = 0;\n> >         framesCaptured_ = 0;\n> >         lastBufferTime_ = 0;\n> > +       queueCount_ = 0;\n> >  \n> >         ret = camera_->start();\n> >         if (ret) {\n> > @@ -783,5 +835,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer)\n> >  \n> >  int MainWindow::queueRequest(Request *request)\n> >  {\n> > +       if (script_)\n> > +               request->controls() = script_->frameControls(queueCount_);\n> > +\n> > +       queueCount_++;\n> > +\n> >         return camera_->queueRequest(request);\n> >  }\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index d161365a..10994b67 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -27,6 +27,7 @@\n> >  #include <QQueue>\n> >  #include <QTimer>\n> >  \n> > +#include \"../cam/capture_script.h\"\n> >  #include \"../cam/stream_options.h\"\n> >  \n> >  #include \"cam_select_dialog.h\"\n> > @@ -89,6 +90,9 @@ private:\n> >         void processHotplug(HotplugEvent *e);\n> >         void processViewfinder(libcamera::FrameBuffer *buffer);\n> >  \n> > +       void loadCaptureScript();\n> > +       void stopCaptureScript();\n> > +\n> >         /* UI elements */\n> >         QToolBar *toolbar_;\n> >         QAction *startStopAction_;\n> > @@ -130,6 +134,9 @@ private:\n> >         QElapsedTimer frameRateInterval_;\n> >         uint32_t previousFrames_;\n> >         uint32_t framesCaptured_;\n> > +       uint32_t queueCount_;\n> >  \n> >         std::vector<std::unique_ptr<libcamera::Request>> requests_;\n> > +       std::unique_ptr<CaptureScript> script_;\n> > +       std::string scriptPath_;\n> >  };\n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index 61861ea6..70a18d7e 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -15,6 +15,7 @@ endif\n> >  qcam_enabled = true\n> >  \n> >  qcam_sources = files([\n> > +    '../cam/capture_script.cpp',\n> >      '../cam/image.cpp',\n> >      '../cam/options.cpp',\n> >      '../cam/stream_options.cpp',\n> > @@ -39,6 +40,7 @@ qcam_resources = files([\n> >  qcam_deps = [\n> >      libatomic,\n> >      libcamera_public,\n> > +    libyaml,\n> >      qt5_dep,\n> >  ]\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6E10FC0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 23 Aug 2022 02:38:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9895661FC0;\n\tTue, 23 Aug 2022 04:38:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BE8960E25\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 23 Aug 2022 04:38:54 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB6712B3;\n\tTue, 23 Aug 2022 04:38:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661222336;\n\tbh=A2t6jbWPt9YLEu9BxdC3NJcVZUBkrTRM7Vy3PX1eLyM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Z09HwhqfNYxKaqNv/6rX/pYdHF3CUcOpfZEicg41i03HhIO5R3xu9lOc2tCRUVDx+\n\tPBrNrrongWJWx4kehpqbwPty3lWStn6UX1Sw8qoezyf1LBDqSycF0smXJfZtImjWka\n\tGUzy5C1LFs2D15WfbuhdGQjGkO01BMaSuePn2mlJ7V+cMlIsuqalV4z0W6lOYAOdN7\n\tY2rK9ixY8+xlKTtgKFTHYNhXubkCm0zJaIIOg70fK/wP8CcYF9+GGT+kuN6sblHhE+\n\tg0RYz+AP9rsYmM9QXwjltXqcPxvBAmJkIORqhA01achYnTUc49Dx+e3Usz6E4ceBqK\n\tSK4nUA5Eb/cEg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661222334;\n\tbh=A2t6jbWPt9YLEu9BxdC3NJcVZUBkrTRM7Vy3PX1eLyM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mWmkRWPbRFvVrSj4NjLsjk0b40ERvjuhgtgNaONvTZrG7WRr/AEAWJBOS3DRi5RFm\n\tz5LsxTFmVwmV4rcWgiJxuNNRKmt03luujKasTN/Gune2rnrUlxArsaPjxiliTrKdzm\n\t+gg0dfrgiCw88XxQGe8LWWHCzLYL6o+vbWXqnPIo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mWmkRWPb\"; dkim-atps=neutral","Date":"Tue, 23 Aug 2022 05:38:50 +0300","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YwQ9utRyia/kr9Me@pendragon.ideasonboard.com>","References":"<20220810150349.414043-7-utkarsh02t@gmail.com>\n\t<20220812121539.21441-1-utkarsh02t@gmail.com>\n\t<166120728523.3484129.5835280252799197205@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166120728523.3484129.5835280252799197205@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v8.1 6/8] qcam: CamSelectDialog: Add\n\tcapture script button","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]