[libcamera-devel] [PATCH v2 4/5] cam: Make use of StreamKeyValueParser

[Laurent Pinchart]

Hi Niklas,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

On Tue, Apr 28, 2020 at 12:05:28AM +0200, Niklas Söderlund wrote:
> Use the StreamOptionsParser helper to parse stream configuration from
> the command line.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/main.cpp | 69 +++++-------------------------------------------
>  1 file changed, 6 insertions(+), 63 deletions(-)
> 
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index ced4f567b8f38e00..c6814f21aa51cf41 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -16,7 +16,7 @@
>  #include "capture.h"
>  #include "event_loop.h"
>  #include "main.h"
> -#include "options.h"
> +#include "stream_options.h"

I'd keep the two files, up to you.

>  using namespace libcamera;
>  
> @@ -154,16 +154,7 @@ void CamApp::quit()
>  
>  int CamApp::parseOptions(int argc, char *argv[])
>  {
> -	KeyValueParser streamKeyValue;
> -	streamKeyValue.addOption("role", OptionString,
> -				 "Role for the stream (viewfinder, video, still, stillraw)",
> -				 ArgumentRequired);
> -	streamKeyValue.addOption("width", OptionInteger, "Width in pixels",
> -				 ArgumentRequired);
> -	streamKeyValue.addOption("height", OptionInteger, "Height in pixels",
> -				 ArgumentRequired);
> -	streamKeyValue.addOption("pixelformat", OptionInteger, "Pixel format",
> -				 ArgumentRequired);
> +	StreamKeyValueParser streamKeyValue;
>  
>  	OptionsParser parser;
>  	parser.addOption(OptCamera, OptionString,
> @@ -202,38 +193,7 @@ int CamApp::parseOptions(int argc, char *argv[])
>  
>  int CamApp::prepareConfig()
>  {
> -	StreamRoles roles;
> -
> -	if (options_.isSet(OptStream)) {
> -		const std::vector<OptionValue> &streamOptions =
> -			options_[OptStream].toArray();
> -
> -		/* Use roles and get a default configuration. */
> -		for (auto const &value : streamOptions) {
> -			KeyValueParser::Options opt = value.toKeyValues();
> -
> -			std::string role = opt.isSet("role")
> -					 ? opt["role"].toString()
> -					 : "viewfinder";
> -
> -			if (role == "viewfinder") {
> -				roles.push_back(StreamRole::Viewfinder);
> -			} else if (role == "video") {
> -				roles.push_back(StreamRole::VideoRecording);
> -			} else if (role == "still") {
> -				roles.push_back(StreamRole::StillCapture);
> -			} else if (role == "stillraw") {
> -				roles.push_back(StreamRole::StillCaptureRaw);
> -			} else {
> -				std::cerr << "Unknown stream role "
> -					  << role << std::endl;
> -				return -EINVAL;
> -			}
> -		}
> -	} else {
> -		/* If no configuration is provided assume a single video stream. */
> -		roles.push_back(StreamRole::VideoRecording);

This differs from the default in the parser. Not a big deal for now, but
I think it shows the default should likely be application-specific.

> -	}
> +	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
>  
>  	config_ = camera_->generateConfiguration(roles);
>  	if (!config_ || config_->size() != roles.size()) {
> @@ -243,26 +203,9 @@ int CamApp::prepareConfig()
>  	}
>  
>  	/* Apply configuration if explicitly requested. */
> -	if (options_.isSet(OptStream)) {
> -		const std::vector<OptionValue> &streamOptions =
> -			options_[OptStream].toArray();
> -
> -		unsigned int i = 0;
> -		for (auto const &value : streamOptions) {
> -			KeyValueParser::Options opt = value.toKeyValues();
> -			StreamConfiguration &cfg = config_->at(i++);
> -
> -			if (opt.isSet("width"))
> -				cfg.size.width = opt["width"];
> -
> -			if (opt.isSet("height"))
> -				cfg.size.height = opt["height"];
> -
> -			/* TODO: Translate 4CC string to ID. */
> -			if (opt.isSet("pixelformat"))
> -				cfg.pixelFormat = PixelFormat(opt["pixelformat"]);
> -		}
> -	}
> +	if (StreamKeyValueParser::updateConfiguration(config_.get(),
> +						      options_[OptStream]))
> +		return -EINVAL;

Maybe logging a message here ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  
>  	switch (config_->validate()) {
>  	case CameraConfiguration::Valid:

-- 
Regards,

Laurent Pinchart