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

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

Commit Message

Utkarsh Tiwari July 26, 2022, 7:41 p.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, display a QMessageBox and
print it on the console informing that the script is invalid. Do not
start the capture.

Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 Changes from v4
	- Now when we have an invalid script we quit and don't start the
	  capture.
 src/qcam/main.cpp        |  3 +++
 src/qcam/main_window.cpp | 20 ++++++++++++++++++++
 src/qcam/main_window.h   |  1 +
 3 files changed, 24 insertions(+)

Comments

Laurent Pinchart July 26, 2022, 10:23 p.m. UTC | #1 
Hi Utkarsh,

Thank you for the patch.

On Wed, Jul 27, 2022 at 01:11:23AM +0530, 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, display a QMessageBox and
> print it on the console informing that the script is invalid. Do not
> start the capture.
> 
> Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  Changes from v4
> 	- Now when we have an invalid script we quit and don't start the
> 	  capture.
>  src/qcam/main.cpp        |  3 +++
>  src/qcam/main_window.cpp | 20 ++++++++++++++++++++
>  src/qcam/main_window.h   |  1 +
>  3 files changed, 24 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 9dc96fbb..7b2bc84b 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -152,6 +152,26 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)

The comment a few lines above should be changed to

	/* Open the camera. */

>  		return;
>  	}
>  

And here,

	/* Open and load the capture script if specified on the command line. */

> +	if (options_.isSet(OptCaptureScript)) {
> +		std::string scriptName = options_[OptCaptureScript].toString();
> +		script_ = std::make_unique<CaptureScript>(camera_, scriptName);

There's one issue here. You create the capture script for the currently
selected camera, which is fine, but only until a different camera is
selected. Then the capture script won't match anymore. We could then
reset the capture script, but what should we do if the user select the
previous camera again ? The behaviour seems confusing to me. I wonder if
a better user experience could be achieved, maybe by redesigning the
camera selection UI to tie it with capture script selection.

Good UI design is difficult :-S

> +		if (!script_->valid()) {
> +			QMessageBox::critical(this, "Invalid Script",
> +					      "Couldn't load the capture script");

I'd skip the message box. If the command line argument is wrong, a
message on the console should be enough.

> +			qInfo() << "Invalid Capture Script";

You can make that a qError().

> +
> +			script_.reset();

That's probably not needed given that you quit() right after.

> +
> +			/* Do not start capture if invalid script. */
> +			quit();
> +			return;
> +		}
> +
> +		/* Show stopping availability. */
> +		scriptExecAction_->setIcon(QIcon(":x-square.svg"));
> +		scriptExecAction_->setText("Stop Script execution");

It would be nice if we could centralize the logic that handles the
button state (and possible the construction of the CaptureScript
object). I'm not sure if there's an easy way to do so that will result
in more readable code.

> +	}
> +

Finally,

	/* Start the camera. */

>  	startStopAction_->setChecked(true);
>  }
>  
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index eb398c1d..59e50b5e 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -43,6 +43,7 @@ enum {
>  	OptRenderer = 'r',
>  	OptStream = 's',
>  	OptVerbose = 'v',
> +	OptCaptureScript = 256,
>  };
>  
>  class MainWindow : public QMainWindow
Utkarsh Tiwari July 28, 2022, 1:58 p.m. UTC | #2 
Hi Laurent, thanks for the review

On Wed, Jul 27, 2022 at 01:23:43AM +0300, Laurent Pinchart wrote:
> Hi Utkarsh,
> 
> Thank you for the patch.
> 
> On Wed, Jul 27, 2022 at 01:11:23AM +0530, 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, display a QMessageBox and
> > print it on the console informing that the script is invalid. Do not
> > start the capture.
> > 
> > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  Changes from v4
> > 	- Now when we have an invalid script we quit and don't start the
> > 	  capture.
> >  src/qcam/main.cpp        |  3 +++
> >  src/qcam/main_window.cpp | 20 ++++++++++++++++++++
> >  src/qcam/main_window.h   |  1 +
> >  3 files changed, 24 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 9dc96fbb..7b2bc84b 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -152,6 +152,26 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> 
> The comment a few lines above should be changed to
> 
> 	/* Open the camera. */
> 
> >  		return;
> >  	}
> >  
> 
> And here,
> 
> 	/* Open and load the capture script if specified on the command line. */
> 

Noted.

> > +	if (options_.isSet(OptCaptureScript)) {
> > +		std::string scriptName = options_[OptCaptureScript].toString();
> > +		script_ = std::make_unique<CaptureScript>(camera_, scriptName);
> 
> There's one issue here. You create the capture script for the currently
> selected camera, which is fine, but only until a different camera is
> selected. Then the capture script won't match anymore. We could then
> reset the capture script, but what should we do if the user select the
> previous camera again ? The behaviour seems confusing to me. I wonder if
> a better user experience could be achieved, maybe by redesigning the
> camera selection UI to tie it with capture script selection.
> 
> Good UI design is difficult :-S

Ah, the camera switch case slipped my mind. I'm currently working on a camera
selection UI, which would replace the current combobox on the toolbar with a
button. This button would open us a dialog which contains the camera selection
combobox and few other things like the model and the location of the camera.
I think this is the pefect place to place the Capture script and also avoid
the whole icon mess.   

> 
> > +		if (!script_->valid()) {
> > +			QMessageBox::critical(this, "Invalid Script",
> > +					      "Couldn't load the capture script");
> 
> I'd skip the message box. If the command line argument is wrong, a
> message on the console should be enough.

I think the behaviour of selecting invalid scripts should be the same
across methods to access the CaptureScript.

> 
> > +			qInfo() << "Invalid Capture Script";
> 
> You can make that a qError().

Noted.

> 
> > +
> > +			script_.reset();
> 
> That's probably not needed given that you quit() right after.

Noted.

> 
> > +
> > +			/* Do not start capture if invalid script. */
> > +			quit();
> > +			return;
> > +		}
> > +
> > +		/* Show stopping availability. */
> > +		scriptExecAction_->setIcon(QIcon(":x-square.svg"));
> > +		scriptExecAction_->setText("Stop Script execution");
> 
> It would be nice if we could centralize the logic that handles the
> button state (and possible the construction of the CaptureScript
> object). I'm not sure if there's an easy way to do so that will result
> in more readable code.

I have a patch on the mailing list which centralizes the button states, and
also the script resetting. I think I would merge it with this patch series. 

> 
> > +	}
> > +
> 
> Finally,
> 
> 	/* Start the camera. */
> 

Noted.

> >  	startStopAction_->setChecked(true);
> >  }
> >  
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index eb398c1d..59e50b5e 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -43,6 +43,7 @@ enum {
> >  	OptRenderer = 'r',
> >  	OptStream = 's',
> >  	OptVerbose = 'v',
> > +	OptCaptureScript = 256,
> >  };
> >  
> >  class MainWindow : public QMainWindow
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart July 28, 2022, 8:19 p.m. UTC | #3 
Hi Utkarsh,

On Thu, Jul 28, 2022 at 07:28:59PM +0530, Utkarsh Tiwari wrote:
> On Wed, Jul 27, 2022 at 01:23:43AM +0300, Laurent Pinchart wrote:
> > On Wed, Jul 27, 2022 at 01:11:23AM +0530, 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, display a QMessageBox and
> > > print it on the console informing that the script is invalid. Do not
> > > start the capture.
> > > 
> > > Signed-off-by: Utkarsh Tiwari <utkarsh02t@gmail.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  Changes from v4
> > > 	- Now when we have an invalid script we quit and don't start the
> > > 	  capture.
> > >  src/qcam/main.cpp        |  3 +++
> > >  src/qcam/main_window.cpp | 20 ++++++++++++++++++++
> > >  src/qcam/main_window.h   |  1 +
> > >  3 files changed, 24 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 9dc96fbb..7b2bc84b 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -152,6 +152,26 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> > 
> > The comment a few lines above should be changed to
> > 
> > 	/* Open the camera. */
> > 
> > >  		return;
> > >  	}
> > >  
> > 
> > And here,
> > 
> > 	/* Open and load the capture script if specified on the command line. */
> 
> Noted.
> 
> > > +	if (options_.isSet(OptCaptureScript)) {
> > > +		std::string scriptName = options_[OptCaptureScript].toString();
> > > +		script_ = std::make_unique<CaptureScript>(camera_, scriptName);
> > 
> > There's one issue here. You create the capture script for the currently
> > selected camera, which is fine, but only until a different camera is
> > selected. Then the capture script won't match anymore. We could then
> > reset the capture script, but what should we do if the user select the
> > previous camera again ? The behaviour seems confusing to me. I wonder if
> > a better user experience could be achieved, maybe by redesigning the
> > camera selection UI to tie it with capture script selection.
> > 
> > Good UI design is difficult :-S
> 
> Ah, the camera switch case slipped my mind. I'm currently working on a camera
> selection UI, which would replace the current combobox on the toolbar with a
> button. This button would open us a dialog which contains the camera selection
> combobox and few other things like the model and the location of the camera.
> I think this is the pefect place to place the Capture script and also avoid
> the whole icon mess.   

Sounds like a very good idea ! Feel free to share mockups before
completing the full implementation if you would like early feedback.

> > > +		if (!script_->valid()) {
> > > +			QMessageBox::critical(this, "Invalid Script",
> > > +					      "Couldn't load the capture script");
> > 
> > I'd skip the message box. If the command line argument is wrong, a
> > message on the console should be enough.
> 
> I think the behaviour of selecting invalid scripts should be the same
> across methods to access the CaptureScript.

Good point. I can see pros and cons, consistent behaviour is certainly
good, but at the same time we could also consider the command line
arguments as a way to start qcam in a "script-driven" mode where one may
not want errors to be displayed to the user of the GUI.

It would be easier to answer this question if we could actually tell
clearly what we wanted qcam to be :-)

> > > +			qInfo() << "Invalid Capture Script";
> > 
> > You can make that a qError().
> 
> Noted.
> 
> > > +
> > > +			script_.reset();
> > 
> > That's probably not needed given that you quit() right after.
> 
> Noted.
> 
> > > +
> > > +			/* Do not start capture if invalid script. */
> > > +			quit();
> > > +			return;
> > > +		}
> > > +
> > > +		/* Show stopping availability. */
> > > +		scriptExecAction_->setIcon(QIcon(":x-square.svg"));
> > > +		scriptExecAction_->setText("Stop Script execution");
> > 
> > It would be nice if we could centralize the logic that handles the
> > button state (and possible the construction of the CaptureScript
> > object). I'm not sure if there's an easy way to do so that will result
> > in more readable code.
> 
> I have a patch on the mailing list which centralizes the button states, and
> also the script resetting. I think I would merge it with this patch series. 
> 
> > > +	}
> > > +
> > 
> > Finally,
> > 
> > 	/* Start the camera. */
> 
> Noted.
> 
> > >  	startStopAction_->setChecked(true);
> > >  }
> > >  
> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > index eb398c1d..59e50b5e 100644
> > > --- a/src/qcam/main_window.h
> > > +++ b/src/qcam/main_window.h
> > > @@ -43,6 +43,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 9dc96fbb..7b2bc84b 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -152,6 +152,26 @@  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");
+			qInfo() << "Invalid Capture Script";
+
+			script_.reset();
+
+			/* Do not start capture if invalid script. */
+			quit();
+			return;
+		}
+
+		/* 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 eb398c1d..59e50b5e 100644
--- a/src/qcam/main_window.h
+++ b/src/qcam/main_window.h
@@ -43,6 +43,7 @@  enum {
 	OptRenderer = 'r',
 	OptStream = 's',
 	OptVerbose = 'v',
+	OptCaptureScript = 256,
 };
 
 class MainWindow : public QMainWindow