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

[Kieran Bingham]

Hi Niklas,

On 29/01/2019 13:34, Niklas Söderlund wrote:
> 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

Ah - that's clear - this is fine then.

> 
>>
>>> +
>>>  	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. 


Of course :) Ok - no issues there either then!
--
Kieran


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

-- 
Regards
--
Kieran