[PATCH v2] qcam: Automatically select the camera if only one is available

[Stanislaw Gruszka]

On Sun, Oct 20, 2024 at 06:35:39PM +0300, Laurent Pinchart wrote:
> On Sun, Oct 20, 2024 at 12:03:30PM +0100, Kieran Bingham wrote:
> > Quoting Stanislaw Gruszka (2024-10-19 11:09:25)
> > > When only a single camera is available, showing the camera selection
> > > dialog is unnecessary. It's better to automatically select the available
> > > camera without prompting the user for input.
> > > 
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> > > ---
> > > v2:
> > > - Avoid cameras().size() vs cameras()[0] race condition
> > > - Update in code comment
> > > 
> > >  src/apps/qcam/main_window.cpp | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> > > index 5144c6b3..86ffa205 100644
> > > --- a/src/apps/qcam/main_window.cpp
> > > +++ b/src/apps/qcam/main_window.cpp
> > > @@ -296,14 +296,23 @@ std::string MainWindow::chooseCamera()
> > >  int MainWindow::openCamera()
> > >  {
> > >         std::string cameraName;
> > > +       int num = 0;
> > >  
> > >         /*
> > > -        * Use the camera specified on the command line, if any, or display the
> > > -        * camera selection dialog box otherwise.
> > > +        * Use the camera specified on the command line, if any, or select the
> > > +        * only one available, otherwise display the camera selection dialog box.
> > >          */
> > > -       if (options_.isSet(OptCamera))
> > > +       if (options_.isSet(OptCamera)) {
> > >                 cameraName = static_cast<std::string>(options_[OptCamera]);
> > > -       else
> > > +       } else {
> > > +               for (const auto &cam : cm_->cameras()) {
> > > +                       num++;
> > > +                       if (num > 1)
> > 
> > Really trivial nit but I would have probably put 'if (++num > 1)' here
> > to remove a line.
> 
> A bit of a larger patch, but would the following be cleaner ?

I think it's subjective it's it's cleaner or not. Looks fine for me.

> -	if (cameraName == "")
> -		return -EINVAL;
> -
> -	/* Get and acquire the camera. */
> -	camera_ = cm_->get(cameraName);
> -	if (!camera_) {
> -		qInfo() << "Camera" << cameraName.c_str() << "not found";
> -		return -ENODEV;
> +		if (cameras.size() == 1)
> +			camera_ = cameras[0];
> +		else
> +			camera_ = chooseCamera();
>  	}
>  
> +	/* Acquire the camera. */
>  	if (camera_->acquire()) {

But it crashes here if there are no cameras and user press OK
button in dialog window, due to lack of !camera_ check.
I can fix that and post as v3...

Regards
Stanislaw


>  		qInfo() << "Failed to acquire camera";
>  		camera_.reset();
> @@ -332,7 +323,7 @@ int MainWindow::openCamera()
>  	}
>  
>  	/* Set the camera switch button with the currently selected Camera id. */
> -	cameraSelectButton_->setText(QString::fromStdString(cameraName));
> +	cameraSelectButton_->setText(QString::fromStdString(camera_->id()));
>  
>  	return 0;
>  }
> diff --git a/src/apps/qcam/main_window.h b/src/apps/qcam/main_window.h
> index 4cead7344d27..81fcf915ade5 100644
> --- a/src/apps/qcam/main_window.h
> +++ b/src/apps/qcam/main_window.h
> @@ -73,7 +73,7 @@ private Q_SLOTS:
>  private:
>  	int createToolbars();
>  
> -	std::string chooseCamera();
> +	std::shared_ptr<libcamera::Camera> chooseCamera();
>  	int openCamera();
>  
>  	int startCapture();
> 
> The change to MainWindow::chooseCamera() is not strictly mandatory, but
> results (I think) in cleaner code in MainWindow::openCamera(). Feel free
> to propose alternatives, the important part to avoid TOCTOU races and
> keep MainWindow::openCamera() clean is
> 
> 		std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
> 
> > But that's no reason to worry.
> > 
> > The fact that we make it so easy for Laurent to worry about TOCTOU here
> > means that's probably just unfriendly in the libcamera API. I don't know
> > how to make it easier at the moment though ... so
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > > +                               break;
> > > +                       cameraName = cam->id();
> > > +               }
> > > +       }
> > > +       if (num > 1)
> > >                 cameraName = chooseCamera();
> > >  
> > >         if (cameraName == "")
> 
> -- 
> Regards,
> 
> Laurent Pinchart