[libcamera-devel,v4,3/3] qcam: Add --script to load capture script
diff mbox series

Message ID 20220703043704.296872-4-utkarsh02t@gmail.com
State Superseded
Headers show
Series
  • Introduce capture scripts to qcam
Related show

Commit Message

Utkarsh Tiwari July 3, 2022, 4:37 a.m. UTC 
Add --script as an individual option to load capture scripts.
Load the capture script before starting the capture.

If an invalid capture script has been given, show error
but start the capture without the script.

Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/qcam/main.cpp        |  3 +++
 src/qcam/main_window.cpp | 14 ++++++++++++++
 src/qcam/main_window.h   |  1 +
 3 files changed, 18 insertions(+)

Comments

Umang Jain July 25, 2022, 6:54 p.m. UTC | #1 
Hi Utkarsh,

On 7/3/22 10:07, Utkarsh Tiwari via libcamera-devel wrote:
> Add --script as an individual option to load capture scripts.
> Load the capture script before starting the capture.
>
> If an invalid capture script has been given, show error
> but start the capture without the script.


umm, why would you do that? If --script is specified, it is pretty clear 
that the user intends to use the capture script. If there are errors in 
the scripts, we sould fail loudly and not proceed with the start() at all.

The user 'might' get a false impression that the script is valid hence, 
the capture is started and "miss" to  take a note of the invalidity 
error. Was this discussed in previous iterations?

>
> Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/qcam/main.cpp        |  3 +++
>   src/qcam/main_window.cpp | 14 ++++++++++++++
>   src/qcam/main_window.h   |  1 +
>   3 files changed, 18 insertions(+)
>
> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> index d3f01a85..91166be5 100644
> --- a/src/qcam/main.cpp
> +++ b/src/qcam/main.cpp
> @@ -43,6 +43,9 @@ OptionsParser::Options parseOptions(int argc, char *argv[])
>   			 "Set configuration of a camera stream", "stream", true);
>   	parser.addOption(OptVerbose, OptionNone,
>   			 "Print verbose log messages", "verbose");
> +	parser.addOption(OptCaptureScript, OptionString,
> +			 "Load a capture session configuration script from a file",
> +			 "script", ArgumentRequired, "script");
>   
>   	OptionsParser::Options options = parser.parse(argc, argv);
>   	if (options.isSet(OptHelp))
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 4fbaeccc..7ec53a7c 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -151,6 +151,20 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>   		return;
>   	}
>   
> +	if (options_.isSet(OptCaptureScript)) {
> +		std::string scriptName = options_[OptCaptureScript].toString();
> +		script_ = std::make_unique<CaptureScript>(camera_, scriptName);
> +		if (!script_->valid()) {
> +			QMessageBox::critical(this, "Invalid Script",
> +					      "Couldn't load the capture script");
> +			script_.reset();
> +		}
> +
> +		/* Show stopping availability. */
> +		scriptExecAction_->setIcon(QIcon(":x-square.svg"));
> +		scriptExecAction_->setText("Stop Script execution");
> +	}
> +
>   	startStopAction_->setChecked(true);
>   }
>   
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 2cdf7169..4d19ab64 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -42,6 +42,7 @@ enum {
>   	OptRenderer = 'r',
>   	OptStream = 's',
>   	OptVerbose = 'v',
> +	OptCaptureScript = 256,
>   };
>   
>   class MainWindow : public QMainWindow
Kieran Bingham July 25, 2022, 7:08 p.m. UTC | #2 
Quoting Umang Jain via libcamera-devel (2022-07-25 19:54:32)
> Hi Utkarsh,
> 
> On 7/3/22 10:07, Utkarsh Tiwari via libcamera-devel wrote:
> > Add --script as an individual option to load capture scripts.
> > Load the capture script before starting the capture.
> >
> > If an invalid capture script has been given, show error
> > but start the capture without the script.
> 
> 
> umm, why would you do that? If --script is specified, it is pretty clear 
> that the user intends to use the capture script. If there are errors in 
> the scripts, we sould fail loudly and not proceed with the start() at all.
> 

That's quite a good point. Failing through the GUI makes sense when
started in the GUI - but indeed, if the script was specified on the
command line, and fails - that's probably expected to fail on the
command line ...

Perhaps it would make sense to keep the qmessagebox::critical to pop up
a gui failure, but also present a console warning, and then exit after
the message box is closed.

> The user 'might' get a false impression that the script is valid hence, 
> the capture is started and "miss" to  take a note of the invalidity 
> error. Was this discussed in previous iterations?

I don't think so.

> 
> >
> > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >   src/qcam/main.cpp        |  3 +++
> >   src/qcam/main_window.cpp | 14 ++++++++++++++
> >   src/qcam/main_window.h   |  1 +
> >   3 files changed, 18 insertions(+)
> >
> > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> > index d3f01a85..91166be5 100644
> > --- a/src/qcam/main.cpp
> > +++ b/src/qcam/main.cpp
> > @@ -43,6 +43,9 @@ OptionsParser::Options parseOptions(int argc, char *argv[])
> >                        "Set configuration of a camera stream", "stream", true);
> >       parser.addOption(OptVerbose, OptionNone,
> >                        "Print verbose log messages", "verbose");
> > +     parser.addOption(OptCaptureScript, OptionString,
> > +                      "Load a capture session configuration script from a file",
> > +                      "script", ArgumentRequired, "script");
> >   
> >       OptionsParser::Options options = parser.parse(argc, argv);
> >       if (options.isSet(OptHelp))
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 4fbaeccc..7ec53a7c 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -151,6 +151,20 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >               return;
> >       }
> >   
> > +     if (options_.isSet(OptCaptureScript)) {
> > +             std::string scriptName = options_[OptCaptureScript].toString();
> > +             script_ = std::make_unique<CaptureScript>(camera_, scriptName);
> > +             if (!script_->valid()) {
> > +                     QMessageBox::critical(this, "Invalid Script",
> > +                                           "Couldn't load the capture script");
> > +                     script_.reset();
> > +             }
> > +
> > +             /* Show stopping availability. */
> > +             scriptExecAction_->setIcon(QIcon(":x-square.svg"));
> > +             scriptExecAction_->setText("Stop Script execution");
> > +     }
> > +
> >       startStopAction_->setChecked(true);
> >   }
> >   
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 2cdf7169..4d19ab64 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -42,6 +42,7 @@ enum {
> >       OptRenderer = 'r',
> >       OptStream = 's',
> >       OptVerbose = 'v',
> > +     OptCaptureScript = 256,
> >   };
> >   
> >   class MainWindow : public QMainWindow
Utkarsh Tiwari July 26, 2022, 2:24 a.m. UTC | #3 
On Tue, 26 Jul, 2022, 00:38 Kieran Bingham, <kieran.bingham@ideasonboard.com>
wrote:

> Quoting Umang Jain via libcamera-devel (2022-07-25 19:54:32)
> > Hi Utkarsh,
> >
> > On 7/3/22 10:07, Utkarsh Tiwari via libcamera-devel wrote:
> > > Add --script as an individual option to load capture scripts.
> > > Load the capture script before starting the capture.
> > >
> > > If an invalid capture script has been given, show error
> > > but start the capture without the script.
> >
> >
> > umm, why would you do that? If --script is specified, it is pretty clear
> > that the user intends to use the capture script. If there are errors in
> > the scripts, we sould fail loudly and not proceed with the start() at
> all.
> >
>
> That's quite a good point. Failing through the GUI makes sense when
> started in the GUI - but indeed, if the script was specified on the
> command line, and fails - that's probably expected to fail on the
> command line ...
>
> Perhaps it would make sense to keep the qmessagebox::critical to pop up
> a gui failure, but also present a console warning, and then exit after
> the message box is closed.
>
> > The user 'might' get a false impression that the script is valid hence,
> > the capture is started and "miss" to  take a note of the invalidity
> > error. Was this discussed in previous iterations?
>
> I don't think so.
>

Well seems reasonable, new version for the patchset it seems.

>
> >
> > >
> > > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >   src/qcam/main.cpp        |  3 +++
> > >   src/qcam/main_window.cpp | 14 ++++++++++++++
> > >   src/qcam/main_window.h   |  1 +
> > >   3 files changed, 18 insertions(+)
> > >
> > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> > > index d3f01a85..91166be5 100644
> > > --- a/src/qcam/main.cpp
> > > +++ b/src/qcam/main.cpp
> > > @@ -43,6 +43,9 @@ OptionsParser::Options parseOptions(int argc, char
> *argv[])
> > >                        "Set configuration of a camera stream",
> "stream", true);
> > >       parser.addOption(OptVerbose, OptionNone,
> > >                        "Print verbose log messages", "verbose");
> > > +     parser.addOption(OptCaptureScript, OptionString,
> > > +                      "Load a capture session configuration script
> from a file",
> > > +                      "script", ArgumentRequired, "script");
> > >
> > >       OptionsParser::Options options = parser.parse(argc, argv);
> > >       if (options.isSet(OptHelp))
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 4fbaeccc..7ec53a7c 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -151,6 +151,20 @@ MainWindow::MainWindow(CameraManager *cm, const
> OptionsParser::Options &options)
> > >               return;
> > >       }
> > >
> > > +     if (options_.isSet(OptCaptureScript)) {
> > > +             std::string scriptName =
> options_[OptCaptureScript].toString();
> > > +             script_ = std::make_unique<CaptureScript>(camera_,
> scriptName);
> > > +             if (!script_->valid()) {
> > > +                     QMessageBox::critical(this, "Invalid Script",
> > > +                                           "Couldn't load the capture
> script");
> > > +                     script_.reset();
> > > +             }
> > > +
> > > +             /* Show stopping availability. */
> > > +             scriptExecAction_->setIcon(QIcon(":x-square.svg"));
> > > +             scriptExecAction_->setText("Stop Script execution");
> > > +     }
> > > +
> > >       startStopAction_->setChecked(true);
> > >   }
> > >
> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > index 2cdf7169..4d19ab64 100644
> > > --- a/src/qcam/main_window.h
> > > +++ b/src/qcam/main_window.h
> > > @@ -42,6 +42,7 @@ enum {
> > >       OptRenderer = 'r',
> > >       OptStream = 's',
> > >       OptVerbose = 'v',
> > > +     OptCaptureScript = 256,
> > >   };
> > >
> > >   class MainWindow : public QMainWindow
>

Patch
diff mbox series 

diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
index d3f01a85..91166be5 100644
--- a/src/qcam/main.cpp
+++ b/src/qcam/main.cpp
@@ -43,6 +43,9 @@  OptionsParser::Options parseOptions(int argc, char *argv[])
 			 "Set configuration of a camera stream", "stream", true);
 	parser.addOption(OptVerbose, OptionNone,
 			 "Print verbose log messages", "verbose");
+	parser.addOption(OptCaptureScript, OptionString,
+			 "Load a capture session configuration script from a file",
+			 "script", ArgumentRequired, "script");
 
 	OptionsParser::Options options = parser.parse(argc, argv);
 	if (options.isSet(OptHelp))
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index 4fbaeccc..7ec53a7c 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -151,6 +151,20 @@  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
 		return;
 	}
 
+	if (options_.isSet(OptCaptureScript)) {
+		std::string scriptName = options_[OptCaptureScript].toString();
+		script_ = std::make_unique<CaptureScript>(camera_, scriptName);
+		if (!script_->valid()) {
+			QMessageBox::critical(this, "Invalid Script",
+					      "Couldn't load the capture script");
+			script_.reset();
+		}
+
+		/* Show stopping availability. */
+		scriptExecAction_->setIcon(QIcon(":x-square.svg"));
+		scriptExecAction_->setText("Stop Script execution");
+	}
+
 	startStopAction_->setChecked(true);
 }
 
diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
index 2cdf7169..4d19ab64 100644
--- a/src/qcam/main_window.h
+++ b/src/qcam/main_window.h
@@ -42,6 +42,7 @@  enum {
 	OptRenderer = 'r',
 	OptStream = 's',
 	OptVerbose = 'v',
+	OptCaptureScript = 256,
 };
 
 class MainWindow : public QMainWindow