[libcamera-devel] [PATCH v5 3/3] qcam: Add --script to load capture script

[Utkarsh Tiwari]

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 at gmail.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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