| Message ID | 20220726194123.170208-2-utkarsh02t@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Utkarsh, Thank you for the patch. On Wed, Jul 27, 2022 at 01:11:22AM +0530, Utkarsh Tiwari via libcamera-devel wrote: > Implement an Open Capture Script button which would allow the user > to open a Capture Script (*.yaml). > This button has two states : > - Open Capture Script > - Stop the execution of current capture script > > When being clicked in open state, present them with a QFileDialog to > allow user to select a single file. > > Introduce a queueCount_ to keep track of the requests queued. > > When stopping the execution of the capture script the queueCount_ is not > reseted and the capture is continues as it is (i.e it is not stopped or > restarted). > > Requests are queued with any controls the script matching the current > queueCount_. > > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Changes from v4 > - Reworded the commit message to clarify button states and > queueCount_ reseting. > > src/qcam/assets/feathericons/feathericons.qrc | 2 + > src/qcam/main_window.cpp | 68 +++++++++++++++++++ > src/qcam/main_window.h | 6 ++ > src/qcam/meson.build | 2 + > 4 files changed, 78 insertions(+) > > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc > index c5302040..6b08395a 100644 > --- a/src/qcam/assets/feathericons/feathericons.qrc > +++ b/src/qcam/assets/feathericons/feathericons.qrc > @@ -3,9 +3,11 @@ > <qresource> > <file>aperture.svg</file> > <file>camera-off.svg</file> > + <file>file.svg</file> I'm a bit annoyed by the fact that this icon is very generic. I looked through the feathericons directory and nothing really strikes me as being a perfect match (it would be too easy). https://feathericons.com/ has a few new icons, but also nothing that really stands out. Here are a few options you may want to consider: - activity: Hints at something that evolves over time. - command: A capture script is somehow a list of commands. Maybe too Apple-related ? - list: The other term in a "list of commands". - settings: The script contains settings, but it may be best to keep this icon for a future settings dialog. - sliders: Same here. - zap: Not sure what the link would be, but there's a zap-off icon that is handy to replace x-square.svg > <file>play-circle.svg</file> > <file>save.svg</file> > <file>stop-circle.svg</file> > <file>x-circle.svg</file> > + <file>x-square.svg</file> As hinted by the zap proposal above, I think the "on" and "off" states of the button should have related icons. Another option is to keep the same icon, and just update the button state, the same way we handle the "Start Capture" button. The feather icons are only fallbacks, we also have to select standard icons from the theme. The latest (but old) version of the icon naming specification is available at https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html. There we have even less options, I've only seen the following ones as possibly relevant: system-run media-playlist-repeat media-playlist-shuffle sync-synchronizing Although there's also text-x-script that may be interesting. Another option would be to rethink the UI and add capture script support somewhere else than in the toolbar. > </qresource> > </RCC> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 4e773c31..9dc96fbb 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -20,6 +20,7 @@ > #include <QImage> > #include <QImageWriter> > #include <QInputDialog> > +#include <QMessageBox> > #include <QMutexLocker> > #include <QStandardPaths> > #include <QStringList> > @@ -233,6 +234,13 @@ int MainWindow::createToolbars() > saveRaw_ = action; > #endif > > + /* Open Script... action. */ > + action = toolbar_->addAction(QIcon::fromTheme("document-open", > + QIcon(":file.svg")), > + "Open Capture Script"); I'd name this "Run Capture Script". The action both opens and runs the script, but the latter seems to be the most important part. > + connect(action, &QAction::triggered, this, &MainWindow::chooseScript); > + scriptExecAction_ = action; > + > return 0; > } > > @@ -256,6 +264,60 @@ void MainWindow::updateTitle() > setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps"); > } > > +/** > + * \brief Load a capture script for handling the capture session. > + * > + * If already capturing, it would restart the capture. Why is that ? Could we apply the capture script from the current point without restarting the camera ? > + */ > +void MainWindow::chooseScript() > +{ > + if (script_) { > + /* > + * This is the second valid press of load script button, > + * It indicates stopping, Stop and set button for new script. Either ", it" or ". It", and ", stop" or ". Stop". > + */ > + script_.reset(); > + scriptExecAction_->setIcon(QIcon::fromTheme("document-open", > + QIcon(":file.svg"))); > + scriptExecAction_->setText("Open Capture Script"); > + return; > + } > + > + QString scriptFile = QFileDialog::getOpenFileName(this, "Open Capture Script", QDir::currentPath(), > + "Capture Script (*.yaml)"); > + if (scriptFile.isEmpty()) > + return; > + > + /* > + * If we are already capturing, > + * stop so we don't have stuck image in viewfinder. * If we are already capturing, stop so we don't have stuck image in * viewfinder. > + */ > + bool wasCapturing = isCapturing_; > + if (isCapturing_) > + toggleCapture(false); > + > + script_ = std::make_unique<CaptureScript>(camera_, scriptFile.toStdString()); > + if (!script_->valid()) { > + script_.reset(); > + QMessageBox::critical(this, "Invalid Script", > + "Couldn't load the capture script"); > + if (wasCapturing) > + toggleCapture(true); > + return; > + } > + > + /* > + * Valid script verified > + * Set the button to indicate stopping availibility. > + */ Generally speaking, you shouldn't have line wraps at the end of a sentence in the middle of a paragraph (other than the ones required to limit the line length to 80 columns): /* * Valid script verified. Set the button to indicate stopping * availibility. */ If you want to separate the two sentences in two paragraphs, you need a blank line: /* * Valid script verified. * * Set the button to indicate stopping availibility. */ I don't think that's what you want here though. > + scriptExecAction_->setIcon(QIcon(":x-square.svg")); > + scriptExecAction_->setText("Stop Script execution"); > + > + /* Start capture again if we were capturing before. */ > + if (wasCapturing) > + toggleCapture(true); > +} > + > /* ----------------------------------------------------------------------------- > * Camera Selection > */ > @@ -511,6 +573,7 @@ int MainWindow::startCapture() > previousFrames_ = 0; > framesCaptured_ = 0; > lastBufferTime_ = 0; > + queueCount_ = 0; > > ret = camera_->start(); > if (ret) { > @@ -790,5 +853,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer) > > int MainWindow::queueRequest(Request *request) > { > + if (script_) > + request->controls() = script_->frameControls(queueCount_); > + > + queueCount_++; > + > return camera_->queueRequest(request); > } > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index bc844711..eb398c1d 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -26,6 +26,7 @@ > #include <QQueue> > #include <QTimer> > > +#include "../cam/capture_script.h" > #include "../cam/stream_options.h" > > #include "viewfinder.h" > @@ -87,11 +88,14 @@ private: > void processHotplug(HotplugEvent *e); > void processViewfinder(libcamera::FrameBuffer *buffer); > > + void chooseScript(); > + > /* UI elements */ > QToolBar *toolbar_; > QAction *startStopAction_; > QComboBox *cameraCombo_; > QAction *saveRaw_; > + QAction *scriptExecAction_; > ViewFinder *viewfinder_; > > QIcon iconPlay_; > @@ -125,6 +129,8 @@ private: > QElapsedTimer frameRateInterval_; > uint32_t previousFrames_; > uint32_t framesCaptured_; > + uint32_t queueCount_; > > std::vector<std::unique_ptr<libcamera::Request>> requests_; > + std::unique_ptr<CaptureScript> script_; > }; > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > index c46f4631..67074252 100644 > --- a/src/qcam/meson.build > +++ b/src/qcam/meson.build > @@ -15,6 +15,7 @@ endif > qcam_enabled = true > > qcam_sources = files([ > + '../cam/capture_script.cpp', > '../cam/image.cpp', > '../cam/options.cpp', > '../cam/stream_options.cpp', At some point we should probably create an application helpers library shared between cam and qcam. Out of scope for this series of course. > @@ -37,6 +38,7 @@ qcam_resources = files([ > qcam_deps = [ > libatomic, > libcamera_public, > + libyaml, > qt5_dep, > ] >
Hi Laurent, thanks for the review. On Wed, Jul 27, 2022 at 01:13:08AM +0300, Laurent Pinchart wrote: > Hi Utkarsh, > > Thank you for the patch. > > On Wed, Jul 27, 2022 at 01:11:22AM +0530, Utkarsh Tiwari via libcamera-devel wrote: > > Implement an Open Capture Script button which would allow the user > > to open a Capture Script (*.yaml). > > This button has two states : > > - Open Capture Script > > - Stop the execution of current capture script > > > > When being clicked in open state, present them with a QFileDialog to > > allow user to select a single file. > > > > Introduce a queueCount_ to keep track of the requests queued. > > > > When stopping the execution of the capture script the queueCount_ is not > > reseted and the capture is continues as it is (i.e it is not stopped or > > restarted). > > > > Requests are queued with any controls the script matching the current > > queueCount_. > > > > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > Changes from v4 > > - Reworded the commit message to clarify button states and > > queueCount_ reseting. > > > > src/qcam/assets/feathericons/feathericons.qrc | 2 + > > src/qcam/main_window.cpp | 68 +++++++++++++++++++ > > src/qcam/main_window.h | 6 ++ > > src/qcam/meson.build | 2 + > > 4 files changed, 78 insertions(+) > > > > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc > > index c5302040..6b08395a 100644 > > --- a/src/qcam/assets/feathericons/feathericons.qrc > > +++ b/src/qcam/assets/feathericons/feathericons.qrc > > @@ -3,9 +3,11 @@ > > <qresource> > > <file>aperture.svg</file> > > <file>camera-off.svg</file> > > + <file>file.svg</file> > > I'm a bit annoyed by the fact that this icon is very generic. I looked > through the feathericons directory and nothing really strikes me as > being a perfect match (it would be too easy). https://feathericons.com/ > has a few new icons, but also nothing that really stands out. Here are a > few options you may want to consider: > > - activity: Hints at something that evolves over time. > - command: A capture script is somehow a list of commands. Maybe too > Apple-related ? > - list: The other term in a "list of commands". > - settings: The script contains settings, but it may be best to keep > this icon for a future settings dialog. > - sliders: Same here. > - zap: Not sure what the link would be, but there's a zap-off icon that > is handy to replace x-square.svg > > > <file>play-circle.svg</file> > > <file>save.svg</file> > > <file>stop-circle.svg</file> > > <file>x-circle.svg</file> > > + <file>x-square.svg</file> > > As hinted by the zap proposal above, I think the "on" and "off" states > of the button should have related icons. Another option is to keep the > same icon, and just update the button state, the same way we handle the > "Start Capture" button. > > The feather icons are only fallbacks, we also have to select standard > icons from the theme. The latest (but old) version of the icon naming > specification is available at > https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html. > There we have even less options, I've only seen the following ones as > possibly relevant: > > system-run > media-playlist-repeat > media-playlist-shuffle > sync-synchronizing > > Although there's also text-x-script that may be interesting. > > Another option would be to rethink the UI and add capture script support > somewhere else than in the toolbar. As discussed In reply to the 3rd patch I think the capture script ui would be changed. > > > </qresource> > > </RCC> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 4e773c31..9dc96fbb 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -20,6 +20,7 @@ > > #include <QImage> > > #include <QImageWriter> > > #include <QInputDialog> > > +#include <QMessageBox> > > #include <QMutexLocker> > > #include <QStandardPaths> > > #include <QStringList> > > @@ -233,6 +234,13 @@ int MainWindow::createToolbars() > > saveRaw_ = action; > > #endif > > > > + /* Open Script... action. */ > > + action = toolbar_->addAction(QIcon::fromTheme("document-open", > > + QIcon(":file.svg")), > > + "Open Capture Script"); > > I'd name this "Run Capture Script". The action both opens and runs the > script, but the latter seems to be the most important part. > > > + connect(action, &QAction::triggered, this, &MainWindow::chooseScript); > > + scriptExecAction_ = action; > > + > > return 0; > > } > > > > @@ -256,6 +264,60 @@ void MainWindow::updateTitle() > > setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps"); > > } > > > > +/** > > + * \brief Load a capture script for handling the capture session. > > + * > > + * If already capturing, it would restart the capture. > > Why is that ? Could we apply the capture script from the current point > without restarting the camera ? As far my understanding goes, the pipeline handlers at the very start of the capture do certain special things. So to maintain consistency, we restart the capture. > > > + */ > > +void MainWindow::chooseScript() > > +{ > > + if (script_) { > > + /* > > + * This is the second valid press of load script button, > > + * It indicates stopping, Stop and set button for new script. > > Either ", it" or ". It", and ", stop" or ". Stop". > > > + */ > > + script_.reset(); > > + scriptExecAction_->setIcon(QIcon::fromTheme("document-open", > > + QIcon(":file.svg"))); > > + scriptExecAction_->setText("Open Capture Script"); > > + return; > > + } > > + > > + QString scriptFile = QFileDialog::getOpenFileName(this, "Open Capture Script", QDir::currentPath(), > > + "Capture Script (*.yaml)"); > > + if (scriptFile.isEmpty()) > > + return; > > + > > + /* > > + * If we are already capturing, > > + * stop so we don't have stuck image in viewfinder. > > * If we are already capturing, stop so we don't have stuck image in > * viewfinder. > Noted. > > + */ > > + bool wasCapturing = isCapturing_; > > + if (isCapturing_) > > + toggleCapture(false); > > + > > + script_ = std::make_unique<CaptureScript>(camera_, scriptFile.toStdString()); > > + if (!script_->valid()) { > > + script_.reset(); > > + QMessageBox::critical(this, "Invalid Script", > > + "Couldn't load the capture script"); > > + if (wasCapturing) > > + toggleCapture(true); > > + return; > > + } > > + > > + /* > > + * Valid script verified > > + * Set the button to indicate stopping availibility. > > + */ > > Generally speaking, you shouldn't have line wraps at the end of a > sentence in the middle of a paragraph (other than the ones required to > limit the line length to 80 columns): > > /* > * Valid script verified. Set the button to indicate stopping > * availibility. > */ > > If you want to separate the two sentences in two paragraphs, you need a > blank line: > > /* > * Valid script verified. > * > * Set the button to indicate stopping availibility. > */ > > I don't think that's what you want here though. Oops, noted. > > > + scriptExecAction_->setIcon(QIcon(":x-square.svg")); > > + scriptExecAction_->setText("Stop Script execution"); > > + > > + /* Start capture again if we were capturing before. */ > > + if (wasCapturing) > > + toggleCapture(true); > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Camera Selection > > */ > > @@ -511,6 +573,7 @@ int MainWindow::startCapture() > > previousFrames_ = 0; > > framesCaptured_ = 0; > > lastBufferTime_ = 0; > > + queueCount_ = 0; > > > > ret = camera_->start(); > > if (ret) { > > @@ -790,5 +853,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer) > > > > int MainWindow::queueRequest(Request *request) > > { > > + if (script_) > > + request->controls() = script_->frameControls(queueCount_); > > + > > + queueCount_++; > > + > > return camera_->queueRequest(request); > > } > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index bc844711..eb398c1d 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -26,6 +26,7 @@ > > #include <QQueue> > > #include <QTimer> > > > > +#include "../cam/capture_script.h" > > #include "../cam/stream_options.h" > > > > #include "viewfinder.h" > > @@ -87,11 +88,14 @@ private: > > void processHotplug(HotplugEvent *e); > > void processViewfinder(libcamera::FrameBuffer *buffer); > > > > + void chooseScript(); > > + > > /* UI elements */ > > QToolBar *toolbar_; > > QAction *startStopAction_; > > QComboBox *cameraCombo_; > > QAction *saveRaw_; > > + QAction *scriptExecAction_; > > ViewFinder *viewfinder_; > > > > QIcon iconPlay_; > > @@ -125,6 +129,8 @@ private: > > QElapsedTimer frameRateInterval_; > > uint32_t previousFrames_; > > uint32_t framesCaptured_; > > + uint32_t queueCount_; > > > > std::vector<std::unique_ptr<libcamera::Request>> requests_; > > + std::unique_ptr<CaptureScript> script_; > > }; > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build > > index c46f4631..67074252 100644 > > --- a/src/qcam/meson.build > > +++ b/src/qcam/meson.build > > @@ -15,6 +15,7 @@ endif > > qcam_enabled = true > > > > qcam_sources = files([ > > + '../cam/capture_script.cpp', > > '../cam/image.cpp', > > '../cam/options.cpp', > > '../cam/stream_options.cpp', > > At some point we should probably create an application helpers library > shared between cam and qcam. Out of scope for this series of course. > > > @@ -37,6 +38,7 @@ qcam_resources = files([ > > qcam_deps = [ > > libatomic, > > libcamera_public, > > + libyaml, > > qt5_dep, > > ] > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc index c5302040..6b08395a 100644 --- a/src/qcam/assets/feathericons/feathericons.qrc +++ b/src/qcam/assets/feathericons/feathericons.qrc @@ -3,9 +3,11 @@ <qresource> <file>aperture.svg</file> <file>camera-off.svg</file> + <file>file.svg</file> <file>play-circle.svg</file> <file>save.svg</file> <file>stop-circle.svg</file> <file>x-circle.svg</file> + <file>x-square.svg</file> </qresource> </RCC> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 4e773c31..9dc96fbb 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -20,6 +20,7 @@ #include <QImage> #include <QImageWriter> #include <QInputDialog> +#include <QMessageBox> #include <QMutexLocker> #include <QStandardPaths> #include <QStringList> @@ -233,6 +234,13 @@ int MainWindow::createToolbars() saveRaw_ = action; #endif + /* Open Script... action. */ + action = toolbar_->addAction(QIcon::fromTheme("document-open", + QIcon(":file.svg")), + "Open Capture Script"); + connect(action, &QAction::triggered, this, &MainWindow::chooseScript); + scriptExecAction_ = action; + return 0; } @@ -256,6 +264,60 @@ void MainWindow::updateTitle() setWindowTitle(title_ + " : " + QString::number(fps, 'f', 2) + " fps"); } +/** + * \brief Load a capture script for handling the capture session. + * + * If already capturing, it would restart the capture. + */ +void MainWindow::chooseScript() +{ + if (script_) { + /* + * This is the second valid press of load script button, + * It indicates stopping, Stop and set button for new script. + */ + script_.reset(); + scriptExecAction_->setIcon(QIcon::fromTheme("document-open", + QIcon(":file.svg"))); + scriptExecAction_->setText("Open Capture Script"); + return; + } + + QString scriptFile = QFileDialog::getOpenFileName(this, "Open Capture Script", QDir::currentPath(), + "Capture Script (*.yaml)"); + if (scriptFile.isEmpty()) + return; + + /* + * If we are already capturing, + * stop so we don't have stuck image in viewfinder. + */ + bool wasCapturing = isCapturing_; + if (isCapturing_) + toggleCapture(false); + + script_ = std::make_unique<CaptureScript>(camera_, scriptFile.toStdString()); + if (!script_->valid()) { + script_.reset(); + QMessageBox::critical(this, "Invalid Script", + "Couldn't load the capture script"); + if (wasCapturing) + toggleCapture(true); + return; + } + + /* + * Valid script verified + * Set the button to indicate stopping availibility. + */ + scriptExecAction_->setIcon(QIcon(":x-square.svg")); + scriptExecAction_->setText("Stop Script execution"); + + /* Start capture again if we were capturing before. */ + if (wasCapturing) + toggleCapture(true); +} + /* ----------------------------------------------------------------------------- * Camera Selection */ @@ -511,6 +573,7 @@ int MainWindow::startCapture() previousFrames_ = 0; framesCaptured_ = 0; lastBufferTime_ = 0; + queueCount_ = 0; ret = camera_->start(); if (ret) { @@ -790,5 +853,10 @@ void MainWindow::renderComplete(FrameBuffer *buffer) int MainWindow::queueRequest(Request *request) { + if (script_) + request->controls() = script_->frameControls(queueCount_); + + queueCount_++; + return camera_->queueRequest(request); } diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h index bc844711..eb398c1d 100644 --- a/src/qcam/main_window.h +++ b/src/qcam/main_window.h @@ -26,6 +26,7 @@ #include <QQueue> #include <QTimer> +#include "../cam/capture_script.h" #include "../cam/stream_options.h" #include "viewfinder.h" @@ -87,11 +88,14 @@ private: void processHotplug(HotplugEvent *e); void processViewfinder(libcamera::FrameBuffer *buffer); + void chooseScript(); + /* UI elements */ QToolBar *toolbar_; QAction *startStopAction_; QComboBox *cameraCombo_; QAction *saveRaw_; + QAction *scriptExecAction_; ViewFinder *viewfinder_; QIcon iconPlay_; @@ -125,6 +129,8 @@ private: QElapsedTimer frameRateInterval_; uint32_t previousFrames_; uint32_t framesCaptured_; + uint32_t queueCount_; std::vector<std::unique_ptr<libcamera::Request>> requests_; + std::unique_ptr<CaptureScript> script_; }; diff --git a/src/qcam/meson.build b/src/qcam/meson.build index c46f4631..67074252 100644 --- a/src/qcam/meson.build +++ b/src/qcam/meson.build @@ -15,6 +15,7 @@ endif qcam_enabled = true qcam_sources = files([ + '../cam/capture_script.cpp', '../cam/image.cpp', '../cam/options.cpp', '../cam/stream_options.cpp', @@ -37,6 +38,7 @@ qcam_resources = files([ qcam_deps = [ libatomic, libcamera_public, + libyaml, qt5_dep, ]
Implement an Open Capture Script button which would allow the user to open a Capture Script (*.yaml). This button has two states : - Open Capture Script - Stop the execution of current capture script When being clicked in open state, present them with a QFileDialog to allow user to select a single file. Introduce a queueCount_ to keep track of the requests queued. When stopping the execution of the capture script the queueCount_ is not reseted and the capture is continues as it is (i.e it is not stopped or restarted). Requests are queued with any controls the script matching the current queueCount_. Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- Changes from v4 - Reworded the commit message to clarify button states and queueCount_ reseting. src/qcam/assets/feathericons/feathericons.qrc | 2 + src/qcam/main_window.cpp | 68 +++++++++++++++++++ src/qcam/main_window.h | 6 ++ src/qcam/meson.build | 2 + 4 files changed, 78 insertions(+)