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

21735 diff mbox series

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.

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>
---
v2:
- Avoid cm_->cameras().size() vs cm_->cameras()[0] race condition
  using custom loop.
- Update in code comment
v3: 
- Avoid TOCTOU race using shared_ptr vector copy
- Use Laurent comment wording

 src/apps/qcam/main_window.cpp | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Quoting Stanislaw Gruszka (2024-10-22 11:24:48)
> 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.
> 
> 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>
> ---
> v2:
> - Avoid cm_->cameras().size() vs cm_->cameras()[0] race condition
>   using custom loop.
> - Update in code comment
> v3: 
> - Avoid TOCTOU race using shared_ptr vector copy
> - Use Laurent comment wording
> 
>  src/apps/qcam/main_window.cpp | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> index 5144c6b3..de487672 100644
> --- a/src/apps/qcam/main_window.cpp
> +++ b/src/apps/qcam/main_window.cpp
> @@ -298,13 +298,19 @@ int MainWindow::openCamera()
>         std::string cameraName;
>  
>         /*
> -        * Use the camera specified on the command line, if any, or display the
> -        * camera selection dialog box otherwise.
> +        * 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))
> +       if (options_.isSet(OptCamera)) {
>                 cameraName = static_cast<std::string>(options_[OptCamera]);
> -       else
> -               cameraName = chooseCamera();
> +       } else {
> +               std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
> +               if (cameras.size() == 1)
> +                       cameraName = cameras[0]->id();
> +               else
> +                       cameraName = chooseCamera();
> +       }

Ok, now I see how the toctou is easier to fix like this ;-)

I like this - clean and short and does what it needs.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>         if (cameraName == "")
>                 return -EINVAL;
> -- 
> 2.43.0
>

On Tue, Oct 22, 2024 at 12:01:28PM +0100, Kieran Bingham wrote:
> Quoting Stanislaw Gruszka (2024-10-22 11:24:48)
> > 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.
> > 
> > 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>
> > ---
> > v2:
> > - Avoid cm_->cameras().size() vs cm_->cameras()[0] race condition
> >   using custom loop.
> > - Update in code comment
> > v3: 
> > - Avoid TOCTOU race using shared_ptr vector copy
> > - Use Laurent comment wording
> > 
> >  src/apps/qcam/main_window.cpp | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> > index 5144c6b3..de487672 100644
> > --- a/src/apps/qcam/main_window.cpp
> > +++ b/src/apps/qcam/main_window.cpp
> > @@ -298,13 +298,19 @@ int MainWindow::openCamera()
> >         std::string cameraName;
> >  
> >         /*
> > -        * Use the camera specified on the command line, if any, or display the
> > -        * camera selection dialog box otherwise.
> > +        * 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))
> > +       if (options_.isSet(OptCamera)) {
> >                 cameraName = static_cast<std::string>(options_[OptCamera]);
> > -       else
> > -               cameraName = chooseCamera();
> > +       } else {
> > +               std::vector<std::shared_ptr<Camera>> cameras = cm_->cameras();
> > +               if (cameras.size() == 1)
> > +                       cameraName = cameras[0]->id();

It's not very efficient to go from camera to name and then back to
camera, but I think that's fine for this patch. We can improve things on
top.

> > +               else
> > +                       cameraName = chooseCamera();
> > +       }
> 
> Ok, now I see how the toctou is easier to fix like this ;-)
> 
> I like this - clean and short and does what it needs.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  
> >         if (cameraName == "")
> >                 return -EINVAL;