| Message ID | 20190402005406.25097-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Tue, Apr 02, 2019 at 02:54:04AM +0200, Niklas Söderlund wrote: > Extend the cam tool to allow configuring more then one stream. Add an s/then/than/ > optional parameter to the --stream option to allow specifying a usage s/allow specifying/specify/ > hint for the stream. The usage hint is passed to libcamera to allow it > to make better decisions on which stream to use. Maybe "to give it control on which streams to use" ? > > To allow multiple streams to work creation of requests needs to be "To support multiple streams, creation ..." > reworked to be limit the number of requests to match the stream with the s/to be limit/to limit/ ? > leasts number of buffers. This should be improved in the future as the s/leasts/least/ > tool and the library evolves. s/evolves/evolve/ > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/main.cpp | 76 ++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 64 insertions(+), 12 deletions(-) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index d3f1f341d44e7f98..1bf28ca8eb8c6da7 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -7,6 +7,7 @@ > > #include <iomanip> > #include <iostream> > +#include <limits.h> > #include <map> > #include <signal.h> > #include <string.h> > @@ -17,6 +18,8 @@ > #include "event_loop.h" > #include "options.h" > > +#define MIN(a, b) ((a) < (b) ? (a) : (b)) > + This is a bit dangerous as arguments are evaluated multiple times. The following may help. #define MIN(a, b) ({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ __a < __b ? __a : __b; \ }) > using namespace libcamera; > > OptionsParser::Options options; > @@ -42,6 +45,9 @@ void signalHandler(int signal) > static int parseOptions(int argc, char *argv[]) > { > KeyValueParser streamKeyValue; > + streamKeyValue.addOption("hint", OptionString, > + "Usage hint for the stream (viewfinder, video, still)", > + ArgumentRequired); > streamKeyValue.addOption("width", OptionInteger, "Width in pixels", > ArgumentRequired); > streamKeyValue.addOption("height", OptionInteger, "Height in pixels", > @@ -61,7 +67,7 @@ static int parseOptions(int argc, char *argv[]) > "The default file name is 'frame-#.bin'.", > "file", ArgumentOptional, "filename"); > parser.addOption(OptStream, &streamKeyValue, > - "Set configuration of the camera's streams", "stream"); > + "Set configuration of the camera's streams", "stream", true); > parser.addOption(OptHelp, OptionNone, "Display this help message", > "help"); > parser.addOption(OptList, OptionNone, "List all cameras", "list"); > @@ -80,12 +86,48 @@ static int parseOptions(int argc, char *argv[]) > > static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config) > { > - std::set<Stream *> streams = camera->streams(); > - *config = camera->streamConfiguration({ Video() }); > - Stream *stream = config->begin()->first; > + std::vector<StreamHint> hints; > > - if (options.isSet(OptStream)) { > - KeyValueParser::Options conf = options[OptStream]; > + /* If no configuration is provided assume a single video stream. */ > + if (!options.isSet(OptStream)) { > + *config = camera->streamConfiguration({ Video() }); > + return 0; > + } > + You could store a const reference to options[OptStream].toArray() in a local variable as you use it multiple times. > + /* Use hints and get a default configuration. */ > + for (auto const &value : options[OptStream].toArray()) { > + KeyValueParser::Options conf = value.toKeyValues(); > + > + if (!conf.isSet("hint")) > + hints.push_back(Video()); > + else if (conf["hint"].toString() == "viewfinder") > + hints.push_back(Viewfinder(conf["width"], > + conf["height"])); > + else if (conf["hint"].toString() == "video") > + hints.push_back(Video()); > + else if (conf["hint"].toString() == "still") > + hints.push_back(Still()); > + else { > + std::cerr << "Unknown stream hint " > + << conf["hint"].toString() << std::endl; > + return -EINVAL; > + } Should we use braces for all statements, as in the kernel coding style ? > + } > + > + *config = camera->streamConfiguration(hints); > + > + if (config->size() != options[OptStream].toArray().size()) { > + std::cerr << "Failed to get default stream configuration" > + << std::endl; > + return -EINVAL; > + } > + > + /* Apply configuration explicitly requested. */ > + std::map<Stream *, StreamConfiguration>::iterator it = config->begin(); > + for (auto const &value : options[OptStream].toArray()) { > + KeyValueParser::Options conf = value.toKeyValues(); > + Stream *stream = it->first; > + it++; > > if (conf.isSet("width")) > (*config)[stream].width = conf["width"]; > @@ -137,7 +179,6 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *> > static int capture() > { > std::map<Stream *, StreamConfiguration> config; > - std::vector<Request *> requests; > int ret; > > ret = prepare_camera_config(&config); > @@ -152,8 +193,6 @@ static int capture() > return ret; > } > > - Stream *stream = config.begin()->first; > - > ret = camera->allocateBuffers(); > if (ret) { > std::cerr << "Failed to allocate buffers" > @@ -163,9 +202,18 @@ static int capture() > > camera->requestCompleted.connect(requestComplete); > > - BufferPool &pool = stream->bufferPool(); > + /* Figure out which stream have least number of buffers. */ s/have least/has the lower/ > + unsigned int nbuffers = UINT_MAX; > + for (auto const &it : config) > + nbuffers = MIN(nbuffers, it.first->bufferPool().count()); > > - for (Buffer &buffer : pool.buffers()) { > + /* > + * TODO: make cam tool smarter to support still capture by for > + * example pushing a button. For now run all streams all the time. > + */ > + > + std::vector<Request *> requests; > + for (unsigned int i = 0; i < nbuffers; i++) { > Request *request = camera->createRequest(); > if (!request) { > std::cerr << "Can't create request" << std::endl; > @@ -174,7 +222,11 @@ static int capture() > } > > std::map<Stream *, Buffer *> map; > - map[stream] = &buffer; > + for (auto const &it : config) { > + Stream *stream = it.first; > + map[stream] = &stream->bufferPool().buffers()[i]; > + } > + > ret = request->setBuffers(map); > if (ret < 0) { > std::cerr << "Can't set buffers for request" << std::endl;
Hi Laurent, Thanks for your feedback, special thanks for language support :-) On 2019-04-02 18:23:29 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Tue, Apr 02, 2019 at 02:54:04AM +0200, Niklas Söderlund wrote: > > Extend the cam tool to allow configuring more then one stream. Add an > > s/then/than/ > > > optional parameter to the --stream option to allow specifying a usage > > s/allow specifying/specify/ > > > hint for the stream. The usage hint is passed to libcamera to allow it > > to make better decisions on which stream to use. > > Maybe "to give it control on which streams to use" ? > > > > > To allow multiple streams to work creation of requests needs to be > > "To support multiple streams, creation ..." > > > reworked to be limit the number of requests to match the stream with the > > s/to be limit/to limit/ ? > > > leasts number of buffers. This should be improved in the future as the > > s/leasts/least/ > > > tool and the library evolves. > > s/evolves/evolve/ > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/main.cpp | 76 ++++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 64 insertions(+), 12 deletions(-) > > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index d3f1f341d44e7f98..1bf28ca8eb8c6da7 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -7,6 +7,7 @@ > > > > #include <iomanip> > > #include <iostream> > > +#include <limits.h> > > #include <map> > > #include <signal.h> > > #include <string.h> > > @@ -17,6 +18,8 @@ > > #include "event_loop.h" > > #include "options.h" > > > > +#define MIN(a, b) ((a) < (b) ? (a) : (b)) > > + > > This is a bit dangerous as arguments are evaluated multiple times. The > following may help. > > #define MIN(a, b) ({ \ > typeof(a) __a = (a); \ > typeof(b) __b = (b); \ > __a < __b ? __a : __b; \ > }) I thought the same, C++11 don't have typeof so this is not a solution. There is __typeof which is a G++ extension so it don't seem to be a good match. In the end I went for std::min() and avoided the macro issue al together. > > > using namespace libcamera; > > > > OptionsParser::Options options; > > @@ -42,6 +45,9 @@ void signalHandler(int signal) > > static int parseOptions(int argc, char *argv[]) > > { > > KeyValueParser streamKeyValue; > > + streamKeyValue.addOption("hint", OptionString, > > + "Usage hint for the stream (viewfinder, video, still)", > > + ArgumentRequired); > > streamKeyValue.addOption("width", OptionInteger, "Width in pixels", > > ArgumentRequired); > > streamKeyValue.addOption("height", OptionInteger, "Height in pixels", > > @@ -61,7 +67,7 @@ static int parseOptions(int argc, char *argv[]) > > "The default file name is 'frame-#.bin'.", > > "file", ArgumentOptional, "filename"); > > parser.addOption(OptStream, &streamKeyValue, > > - "Set configuration of the camera's streams", "stream"); > > + "Set configuration of the camera's streams", "stream", true); > > parser.addOption(OptHelp, OptionNone, "Display this help message", > > "help"); > > parser.addOption(OptList, OptionNone, "List all cameras", "list"); > > @@ -80,12 +86,48 @@ static int parseOptions(int argc, char *argv[]) > > > > static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config) > > { > > - std::set<Stream *> streams = camera->streams(); > > - *config = camera->streamConfiguration({ Video() }); > > - Stream *stream = config->begin()->first; > > + std::vector<StreamHint> hints; > > > > - if (options.isSet(OptStream)) { > > - KeyValueParser::Options conf = options[OptStream]; > > + /* If no configuration is provided assume a single video stream. */ > > + if (!options.isSet(OptStream)) { > > + *config = camera->streamConfiguration({ Video() }); > > + return 0; > > + } > > + > > You could store a const reference to options[OptStream].toArray() in a > local variable as you use it multiple times. > > > + /* Use hints and get a default configuration. */ > > + for (auto const &value : options[OptStream].toArray()) { > > + KeyValueParser::Options conf = value.toKeyValues(); > > + > > + if (!conf.isSet("hint")) > > + hints.push_back(Video()); > > + else if (conf["hint"].toString() == "viewfinder") > > + hints.push_back(Viewfinder(conf["width"], > > + conf["height"])); > > + else if (conf["hint"].toString() == "video") > > + hints.push_back(Video()); > > + else if (conf["hint"].toString() == "still") > > + hints.push_back(Still()); > > + else { > > + std::cerr << "Unknown stream hint " > > + << conf["hint"].toString() << std::endl; > > + return -EINVAL; > > + } > > Should we use braces for all statements, as in the kernel coding style ? Yes we should. > > > + } > > + > > + *config = camera->streamConfiguration(hints); > > + > > + if (config->size() != options[OptStream].toArray().size()) { > > + std::cerr << "Failed to get default stream configuration" > > + << std::endl; > > + return -EINVAL; > > + } > > + > > + /* Apply configuration explicitly requested. */ > > + std::map<Stream *, StreamConfiguration>::iterator it = config->begin(); > > + for (auto const &value : options[OptStream].toArray()) { > > + KeyValueParser::Options conf = value.toKeyValues(); > > + Stream *stream = it->first; > > + it++; > > > > if (conf.isSet("width")) > > (*config)[stream].width = conf["width"]; > > @@ -137,7 +179,6 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *> > > static int capture() > > { > > std::map<Stream *, StreamConfiguration> config; > > - std::vector<Request *> requests; > > int ret; > > > > ret = prepare_camera_config(&config); > > @@ -152,8 +193,6 @@ static int capture() > > return ret; > > } > > > > - Stream *stream = config.begin()->first; > > - > > ret = camera->allocateBuffers(); > > if (ret) { > > std::cerr << "Failed to allocate buffers" > > @@ -163,9 +202,18 @@ static int capture() > > > > camera->requestCompleted.connect(requestComplete); > > > > - BufferPool &pool = stream->bufferPool(); > > + /* Figure out which stream have least number of buffers. */ > > s/have least/has the lower/ > > > + unsigned int nbuffers = UINT_MAX; > > + for (auto const &it : config) > > + nbuffers = MIN(nbuffers, it.first->bufferPool().count()); > > > > - for (Buffer &buffer : pool.buffers()) { > > + /* > > + * TODO: make cam tool smarter to support still capture by for > > + * example pushing a button. For now run all streams all the time. > > + */ > > + > > + std::vector<Request *> requests; > > + for (unsigned int i = 0; i < nbuffers; i++) { > > Request *request = camera->createRequest(); > > if (!request) { > > std::cerr << "Can't create request" << std::endl; > > @@ -174,7 +222,11 @@ static int capture() > > } > > > > std::map<Stream *, Buffer *> map; > > - map[stream] = &buffer; > > + for (auto const &it : config) { > > + Stream *stream = it.first; > > + map[stream] = &stream->bufferPool().buffers()[i]; > > + } > > + > > ret = request->setBuffers(map); > > if (ret < 0) { > > std::cerr << "Can't set buffers for request" << std::endl; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/cam/main.cpp b/src/cam/main.cpp index d3f1f341d44e7f98..1bf28ca8eb8c6da7 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -7,6 +7,7 @@ #include <iomanip> #include <iostream> +#include <limits.h> #include <map> #include <signal.h> #include <string.h> @@ -17,6 +18,8 @@ #include "event_loop.h" #include "options.h" +#define MIN(a, b) ((a) < (b) ? (a) : (b)) + using namespace libcamera; OptionsParser::Options options; @@ -42,6 +45,9 @@ void signalHandler(int signal) static int parseOptions(int argc, char *argv[]) { KeyValueParser streamKeyValue; + streamKeyValue.addOption("hint", OptionString, + "Usage hint for the stream (viewfinder, video, still)", + ArgumentRequired); streamKeyValue.addOption("width", OptionInteger, "Width in pixels", ArgumentRequired); streamKeyValue.addOption("height", OptionInteger, "Height in pixels", @@ -61,7 +67,7 @@ static int parseOptions(int argc, char *argv[]) "The default file name is 'frame-#.bin'.", "file", ArgumentOptional, "filename"); parser.addOption(OptStream, &streamKeyValue, - "Set configuration of the camera's streams", "stream"); + "Set configuration of the camera's streams", "stream", true); parser.addOption(OptHelp, OptionNone, "Display this help message", "help"); parser.addOption(OptList, OptionNone, "List all cameras", "list"); @@ -80,12 +86,48 @@ static int parseOptions(int argc, char *argv[]) static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config) { - std::set<Stream *> streams = camera->streams(); - *config = camera->streamConfiguration({ Video() }); - Stream *stream = config->begin()->first; + std::vector<StreamHint> hints; - if (options.isSet(OptStream)) { - KeyValueParser::Options conf = options[OptStream]; + /* If no configuration is provided assume a single video stream. */ + if (!options.isSet(OptStream)) { + *config = camera->streamConfiguration({ Video() }); + return 0; + } + + /* Use hints and get a default configuration. */ + for (auto const &value : options[OptStream].toArray()) { + KeyValueParser::Options conf = value.toKeyValues(); + + if (!conf.isSet("hint")) + hints.push_back(Video()); + else if (conf["hint"].toString() == "viewfinder") + hints.push_back(Viewfinder(conf["width"], + conf["height"])); + else if (conf["hint"].toString() == "video") + hints.push_back(Video()); + else if (conf["hint"].toString() == "still") + hints.push_back(Still()); + else { + std::cerr << "Unknown stream hint " + << conf["hint"].toString() << std::endl; + return -EINVAL; + } + } + + *config = camera->streamConfiguration(hints); + + if (config->size() != options[OptStream].toArray().size()) { + std::cerr << "Failed to get default stream configuration" + << std::endl; + return -EINVAL; + } + + /* Apply configuration explicitly requested. */ + std::map<Stream *, StreamConfiguration>::iterator it = config->begin(); + for (auto const &value : options[OptStream].toArray()) { + KeyValueParser::Options conf = value.toKeyValues(); + Stream *stream = it->first; + it++; if (conf.isSet("width")) (*config)[stream].width = conf["width"]; @@ -137,7 +179,6 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *> static int capture() { std::map<Stream *, StreamConfiguration> config; - std::vector<Request *> requests; int ret; ret = prepare_camera_config(&config); @@ -152,8 +193,6 @@ static int capture() return ret; } - Stream *stream = config.begin()->first; - ret = camera->allocateBuffers(); if (ret) { std::cerr << "Failed to allocate buffers" @@ -163,9 +202,18 @@ static int capture() camera->requestCompleted.connect(requestComplete); - BufferPool &pool = stream->bufferPool(); + /* Figure out which stream have least number of buffers. */ + unsigned int nbuffers = UINT_MAX; + for (auto const &it : config) + nbuffers = MIN(nbuffers, it.first->bufferPool().count()); - for (Buffer &buffer : pool.buffers()) { + /* + * TODO: make cam tool smarter to support still capture by for + * example pushing a button. For now run all streams all the time. + */ + + std::vector<Request *> requests; + for (unsigned int i = 0; i < nbuffers; i++) { Request *request = camera->createRequest(); if (!request) { std::cerr << "Can't create request" << std::endl; @@ -174,7 +222,11 @@ static int capture() } std::map<Stream *, Buffer *> map; - map[stream] = &buffer; + for (auto const &it : config) { + Stream *stream = it.first; + map[stream] = &stream->bufferPool().buffers()[i]; + } + ret = request->setBuffers(map); if (ret < 0) { std::cerr << "Can't set buffers for request" << std::endl;
Extend the cam tool to allow configuring more then one stream. Add an optional parameter to the --stream option to allow specifying a usage hint for the stream. The usage hint is passed to libcamera to allow it to make better decisions on which stream to use. To allow multiple streams to work creation of requests needs to be reworked to be limit the number of requests to match the stream with the leasts number of buffers. This should be improved in the future as the tool and the library evolves. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/main.cpp | 76 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 64 insertions(+), 12 deletions(-)