[libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for selection of cameras at startup
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 8 22:38:21 CEST 2022
On Mon, Aug 08, 2022 at 10:49:20PM +0530, Utkarsh Tiwari via libcamera-devel wrote:
> On Mon, Aug 8, 2022 at 2:15 PM Kieran Bingham wrote:
> > Quoting Utkarsh Tiwari (2022-08-08 07:56:41)
> > > On Mon, 8 Aug, 2022, 04:49 Kieran Bingham wrote:
> > > > Quoting Utkarsh Tiwari via libcamera-devel (2022-08-06 20:04:30)
> > > > > Currently we use QInputDialog convenience dialogs to allow the user to
> > > > > select a camera. This doesn't allow adding of more information (such as
> > > > > camera location, model etc).
> > > > >
> > > > > Create a QDialog with a QFormLayout that shows a QComboBox with camera
> > > > > Ids. Use a QDialogButtonBox to provide buttons for accepting and
> > > > > cancelling the action.
> > > > >
> > > > > Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> > > > > ---
> > > > > Difference from V1:
> > > > > The biggest change here is the introduction of class, inside of
> > > > > the class the layout remains same.
> > > > > src/qcam/cam_select_dialog.h | 64 ++++++++++++++++++++++++++++++++++++
> > > > > src/qcam/main_window.cpp | 22 +++----------
> > > > > src/qcam/main_window.h | 4 +++
> > > > > src/qcam/meson.build | 1 +
> > > > > 4 files changed, 74 insertions(+), 17 deletions(-)
> > > > > create mode 100644 src/qcam/cam_select_dialog.h
> > > > >
> > > > > diff --git a/src/qcam/cam_select_dialog.h b/src/qcam/cam_select_dialog.h
> > > > > new file mode 100644
> > > > > index 00000000..c23bad59
> > > > > --- /dev/null
> > > > > +++ b/src/qcam/cam_select_dialog.h
> > > > > @@ -0,0 +1,64 @@
> > > > > +
> > > >
> > > > Remove the first blank line please.
> > > >
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2019, Google Inc.
> > > >
> > > > I think this should be 2022. I'm not sure if copyright goes to you or
> > > > Google for GSoC.
> > >
> > > Oops copy paste error, It's my copyright.
> > >
> > > > > + *
> > > > > + * cam_select_dialog.h - qcam - Camera Selection dialog
> > > > > + */
> > > > > +
> > > > > +#pragma once
> > > > > +
> > > > > +#include <string>
> > > > > +
> > > > > +#include <libcamera/camera.h>
> > > > > +#include <libcamera/camera_manager.h>
> > > > > +
> > > > > +#include <QComboBox>
> > > > > +#include <QDialog>
> > > > > +#include <QDialogButtonBox>
> > > > > +#include <QFormLayout>
> > > > > +#include <QString>
> > > > > +
> > > > > +class CamSelectDialog : public QDialog
> > > >
> > > > It's bikeshedding territory - but CameraSelectDialog sounds better to me
> > > > here. (Spell 'Camera' in full).
> > > >
> > > > It seems Laurent suggested CameraSelectorDialog too.
> > >
> > > Fine by me.
> > >
> > > > > +{
> > > > > + Q_OBJECT
> > > > > +public:
> > > > > + CamSelectDialog(libcamera::CameraManager *cameraManager, QWidget *parent)
> > > > > + : QDialog(parent), cm_(cameraManager)
> > > >
> > > > Is cm_ used later? I don't mind it being added here if it's needed
> > > > later, but it only seems to be used in this construction for the moment.
> > >
> > > Yes, to show the camera info I need to use cm_ to get the camera.
> > >
> > > > > + {
> > > > > + /* Use a QFormLayout for the dialog. */
> > > > > + QFormLayout *camSelectDialogLayout = new QFormLayout(this);
You can simply call the variable "layout".
> > > > > +
> > > > > + /* Setup the camera id combo-box. */
> > > > > + cameraIdComboBox_ = new QComboBox;
> > > > > + for (const auto &cam : cm_->cameras())
> > > > > + cameraIdComboBox_->addItem(QString::fromStdString(cam->id()));
> > > >
> > > > I could imagine we need to add hotplug events in here too.
> > > > It could be done on top, but to test it - try removing or adding a
> > > > camera while the dialog box is opened.
> > > >
> > > > As long as it doesn't crash if the camera is removed while the dialog
> > > > box is open, I don't mind if the hotplug (adding cameras when they are
> > > > plugged in, removing when removed) is added on top. Especially as the
> > > > existing dialog didn't support hotplug either.
> > > >
> > > > I guess if there's no camera left, the dialog box doesn't have much to
> > > > do ;-) (though should stay open in case someone replugs the UVC camera)
> > >
> > > Yes saw your other reply, and yes it's done in patch 2nd.
> > >
> > > > > +
> > > > > + /* Setup the QDialogButton Box */
> > > > > + QDialogButtonBox *dialogButtonBox =
And this can be called "buttonBox" or even just "buttons". There's no
need for the "dialog" prefix in the name as all these variables are
within the scope of CamSelectDialog.
> > > > > + new QDialogButtonBox(QDialogButtonBox::Ok |
> > > > > + QDialogButtonBox::Cancel);
> > > > > +
> > > > > + connect(dialogButtonBox, &QDialogButtonBox::accepted,
> > > > > + this, &QDialog::accept);
> > > > > + connect(dialogButtonBox, &QDialogButtonBox::rejected,
> > > > > + this, &QDialog::reject);
> > > > > +
> > > > > + /* Set the layout. */
> > > > > + camSelectDialogLayout->addRow("Camera: ", cameraIdComboBox_);
No need for a space after ':'.
> > > > > + camSelectDialogLayout->addWidget(dialogButtonBox);
> > > > > + }
> > > > > +
> > > > > + ~CamSelectDialog() = default;
> > > > > +
> > > > > + std::string getCameraId()
> > > > > + {
> > > > > + return cameraIdComboBox_->currentText().toStdString();
> > > > > + }
> > > >
> > > > While there's not a lot of implementation here, I think the code should
> > > > be in a .cpp file. I'm not sure where the threshold is for how small a
> > > > class can be to be inlined directly in the header, but I think this one
> > > > can potentially expand, so probably deserves it's own implementation
> > > > file.
I was about to say the same, the implementation should move to
cam_select_dialog.cpp.
> > > Sure, I would wait on review for the other patches then move everything
> > > to its .cpp.
> > >
> > > > > +
> > > > > +private:
> > > > > + libcamera::CameraManager *cm_;
> > > > > +
> > > > > + /* UI elements. */
> > > > > + QComboBox *cameraIdComboBox_;
> > > > > +};
> > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > > > index 7433d647..758e2c94 100644
> > > > > --- a/src/qcam/main_window.cpp
> > > > > +++ b/src/qcam/main_window.cpp
> > > > > @@ -19,7 +19,6 @@
> > > > > #include <QFileDialog>
> > > > > #include <QImage>
> > > > > #include <QImageWriter>
> > > > > -#include <QInputDialog>
> > > > > #include <QMutexLocker>
> > > > > #include <QStandardPaths>
> > > > > #include <QStringList>
> > > > > @@ -30,6 +29,7 @@
> > > > >
> > > > > #include "../cam/image.h"
> > > > >
> > > > > +#include "cam_select_dialog.h"
> > > > > #include "dng_writer.h"
> > > > > #ifndef QT_NO_OPENGL
> > > > > #include "viewfinder_gl.h"
> > > > > @@ -290,24 +290,12 @@ void MainWindow::switchCamera(int index)
> > > > >
> > > > > std::string MainWindow::chooseCamera()
> > > > > {
> > > > > - QStringList cameras;
> > > > > - bool result;
> > > > > + camSelectDialog_ = new CamSelectDialog(cm_, this);
> > > >
> > > > Do we need to keep the dialog in a class variable? or could it be local
> > > > and destroyed at the end of this function?
> > >
> > > Yes we need it as a class variable for camera hotplugging.
> >
> > This seems to recreate the CamSelectDialog box each time from scratch,
> > even though we constructed it - and keep it updated with hotplug events.
> >
> > Is it possible to re-use it when it's created? or does accepting the
> > dialog or closing it make it no longer possible to re-use?
> >
> > If we can re-use it - we should ... Otherwise - we probably shouldn't
> > send hotplug events when it's been closed. ?
>
> I think we can re-use it. And I think the current way is causing a leak
> :P.
Reusing the dialog would be best indeed. Also, why do you use a QPointer
?
> > > > > - /* If only one camera is available, use it automatically. */
> > > > > - if (cm_->cameras().size() == 1)
> > > > > - return cm_->cameras()[0]->id();
> > > > > -
> > > > > - /* Present a dialog box to pick a camera. */
> > > > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > > > > - cameras.append(QString::fromStdString(cam->id()));
> > > > > -
> > > > > - QString id = QInputDialog::getItem(this, "Select Camera",
> > > > > - "Camera:", cameras, 0,
> > > > > - false, &result);
> > > > > - if (!result)
> > > > > + if (camSelectDialog_->exec() == QDialog::Accepted)
> > > > > + return camSelectDialog_->getCameraId();
> > > > > + else
> > > > > return std::string();
> > > > > -
> > > > > - return id.toStdString();
> > > > > }
> > > > >
> > > > > int MainWindow::openCamera()
> > > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > > > index fc70920f..6d80b5be 100644
> > > > > --- a/src/qcam/main_window.h
> > > > > +++ b/src/qcam/main_window.h
> > > > > @@ -23,11 +23,13 @@
> > > > > #include <QMainWindow>
> > > > > #include <QMutex>
> > > > > #include <QObject>
> > > > > +#include <QPointer>
> > > > > #include <QQueue>
> > > > > #include <QTimer>
> > > > >
> > > > > #include "../cam/stream_options.h"
> > > > >
> > > > > +#include "cam_select_dialog.h"
> > > > > #include "viewfinder.h"
> > > > >
> > > > > class QAction;
> > > > > @@ -99,6 +101,8 @@ private:
> > > > > QString title_;
> > > > > QTimer titleTimer_;
> > > > >
> > > > > + QPointer<CamSelectDialog> camSelectDialog_;
> > > > > +
> > > > > /* Options */
> > > > > const OptionsParser::Options &options_;
> > > > >
> > > > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > > > > index c46f4631..70615e92 100644
> > > > > --- a/src/qcam/meson.build
> > > > > +++ b/src/qcam/meson.build
> > > > > @@ -26,6 +26,7 @@ qcam_sources = files([
> > > > > ])
> > > > >
> > > > > qcam_moc_headers = files([
> > > > > + 'cam_select_dialog.h',
> > > > > 'main_window.h',
> > > > > 'viewfinder_qt.h',
> > > > > ])
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list