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

Wed Apr 3 02:17:33 CEST 2019

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 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;						\
> })

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

-- 
Regards,
Niklas Söderlund