[libcamera-devel] [PATCH v2 1/4] qcam: Use QDialog for selection of cameras at startup
Utkarsh Tiwari
utkarsh02t at gmail.com
Mon Aug 8 19:19:20 CEST 2022
Hi Kieran thanks for the review.
On Mon, Aug 8, 2022 at 2:15 PM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> Quoting Utkarsh Tiwari (2022-08-08 07:56:41)
> > Hi Kieran thanks for the review.
> >
> > On Mon, 8 Aug, 2022, 04:49 Kieran Bingham, <
> kieran.bingham at ideasonboard.com>
> > 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);
> > > > +
> > > > + /* 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 =
> > > > + 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_);
> > > > + 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.
> > >
> > > 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.
>
> >
> > >
> > > >
> > > > - /* 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',
> > > > ])
> > > > --
> > > > 2.25.1
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220808/0eee6706/attachment.htm>
More information about the libcamera-devel
mailing list