[PATCH] qcam: Use pointer when choosing camera

[Stanislaw Gruszka]

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 at ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at 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