| Message ID | 20200929144648.429397-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ragnatech.se> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Umang Jain <email@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` :) > + break; > + } > + > + name += " (" + camera->id() + ")"; > + > + return name; > +} > + > void signalHandler([[maybe_unused]] int signal) > { > std::cout << "Exiting" << std::endl;
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@ragnatech.se> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Umang Jain <email@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). >> + break; >> + } >> + >> + name += " (" + camera->id() + ")"; >> + >> + return name; >> +} >> + >> void signalHandler([[maybe_unused]] int signal) >> { >> std::cout << "Exiting" << std::endl; >
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@ragnatech.se> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> Reviewed-by: Umang Jain <email@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;
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@ragnatech.se> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> Reviewed-by: Umang Jain <email@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; >
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@ragnatech.se> > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> Reviewed-by: Umang Jain <email@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
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@ragnatech.se> >>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>>> Reviewed-by: Umang Jain <email@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
Hi Niklas, Thank you for the patch. On Tue, Sep 29, 2020 at 04:46:47PM +0200, 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 s/user friendly/user-friendly/ > 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> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Umang Jain <email@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); You can make this static. Apart from those two small issues, let's step the bikeshedding and work on top. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > 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) + "'"; > + break; > + } > + > + name += " (" + camera->id() + ")"; > + > + return name; > +} > + > void signalHandler([[maybe_unused]] int signal) > { > std::cout << "Exiting" << std::endl;
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) + "'"; + break; + } + + name += " (" + camera->id() + ")"; + + return name; +} + void signalHandler([[maybe_unused]] int signal) { std::cout << "Exiting" << std::endl;