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

[Umang Jain]

Hi Niklas

On 9/30/20 3:42 PM, Niklas Söderlund wrote:
> Hi Umang and Kieran,
>
> On 2020-09-30 09:39:23 +0100, Kieran Bingham wrote:
>> 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 is more or less how it looked in v5 and all earlier versions of
> this series. Laurent commented in v5 that he only wanted model to be
> printed fro external cameras so that is what I do in this series.
Ah ok. I re-read it again and I think he is right. Sorry for the noise. 
Please keep the patch in the series :-)
>
>>> 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.
> Model is available for all cameras already (please compare the cover
> letter of this version and v4).
>
>> 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.
> I don't like to bikeshedd and have no strong opinion on the format
> camera names are printed in a test application. I'm however less excited
> that each new version of this series receive new ideas on how the name
> should look. I think I will drop this patch for the next version and
> then post it once the meat of this series is picked up.
>
>> --
>> Kieran
>>
>>
>>>>>> +        break;
>>>>>> +    }
>>>>>> +
>>>>>> +    name += " (" + camera->id() + ")";
>>>>>> +
>>>>>> +    return name;
>>>>>> +}
>>>>>> +
>>>>>>     void signalHandler([[maybe_unused]] int signal)
>>>>>>     {
>>>>>>         std::cout << "Exiting" << std::endl;
>> -- 
>> Regards
>> --
>> Kieran