[libcamera-devel] [PATCH v2 2/4] cam: Add support to specify multiple stream configurations with roles
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Apr 4 02:00:32 CEST 2019
Hi Jacopo,
Thanks for your feedback.
On 2019-04-03 15:39:47 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> one general question below
>
> On Wed, Apr 03, 2019 at 03:12:33AM +0200, Niklas Söderlund wrote:
> > Extend the cam tool to allow configuring more than one stream. Add an
> > optional parameter to the --stream option to specify a usage role for
> > the stream. The stream role is passed to libcamera to give it control
> > over which streams to use.
> >
> > To support multiple streams, creation of requests needs to be reworked
> > to limit the number of requests to match the stream with the least
> > number of buffers. This should be improved in the future as the tool and
> > the library evolve.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/cam/main.cpp | 78 ++++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 6bf5e5926704d6e9..b47bda21cbb7f220 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -5,8 +5,10 @@
> > * main.cpp - cam - The libcamera swiss army knife
> > */
> >
> > +#include <algorithm>
> > #include <iomanip>
> > #include <iostream>
> > +#include <limits.h>
> > #include <map>
> > #include <signal.h>
> > #include <string.h>
> > @@ -42,6 +44,9 @@ void signalHandler(int signal)
> > static int parseOptions(int argc, char *argv[])
> > {
> > KeyValueParser streamKeyValue;
> > + streamKeyValue.addOption("role", OptionString,
> > + "Role for the stream (viewfinder, video, still)",
> > + ArgumentRequired);
> > streamKeyValue.addOption("width", OptionInteger, "Width in pixels",
> > ArgumentRequired);
> > streamKeyValue.addOption("height", OptionInteger, "Height in pixels",
> > @@ -61,7 +66,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 a camera stream", "stream");
> > + "Set configuration of a camera stream", "stream", true);
> > parser.addOption(OptHelp, OptionNone, "Display this help message",
> > "help");
> > parser.addOption(OptList, OptionNone, "List all cameras", "list");
> > @@ -80,12 +85,51 @@ static int parseOptions(int argc, char *argv[])
> >
> > static int prepareCameraConfig(std::map<Stream *, StreamConfiguration> *config)
> > {
> > - std::set<Stream *> streams = camera->streams();
> > - *config = camera->streamConfiguration({ Stream::VideoRecording() });
> > - Stream *stream = config->begin()->first;
> > + std::vector<StreamRole> roles;
> >
> > - 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({ Stream::VideoRecording() });
> > + return 0;
> > + }
> > +
> > + const std::vector<OptionValue> &streamopts =
> > + options[OptStream].toArray();
> > +
> > + /* Use roles and get a default configuration. */
> > + for (auto const &value : streamopts) {
> > + KeyValueParser::Options conf = value.toKeyValues();
> > +
> > + if (!conf.isSet("role")) {
> > + roles.push_back(Stream::VideoRecording());
> > + } else if (conf["role"].toString() == "viewfinder") {
> > + roles.push_back(Stream::Viewfinder(conf["width"],
> > + conf["height"]));
> > + } else if (conf["role"].toString() == "video") {
> > + roles.push_back(Stream::VideoRecording());
> > + } else if (conf["role"].toString() == "still") {
> > + roles.push_back(Stream::StillCapture());
> > + } else {
> > + std::cerr << "Unknown stream role "
> > + << conf["role"].toString() << std::endl;
> > + return -EINVAL;
> > + }
> > + }
>
> Open question and not specifically on this patch, but on this and the
> other series you have in flight: this implementation does not allow to
> specify multiple roles for a stream, as you could have done using a
> bitmaks. For applications to stay as generic as possible, is it very
> unlikely to have them as for something like (VIEWFINDER | VIDEOCAPTURE) ?
> If I'm not wrong this implementation makes roles mutually exclusive,
> doesn't it?
Yes it makes roles mutually exclusive on a stream level. It is not
possible to use the same stream simultaneously for both still image
capture and as a view finder. You can however use the same stream for
both purposes in two different capture sessions.
Did I understand your question correctly?
>
> This would call for pipeline handler to match at least one of the intended
> usages, to assign a stream to a stream usage.
>
> > +
> > + *config = camera->streamConfiguration(roles);
> > +
> > + if (config->size() != streamopts.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 : streamopts) {
> > + KeyValueParser::Options conf = value.toKeyValues();
> > + Stream *stream = it->first;
> > + it++;
> >
> > if (conf.isSet("width"))
> > (*config)[stream].width = conf["width"];
> > @@ -137,7 +181,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 = prepareCameraConfig(&config);
> > @@ -152,8 +195,6 @@ static int capture()
> > return ret;
> > }
> >
> > - Stream *stream = config.begin()->first;
> > -
> > ret = camera->allocateBuffers();
> > if (ret) {
> > std::cerr << "Failed to allocate buffers"
> > @@ -163,9 +204,18 @@ static int capture()
> >
> > camera->requestCompleted.connect(requestComplete);
> >
> > - BufferPool &pool = stream->bufferPool();
> > + /* Figure out which stream s the lower number of buffers. */
>
> s/ s/has/ ?
>
> > + unsigned int nbuffers = UINT_MAX;
> > + for (auto const &it : config)
> > + nbuffers = std::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 +224,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;
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list