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

[Laurent Pinchart]

Hi Niklas,

On Tue, Apr 28, 2020 at 12:05:29AM +0200, Niklas Söderlund wrote:
> Use the StreamKeyValueParser helper to parse stream configuration from
> the command line. This extends qcam to accept role hints and pixel
> format in addition to a size.
> 
> Currently only one viewfinder stream is supported, add a check to keep
> this behavior. Going forward this restriction will be lifted to support
> more then one stream.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/qcam/main.cpp        | 12 ++++--------
>  src/qcam/main_window.cpp | 35 ++++++++++++++++++-----------------
>  src/qcam/main_window.h   |  4 ++--
>  src/qcam/meson.build     |  1 +
>  4 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> index 862d714f467c04f9..cd73fa764614e7e7 100644
> --- a/src/qcam/main.cpp
> +++ b/src/qcam/main.cpp
> @@ -13,8 +13,8 @@
>  
>  #include <libcamera/camera_manager.h>
>  
> +#include "../cam/stream_options.h"
>  #include "main_window.h"
> -#include "../cam/options.h"

As with the previous patch, I'd keep both.

>  
>  void signalHandler(int signal)
>  {
> @@ -24,11 +24,7 @@ void signalHandler(int signal)
>  
>  OptionsParser::Options parseOptions(int argc, char *argv[])
>  {
> -	KeyValueParser sizeParser;
> -	sizeParser.addOption("width", OptionInteger, "Width in pixels",
> -			     ArgumentRequired);
> -	sizeParser.addOption("height", OptionInteger, "Height in pixels",
> -			     ArgumentRequired);
> +	StreamKeyValueParser streamKeyValue;
>  
>  	OptionsParser parser;
>  	parser.addOption(OptCamera, OptionString,
> @@ -36,8 +32,8 @@ OptionsParser::Options parseOptions(int argc, char *argv[])
>  			 ArgumentRequired, "camera");
>  	parser.addOption(OptHelp, OptionNone, "Display this help message",
>  			 "help");
> -	parser.addOption(OptSize, &sizeParser, "Set the stream size",
> -			 "size", true);
> +	parser.addOption(OptStream, &streamKeyValue,
> +			 "Set configuration of a camera stream", "stream", true);
>  
>  	OptionsParser::Options options = parser.parse(argc, argv);
>  	if (options.isSet(OptHelp))
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index ed0cad417d62abea..ee779728fc630da8 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -277,29 +277,25 @@ void MainWindow::toggleCapture(bool start)
>   */
>  int MainWindow::startCapture()
>  {
> +	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
>  	int ret;
>  
> +	/* Verify roles are supported. */
> +	if (roles.size() != 1) {
> +		qWarning() << "Only one stream supported";

s/qWarning()/qCritical()/ as we can't continue ? Same below.

> +		return -EINVAL;
> +	}
> +
> +	if (roles[0] != StreamRole::Viewfinder) {
> +		qWarning() << "Only viewfinder supported";
> +		return -EINVAL;
> +	}
> +
>  	/* Configure the camera. */
> -	config_ = camera_->generateConfiguration({ StreamRole::Viewfinder });
> +	config_ = camera_->generateConfiguration(roles);
>  
>  	StreamConfiguration &cfg = config_->at(0);
>  
> -	if (options_.isSet(OptSize)) {
> -		const std::vector<OptionValue> &sizeOptions =
> -			options_[OptSize].toArray();
> -
> -		/* Set desired stream size if requested. */
> -		for (const auto &value : sizeOptions) {
> -			KeyValueParser::Options opt = value.toKeyValues();
> -
> -			if (opt.isSet("width"))
> -				cfg.size.width = opt["width"];
> -
> -			if (opt.isSet("height"))
> -				cfg.size.height = opt["height"];
> -		}
> -	}
> -
>  	/* Use a format supported by the viewfinder if available. */
>  	std::vector<PixelFormat> formats = cfg.formats().pixelformats();
>  	for (const PixelFormat &format : viewfinder_->nativeFormats()) {
> @@ -313,6 +309,11 @@ int MainWindow::startCapture()
>  		}
>  	}
>  
> +	/* Allow user to override configuration. */
> +	if (StreamKeyValueParser::updateConfiguration(config_.get(),
> +						      options_[OptStream]))

Error here too ?

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

> +		return -EINVAL;
> +
>  	CameraConfiguration::Status validation = config_->validate();
>  	if (validation == CameraConfiguration::Invalid) {
>  		qWarning() << "Failed to create valid camera configuration";
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 5d6251c830707a79..5e9d9b8d9c6b2d6d 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -23,7 +23,7 @@
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/stream.h>
>  
> -#include "../cam/options.h"
> +#include "../cam/stream_options.h"
>  #include "viewfinder.h"
>  
>  using namespace libcamera;
> @@ -33,7 +33,7 @@ class QAction;
>  enum {
>  	OptCamera = 'c',
>  	OptHelp = 'h',
> -	OptSize = 's',
> +	OptStream = 's',
>  };
>  
>  class MainWindow : public QMainWindow
> diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> index c256d06f8ccfc0ae..895264be4a3388f4 100644
> --- a/src/qcam/meson.build
> +++ b/src/qcam/meson.build
> @@ -3,6 +3,7 @@ qcam_sources = files([
>      'main.cpp',
>      'main_window.cpp',
>      '../cam/options.cpp',
> +    '../cam/stream_options.cpp',
>      'viewfinder.cpp',
>  ])
>  

-- 
Regards,

Laurent Pinchart