[libcamera-devel,v2,3/5] cam: Add helper class to parse stream configuration

3580 diff mbox series

Create a new helper class StreamKeyValueParser to parse command line
options describing stream configurations. The goal is to share this new
class between cam and qcam.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/cam/meson.build        |   1 +
 src/cam/stream_options.cpp | 133 +++++++++++++++++++++++++++++++++++++
 src/cam/stream_options.h   |  32 +++++++++
 3 files changed, 166 insertions(+)
 create mode 100644 src/cam/stream_options.cpp
 create mode 100644 src/cam/stream_options.h

Hi Niklas,

Thank you for the patch.

On 28.04.2020 01:05, Niklas Söderlund wrote:
> Create a new helper class StreamKeyValueParser to parse command line
> options describing stream configurations. The goal is to share this new
> class between cam and qcam.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>   src/cam/meson.build        |   1 +
>   src/cam/stream_options.cpp | 133 +++++++++++++++++++++++++++++++++++++
>   src/cam/stream_options.h   |  32 +++++++++
>   3 files changed, 166 insertions(+)
>   create mode 100644 src/cam/stream_options.cpp
>   create mode 100644 src/cam/stream_options.h
> 
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index 2419d648bc17e02b..162d6333f94e4851 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -4,6 +4,7 @@ cam_sources = files([
>       'event_loop.cpp',
>       'main.cpp',
>       'options.cpp',
> +    'stream_options.cpp',
>   ])
>   
>   cam  = executable('cam', cam_sources,
> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> new file mode 100644
> index 0000000000000000..9f726a7e70628648
> --- /dev/null
> +++ b/src/cam/stream_options.cpp
> @@ -0,0 +1,133 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * stream_options.cpp - Helper to parse options for streams
> + */
> +#include "stream_options.h"
> +
> +#include <iostream>
> +
> +using namespace libcamera;
> +
> +StreamKeyValueParser::StreamKeyValueParser()
> +{
> +	addOption("role", OptionString,
> +		  "Role for the stream (viewfinder, video, still, stillraw)",
> +		  ArgumentRequired);
> +	addOption("width", OptionInteger, "Width in pixels",
> +		  ArgumentRequired);
> +	addOption("height", OptionInteger, "Height in pixels",
> +		  ArgumentRequired);
> +	addOption("pixelformat", OptionInteger, "Pixel format",
> +		  ArgumentRequired);
> +}
> +
> +KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
> +{
> +	KeyValueParser::Options options = KeyValueParser::parse(arguments);
> +	StreamRole role;
> +
> +	if (options.valid() && options.isSet("role") &&
> +	    parseRole(&role, options)) {
> +		std::cerr << "Unknown stream role "
> +			  << options["role"].toString() << std::endl;
> +		options.invalidate();
> +	}
> +
> +	return options;
> +}
> +
> +StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> +{
> +	const std::vector<OptionValue> &streamParameters = values.toArray();
> +
> +	/* If no configuration values to examine default to viewfinder. */
> +	if (!streamParameters.size())
> +		return { StreamRole::Viewfinder };
> +
> +	StreamRoles roles;
> +	for (auto const &value : streamParameters) {
> +		KeyValueParser::Options opts = value.toKeyValues();
> +		StreamRole role;
> +
> +		/* If role is invalid or not set default to viewfinder. */
> +		if (parseRole(&role, value))
> +			role = StreamRole::Viewfinder;
> +
> +		roles.push_back(role);
> +	}
> +
> +	return roles;
> +}
> +
> +int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> +					      const OptionValue &values)
> +{
> +	const std::vector<OptionValue> &streamParameters = values.toArray();
> +
> +	if (!config) {
> +		std::cerr << "No configuration provided" << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	/* If no configuration values nothing to do. */
> +	if (!streamParameters.size())
> +		return 0;
> +
> +	if (config->size() != streamParameters.size()) {
> +		std::cerr
> +			<< "Number of streams in configuration "
> +			<< config->size()
> +			<< " does not match number of streams parsed "
> +			<< streamParameters.size()
> +			<< std::endl;
> +		return -EINVAL;
> +	}
> +
> +	unsigned int i = 0;
> +	for (auto const &value : streamParameters) {
> +		KeyValueParser::Options opts = value.toKeyValues();
> +		StreamConfiguration &cfg = config->at(i++);
> +
> +		if (opts.isSet("width") && opts.isSet("height")) {
> +			cfg.size.width = opts["width"];
> +			cfg.size.height = opts["height"];
> +		}
> +
> +		/* TODO: Translate 4CC string to ID. */
> +		if (opts.isSet("pixelformat"))
> +			cfg.pixelFormat = PixelFormat(opts["pixelformat"]);

This is not quite related to this patch set, as it doesn't change the current behavior.

But with the current implementation this is not possible to specify a PixelFormat with
(nonzero) modifier from the command line.
Does it worth adding the "TODO:"?

Thanks,
Andrey

> +	}
> +
> +	return 0;
> +}
> +
> +int StreamKeyValueParser::parseRole(StreamRole *role,
> +				    const KeyValueParser::Options &options)
> +{
> +	bool found = false;
> +
> +	if (options.isSet("role")) {
> +		std::string name = options["role"].toString();
> +
> +		if (name == "viewfinder") {
> +			*role = StreamRole::Viewfinder;
> +			found = true;
> +		}
> +		if (name == "video") {
> +			*role = StreamRole::VideoRecording;
> +			found = true;
> +		}
> +		if (name == "still") {
> +			*role = StreamRole::StillCapture;
> +			found = true;
> +		}
> +		if (name == "stillraw") {
> +			*role = StreamRole::StillCaptureRaw;
> +			found = true;
> +		}
> +	}
> +
> +	return found ? 0 : -EINVAL;
> +}
> diff --git a/src/cam/stream_options.h b/src/cam/stream_options.h
> new file mode 100644
> index 0000000000000000..c90d8c4dc595814d
> --- /dev/null
> +++ b/src/cam/stream_options.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * stream_options.h - Helper to parse options for streams
> + */
> +#ifndef __CAM_STREAM_OPTIONS_H__
> +#define __CAM_STREAM_OPTIONS_H__
> +
> +#include <libcamera/camera.h>
> +
> +#include "options.h"
> +
> +using namespace libcamera;
> +
> +class StreamKeyValueParser : public KeyValueParser
> +{
> +public:
> +	StreamKeyValueParser();
> +
> +	KeyValueParser::Options parse(const char *arguments) override;
> +
> +	static StreamRoles roles(const OptionValue &values);
> +	static int updateConfiguration(CameraConfiguration *config,
> +				       const OptionValue &values);
> +
> +private:
> +	static int parseRole(StreamRole *role,
> +			     const KeyValueParser::Options &options);
> +};
> +
> +#endif /* __CAM_STREAM_OPTIONS_H__ */
>

Hi Andrey,

Thanks for your feedback.

On 2020-04-29 03:02:23 +0300, Andrey Konovalov wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On 28.04.2020 01:05, Niklas Söderlund wrote:
> > Create a new helper class StreamKeyValueParser to parse command line
> > options describing stream configurations. The goal is to share this new
> > class between cam and qcam.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >   src/cam/meson.build        |   1 +
> >   src/cam/stream_options.cpp | 133 +++++++++++++++++++++++++++++++++++++
> >   src/cam/stream_options.h   |  32 +++++++++
> >   3 files changed, 166 insertions(+)
> >   create mode 100644 src/cam/stream_options.cpp
> >   create mode 100644 src/cam/stream_options.h
> > 
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index 2419d648bc17e02b..162d6333f94e4851 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -4,6 +4,7 @@ cam_sources = files([
> >       'event_loop.cpp',
> >       'main.cpp',
> >       'options.cpp',
> > +    'stream_options.cpp',
> >   ])
> >   cam  = executable('cam', cam_sources,
> > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> > new file mode 100644
> > index 0000000000000000..9f726a7e70628648
> > --- /dev/null
> > +++ b/src/cam/stream_options.cpp
> > @@ -0,0 +1,133 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * stream_options.cpp - Helper to parse options for streams
> > + */
> > +#include "stream_options.h"
> > +
> > +#include <iostream>
> > +
> > +using namespace libcamera;
> > +
> > +StreamKeyValueParser::StreamKeyValueParser()
> > +{
> > +	addOption("role", OptionString,
> > +		  "Role for the stream (viewfinder, video, still, stillraw)",
> > +		  ArgumentRequired);
> > +	addOption("width", OptionInteger, "Width in pixels",
> > +		  ArgumentRequired);
> > +	addOption("height", OptionInteger, "Height in pixels",
> > +		  ArgumentRequired);
> > +	addOption("pixelformat", OptionInteger, "Pixel format",
> > +		  ArgumentRequired);
> > +}
> > +
> > +KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
> > +{
> > +	KeyValueParser::Options options = KeyValueParser::parse(arguments);
> > +	StreamRole role;
> > +
> > +	if (options.valid() && options.isSet("role") &&
> > +	    parseRole(&role, options)) {
> > +		std::cerr << "Unknown stream role "
> > +			  << options["role"].toString() << std::endl;
> > +		options.invalidate();
> > +	}
> > +
> > +	return options;
> > +}
> > +
> > +StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> > +{
> > +	const std::vector<OptionValue> &streamParameters = values.toArray();
> > +
> > +	/* If no configuration values to examine default to viewfinder. */
> > +	if (!streamParameters.size())
> > +		return { StreamRole::Viewfinder };
> > +
> > +	StreamRoles roles;
> > +	for (auto const &value : streamParameters) {
> > +		KeyValueParser::Options opts = value.toKeyValues();
> > +		StreamRole role;
> > +
> > +		/* If role is invalid or not set default to viewfinder. */
> > +		if (parseRole(&role, value))
> > +			role = StreamRole::Viewfinder;
> > +
> > +		roles.push_back(role);
> > +	}
> > +
> > +	return roles;
> > +}
> > +
> > +int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> > +					      const OptionValue &values)
> > +{
> > +	const std::vector<OptionValue> &streamParameters = values.toArray();
> > +
> > +	if (!config) {
> > +		std::cerr << "No configuration provided" << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* If no configuration values nothing to do. */
> > +	if (!streamParameters.size())
> > +		return 0;
> > +
> > +	if (config->size() != streamParameters.size()) {
> > +		std::cerr
> > +			<< "Number of streams in configuration "
> > +			<< config->size()
> > +			<< " does not match number of streams parsed "
> > +			<< streamParameters.size()
> > +			<< std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	unsigned int i = 0;
> > +	for (auto const &value : streamParameters) {
> > +		KeyValueParser::Options opts = value.toKeyValues();
> > +		StreamConfiguration &cfg = config->at(i++);
> > +
> > +		if (opts.isSet("width") && opts.isSet("height")) {
> > +			cfg.size.width = opts["width"];
> > +			cfg.size.height = opts["height"];
> > +		}
> > +
> > +		/* TODO: Translate 4CC string to ID. */
> > +		if (opts.isSet("pixelformat"))
> > +			cfg.pixelFormat = PixelFormat(opts["pixelformat"]);
> 
> This is not quite related to this patch set, as it doesn't change the current behavior.
> 
> But with the current implementation this is not possible to specify a PixelFormat with
> (nonzero) modifier from the command line.
> Does it worth adding the "TODO:"?

That is a good question. I'm not yet sure on how we shall create a 
string representation of a FourCC + modifier in a neat way. But 
extending the TODO to also mention we need to deal with modifiers is a 
good idea.

> 
> Thanks,
> Andrey
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int StreamKeyValueParser::parseRole(StreamRole *role,
> > +				    const KeyValueParser::Options &options)
> > +{
> > +	bool found = false;
> > +
> > +	if (options.isSet("role")) {
> > +		std::string name = options["role"].toString();
> > +
> > +		if (name == "viewfinder") {
> > +			*role = StreamRole::Viewfinder;
> > +			found = true;
> > +		}
> > +		if (name == "video") {
> > +			*role = StreamRole::VideoRecording;
> > +			found = true;
> > +		}
> > +		if (name == "still") {
> > +			*role = StreamRole::StillCapture;
> > +			found = true;
> > +		}
> > +		if (name == "stillraw") {
> > +			*role = StreamRole::StillCaptureRaw;
> > +			found = true;
> > +		}
> > +	}
> > +
> > +	return found ? 0 : -EINVAL;
> > +}
> > diff --git a/src/cam/stream_options.h b/src/cam/stream_options.h
> > new file mode 100644
> > index 0000000000000000..c90d8c4dc595814d
> > --- /dev/null
> > +++ b/src/cam/stream_options.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * stream_options.h - Helper to parse options for streams
> > + */
> > +#ifndef __CAM_STREAM_OPTIONS_H__
> > +#define __CAM_STREAM_OPTIONS_H__
> > +
> > +#include <libcamera/camera.h>
> > +
> > +#include "options.h"
> > +
> > +using namespace libcamera;
> > +
> > +class StreamKeyValueParser : public KeyValueParser
> > +{
> > +public:
> > +	StreamKeyValueParser();
> > +
> > +	KeyValueParser::Options parse(const char *arguments) override;
> > +
> > +	static StreamRoles roles(const OptionValue &values);
> > +	static int updateConfiguration(CameraConfiguration *config,
> > +				       const OptionValue &values);
> > +
> > +private:
> > +	static int parseRole(StreamRole *role,
> > +			     const KeyValueParser::Options &options);
> > +};
> > +
> > +#endif /* __CAM_STREAM_OPTIONS_H__ */
> >

Hi Niklas,

Thank you for the patch.

On Tue, Apr 28, 2020 at 12:05:27AM +0200, Niklas Söderlund wrote:
> Create a new helper class StreamKeyValueParser to parse command line
> options describing stream configurations. The goal is to share this new
> class between cam and qcam.

This looks good. I have a few comments, but they don't need to be
addressed now.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/meson.build        |   1 +
>  src/cam/stream_options.cpp | 133 +++++++++++++++++++++++++++++++++++++
>  src/cam/stream_options.h   |  32 +++++++++
>  3 files changed, 166 insertions(+)
>  create mode 100644 src/cam/stream_options.cpp
>  create mode 100644 src/cam/stream_options.h
> 
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index 2419d648bc17e02b..162d6333f94e4851 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -4,6 +4,7 @@ cam_sources = files([
>      'event_loop.cpp',
>      'main.cpp',
>      'options.cpp',
> +    'stream_options.cpp',
>  ])
>  
>  cam  = executable('cam', cam_sources,
> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> new file mode 100644
> index 0000000000000000..9f726a7e70628648
> --- /dev/null
> +++ b/src/cam/stream_options.cpp
> @@ -0,0 +1,133 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * stream_options.cpp - Helper to parse options for streams
> + */
> +#include "stream_options.h"
> +
> +#include <iostream>
> +
> +using namespace libcamera;
> +
> +StreamKeyValueParser::StreamKeyValueParser()
> +{
> +	addOption("role", OptionString,
> +		  "Role for the stream (viewfinder, video, still, stillraw)",
> +		  ArgumentRequired);
> +	addOption("width", OptionInteger, "Width in pixels",
> +		  ArgumentRequired);
> +	addOption("height", OptionInteger, "Height in pixels",
> +		  ArgumentRequired);
> +	addOption("pixelformat", OptionInteger, "Pixel format",
> +		  ArgumentRequired);
> +}
> +
> +KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
> +{
> +	KeyValueParser::Options options = KeyValueParser::parse(arguments);
> +	StreamRole role;
> +
> +	if (options.valid() && options.isSet("role") &&
> +	    parseRole(&role, options)) {
> +		std::cerr << "Unknown stream role "
> +			  << options["role"].toString() << std::endl;

Error messages should be printed with an application-specific API
(qError() for qcam for instance), but that's for later

> +		options.invalidate();
> +	}
> +
> +	return options;
> +}
> +
> +StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> +{
> +	const std::vector<OptionValue> &streamParameters = values.toArray();
> +
> +	/* If no configuration values to examine default to viewfinder. */
> +	if (!streamParameters.size())

	if (streamParameters.empty())

> +		return { StreamRole::Viewfinder };

Would it make sense to return an empty vector here ? I could imagine
that the default could be application-specific. Can be done later too.

> +
> +	StreamRoles roles;
> +	for (auto const &value : streamParameters) {
> +		KeyValueParser::Options opts = value.toKeyValues();
> +		StreamRole role;
> +
> +		/* If role is invalid or not set default to viewfinder. */
> +		if (parseRole(&role, value))
> +			role = StreamRole::Viewfinder;
> +
> +		roles.push_back(role);
> +	}
> +
> +	return roles;
> +}
> +
> +int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> +					      const OptionValue &values)
> +{
> +	const std::vector<OptionValue> &streamParameters = values.toArray();
> +
> +	if (!config) {
> +		std::cerr << "No configuration provided" << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	/* If no configuration values nothing to do. */
> +	if (!streamParameters.size())
> +		return 0;
> +
> +	if (config->size() != streamParameters.size()) {
> +		std::cerr
> +			<< "Number of streams in configuration "
> +			<< config->size()
> +			<< " does not match number of streams parsed "
> +			<< streamParameters.size()
> +			<< std::endl;
> +		return -EINVAL;
> +	}
> +
> +	unsigned int i = 0;
> +	for (auto const &value : streamParameters) {
> +		KeyValueParser::Options opts = value.toKeyValues();
> +		StreamConfiguration &cfg = config->at(i++);
> +
> +		if (opts.isSet("width") && opts.isSet("height")) {
> +			cfg.size.width = opts["width"];
> +			cfg.size.height = opts["height"];
> +		}
> +
> +		/* TODO: Translate 4CC string to ID. */

/TODO:/\todo/

> +		if (opts.isSet("pixelformat"))
> +			cfg.pixelFormat = PixelFormat(opts["pixelformat"]);
> +	}
> +
> +	return 0;
> +}
> +
> +int StreamKeyValueParser::parseRole(StreamRole *role,
> +				    const KeyValueParser::Options &options)

Should this return bool (true if parsing succeeds, false otherwise) ?

> +{
> +	bool found = false;
> +
> +	if (options.isSet("role")) {
> +		std::string name = options["role"].toString();
> +
> +		if (name == "viewfinder") {
> +			*role = StreamRole::Viewfinder;
> +			found = true;
> +		}
> +		if (name == "video") {
> +			*role = StreamRole::VideoRecording;
> +			found = true;
> +		}
> +		if (name == "still") {
> +			*role = StreamRole::StillCapture;
> +			found = true;
> +		}
> +		if (name == "stillraw") {
> +			*role = StreamRole::StillCaptureRaw;
> +			found = true;
> +		}
> +	}
> +
> +	return found ? 0 : -EINVAL;

You could simplify this.

	if (!options.isSet("role"))
		return -EINVAL;

	std::string name = options["role"].toString();

	if (name == "viewfinder") {
		*role = StreamRole::Viewfinder;
		return 0;
	} else if (name == "video") {
		*role = StreamRole::VideoRecording;
		return 0;
	} else if (name == "still") {
		*role = StreamRole::StillCapture;
		return 0;
	} else if (name == "stillraw") {
		*role = StreamRole::StillCaptureRaw;
		return 0;
	} else {
		return -EINVAL;
	}

Please consider the changes you think are appropriate, and apply

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

> +}
> diff --git a/src/cam/stream_options.h b/src/cam/stream_options.h
> new file mode 100644
> index 0000000000000000..c90d8c4dc595814d
> --- /dev/null
> +++ b/src/cam/stream_options.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * stream_options.h - Helper to parse options for streams
> + */
> +#ifndef __CAM_STREAM_OPTIONS_H__
> +#define __CAM_STREAM_OPTIONS_H__
> +
> +#include <libcamera/camera.h>
> +
> +#include "options.h"
> +
> +using namespace libcamera;
> +
> +class StreamKeyValueParser : public KeyValueParser
> +{
> +public:
> +	StreamKeyValueParser();
> +
> +	KeyValueParser::Options parse(const char *arguments) override;
> +
> +	static StreamRoles roles(const OptionValue &values);
> +	static int updateConfiguration(CameraConfiguration *config,
> +				       const OptionValue &values);
> +
> +private:
> +	static int parseRole(StreamRole *role,
> +			     const KeyValueParser::Options &options);
> +};
> +
> +#endif /* __CAM_STREAM_OPTIONS_H__ */