[libcamera-devel,v9,7/7] qcam: Add --script to load capture script

17271 diff mbox series

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 an critical error
QMessageBox and close the application.
---
 Differences from v8:
    1. Functionally the patch remains same.
    2. The difference is that checking for OptCaptureScript is now done
    in MainWindow::MainWindow()
 src/qcam/main.cpp        |  3 +++
 src/qcam/main_window.cpp | 11 +++++++++--
 src/qcam/main_window.h   |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

Quoting Utkarsh Tiwari via libcamera-devel (2022-08-31 06:49:38)
> 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 an critical error
> QMessageBox and close the application.
> ---
>  Differences from v8:
>     1. Functionally the patch remains same.
>     2. The difference is that checking for OptCaptureScript is now done
>     in MainWindow::MainWindow()
>  src/qcam/main.cpp        |  3 +++
>  src/qcam/main_window.cpp | 11 +++++++++--
>  src/qcam/main_window.h   |  1 +
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> 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 af992b94..e488b67f 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -145,6 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>         cm_->cameraAdded.connect(this, &MainWindow::addCamera);
>         cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);
>  
> +       if (options_.isSet(OptCaptureScript))
> +               scriptPath_ = options_[OptCaptureScript].toString();
> +
>         cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);
>         connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
>                 this, &MainWindow::stopCaptureScript);
> @@ -157,9 +160,13 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>         }
>  
>         /* Start capture script. */
> -       if (!scriptPath_.empty())
> +       if (!scriptPath_.empty()) {
>                 ret = loadCaptureScript();
> -
> +               if (options_.isSet(OptCaptureScript)) {

Isn't ret supposed to be checked here ?

> +                       quit();
> +                       return;
> +               }
> +       }
>         startStopAction_->setChecked(true);
>  }
>  
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 7c877ae1..24ebd019 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -45,6 +45,7 @@ enum {
>         OptRenderer = 'r',
>         OptStream = 's',
>         OptVerbose = 'v',
> +       OptCaptureScript = 256,
>  };
>  
>  class MainWindow : public QMainWindow
> -- 
> 2.34.1
>

On Wed, Aug 31, 2022 at 10:41:32AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Utkarsh Tiwari via libcamera-devel (2022-08-31 06:49:38)
> > 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 an critical error
> > QMessageBox and close the application.
> > ---
> >  Differences from v8:
> >     1. Functionally the patch remains same.
> >     2. The difference is that checking for OptCaptureScript is now done
> >     in MainWindow::MainWindow()
> >  src/qcam/main.cpp        |  3 +++
> >  src/qcam/main_window.cpp | 11 +++++++++--
> >  src/qcam/main_window.h   |  1 +
> >  3 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > 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 af992b94..e488b67f 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -145,6 +145,9 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >         cm_->cameraAdded.connect(this, &MainWindow::addCamera);
> >         cm_->cameraRemoved.connect(this, &MainWindow::removeCamera);
> >  
> > +       if (options_.isSet(OptCaptureScript))
> > +               scriptPath_ = options_[OptCaptureScript].toString();

At this point the script isn't selected yet, it's only a default. I'd
store it in a local variable, passed to the camera selector dialog box
constructor, and let the normal capture script selection mechanism then
set scriptPath_.

Adding the script path parameter to the camera selector dialog box in
this patch instead of 6/7 would also be better, to make 6/7 clearer.

> > +
> >         cameraSelectorDialog_ = new CameraSelectorDialog(cm_, scriptPath_, this);
> >         connect(cameraSelectorDialog_, &CameraSelectorDialog::stopCaptureScript,
> >                 this, &MainWindow::stopCaptureScript);
> > @@ -157,9 +160,13 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> >         }
> >  
> >         /* Start capture script. */
> > -       if (!scriptPath_.empty())
> > +       if (!scriptPath_.empty()) {
> >                 ret = loadCaptureScript();
> > -
> > +               if (options_.isSet(OptCaptureScript)) {
> 
> Isn't ret supposed to be checked here ?
> 
> > +                       quit();

I don't think quitting is right here. Imagine the following scenerio:

- The user selects a capture script on the command line
- When the camera selection dialog box is opened, the user picks a
  different script
- The script fails to load

Why should we quit in that case, compared to the case where the user
will not have selected a script on the command line ?

> > +                       return;
> > +               }
> > +       }
> >         startStopAction_->setChecked(true);
> >  }
> >  
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 7c877ae1..24ebd019 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -45,6 +45,7 @@ enum {
> >         OptRenderer = 'r',
> >         OptStream = 's',
> >         OptVerbose = 'v',
> > +       OptCaptureScript = 256,
> >  };
> >  
> >  class MainWindow : public QMainWindow