[libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 21 14:04:16 CET 2022
Quoting Umang Jain via libcamera-devel (2022-03-21 06:33:47)
> Hi Utkarsh
>
> Thank you for the patch.
>
> On 3/19/22 19:55, Utkarsh Tiwari via libcamera-devel wrote:
> > Showing Camera Id on the main window is quite non-intuitive for the user
> >
> > Implementation: A duplicate function exists in cam/main.cpp . There must be a better way to do this instead of duplication of code
A cleaner commit message could look something like:
"""
qcam: Display a human readable name in camera selection
Showing the Camera ID on the main window and when selecting a camera is
not very intuitive for the user.
Camera IDs are unique references for the camera and include internal
details that do not present an easy indicator for which camera it
represents.
Construct a Camera Name based on the camera properties by duplicating
the existing naming scheme from cam to present more understandable
device name to users of Qcam.
"""
We try to make sure the commit title clearly references which part of
libcamera is being changed, (in this case 'qcam:'), and a commit message
should try to clearly state the problem it's solving, and how it solves
it.
> > Signed-off-by: Utkarsh Tiwari <utkarsh02t at gmail.com>
> > ---
> > src/qcam/main_window.cpp | 61 +++++++++++++++++++++++++++++++++-------
> > src/qcam/main_window.h | 2 +-
> > 2 files changed, 52 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index dd0e51f5..35e737c7 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -12,8 +12,10 @@
> > #include <string>
> >
> > #include <libcamera/camera_manager.h>
> > +#include <libcamera/property_ids.h>
> > #include <libcamera/version.h>
> >
> > +
> spurious new line
> > #include <QComboBox>
> > #include <QCoreApplication>
> > #include <QFileDialog>
> > @@ -197,7 +199,7 @@ int MainWindow::createToolbars()
> > this, &MainWindow::switchCamera);
> >
> > for (const std::shared_ptr<Camera> &cam : cm_->cameras())
> > - cameraCombo_->addItem(QString::fromStdString(cam->id()));
> > + cameraCombo_->addItem(QString::fromStdString(cameraName(cam.get())));
I think if MainWindow::cameraName() took a shared_ptr:
std::string MainWindow::cameraName(const std::shared_ptr<Camera> &camera)
then this doesn't have to '.get()' the ptr, it would just pass 'cam',
and the cameraName function wouldn't have to otherwise change.
> >
> > toolbar_->addWidget(cameraCombo_);
> >
> > @@ -287,6 +289,45 @@ void MainWindow::switchCamera(int index)
> > startStopAction_->setChecked(true);
> > }
> >
> > +std::string MainWindow::cameraName(const libcamera::Camera *camera)
> > +{
> > + const ControlList &props = camera->properties();
> > + bool addModel = true;
> > + std::string name;
> > +
> > + /*
> > + * Construct the name from the camera location, model and ID. The model
> > + * is only used if the location isn't present or is set to External.
> > + */
> > + if (props.contains(properties::Location)) {
> > + switch (props.get(properties::Location)) {
> > + case properties::CameraLocationFront:
> > + addModel = false;
> > + name = "Internal front camera ";
> > + break;
> > + case properties::CameraLocationBack:
> > + addModel = false;
> > + name = "Internal back camera ";
> > + break;
> > + case properties::CameraLocationExternal:
> > + name = "External camera ";
> > + break;
> > + }
> > + }
> > +
> > + if (addModel && props.contains(properties::Model)) {
> > + /*
> > + * If the camera location is not availble use the camera model
> > + * to build the camera name.
> > + */
> > + name = "'" + props.get(properties::Model) + "' ";
> > + }
> > +
> > + name += "(" + camera->id() + ")";
> > +
> > + return name;
> > +}
> > +
>
>
> I get it that this is copied over from cam/main.cpp but since we are
> trying to keep the camera names construction in-sync across applications
> (cam and qcam), have you thought or discussed the possibility that this
> helper can go in libcamera::Camera class itself ? For e.g. as
>
> Camera::name()
>
> OR
>
> Camera::displayName()
>
> as a helper provided by libcamera. Applications not happy with this
> helper (given out by libcamera) can still construct/append their own
> quirks based on id(), location-property and so on.. as it is done right
> now anyway.
>
> I don't know, this might require some discussion but would simplify
> things a bit.
Defining how to name a camera is supposed to be up to the application,
not libcamera, and there has indeed been some previous discussions about
this.
I wonder if providing a helper that does what we expect is useful
though, and doesn't prevent applications from deciding to copy/or change
that themselves as long as all the properties used are public anyway.
But I think that means that duplicating the code is acceptable for now.
> > std::string MainWindow::chooseCamera()
> > {
> > QStringList cameras;
> > @@ -311,24 +352,24 @@ std::string MainWindow::chooseCamera()
> >
> > int MainWindow::openCamera()
> > {
> > - std::string cameraName;
> > + std::string cameraId;
> >
> > /*
> > * Use the camera specified on the command line, if any, or display the
> > * camera selection dialog box otherwise.
> > */
> > if (options_.isSet(OptCamera))
> > - cameraName = static_cast<std::string>(options_[OptCamera]);
> > + cameraId = static_cast<std::string>(options_[OptCamera]);
> > else
> > - cameraName = chooseCamera();
> > + cameraId = chooseCamera();
> >
> > - if (cameraName == "")
> > + if (cameraId == "")
> > return -EINVAL;
> >
> > /* Get and acquire the camera. */
> > - camera_ = cm_->get(cameraName);
> > + camera_ = cm_->get(cameraId);
> > if (!camera_) {
> > - qInfo() << "Camera" << cameraName.c_str() << "not found";
> > + qInfo() << "Camera" << cameraId.c_str() << "not found";
> > return -ENODEV;
> > }
>
>
> This hunk is a variable rename so should be separated out in a different
> patch I think.
Maybe not essential, but certainly would be possible and cleaner I think
to have this rename done as a first patch.
>
> >
> > @@ -339,7 +380,7 @@ int MainWindow::openCamera()
> > }
> >
> > /* Set the combo-box entry with the currently selected Camera. */
> > - cameraCombo_->setCurrentText(QString::fromStdString(cameraName));
> > + cameraCombo_->setCurrentText(QString::fromStdString(cameraName(camera_.get())));
> >
> > return 0;
> > }
> > @@ -604,7 +645,7 @@ void MainWindow::processHotplug(HotplugEvent *e)
> > HotplugEvent::PlugEvent event = e->hotplugEvent();
> >
> > if (event == HotplugEvent::HotPlug) {
> > - cameraCombo_->addItem(QString::fromStdString(camera->id()));
> > + cameraCombo_->addItem(QString::fromStdString(cameraName(camera)));
> > } else if (event == HotplugEvent::HotUnplug) {
> > /* Check if the currently-streaming camera is removed. */
> > if (camera == camera_.get()) {
> > @@ -614,7 +655,7 @@ void MainWindow::processHotplug(HotplugEvent *e)
> > cameraCombo_->setCurrentIndex(0);
> > }
> >
> > - int camIndex = cameraCombo_->findText(QString::fromStdString(camera->id()));
> > + int camIndex = cameraCombo_->findText(QString::fromStdString(cameraName(camera)));
> > cameraCombo_->removeItem(camIndex);
> > }
> > }
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 3fbe872c..b31c584f 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -70,7 +70,7 @@ private Q_SLOTS:
> >
> > private:
> > int createToolbars();
> > -
> Spurious deletion of newline. It should be retained
> > + std::string cameraName(const libcamera::Camera *camera);
> > std::string chooseCamera();
> > int openCamera();
> >
More information about the libcamera-devel
mailing list