qcam: Use pointer when choosing camera
diff mbox series

Message ID 20241030071739.271004-1-stanislaw.gruszka@linux.intel.com
State New
Headers show
Series
  • qcam: Use pointer when choosing camera
Related show

Commit Message

Stanislaw Gruszka Oct. 30, 2024, 7:17 a.m. UTC 
In order to remove redundant camera ID lookups and comparisons switch
to pointer-based checks when choosing and switching cameras.

Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 src/apps/qcam/main_window.cpp | 30 +++++++++++++-----------------
 src/apps/qcam/main_window.h   |  2 +-
 2 files changed, 14 insertions(+), 18 deletions(-)

Comments

Laurent Pinchart Oct. 30, 2024, 11:42 a.m. UTC | #1 
Hi Stanislaw,

Thank you for the patch.

On Wed, Oct 30, 2024 at 08:17:39AM +0100, Stanislaw Gruszka wrote:
> In order to remove redundant camera ID lookups and comparisons switch
> to pointer-based checks when choosing and switching cameras.
> 
> Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  src/apps/qcam/main_window.cpp | 30 +++++++++++++-----------------
>  src/apps/qcam/main_window.h   |  2 +-
>  2 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> index de487672..e0bbcc75 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,20 +280,21 @@ 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;
> +	std::string cameraName = "";
>  
>  	/*
>  	 * If a camera is specified on the command line, get it. Otherwise, if
> @@ -304,24 +303,21 @@ int MainWindow::openCamera()
>  	 */
>  	if (options_.isSet(OptCamera)) {
>  		cameraName = static_cast<std::string>(options_[OptCamera]);
> +		camera_ = cm_->get(cameraName);
>  	} else {
>  		std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
>  		if (cameras.size() == 1)
> -			cameraName = cameras[0]->id();
> +			camera_ = cameras[0];
>  		else
> -			cameraName = chooseCamera();
> +			camera_ = chooseCamera();
>  	}
>  
> -	if (cameraName == "")
> -		return -EINVAL;
> -
> -	/* Get and acquire the camera. */
> -	camera_ = cm_->get(cameraName);
>  	if (!camera_) {
>  		qInfo() << "Camera" << cameraName.c_str() << "not found";

This will print "Camera  not found" if the user presses the cancel
button in the selector dialog. Should we restrict printing the message
to the case where the camera name is specified on the command line ?
Something like

	if (options_.isSet(OptCamera)) {
		std::string name = static_cast<std::string>(options_[OptCamera]);
		camera_ = cm_->get(name);
		if (!camera_)
			qInfo() << "Camera" << name.c_str() << "not found";
	} else {
		std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
		if (cameras.size() == 1)
			camera_ = cameras[0];
		else
			camera_ = chooseCamera();
	}

	if (!camera_)
		return -ENODEV;

>  		return -ENODEV;
>  	}
>  
> +	/* Acquire the camera. */
>  	if (camera_->acquire()) {
>  		qInfo() << "Failed to acquire camera";
>  		camera_.reset();
> diff --git a/src/apps/qcam/main_window.h b/src/apps/qcam/main_window.h
> index 4cead734..81fcf915 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();
Stanislaw Gruszka Oct. 30, 2024, 4:20 p.m. UTC | #2 
Hi Laurent,

On Wed, Oct 30, 2024 at 01:42:33PM +0200, Laurent Pinchart wrote:
> Hi Stanislaw,
> 
> Thank you for the patch.
> 
> On Wed, Oct 30, 2024 at 08:17:39AM +0100, Stanislaw Gruszka wrote:
> > In order to remove redundant camera ID lookups and comparisons switch
> > to pointer-based checks when choosing and switching cameras.
> > 
> > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  src/apps/qcam/main_window.cpp | 30 +++++++++++++-----------------
> >  src/apps/qcam/main_window.h   |  2 +-
> >  2 files changed, 14 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> > index de487672..e0bbcc75 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,20 +280,21 @@ 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;
> > +	std::string cameraName = "";
> >  
> >  	/*
> >  	 * If a camera is specified on the command line, get it. Otherwise, if
> > @@ -304,24 +303,21 @@ int MainWindow::openCamera()
> >  	 */
> >  	if (options_.isSet(OptCamera)) {
> >  		cameraName = static_cast<std::string>(options_[OptCamera]);
> > +		camera_ = cm_->get(cameraName);
> >  	} else {
> >  		std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
> >  		if (cameras.size() == 1)
> > -			cameraName = cameras[0]->id();
> > +			camera_ = cameras[0];
> >  		else
> > -			cameraName = chooseCamera();
> > +			camera_ = chooseCamera();
> >  	}
> >  
> > -	if (cameraName == "")
> > -		return -EINVAL;
> > -
> > -	/* Get and acquire the camera. */
> > -	camera_ = cm_->get(cameraName);
> >  	if (!camera_) {
> >  		qInfo() << "Camera" << cameraName.c_str() << "not found";
> 
> This will print "Camera  not found" if the user presses the cancel

Yes, it's not nice, but I wanted some message when there is no camera ...

> button in the selector dialog. Should we restrict printing the message
> to the case where the camera name is specified on the command line ?
> Something like
> 
> 	if (options_.isSet(OptCamera)) {
> 		std::string name = static_cast<std::string>(options_[OptCamera]);
> 		camera_ = cm_->get(name);
> 		if (!camera_)
> 			qInfo() << "Camera" << name.c_str() << "not found";
> 	} else {
> 		std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
> 		if (cameras.size() == 1)
> 			camera_ = cameras[0];
> 		else
> 			camera_ = chooseCamera();

.. so I think here we can add message like below, to cover both branches:

	        if (!camara_)
 			qInfo() << "No camera detected";

Regards
Stanislaw

Patch
diff mbox series 

diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
index de487672..e0bbcc75 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,20 +280,21 @@  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;
+	std::string cameraName = "";
 
 	/*
 	 * If a camera is specified on the command line, get it. Otherwise, if
@@ -304,24 +303,21 @@  int MainWindow::openCamera()
 	 */
 	if (options_.isSet(OptCamera)) {
 		cameraName = static_cast<std::string>(options_[OptCamera]);
+		camera_ = cm_->get(cameraName);
 	} else {
 		std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
 		if (cameras.size() == 1)
-			cameraName = cameras[0]->id();
+			camera_ = cameras[0];
 		else
-			cameraName = chooseCamera();
+			camera_ = chooseCamera();
 	}
 
-	if (cameraName == "")
-		return -EINVAL;
-
-	/* Get and acquire the camera. */
-	camera_ = cm_->get(cameraName);
 	if (!camera_) {
 		qInfo() << "Camera" << cameraName.c_str() << "not found";
 		return -ENODEV;
 	}
 
+	/* Acquire the camera. */
 	if (camera_->acquire()) {
 		qInfo() << "Failed to acquire camera";
 		camera_.reset();
diff --git a/src/apps/qcam/main_window.h b/src/apps/qcam/main_window.h
index 4cead734..81fcf915 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();