| Message ID | 20200813095726.3497193-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On 13/08/2020 10:57, 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@ragnatech.se> > --- > * 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 | 33 ++++++++++++++++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index cc3facd5a5b22092..57f8c79b21090ece 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -21,6 +21,37 @@ > > using namespace libcamera; > > +std::string cameraName(const Camera *camera) > +{ > + const ControlList &props = camera->properties(); > + std::string name; > + > + /* Use camera model as fallback name if available. */ > + if (props.contains(properties::Model)) > + name = props.get(properties::Model); > + else > + name = "Unknown camera"; > + > + /* If camera location is available use it as highest priority name. */ > + if (props.contains(properties::Location)) { > + 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; > + } > + } Err, aren't we going to make UVC cameras all marked as CameraLocationExternal? I'm not sure I'd use this as the 'highest' priority, more as the lowest priority if the model property wasn't known but the location was... Though I don't think we'll ever not have a model would we ? > + > + name += " (" + camera->id() + ")"; > + > + return name; > +} > + > class CamApp > { > public: > @@ -340,7 +371,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++; > } > } >
Hi Kieran, Thanks for your feedback. On 2020-08-13 16:49:26 +0100, Kieran Bingham wrote: > Hi Niklas, > > On 13/08/2020 10:57, 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@ragnatech.se> > > --- > > * 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 | 33 ++++++++++++++++++++++++++++++++- > > 1 file changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index cc3facd5a5b22092..57f8c79b21090ece 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -21,6 +21,37 @@ > > > > using namespace libcamera; > > > > +std::string cameraName(const Camera *camera) > > +{ > > + const ControlList &props = camera->properties(); > > + std::string name; > > + > > + /* Use camera model as fallback name if available. */ > > + if (props.contains(properties::Model)) > > + name = props.get(properties::Model); > > + else > > + name = "Unknown camera"; > > + > > + /* If camera location is available use it as highest priority name. */ > > + if (props.contains(properties::Location)) { > > + 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; > > + } > > + } > > > Err, aren't we going to make UVC cameras all marked as > CameraLocationExternal? > > I'm not sure I'd use this as the 'highest' priority, more as the lowest > priority if the model property wasn't known but the location was... Laurent made a good case for having location front and back as highest priority as it makes most sens for users, and I would like to keep this. Model really is only useful for us in this case. I agree with the proposed change to mark all UVC cameras as external vs not giving them a location maybe properties::Model should be used instead of "External camera"? As I commented on the series which changes this for the UVC pipeline I'm not sure location is the best property to introduce to UVC as what that series really want is to know if the camera is hot-pluggable nor not. How about we keep this as it is for now and adopt it once we know how the other series turns out? I feel chancing this patch now for something we don't know might be busy work. > > Though I don't think we'll ever not have a model would we ? We will always have a model, but it's not enforced (yet). > > > > > > + > > + name += " (" + camera->id() + ")"; > > + > > + return name; > > +} > > + > > class CamApp > > { > > public: > > @@ -340,7 +371,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++; > > } > > } > > > > -- > Regards > -- > Kieran
Hi Niklas, On 13/08/2020 16:59, Niklas Söderlund wrote: > Hi Kieran, > > Thanks for your feedback. > > On 2020-08-13 16:49:26 +0100, Kieran Bingham wrote: >> Hi Niklas, >> >> On 13/08/2020 10:57, 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@ragnatech.se> >>> --- >>> * 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 | 33 ++++++++++++++++++++++++++++++++- >>> 1 file changed, 32 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp >>> index cc3facd5a5b22092..57f8c79b21090ece 100644 >>> --- a/src/cam/main.cpp >>> +++ b/src/cam/main.cpp >>> @@ -21,6 +21,37 @@ >>> >>> using namespace libcamera; >>> >>> +std::string cameraName(const Camera *camera) >>> +{ >>> + const ControlList &props = camera->properties(); >>> + std::string name; >>> + >>> + /* Use camera model as fallback name if available. */ >>> + if (props.contains(properties::Model)) >>> + name = props.get(properties::Model); >>> + else >>> + name = "Unknown camera"; >>> + >>> + /* If camera location is available use it as highest priority name. */ >>> + if (props.contains(properties::Location)) { >>> + 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; >>> + } >>> + } >> >> >> Err, aren't we going to make UVC cameras all marked as >> CameraLocationExternal? >> >> I'm not sure I'd use this as the 'highest' priority, more as the lowest >> priority if the model property wasn't known but the location was... > > Laurent made a good case for having location front and back as highest > priority as it makes most sens for users, and I would like to keep this. > Model really is only useful for us in this case. > > I agree with the proposed change to mark all UVC cameras as external vs > not giving them a location maybe properties::Model should be used > instead of "External camera"? > > As I commented on the series which changes this for the UVC pipeline I'm > not sure location is the best property to introduce to UVC as what that > series really want is to know if the camera is hot-pluggable nor not. > How about we keep this as it is for now and adopt it once we know how > the other series turns out? I feel chancing this patch now for something > we don't know might be busy work. Ok, that's fine with me, with these current patches I get: > Available cameras: > 1: VF0520 Live! Cam Sync: VF0520 L (\_SB_.PCI0.XHC_.RHUB.HS01-1:1.0-041e:406c) > 2: HP Wide Vision FHD Camera: HP W (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251) > 3: HP Wide Vision FHD Camera: HP I (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.2-0408:5251) Which looks good to me, I see the name I expect, and a unique identifier. In particular this helps me identify which is the IR camera and which is the normal. If this were to change to external it would become quite awkard though: > Available cameras: > 1: External camera (\_SB_.PCI0.XHC_.RHUB.HS01-1:1.0-041e:406c) > 2: External camera (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251) > 3: External camera (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.2-0408:5251) At that point, I'm not sure "External camera" shows any benefit at all ;-) (and indeed, camera 2 and 3 are 'internal' on this laptop ....) Anyway, with these patches the output looks good to me, so I agree we'll deal with the hotplug issues separately. Given how easy it is to add properties, I can foresee a 'hotpluggable' property being a good way to resolve that, rather than using the inaccurate location. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Though I don't think we'll ever not have a model would we ? > > We will always have a model, but it's not enforced (yet). Ok, that sounds fine for now. > >> >> >> >> >>> + >>> + name += " (" + camera->id() + ")"; >>> + >>> + return name; >>> +} >>> + >>> class CamApp >>> { >>> public: >>> @@ -340,7 +371,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++; >>> } >>> } >>> >> >> -- >> Regards >> -- >> Kieran >
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index cc3facd5a5b22092..57f8c79b21090ece 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -21,6 +21,37 @@ using namespace libcamera; +std::string cameraName(const Camera *camera) +{ + const ControlList &props = camera->properties(); + std::string name; + + /* Use camera model as fallback name if available. */ + if (props.contains(properties::Model)) + name = props.get(properties::Model); + else + name = "Unknown camera"; + + /* If camera location is available use it as highest priority name. */ + if (props.contains(properties::Location)) { + 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; + } + } + + name += " (" + camera->id() + ")"; + + return name; +} + class CamApp { public: @@ -340,7 +371,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++; } }
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@ragnatech.se> --- * 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 | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)