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

[Kieran Bingham]

Heya,

On 30/09/2020 09:28, Umang Jain wrote:
> 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.

Aha, I see - that makes more sense now - sorry I didn't understand at first.

Well, it seems a good idea to me - but I'll leave Niklas' to respond on
that, as I know he's already trying to manage the many
bikeshedding/naming schemes on this part.

--
Kieran


>>
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    name += " (" + camera->id() + ")";
>>>> +
>>>> +    return name;
>>>> +}
>>>> +
>>>>    void signalHandler([[maybe_unused]] int signal)
>>>>    {
>>>>        std::cout << "Exiting" << std::endl;
> 

-- 
Regards
--
Kieran