| Message ID | 20190713145119.11193-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
"Don't rush when sending patches!" On Sat, Jul 13, 2019 at 04:51:19PM +0200, Jacopo Mondi wrote: > Add a '-s|--size' option to qcam to allow selecting the stream > resolution using a command line option. > > If the sizes are not supported by the camera, they get automatically > adjusted and the user notified via an output message. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/qcam/main.cpp | 8 ++++++++ > src/qcam/main_window.cpp | 31 ++++++++++++++++++++++++++++++- > src/qcam/main_window.h | 1 + > 3 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > index 106b8a162d9f..da942f3daed6 100644 > --- a/src/qcam/main.cpp > +++ b/src/qcam/main.cpp > @@ -25,12 +25,20 @@ void signalHandler(int signal) > > OptionsParser::Options parseOptions(int argc, char *argv[]) > { > + KeyValueParser sizeParser; > + sizeParser.addOption("width", OptionInteger, "Width in pixels", > + ArgumentRequired); > + sizeParser.addOption("height", OptionInteger, "Height in pixels", > + ArgumentRequired); > + > OptionsParser parser; > parser.addOption(OptCamera, OptionString, > "Specify which camera to operate on", "camera", > ArgumentRequired, "camera"); > parser.addOption(OptHelp, OptionNone, "Display this help message", > "help"); > + parser.addOption(OptSize, &sizeParser, "Set the stream size", > + "size", true); > > OptionsParser::Options options = parser.parse(argc, argv); > if (options.isSet(OptHelp)) > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 907d2423ed15..d9a30aa15db0 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -116,13 +116,42 @@ int MainWindow::startCapture() > int ret; > > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording }); > + > + StreamConfiguration &cfg = config_->at(0); > + if (options_.isSet(OptSize)) { > + const std::vector<OptionValue> &sizeOptions = > + options_[OptSize].toArray(); > + > + /* Set desired stream size if requested. */ > + for (auto const &value : sizeOptions) { > + KeyValueParser::Options opt = value.toKeyValues(); > + > + if (opt.isSet("width")) > + cfg.size.width = opt["width"]; > + > + if (opt.isSet("height")) > + cfg.size.height = opt["height"]; > + } > + } > + > + CameraConfiguration::Status validation = config_->validate(); > + if (validation == CameraConfiguration::Invalid) { > + std::cerr << "Faild to apply camera configuration"; Missing std::endl > + return -EINVAL; > + } > + > + if (validation == CameraConfiguration::Adjusted) { > + std::cout << "Stream sizes adjusted to " > + << cfg.size.width << "x" << cfg.size.height > + << std::endl; > + } I could have saved the { } but I actually like the symmetry with the above case. Sorry for the trivial mistakes, just noticed. > + > ret = camera_->configure(config_.get()); > if (ret < 0) { > std::cout << "Failed to configure camera" << std::endl; > return ret; > } > > - const StreamConfiguration &cfg = config_->at(0); > Stream *stream = cfg.stream(); > ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width, > cfg.size.height); > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index f58cb6a65b2b..b45cbca725fa 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -27,6 +27,7 @@ class ViewFinder; > enum { > OptCamera = 'c', > OptHelp = 'h', > + OptSize = 's', > }; > > class MainWindow : public QMainWindow > -- > 2.21.0 >
Hi Jacopo, Thanks for your work. On 2019-07-13 16:51:19 +0200, Jacopo Mondi wrote: > Add a '-s|--size' option to qcam to allow selecting the stream > resolution using a command line option. > > If the sizes are not supported by the camera, they get automatically > adjusted and the user notified via an output message. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/qcam/main.cpp | 8 ++++++++ > src/qcam/main_window.cpp | 31 ++++++++++++++++++++++++++++++- > src/qcam/main_window.h | 1 + > 3 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > index 106b8a162d9f..da942f3daed6 100644 > --- a/src/qcam/main.cpp > +++ b/src/qcam/main.cpp > @@ -25,12 +25,20 @@ void signalHandler(int signal) > > OptionsParser::Options parseOptions(int argc, char *argv[]) > { > + KeyValueParser sizeParser; > + sizeParser.addOption("width", OptionInteger, "Width in pixels", > + ArgumentRequired); > + sizeParser.addOption("height", OptionInteger, "Height in pixels", > + ArgumentRequired); > + > OptionsParser parser; > parser.addOption(OptCamera, OptionString, > "Specify which camera to operate on", "camera", > ArgumentRequired, "camera"); > parser.addOption(OptHelp, OptionNone, "Display this help message", > "help"); > + parser.addOption(OptSize, &sizeParser, "Set the stream size", > + "size", true); I wonder if we should not call this --stream to match with the cam utility, whit this and the comments pointed out by you. Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > OptionsParser::Options options = parser.parse(argc, argv); > if (options.isSet(OptHelp)) > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 907d2423ed15..d9a30aa15db0 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -116,13 +116,42 @@ int MainWindow::startCapture() > int ret; > > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording }); > + > + StreamConfiguration &cfg = config_->at(0); > + if (options_.isSet(OptSize)) { > + const std::vector<OptionValue> &sizeOptions = > + options_[OptSize].toArray(); > + > + /* Set desired stream size if requested. */ > + for (auto const &value : sizeOptions) { > + KeyValueParser::Options opt = value.toKeyValues(); > + > + if (opt.isSet("width")) > + cfg.size.width = opt["width"]; > + > + if (opt.isSet("height")) > + cfg.size.height = opt["height"]; > + } > + } > + > + CameraConfiguration::Status validation = config_->validate(); > + if (validation == CameraConfiguration::Invalid) { > + std::cerr << "Faild to apply camera configuration"; > + return -EINVAL; > + } > + > + if (validation == CameraConfiguration::Adjusted) { > + std::cout << "Stream sizes adjusted to " > + << cfg.size.width << "x" << cfg.size.height > + << std::endl; > + } > + > ret = camera_->configure(config_.get()); > if (ret < 0) { > std::cout << "Failed to configure camera" << std::endl; > return ret; > } > > - const StreamConfiguration &cfg = config_->at(0); > Stream *stream = cfg.stream(); > ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width, > cfg.size.height); > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index f58cb6a65b2b..b45cbca725fa 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -27,6 +27,7 @@ class ViewFinder; > enum { > OptCamera = 'c', > OptHelp = 'h', > + OptSize = 's', > }; > > class MainWindow : public QMainWindow > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Sat, Jul 13, 2019 at 04:56:25PM +0200, Jacopo Mondi wrote: > "Don't rush when sending patches!" Or reviews for that matter :-) > On Sat, Jul 13, 2019 at 04:51:19PM +0200, Jacopo Mondi wrote: > > Add a '-s|--size' option to qcam to allow selecting the stream > > resolution using a command line option. > > > > If the sizes are not supported by the camera, they get automatically > > adjusted and the user notified via an output message. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/qcam/main.cpp | 8 ++++++++ > > src/qcam/main_window.cpp | 31 ++++++++++++++++++++++++++++++- > > src/qcam/main_window.h | 1 + > > 3 files changed, 39 insertions(+), 1 deletion(-) > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > index 106b8a162d9f..da942f3daed6 100644 > > --- a/src/qcam/main.cpp > > +++ b/src/qcam/main.cpp > > @@ -25,12 +25,20 @@ void signalHandler(int signal) > > > > OptionsParser::Options parseOptions(int argc, char *argv[]) > > { > > + KeyValueParser sizeParser; > > + sizeParser.addOption("width", OptionInteger, "Width in pixels", > > + ArgumentRequired); > > + sizeParser.addOption("height", OptionInteger, "Height in pixels", > > + ArgumentRequired); > > + > > OptionsParser parser; > > parser.addOption(OptCamera, OptionString, > > "Specify which camera to operate on", "camera", > > ArgumentRequired, "camera"); > > parser.addOption(OptHelp, OptionNone, "Display this help message", > > "help"); > > + parser.addOption(OptSize, &sizeParser, "Set the stream size", > > + "size", true); > > > > OptionsParser::Options options = parser.parse(argc, argv); > > if (options.isSet(OptHelp)) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 907d2423ed15..d9a30aa15db0 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -116,13 +116,42 @@ int MainWindow::startCapture() > > int ret; > > > > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording }); > > + > > + StreamConfiguration &cfg = config_->at(0); > > + if (options_.isSet(OptSize)) { > > + const std::vector<OptionValue> &sizeOptions = > > + options_[OptSize].toArray(); > > + > > + /* Set desired stream size if requested. */ > > + for (auto const &value : sizeOptions) { The const keyword should come before auto. > > + KeyValueParser::Options opt = value.toKeyValues(); > > + > > + if (opt.isSet("width")) > > + cfg.size.width = opt["width"]; > > + > > + if (opt.isSet("height")) > > + cfg.size.height = opt["height"]; > > + } > > + } > > + > > + CameraConfiguration::Status validation = config_->validate(); > > + if (validation == CameraConfiguration::Invalid) { > > + std::cerr << "Faild to apply camera configuration"; s/Faild/Failed/ And actually I would write "Failed to create valid configuration" as this doesn't apply the configuration. > > Missing std::endl > > > + return -EINVAL; > > + } > > + > > + if (validation == CameraConfiguration::Adjusted) { > > + std::cout << "Stream sizes adjusted to " s/sizes/size/ > > + << cfg.size.width << "x" << cfg.size.height cfg.size.toString() ? > > + << std::endl; > > + } > > I could have saved the { } but I actually like the symmetry with the > above case. > > Sorry for the trivial mistakes, just noticed. > > > + > > ret = camera_->configure(config_.get()); > > if (ret < 0) { > > std::cout << "Failed to configure camera" << std::endl; > > return ret; > > } > > > > - const StreamConfiguration &cfg = config_->at(0); > > Stream *stream = cfg.stream(); > > ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width, > > cfg.size.height); > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index f58cb6a65b2b..b45cbca725fa 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -27,6 +27,7 @@ class ViewFinder; > > enum { > > OptCamera = 'c', > > OptHelp = 'h', > > + OptSize = 's', > > }; > > > > class MainWindow : public QMainWindow
Hi Niklas, On Sun, Jul 14, 2019 at 04:19:30PM +0900, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2019-07-13 16:51:19 +0200, Jacopo Mondi wrote: > > Add a '-s|--size' option to qcam to allow selecting the stream > > resolution using a command line option. > > > > If the sizes are not supported by the camera, they get automatically > > adjusted and the user notified via an output message. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/qcam/main.cpp | 8 ++++++++ > > src/qcam/main_window.cpp | 31 ++++++++++++++++++++++++++++++- > > src/qcam/main_window.h | 1 + > > 3 files changed, 39 insertions(+), 1 deletion(-) > > > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp > > index 106b8a162d9f..da942f3daed6 100644 > > --- a/src/qcam/main.cpp > > +++ b/src/qcam/main.cpp > > @@ -25,12 +25,20 @@ void signalHandler(int signal) > > > > OptionsParser::Options parseOptions(int argc, char *argv[]) > > { > > + KeyValueParser sizeParser; > > + sizeParser.addOption("width", OptionInteger, "Width in pixels", > > + ArgumentRequired); > > + sizeParser.addOption("height", OptionInteger, "Height in pixels", > > + ArgumentRequired); > > + > > OptionsParser parser; > > parser.addOption(OptCamera, OptionString, > > "Specify which camera to operate on", "camera", > > ArgumentRequired, "camera"); > > parser.addOption(OptHelp, OptionNone, "Display this help message", > > "help"); > > + parser.addOption(OptSize, &sizeParser, "Set the stream size", > > + "size", true); > > I wonder if we should not call this --stream to match with the cam > utility, whit this and the comments pointed out by you. > As qcam does not support multiple streams, and this option only configure sizes (not even pixel formats), I would keep using 'sizes' as the option name. > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Thanks, I'll fix Laurent's comments, take your tag in and then push. > > > > > OptionsParser::Options options = parser.parse(argc, argv); > > if (options.isSet(OptHelp)) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 907d2423ed15..d9a30aa15db0 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -116,13 +116,42 @@ int MainWindow::startCapture() > > int ret; > > > > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording }); > > + > > + StreamConfiguration &cfg = config_->at(0); > > + if (options_.isSet(OptSize)) { > > + const std::vector<OptionValue> &sizeOptions = > > + options_[OptSize].toArray(); > > + > > + /* Set desired stream size if requested. */ > > + for (auto const &value : sizeOptions) { > > + KeyValueParser::Options opt = value.toKeyValues(); > > + > > + if (opt.isSet("width")) > > + cfg.size.width = opt["width"]; > > + > > + if (opt.isSet("height")) > > + cfg.size.height = opt["height"]; > > + } > > + } > > + > > + CameraConfiguration::Status validation = config_->validate(); > > + if (validation == CameraConfiguration::Invalid) { > > + std::cerr << "Faild to apply camera configuration"; > > + return -EINVAL; > > + } > > + > > + if (validation == CameraConfiguration::Adjusted) { > > + std::cout << "Stream sizes adjusted to " > > + << cfg.size.width << "x" << cfg.size.height > > + << std::endl; > > + } > > + > > ret = camera_->configure(config_.get()); > > if (ret < 0) { > > std::cout << "Failed to configure camera" << std::endl; > > return ret; > > } > > > > - const StreamConfiguration &cfg = config_->at(0); > > Stream *stream = cfg.stream(); > > ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width, > > cfg.size.height); > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index f58cb6a65b2b..b45cbca725fa 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -27,6 +27,7 @@ class ViewFinder; > > enum { > > OptCamera = 'c', > > OptHelp = 'h', > > + OptSize = 's', > > }; > > > > class MainWindow : public QMainWindow > > -- > > 2.21.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp index 106b8a162d9f..da942f3daed6 100644 --- a/src/qcam/main.cpp +++ b/src/qcam/main.cpp @@ -25,12 +25,20 @@ void signalHandler(int signal) OptionsParser::Options parseOptions(int argc, char *argv[]) { + KeyValueParser sizeParser; + sizeParser.addOption("width", OptionInteger, "Width in pixels", + ArgumentRequired); + sizeParser.addOption("height", OptionInteger, "Height in pixels", + ArgumentRequired); + OptionsParser parser; parser.addOption(OptCamera, OptionString, "Specify which camera to operate on", "camera", ArgumentRequired, "camera"); parser.addOption(OptHelp, OptionNone, "Display this help message", "help"); + parser.addOption(OptSize, &sizeParser, "Set the stream size", + "size", true); OptionsParser::Options options = parser.parse(argc, argv); if (options.isSet(OptHelp)) diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 907d2423ed15..d9a30aa15db0 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -116,13 +116,42 @@ int MainWindow::startCapture() int ret; config_ = camera_->generateConfiguration({ StreamRole::VideoRecording }); + + StreamConfiguration &cfg = config_->at(0); + if (options_.isSet(OptSize)) { + const std::vector<OptionValue> &sizeOptions = + options_[OptSize].toArray(); + + /* Set desired stream size if requested. */ + for (auto const &value : sizeOptions) { + KeyValueParser::Options opt = value.toKeyValues(); + + if (opt.isSet("width")) + cfg.size.width = opt["width"]; + + if (opt.isSet("height")) + cfg.size.height = opt["height"]; + } + } + + CameraConfiguration::Status validation = config_->validate(); + if (validation == CameraConfiguration::Invalid) { + std::cerr << "Faild to apply camera configuration"; + return -EINVAL; + } + + if (validation == CameraConfiguration::Adjusted) { + std::cout << "Stream sizes adjusted to " + << cfg.size.width << "x" << cfg.size.height + << std::endl; + } + ret = camera_->configure(config_.get()); if (ret < 0) { std::cout << "Failed to configure camera" << std::endl; return ret; } - const StreamConfiguration &cfg = config_->at(0); Stream *stream = cfg.stream(); ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width, cfg.size.height); diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h index f58cb6a65b2b..b45cbca725fa 100644 --- a/src/qcam/main_window.h +++ b/src/qcam/main_window.h @@ -27,6 +27,7 @@ class ViewFinder; enum { OptCamera = 'c', OptHelp = 'h', + OptSize = 's', }; class MainWindow : public QMainWindow
Add a '-s|--size' option to qcam to allow selecting the stream resolution using a command line option. If the sizes are not supported by the camera, they get automatically adjusted and the user notified via an output message. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/qcam/main.cpp | 8 ++++++++ src/qcam/main_window.cpp | 31 ++++++++++++++++++++++++++++++- src/qcam/main_window.h | 1 + 3 files changed, 39 insertions(+), 1 deletion(-)