>
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>
> 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 <dinechin@xxxxxxxxxx>
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 <spice/enums.h>
> +#include <stdexcept>
> +#include <vector>
> +#include <climits>
> +#include <cstdio>
>
> /*!
> * \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
> {
> + unsigned framerate = 30; // Frames per second (1-240)
> + unsigned quality = 80; // Normalized in 0-100 (100=high)
> + unsigned avg_bitrate = 3000000; // Target average bitrate in bps
> + unsigned max_bitrate = 8000000; // 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 &what): 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;
>
> /*!
> * Request an object for getting frames.
> @@ -89,6 +138,22 @@ public:
> * Get video codec used to encode last frame
> */
> virtual SpiceVideoCodecType VideoCodecType() const=0;
> +
> + /*!
> + * Return the concrete Settings for this plugin
> + */
> + virtual Settings & PluginSettings() = 0;
> +
> + /*!
> + * Apply a given option to the plugin settings.
> + * \return true if the option is valid, false if it is not recognized
> + * If there is an error applying the settings, set \arg error.
> + * If this returns false, agent will process standard options itself.
> + */
> + virtual bool ApplyOption(const Option &o) throw(OptionError) = 0;
> +
> +protected:
> + Agent *agent;
> };
>
> /*!
> @@ -113,20 +178,23 @@ public:
> * Check if a given plugin version is compatible with this agent
> * \return true is compatible
> */
> - virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const=0;
> + virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const =
> 0;
>
> /*!
> * Register a plugin in the system.
> + * The agent then has shared ownership of the plugin, meaning that
> + * the plugin will be deleted when the agent is deleted unless another
> + * shared_ptr instance still holds it at that time.
> */
> - virtual void Register(Plugin& plugin)=0;
> + virtual void Register(Plugin *plugin)=0;
>
> /*!
> - * Get options array.
> - * Array is terminated with {nullptr, nullptr}.
> - * Never nullptr.
> - * \todo passing options to entry point instead?
> + * Helper to get integer values from command-line options
> */
> - virtual const ConfigureOption* Options() const=0;
> + virtual int IntOption(const Option &opt,
> + int min = INT_MIN, int max = INT_MAX,
> + bool sizeSuffixOK = false) const
> + throw(OptionError) = 0;
> };
>
> typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
> diff --git a/m4/spice-compile-warnings.m4 b/m4/spice-compile-warnings.m4
> index 66d7179..9e8bb6e 100644
> --- a/m4/spice-compile-warnings.m4
> +++ b/m4/spice-compile-warnings.m4
> @@ -86,6 +86,8 @@ AC_DEFUN([SPICE_COMPILE_WARNINGS],[
> dontwarn="$dontwarn -Wpointer-sign"
> dontwarn="$dontwarn -Wpointer-to-int-cast"
> dontwarn="$dontwarn -Wstrict-prototypes"
> + dontwarn="$dontwarn -Wsuggest-final-types"
> + dontwarn="$dontwarn -Wsuggest-final-methods"
>
> # We want to enable these, but need to sort out the
> # decl mess with gtk/generated_*.c
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 51b4504..5c93e42 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -9,6 +9,10 @@
> #include <syslog.h>
> #include <glob.h>
> #include <dlfcn.h>
> +#include <mutex>
> +#include <climits>
> +#include <sstream>
> +#include <string>
>
> #include "concrete-agent.hpp"
> #include "static-plugin.hpp"
> @@ -28,7 +32,11 @@ static inline unsigned MinorVersion(unsigned version)
>
> ConcreteAgent::ConcreteAgent()
> {
> - options.push_back(ConcreteConfigureOption(nullptr, nullptr));
> +}
> +
> +void ConcreteAgent::Register(Plugin *plugin)
> +{
> + plugins.push_back(shared_ptr<Plugin>(plugin));
> }
>
> bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
> @@ -41,22 +49,83 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned
> pluginVersion) const
> MinorVersion(version) >= MinorVersion(pluginVersion);
> }
>
> -void ConcreteAgent::Register(Plugin& plugin)
> +int ConcreteAgent::IntOption(const Option &opt,
> + int min, int max, bool sizeSuffixOK) const
> + throw(OptionError)
> {
> - plugins.push_back(shared_ptr<Plugin>(&plugin));
> + std::string name = opt.name;
> + std::string value = opt.value;
> + std::istringstream input(value);
> + std::ostringstream err;
> +
> + int result = 0;
> + input >> result;
> +
> + if (!input.fail() && !input.eof() && sizeSuffixOK) {
> + std::string suffix;
> + input >> suffix;
> + bool ok = false;
> + if (!input.fail() && suffix.length() == 1) {
> + switch (suffix[0]) {
> + case 'b': case 'B': ok = true; break;
> + case 'k': case 'K': ok = true; result *= 1000; break;
> + case 'm': case 'M': ok = true; result *= 1000000; break;
> + default: break;
> + }
> + }
> + if (!ok) {
> + err << "Unknown number suffix " << suffix
> + << " for " << name << "\n";
> + throw OptionError(err.str());
> + }
> + }
> +
> + if (input.fail()) {
> + err << "Value " << value << " for " << name
> + << " is not a number\n";
> + throw OptionError(err.str());
> + }
> + if (!input.eof()) {
> + err << "Value " << value << " for " << name
> + << " does not end like an integer\n";
> + throw OptionError(err.str());
> + }
> + if (result < min || result > max) {
> + err << "The value " << value << " for " << name
> + << " is out of range (must be in " << min << ".." << max <<
> ")\n";
> + throw OptionError(err.str());
> + }
> +
> + return result;
> }
>
> -const ConfigureOption* ConcreteAgent::Options() const
> +bool ConcreteAgent::StandardOption(Settings &settings, const Option &option)
> + throw (OptionError)
> {
> - static_assert(sizeof(ConcreteConfigureOption) ==
> sizeof(ConfigureOption),
> - "ConcreteConfigureOption should be binary compatible with
> ConfigureOption");
> - return static_cast<const ConfigureOption*>(&options[0]);
> + string name = option.name;
> + if (name == "framerate" || name == "fps") {
> + settings.framerate = IntOption(option, 1, 240);
> + return true;
> + }
> + if (name == "quality") {
> + settings.quality = IntOption(option, 0, 100);
> + return true;
> + }
> + if (name == "avg_bitrate" || name == "bitrate") {
> + settings.avg_bitrate = IntOption(option, 10000, 32000000, true);
> + return true;
> + }
> + if (name == "max_bitrate") {
> + settings.max_bitrate = IntOption(option, 10000, 32000000, true);
> + return true;
> + }
> +
> + return false; // Unrecognized option
> }
>
> void ConcreteAgent::AddOption(const char *name, const char *value)
> {
> - // insert before the last {nullptr, nullptr} value
> - options.insert(--options.end(), ConcreteConfigureOption(name, value));
> + options.push_back(Option(name, value));
> }
>
> void ConcreteAgent::LoadPlugins(const char *directory)
> @@ -84,7 +153,8 @@ void ConcreteAgent::LoadPlugin(const char
> *plugin_filename)
> {
> void *dl = dlopen(plugin_filename, RTLD_LOCAL|RTLD_NOW);
> if (!dl) {
> - syslog(LOG_ERR, "error loading plugin %s", plugin_filename);
> + syslog(LOG_ERR, "error loading plugin %s: %s",
> + plugin_filename, dlerror());
> return;
> }
>
> @@ -106,7 +176,7 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
> vector<pair<unsigned, shared_ptr<Plugin>>> sorted_plugins;
>
> // sort plugins base on ranking, reverse order
> - for (const auto& plugin: plugins) {
> + for (auto& plugin: plugins) {
> sorted_plugins.push_back(make_pair(plugin->Rank(), plugin));
> }
> sort(sorted_plugins.rbegin(), sorted_plugins.rend());
> @@ -116,6 +186,7 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
> if (plugin.first == DontUse) {
> break;
> }
> + ApplyOptions(plugin.second.get());
> FrameCapture *capture = plugin.second->CreateCapture();
> if (capture) {
> return capture;
> @@ -123,3 +194,53 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
> }
> return nullptr;
> }
> +
> +void ConcreteAgent::PluginsUsage()
> +{
> + static const UsageInfo standardSettingsInfo[] =
> + {
> + { "framerate", "[1-240]", "Number of frames per second" },
> + { "quality", "[1-100]", "Normalized quality, 0=low, 100 =
> high" },
> + { "avg_bitrate","[1-10000000]", "Average bits per second for stream"
> },
> + { "max_bitrate","[1-10000000]", "Maximum bits per second for stream"
> }
> + };
> + printf("\nsettings shared by all plugins:\n");
> + for (const UsageInfo &info : standardSettingsInfo) {
> + printf("%16s = %-16s : %s\n",
> + info.name, info.range, info.description);
> + }
> +
> + for (auto &plugin: plugins) {
> + const UsageInfo *info = plugin->Usage();
> + if (info) {
> + printf("\nsettings for %s:\n", plugin->Name());
> + for (info = info; info->name; info++) {
> + printf("%16s = %-16s : %s\n",
> + info->name, info->range, info->description);
> + }
> + }
> + }
> +}
> +
> +void ConcreteAgent::ApplyOptions(Plugin *plugin)
> +{
> + for (const auto &opt : options) {
> + try
> + {
> + bool known = plugin->ApplyOption(opt);
> + if (!known) {
> + Settings &settings = plugin->PluginSettings();
> + known = StandardOption(settings, opt);
> + }
> + if (!known) {
> + syslog(LOG_ERR, "Option %s not recognized by plugin %s",
> + opt.name, plugin->Name());
> + }
> + }
> + catch (OptionError &err)
> + {
> + syslog(LOG_ERR, "Plugin %s did not accept setting %s=%s: %s",
> + plugin->Name(), opt.name, opt.value, err.what());
> + }
> + }
> +}
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index 828368b..6852b51 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -12,33 +12,37 @@
>
> namespace SpiceStreamingAgent {
>
> -struct ConcreteConfigureOption: ConfigureOption
> -{
> - ConcreteConfigureOption(const char *name, const char *value)
> - {
> - this->name = name;
> - this->value = value;
> - }
> -};
> -
> class ConcreteAgent final : public Agent
> {
> public:
> ConcreteAgent();
> +
> +public:
> + // Implementation of the Agent class virtual methods
> unsigned Version() const override {
> return PluginVersion;
> }
> - void Register(Plugin& plugin) override;
> - const ConfigureOption* Options() const override;
> - void LoadPlugins(const char *directory);
> - // pointer must remain valid
> + void Register(Plugin *plugin) override;
> + bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
> + int IntOption(const Option &opt,
> + int min = INT_MIN, int max = INT_MAX,
> + bool sizeSuffixOK = false) const
> + throw(OptionError) override;
> +
> +public:
> + // Interface used by the main agent program
> void AddOption(const char *name, const char *value);
> + void LoadPlugins(const char *directory);
> FrameCapture *GetBestFrameCapture();
> - bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
> + bool StandardOption(Settings &settings, const Option &option)
> + throw (OptionError);
> + void PluginsUsage();
> +
> private:
> void LoadPlugin(const char *plugin_filename);
> + void ApplyOptions(Plugin *plugin);
> std::vector<std::shared_ptr<Plugin>> plugins;
> - std::vector<ConcreteConfigureOption> options;
> + std::vector<Option> options;
> };
>
> }
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index f41e68a..6bd8bf5 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -41,16 +41,11 @@ static inline uint64_t get_time()
> }
>
> namespace {
> -struct MjpegSettings
> -{
> - int fps;
> - int quality;
> -};
>
> class MjpegFrameCapture final: public FrameCapture
> {
> public:
> - MjpegFrameCapture(const MjpegSettings &settings);
> + MjpegFrameCapture(Settings &settings);
> ~MjpegFrameCapture();
> FrameInfo CaptureFrame() override;
> void Reset() override;
> @@ -58,8 +53,8 @@ public:
> return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> }
> private:
> - MjpegSettings settings;
> Display *dpy;
> + Settings &settings;
>
> vector<uint8_t> frame;
>
> @@ -72,19 +67,21 @@ private:
> class MjpegPlugin final: public Plugin
> {
> public:
> + MjpegPlugin(Agent *agent): Plugin(agent) {}
> + virtual const char *Name() override;
> + virtual const UsageInfo *Usage() override;
> FrameCapture *CreateCapture() override;
> unsigned Rank() override;
> - void ParseOptions(const ConfigureOption *options);
> - SpiceVideoCodecType VideoCodecType() const {
> - return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> - }
> + SpiceVideoCodecType VideoCodecType() const override;
> + Settings &PluginSettings() override;
> + bool ApplyOption(const Option &o) throw(OptionError) override;
> private:
> - MjpegSettings settings = { 10, 80 };
> + Settings settings;
> };
> }
>
> -MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& settings):
> - settings(settings)
> +MjpegFrameCapture::MjpegFrameCapture(Settings &settings)
> + : settings(settings)
> {
> dpy = XOpenDisplay(NULL);
> if (!dpy)
> @@ -111,7 +108,7 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
> if (last_time == 0) {
> last_time = now;
> } else {
> - const uint64_t delta = 1000000000u / settings.fps;
> + const uint64_t delta = 1000000000u / settings.framerate;
> if (now >= last_time + delta) {
> last_time = now;
> } else {
> @@ -148,13 +145,13 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>
> int format = ZPixmap;
> // TODO handle errors
> - XImage *image = XGetImage(dpy, win, win_info.x, win_info.y,
> + XImage *image = XGetImage(dpy, win, win_info.x, win_info.y,
> win_info.width, win_info.height, AllPlanes,
> format);
>
> // TODO handle errors
> // TODO multiple formats (only 32 bit)
> - write_JPEG_file(frame, settings.quality, (uint8_t*) image->data,
> - image->width, image->height);
> + write_JPEG_file(frame, settings.quality,
> + (uint8_t*) image->data, image->width, image->height);
>
> image->f.destroy_image(image);
>
> @@ -166,6 +163,16 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
> return info;
> }
>
> +const char *MjpegPlugin::Name()
> +{
> + return "MJPEG";
> +}
> +
> +const UsageInfo *MjpegPlugin::Usage()
> +{
> + return NULL;
> +}
> +
> FrameCapture *MjpegPlugin::CreateCapture()
> {
> return new MjpegFrameCapture(settings);
> @@ -176,33 +183,20 @@ unsigned MjpegPlugin::Rank()
> return FallBackMin;
> }
>
> -void MjpegPlugin::ParseOptions(const ConfigureOption *options)
> +SpiceVideoCodecType MjpegPlugin::VideoCodecType() const
> {
> -#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> -
> - for (; options->name; ++options) {
> - const char *name = options->name;
> - const char *value = options->value;
> -
> - if (strcmp(name, "framerate") == 0) {
> - int val = atoi(value);
> - if (val > 0) {
> - settings.fps = val;
> - }
> - else {
> - arg_error("wrong framerate arg %s\n", value);
> - }
> - }
> - if (strcmp(name, "mjpeg.quality") == 0) {
> - int val = atoi(value);
> - if (val > 0) {
> - settings.quality = val;
> - }
> - else {
> - arg_error("wrong mjpeg.quality arg %s\n", value);
> - }
> - }
> - }
> + return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> +}
> +
> +Settings &MjpegPlugin::PluginSettings()
> +{
> + return settings;
> +}
> +
> +bool MjpegPlugin::ApplyOption(const Option &o) throw(OptionError)
> +{
> + // This plugin only relies solely on base options
> + return false;
> }
>
> static bool
> @@ -211,12 +205,7 @@ mjpeg_plugin_init(Agent* agent)
> if (agent->Version() != PluginVersion)
> return false;
>
> - std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
> -
> - plugin->ParseOptions(agent->Options());
> -
> - agent->Register(*plugin.release());
> -
> + agent->Register(new MjpegPlugin(agent));
> return true;
> }
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index d5984bc..71a36e1 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -269,18 +269,16 @@ static void register_interrupts(void)
>
> static void usage(const char *progname)
> {
> - printf("usage: %s <options>\n", progname);
> - printf("options are:\n");
> - printf("\t-p portname -- virtio-serial port to use\n");
> - printf("\t-i accept commands from stdin\n");
> - printf("\t-l file -- log frames to file\n");
> - printf("\t--log-binary -- log binary frames (following -l)\n");
> - printf("\t-d -- enable debug logs\n");
> - printf("\t-c variable=value -- change settings\n");
> - printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
> - printf("\n");
> - printf("\t-h or --help -- print this help message\n");
> -
> + printf("usage: %s <options>\n"
> + "options are:\n"
> + "\t-p portname -- virtio-serial port to use\n"
> + "\t-i accept commands from stdin\n"
> + "\t-l file -- log frames to file\n"
> + "\t--log-binary -- log binary frames (following -l)\n"
> + "\t-d -- enable debug logs\n"
> + "\t-c variable=value -- change settings (see below)\n"
> + "\t-h or --help -- print this help message\n", progname);
> + agent.PluginsUsage();
> exit(1);
> }
>
> @@ -474,6 +472,7 @@ int main(int argc, char* argv[])
> setlogmask(logmask);
> break;
> case 'h':
> + agent.LoadPlugins(PLUGINSDIR);
> usage(argv[0]);
> break;
> }
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel