| Message ID | 20241019100925.42808-1-stanislaw.gruszka@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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. 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@ideasonboard.com> > + break; > + cameraName = cam->id(); > + } > + } > + if (num > 1) > cameraName = chooseCamera(); > > if (cameraName == "") > -- > 2.43.0 >
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@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@ideasonboard.com> > > > + break; > > + cameraName = cam->id(); > > + } > > + } > > + if (num > 1) > > cameraName = chooseCamera(); > > > > if (cameraName == "")
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@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@ideasonboard.com> > > > > > + break; > > > + cameraName = cam->id(); > > > + } > > > + } > > > + if (num > 1) > > > cameraName = chooseCamera(); > > > > > > if (cameraName == "") > > -- > Regards, > > Laurent Pinchart
On Mon, Oct 21, 2024 at 05:25:09PM +0200, Stanislaw Gruszka wrote: > 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... <snip> > > 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(); After second thought I rather opted to fixup my small patch using above vector copy and add your's comment change since is nicer :-) The change of MainWindow::chooseCamera() make sense to me, but can be done separately as cleanup. Regards Stanislaw
On Tue, Oct 22, 2024 at 12:04:09PM +0200, Stanislaw Gruszka wrote: > On Mon, Oct 21, 2024 at 05:25:09PM +0200, Stanislaw Gruszka wrote: > > 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... > <snip> > > > > 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(); > > After second thought I rather opted to fixup my small patch using above vector > copy and add your's comment change since is nicer :-) My proposal was very much meant to generate first *and* second thoughts, so I'm happy it was useful for something :-) > The change of MainWindow::chooseCamera() make sense to me, but can be done > separately as cleanup. Fine with me.
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) + break; + cameraName = cam->id(); + } + } + if (num > 1) cameraName = chooseCamera(); if (cameraName == "")
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@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(-)