[{"id":24855,"web_url":"https://patchwork.libcamera.org/comment/24855/","msgid":"<166193853183.15972.5974522827658911264@Monstersaurus>","date":"2022-08-31T09:35:31","subject":"Re: [libcamera-devel] [PATCH v9 6/7] 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-31 06:49:37)\n> Display a QLabel which is readonly, and displays the currently\n> selected capture script. The tooltip of the QLabel displays the file\n> path of the script.\n> \n> Implement a capture script button which is a QToolButton which when\n> clicked opens a QFileDialog this allows to select a capture script\n> (*.yaml) file.\n> \n> Next to the capture scipt button, show a QToolButton which stops the\n> capture script.\n> \n> If an invalid script has been selected show a QMesssageBox::critical and\n> continue with the capture's previous state.\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\"from the script\" ?\n\n> ---\n> Differnce from v8:\n> 1. Now display a QLabel with the fileName and filePath with button\n> on the side.\n> 2. infromScriptReset() informScriptRunning() are removed\n> 3. Local script makes handling of invalid scripts easy.\n> src/qcam/assets/feathericons/feathericons.qrc | 2 +\n> src/qcam/cam_select_dialog.cpp | 88 ++++++++++++++++++-\n> src/qcam/cam_select_dialog.h | 20 ++++-\n> src/qcam/main_window.cpp | 67 +++++++++++++-\n> src/qcam/main_window.h | 7 ++\n> src/qcam/meson.build | 2 +\n> 6 files changed, 181 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> index c5302040..0ea0c2d5 100644\n> --- a/src/qcam/assets/feathericons/feathericons.qrc\n> +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> @@ -3,7 +3,9 @@\n> \n> aperture.svg\n> camera-off.svg\n> + delete.svg\n> play-circle.svg\n> + upload.svg\n\nIndentations are wrong here.\n\n\n> save.svg\n> stop-circle.svg\n> x-circle.svg\n> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> index 6543228a..99405cc1 100644\n> --- a/src/qcam/cam_select_dialog.cpp\n> +++ b/src/qcam/cam_select_dialog.cpp\n> @@ -14,13 +14,18 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> +#include \n> +#include \n> #include \n> #include \n> +#include \n> +#include \n> \n> CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> - QWidget *parent)\n> - : QDialog(parent), cm_(cameraManager)\n> + std::string scriptPath, QWidget *parent)\n> + : QDialog(parent), cm_(cameraManager), scriptPath_(scriptPath)\n> {\n> /* Use a QFormLayout for the dialog. */\n> QFormLayout *layout = new QFormLayout(this);\n> @@ -38,6 +43,41 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> this, &CameraSelectorDialog::updateCamInfo);\n> \n> + /* Set capture script selection / removal button. */\n> + QWidget *captureWidget = new QWidget(this);\n> + QHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);\n> +\n> + scriptFileLine_ = new QLabel;\n> + scriptFileLine_->setFrameStyle(QFrame::Sunken | QFrame::StyledPanel);\n> +\n> + chooseCaptureScriptButton_ = new QToolButton;\n> + chooseCaptureScriptButton_->setIcon(QIcon::fromTheme(\"document-open\",\n> + QIcon(\":upload.svg\")));\n> + chooseCaptureScriptButton_->setStyleSheet(\"border:none\");\n> + connect(chooseCaptureScriptButton_, &QToolButton::clicked,\n> + this, &CameraSelectorDialog::selectCaptureScript);\n> +\n> + QToolButton *stopCaptureScriptButton = new QToolButton;\n> + stopCaptureScriptButton->setIcon(QIcon::fromTheme(\"edit-clear\",\n> + QIcon(\":delete.svg\")));\n> + stopCaptureScriptButton->setStyleSheet(\"border:node;\");\n> + connect(stopCaptureScriptButton, &QToolButton::clicked,\n> + this, &CameraSelectorDialog::resetCaptureScript);\n> +\n> + captureLayout->addWidget(scriptFileLine_);\n> + captureLayout->addWidget(chooseCaptureScriptButton_);\n> + captureLayout->addWidget(stopCaptureScriptButton);\n> + captureLayout->setMargin(0);\n> +\n> + /* Set the file name of the capture script. */\n> + if (scriptPath_.empty()) {\n> + scriptFileLine_->setText(\"No File Selected\");\n> + } else {\n> + scriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> + scriptFileLine_->setText(scriptFileInfo_.fileName());\n> + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> + }\n> +\n> /* Setup the QDialogButton Box */\n> QDialogButtonBox *buttonBox =\n> new QDialogButtonBox(QDialogButtonBox::Ok |\n> @@ -52,6 +92,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> layout->addRow(\"Camera:\", cameraIdComboBox_);\n> layout->addRow(\"Location:\", cameraLocation_);\n> layout->addRow(\"Model:\", cameraModel_);\n> + layout->addRow(\"Capture Script:\", captureWidget);\n> layout->addWidget(buttonBox);\n> }\n> \n> @@ -110,3 +151,46 @@ void CameraSelectorDialog::updateCamInfo(QString cameraId)\n> \n> cameraModel_->setText(QString::fromStdString(model));\n> }\n> +\n> +/* Capture script support. */\n> +void CameraSelectorDialog::selectCaptureScript()\n> +{\n> + selectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> + \"Run Capture Script\", QDir::currentPath(),\n> + \"Capture Script (*.yaml)\")\n> + .toStdString();\n\nWhere does checkstyle recommend to place this? It seems to be 'floating'\ntoo.\n\nIt's tricky, but I'm not sure there is much better options anyway, so\nI'll move on.\n\n> +\n> + if (!selectedScriptPath_.empty()) {\n> + scriptFileInfo_.setFile(QString::fromStdString(selectedScriptPath_));\n> + scriptFileLine_->setText(scriptFileInfo_.fileName());\n> + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> + } else {\n> + selectedScriptPath_ = scriptPath_;\n> + }\n> +}\n> +\n> +void CameraSelectorDialog::resetCaptureScript()\n> +{\n> + Q_EMIT stopCaptureScript();\n> + scriptPath_.clear();\n> + selectedScriptPath_.clear();\n> + scriptFileLine_->setText(\"No File Selected\");\n> +}\n> +\n> +void CameraSelectorDialog::accept()\n> +{\n> + scriptPath_ = selectedScriptPath_;\n> + QDialog::accept();\n> +}\n> +\n> +void CameraSelectorDialog::reject()\n> +{\n> + if (scriptPath_.empty()) {\n> + scriptFileLine_->setText(\"No File Selected\");\n> + } else {\n> + scriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> + scriptFileLine_->setText(scriptFileInfo_.fileName());\n> + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> + }\n> + QDialog::reject();\n> +}\n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> index c91b7ebe..377faebc 100644\n> --- a/src/qcam/cam_select_dialog.h\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -15,21 +15,24 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> class QComboBox;\n> class QLabel;\n> +class QToolButton;\n> \n> class CameraSelectorDialog : public QDialog\n> {\n> Q_OBJECT\n> public:\n> CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> - QWidget *parent);\n> + std::string scriptPath, QWidget *parent);\n> \n> ~CameraSelectorDialog();\n> \n> std::string getCameraId();\n> + std::string getCaptureScript() { return scriptPath_; };\n> \n> /* Hotplug / Unplug Support. */\n> void addCamera(QString cameraId);\n> @@ -38,11 +41,26 @@ public:\n> /* Camera Information */\n> void updateCamInfo(QString cameraId);\n> \n> + /* Capture script support. */\n> + void selectCaptureScript();\n> + void resetCaptureScript();\n> +\n> + void accept() override;\n> + void reject() override;\n> +\n> +Q_SIGNALS:\n> + void stopCaptureScript();\n> +\n> private:\n> libcamera::CameraManager *cm_;\n> \n> + std::string scriptPath_;\n> + std::string selectedScriptPath_;\n> + QFileInfo scriptFileInfo_;\n> /* UI elements. */\n> QComboBox *cameraIdComboBox_;\n> QLabel *cameraLocation_;\n> QLabel *cameraModel_;\n> + QLabel *scriptFileLine_;\n> + QToolButton *chooseCaptureScriptButton_;\n> };\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 2a9ca830..af992b94 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -18,6 +19,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -143,7 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> cm_->cameraAdded.connect(this, &MainWindow::addCamera);\n> cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);\n> \n> - cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);\n> + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);\n> + connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> + this, &MainWindow::stopCaptureScript);\n> \n> /* Open the camera and start capture. */\n> ret = openCamera();\n> @@ -152,6 +156,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> return;\n> }\n> \n> + /* Start capture script. */\n> + if (!scriptPath_.empty())\n> + ret = loadCaptureScript();\n> +\n> startStopAction_->setChecked(true);\n> }\n> \n> @@ -266,8 +274,11 @@ void MainWindow::switchCamera()\n> if (newCameraId.empty())\n> return;\n> \n> - if (camera_ && newCameraId == camera_->id())\n> + if (camera_ && newCameraId == camera_->id()) {\n> + // When user opens camera selection dialog for CaptureScript selection\n\nPerhaps:\n\t\t/*\n\t\t * If the camera has not changed, We still need to\n\t\t * handle any update to the capture script. This will\n\t\t * cause the script to restart if it was already\n\t\t * selected.\n\t\t */\n\n\nOr something like that (and correctly formatted).\n\n\n> + loadCaptureScript();\n> return;\n> + }\n> \n> const std::shared_ptr &cam = cm_->get(newCameraId);\n> \n> @@ -287,17 +298,63 @@ void MainWindow::switchCamera()\n> camera_->release();\n> camera_ = cam;\n> \n> + loadCaptureScript();\n> +\n> startStopAction_->setChecked(true);\n> \n> /* Display the current cameraId in the toolbar .*/\n> cameraSelectButton_->setText(QString::fromStdString(newCameraId));\n> }\n> \n> +void MainWindow::stopCaptureScript()\n> +{\n> + if (script_)\n> + script_.reset();\n> +}\n\nThis needs a blank line.\n\n> +/**\n> + * \\brief Loads and validates the current capture script\n> + *\n\nCan we describe here that the current capture session will be restarted\nif it was active?\n\n> + * returns -EINVAL on failure and 0 on success\n> + */\n> +int MainWindow::loadCaptureScript()\n> +{\n> + if (scriptPath_.empty() || camera_ == nullptr)\n> + return -EINVAL;\n\nPerhaps -ENODEV? But I expect this can't happen. I think it's fine to be\ndefensive here though.\n\n> +\n> + auto script = std::make_unique(camera_, scriptPath_);\n> +\n> + if (!script->valid()) {\n> + script.reset();\n> +\n> + QMessageBox::critical(this, \"Invalid Script\",\n> + \"Couldn't load the capture script\");\n> +\n> + return -EINVAL;\n> + }\n> +\n> + /*\n> + * If we are already capturing, stop so we don't have stuck image\n\n\"have a stuck image\"\n\n> + * in viewfinder.\n\n\"in the viewfinder\"\n\n> + */\n> + bool wasCapturing = isCapturing_;\n> + if (isCapturing_)\n> + toggleCapture(false);\n> +\n> + script_ = std::move(script);\n> +\n> + /* Start capture again if we were capturing before. */\n> + if (wasCapturing)\n> + toggleCapture(true);\n\nNew line here.\n\n\nAll of those comments seem quite minor, so with those resolved:\n\n\nReviewed-by: Kieran Bingham \n\n> + return 0;\n> +}\n> +\n> std::string MainWindow::chooseCamera()\n> {\n> if (cameraSelectorDialog_->exec() != QDialog::Accepted)\n> return std::string();\n> \n> + scriptPath_ = cameraSelectorDialog_->getCaptureScript();\n> +\n> return cameraSelectorDialog_->getCameraId();\n> }\n> \n> @@ -499,6 +556,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> @@ -777,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 22c85247..7c877ae1 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -27,6 +27,7 @@\n> #include \n> #include \n> \n> +#include \"../cam/capture_script.h\"\n> #include \"../cam/stream_options.h\"\n> \n> #include \"viewfinder.h\"\n> @@ -89,6 +90,9 @@ private:\n> void processHotplug(HotplugEvent *e);\n> void processViewfinder(libcamera::FrameBuffer *buffer);\n> \n> + int loadCaptureScript();\n> + void stopCaptureScript();\n> +\n> /* UI elements */\n> QToolBar *toolbar_;\n> QAction *startStopAction_;\n> @@ -129,6 +133,9 @@ private:\n> QElapsedTimer frameRateInterval_;\n> uint32_t previousFrames_;\n> uint32_t framesCaptured_;\n> + uint32_t queueCount_;\n> \n> std::vector> requests_;\n> + std::unique_ptr 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.34.1\n>","headers":{"Return-Path":"","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 0D3B1C0DA4\n\tfor ;\n\tWed, 31 Aug 2022 09:35:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 797A961FBE;\n\tWed, 31 Aug 2022 11:35:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2BDE603E1\n\tfor ;\n\tWed, 31 Aug 2022 11:35:34 +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 EAF59481;\n\tWed, 31 Aug 2022 11:35:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661938536;\n\tbh=JzZ+bFo1eT3uB+lJU053T+ldDnxOsw9TdKb36meUwBc=;\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=aH7tQdJxNiSYBrtr9Yb+YlTSmoz102Z3S0TDfmiGbd3MdWz7uUHJwXAgObCStOjEG\n\t2gJiESTjVknDycrjXnWLM52z+RNQEpbTBXmxk2UZjxtXmktUz666TuFc2PdzeITDpf\n\tW1MeqqWmPJ4CUeyvI/gpkg6W4x97Y0E0guhXFeMtSEUgTTOs3hcq8mBLmQ8be5bAbx\n\toL/Ox3tlqDUYG7I9navnYy4TNZ1uKBKzO8a+qMh8G1rCFuX+oMtJvfOoYLnCF1fKOH\n\tVGuDWyhTZh1WU7yCzdUkUDCFX8K1mSUPtuWXVE3+jt7NQPPc6UAdg084wn5zEFj1G+\n\tDXXqOBVaGQa3w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661938534;\n\tbh=JzZ+bFo1eT3uB+lJU053T+ldDnxOsw9TdKb36meUwBc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=mzqOJNrTnr7jYeWGFGGLLztODD97X/sza/0lJ8ArQpuMkXl3i/hHmCFgZs1D1sFAp\n\tHbocp713p0TERTG2YK36qPTG98BFRsTK03flIsxYYy2hmMz626aFBsLQQ04MgPpJVN\n\t57+MsR2HXZYNQxgy6C/jA6NpecjOUexxiCOHa5c0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mzqOJNrT\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220831054938.21617-7-utkarsh02t@gmail.com>","References":"<20220831054938.21617-1-utkarsh02t@gmail.com>\n\t<20220831054938.21617-7-utkarsh02t@gmail.com>","To":"Utkarsh Tiwari ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 31 Aug 2022 10:35:31 +0100","Message-ID":"<166193853183.15972.5974522827658911264@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add\n\tcapture script button","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24857,"web_url":"https://patchwork.libcamera.org/comment/24857/","msgid":"","date":"2022-08-31T09:59:12","subject":"Re: [libcamera-devel] [PATCH v9 6/7] 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":"Hi Utkarsh,\n\nThank you for the patch.\n\nOn Wed, Aug 31, 2022 at 11:19:37AM +0530, Utkarsh Tiwari via libcamera-devel wrote:\n> Display a QLabel which is readonly, and displays the currently\n\nLabels are always read-only :-)\n\n> selected capture script. The tooltip of the QLabel displays the file\n> path of the script.\n> \n> Implement a capture script button which is a QToolButton which when\n> clicked opens a QFileDialog this allows to select a capture script\n> (*.yaml) file.\n> \n> Next to the capture scipt button, show a QToolButton which stops the\n> capture script.\n> \n> If an invalid script has been selected show a QMesssageBox::critical and\n> continue with the capture's previous state.\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\nMissing SoB line.\n\n> ---\n> Differnce from v8:\n> 1. Now display a QLabel with the fileName and filePath with button\n> on the side.\n> 2. infromScriptReset() informScriptRunning() are removed\n> 3. Local script makes handling of invalid scripts easy.\n> src/qcam/assets/feathericons/feathericons.qrc | 2 +\n> src/qcam/cam_select_dialog.cpp | 88 ++++++++++++++++++-\n> src/qcam/cam_select_dialog.h | 20 ++++-\n> src/qcam/main_window.cpp | 67 +++++++++++++-\n> src/qcam/main_window.h | 7 ++\n> src/qcam/meson.build | 2 +\n> 6 files changed, 181 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> index c5302040..0ea0c2d5 100644\n> --- a/src/qcam/assets/feathericons/feathericons.qrc\n> +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> @@ -3,7 +3,9 @@\n> \n> \taperture.svg\n> \tcamera-off.svg\n> + delete.svg\n\nPlease indent with tabs, not spaces.\n\n> \tplay-circle.svg\n> + upload.svg\n\nSame here.\n\n> \tsave.svg\n> \tstop-circle.svg\n> \tx-circle.svg\n> diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> index 6543228a..99405cc1 100644\n> --- a/src/qcam/cam_select_dialog.cpp\n> +++ b/src/qcam/cam_select_dialog.cpp\n> @@ -14,13 +14,18 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> +#include \n> +#include \n> #include \n> #include \n> +#include \n\nWhy not a QPushButton ? QToolButton is usually meant for toolbars.\n\n> +#include \n> \n> CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> -\t\t\t\t\t QWidget *parent)\n> -\t: QDialog(parent), cm_(cameraManager)\n> +\t\t\t\t\t std::string scriptPath, QWidget *parent)\n\nMake it a const std::string & to avoid copies.\n\n> +\t: QDialog(parent), cm_(cameraManager), scriptPath_(scriptPath)\n> {\n> \t/* Use a QFormLayout for the dialog. */\n> \tQFormLayout *layout = new QFormLayout(this);\n> @@ -38,6 +43,41 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> \tconnect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> \t\tthis, &CameraSelectorDialog::updateCamInfo);\n> \n> +\t/* Set capture script selection / removal button. */\n> +\tQWidget *captureWidget = new QWidget(this);\n\nMaybe captureScriptWidhet ?\n\n> +\tQHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);\n\nDitto.\n\n> +\n> +\tscriptFileLine_ = new QLabel;\n> +\tscriptFileLine_->setFrameStyle(QFrame::Sunken | QFrame::StyledPanel);\n\nGenerally speaking, changing styles manually should be avoided as much\nas possible. It makes the overall style inconsistent and is confusing\nfor users. I'd drop this.\n\n> +\n> +\tchooseCaptureScriptButton_ = new QToolButton;\n> +\tchooseCaptureScriptButton_->setIcon(QIcon::fromTheme(\"document-open\",\n> +\t\t\t\t\t\t\t QIcon(\":upload.svg\")));\n> +\tchooseCaptureScriptButton_->setStyleSheet(\"border:none\");\n\nSame here, drop this. A button that doesn't look like a button gives no\nvisual clue that it can be clicked.\n\n> +\tconnect(chooseCaptureScriptButton_, &QToolButton::clicked,\n> +\t\tthis, &CameraSelectorDialog::selectCaptureScript);\n> +\n> +\tQToolButton *stopCaptureScriptButton = new QToolButton;\n> +\tstopCaptureScriptButton->setIcon(QIcon::fromTheme(\"edit-clear\",\n> +\t\t\t\t\t\t\t QIcon(\":delete.svg\")));\n> +\tstopCaptureScriptButton->setStyleSheet(\"border:node;\");\n> +\tconnect(stopCaptureScriptButton, &QToolButton::clicked,\n> +\t\tthis, &CameraSelectorDialog::resetCaptureScript);\n> +\n> +\tcaptureLayout->addWidget(scriptFileLine_);\n> +\tcaptureLayout->addWidget(chooseCaptureScriptButton_);\n> +\tcaptureLayout->addWidget(stopCaptureScriptButton);\n> +\tcaptureLayout->setMargin(0);\n> +\n> +\t/* Set the file name of the capture script. */\n> +\tif (scriptPath_.empty()) {\n> +\t\tscriptFileLine_->setText(\"No File Selected\");\n> +\t} else {\n> +\t\tscriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> +\t}\n> +\n> \t/* Setup the QDialogButton Box */\n> \tQDialogButtonBox *buttonBox =\n> \t\tnew QDialogButtonBox(QDialogButtonBox::Ok |\n> @@ -52,6 +92,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> \tlayout->addRow(\"Camera:\", cameraIdComboBox_);\n> \tlayout->addRow(\"Location:\", cameraLocation_);\n> \tlayout->addRow(\"Model:\", cameraModel_);\n> +\tlayout->addRow(\"Capture Script:\", captureWidget);\n> \tlayout->addWidget(buttonBox);\n> }\n> \n> @@ -110,3 +151,46 @@ void CameraSelectorDialog::updateCamInfo(QString cameraId)\n> \n> \tcameraModel_->setText(QString::fromStdString(model));\n> }\n> +\n> +/* Capture script support. */\n> +void CameraSelectorDialog::selectCaptureScript()\n> +{\n> +\tselectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> +\t\t\t\t\t\t\t \"Run Capture Script\", QDir::currentPath(),\n> +\t\t\t\t\t\t\t \"Capture Script (*.yaml)\")\n> +\t\t\t\t .toStdString();\n> +\n> +\tif (!selectedScriptPath_.empty()) {\n> +\t\tscriptFileInfo_.setFile(QString::fromStdString(selectedScriptPath_));\n\nYou can make this a local variable:\n\n\t\tQFileInfo fileInfo(QString::fromStdString(selectedScriptPath_));\n\nDon't forget to move the #include from the header to this\nfile.\n\nFurthermore, converting the QString to std::string to then convert it\nback to QString here is not efficient. I would use QString everywhere in\nthis class and convert to std::string in the caller (MainWindow).\n\n> +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> +\t} else {\n> +\t\tselectedScriptPath_ = scriptPath_;\n> +\t}\n\nThis while logic looks weird, selectedScriptPath_ is modified when the\nfile dialog is rejected, to then be restored to the previous value.\nStore the return value of getOpenFileName() in a local QString\nscriptPath variable first, with then\n\n\tif (scriptPath.isNull))\n\t\treturn;\n\n\tQFileInfo fileInfo(scriptPath);\n\tscriptFileLine_->setText(fileInfo.fileName());\n\tscriptFileLine_->setToolTip(fileInfo.filePath());\n\n\tselectedScriptPath_ = scriptPath.toStdString();\n\n> +}\n> +\n> +void CameraSelectorDialog::resetCaptureScript()\n> +{\n> +\tQ_EMIT stopCaptureScript();\n> +\tscriptPath_.clear();\n> +\tselectedScriptPath_.clear();\n> +\tscriptFileLine_->setText(\"No File Selected\");\n> +}\n> +\n> +void CameraSelectorDialog::accept()\n> +{\n> +\tscriptPath_ = selectedScriptPath_;\n> +\tQDialog::accept();\n> +}\n> +\n> +void CameraSelectorDialog::reject()\n> +{\n> +\tif (scriptPath_.empty()) {\n> +\t\tscriptFileLine_->setText(\"No File Selected\");\n> +\t} else {\n> +\t\tscriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> +\t}\n\nNo need for this. When the camera selector dialog is rejected it can\nstill remember which capture script has been selected. You can thus drop\nthe accept() and reject() overrides, and merge the two scriptPath_ and\nselectedScriptPath_ variables into a single one.\n\n> +\tQDialog::reject();\n> +}\n> diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> index c91b7ebe..377faebc 100644\n> --- a/src/qcam/cam_select_dialog.h\n> +++ b/src/qcam/cam_select_dialog.h\n> @@ -15,21 +15,24 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> class QComboBox;\n> class QLabel;\n> +class QToolButton;\n> \n> class CameraSelectorDialog : public QDialog\n> {\n> \tQ_OBJECT\n> public:\n> \tCameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> -\t\t\t QWidget *parent);\n> +\t\t\t std::string scriptPath, QWidget *parent);\n> \n> \t~CameraSelectorDialog();\n> \n> \tstd::string getCameraId();\n> +\tstd::string getCaptureScript() { return scriptPath_; };\n> \n> \t/* Hotplug / Unplug Support. */\n> \tvoid addCamera(QString cameraId);\n> @@ -38,11 +41,26 @@ public:\n> \t/* Camera Information */\n> \tvoid updateCamInfo(QString cameraId);\n> \n> +\t/* Capture script support. */\n> +\tvoid selectCaptureScript();\n> +\tvoid resetCaptureScript();\n\nThese should be private Q_SLOTS.\n\n> +\n> +\tvoid accept() override;\n> +\tvoid reject() override;\n> +\n> +Q_SIGNALS:\n> +\tvoid stopCaptureScript();\n\nDrop this signal, the capture script should be retrieved from the dialog\nby the MainWindow class when the dialog is accepted, there's no need for\ndynamic notification.\n\n> +\n> private:\n> \tlibcamera::CameraManager *cm_;\n> \n> +\tstd::string scriptPath_;\n> +\tstd::string selectedScriptPath_;\n> +\tQFileInfo scriptFileInfo_;\n\nAdd a blank line here.\n\n> \t/* UI elements. */\n> \tQComboBox *cameraIdComboBox_;\n> \tQLabel *cameraLocation_;\n> \tQLabel *cameraModel_;\n> +\tQLabel *scriptFileLine_;\n> +\tQToolButton *chooseCaptureScriptButton_;\n> };\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 2a9ca830..af992b94 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -18,6 +19,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -143,7 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> \tcm_->cameraAdded.connect(this, &MainWindow::addCamera);\n> \tcm_->cameraRemoved.connect(this, &MainWindow::removeCamera);\n> \n> -\tcameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);\n> +\tcameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);\n> +\tconnect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> +\t\tthis, &MainWindow::stopCaptureScript);\n> \n> \t/* Open the camera and start capture. */\n> \tret = openCamera();\n> @@ -152,6 +156,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> \t\treturn;\n> \t}\n> \n> +\t/* Start capture script. */\n> +\tif (!scriptPath_.empty())\n> +\t\tret = loadCaptureScript();\n\nret isn't used.\n\n> +\n> \tstartStopAction_->setChecked(true);\n> }\n> \n> @@ -266,8 +274,11 @@ void MainWindow::switchCamera()\n> \tif (newCameraId.empty())\n> \t\treturn;\n> \n> -\tif (camera_ && newCameraId == camera_->id())\n> +\tif (camera_ && newCameraId == camera_->id()) {\n> +\t\t// When user opens camera selection dialog for CaptureScript selection\n\nC-style comments and line wrap.\n\n> +\t\tloadCaptureScript();\n> \t\treturn;\n> +\t}\n> \n> \tconst std::shared_ptr &cam = cm_->get(newCameraId);\n> \n> @@ -287,17 +298,63 @@ void MainWindow::switchCamera()\n> \tcamera_->release();\n> \tcamera_ = cam;\n> \n> +\tloadCaptureScript();\n> +\n> \tstartStopAction_->setChecked(true);\n> \n> \t/* Display the current cameraId in the toolbar .*/\n> \tcameraSelectButton_->setText(QString::fromStdString(newCameraId));\n> }\n> \n> +void MainWindow::stopCaptureScript()\n> +{\n> +\tif (script_)\n> +\t\tscript_.reset();\n> +}\n\nBlank line.\n\n> +/**\n> + * \\brief Loads and validates the current capture script\n> + *\n> + * returns -EINVAL on failure and 0 on success\n> + */\n\nWe don't want to generate documentation from this file, so use /*\ninstead of /** and drop the \\brief:\n\n/*\n * Load and validate the current capture script. Return -EINVAL on failure and 0\n * on success.\n */\n\n> +int MainWindow::loadCaptureScript()\n> +{\n> +\tif (scriptPath_.empty() || camera_ == nullptr)\n> +\t\treturn -EINVAL;\n> +\n> +\tauto script = std::make_unique(camera_, scriptPath_);\n> +\n> +\tif (!script->valid()) {\n> +\t\tscript.reset();\n> +\n> +\t\tQMessageBox::critical(this, \"Invalid Script\",\n> +\t\t\t\t \"Couldn't load the capture script\");\n> +\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/*\n> +\t * If we are already capturing, stop so we don't have stuck image\n> +\t * in viewfinder.\n> +\t */\n> +\tbool wasCapturing = isCapturing_;\n> +\tif (isCapturing_)\n> +\t\ttoggleCapture(false);\n> +\n> +\tscript_ = std::move(script);\n> +\n> +\t/* Start capture again if we were capturing before. */\n> +\tif (wasCapturing)\n> +\t\ttoggleCapture(true);\n> +\treturn 0;\n> +}\n> +\n> std::string MainWindow::chooseCamera()\n> {\n> \tif (cameraSelectorDialog_->exec() != QDialog::Accepted)\n> \t\treturn std::string();\n> \n> +\tscriptPath_ = cameraSelectorDialog_->getCaptureScript();\n> +\n> \treturn cameraSelectorDialog_->getCameraId();\n\nI don't like much how the capture script is stored in a member variable\nbut the camera ID is returned. This isn't consistent, you should treat\nboth the same way (returning an std::tuple is an option if you want to\nreturn them).\n\n> }\n> \n> @@ -499,6 +556,7 @@ int MainWindow::startCapture()\n> \tpreviousFrames_ = 0;\n> \tframesCaptured_ = 0;\n> \tlastBufferTime_ = 0;\n> +\tqueueCount_ = 0;\n> \n> \tret = camera_->start();\n> \tif (ret) {\n> @@ -777,5 +835,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer)\n> \n> int MainWindow::queueRequest(Request *request)\n> {\n> +\tif (script_)\n> +\t\trequest->controls() = script_->frameControls(queueCount_);\n> +\n> +\tqueueCount_++;\n> +\n> \treturn camera_->queueRequest(request);\n> }\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 22c85247..7c877ae1 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -27,6 +27,7 @@\n> #include \n> #include \n> \n> +#include \"../cam/capture_script.h\"\n> #include \"../cam/stream_options.h\"\n> \n> #include \"viewfinder.h\"\n> @@ -89,6 +90,9 @@ private:\n> \tvoid processHotplug(HotplugEvent *e);\n> \tvoid processViewfinder(libcamera::FrameBuffer *buffer);\n> \n> +\tint loadCaptureScript();\n> +\tvoid stopCaptureScript();\n> +\n> \t/* UI elements */\n> \tQToolBar *toolbar_;\n> \tQAction *startStopAction_;\n> @@ -129,6 +133,9 @@ private:\n> \tQElapsedTimer frameRateInterval_;\n> \tuint32_t previousFrames_;\n> \tuint32_t framesCaptured_;\n> +\tuint32_t queueCount_;\n> \n> \tstd::vector> requests_;\n> +\tstd::unique_ptr script_;\n> +\tstd::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":"","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 27956C0DA4\n\tfor ;\n\tWed, 31 Aug 2022 09:59:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E8D761FBE;\n\tWed, 31 Aug 2022 11:59:25 +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 6C64D603E1\n\tfor ;\n\tWed, 31 Aug 2022 11:59:24 +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 5D59351E;\n\tWed, 31 Aug 2022 11:59:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661939965;\n\tbh=VSZ5QZ2nrL6W9mVCAjV1TRzJV0B3l0C9QJU/aUwtSho=;\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=OggaEMQHF/OdR+PrbQaSasbpmCawYsiES7TDVi//gR7gA2OLQ486BuntsB97hqi9a\n\tRc8SuxZ6sVItXhnfbfWgb7304YZgGhsh7W1UksYTOAt7HhHWYEkoVs1flDN6YLrIkp\n\tzeAwwvQ5irYR48fhQjGCQQj2y7ctCSFlVwwI/Ty8GZ2AfkB+i0kugQoN4ABweco7GV\n\tu4WoORJCuMqXbUK031yYeDZ/mh2t8C0o+CiRQlmzCKFmvbDuixZZ94BhUmu+U2X9WX\n\tVJ7Znk2LPz5WD06JVUft1/516axhkVclZfJr9VGoeQqOkOAiY7ECruzGnS96nI8d/b\n\tNtjLezpKKAvRg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661939964;\n\tbh=VSZ5QZ2nrL6W9mVCAjV1TRzJV0B3l0C9QJU/aUwtSho=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RY+deG7IcJub1/aEUmpxa8ANfKjHHh1/RwbZKWMOr4H1khm+B/a6YyZWmV6LGQ7I0\n\t4St1FaoOgWHraLRYbG9C/+mH/aUk04pRw5KoeoRmdTVmgPF/NzH7DdT/EITC64ndHe\n\tBBvHpmJWwcUphErvU/zl6MQcglbsIOOBVMeD6hvY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RY+deG7I\"; dkim-atps=neutral","Date":"Wed, 31 Aug 2022 12:59:12 +0300","To":"Utkarsh Tiwari ","Message-ID":"","References":"<20220831054938.21617-1-utkarsh02t@gmail.com>\n\t<20220831054938.21617-7-utkarsh02t@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220831054938.21617-7-utkarsh02t@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add\n\tcapture script button","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24880,"web_url":"https://patchwork.libcamera.org/comment/24880/","msgid":"<20220901054842.yfpjycdzqxcxaz5v@gmail.com>","date":"2022-09-01T05:48:42","subject":"Re: [libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add\n\tcapture script button","submitter":{"id":114,"url":"https://patchwork.libcamera.org/api/people/114/","name":"Utkarsh Tiwari","email":"utkarsh02t@gmail.com"},"content":"Hi Kieran, thanks for the review.\nOn Wed, Aug 31, 2022 at 10:35:31AM +0100, Kieran Bingham wrote:\n> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-31 06:49:37)\n> > Display a QLabel which is readonly, and displays the currently\n> > selected capture script. The tooltip of the QLabel displays the file\n> > path of the script.\n> > \n> > Implement a capture script button which is a QToolButton which when\n> > clicked opens a QFileDialog this allows to select a capture script\n> > (*.yaml) file.\n> > \n> > Next to the capture scipt button, show a QToolButton which stops the\n> > capture script.\n> > \n> > If an invalid script has been selected show a QMesssageBox::critical and\n> > continue with the capture's previous state.\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> \"from the script\" ?\nNoted.\n> \n> > ---\n> > Differnce from v8:\n> > 1. Now display a QLabel with the fileName and filePath with button\n> > on the side.\n> > 2. infromScriptReset() informScriptRunning() are removed\n> > 3. Local script makes handling of invalid scripts easy.\n> > src/qcam/assets/feathericons/feathericons.qrc | 2 +\n> > src/qcam/cam_select_dialog.cpp | 88 ++++++++++++++++++-\n> > src/qcam/cam_select_dialog.h | 20 ++++-\n> > src/qcam/main_window.cpp | 67 +++++++++++++-\n> > src/qcam/main_window.h | 7 ++\n> > src/qcam/meson.build | 2 +\n> > 6 files changed, 181 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> > index c5302040..0ea0c2d5 100644\n> > --- a/src/qcam/assets/feathericons/feathericons.qrc\n> > +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> > @@ -3,7 +3,9 @@\n> > \n> > aperture.svg\n> > camera-off.svg\n> > + delete.svg\n> > play-circle.svg\n> > + upload.svg\n> \n> Indentations are wrong here.\nAh should have not set the expandtabs. Would fix.\n> \n> \n> > save.svg\n> > stop-circle.svg\n> > x-circle.svg\n> > diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> > index 6543228a..99405cc1 100644\n> > --- a/src/qcam/cam_select_dialog.cpp\n> > +++ b/src/qcam/cam_select_dialog.cpp\n> > @@ -14,13 +14,18 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > +#include \n> > +#include \n> > #include \n> > #include \n> > +#include \n> > +#include \n> > \n> > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > - QWidget *parent)\n> > - : QDialog(parent), cm_(cameraManager)\n> > + std::string scriptPath, QWidget *parent)\n> > + : QDialog(parent), cm_(cameraManager), scriptPath_(scriptPath)\n> > {\n> > /* Use a QFormLayout for the dialog. */\n> > QFormLayout *layout = new QFormLayout(this);\n> > @@ -38,6 +43,41 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> > connect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> > this, &CameraSelectorDialog::updateCamInfo);\n> > \n> > + /* Set capture script selection / removal button. */\n> > + QWidget *captureWidget = new QWidget(this);\n> > + QHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);\n> > +\n> > + scriptFileLine_ = new QLabel;\n> > + scriptFileLine_->setFrameStyle(QFrame::Sunken | QFrame::StyledPanel);\n> > +\n> > + chooseCaptureScriptButton_ = new QToolButton;\n> > + chooseCaptureScriptButton_->setIcon(QIcon::fromTheme(\"document-open\",\n> > + QIcon(\":upload.svg\")));\n> > + chooseCaptureScriptButton_->setStyleSheet(\"border:none\");\n> > + connect(chooseCaptureScriptButton_, &QToolButton::clicked,\n> > + this, &CameraSelectorDialog::selectCaptureScript);\n> > +\n> > + QToolButton *stopCaptureScriptButton = new QToolButton;\n> > + stopCaptureScriptButton->setIcon(QIcon::fromTheme(\"edit-clear\",\n> > + QIcon(\":delete.svg\")));\n> > + stopCaptureScriptButton->setStyleSheet(\"border:node;\");\n> > + connect(stopCaptureScriptButton, &QToolButton::clicked,\n> > + this, &CameraSelectorDialog::resetCaptureScript);\n> > +\n> > + captureLayout->addWidget(scriptFileLine_);\n> > + captureLayout->addWidget(chooseCaptureScriptButton_);\n> > + captureLayout->addWidget(stopCaptureScriptButton);\n> > + captureLayout->setMargin(0);\n> > +\n> > + /* Set the file name of the capture script. */\n> > + if (scriptPath_.empty()) {\n> > + scriptFileLine_->setText(\"No File Selected\");\n> > + } else {\n> > + scriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> > + scriptFileLine_->setText(scriptFileInfo_.fileName());\n> > + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > + }\n> > +\n> > /* Setup the QDialogButton Box */\n> > QDialogButtonBox *buttonBox =\n> > new QDialogButtonBox(QDialogButtonBox::Ok |\n> > @@ -52,6 +92,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> > layout->addRow(\"Camera:\", cameraIdComboBox_);\n> > layout->addRow(\"Location:\", cameraLocation_);\n> > layout->addRow(\"Model:\", cameraModel_);\n> > + layout->addRow(\"Capture Script:\", captureWidget);\n> > layout->addWidget(buttonBox);\n> > }\n> > \n> > @@ -110,3 +151,46 @@ void CameraSelectorDialog::updateCamInfo(QString cameraId)\n> > \n> > cameraModel_->setText(QString::fromStdString(model));\n> > }\n> > +\n> > +/* Capture script support. */\n> > +void CameraSelectorDialog::selectCaptureScript()\n> > +{\n> > + selectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> > + \"Run Capture Script\", QDir::currentPath(),\n> > + \"Capture Script (*.yaml)\")\n> > + .toStdString();\n> \n> Where does checkstyle recommend to place this? It seems to be 'floating'\n> too.\n> \n> It's tricky, but I'm not sure there is much better options anyway, so\n> I'll move on.\n> \n> > +\n> > + if (!selectedScriptPath_.empty()) {\n> > + scriptFileInfo_.setFile(QString::fromStdString(selectedScriptPath_));\n> > + scriptFileLine_->setText(scriptFileInfo_.fileName());\n> > + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > + } else {\n> > + selectedScriptPath_ = scriptPath_;\n> > + }\n> > +}\n> > +\n> > +void CameraSelectorDialog::resetCaptureScript()\n> > +{\n> > + Q_EMIT stopCaptureScript();\n> > + scriptPath_.clear();\n> > + selectedScriptPath_.clear();\n> > + scriptFileLine_->setText(\"No File Selected\");\n> > +}\n> > +\n> > +void CameraSelectorDialog::accept()\n> > +{\n> > + scriptPath_ = selectedScriptPath_;\n> > + QDialog::accept();\n> > +}\n> > +\n> > +void CameraSelectorDialog::reject()\n> > +{\n> > + if (scriptPath_.empty()) {\n> > + scriptFileLine_->setText(\"No File Selected\");\n> > + } else {\n> > + scriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> > + scriptFileLine_->setText(scriptFileInfo_.fileName());\n> > + scriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > + }\n> > + QDialog::reject();\n> > +}\n> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > index c91b7ebe..377faebc 100644\n> > --- a/src/qcam/cam_select_dialog.h\n> > +++ b/src/qcam/cam_select_dialog.h\n> > @@ -15,21 +15,24 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > class QComboBox;\n> > class QLabel;\n> > +class QToolButton;\n> > \n> > class CameraSelectorDialog : public QDialog\n> > {\n> > Q_OBJECT\n> > public:\n> > CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > - QWidget *parent);\n> > + std::string scriptPath, QWidget *parent);\n> > \n> > ~CameraSelectorDialog();\n> > \n> > std::string getCameraId();\n> > + std::string getCaptureScript() { return scriptPath_; };\n> > \n> > /* Hotplug / Unplug Support. */\n> > void addCamera(QString cameraId);\n> > @@ -38,11 +41,26 @@ public:\n> > /* Camera Information */\n> > void updateCamInfo(QString cameraId);\n> > \n> > + /* Capture script support. */\n> > + void selectCaptureScript();\n> > + void resetCaptureScript();\n> > +\n> > + void accept() override;\n> > + void reject() override;\n> > +\n> > +Q_SIGNALS:\n> > + void stopCaptureScript();\n> > +\n> > private:\n> > libcamera::CameraManager *cm_;\n> > \n> > + std::string scriptPath_;\n> > + std::string selectedScriptPath_;\n> > + QFileInfo scriptFileInfo_;\n> > /* UI elements. */\n> > QComboBox *cameraIdComboBox_;\n> > QLabel *cameraLocation_;\n> > QLabel *cameraModel_;\n> > + QLabel *scriptFileLine_;\n> > + QToolButton *chooseCaptureScriptButton_;\n> > };\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 2a9ca830..af992b94 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -9,6 +9,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -18,6 +19,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -143,7 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > cm_->cameraAdded.connect(this, &MainWindow::addCamera);\n> > cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);\n> > \n> > - cameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);\n> > + cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);\n> > + connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> > + this, &MainWindow::stopCaptureScript);\n> > \n> > /* Open the camera and start capture. */\n> > ret = openCamera();\n> > @@ -152,6 +156,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > return;\n> > }\n> > \n> > + /* Start capture script. */\n> > + if (!scriptPath_.empty())\n> > + ret = loadCaptureScript();\n> > +\n> > startStopAction_->setChecked(true);\n> > }\n> > \n> > @@ -266,8 +274,11 @@ void MainWindow::switchCamera()\n> > if (newCameraId.empty())\n> > return;\n> > \n> > - if (camera_ && newCameraId == camera_->id())\n> > + if (camera_ && newCameraId == camera_->id()) {\n> > + // When user opens camera selection dialog for CaptureScript selection\n> \n> Perhaps:\n> \t\t/*\n> \t\t * If the camera has not changed, We still need to\n> \t\t * handle any update to the capture script. This will\n> \t\t * cause the script to restart if it was already\n> \t\t * selected.\n> \t\t */\n> \n> \n> Or something like that (and correctly formatted).\nSure.\n> \n> \n> > + loadCaptureScript();\n> > return;\n> > + }\n> > \n> > const std::shared_ptr &cam = cm_->get(newCameraId);\n> > \n> > @@ -287,17 +298,63 @@ void MainWindow::switchCamera()\n> > camera_->release();\n> > camera_ = cam;\n> > \n> > + loadCaptureScript();\n> > +\n> > startStopAction_->setChecked(true);\n> > \n> > /* Display the current cameraId in the toolbar .*/\n> > cameraSelectButton_->setText(QString::fromStdString(newCameraId));\n> > }\n> > \n> > +void MainWindow::stopCaptureScript()\n> > +{\n> > + if (script_)\n> > + script_.reset();\n> > +}\n> \n> This needs a blank line.\nSure. I thought I have fixed these in a rebase but seems not. anyway\nseems like this would need v10.\n> \n> > +/**\n> > + * \\brief Loads and validates the current capture script\n> > + *\n> \n> Can we describe here that the current capture session will be restarted\n> if it was active?\nYes I think.\n> \n> > + * returns -EINVAL on failure and 0 on success\n> > + */\n> > +int MainWindow::loadCaptureScript()\n> > +{\n> > + if (scriptPath_.empty() || camera_ == nullptr)\n> > + return -EINVAL;\n> \n> Perhaps -ENODEV? But I expect this can't happen. I think it's fine to be\n> defensive here though.\nYes.\n if(scriptPath_.empty())\n return -EINVAL;\n if(!camera_)\n return -ENODEV;\n> \n> > +\n> > + auto script = std::make_unique(camera_, scriptPath_);\n> > +\n> > + if (!script->valid()) {\n> > + script.reset();\n> > +\n> > + QMessageBox::critical(this, \"Invalid Script\",\n> > + \"Couldn't load the capture script\");\n> > +\n> > + return -EINVAL;\n> > + }\n> > +\n> > + /*\n> > + * If we are already capturing, stop so we don't have stuck image\n> \n> \"have a stuck image\"\n> \n> > + * in viewfinder.\n> \n> \"in the viewfinder\"\nNoted.\n> \n> > + */\n> > + bool wasCapturing = isCapturing_;\n> > + if (isCapturing_)\n> > + toggleCapture(false);\n> > +\n> > + script_ = std::move(script);\n> > +\n> > + /* Start capture again if we were capturing before. */\n> > + if (wasCapturing)\n> > + toggleCapture(true);\n> \n> New line here.\n> \n> \n> All of those comments seem quite minor, so with those resolved:\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > + return 0;\n> > +}\n> > +\n> > std::string MainWindow::chooseCamera()\n> > {\n> > if (cameraSelectorDialog_->exec() != QDialog::Accepted)\n> > return std::string();\n> > \n> > + scriptPath_ = cameraSelectorDialog_->getCaptureScript();\n> > +\n> > return cameraSelectorDialog_->getCameraId();\n> > }\n> > \n> > @@ -499,6 +556,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> > @@ -777,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 22c85247..7c877ae1 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -27,6 +27,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"../cam/capture_script.h\"\n> > #include \"../cam/stream_options.h\"\n> > \n> > #include \"viewfinder.h\"\n> > @@ -89,6 +90,9 @@ private:\n> > void processHotplug(HotplugEvent *e);\n> > void processViewfinder(libcamera::FrameBuffer *buffer);\n> > \n> > + int loadCaptureScript();\n> > + void stopCaptureScript();\n> > +\n> > /* UI elements */\n> > QToolBar *toolbar_;\n> > QAction *startStopAction_;\n> > @@ -129,6 +133,9 @@ private:\n> > QElapsedTimer frameRateInterval_;\n> > uint32_t previousFrames_;\n> > uint32_t framesCaptured_;\n> > + uint32_t queueCount_;\n> > \n> > std::vector> requests_;\n> > + std::unique_ptr 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.34.1\n> >","headers":{"Return-Path":"","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 082C7C0DA4\n\tfor ;\n\tThu, 1 Sep 2022 05:48:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41DF061FCD;\n\tThu, 1 Sep 2022 07:48:50 +0200 (CEST)","from mail-pl1-x636.google.com (mail-pl1-x636.google.com\n\t[IPv6:2607:f8b0:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4008461F9A\n\tfor ;\n\tThu, 1 Sep 2022 07:48:49 +0200 (CEST)","by mail-pl1-x636.google.com with SMTP id p18so16110773plr.8\n\tfor ;\n\tWed, 31 Aug 2022 22:48:49 -0700 (PDT)","from gmail.com ([2404:bd00:3:de55:7693:8bef:3577:357a])\n\tby smtp.gmail.com with ESMTPSA id\n\te15-20020a170902784f00b00172f4835f65sm10742493pln.271.2022.08.31.22.48.45\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 31 Aug 2022 22:48:46 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662011330;\n\tbh=hJI2sKzWQQn2+SBrh61Q5axnCJ53CQLVgrLHeJ9Jnxc=;\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=a8/IhsbXU6aPdtrgSnOYY52j1nKhZQwo28Uu6sIrI7CpewDsOJyfqjVjuY5qWb6CC\n\tjg5nGIgwuARaH71yKZ7hpmDFHH4dQKf+r6B1a4w9I3AudvO0XVTFfD1OgpsJgtZ9WJ\n\tNeeibpfh4DuPut7AJuSAJZ48ni4lhN3dWbJFYFLNzSVM9/bS8cLg/Z+qWakXN1bARf\n\tWyBeoUsv/S55hlciI5xdHedxJx1+OZXwtZywHUwKeH1rR602q5LMkjnf6Yvoe18fHu\n\tUz3hkEbaXw6TKBaKGj7dHImXVGolg+f5m1VhaOrukZyEAi0jWXhBvq8d0IIzfSP0fA\n\tZJf7wGeOet7VA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=in-reply-to:content-disposition:mime-version:references\n\t:mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc\n\t:subject:date; bh=DYKJNfV0N8l8EmKcp4gua0bJbkNZ8VReADXDga2Kguk=;\n\tb=ONNUcTS0vBs5dfUojyvt1aQA4To7I5jXS/cfhcatVylRQILxlGra7wG0i4WOOObB4J\n\t+MqOxWF/wNHPngjwp2x9A880m63PrcEJBknaBzWjSUhjapRcsrN0FD2ckmHhaN9/05/O\n\tCy1ptlMxVypfzUxwkvpfWmehfJh+KxNO2F/H7U8i4TjEeh326YIpRj7gBZPPaUAwmuMg\n\ti0mk23+rBXDF2wzEvSznG79/mSEKrMGBdfzbjcrql4PaOlnMhzbV9idYmTiuCjxnQWnU\n\t4hnOoxzOm65VA4qz5GVdhF4itpoE0uNhUe51niN6vidFWBGVST1+DtPh0B04btdaNsOd\n\tPQVQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"ONNUcTS0\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=in-reply-to:content-disposition:mime-version:references\n\t:mail-followup-to:message-id:subject:cc:to:from:date\n\t:x-gm-message-state:from:to:cc:subject:date;\n\tbh=DYKJNfV0N8l8EmKcp4gua0bJbkNZ8VReADXDga2Kguk=;\n\tb=KSrwpfJnoyjNUrJdmTPP9wORz3WtLOdlF+BdGP5y3Gyi32YNR3yFZU29eaa0EyQn/w\n\tBW8XEUE2PzbsoUtfMVn/ceYoqu6Sngl3Ch6FdG3TffwOf5vA63K15pifzrFPuGOOpKtX\n\ty8k7/D3Z/B2B4lhAfCDSHOEjEQ4WybrAKreuluC3sjDulZrpLStuogFP9Vije0JSecgw\n\tnt/ZhCnNzFnIPkI8+cYFZyd6d8uPzaAmXtiu9QFcG6uKNyDGR+Gwcd6+GrUfvmX2qvcT\n\tqB6chELJpJFziJCsbcGJiFSXLrKtAYQRLfMJdlAfL5H4eT9LNVH6Hmt1Q6Pj72EKNlCC\n\tRLuw==","X-Gm-Message-State":"ACgBeo3CHo/V1TJbZV3Yr7VBiXxeIFP+DA8CjvZgDcXJHNtlvmt8oh9t\n\tQCNE8foCs39Yu0n5w676cwNEaXFb2b0=","X-Google-Smtp-Source":"AA6agR6ysbg6Zhg7h+k98DLMZV6Spl/yQEMKc17oAiE7iRcU0nIhKIdUgQ8VQ9Cco9lcebk6CpROCw==","X-Received":"by 2002:a17:90b:4d12:b0:1f5:5af8:c093 with SMTP id\n\tmw18-20020a17090b4d1200b001f55af8c093mr7097617pjb.33.1662011327123; \n\tWed, 31 Aug 2022 22:48:47 -0700 (PDT)","Date":"Thu, 1 Sep 2022 11:18:42 +0530","To":"Kieran Bingham ","Message-ID":"<20220901054842.yfpjycdzqxcxaz5v@gmail.com>","Mail-Followup-To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220831054938.21617-1-utkarsh02t@gmail.com>\n\t<20220831054938.21617-7-utkarsh02t@gmail.com>\n\t<166193853183.15972.5974522827658911264@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<166193853183.15972.5974522827658911264@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add\n\tcapture script button","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Utkarsh Tiwari via libcamera-devel\n\t","Reply-To":"Utkarsh Tiwari ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24881,"web_url":"https://patchwork.libcamera.org/comment/24881/","msgid":"<20220901060459.cy3pruqh4ldnhixc@gmail.com>","date":"2022-09-01T06:04:59","subject":"Re: [libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add\n\tcapture script button","submitter":{"id":114,"url":"https://patchwork.libcamera.org/api/people/114/","name":"Utkarsh Tiwari","email":"utkarsh02t@gmail.com"},"content":"Hi Laurent, thanks for the review.\nOn Wed, Aug 31, 2022 at 12:59:12PM +0300, Laurent Pinchart wrote:\n> Hi Utkarsh,\n> \n> Thank you for the patch.\n> \n> On Wed, Aug 31, 2022 at 11:19:37AM +0530, Utkarsh Tiwari via libcamera-devel wrote:\n> > Display a QLabel which is readonly, and displays the currently\n> \n> Labels are always read-only :-)\n:P would fix.\n> \n> > selected capture script. The tooltip of the QLabel displays the file\n> > path of the script.\n> > \n> > Implement a capture script button which is a QToolButton which when\n> > clicked opens a QFileDialog this allows to select a capture script\n> > (*.yaml) file.\n> > \n> > Next to the capture scipt button, show a QToolButton which stops the\n> > capture script.\n> > \n> > If an invalid script has been selected show a QMesssageBox::critical and\n> > continue with the capture's previous state.\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> Missing SoB line.\nOops would add that.\n> \n> > ---\n> > Differnce from v8:\n> > 1. Now display a QLabel with the fileName and filePath with button\n> > on the side.\n> > 2. infromScriptReset() informScriptRunning() are removed\n> > 3. Local script makes handling of invalid scripts easy.\n> > src/qcam/assets/feathericons/feathericons.qrc | 2 +\n> > src/qcam/cam_select_dialog.cpp | 88 ++++++++++++++++++-\n> > src/qcam/cam_select_dialog.h | 20 ++++-\n> > src/qcam/main_window.cpp | 67 +++++++++++++-\n> > src/qcam/main_window.h | 7 ++\n> > src/qcam/meson.build | 2 +\n> > 6 files changed, 181 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> > index c5302040..0ea0c2d5 100644\n> > --- a/src/qcam/assets/feathericons/feathericons.qrc\n> > +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> > @@ -3,7 +3,9 @@\n> > \n> > \taperture.svg\n> > \tcamera-off.svg\n> > + delete.svg\n> \n> Please indent with tabs, not spaces.\nGotta learn to use in vim.\n> \n> > \tplay-circle.svg\n> > + upload.svg\n> \n> Same here.\n> \n> > \tsave.svg\n> > \tstop-circle.svg\n> > \tx-circle.svg\n> > diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> > index 6543228a..99405cc1 100644\n> > --- a/src/qcam/cam_select_dialog.cpp\n> > +++ b/src/qcam/cam_select_dialog.cpp\n> > @@ -14,13 +14,18 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > +#include \n> > +#include \n> > #include \n> > #include \n> > +#include \n> \n> Why not a QPushButton ? QToolButton is usually meant for toolbars.\nQToolButton works much better in cases when we require only the icon.\n> \n> > +#include \n> > \n> > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > -\t\t\t\t\t QWidget *parent)\n> > -\t: QDialog(parent), cm_(cameraManager)\n> > +\t\t\t\t\t std::string scriptPath, QWidget *parent)\n> \n> Make it a const std::string & to avoid copies.\nSure.\n> \n> > +\t: QDialog(parent), cm_(cameraManager), scriptPath_(scriptPath)\n> > {\n> > \t/* Use a QFormLayout for the dialog. */\n> > \tQFormLayout *layout = new QFormLayout(this);\n> > @@ -38,6 +43,41 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> > \tconnect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> > \t\tthis, &CameraSelectorDialog::updateCamInfo);\n> > \n> > +\t/* Set capture script selection / removal button. */\n> > +\tQWidget *captureWidget = new QWidget(this);\n> \n> Maybe captureScriptWidhet ?\nSure, maybe not in a Mike Tyson accept tho :P\n> \n> > +\tQHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);\n> \n> Ditto.\nNoted.\n> \n> > +\n> > +\tscriptFileLine_ = new QLabel;\n> > +\tscriptFileLine_->setFrameStyle(QFrame::Sunken | QFrame::StyledPanel);\n> \n> Generally speaking, changing styles manually should be avoided as much\n> as possible. It makes the overall style inconsistent and is confusing\n> for users. I'd drop this.\n> \nI think we the frame provides a border in which the user knows placing\nthe pointer would result in the tooltip showing up and displaying the\npath. Provides more visual feedback.\n> > +\n> > +\tchooseCaptureScriptButton_ = new QToolButton;\n> > +\tchooseCaptureScriptButton_->setIcon(QIcon::fromTheme(\"document-open\",\n> > +\t\t\t\t\t\t\t QIcon(\":upload.svg\")));\n> > +\tchooseCaptureScriptButton_->setStyleSheet(\"border:none\");\n> \n> Same here, drop this. A button that doesn't look like a button gives no\n> visual clue that it can be clicked.\n> \nAh, I think went overboard making it icon only, would do.\n> > +\tconnect(chooseCaptureScriptButton_, &QToolButton::clicked,\n> > +\t\tthis, &CameraSelectorDialog::selectCaptureScript);\n> > +\n> > +\tQToolButton *stopCaptureScriptButton = new QToolButton;\n> > +\tstopCaptureScriptButton->setIcon(QIcon::fromTheme(\"edit-clear\",\n> > +\t\t\t\t\t\t\t QIcon(\":delete.svg\")));\n> > +\tstopCaptureScriptButton->setStyleSheet(\"border:node;\");\nSame here.\n> > +\tconnect(stopCaptureScriptButton, &QToolButton::clicked,\n> > +\t\tthis, &CameraSelectorDialog::resetCaptureScript);\n> > +\n> > +\tcaptureLayout->addWidget(scriptFileLine_);\n> > +\tcaptureLayout->addWidget(chooseCaptureScriptButton_);\n> > +\tcaptureLayout->addWidget(stopCaptureScriptButton);\n> > +\tcaptureLayout->setMargin(0);\n> > +\n> > +\t/* Set the file name of the capture script. */\n> > +\tif (scriptPath_.empty()) {\n> > +\t\tscriptFileLine_->setText(\"No File Selected\");\n> > +\t} else {\n> > +\t\tscriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> > +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> > +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > +\t}\n> > +\n> > \t/* Setup the QDialogButton Box */\n> > \tQDialogButtonBox *buttonBox =\n> > \t\tnew QDialogButtonBox(QDialogButtonBox::Ok |\n> > @@ -52,6 +92,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> > \tlayout->addRow(\"Camera:\", cameraIdComboBox_);\n> > \tlayout->addRow(\"Location:\", cameraLocation_);\n> > \tlayout->addRow(\"Model:\", cameraModel_);\n> > +\tlayout->addRow(\"Capture Script:\", captureWidget);\n> > \tlayout->addWidget(buttonBox);\n> > }\n> > \n> > @@ -110,3 +151,46 @@ void CameraSelectorDialog::updateCamInfo(QString cameraId)\n> > \n> > \tcameraModel_->setText(QString::fromStdString(model));\n> > }\n> > +\n> > +/* Capture script support. */\n> > +void CameraSelectorDialog::selectCaptureScript()\n> > +{\n> > +\tselectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> > +\t\t\t\t\t\t\t \"Run Capture Script\", QDir::currentPath(),\n> > +\t\t\t\t\t\t\t \"Capture Script (*.yaml)\")\n> > +\t\t\t\t .toStdString();\n> > +\n> > +\tif (!selectedScriptPath_.empty()) {\n> > +\t\tscriptFileInfo_.setFile(QString::fromStdString(selectedScriptPath_));\n> \n> You can make this a local variable:\n> \n> \t\tQFileInfo fileInfo(QString::fromStdString(selectedScriptPath_));\n> \n> Don't forget to move the #include from the header to this\n> file.\n> \n> Furthermore, converting the QString to std::string to then convert it\n> back to QString here is not efficient. I would use QString everywhere in\n> this class and convert to std::string in the caller (MainWindow).\n> \nI agree, we only take scriptPath in std::string, we can convert that and\nuse QString everywhere here.\n> > +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> > +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > +\t} else {\n> > +\t\tselectedScriptPath_ = scriptPath_;\n> > +\t}\n> \n> This while logic looks weird, selectedScriptPath_ is modified when the\n> file dialog is rejected, to then be restored to the previous value.\n> Store the return value of getOpenFileName() in a local QString\n> scriptPath variable first, with then\n> \n> \tif (scriptPath.isNull))\n> \t\treturn;\n> \n> \tQFileInfo fileInfo(scriptPath);\n> \tscriptFileLine_->setText(fileInfo.fileName());\n> \tscriptFileLine_->setToolTip(fileInfo.filePath());\n> \n> \tselectedScriptPath_ = scriptPath.toStdString();\n> \nOops, makes much easier would use.\n> > +}\n> > +\n> > +void CameraSelectorDialog::resetCaptureScript()\n> > +{\n> > +\tQ_EMIT stopCaptureScript();\n> > +\tscriptPath_.clear();\n> > +\tselectedScriptPath_.clear();\n> > +\tscriptFileLine_->setText(\"No File Selected\");\n> > +}\n> > +\n> > +void CameraSelectorDialog::accept()\n> > +{\n> > +\tscriptPath_ = selectedScriptPath_;\n> > +\tQDialog::accept();\n> > +}\n> > +\n> > +void CameraSelectorDialog::reject()\n> > +{\n> > +\tif (scriptPath_.empty()) {\n> > +\t\tscriptFileLine_->setText(\"No File Selected\");\n> > +\t} else {\n> > +\t\tscriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> > +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> > +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > +\t}\n> \n> No need for this. When the camera selector dialog is rejected it can\n> still remember which capture script has been selected. You can thus drop\n> the accept() and reject() overrides, and merge the two scriptPath_ and\n> selectedScriptPath_ variables into a single one.\n> \n\nI'm a bit doubtful on this,\n 1. I start a script.\n 2. Open the dialog select some different script.\n 3. Reject the dialog.\n 4. Few minutes pass\n 5. I open the CameraSelectorDialog I think I would expect the dialog\n to show me the current script which is running.\n\nIn my opinion after doing any action, reject or accept the label should\nshow what has been already passed to loadCaptureScript().\nOpen to more suggetions.\n> > +\tQDialog::reject();\n> > +}\n> > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > index c91b7ebe..377faebc 100644\n> > --- a/src/qcam/cam_select_dialog.h\n> > +++ b/src/qcam/cam_select_dialog.h\n> > @@ -15,21 +15,24 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > class QComboBox;\n> > class QLabel;\n> > +class QToolButton;\n> > \n> > class CameraSelectorDialog : public QDialog\n> > {\n> > \tQ_OBJECT\n> > public:\n> > \tCameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > -\t\t\t QWidget *parent);\n> > +\t\t\t std::string scriptPath, QWidget *parent);\n> > \n> > \t~CameraSelectorDialog();\n> > \n> > \tstd::string getCameraId();\n> > +\tstd::string getCaptureScript() { return scriptPath_; };\n> > \n> > \t/* Hotplug / Unplug Support. */\n> > \tvoid addCamera(QString cameraId);\n> > @@ -38,11 +41,26 @@ public:\n> > \t/* Camera Information */\n> > \tvoid updateCamInfo(QString cameraId);\n> > \n> > +\t/* Capture script support. */\n> > +\tvoid selectCaptureScript();\n> > +\tvoid resetCaptureScript();\n> \n> These should be private Q_SLOTS.\n> \n> > +\n> > +\tvoid accept() override;\n> > +\tvoid reject() override;\n> > +\n> > +Q_SIGNALS:\n> > +\tvoid stopCaptureScript();\n> \n> Drop this signal, the capture script should be retrieved from the dialog\n> by the MainWindow class when the dialog is accepted, there's no need for\n> dynamic notification.\n> \n> > +\n> > private:\n> > \tlibcamera::CameraManager *cm_;\n> > \n> > +\tstd::string scriptPath_;\n> > +\tstd::string selectedScriptPath_;\n> > +\tQFileInfo scriptFileInfo_;\n> \n> Add a blank line here.\n> \n> > \t/* UI elements. */\n> > \tQComboBox *cameraIdComboBox_;\n> > \tQLabel *cameraLocation_;\n> > \tQLabel *cameraModel_;\n> > +\tQLabel *scriptFileLine_;\n> > +\tQToolButton *chooseCaptureScriptButton_;\n> > };\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 2a9ca830..af992b94 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -9,6 +9,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -18,6 +19,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -143,7 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > \tcm_->cameraAdded.connect(this, &MainWindow::addCamera);\n> > \tcm_->cameraRemoved.connect(this, &MainWindow::removeCamera);\n> > \n> > -\tcameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);\n> > +\tcameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);\n> > +\tconnect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> > +\t\tthis, &MainWindow::stopCaptureScript);\n> > \n> > \t/* Open the camera and start capture. */\n> > \tret = openCamera();\n> > @@ -152,6 +156,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > \t\treturn;\n> > \t}\n> > \n> > +\t/* Start capture script. */\n> > +\tif (!scriptPath_.empty())\n> > +\t\tret = loadCaptureScript();\n> \n> ret isn't used.\n> \n> > +\n> > \tstartStopAction_->setChecked(true);\n> > }\n> > \n> > @@ -266,8 +274,11 @@ void MainWindow::switchCamera()\n> > \tif (newCameraId.empty())\n> > \t\treturn;\n> > \n> > -\tif (camera_ && newCameraId == camera_->id())\n> > +\tif (camera_ && newCameraId == camera_->id()) {\n> > +\t\t// When user opens camera selection dialog for CaptureScript selection\n> \n> C-style comments and line wrap.\n> \n> > +\t\tloadCaptureScript();\n> > \t\treturn;\n> > +\t}\n> > \n> > \tconst std::shared_ptr &cam = cm_->get(newCameraId);\n> > \n> > @@ -287,17 +298,63 @@ void MainWindow::switchCamera()\n> > \tcamera_->release();\n> > \tcamera_ = cam;\n> > \n> > +\tloadCaptureScript();\n> > +\n> > \tstartStopAction_->setChecked(true);\n> > \n> > \t/* Display the current cameraId in the toolbar .*/\n> > \tcameraSelectButton_->setText(QString::fromStdString(newCameraId));\n> > }\n> > \n> > +void MainWindow::stopCaptureScript()\n> > +{\n> > +\tif (script_)\n> > +\t\tscript_.reset();\n> > +}\n> \n> Blank line.\n> \n> > +/**\n> > + * \\brief Loads and validates the current capture script\n> > + *\n> > + * returns -EINVAL on failure and 0 on success\n> > + */\n> \n> We don't want to generate documentation from this file, so use /*\n> instead of /** and drop the \\brief:\n> \n> /*\n> * Load and validate the current capture script. Return -EINVAL on failure and 0\n> * on success.\n> */\n> \n> > +int MainWindow::loadCaptureScript()\n> > +{\n> > +\tif (scriptPath_.empty() || camera_ == nullptr)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tauto script = std::make_unique(camera_, scriptPath_);\n> > +\n> > +\tif (!script->valid()) {\n> > +\t\tscript.reset();\n> > +\n> > +\t\tQMessageBox::critical(this, \"Invalid Script\",\n> > +\t\t\t\t \"Couldn't load the capture script\");\n> > +\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * If we are already capturing, stop so we don't have stuck image\n> > +\t * in viewfinder.\n> > +\t */\n> > +\tbool wasCapturing = isCapturing_;\n> > +\tif (isCapturing_)\n> > +\t\ttoggleCapture(false);\n> > +\n> > +\tscript_ = std::move(script);\n> > +\n> > +\t/* Start capture again if we were capturing before. */\n> > +\tif (wasCapturing)\n> > +\t\ttoggleCapture(true);\n> > +\treturn 0;\n> > +}\n> > +\n> > std::string MainWindow::chooseCamera()\n> > {\n> > \tif (cameraSelectorDialog_->exec() != QDialog::Accepted)\n> > \t\treturn std::string();\n> > \n> > +\tscriptPath_ = cameraSelectorDialog_->getCaptureScript();\n> > +\n> > \treturn cameraSelectorDialog_->getCameraId();\n> \n> I don't like much how the capture script is stored in a member variable\n> but the camera ID is returned. This isn't consistent, you should treat\n> both the same way (returning an std::tuple is an option if you want to\n> return them).\n> \n> > }\n> > \n> > @@ -499,6 +556,7 @@ int MainWindow::startCapture()\n> > \tpreviousFrames_ = 0;\n> > \tframesCaptured_ = 0;\n> > \tlastBufferTime_ = 0;\n> > +\tqueueCount_ = 0;\n> > \n> > \tret = camera_->start();\n> > \tif (ret) {\n> > @@ -777,5 +835,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer)\n> > \n> > int MainWindow::queueRequest(Request *request)\n> > {\n> > +\tif (script_)\n> > +\t\trequest->controls() = script_->frameControls(queueCount_);\n> > +\n> > +\tqueueCount_++;\n> > +\n> > \treturn camera_->queueRequest(request);\n> > }\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index 22c85247..7c877ae1 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -27,6 +27,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"../cam/capture_script.h\"\n> > #include \"../cam/stream_options.h\"\n> > \n> > #include \"viewfinder.h\"\n> > @@ -89,6 +90,9 @@ private:\n> > \tvoid processHotplug(HotplugEvent *e);\n> > \tvoid processViewfinder(libcamera::FrameBuffer *buffer);\n> > \n> > +\tint loadCaptureScript();\n> > +\tvoid stopCaptureScript();\n> > +\n> > \t/* UI elements */\n> > \tQToolBar *toolbar_;\n> > \tQAction *startStopAction_;\n> > @@ -129,6 +133,9 @@ private:\n> > \tQElapsedTimer frameRateInterval_;\n> > \tuint32_t previousFrames_;\n> > \tuint32_t framesCaptured_;\n> > +\tuint32_t queueCount_;\n> > \n> > \tstd::vector> requests_;\n> > +\tstd::unique_ptr script_;\n> > +\tstd::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> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","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 AA268C3272\n\tfor ;\n\tThu, 1 Sep 2022 06:05:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22B9361FCD;\n\tThu, 1 Sep 2022 08:05:38 +0200 (CEST)","from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com\n\t[IPv6:2607:f8b0:4864:20::1030])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 52B3561F9A\n\tfor ;\n\tThu, 1 Sep 2022 08:05:36 +0200 (CEST)","by mail-pj1-x1030.google.com with SMTP id\n\ti5-20020a17090a2a0500b001fd8708ffdfso1450049pjd.2\n\tfor ;\n\tWed, 31 Aug 2022 23:05:36 -0700 (PDT)","from gmail.com ([2404:bd00:3:de55:7693:8bef:3577:357a])\n\tby smtp.gmail.com with ESMTPSA id\n\t7-20020a621807000000b0052a297324cbsm12303105pfy.41.2022.08.31.23.05.02\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 31 Aug 2022 23:05:14 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662012338;\n\tbh=aLpPTCpPEL9RlToZiCsRRlMcTWIPQYJkvg18JpeqAcQ=;\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=XK1Y5r7UM3va9I85QKLntyJ801SexTa4w/obk8v3mstpgmPrdUwsCBzHwUC4Um4zq\n\tIRe6AEv4x5KHct+wRZuduRZLHSsXp/TGdhTe0fFK7EallDPEH+v8U8DLtD9pDcgI9x\n\tm5w73wSrdO2FuyHVm9G2kYzgj9iD+F3Up8wZcfs/2H2k5NRN8IGyS0RiKYIxWIZ45C\n\tRdwuR1T9AYsFqNs0cDawXPtL99M7zciCLhe/Ownu1oIB4AI1ruKtg3s+6xPEzTUNjp\n\tmzrWt9yBAA1oP7ekGkLvkYvRPIkoDVaFnUEXdYUlJR57jEuSy7rAs7darrKuwyS6W9\n\tdNEYtkJpI/sxw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=in-reply-to:content-disposition:mime-version:references\n\t:mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc\n\t:subject:date; bh=dC4fiaUbXiNdezjXEYp8Bp9Dg7ckTC/Ad6tHAieF5us=;\n\tb=cQfY3/9VM6dZ5kxJuechgW8KeQQx3yzLoiN5wK5g5sxyXBKker5jqWCq2U2DVtb3N/\n\trBMMOnrDLx6eqS1fypciR73O+91PHu/mIkd1wDwxRB2JX16H7iS6xSb9RIzua3wFaxfZ\n\t565v1ILWpd8LV2Tz+OqfqN/PLZ7JYs0w/gy0OmGlZaJos3DGB2n/rMsQaOjjJsoshf2H\n\tJi9uGK0jt1yDYCuLW9SkwEabkbTBX23WmYwpWbQMaJ8y9P1SVP033lgTq/Wt8ZR097y4\n\tZAY5X8yMvOwTdEyZVd2etXpcpwYGr4W/Y0kbmpERNIAqNsWABLOkrI7hGjo34RM1k36a\n\tPxZw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"cQfY3/9V\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=in-reply-to:content-disposition:mime-version:references\n\t:mail-followup-to:message-id:subject:cc:to:from:date\n\t:x-gm-message-state:from:to:cc:subject:date;\n\tbh=dC4fiaUbXiNdezjXEYp8Bp9Dg7ckTC/Ad6tHAieF5us=;\n\tb=rGsteNK8e6xNxHthgyBSgVZCG7rR6pp9usBHoBh6mir2Gql7piboxzWjtiUbk7ZyOc\n\tgFmLu3Mc0EsTPVxY9AdYrilxzW1/HUxsl/3mJ39dvbewyem9QVmBOLytSdOUi473rCoh\n\t2T3rtEpYOGyBHcsRyqu4ai/EpmpWtvP+/zMeu58aiMLkBWxLUVNpidxZC2BrBxNoAvYo\n\tP4xdIkcCXwjqK/zydxOUnIAp9U6lH1QSBd0dBqNV8IxDRxo+VYl6zlsdI9sF4CFyN5De\n\twqcM04BH/l5buML6EW28Wijq/ftrQlxFQquJJlrAB30hKnDZpgbTb4NCwhj70chDBp2x\n\tYkzQ==","X-Gm-Message-State":"ACgBeo3pGbb0MRvoB39TIKy4QDhxQk72a7GkQ6hKxD1h2WdB45N9dqjw\n\tD/JabTPdsey3zZJWfr2G5uJ4gkWw4xk=","X-Google-Smtp-Source":"AA6agR5ivFEMMFANnu1K8pyc7yEFfaFzESlg1LF+X/gM3e6zoRA2s48mB1jrPrrG+6SQTX/HTUqFFw==","X-Received":"by 2002:a17:90a:e586:b0:1fa:d28b:ab9b with SMTP id\n\tg6-20020a17090ae58600b001fad28bab9bmr7125471pjz.47.1662012334531; \n\tWed, 31 Aug 2022 23:05:34 -0700 (PDT)","Date":"Thu, 1 Sep 2022 11:34:59 +0530","To":"Laurent Pinchart ","Message-ID":"<20220901060459.cy3pruqh4ldnhixc@gmail.com>","Mail-Followup-To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220831054938.21617-1-utkarsh02t@gmail.com>\n\t<20220831054938.21617-7-utkarsh02t@gmail.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add\n\tcapture script button","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Utkarsh Tiwari via libcamera-devel\n\t","Reply-To":"Utkarsh Tiwari ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24882,"web_url":"https://patchwork.libcamera.org/comment/24882/","msgid":"","date":"2022-09-01T10:08:29","subject":"Re: [libcamera-devel] [PATCH v9 6/7] 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":"Hi Utkarsh,\n\nOn Thu, Sep 01, 2022 at 11:34:59AM +0530, Utkarsh Tiwari wrote:\n> On Wed, Aug 31, 2022 at 12:59:12PM +0300, Laurent Pinchart wrote:\n> > On Wed, Aug 31, 2022 at 11:19:37AM +0530, Utkarsh Tiwari via libcamera-devel wrote:\n> > > Display a QLabel which is readonly, and displays the currently\n> > \n> > Labels are always read-only :-)\n> \n> :P would fix.\n> \n> > > selected capture script. The tooltip of the QLabel displays the file\n> > > path of the script.\n> > > \n> > > Implement a capture script button which is a QToolButton which when\n> > > clicked opens a QFileDialog this allows to select a capture script\n> > > (*.yaml) file.\n> > > \n> > > Next to the capture scipt button, show a QToolButton which stops the\n> > > capture script.\n> > > \n> > > If an invalid script has been selected show a QMesssageBox::critical and\n> > > continue with the capture's previous state.\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> > Missing SoB line.\n> \n> Oops would add that.\n> \n> > > ---\n> > > Differnce from v8:\n> > > 1. Now display a QLabel with the fileName and filePath with button\n> > > on the side.\n> > > 2. infromScriptReset() informScriptRunning() are removed\n> > > 3. Local script makes handling of invalid scripts easy.\n> > > src/qcam/assets/feathericons/feathericons.qrc | 2 +\n> > > src/qcam/cam_select_dialog.cpp | 88 ++++++++++++++++++-\n> > > src/qcam/cam_select_dialog.h | 20 ++++-\n> > > src/qcam/main_window.cpp | 67 +++++++++++++-\n> > > src/qcam/main_window.h | 7 ++\n> > > src/qcam/meson.build | 2 +\n> > > 6 files changed, 181 insertions(+), 5 deletions(-)\n> > > \n> > > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc\n> > > index c5302040..0ea0c2d5 100644\n> > > --- a/src/qcam/assets/feathericons/feathericons.qrc\n> > > +++ b/src/qcam/assets/feathericons/feathericons.qrc\n> > > @@ -3,7 +3,9 @@\n> > > \n> > > \taperture.svg\n> > > \tcamera-off.svg\n> > > + delete.svg\n> > \n> > Please indent with tabs, not spaces.\n> \n> Gotta learn to use in vim.\n> \n> > > \tplay-circle.svg\n> > > + upload.svg\n> > \n> > Same here.\n> > \n> > > \tsave.svg\n> > > \tstop-circle.svg\n> > > \tx-circle.svg\n> > > diff --git a/src/qcam/cam_select_dialog.cpp b/src/qcam/cam_select_dialog.cpp\n> > > index 6543228a..99405cc1 100644\n> > > --- a/src/qcam/cam_select_dialog.cpp\n> > > +++ b/src/qcam/cam_select_dialog.cpp\n> > > @@ -14,13 +14,18 @@\n> > > \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > +#include \n> > > +#include \n> > > #include \n> > > #include \n> > > +#include \n> > \n> > Why not a QPushButton ? QToolButton is usually meant for toolbars.\n> \n> QToolButton works much better in cases when we require only the icon.\n\nDoes it ? I've tried replacing QToolButton with QPushButton and didn't\nnotice any difference in the dialog box.\n\n> > > +#include \n> > > \n> > > CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > > -\t\t\t\t\t QWidget *parent)\n> > > -\t: QDialog(parent), cm_(cameraManager)\n> > > +\t\t\t\t\t std::string scriptPath, QWidget *parent)\n> > \n> > Make it a const std::string & to avoid copies.\n> \n> Sure.\n> \n> > > +\t: QDialog(parent), cm_(cameraManager), scriptPath_(scriptPath)\n> > > {\n> > > \t/* Use a QFormLayout for the dialog. */\n> > > \tQFormLayout *layout = new QFormLayout(this);\n> > > @@ -38,6 +43,41 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> > > \tconnect(cameraIdComboBox_, &QComboBox::currentTextChanged,\n> > > \t\tthis, &CameraSelectorDialog::updateCamInfo);\n> > > \n> > > +\t/* Set capture script selection / removal button. */\n> > > +\tQWidget *captureWidget = new QWidget(this);\n> > \n> > Maybe captureScriptWidhet ?\n> \n> Sure, maybe not in a Mike Tyson accept tho :P\n\n:-)\n\n> > > +\tQHBoxLayout *captureLayout = new QHBoxLayout(captureWidget);\n> > \n> > Ditto.\n> \n> Noted.\n> \n> > > +\n> > > +\tscriptFileLine_ = new QLabel;\n> > > +\tscriptFileLine_->setFrameStyle(QFrame::Sunken | QFrame::StyledPanel);\n> > \n> > Generally speaking, changing styles manually should be avoided as much\n> > as possible. It makes the overall style inconsistent and is confusing\n> > for users. I'd drop this.\n> \n> I think we the frame provides a border in which the user knows placing\n> the pointer would result in the tooltip showing up and displaying the\n> path. Provides more visual feedback.\n\nMaybe QLabel isn't the right widget then. What I dislike is manual\nstyling, as it breaks user assumptions.\n\n> > > +\n> > > +\tchooseCaptureScriptButton_ = new QToolButton;\n> > > +\tchooseCaptureScriptButton_->setIcon(QIcon::fromTheme(\"document-open\",\n> > > +\t\t\t\t\t\t\t QIcon(\":upload.svg\")));\n> > > +\tchooseCaptureScriptButton_->setStyleSheet(\"border:none\");\n> > \n> > Same here, drop this. A button that doesn't look like a button gives no\n> > visual clue that it can be clicked.\n> \n> Ah, I think went overboard making it icon only, would do.\n> \n> > > +\tconnect(chooseCaptureScriptButton_, &QToolButton::clicked,\n> > > +\t\tthis, &CameraSelectorDialog::selectCaptureScript);\n> > > +\n> > > +\tQToolButton *stopCaptureScriptButton = new QToolButton;\n> > > +\tstopCaptureScriptButton->setIcon(QIcon::fromTheme(\"edit-clear\",\n> > > +\t\t\t\t\t\t\t QIcon(\":delete.svg\")));\n> > > +\tstopCaptureScriptButton->setStyleSheet(\"border:node;\");\n> \n> Same here.\n> \n> > > +\tconnect(stopCaptureScriptButton, &QToolButton::clicked,\n> > > +\t\tthis, &CameraSelectorDialog::resetCaptureScript);\n> > > +\n> > > +\tcaptureLayout->addWidget(scriptFileLine_);\n> > > +\tcaptureLayout->addWidget(chooseCaptureScriptButton_);\n> > > +\tcaptureLayout->addWidget(stopCaptureScriptButton);\n> > > +\tcaptureLayout->setMargin(0);\n> > > +\n> > > +\t/* Set the file name of the capture script. */\n> > > +\tif (scriptPath_.empty()) {\n> > > +\t\tscriptFileLine_->setText(\"No File Selected\");\n> > > +\t} else {\n> > > +\t\tscriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> > > +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> > > +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > > +\t}\n> > > +\n> > > \t/* Setup the QDialogButton Box */\n> > > \tQDialogButtonBox *buttonBox =\n> > > \t\tnew QDialogButtonBox(QDialogButtonBox::Ok |\n> > > @@ -52,6 +92,7 @@ CameraSelectorDialog::CameraSelectorDialog(libcamera::CameraManager *cameraManag\n> > > \tlayout->addRow(\"Camera:\", cameraIdComboBox_);\n> > > \tlayout->addRow(\"Location:\", cameraLocation_);\n> > > \tlayout->addRow(\"Model:\", cameraModel_);\n> > > +\tlayout->addRow(\"Capture Script:\", captureWidget);\n> > > \tlayout->addWidget(buttonBox);\n> > > }\n> > > \n> > > @@ -110,3 +151,46 @@ void CameraSelectorDialog::updateCamInfo(QString cameraId)\n> > > \n> > > \tcameraModel_->setText(QString::fromStdString(model));\n> > > }\n> > > +\n> > > +/* Capture script support. */\n> > > +void CameraSelectorDialog::selectCaptureScript()\n> > > +{\n> > > +\tselectedScriptPath_ = QFileDialog::getOpenFileName(this,\n> > > +\t\t\t\t\t\t\t \"Run Capture Script\", QDir::currentPath(),\n> > > +\t\t\t\t\t\t\t \"Capture Script (*.yaml)\")\n> > > +\t\t\t\t .toStdString();\n> > > +\n> > > +\tif (!selectedScriptPath_.empty()) {\n> > > +\t\tscriptFileInfo_.setFile(QString::fromStdString(selectedScriptPath_));\n> > \n> > You can make this a local variable:\n> > \n> > \t\tQFileInfo fileInfo(QString::fromStdString(selectedScriptPath_));\n> > \n> > Don't forget to move the #include from the header to this\n> > file.\n> > \n> > Furthermore, converting the QString to std::string to then convert it\n> > back to QString here is not efficient. I would use QString everywhere in\n> > this class and convert to std::string in the caller (MainWindow).\n> \n> I agree, we only take scriptPath in std::string, we can convert that and\n> use QString everywhere here.\n> \n> > > +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> > > +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > > +\t} else {\n> > > +\t\tselectedScriptPath_ = scriptPath_;\n> > > +\t}\n> > \n> > This while logic looks weird, selectedScriptPath_ is modified when the\n> > file dialog is rejected, to then be restored to the previous value.\n> > Store the return value of getOpenFileName() in a local QString\n> > scriptPath variable first, with then\n> > \n> > \tif (scriptPath.isNull))\n> > \t\treturn;\n> > \n> > \tQFileInfo fileInfo(scriptPath);\n> > \tscriptFileLine_->setText(fileInfo.fileName());\n> > \tscriptFileLine_->setToolTip(fileInfo.filePath());\n> > \n> > \tselectedScriptPath_ = scriptPath.toStdString();\n> \n> Oops, makes much easier would use.\n> \n> > > +}\n> > > +\n> > > +void CameraSelectorDialog::resetCaptureScript()\n> > > +{\n> > > +\tQ_EMIT stopCaptureScript();\n> > > +\tscriptPath_.clear();\n> > > +\tselectedScriptPath_.clear();\n> > > +\tscriptFileLine_->setText(\"No File Selected\");\n> > > +}\n> > > +\n> > > +void CameraSelectorDialog::accept()\n> > > +{\n> > > +\tscriptPath_ = selectedScriptPath_;\n> > > +\tQDialog::accept();\n> > > +}\n> > > +\n> > > +void CameraSelectorDialog::reject()\n> > > +{\n> > > +\tif (scriptPath_.empty()) {\n> > > +\t\tscriptFileLine_->setText(\"No File Selected\");\n> > > +\t} else {\n> > > +\t\tscriptFileInfo_.setFile(QString::fromStdString(scriptPath_));\n> > > +\t\tscriptFileLine_->setText(scriptFileInfo_.fileName());\n> > > +\t\tscriptFileLine_->setToolTip(scriptFileInfo_.filePath());\n> > > +\t}\n> > \n> > No need for this. When the camera selector dialog is rejected it can\n> > still remember which capture script has been selected. You can thus drop\n> > the accept() and reject() overrides, and merge the two scriptPath_ and\n> > selectedScriptPath_ variables into a single one.\n> \n> I'm a bit doubtful on this,\n> 1. I start a script.\n> 2. Open the dialog select some different script.\n> 3. Reject the dialog.\n> 4. Few minutes pass\n> 5. I open the CameraSelectorDialog I think I would expect the dialog\n> to show me the current script which is running.\n\nIf you reject the dialog you won't have selected a camera, so you won't\nstart capture, right ? There's thus no script running.\n\n> In my opinion after doing any action, reject or accept the label should\n> show what has been already passed to loadCaptureScript().\n> Open to more suggetions.\n> \n> > > +\tQDialog::reject();\n> > > +}\n> > > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h\n> > > index c91b7ebe..377faebc 100644\n> > > --- a/src/qcam/cam_select_dialog.h\n> > > +++ b/src/qcam/cam_select_dialog.h\n> > > @@ -15,21 +15,24 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > class QComboBox;\n> > > class QLabel;\n> > > +class QToolButton;\n> > > \n> > > class CameraSelectorDialog : public QDialog\n> > > {\n> > > \tQ_OBJECT\n> > > public:\n> > > \tCameraSelectorDialog(libcamera::CameraManager *cameraManager,\n> > > -\t\t\t QWidget *parent);\n> > > +\t\t\t std::string scriptPath, QWidget *parent);\n> > > \n> > > \t~CameraSelectorDialog();\n> > > \n> > > \tstd::string getCameraId();\n> > > +\tstd::string getCaptureScript() { return scriptPath_; };\n> > > \n> > > \t/* Hotplug / Unplug Support. */\n> > > \tvoid addCamera(QString cameraId);\n> > > @@ -38,11 +41,26 @@ public:\n> > > \t/* Camera Information */\n> > > \tvoid updateCamInfo(QString cameraId);\n> > > \n> > > +\t/* Capture script support. */\n> > > +\tvoid selectCaptureScript();\n> > > +\tvoid resetCaptureScript();\n> > \n> > These should be private Q_SLOTS.\n> > \n> > > +\n> > > +\tvoid accept() override;\n> > > +\tvoid reject() override;\n> > > +\n> > > +Q_SIGNALS:\n> > > +\tvoid stopCaptureScript();\n> > \n> > Drop this signal, the capture script should be retrieved from the dialog\n> > by the MainWindow class when the dialog is accepted, there's no need for\n> > dynamic notification.\n> > \n> > > +\n> > > private:\n> > > \tlibcamera::CameraManager *cm_;\n> > > \n> > > +\tstd::string scriptPath_;\n> > > +\tstd::string selectedScriptPath_;\n> > > +\tQFileInfo scriptFileInfo_;\n> > \n> > Add a blank line here.\n> > \n> > > \t/* UI elements. */\n> > > \tQComboBox *cameraIdComboBox_;\n> > > \tQLabel *cameraLocation_;\n> > > \tQLabel *cameraModel_;\n> > > +\tQLabel *scriptFileLine_;\n> > > +\tQToolButton *chooseCaptureScriptButton_;\n> > > };\n> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > index 2a9ca830..af992b94 100644\n> > > --- a/src/qcam/main_window.cpp\n> > > +++ b/src/qcam/main_window.cpp\n> > > @@ -9,6 +9,7 @@\n> > > \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \n> > > @@ -18,6 +19,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -143,7 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > > \tcm_->cameraAdded.connect(this, &MainWindow::addCamera);\n> > > \tcm_->cameraRemoved.connect(this, &MainWindow::removeCamera);\n> > > \n> > > -\tcameraSelectorDialog_ = new CameraSelectorDialog(cm_, this);\n> > > +\tcameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);\n> > > +\tconnect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,\n> > > +\t\tthis, &MainWindow::stopCaptureScript);\n> > > \n> > > \t/* Open the camera and start capture. */\n> > > \tret = openCamera();\n> > > @@ -152,6 +156,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> > > \t\treturn;\n> > > \t}\n> > > \n> > > +\t/* Start capture script. */\n> > > +\tif (!scriptPath_.empty())\n> > > +\t\tret = loadCaptureScript();\n> > \n> > ret isn't used.\n> > \n> > > +\n> > > \tstartStopAction_->setChecked(true);\n> > > }\n> > > \n> > > @@ -266,8 +274,11 @@ void MainWindow::switchCamera()\n> > > \tif (newCameraId.empty())\n> > > \t\treturn;\n> > > \n> > > -\tif (camera_ && newCameraId == camera_->id())\n> > > +\tif (camera_ && newCameraId == camera_->id()) {\n> > > +\t\t// When user opens camera selection dialog for CaptureScript selection\n> > \n> > C-style comments and line wrap.\n> > \n> > > +\t\tloadCaptureScript();\n> > > \t\treturn;\n> > > +\t}\n> > > \n> > > \tconst std::shared_ptr &cam = cm_->get(newCameraId);\n> > > \n> > > @@ -287,17 +298,63 @@ void MainWindow::switchCamera()\n> > > \tcamera_->release();\n> > > \tcamera_ = cam;\n> > > \n> > > +\tloadCaptureScript();\n> > > +\n> > > \tstartStopAction_->setChecked(true);\n> > > \n> > > \t/* Display the current cameraId in the toolbar .*/\n> > > \tcameraSelectButton_->setText(QString::fromStdString(newCameraId));\n> > > }\n> > > \n> > > +void MainWindow::stopCaptureScript()\n> > > +{\n> > > +\tif (script_)\n> > > +\t\tscript_.reset();\n> > > +}\n> > \n> > Blank line.\n> > \n> > > +/**\n> > > + * \\brief Loads and validates the current capture script\n> > > + *\n> > > + * returns -EINVAL on failure and 0 on success\n> > > + */\n> > \n> > We don't want to generate documentation from this file, so use /*\n> > instead of /** and drop the \\brief:\n> > \n> > /*\n> > * Load and validate the current capture script. Return -EINVAL on failure and 0\n> > * on success.\n> > */\n> > \n> > > +int MainWindow::loadCaptureScript()\n> > > +{\n> > > +\tif (scriptPath_.empty() || camera_ == nullptr)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tauto script = std::make_unique(camera_, scriptPath_);\n> > > +\n> > > +\tif (!script->valid()) {\n> > > +\t\tscript.reset();\n> > > +\n> > > +\t\tQMessageBox::critical(this, \"Invalid Script\",\n> > > +\t\t\t\t \"Couldn't load the capture script\");\n> > > +\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * If we are already capturing, stop so we don't have stuck image\n> > > +\t * in viewfinder.\n> > > +\t */\n> > > +\tbool wasCapturing = isCapturing_;\n> > > +\tif (isCapturing_)\n> > > +\t\ttoggleCapture(false);\n> > > +\n> > > +\tscript_ = std::move(script);\n> > > +\n> > > +\t/* Start capture again if we were capturing before. */\n> > > +\tif (wasCapturing)\n> > > +\t\ttoggleCapture(true);\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > std::string MainWindow::chooseCamera()\n> > > {\n> > > \tif (cameraSelectorDialog_->exec() != QDialog::Accepted)\n> > > \t\treturn std::string();\n> > > \n> > > +\tscriptPath_ = cameraSelectorDialog_->getCaptureScript();\n> > > +\n> > > \treturn cameraSelectorDialog_->getCameraId();\n> > \n> > I don't like much how the capture script is stored in a member variable\n> > but the camera ID is returned. This isn't consistent, you should treat\n> > both the same way (returning an std::tuple is an option if you want to\n> > return them).\n> > \n> > > }\n> > > \n> > > @@ -499,6 +556,7 @@ int MainWindow::startCapture()\n> > > \tpreviousFrames_ = 0;\n> > > \tframesCaptured_ = 0;\n> > > \tlastBufferTime_ = 0;\n> > > +\tqueueCount_ = 0;\n> > > \n> > > \tret = camera_->start();\n> > > \tif (ret) {\n> > > @@ -777,5 +835,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer)\n> > > \n> > > int MainWindow::queueRequest(Request *request)\n> > > {\n> > > +\tif (script_)\n> > > +\t\trequest->controls() = script_->frameControls(queueCount_);\n> > > +\n> > > +\tqueueCount_++;\n> > > +\n> > > \treturn camera_->queueRequest(request);\n> > > }\n> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > > index 22c85247..7c877ae1 100644\n> > > --- a/src/qcam/main_window.h\n> > > +++ b/src/qcam/main_window.h\n> > > @@ -27,6 +27,7 @@\n> > > #include \n> > > #include \n> > > \n> > > +#include \"../cam/capture_script.h\"\n> > > #include \"../cam/stream_options.h\"\n> > > \n> > > #include \"viewfinder.h\"\n> > > @@ -89,6 +90,9 @@ private:\n> > > \tvoid processHotplug(HotplugEvent *e);\n> > > \tvoid processViewfinder(libcamera::FrameBuffer *buffer);\n> > > \n> > > +\tint loadCaptureScript();\n> > > +\tvoid stopCaptureScript();\n> > > +\n> > > \t/* UI elements */\n> > > \tQToolBar *toolbar_;\n> > > \tQAction *startStopAction_;\n> > > @@ -129,6 +133,9 @@ private:\n> > > \tQElapsedTimer frameRateInterval_;\n> > > \tuint32_t previousFrames_;\n> > > \tuint32_t framesCaptured_;\n> > > +\tuint32_t queueCount_;\n> > > \n> > > \tstd::vector> requests_;\n> > > +\tstd::unique_ptr script_;\n> > > +\tstd::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":"","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 165CCC0DA4\n\tfor ;\n\tThu, 1 Sep 2022 10:08:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71D2361FCE;\n\tThu, 1 Sep 2022 12:08:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2092161FB9\n\tfor ;\n\tThu, 1 Sep 2022 12:08:42 +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 5EEBF6CD;\n\tThu, 1 Sep 2022 12:08:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662026923;\n\tbh=TQuflxatm3bARrC5oNWRm2LtpIPH4/4vHHpa9fTwKDA=;\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=HkEcu/NEDFPf0rzZ3zZAXVZbmLWBaaDZD3wxsJlImqQck45qYP/NvVKEpi5z6fM48\n\tJANbWKeU9eMueTfVkG11JImO2VXrB8CfDJfNQjZ2BD0MApOXGSIJbmaX0W2/C+M1Vb\n\t6Nu1JEloywuf18eOeqrGyIpzIvXWvmYupXfv/ivjpWHywpLm+/XgKXCqCCTx/+mrQg\n\trnlmwAk3q5grQN+Ck3GmaP6ONqkSDKXv3fJez6iVuRd/tfJqYBy/99xDBzV6djWrYN\n\tzM6v5A7Iu35oL4xiV9tkoZYWfc55iORlOR3LqO+iKXvzwREf5tGIM+zYYBDWzSeGXg\n\tl1yXih/3KWeYg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662026921;\n\tbh=TQuflxatm3bARrC5oNWRm2LtpIPH4/4vHHpa9fTwKDA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JErf3AAwgl2+RqZfobBH+dONtSw8qm2h66mR6DEss7uhef3RDm9m8IEFrJuRsDNuR\n\tok0ukAN5xa2pahbMjlb+Yr7jdggrL1/edycYjmz46jrnLiBGtp7pE79ye38vda2lz6\n\tSUNonqVXhuxH3gehrLgvnUzQF6IQVb9mbXMzjr8A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JErf3AAw\"; dkim-atps=neutral","Date":"Thu, 1 Sep 2022 13:08:29 +0300","To":"Utkarsh Tiwari ","Message-ID":"","References":"<20220831054938.21617-1-utkarsh02t@gmail.com>\n\t<20220831054938.21617-7-utkarsh02t@gmail.com>\n\t\n\t<20220901060459.cy3pruqh4ldnhixc@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220901060459.cy3pruqh4ldnhixc@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v9 6/7] qcam: CamSelectDialog: Add\n\tcapture script button","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]