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

[Niklas Söderlund]

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.

> > 
> > 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

-- 
Regards,
Niklas Söderlund