Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/2] Rework option handling

[Christophe Fergeau]

On Thu, Nov 16, 2017 at 10:17:28AM -0500, Frediano Ziglio wrote:
> > 4. It delegates usage information to the plugins, so that each plugin
> >can document its specific options in a consistent way.
> > 
> > 5. It cleans up some confusing or surprising aspects of the interace,
> >notably null-terminated option vectors and the ownership of
> >dynamically loaded plugins.
> > 
> > Signed-off-by: Christophe de Dinechin 
> 
> Maybe you should split the patch instead of having multiple points
> inside the comment? I would personally starts with some commit that
> can be merged without waiting, like adding a Name() method.

Yes, that was my reaction when reading through it, this is trying to do far too
much in one commit.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/2] Rework option handling

[Christophe de Dinechin]

Frediano Ziglio writes:

>>
>> From: Christophe de Dinechin 
>>
>> The overall objective is to pave the way for features that will likely
>> become important, notably the ability to dynamically adjust quality of
>> service to respond to network conditions or client load issues.
>>
>> Consequently, this patch cleans up the option-related aspects of the
>> plugin ABI and API as follows:
>>
>> 1. It adds standard settings that are shared across plugins, e.g. framerate.
>>This will enable future QoS adjustments to be applied by the agent
>>in a consistent way, irrespective of the actual capture plugin.
>>
>> 2. It makes the interface for option handling dynamic, so that the agent
>>can in the future update settings without having to do expensive
>>operations such as searching for pre-existing options.
>>
>> 3. It exposes a consistent way to deal witih settings values, so that
>
> typo
>
>>error messages and acceptable values are consistent across plugins.
>>The current interfaces only deals with integer values, possibly with
>>a range of acceptable values, and possibly with a K/M suffix, e.g.
>>to accept max_bitrate=100k.
>>
>
> Was thinking about this unit thing. Maybe we can use an enum making possible
> to add additional units in the future?

Well, if we want arbitrary units, we could pass an array of acceptable
units and corresponding multipliers. It can't just be an enum.

One of the things I realized after sending the patch is that we should
be able to write 8Mb and 8MB and have megabits and megabytes
respectively, since that's what the units normally means.

>
>> 4. It delegates usage information to the plugins, so that each plugin
>>can document its specific options in a consistent way.
>>
>> 5. It cleans up some confusing or surprising aspects of the interace,
>>notably null-terminated option vectors and the ownership of
>>dynamically loaded plugins.
>>
>> Signed-off-by: Christophe de Dinechin 
>
> Maybe you should split the patch instead of having multiple points
> inside the comment? I would personally starts with some commit that
> can be merged without waiting, like adding a Name() method.

The Name() method by itself introduces an ABI incompatibility. For now,
I have not really understood your objections about the first patch
in the series, so I am awaiting for your response there, and then
I can decide how to split.

My preference is clearly to mark a strong ABI incompatiblity for now,
by calling the ABI 0.x, and add the rule that all 0.x versions are
assumed to be incompatible with one another. If we do that, then
it is no problem to bump the minor number for something like adding
Name().

If I have to bump the major number just because I add Name(), and
we end up in a month with ABI version 42.0, that's OK too, but I don't
like it.

>
>> ---
>>  include/spice-streaming-agent/plugin.hpp |  94 +---
>>  m4/spice-compile-warnings.m4 |   2 +
>>  src/concrete-agent.cpp   | 143
>>  ---
>>  src/concrete-agent.hpp   |  34 
>>  src/mjpeg-fallback.cpp   |  89 +--
>>  src/spice-streaming-agent.cpp|  23 +++--
>>  6 files changed, 284 insertions(+), 101 deletions(-)
>>
>> diff --git a/include/spice-streaming-agent/plugin.hpp
>> b/include/spice-streaming-agent/plugin.hpp
>> index 607fabf..6202d2c 100644
>> --- a/include/spice-streaming-agent/plugin.hpp
>> +++ b/include/spice-streaming-agent/plugin.hpp
>> @@ -8,6 +8,10 @@
>>  #define SPICE_STREAMING_AGENT_PLUGIN_HPP
>>
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>>
>>  /*!
>>   * \file
>> @@ -20,6 +24,7 @@
>>  namespace SpiceStreamingAgent {
>>
>>  class FrameCapture;
>> +class Agent;
>>
>>  /*!
>>   * Plugin version, only using few bits, schema is 0xMMmm
>> @@ -42,31 +47,75 @@ enum Ranks : unsigned {
>>  };
>>
>>  /*!
>> - * Configuration option.
>> - * An array of these will be passed to the plugin.
>> - * Simply a couple name and value passed as string.
>> - * For instance "framerate" and "20".
>> + * Settings that are common to all plugins
>>   */
>> -struct ConfigureOption
>> +struct Settings
>>  {
>> +unsignedframerate   =  30; // Frames per second (1-240)
>> +unsignedquality =  80; // Normalized in 0-100 (100=high)
>> +unsignedavg_bitrate = 300; // Target average bitrate in bps
>> +unsignedmax_bitrate = 800; // Target maximum bitrate
>> +};
>> +
>> +/*!
>> + * Command line option
>> + */
>> +struct Option
>> +{
>> +Option(const char *name, const char *value): name(name), value(value) {}
>>  const char *name;
>>  const char *value;
>>  };
>>
>>  /*!
>> + * Error applying an option
>> + */
>> +class OptionError : public std::runtime_error
>> +{
>> +public:
>> +OptionError(const std::string ): std::runtime_error(what) {}
>> 

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/2] Rework option handling

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> The overall objective is to pave the way for features that will likely
> become important, notably the ability to dynamically adjust quality of
> service to respond to network conditions or client load issues.
> 
> Consequently, this patch cleans up the option-related aspects of the
> plugin ABI and API as follows:
> 
> 1. It adds standard settings that are shared across plugins, e.g. framerate.
>This will enable future QoS adjustments to be applied by the agent
>in a consistent way, irrespective of the actual capture plugin.
> 
> 2. It makes the interface for option handling dynamic, so that the agent
>can in the future update settings without having to do expensive
>operations such as searching for pre-existing options.
> 
> 3. It exposes a consistent way to deal witih settings values, so that

typo

>error messages and acceptable values are consistent across plugins.
>The current interfaces only deals with integer values, possibly with
>a range of acceptable values, and possibly with a K/M suffix, e.g.
>to accept max_bitrate=100k.
> 

Was thinking about this unit thing. Maybe we can use an enum making possible
to add additional units in the future?

> 4. It delegates usage information to the plugins, so that each plugin
>can document its specific options in a consistent way.
> 
> 5. It cleans up some confusing or surprising aspects of the interace,
>notably null-terminated option vectors and the ownership of
>dynamically loaded plugins.
> 
> Signed-off-by: Christophe de Dinechin 

Maybe you should split the patch instead of having multiple points
inside the comment? I would personally starts with some commit that
can be merged without waiting, like adding a Name() method.

> ---
>  include/spice-streaming-agent/plugin.hpp |  94 +---
>  m4/spice-compile-warnings.m4 |   2 +
>  src/concrete-agent.cpp   | 143
>  ---
>  src/concrete-agent.hpp   |  34 
>  src/mjpeg-fallback.cpp   |  89 +--
>  src/spice-streaming-agent.cpp|  23 +++--
>  6 files changed, 284 insertions(+), 101 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp
> b/include/spice-streaming-agent/plugin.hpp
> index 607fabf..6202d2c 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -8,6 +8,10 @@
>  #define SPICE_STREAMING_AGENT_PLUGIN_HPP
>  
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  /*!
>   * \file
> @@ -20,6 +24,7 @@
>  namespace SpiceStreamingAgent {
>  
>  class FrameCapture;
> +class Agent;
>  
>  /*!
>   * Plugin version, only using few bits, schema is 0xMMmm
> @@ -42,31 +47,75 @@ enum Ranks : unsigned {
>  };
>  
>  /*!
> - * Configuration option.
> - * An array of these will be passed to the plugin.
> - * Simply a couple name and value passed as string.
> - * For instance "framerate" and "20".
> + * Settings that are common to all plugins
>   */
> -struct ConfigureOption
> +struct Settings
>  {
> +unsignedframerate   =  30; // Frames per second (1-240)
> +unsignedquality =  80; // Normalized in 0-100 (100=high)
> +unsignedavg_bitrate = 300; // Target average bitrate in bps
> +unsignedmax_bitrate = 800; // Target maximum bitrate
> +};
> +
> +/*!
> + * Command line option
> + */
> +struct Option
> +{
> +Option(const char *name, const char *value): name(name), value(value) {}
>  const char *name;
>  const char *value;
>  };
>  
>  /*!
> + * Error applying an option
> + */
> +class OptionError : public std::runtime_error
> +{
> +public:
> +OptionError(const std::string ): std::runtime_error(what) {}
> +};
> +
> +/*!
> + * Usage information
> + */
> +struct UsageInfo
> +{
> +const char *name;
> +const char *range;
> +const char *description;
> +};
> +
> +/*!
>   * Interface a plugin should implement and register to the Agent.
>   *
>   * A plugin module can register multiple Plugin interfaces to handle
>   * multiple codecs. In this case each Plugin will report data for a
>   * specific codec.
> + *
> + * A plugin will typically extend the Settings struct to add its own
> + * settings.
>   */
>  class Plugin
>  {
>  public:
> +Plugin(Agent *agent): agent(agent) {}
> +
>  /*!
>   * Allows to free the plugin when not needed
>   */
> -virtual ~Plugin() {};
> +virtual ~Plugin() { }
> +
> +/*!
> + * Return the name of the plugin, e.g. for command-line usage
> information
> + */
> +virtual const char *Name() = 0;
> +
> +/*!
> + * Return usage information for the plug-in, e.g. command-line options
> + * Usage is returned as a NULL-terminated array of UsageInfo
> + */
> +virtual const UsageInfo *Usage() = 0;
>  
>  /*!
>