[libcamera-devel] [PATCH v6 6/7] cam: Print user-friendly camera names

[Umang Jain]

Hi Kieran

On 9/30/20 1:44 PM, Kieran Bingham wrote:
> Hi Umang,
>
> On 30/09/2020 06:32, Umang Jain wrote:
>> Hi Niklas,
>>
>> On 9/29/20 8:16 PM, Niklas Söderlund wrote:
>>> Instead of only printing the camera ID which is not intended for humans
>>> to read and parse create a more user friendly string when printing
>>> camera names. The ID is still printed as it is one option used to select
>>> camera using the --camera option.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>> Reviewed-by: Umang Jain <email at uajain.com>
>>> ---
>>> * Changes since v5
>>> - Rework camera name.
>>>
>>> * Changes since v4
>>> - Make cameraName() member of CamApp.
>>>
>>> * Changes since v1
>>> - Only print user-friendly names when listing cameras.
>>> - Update format of user-friendly names printed.
>>> - Update commit message.
>>> ---
>>>    src/cam/main.cpp | 28 +++++++++++++++++++++++++++-
>>>    1 file changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
>>> index 244720b491f5c462..311df171a21f6152 100644
>>> --- a/src/cam/main.cpp
>>> +++ b/src/cam/main.cpp
>>> @@ -45,6 +45,8 @@ private:
>>>        int infoConfiguration();
>>>        int run();
>>>    +    std::string cameraName(const Camera *camera);
>>> +
>>>        static CamApp *app_;
>>>        OptionsParser::Options options_;
>>>        CameraManager *cm_;
>>> @@ -340,7 +342,7 @@ int CamApp::run()
>>>              unsigned int index = 1;
>>>            for (const std::shared_ptr<Camera> &cam : cm_->cameras()) {
>>> -            std::cout << index << ": " << cam->id() << std::endl;
>>> +            std::cout << index << ": " << cameraName(cam.get()) <<
>>> std::endl;
>>>                index++;
>>>            }
>>>        }
>>> @@ -378,6 +380,30 @@ int CamApp::run()
>>>        return 0;
>>>    }
>>>    +std::string CamApp::cameraName(const Camera *camera)
>>> +{
>>> +    const ControlList &props = camera->properties();
>>> +    std::string name;
>>> +
>>> +    switch (props.get(properties::Location)) {
>>> +    case properties::CameraLocationFront:
>>> +        name = "Internal front camera";
>>> +        break;
>>> +    case properties::CameraLocationBack:
>>> +        name = "Internal back camera";
>>> +        break;
>>> +    case properties::CameraLocationExternal:
>>> +        name = "External camera";
>>> +        if (props.contains(properties::Model))
>>> +            name += " '" + props.get(properties::Model) + "'";
>> Quick comment: Can we probably move this out of the switch block? I
>> understand, as of now, only UVC will return the model property, but
>> application's point-of-view, I don't think we need to carry that
>> implementation detail in cam itself. As and when the reign the model
>> property expands in libcamera, it shall auto-magically start to show up
>> via `cam -l` :)
> Wouldn't it be quite difficult to identify UVC cameras in the system
> then? It's currently quite rare to have more than one front or back
> camera (at least in systems we're developing with) but my laptop has 2
> UVC devices internally. If they both simply say "External camera" it
> would be quite hard to know which is the face-detect camera, and which
> is the 'real' camera.
>
> Or maybe it doesn't matter - perhaps 'face detect' is another feature to
> express in the properties... (but we can only guess that if the only
> format is R8/ greyscale, I think).
Not sure if I follow clearly, What I am suggesting is something on the 
lines:

+std::string CamApp::cameraName(const Camera *camera)
+{
+    const ControlList &props = camera->properties();
+    std::string name;
+
+    switch (props.get(properties::Location)) {
+    case properties::CameraLocationFront:
+        name = "Internal front camera";
+        break;
+    case properties::CameraLocationBack:
+        name = "Internal back camera";
+        break;
+    case properties::CameraLocationExternal:
+        name = "External camera";
+        break;
+    }
+
+    if (props.contains(properties::Model))
+        name += " '" + props.get(properties::Model) + "'";
+
+    name += " (" + camera->id() + ")";
+
+    return name;
+}

This way, as and when the support for model property improves in the 
future, `cam -l` will tell us about it (without requiring to introduce 
any patch up code  here). If it is absent (as of now), so be it.
>
>>> +        break;
>>> +    }
>>> +
>>> +    name += " (" + camera->id() + ")";
>>> +
>>> +    return name;
>>> +}
>>> +
>>>    void signalHandler([[maybe_unused]] int signal)
>>>    {
>>>        std::cout << "Exiting" << std::endl;