[libcamera-devel] [PATCH 6/6] cam: add --format option to configure a stream

[Niklas Söderlund]

Hi Kieran,

Thanks for your comment.

On 2019-01-29 09:29:37 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 28/01/2019 00:41, Niklas Söderlund wrote:
> > Add a option to configure the first stream of a camera from an argument
> > with options and parse the width, height and pixel format from that
> > list.
> > 
> > The pixel format is still specified as a integer which should correspond
> > to the kernels FOURCC identifiers. Going forward this should be turned
> > into a string representation and the cam parser should translate between
> > the two.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/main.cpp | 104 +++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 92 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index bde47a8f17983912..4b4ce9aa29c80bd1 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -20,7 +20,8 @@ using namespace libcamera;
> >  OptionsParser::Options options;
> >  
> >  enum {
> > -	OptCamera = 'c',
> > +	OptCamera = 'C',
> > +	OptFormat = 'f',
> >  	OptHelp = 'h',
> >  	OptList = 'l',
> >  };
> > @@ -35,10 +36,20 @@ void signalHandler(int signal)
> >  
> >  static int parseOptions(int argc, char *argv[])
> >  {
> > +	KeyValueParser formatKeyValue;
> > +	formatKeyValue.addOption("width", "Set width in pixels",
> > +				 ArgumentRequired, "w");
> > +	formatKeyValue.addOption("height", "Set height in pixels",
> > +				 ArgumentRequired, "h");
> > +	formatKeyValue.addOption("pixelformat", "Set pixel format",
> > +				 ArgumentRequired, "pf");
> 
> Hrm - can a short-option have two letters? (if so, should it?)

This is not a shot-option :-) It is a descriptive name for the parameter 
only used for printing the usage. The usage printing for the whole 
--format options:

  -f, --format key=value[,key=value,...]        Set format of the cameras first stream
                                                height=h                Set height in pixels
                                                pixelformat=pf          Set pixel format
                                                width=w                 Set width in pixels

And one would use it as --format width=800,pixelformat=42

> 
> > +
> >  	OptionsParser parser;
> >  
> >  	parser.addOption(OptCamera, "Specify which camera to operate on",
> >  			 "camera", ArgumentRequired, "camera");
> > +	parser.addOption(OptFormat, "Set format of the cameras first stream",
> > +			 formatKeyValue, "format");
> >  	parser.addOption(OptHelp, "Display this help message", "help");
> >  	parser.addOption(OptList, "List all cameras", "list");
> >  
> > @@ -54,6 +65,42 @@ static int parseOptions(int argc, char *argv[])
> >  	return 0;
> >  }
> >  
> > +int str2uint(std::string str, unsigned int *dest)
> > +{
> > +	if (!dest || str == "" || !sscanf(str.c_str(), "%d", dest))
> > +		return -EINVAL;
> > +	return 0;
> > +}
> > +
> > +bool configureStreams(Camera *camera, std::vector<Stream> &streams)
> > +{
> > +	if (!options.isKeyValue(OptFormat))
> > +		return false;
> > +
> > +	KeyValueParser::Options format = options.keyValues(OptFormat);
> > +
> > +	std::map<unsigned int, StreamConfiguration> config = camera->streamConfiguration(streams);
> > +	unsigned int id = streams.front().id();
> > +
> > +	if (format.isSet("width"))
> > +		if (str2uint(format["width"], &config[id].width))
> > +			return false;
> > +
> > +	if (format.isSet("height"))
> > +		if (str2uint(format["height"], &config[id].height))
> > +			return false;
> > +
> > +	/* TODO: Translate 4CC string to ID. */
> > +	if (format.isSet("pixelformat"))
> > +		if (str2uint(format["pixelformat"], &config[id].pixelFormat))
> > +			return false;
> > +
> > +	if (camera->configureStreams(config))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >  	int ret;
> > @@ -63,6 +110,8 @@ int main(int argc, char **argv)
> >  		return EXIT_FAILURE;
> >  
> >  	CameraManager *cm = CameraManager::instance();
> > +	std::shared_ptr<Camera> camera;
> > +	std::vector<Stream> streams;
> >  
> >  	ret = cm->start();
> >  	if (ret) {
> > @@ -71,31 +120,62 @@ int main(int argc, char **argv)
> >  		return EXIT_FAILURE;
> >  	}
> >  
> > +	loop = new EventLoop(cm->eventDispatcher());
> > +
> > +	struct sigaction sa = {};
> > +	sa.sa_handler = &signalHandler;
> > +	sigaction(SIGINT, &sa, nullptr);
> > +
> >  	if (options.isSet(OptList)) {
> >  		std::cout << "Available cameras:" << std::endl;
> > -		for (const std::shared_ptr<Camera> &camera : cm->cameras())
> > -			std::cout << "- " << camera->name() << std::endl;
> > +		for (const std::shared_ptr<Camera> &cam : cm->cameras())
> > +			std::cout << "- " << cam->name() << std::endl;
> >  	}
> >  
> >  	if (options.isSet(OptCamera)) {
> > -		std::shared_ptr<Camera> cam = cm->get(options[OptCamera]);
> > -
> > -		if (cam) {
> > -			std::cout << "Using camera " << cam->name() << std::endl;
> > -		} else {
> > +		camera = cm->get(options[OptCamera]);
> > +		if (!camera) {
> >  			std::cout << "Camera " << options[OptCamera]
> >  				  << " not found" << std::endl;
> > +			goto out;
> > +		}
> > +
> > +		streams = camera->streams();
> > +		if (streams.size() != 1) {
> > +			std::cout << "Camera have " << streams.size()
> 
> Camera 'has' N streams...

Thanks :-)

> 
> > +				  << " streams, I only know how to work with 1"
> > +				  << std::endl;
> > +			goto out;
> > +		}
> > +
> > +		if (camera->acquire()) {
> > +			std::cout << "Failed to acquire camera" << std::endl;
> > +			goto out;
> >  		}
> > +
> > +		std::cout << "Using camera " << camera->name() << std::endl;
> >  	}
> >  
> > -	loop = new EventLoop(cm->eventDispatcher());
> > +	if (options.isSet(OptFormat)) {
> > +		if (!camera) {
> > +			std::cout << "Can't configure stream, no camera selected" << std::endl;
> > +			goto out_camera;
> > +		}
> >  
> > -	struct sigaction sa = {};
> > -	sa.sa_handler = &signalHandler;
> > -	sigaction(SIGINT, &sa, nullptr);
> > +		if (!configureStreams(camera.get(), streams)) {
> > +			std::cout << "Failed to configure camera" << std::endl;
> > +			goto out_camera;
> > +		}
> > +	}
> >  
> >  	ret = loop->exec();
> >  
> > +out_camera:
> > +	if (camera) {
> > +		camera->release();
> > +		camera.reset();
> 
> Uhm ... first a pointer call '->' then a direct member call ? '.' ?

camera is a shard_ptr<Camera*> so the ->release() access the Camera 
pointers that the shard_ptr<> holds while .reset() clears the 
shard_ptr<> of the reference to the Camera hence reducing the refcount 
by one and potentialy deleting the Camera object. 

> 
> 
> > +	}
> > +out:
> >  	delete loop;
> >  
> >  	cm->stop();
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund