[{"id":4631,"web_url":"https://patchwork.libcamera.org/comment/4631/","msgid":"<84816f26-ce50-25c6-a447-8f3cd3044431@linaro.org>","date":"2020-04-29T00:02:23","subject":"Re: [libcamera-devel] [PATCH v2 3/5] cam: Add helper class to parse\n\tstream configuration","submitter":{"id":25,"url":"https://patchwork.libcamera.org/api/people/25/","name":"Andrey Konovalov","email":"andrey.konovalov@linaro.org"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn 28.04.2020 01:05, Niklas Söderlund wrote:\n> Create a new helper class StreamKeyValueParser to parse command line\n> options describing stream configurations. The goal is to share this new\n> class between cam and qcam.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/meson.build | 1 +\n> src/cam/stream_options.cpp | 133 +++++++++++++++++++++++++++++++++++++\n> src/cam/stream_options.h | 32 +++++++++\n> 3 files changed, 166 insertions(+)\n> create mode 100644 src/cam/stream_options.cpp\n> create mode 100644 src/cam/stream_options.h\n> \n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 2419d648bc17e02b..162d6333f94e4851 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -4,6 +4,7 @@ cam_sources = files([\n> 'event_loop.cpp',\n> 'main.cpp',\n> 'options.cpp',\n> + 'stream_options.cpp',\n> ])\n> \n> cam = executable('cam', cam_sources,\n> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> new file mode 100644\n> index 0000000000000000..9f726a7e70628648\n> --- /dev/null\n> +++ b/src/cam/stream_options.cpp\n> @@ -0,0 +1,133 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * stream_options.cpp - Helper to parse options for streams\n> + */\n> +#include \"stream_options.h\"\n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +StreamKeyValueParser::StreamKeyValueParser()\n> +{\n> +\taddOption(\"role\", OptionString,\n> +\t\t \"Role for the stream (viewfinder, video, still, stillraw)\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"width\", OptionInteger, \"Width in pixels\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"height\", OptionInteger, \"Height in pixels\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> +\t\t ArgumentRequired);\n> +}\n> +\n> +KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)\n> +{\n> +\tKeyValueParser::Options options = KeyValueParser::parse(arguments);\n> +\tStreamRole role;\n> +\n> +\tif (options.valid() && options.isSet(\"role\") &&\n> +\t parseRole(&role, options)) {\n> +\t\tstd::cerr << \"Unknown stream role \"\n> +\t\t\t << options[\"role\"].toString() << std::endl;\n> +\t\toptions.invalidate();\n> +\t}\n> +\n> +\treturn options;\n> +}\n> +\n> +StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> +{\n> +\tconst std::vector &streamParameters = values.toArray();\n> +\n> +\t/* If no configuration values to examine default to viewfinder. */\n> +\tif (!streamParameters.size())\n> +\t\treturn { StreamRole::Viewfinder };\n> +\n> +\tStreamRoles roles;\n> +\tfor (auto const &value : streamParameters) {\n> +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> +\t\tStreamRole role;\n> +\n> +\t\t/* If role is invalid or not set default to viewfinder. */\n> +\t\tif (parseRole(&role, value))\n> +\t\t\trole = StreamRole::Viewfinder;\n> +\n> +\t\troles.push_back(role);\n> +\t}\n> +\n> +\treturn roles;\n> +}\n> +\n> +int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> +\t\t\t\t\t const OptionValue &values)\n> +{\n> +\tconst std::vector &streamParameters = values.toArray();\n> +\n> +\tif (!config) {\n> +\t\tstd::cerr << \"No configuration provided\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* If no configuration values nothing to do. */\n> +\tif (!streamParameters.size())\n> +\t\treturn 0;\n> +\n> +\tif (config->size() != streamParameters.size()) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Number of streams in configuration \"\n> +\t\t\t<< config->size()\n> +\t\t\t<< \" does not match number of streams parsed \"\n> +\t\t\t<< streamParameters.size()\n> +\t\t\t<< std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tunsigned int i = 0;\n> +\tfor (auto const &value : streamParameters) {\n> +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> +\t\tStreamConfiguration &cfg = config->at(i++);\n> +\n> +\t\tif (opts.isSet(\"width\") && opts.isSet(\"height\")) {\n> +\t\t\tcfg.size.width = opts[\"width\"];\n> +\t\t\tcfg.size.height = opts[\"height\"];\n> +\t\t}\n> +\n> +\t\t/* TODO: Translate 4CC string to ID. */\n> +\t\tif (opts.isSet(\"pixelformat\"))\n> +\t\t\tcfg.pixelFormat = PixelFormat(opts[\"pixelformat\"]);\n\nThis is not quite related to this patch set, as it doesn't change the current behavior.\n\nBut with the current implementation this is not possible to specify a PixelFormat with\n(nonzero) modifier from the command line.\nDoes it worth adding the \"TODO:\"?\n\nThanks,\nAndrey\n\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int StreamKeyValueParser::parseRole(StreamRole *role,\n> +\t\t\t\t const KeyValueParser::Options &options)\n> +{\n> +\tbool found = false;\n> +\n> +\tif (options.isSet(\"role\")) {\n> +\t\tstd::string name = options[\"role\"].toString();\n> +\n> +\t\tif (name == \"viewfinder\") {\n> +\t\t\t*role = StreamRole::Viewfinder;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t\tif (name == \"video\") {\n> +\t\t\t*role = StreamRole::VideoRecording;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t\tif (name == \"still\") {\n> +\t\t\t*role = StreamRole::StillCapture;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t\tif (name == \"stillraw\") {\n> +\t\t\t*role = StreamRole::StillCaptureRaw;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t}\n> +\n> +\treturn found ? 0 : -EINVAL;\n> +}\n> diff --git a/src/cam/stream_options.h b/src/cam/stream_options.h\n> new file mode 100644\n> index 0000000000000000..c90d8c4dc595814d\n> --- /dev/null\n> +++ b/src/cam/stream_options.h\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * stream_options.h - Helper to parse options for streams\n> + */\n> +#ifndef __CAM_STREAM_OPTIONS_H__\n> +#define __CAM_STREAM_OPTIONS_H__\n> +\n> +#include \n> +\n> +#include \"options.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class StreamKeyValueParser : public KeyValueParser\n> +{\n> +public:\n> +\tStreamKeyValueParser();\n> +\n> +\tKeyValueParser::Options parse(const char *arguments) override;\n> +\n> +\tstatic StreamRoles roles(const OptionValue &values);\n> +\tstatic int updateConfiguration(CameraConfiguration *config,\n> +\t\t\t\t const OptionValue &values);\n> +\n> +private:\n> +\tstatic int parseRole(StreamRole *role,\n> +\t\t\t const KeyValueParser::Options &options);\n> +};\n> +\n> +#endif /* __CAM_STREAM_OPTIONS_H__ */\n>","headers":{"Return-Path":"","Received":["from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A100603F9\n\tfor ;\n\tWed, 29 Apr 2020 02:02:26 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id u15so748507ljd.3\n\tfor ;\n\tTue, 28 Apr 2020 17:02:26 -0700 (PDT)","from [192.168.118.216] (37-144-159-139.broadband.corbina.ru.\n\t[37.144.159.139]) by smtp.gmail.com with ESMTPSA id\n\ta28sm777028lfo.47.2020.04.28.17.02.24\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 28 Apr 2020 17:02:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=linaro.org\n\theader.i=@linaro.org header.b=\"VSjQqBQF\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=qvc5MvWcbWDk1ZZPnQ61rKEL/u09T1f/DLXToRMXymM=;\n\tb=VSjQqBQFXTHBkXlXc771ql0PSIvdCKn/IUnZGF+PDJk39+6J5FlYZ9r2NxWsjXYw9G\n\tNDwAdPrhBKDIdgT1cZnt2bQ50A9ghIXzK/H1ac+RzOsWwKRkWYi1Yw3x2nJp05Gst57N\n\tz6SxzIV1knFRNd0fcSadXj1Auoxw2VuP/RSVA4nPGelwgvH6PdSxOEwNzTAcvWwJ1UDA\n\tDwYIQfdivWxXEPagLyPlaD7iwDjL9KpYqzbsMjiDE34HgHN0qg1TbjG1nJfO9/YST46b\n\tA4Fs0oEJn6q7obyefoPQGH6wi08EJwQnxgGFBFqXP7jix9CxKe0NQ3JPW/AgtvTxLEBC\n\t+zDQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=qvc5MvWcbWDk1ZZPnQ61rKEL/u09T1f/DLXToRMXymM=;\n\tb=Btfc8Sh3EO9oCv/m/sVjEBjVdBKpl3lv0HtOddkRw6m03+Mnq4KHIvRdQUPcMq9Q3k\n\tzqoAC1L09lPTpO26R8CRuDlbgc3EhlXT6xvWbEydAf9Uba22O8NCjRIhFZ61oFoCkPXX\n\t0LU/cYuncK5yq9LkxxbZ3Ij1cHt6eUr2gKXRvi2yd4mk0FnyBHjn5xvxg+x87BkrdLPo\n\tdwq4DT5rlax3NQSr5fHNVkWXAi388wZOGJyTdYds9/hz3rO5w4VJRFIXE14f75LkVnJ6\n\toIeYIsL4jIE1ZahkcCmYXNf1XNjs8lNJYmJ2VjHkMoMjlV9B1qvyFpwlxgANtNJpW1MI\n\tbIUA==","X-Gm-Message-State":"AGi0PuY0iqnx+pBzlEpLOAAOJkYcJnfXe5t5dOadVALtiFM/XZXdOleW\n\tirvOOD+OuafD1LnrjrTMwP5UuA==","X-Google-Smtp-Source":"APiQypLBGs2AgJlrq4AsZt/TAc1E7qqiTfIBF3Hq8rS0/OcTLfVKrOXZwRpvjUXari8MwTrkzBntNg==","X-Received":"by 2002:a05:651c:390:: with SMTP id\n\te16mr19358636ljp.186.1588118545361; \n\tTue, 28 Apr 2020 17:02:25 -0700 (PDT)","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","References":"<20200427220529.1085074-1-niklas.soderlund@ragnatech.se>\n\t<20200427220529.1085074-4-niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","From":"Andrey Konovalov ","Message-ID":"<84816f26-ce50-25c6-a447-8f3cd3044431@linaro.org>","Date":"Wed, 29 Apr 2020 03:02:23 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.7.0","MIME-Version":"1.0","In-Reply-To":"<20200427220529.1085074-4-niklas.soderlund@ragnatech.se>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] cam: Add helper class to parse\n\tstream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 29 Apr 2020 00:02:26 -0000"}},{"id":4632,"web_url":"https://patchwork.libcamera.org/comment/4632/","msgid":"<20200429000748.GD1372619@oden.dyn.berto.se>","date":"2020-04-29T00:07:48","subject":"Re: [libcamera-devel] [PATCH v2 3/5] cam: Add helper class to parse\n\tstream configuration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Andrey,\n\nThanks for your feedback.\n\nOn 2020-04-29 03:02:23 +0300, Andrey Konovalov wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On 28.04.2020 01:05, Niklas Söderlund wrote:\n> > Create a new helper class StreamKeyValueParser to parse command line\n> > options describing stream configurations. The goal is to share this new\n> > class between cam and qcam.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/meson.build | 1 +\n> > src/cam/stream_options.cpp | 133 +++++++++++++++++++++++++++++++++++++\n> > src/cam/stream_options.h | 32 +++++++++\n> > 3 files changed, 166 insertions(+)\n> > create mode 100644 src/cam/stream_options.cpp\n> > create mode 100644 src/cam/stream_options.h\n> > \n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index 2419d648bc17e02b..162d6333f94e4851 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -4,6 +4,7 @@ cam_sources = files([\n> > 'event_loop.cpp',\n> > 'main.cpp',\n> > 'options.cpp',\n> > + 'stream_options.cpp',\n> > ])\n> > cam = executable('cam', cam_sources,\n> > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> > new file mode 100644\n> > index 0000000000000000..9f726a7e70628648\n> > --- /dev/null\n> > +++ b/src/cam/stream_options.cpp\n> > @@ -0,0 +1,133 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * stream_options.cpp - Helper to parse options for streams\n> > + */\n> > +#include \"stream_options.h\"\n> > +\n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +StreamKeyValueParser::StreamKeyValueParser()\n> > +{\n> > +\taddOption(\"role\", OptionString,\n> > +\t\t \"Role for the stream (viewfinder, video, still, stillraw)\",\n> > +\t\t ArgumentRequired);\n> > +\taddOption(\"width\", OptionInteger, \"Width in pixels\",\n> > +\t\t ArgumentRequired);\n> > +\taddOption(\"height\", OptionInteger, \"Height in pixels\",\n> > +\t\t ArgumentRequired);\n> > +\taddOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> > +\t\t ArgumentRequired);\n> > +}\n> > +\n> > +KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)\n> > +{\n> > +\tKeyValueParser::Options options = KeyValueParser::parse(arguments);\n> > +\tStreamRole role;\n> > +\n> > +\tif (options.valid() && options.isSet(\"role\") &&\n> > +\t parseRole(&role, options)) {\n> > +\t\tstd::cerr << \"Unknown stream role \"\n> > +\t\t\t << options[\"role\"].toString() << std::endl;\n> > +\t\toptions.invalidate();\n> > +\t}\n> > +\n> > +\treturn options;\n> > +}\n> > +\n> > +StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> > +{\n> > +\tconst std::vector &streamParameters = values.toArray();\n> > +\n> > +\t/* If no configuration values to examine default to viewfinder. */\n> > +\tif (!streamParameters.size())\n> > +\t\treturn { StreamRole::Viewfinder };\n> > +\n> > +\tStreamRoles roles;\n> > +\tfor (auto const &value : streamParameters) {\n> > +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> > +\t\tStreamRole role;\n> > +\n> > +\t\t/* If role is invalid or not set default to viewfinder. */\n> > +\t\tif (parseRole(&role, value))\n> > +\t\t\trole = StreamRole::Viewfinder;\n> > +\n> > +\t\troles.push_back(role);\n> > +\t}\n> > +\n> > +\treturn roles;\n> > +}\n> > +\n> > +int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> > +\t\t\t\t\t const OptionValue &values)\n> > +{\n> > +\tconst std::vector &streamParameters = values.toArray();\n> > +\n> > +\tif (!config) {\n> > +\t\tstd::cerr << \"No configuration provided\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/* If no configuration values nothing to do. */\n> > +\tif (!streamParameters.size())\n> > +\t\treturn 0;\n> > +\n> > +\tif (config->size() != streamParameters.size()) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Number of streams in configuration \"\n> > +\t\t\t<< config->size()\n> > +\t\t\t<< \" does not match number of streams parsed \"\n> > +\t\t\t<< streamParameters.size()\n> > +\t\t\t<< std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tunsigned int i = 0;\n> > +\tfor (auto const &value : streamParameters) {\n> > +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> > +\t\tStreamConfiguration &cfg = config->at(i++);\n> > +\n> > +\t\tif (opts.isSet(\"width\") && opts.isSet(\"height\")) {\n> > +\t\t\tcfg.size.width = opts[\"width\"];\n> > +\t\t\tcfg.size.height = opts[\"height\"];\n> > +\t\t}\n> > +\n> > +\t\t/* TODO: Translate 4CC string to ID. */\n> > +\t\tif (opts.isSet(\"pixelformat\"))\n> > +\t\t\tcfg.pixelFormat = PixelFormat(opts[\"pixelformat\"]);\n> \n> This is not quite related to this patch set, as it doesn't change the current behavior.\n> \n> But with the current implementation this is not possible to specify a PixelFormat with\n> (nonzero) modifier from the command line.\n> Does it worth adding the \"TODO:\"?\n\nThat is a good question. I'm not yet sure on how we shall create a \nstring representation of a FourCC + modifier in a neat way. But \nextending the TODO to also mention we need to deal with modifiers is a \ngood idea.\n\n> \n> Thanks,\n> Andrey\n> \n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int StreamKeyValueParser::parseRole(StreamRole *role,\n> > +\t\t\t\t const KeyValueParser::Options &options)\n> > +{\n> > +\tbool found = false;\n> > +\n> > +\tif (options.isSet(\"role\")) {\n> > +\t\tstd::string name = options[\"role\"].toString();\n> > +\n> > +\t\tif (name == \"viewfinder\") {\n> > +\t\t\t*role = StreamRole::Viewfinder;\n> > +\t\t\tfound = true;\n> > +\t\t}\n> > +\t\tif (name == \"video\") {\n> > +\t\t\t*role = StreamRole::VideoRecording;\n> > +\t\t\tfound = true;\n> > +\t\t}\n> > +\t\tif (name == \"still\") {\n> > +\t\t\t*role = StreamRole::StillCapture;\n> > +\t\t\tfound = true;\n> > +\t\t}\n> > +\t\tif (name == \"stillraw\") {\n> > +\t\t\t*role = StreamRole::StillCaptureRaw;\n> > +\t\t\tfound = true;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn found ? 0 : -EINVAL;\n> > +}\n> > diff --git a/src/cam/stream_options.h b/src/cam/stream_options.h\n> > new file mode 100644\n> > index 0000000000000000..c90d8c4dc595814d\n> > --- /dev/null\n> > +++ b/src/cam/stream_options.h\n> > @@ -0,0 +1,32 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * stream_options.h - Helper to parse options for streams\n> > + */\n> > +#ifndef __CAM_STREAM_OPTIONS_H__\n> > +#define __CAM_STREAM_OPTIONS_H__\n> > +\n> > +#include \n> > +\n> > +#include \"options.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +class StreamKeyValueParser : public KeyValueParser\n> > +{\n> > +public:\n> > +\tStreamKeyValueParser();\n> > +\n> > +\tKeyValueParser::Options parse(const char *arguments) override;\n> > +\n> > +\tstatic StreamRoles roles(const OptionValue &values);\n> > +\tstatic int updateConfiguration(CameraConfiguration *config,\n> > +\t\t\t\t const OptionValue &values);\n> > +\n> > +private:\n> > +\tstatic int parseRole(StreamRole *role,\n> > +\t\t\t const KeyValueParser::Options &options);\n> > +};\n> > +\n> > +#endif /* __CAM_STREAM_OPTIONS_H__ */\n> >","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1C6F603F9\n\tfor ;\n\tWed, 29 Apr 2020 02:07:50 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id y4so723869ljn.7\n\tfor ;\n\tTue, 28 Apr 2020 17:07:50 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tx29sm793426lfn.64.2020.04.28.17.07.49\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 28 Apr 2020 17:07:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"A2sjMPij\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=GF3Et6kVj7+hJM8zVtvpQWDRjwVFS4rrFOpLLj7QlVY=;\n\tb=A2sjMPijXdbOFq7EV7xJH/cuIC7J0H+xbVKyIGpWkrsqxoBMrGAmGyQIkFKd4wCkT5\n\tBIGTVvxfojx76rWR9aC2ALFBoiJxM0z0Vcz3Hg/eQT9itNEEe/9EoWbyhjTXMEeRVa8X\n\tSWjhv+9bhXjn2R9lR/0zyPlzazcHcKgv2TN5ZEtNS8KRrcFGDYWsa8ReCoMPYA8JYCyo\n\tP1WSkrc2ttaLAIBxdQgi7+2WteZxXGadzboeLOObQa/YEmqFmZ+cmnyKANN4aff1KNZn\n\tRh05FRAqPlflnxwJfnTcQsmH2b9pOo/jBIR2A4Hy0Wudy9Dgoy9/iX5dU6XtTNJsGP/J\n\t/I/A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=GF3Et6kVj7+hJM8zVtvpQWDRjwVFS4rrFOpLLj7QlVY=;\n\tb=Yv7JylLX7cPKFKb7/uImQR7VjvPQeQMBtV21K9Fq3N8FhEDwbRjnu6HUC2v0CXDLWD\n\tdH81JGW6cvVGhOomxhYbI65MKdexLk5vdu97faQRgHRJCf4M+IasvKCAppiKBNsTCYJl\n\twAZWRzkpjD2n+KHMScl3oH5ciWskH5nmIjC5YybVUSmnALv2mz0rGmTpwoGOsEiCSFdL\n\tVhamGTsnJusClRuGgxVIFdg4h/mcIQ4girQ/d+9EAaKKyIl1yWCDjyuwo3BntdPodRl6\n\tEuCtnxetb2/dMkoVvYA07J3w+43EgY3jMspgQQDBztHYkWZDSo56FHr4ieuPLv8Gt3ME\n\tgibw==","X-Gm-Message-State":"AGi0PuYq/R80tr0RxnhJnNQxd8F/x0Y2KlS+F934smiBv3ByzFHYxv4K\n\tLV+4dP8bd5Ncga+xolnMeJ4Pomum240=","X-Google-Smtp-Source":"APiQypIWNIz2K3g+J0y7XRyGkh8rO6by5bHEjiH7lWJ/2XvP1jv8kLNlQoZe5T4/v7Jkwd1CLpl/SQ==","X-Received":"by 2002:a2e:9818:: with SMTP id\n\ta24mr19108401ljj.126.1588118870076; \n\tTue, 28 Apr 2020 17:07:50 -0700 (PDT)","Date":"Wed, 29 Apr 2020 02:07:48 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Andrey Konovalov ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200429000748.GD1372619@oden.dyn.berto.se>","References":"<20200427220529.1085074-1-niklas.soderlund@ragnatech.se>\n\t<20200427220529.1085074-4-niklas.soderlund@ragnatech.se>\n\t<84816f26-ce50-25c6-a447-8f3cd3044431@linaro.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<84816f26-ce50-25c6-a447-8f3cd3044431@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] cam: Add helper class to parse\n\tstream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 29 Apr 2020 00:07:51 -0000"}},{"id":4683,"web_url":"https://patchwork.libcamera.org/comment/4683/","msgid":"<20200430164028.GP5856@pendragon.ideasonboard.com>","date":"2020-04-30T16:40:28","subject":"Re: [libcamera-devel] [PATCH v2 3/5] cam: Add helper class to parse\n\tstream configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Apr 28, 2020 at 12:05:27AM +0200, Niklas Söderlund wrote:\n> Create a new helper class StreamKeyValueParser to parse command line\n> options describing stream configurations. The goal is to share this new\n> class between cam and qcam.\n\nThis looks good. I have a few comments, but they don't need to be\naddressed now.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/meson.build | 1 +\n> src/cam/stream_options.cpp | 133 +++++++++++++++++++++++++++++++++++++\n> src/cam/stream_options.h | 32 +++++++++\n> 3 files changed, 166 insertions(+)\n> create mode 100644 src/cam/stream_options.cpp\n> create mode 100644 src/cam/stream_options.h\n> \n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 2419d648bc17e02b..162d6333f94e4851 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -4,6 +4,7 @@ cam_sources = files([\n> 'event_loop.cpp',\n> 'main.cpp',\n> 'options.cpp',\n> + 'stream_options.cpp',\n> ])\n> \n> cam = executable('cam', cam_sources,\n> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp\n> new file mode 100644\n> index 0000000000000000..9f726a7e70628648\n> --- /dev/null\n> +++ b/src/cam/stream_options.cpp\n> @@ -0,0 +1,133 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * stream_options.cpp - Helper to parse options for streams\n> + */\n> +#include \"stream_options.h\"\n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +StreamKeyValueParser::StreamKeyValueParser()\n> +{\n> +\taddOption(\"role\", OptionString,\n> +\t\t \"Role for the stream (viewfinder, video, still, stillraw)\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"width\", OptionInteger, \"Width in pixels\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"height\", OptionInteger, \"Height in pixels\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> +\t\t ArgumentRequired);\n> +}\n> +\n> +KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)\n> +{\n> +\tKeyValueParser::Options options = KeyValueParser::parse(arguments);\n> +\tStreamRole role;\n> +\n> +\tif (options.valid() && options.isSet(\"role\") &&\n> +\t parseRole(&role, options)) {\n> +\t\tstd::cerr << \"Unknown stream role \"\n> +\t\t\t << options[\"role\"].toString() << std::endl;\n\nError messages should be printed with an application-specific API\n(qError() for qcam for instance), but that's for later\n\n> +\t\toptions.invalidate();\n> +\t}\n> +\n> +\treturn options;\n> +}\n> +\n> +StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> +{\n> +\tconst std::vector &streamParameters = values.toArray();\n> +\n> +\t/* If no configuration values to examine default to viewfinder. */\n> +\tif (!streamParameters.size())\n\n\tif (streamParameters.empty())\n\n> +\t\treturn { StreamRole::Viewfinder };\n\nWould it make sense to return an empty vector here ? I could imagine\nthat the default could be application-specific. Can be done later too.\n\n> +\n> +\tStreamRoles roles;\n> +\tfor (auto const &value : streamParameters) {\n> +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> +\t\tStreamRole role;\n> +\n> +\t\t/* If role is invalid or not set default to viewfinder. */\n> +\t\tif (parseRole(&role, value))\n> +\t\t\trole = StreamRole::Viewfinder;\n> +\n> +\t\troles.push_back(role);\n> +\t}\n> +\n> +\treturn roles;\n> +}\n> +\n> +int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> +\t\t\t\t\t const OptionValue &values)\n> +{\n> +\tconst std::vector &streamParameters = values.toArray();\n> +\n> +\tif (!config) {\n> +\t\tstd::cerr << \"No configuration provided\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* If no configuration values nothing to do. */\n> +\tif (!streamParameters.size())\n> +\t\treturn 0;\n> +\n> +\tif (config->size() != streamParameters.size()) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Number of streams in configuration \"\n> +\t\t\t<< config->size()\n> +\t\t\t<< \" does not match number of streams parsed \"\n> +\t\t\t<< streamParameters.size()\n> +\t\t\t<< std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tunsigned int i = 0;\n> +\tfor (auto const &value : streamParameters) {\n> +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> +\t\tStreamConfiguration &cfg = config->at(i++);\n> +\n> +\t\tif (opts.isSet(\"width\") && opts.isSet(\"height\")) {\n> +\t\t\tcfg.size.width = opts[\"width\"];\n> +\t\t\tcfg.size.height = opts[\"height\"];\n> +\t\t}\n> +\n> +\t\t/* TODO: Translate 4CC string to ID. */\n\n/TODO:/\\todo/\n\n> +\t\tif (opts.isSet(\"pixelformat\"))\n> +\t\t\tcfg.pixelFormat = PixelFormat(opts[\"pixelformat\"]);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int StreamKeyValueParser::parseRole(StreamRole *role,\n> +\t\t\t\t const KeyValueParser::Options &options)\n\nShould this return bool (true if parsing succeeds, false otherwise) ?\n\n> +{\n> +\tbool found = false;\n> +\n> +\tif (options.isSet(\"role\")) {\n> +\t\tstd::string name = options[\"role\"].toString();\n> +\n> +\t\tif (name == \"viewfinder\") {\n> +\t\t\t*role = StreamRole::Viewfinder;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t\tif (name == \"video\") {\n> +\t\t\t*role = StreamRole::VideoRecording;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t\tif (name == \"still\") {\n> +\t\t\t*role = StreamRole::StillCapture;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t\tif (name == \"stillraw\") {\n> +\t\t\t*role = StreamRole::StillCaptureRaw;\n> +\t\t\tfound = true;\n> +\t\t}\n> +\t}\n> +\n> +\treturn found ? 0 : -EINVAL;\n\nYou could simplify this.\n\n\tif (!options.isSet(\"role\"))\n\t\treturn -EINVAL;\n\n\tstd::string name = options[\"role\"].toString();\n\n\tif (name == \"viewfinder\") {\n\t\t*role = StreamRole::Viewfinder;\n\t\treturn 0;\n\t} else if (name == \"video\") {\n\t\t*role = StreamRole::VideoRecording;\n\t\treturn 0;\n\t} else if (name == \"still\") {\n\t\t*role = StreamRole::StillCapture;\n\t\treturn 0;\n\t} else if (name == \"stillraw\") {\n\t\t*role = StreamRole::StillCaptureRaw;\n\t\treturn 0;\n\t} else {\n\t\treturn -EINVAL;\n\t}\n\nPlease consider the changes you think are appropriate, and apply\n\nReviewed-by: Laurent Pinchart \n\n> +}\n> diff --git a/src/cam/stream_options.h b/src/cam/stream_options.h\n> new file mode 100644\n> index 0000000000000000..c90d8c4dc595814d\n> --- /dev/null\n> +++ b/src/cam/stream_options.h\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * stream_options.h - Helper to parse options for streams\n> + */\n> +#ifndef __CAM_STREAM_OPTIONS_H__\n> +#define __CAM_STREAM_OPTIONS_H__\n> +\n> +#include \n> +\n> +#include \"options.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class StreamKeyValueParser : public KeyValueParser\n> +{\n> +public:\n> +\tStreamKeyValueParser();\n> +\n> +\tKeyValueParser::Options parse(const char *arguments) override;\n> +\n> +\tstatic StreamRoles roles(const OptionValue &values);\n> +\tstatic int updateConfiguration(CameraConfiguration *config,\n> +\t\t\t\t const OptionValue &values);\n> +\n> +private:\n> +\tstatic int parseRole(StreamRole *role,\n> +\t\t\t const KeyValueParser::Options &options);\n> +};\n> +\n> +#endif /* __CAM_STREAM_OPTIONS_H__ */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DC3C613A8\n\tfor ;\n\tThu, 30 Apr 2020 18:40:30 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ACC59321;\n\tThu, 30 Apr 2020 18:40:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wWbOSXTF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588264829;\n\tbh=yUQ70aEe8aImdlgus2H1HKFSSU7JR54P9Ou7KY5zfP0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wWbOSXTFnS5yCXWsYMz4GgX9lEDhCz+lz/Ws/+bgs/mtlPNoaEOjdGmSjxbViqbDS\n\t/goifB0M6bEu1rPwYo1tttqEi9IiI/6JqiFQ68SiJDksyXIVDV9CqFs4EEjeFrummm\n\tr8n+e/frOsi6QGqA0zKjRT2Td37/SAJ6JuwUhhSo=","Date":"Thu, 30 Apr 2020 19:40:28 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200430164028.GP5856@pendragon.ideasonboard.com>","References":"<20200427220529.1085074-1-niklas.soderlund@ragnatech.se>\n\t<20200427220529.1085074-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200427220529.1085074-4-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] cam: Add helper class to parse\n\tstream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 30 Apr 2020 16:40:30 -0000"}}]