[libcamera-devel] [PATCH v2 2/4] cam: Add support to specify multiple stream configurations with roles

[Jacopo Mondi]

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?

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190403/64011bf8/attachment.sig>