| Message ID | 20190617100910.10879-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Jun 17, 2019 at 01:09:10PM +0300, Laurent Pinchart wrote: > As camera names can be cumbersome to type, selection of cameras by index > from a list can be useful. Print the list of detected cameras with an > index for each item, and interpret the camera name as an index if it is > a numerical value in the range from 1 to the number of cameras. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> and tested on IPU3 Soraka. Thanks j > --- > src/cam/main.cpp | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index f03a9faf87fa..0e7610b1befd 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -73,7 +73,14 @@ int CamApp::init(int argc, char **argv) > } > > if (options_.isSet(OptCamera)) { > - camera_ = cm_->get(options_[OptCamera]); > + const std::string &cameraName = options_[OptCamera]; > + char *endptr; > + unsigned long index = strtoul(cameraName.c_str(), &endptr, 10); > + if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size()) > + camera_ = cm_->cameras()[index - 1]; > + else > + camera_ = cm_->get(cameraName); > + > if (!camera_) { > std::cout << "Camera " > << std::string(options_[OptCamera]) > @@ -172,8 +179,12 @@ int CamApp::run() > { > if (options_.isSet(OptList)) { > std::cout << "Available cameras:" << std::endl; > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > - std::cout << "- " << cam->name() << std::endl; > + > + unsigned int index = 1; > + for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > + std::cout << index << ": " << cam->name() << std::endl; > + index++; > + } > } > > if (options_.isSet(OptCapture)) { > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your work. On 2019-06-17 13:09:10 +0300, Laurent Pinchart wrote: > As camera names can be cumbersome to type, selection of cameras by index > from a list can be useful. Print the list of detected cameras with an > index for each item, and interpret the camera name as an index if it is > a numerical value in the range from 1 to the number of cameras. I think you should also update the usage information for --camera to indicate both camera name and index can be used to select a camera. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/main.cpp | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index f03a9faf87fa..0e7610b1befd 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -73,7 +73,14 @@ int CamApp::init(int argc, char **argv) > } > > if (options_.isSet(OptCamera)) { > - camera_ = cm_->get(options_[OptCamera]); > + const std::string &cameraName = options_[OptCamera]; > + char *endptr; > + unsigned long index = strtoul(cameraName.c_str(), &endptr, 10); > + if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size()) > + camera_ = cm_->cameras()[index - 1]; > + else > + camera_ = cm_->get(cameraName); > + > if (!camera_) { > std::cout << "Camera " > << std::string(options_[OptCamera]) > @@ -172,8 +179,12 @@ int CamApp::run() > { > if (options_.isSet(OptList)) { > std::cout << "Available cameras:" << std::endl; > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > - std::cout << "- " << cam->name() << std::endl; > + > + unsigned int index = 1; > + for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > + std::cout << index << ": " << cam->name() << std::endl; > + index++; > + } > } > > if (options_.isSet(OptCapture)) { > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Tue, Jun 18, 2019 at 09:44:42AM +0200, Niklas Söderlund wrote: > On 2019-06-17 13:09:10 +0300, Laurent Pinchart wrote: > > As camera names can be cumbersome to type, selection of cameras by index > > from a list can be useful. Print the list of detected cameras with an > > index for each item, and interpret the camera name as an index if it is > > a numerical value in the range from 1 to the number of cameras. > > I think you should also update the usage information for --camera to > indicate both camera name and index can be used to select a camera. Good point ! Would the following be enough ? parser.addOption(OptCamera, OptionString, - "Specify which camera to operate on", "camera", + "Specify which camera to operate on, by name or by index", "camera", ArgumentRequired, "camera"); > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/cam/main.cpp | 17 ++++++++++++++--- > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index f03a9faf87fa..0e7610b1befd 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -73,7 +73,14 @@ int CamApp::init(int argc, char **argv) > > } > > > > if (options_.isSet(OptCamera)) { > > - camera_ = cm_->get(options_[OptCamera]); > > + const std::string &cameraName = options_[OptCamera]; > > + char *endptr; > > + unsigned long index = strtoul(cameraName.c_str(), &endptr, 10); > > + if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size()) > > + camera_ = cm_->cameras()[index - 1]; > > + else > > + camera_ = cm_->get(cameraName); > > + > > if (!camera_) { > > std::cout << "Camera " > > << std::string(options_[OptCamera]) > > @@ -172,8 +179,12 @@ int CamApp::run() > > { > > if (options_.isSet(OptList)) { > > std::cout << "Available cameras:" << std::endl; > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > - std::cout << "- " << cam->name() << std::endl; > > + > > + unsigned int index = 1; > > + for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > + std::cout << index << ": " << cam->name() << std::endl; > > + index++; > > + } > > } > > > > if (options_.isSet(OptCapture)) {
Hi Laurent, On 2019-06-18 21:40:39 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Tue, Jun 18, 2019 at 09:44:42AM +0200, Niklas Söderlund wrote: > > On 2019-06-17 13:09:10 +0300, Laurent Pinchart wrote: > > > As camera names can be cumbersome to type, selection of cameras by index > > > from a list can be useful. Print the list of detected cameras with an > > > index for each item, and interpret the camera name as an index if it is > > > a numerical value in the range from 1 to the number of cameras. > > > > I think you should also update the usage information for --camera to > > indicate both camera name and index can be used to select a camera. > > Good point ! Would the following be enough ? > > parser.addOption(OptCamera, OptionString, > - "Specify which camera to operate on", "camera", > + "Specify which camera to operate on, by name or by index", "camera", Works for me, with this changed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > ArgumentRequired, "camera"); > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/cam/main.cpp | 17 ++++++++++++++--- > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > > index f03a9faf87fa..0e7610b1befd 100644 > > > --- a/src/cam/main.cpp > > > +++ b/src/cam/main.cpp > > > @@ -73,7 +73,14 @@ int CamApp::init(int argc, char **argv) > > > } > > > > > > if (options_.isSet(OptCamera)) { > > > - camera_ = cm_->get(options_[OptCamera]); > > > + const std::string &cameraName = options_[OptCamera]; > > > + char *endptr; > > > + unsigned long index = strtoul(cameraName.c_str(), &endptr, 10); > > > + if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size()) > > > + camera_ = cm_->cameras()[index - 1]; > > > + else > > > + camera_ = cm_->get(cameraName); > > > + > > > if (!camera_) { > > > std::cout << "Camera " > > > << std::string(options_[OptCamera]) > > > @@ -172,8 +179,12 @@ int CamApp::run() > > > { > > > if (options_.isSet(OptList)) { > > > std::cout << "Available cameras:" << std::endl; > > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > > - std::cout << "- " << cam->name() << std::endl; > > > + > > > + unsigned int index = 1; > > > + for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > > + std::cout << index << ": " << cam->name() << std::endl; > > > + index++; > > > + } > > > } > > > > > > if (options_.isSet(OptCapture)) { > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On 17/06/2019 11:09, Laurent Pinchart wrote: > As camera names can be cumbersome to type, selection of cameras by index > from a list can be useful. Print the list of detected cameras with an > index for each item, and interpret the camera name as an index if it is > a numerical value in the range from 1 to the number of cameras. I'm very pleased to see this patch. I asked about allowing an index to identify the camera, rather than the extended strings sometime ago, especially as I have to deal with: - HP Wide Vision FHD Camera: HP W - HP Wide Vision FHD Camera: HP I But I recall I was told that we were not going to guarantee ordering of the cameras, thus an ID was not appropriate. But as all remaining members of the team seem happy with this now, I finally get to use an index! So Highly-desired-by and: Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/main.cpp | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index f03a9faf87fa..0e7610b1befd 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -73,7 +73,14 @@ int CamApp::init(int argc, char **argv) > } > > if (options_.isSet(OptCamera)) { > - camera_ = cm_->get(options_[OptCamera]); > + const std::string &cameraName = options_[OptCamera]; > + char *endptr; > + unsigned long index = strtoul(cameraName.c_str(), &endptr, 10); > + if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size()) > + camera_ = cm_->cameras()[index - 1]; > + else > + camera_ = cm_->get(cameraName); > + > if (!camera_) { > std::cout << "Camera " > << std::string(options_[OptCamera]) > @@ -172,8 +179,12 @@ int CamApp::run() > { > if (options_.isSet(OptList)) { > std::cout << "Available cameras:" << std::endl; > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > - std::cout << "- " << cam->name() << std::endl; > + > + unsigned int index = 1; > + for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > + std::cout << index << ": " << cam->name() << std::endl; > + index++; > + } > } > > if (options_.isSet(OptCapture)) { >
Hi Kieran, On Tue, Jun 18, 2019 at 09:42:05PM +0100, Kieran Bingham wrote: > On 17/06/2019 11:09, Laurent Pinchart wrote: > > As camera names can be cumbersome to type, selection of cameras by index > > from a list can be useful. Print the list of detected cameras with an > > index for each item, and interpret the camera name as an index if it is > > a numerical value in the range from 1 to the number of cameras. > > > I'm very pleased to see this patch. > > I asked about allowing an index to identify the camera, rather than the > extended strings sometime ago, especially as I have to deal with: > > - HP Wide Vision FHD Camera: HP W > - HP Wide Vision FHD Camera: HP I > > But I recall I was told that we were not going to guarantee ordering of > the cameras, thus an ID was not appropriate. > > But as all remaining members of the team seem happy with this now, I > finally get to use an index! I don't think libcamera should expose an index-based API as the primary mean to get hold of a camera, as that's all but stable. In the cam application, and for the time being, I think it's fine. We may reconsider that later when we'll have a good naming scheme. > So > > Highly-desired-by and: > Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/cam/main.cpp | 17 ++++++++++++++--- > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index f03a9faf87fa..0e7610b1befd 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -73,7 +73,14 @@ int CamApp::init(int argc, char **argv) > > } > > > > if (options_.isSet(OptCamera)) { > > - camera_ = cm_->get(options_[OptCamera]); > > + const std::string &cameraName = options_[OptCamera]; > > + char *endptr; > > + unsigned long index = strtoul(cameraName.c_str(), &endptr, 10); > > + if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size()) > > + camera_ = cm_->cameras()[index - 1]; > > + else > > + camera_ = cm_->get(cameraName); > > + > > if (!camera_) { > > std::cout << "Camera " > > << std::string(options_[OptCamera]) > > @@ -172,8 +179,12 @@ int CamApp::run() > > { > > if (options_.isSet(OptList)) { > > std::cout << "Available cameras:" << std::endl; > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > - std::cout << "- " << cam->name() << std::endl; > > + > > + unsigned int index = 1; > > + for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { > > + std::cout << index << ": " << cam->name() << std::endl; > > + index++; > > + } > > } > > > > if (options_.isSet(OptCapture)) {
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index f03a9faf87fa..0e7610b1befd 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -73,7 +73,14 @@ int CamApp::init(int argc, char **argv) } if (options_.isSet(OptCamera)) { - camera_ = cm_->get(options_[OptCamera]); + const std::string &cameraName = options_[OptCamera]; + char *endptr; + unsigned long index = strtoul(cameraName.c_str(), &endptr, 10); + if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size()) + camera_ = cm_->cameras()[index - 1]; + else + camera_ = cm_->get(cameraName); + if (!camera_) { std::cout << "Camera " << std::string(options_[OptCamera]) @@ -172,8 +179,12 @@ int CamApp::run() { if (options_.isSet(OptList)) { std::cout << "Available cameras:" << std::endl; - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) - std::cout << "- " << cam->name() << std::endl; + + unsigned int index = 1; + for (const std::shared_ptr<Camera> &cam : cm_->cameras()) { + std::cout << index << ": " << cam->name() << std::endl; + index++; + } } if (options_.isSet(OptCapture)) {
As camera names can be cumbersome to type, selection of cameras by index from a list can be useful. Print the list of detected cameras with an index for each item, and interpret the camera name as an index if it is a numerical value in the range from 1 to the number of cameras. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/cam/main.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)