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

[Laurent Pinchart]

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 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.   

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

-- 
Regards,

Laurent Pinchart