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

[Laurent Pinchart]

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 ?

diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
index 86ffa2050251..305bba79b1b2 100644
--- a/src/apps/qcam/main_window.cpp
+++ b/src/apps/qcam/main_window.cpp
@@ -251,16 +251,14 @@ void MainWindow::updateTitle()
 void MainWindow::switchCamera()
 {
 	/* Get and acquire the new camera. */
-	std::string newCameraId = chooseCamera();
+	std::shared_ptr<Camera> cam = chooseCamera();
 
-	if (newCameraId.empty())
+	if (!cam)
 		return;
 
-	if (camera_ && newCameraId == camera_->id())
+	if (camera_ && cam == camera_)
 		return;
 
-	const std::shared_ptr<Camera> &cam = cm_->get(newCameraId);
-
 	if (cam->acquire()) {
 		qInfo() << "Failed to acquire camera" << cam->id().c_str();
 		return;
@@ -282,49 +280,42 @@ void MainWindow::switchCamera()
 	startStopAction_->setChecked(true);
 
 	/* Display the current cameraId in the toolbar .*/
-	cameraSelectButton_->setText(QString::fromStdString(newCameraId));
+	cameraSelectButton_->setText(QString::fromStdString(cam->id()));
 }
 
-std::string MainWindow::chooseCamera()
+std::shared_ptr<Camera> MainWindow::chooseCamera()
 {
 	if (cameraSelectorDialog_->exec() != QDialog::Accepted)
-		return std::string();
+		return {};
 
-	return cameraSelectorDialog_->getCameraId();
+	std::string id = cameraSelectorDialog_->getCameraId();
+	return cm_->get(id);
 }
 
 int MainWindow::openCamera()
 {
-	std::string cameraName;
-	int num = 0;
-
 	/*
-	 * Use the camera specified on the command line, if any, or select the
-	 * only one available, otherwise display the camera selection dialog box.
+	 * If a camera is specified on the command line, get it. Otherwise, if
+	 * only one camera is available, pick it automatically, else, display
+	 * the selector dialog box.
 	 */
 	if (options_.isSet(OptCamera)) {
-		cameraName = static_cast<std::string>(options_[OptCamera]);
-	} else {
-		for (const auto &cam : cm_->cameras()) {
-			num++;
-			if (num > 1)
-				break;
-			cameraName = cam->id();
+		std::string cameraName = static_cast<std::string>(options_[OptCamera]);
+		camera_ = cm_->get(cameraName);
+		if (!camera_) {
+			qInfo() << "Camera" << cameraName.c_str() << "not found";
+			return -ENODEV;
 		}
-	}
-	if (num > 1)
-		cameraName = chooseCamera();
+	} else {
+		std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
 
-	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()) {
 		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