[libcamera-devel] [RFC PATCH] Add CameraName to the MainWindow
Umang Jain
umang.jain at ideasonboard.com
Mon Mar 21 15:23:15 CET 2022
Hi Kieran,
On 3/21/22 18:34, Kieran Bingham wrote:
> 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.
The previous discussions were around a uniquely-defined camera-id
instead of camera-name as far as I remember.
>
> 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.
Precisely. Applications are free to append/structure the camera-name as
their will - but a helper from libcamera might help as a base reference
on what we might think as the best option.
>
> 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