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

[Laurent Pinchart]

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 at 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;

-- 
Regards,

Laurent Pinchart