[libcamera-devel] [PATCH 09/30] cam: options: Support parent-child relationship between options
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 12 22:03:00 CEST 2021
Hi Kieran,
On Mon, Jul 12, 2021 at 03:12:28PM +0100, Kieran Bingham wrote:
> On 07/07/2021 03:19, Laurent Pinchart wrote:
> > Add support for creating a tree-based hiearchy of options instead of a
>
> s/hiearchy/hierarchy/
>
> > flat list. This is useful to support options that need to be interpreted
> > in the context of a particular occurrence of an array option.
>
> How is this expressed in the help? (i.e.l how does the user know which
> ones establish a new parent set of options).
>
> An example output up here would be useful if it changed.
I'll add this:
The usage text automatically documents the options in their
corresponding context:
Options:
-c, --camera camera ... Specify which camera to operate on, by id or by index
-h, --help Display this help message
-I, --info Display information about stream(s)
-l, --list List all cameras
--list-controls List cameras controls
-p, --list-properties List cameras properties
-m, --monitor Monitor for hotplug and unplug camera events
Options valid in the context of --camera:
-C, --capture[=count] Capture until interrupted by user or until <count> frames captured
-F, --file[=filename] Write captured frames to disk
If the file name ends with a '/', it sets the directory in which
to write files, using the default file name. Otherwise it sets the
full file path and name. The first '#' character in the file name
is expanded to the camera index, stream name and frame sequence number.
The default file name is 'frame-#.bin'.
-s, --stream key=value[,key=value,...] ... Set configuration of a camera stream
height=integer Height in pixels
pixelformat=string Pixel format name
role=string Role for the stream (viewfinder, video, still, raw)
width=integer Width in pixels
--strict-formats Do not allow requested stream format(s) to be adjusted
--metadata Print the metadata for completed requests
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/cam/options.cpp | 217 ++++++++++++++++++++++++++++++++++++++------
> > src/cam/options.h | 14 ++-
> > 2 files changed, 199 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> > index 4a88c38fb154..d7f9f741c731 100644
> > --- a/src/cam/options.cpp
> > +++ b/src/cam/options.cpp
> > @@ -79,6 +79,12 @@
> > * \var Option::isArray
> > * \brief Whether the option can appear once or multiple times
> > *
> > + * \var Option::parent
> > + * \brief The parent option
> > + *
> > + * \var Option::children
> > + * \brief List of child options, storing all options whose parent is this option
> > + *
> > * \fn Option::hasShortOption()
> > * \brief Tell if the option has a short option specifier (e.g. `-f`)
> > * \return True if the option has a short option specifier, false otherwise
> > @@ -96,6 +102,8 @@ struct Option {
> > const char *help;
> > KeyValueParser *keyValueParser;
> > bool isArray;
> > + Option *parent;
> > + std::list<Option> children;
> >
> > bool hasShortOption() const { return isalnum(opt); }
> > bool hasLongOption() const { return name != nullptr; }
> > @@ -336,7 +344,7 @@ bool KeyValueParser::addOption(const char *name, OptionType type,
> > return false;
> >
> > optionsMap_[name] = Option({ 0, type, name, argument, nullptr,
> > - help, nullptr, false });
> > + help, nullptr, false, nullptr, {} });
> > return true;
> > }
> >
> > @@ -473,6 +481,11 @@ void KeyValueParser::usage(int indent)
> > * option. It supports empty values, integers, strings, key-value lists, as well
> > * as arrays of those types. For array values, all array elements shall have the
> > * same type.
> > + *
> > + * OptionValue instances are organized in a tree-based structure that matches
> > + * the parent-child relationship of the options added to the parser. Children
> > + * are retrieve with the children() function, and are stored as an
>
> s/retrieve/retrieved/
>
> > + * OptionsBase<int>.
> > */
> >
> > /**
> > @@ -663,6 +676,15 @@ std::vector<OptionValue> OptionValue::toArray() const
> > return array_;
> > }
> >
> > +/**
> > + * \brief Retrieve the list of child values
> > + * \return The list of child values
> > + */
> > +const OptionsParser::Options &OptionValue::children() const
> > +{
> > + return children_;
> > +}
> > +
> > /* -----------------------------------------------------------------------------
> > * OptionsParser
> > */
> > @@ -725,6 +747,32 @@ std::vector<OptionValue> OptionValue::toArray() const
> > * accept an argument, the option value can be access by Options::operator[]()
> > * using the option identifier as the key. The order in which different options
> > * are specified on the command line isn't preserved.
> > + *
> > + * Options can be created with parent-child relationships to organize them as a
> > + * tree instead of a flat list. When parsing a command line, the child options
> > + * are considered related to the parent option that precedes them. This is
> > + * useful when the parent is an array option. The Options values list generated
> > + * by the parser then turns into a tree, which each parent value storing the
> > + * values of child options that follow that instance of the parent option.
> > + * For instance, with a `capture` option specified as a child of a `camera`
> > + * array option, parsing the command line
> > + *
> > + * `--camera 1 --capture=10 --camera 2 --capture=20`
> > + *
> > + * will return an Options instance containing a single OptionValue instance of
> > + * array type, for the `camera` option. The OptionValue will contain two
> > + * entries, with the first entry containing the integer value 1 and the second
> > + * entry the integer value 2. Each of those entries will in turn store an
> > + * Options instance that contains the respective children. The first entry will
> > + * store in its children a `capture` option of value 10, and the second entry a
> > + * `capture` option of value 20.
> > + *
> > + * The command line
> > + *
> > + * `--capture=10 --camera 1`
> > + *
> > + * would result in a parsing error, as the `capture` option has no preceding
> > + * `camera` option on the command line.
> > */
> >
> > /**
> > @@ -748,13 +796,14 @@ OptionsParser::~OptionsParser() = default;
> > * mandatory argument, or no argument at all
> > * \param[in] argumentName The argument name used in the help text
> > * \param[in] array Whether the option can appear once or multiple times
> > + * \param[in] parent The identifier of the parent option (optional)
> > *
> > * \return True if the option was added successfully, false if an error
> > * occurred.
> > */
> > bool OptionsParser::addOption(int opt, OptionType type, const char *help,
> > const char *name, OptionArgument argument,
> > - const char *argumentName, bool array)
> > + const char *argumentName, bool array, int parent)
> > {
> > /*
> > * Options must have at least a short or long name, and a text message.
> > @@ -771,9 +820,31 @@ bool OptionsParser::addOption(int opt, OptionType type, const char *help,
> > if (optionsMap_.find(opt) != optionsMap_.end())
> > return false;
> >
> > - options_.push_back(Option({ opt, type, name, argument, argumentName,
> > - help, nullptr, array }));
> > - optionsMap_[opt] = &options_.back();
> > + /*
> > + * If a parent is specified, create the option as a child of its parent.
> > + * Otherwise, create it in the parser's options list.
> > + */
> > + Option *option;
> > +
> > + if (parent) {
> > + auto iter = optionsMap_.find(parent);
> > + if (iter == optionsMap_.end())
> > + return false;
> > +
> > + Option *parentOpt = iter->second;
> > + parentOpt->children.push_back({
> > + opt, type, name, argument, argumentName, help, nullptr,
> > + array, parentOpt, {}
> > + });
> > + option = &parentOpt->children.back();
> > + } else {
> > + options_.push_back({ opt, type, name, argument, argumentName,
> > + help, nullptr, array, nullptr, {} });
> > + option = &options_.back();
> > + }
> > +
> > + optionsMap_[opt] = option;
> > +
> > return true;
> > }
> >
> > @@ -791,13 +862,13 @@ bool OptionsParser::addOption(int opt, OptionType type, const char *help,
> > * occurred.
> > */
> > bool OptionsParser::addOption(int opt, KeyValueParser *parser, const char *help,
> > - const char *name, bool array)
> > + const char *name, bool array, int parent)
> > {
> > if (!addOption(opt, OptionKeyValue, help, name, ArgumentRequired,
> > - "key=value[,key=value,...]", array))
> > + "key=value[,key=value,...]", array, parent))
> > return false;
> >
> > - options_.back().keyValueParser = parser;
> > + optionsMap_[opt]->keyValueParser = parser;
> > return true;
> > }
> >
> > @@ -822,26 +893,26 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> > * Allocate short and long options arrays large enough to contain all
> > * options.
> > */
> > - char shortOptions[options_.size() * 3 + 2];
> > - struct option longOptions[options_.size() + 1];
> > + char shortOptions[optionsMap_.size() * 3 + 2];
> > + struct option longOptions[optionsMap_.size() + 1];
> > unsigned int ids = 0;
> > unsigned int idl = 0;
> >
> > shortOptions[ids++] = ':';
> >
> > - for (const Option &option : options_) {
> > - if (option.hasShortOption()) {
> > - shortOptions[ids++] = option.opt;
> > - if (option.argument != ArgumentNone)
> > + for (const auto [opt, option] : optionsMap_) {
> > + if (option->hasShortOption()) {
> > + shortOptions[ids++] = opt;
> > + if (option->argument != ArgumentNone)
> > shortOptions[ids++] = ':';
> > - if (option.argument == ArgumentOptional)
> > + if (option->argument == ArgumentOptional)
> > shortOptions[ids++] = ':';
> > }
> >
> > - if (option.hasLongOption()) {
> > - longOptions[idl].name = option.name;
> > + if (option->hasLongOption()) {
> > + longOptions[idl].name = option->name;
> >
> > - switch (option.argument) {
> > + switch (option->argument) {
> > case ArgumentNone:
> > longOptions[idl].has_arg = no_argument;
> > break;
> > @@ -854,7 +925,7 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> > }
> >
> > longOptions[idl].flag = 0;
> > - longOptions[idl].val = option.opt;
> > + longOptions[idl].val = option->opt;
> > idl++;
> > }
> > }
> > @@ -882,10 +953,7 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> > }
> >
> > const Option &option = *optionsMap_[c];
> > - if (!options.parseValue(c, option, optarg)) {
> > - std::cerr << "Can't parse " << option.typeName()
> > - << " argument for option " << option.optionName()
> > - << std::endl;
> > + if (!parseValue(option, optarg, &options)) {
> > usage();
> > return options;
> > }
> > @@ -907,15 +975,16 @@ void OptionsParser::usage()
> > {
> > unsigned int indent = 0;
> >
> > - for (const Option &option : options_) {
> > + for (const auto &opt : optionsMap_) {
> > + const Option *option = opt.second;
> > unsigned int length = 14;
> > - if (option.hasLongOption())
> > - length += 2 + strlen(option.name);
> > - if (option.argument != ArgumentNone)
> > - length += 1 + strlen(option.argumentName);
> > - if (option.argument == ArgumentOptional)
> > + if (option->hasLongOption())
> > + length += 2 + strlen(option->name);
> > + if (option->argument != ArgumentNone)
> > + length += 1 + strlen(option->argumentName);
> > + if (option->argument == ArgumentOptional)
> > length += 2;
> > - if (option.isArray)
> > + if (option->isArray)
> > length += 4;
> >
> > if (length > indent)
> > @@ -938,6 +1007,8 @@ void OptionsParser::usage()
> > void OptionsParser::usageOptions(const std::list<Option> &options,
> > unsigned int indent)
> > {
> > + std::vector<const Option *> parentOptions;
> > +
> > for (const Option &option : options) {
> > std::string argument;
> > if (option.hasShortOption())
> > @@ -982,5 +1053,91 @@ void OptionsParser::usageOptions(const std::list<Option> &options,
> >
> > if (option.keyValueParser)
> > option.keyValueParser->usage(indent);
> > +
> > + if (!option.children.empty())
> > + parentOptions.push_back(&option);
> > }
> > +
> > + if (parentOptions.empty())
> > + return;
> > +
> > + for (const Option *option : parentOptions) {
> > + std::cerr << std::endl << "Options valid in the context of "
> > + << option->optionName() << ":" << std::endl;
> > + usageOptions(option->children, indent);
> > + }
> > +}
> > +
> > +std::tuple<OptionsParser::Options *, const Option *>
> > +OptionsParser::childOption(const Option *parent, Options *options)
> > +{
> > + /*
> > + * The parent argument points to the parent of the leaf node Option,
> > + * and the options argument to the root node of the Options tree. Use
> > + * recursive calls traverse the Option tree up to the root node while
>
> Use recursive calls "to" traverse the Option tree ?
>
> I.e. is the to missing?
Absolutely.
> > + * traversing the Options tree down to the leaf node:
>
> To traverse up while traversing down ... sounds ... odd ?
One tree it traversed up from the leaf node to the root, while the other
one is traversed down from the root to the leaf node. I've struggled
with this comment when writing it :-S It's a non-trivial function.
> > + */
> > +
> > + /*
> > + * - If we have no parent, we're reached the root node of the Option
>
> s/we're/we've/
>
> > + * tree, the options argument is what we need.
> > + */
> > + if (!parent)
> > + return { options, nullptr };
> > +
> > + /*
> > + * - If the parent has a parent, use recursion to move one level up the
> > + * Option tree. This returns the Options corresponding to parent, or
> > + * nullptr if a suitable Options child isn't found.
> > + */
> > + if (parent->parent) {
> > + const Option *error;
> > + std::tie(options, error) = childOption(parent->parent, options);
> > +
> > + /* Propagate the error all the way up. */
>
> propagate up or down ? The previous comment says we're moving up a
> level, so do we then propagate the error down? Or perhaps do you mean
> 'back up the callstack' instead...
>
>
> Otherwise we have 'recurse up', and 'propagate up' which I think are
> both different 'directions'...
I meant up the call stack, yes. I'll write it out explicitly.
/* Propagate the error all the way back up the call stack. */
> > + if (!error)
> > + return { options, error };
> > + }
> > +
> > + /*
> > + * - The parent has no parent, we're now one level down the root.
> > + * Return the Options child corresponding to the parent. The child may
> > + * not exists if options are specified in an incorrect order.
>
> s/exists/exist/
>
> Phew, ok - well this patch is terse, but I can't see anything
> specifically other than those minors so:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > + */
> > + if (!options->isSet(parent->opt))
> > + return { nullptr, parent };
> > +
> > + /*
> > + * If the child value is of array type, children are not stored in the
> > + * value .children() list, but in the .children() of the value's array
> > + * elements. Use the last array element in that case, as a child option
> > + * relates to the last instance of its parent option.
> > + */
> > + const OptionValue *value = &(*options)[parent->opt];
> > + if (value->type() == OptionValue::ValueArray)
> > + value = &value->toArray().back();
> > +
> > + return { const_cast<Options *>(&value->children()), nullptr };
> > +}
> > +
> > +bool OptionsParser::parseValue(const Option &option, const char *arg,
> > + Options *options)
> > +{
> > + const Option *error;
> > +
> > + std::tie(options, error) = childOption(option.parent, options);
> > + if (error) {
> > + std::cerr << "Option " << option.optionName() << " requires a "
> > + << error->optionName() << " context" << std::endl;
> > + return false;
> > + }
> > +
> > + if (!options->parseValue(option.opt, option, arg)) {
> > + std::cerr << "Can't parse " << option.typeName()
> > + << " argument for option " << option.optionName()
> > + << std::endl;
> > + return false;
> > + }
> > +
> > + return true;
> > }
> > diff --git a/src/cam/options.h b/src/cam/options.h
> > index 5c51a94c2f37..e894822c0061 100644
> > --- a/src/cam/options.h
> > +++ b/src/cam/options.h
> > @@ -10,6 +10,7 @@
> > #include <ctype.h>
> > #include <list>
> > #include <map>
> > +#include <tuple>
> > #include <vector>
> >
> > class KeyValueParser;
> > @@ -91,9 +92,11 @@ public:
> > bool addOption(int opt, OptionType type, const char *help,
> > const char *name = nullptr,
> > OptionArgument argument = ArgumentNone,
> > - const char *argumentName = nullptr, bool array = false);
> > + const char *argumentName = nullptr, bool array = false,
> > + int parent = 0);
> > bool addOption(int opt, KeyValueParser *parser, const char *help,
> > - const char *name = nullptr, bool array = false);
> > + const char *name = nullptr, bool array = false,
> > + int parent = 0);
> >
> > Options parse(int argc, char *argv[]);
> > void usage();
> > @@ -104,6 +107,10 @@ private:
> >
> > void usageOptions(const std::list<Option> &options, unsigned int indent);
> >
> > + std::tuple<OptionsParser::Options *, const Option *>
> > + childOption(const Option *parent, Options *options);
> > + bool parseValue(const Option &option, const char *arg, Options *options);
> > +
> > std::list<Option> options_;
> > std::map<unsigned int, Option *> optionsMap_;
> > };
> > @@ -139,12 +146,15 @@ public:
> > KeyValueParser::Options toKeyValues() const;
> > std::vector<OptionValue> toArray() const;
> >
> > + const OptionsParser::Options &children() const;
> > +
> > private:
> > ValueType type_;
> > int integer_;
> > std::string string_;
> > KeyValueParser::Options keyValues_;
> > std::vector<OptionValue> array_;
> > + OptionsParser::Options children_;
> > };
> >
> > #endif /* __CAM_OPTIONS_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list