[libcamera-devel] [PATCH v2 3/5] cam: Add helper class to parse stream configuration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Apr 30 18:40:28 CEST 2020
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 at 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 at 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__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list