[Spice-devel] [PATCH spice-streaming-agent 2/3] Rework option handling
Christophe de Dinechin
christophe.de.dinechin at gmail.com
Thu Nov 16 09:49:42 UTC 2017
> On 15 Nov 2017, at 13:55, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>
>> From: Christophe de Dinechin <dinechin at redhat.com>
>>
>> Several functional objectives:
>>
>> 1. Be able to share some common options, e.g. fps
>> 2. Prepare for the possibility to update options on the fly
>>
>> Also get rid of the null-terminated C++ vector
>>
>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
>> ---
>> include/spice-streaming-agent/plugin.hpp | 120
>> +++++++++++++++++++++++++++----
>> m4/spice-compile-warnings.m4 | 2 +
>> src/concrete-agent.cpp | 77 ++++++++++++++++----
>> src/concrete-agent.hpp | 30 ++++----
>> src/mjpeg-fallback.cpp | 90 +++++++++++------------
>> 5 files changed, 227 insertions(+), 92 deletions(-)
>>
>> diff --git a/include/spice-streaming-agent/plugin.hpp
>> b/include/spice-streaming-agent/plugin.hpp
>> index 607fabf..41ad11f 100644
>> --- a/include/spice-streaming-agent/plugin.hpp
>> +++ b/include/spice-streaming-agent/plugin.hpp
>> @@ -8,6 +8,11 @@
>> #define SPICE_STREAMING_AGENT_PLUGIN_HPP
>>
>> #include <spice/enums.h>
>> +#include <climits>
>> +#include <sstream>
>> +#include <string>
>> +#include <vector>
>> +
>>
>> /*!
>> * \file
>> @@ -20,6 +25,7 @@
>> namespace SpiceStreamingAgent {
>>
>> class FrameCapture;
>> +class Agent;
>>
>> /*!
>> * Plugin version, only using few bits, schema is 0xMMmm
>> @@ -42,13 +48,23 @@ 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;
>> };
>> @@ -59,6 +75,9 @@ struct ConfigureOption
>> * 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.
>
> Not clear how and why.
Yes, the other patch series for the plugin did not go through, the originating
email address was rejected by the list server. Working on it :-)
>
>> */
>> class Plugin
>> {
>> @@ -66,7 +85,17 @@ public:
>> /*!
>> * 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
>> + */
>> + virtual const char *Usage() = 0;
>>
>> /*!
>> * Request an object for getting frames.
>> @@ -89,6 +118,19 @@ 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 try StandardOption.
>
> The agent knows its options so does not need to call ApplyOption to
> know if is a standard option or not.
I decided to do it the other way round, so that a plugin can do something
special with a standard option, e.g. special limits on bitrate.
> Why not using exceptions instead of a value passed as reference?
I was concerned about exceptions requiring RTTI information to be
shared between the agent and the plugin, which I thought might lead
to linking issues at dlopen time (much like if you leave the Plugin base
class have non-pure virtual methods).
Experimentally, that does not appear to be an issue, so I will do
that in the next iteration.
(BTW, you might think that having implemented the first “efficient”
exception handling mechanism, I would naturally be a big proponent
and user of exceptions. But it’s actually the opposite. I know how
expensive this stuff is, and how much it still hinders optimizations
for practical reasons. That probably tends to color my preferences
towards not using exception unless it’s really in some very slow
or infrequent execution path.)
>
>> + */
>> + virtual bool ApplyOption(const Option &o, std::string &error) = 0;
>> };
>>
>> /*!
>> @@ -113,24 +155,74 @@ 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.
>> + * Register a plugin in the system, which becomes the owner and should
>> delete it.
>> */
>> - virtual void Register(Plugin& plugin)=0;
>> + virtual void Register(Plugin *plugin)=0;
>>
>
> We should add a style document file.
The reason I made that change is not style, it is documenting that the target
takes ownership of it and deletes it (through the shared_ptr vector).
If you use a reference, it encourages the caller to pass a stack-based plugin,
expecting that the agent would make a copy or something, e.g. the following
would seem to be a natural usage:
MyPlugin plugin(…);
agent->Register(plugin);
not the rather clumsy:
shared_ptr<MyPlugin> plugin = new MyPlugin(…);
agent->Register(*plugin);
So I added a comment stating that the plugin has shared ownership of the pointer
(instead of stating that the pointer will be deleted). Now the natural usage is
the most natural way to dynamically allocate the plugin:
agent->Register(new MyPlugin(…));
>
>> /*!
>> - * Get options array.
>> - * Array is terminated with {nullptr, nullptr}.
>> - * Never nullptr.
>> - * \todo passing options to entry point instead?
>> + * Apply the standard options to the given settings (base Settings
>> class)
>> */
>> - virtual const ConfigureOption* Options() const=0;
>> + virtual bool StandardOption(Settings &, const Option &, std::string
>> &error) = 0;
>> +
>
> Is not clear this. Is the plugin telling the agent it changed some settings?
> Or telling the agent to update the setting structure?
> What the result mean?
It’s a historical leftover. Initially, the plugin was telling the agent, but then
I decided it was too dangerous to leave that to the agent, and forgot to remove
the function. I will remove it.
>
>> };
>>
>> typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
>>
>> +/*!
>> + * Helper to get integer values from command-line options
>> + */
>> +
>> +static inline int IntOption(const Option &opt, std::string &error,
>> + int min=INT_MIN, int max=INT_MAX, bool
>> sizeSuffixOK = false)
>
> Why not Agent::ParseIntOption or something similar?
I hesitated between the two. That was forcing the plugin to call back into
the agent.
One thing I hesitated to do was to add a field to the Plugin:
Agent *agent;
I don’t like globals much. If that’s OK with you, I’d switch to having
the option parsing exposed as an agent feature. That also reduces
the number of standard headers needed for the interface.
Definitely better.
>
>> +{
>> + std::string name = opt.name;
>> + std::string value = opt.value;
>> + std::ostringstream err;
>> + std::istringstream input(value);
>> + 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";
>> + error = err.str();
>> + }
>> + }
>> +
>> + if (input.fail()) {
>> + err << "Value " << value << " for " << name << " is not a number\n";
>> + error = err.str();
>> + }
>> + if (!input.eof()) {
>> + err << "Value " << value << " for " << name << " does not end like
>> an integer\n";
>> + error = err.str();
>> + }
>> + if (result < min || result > max) {
>> + err << "The value " << value << " for " << name
>> + << " is out of range (must be in " << min << ".." << max <<
>> ")\n";
>> + error = err.str(); // May actually combine an earlier error
>> + result = (min + max) / 2; // Give a value acceptable by caller
>> + }
>> +
>> + return result;
>> +}
>> +
>> +
>> }
>>
>> #ifndef SPICE_STREAMING_AGENT_PROGRAM
>> 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 192054a..59f11b2 100644
>> --- a/src/concrete-agent.cpp
>> +++ b/src/concrete-agent.cpp
>> @@ -9,6 +9,7 @@
>> #include <syslog.h>
>> #include <glob.h>
>> #include <dlfcn.h>
>> +#include <mutex>
>>
>> #include "concrete-agent.hpp"
>> #include "static-plugin.hpp"
>> @@ -28,7 +29,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
>> @@ -38,22 +43,34 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned
>> pluginVersion) const
>> MinorVersion(version) >= MinorVersion(pluginVersion);
>> }
>>
>> -void ConcreteAgent::Register(Plugin& plugin)
>> +bool ConcreteAgent::StandardOption(Settings &settings,
>> + const Option &option,
>> + string &error)
>> {
>> - plugins.push_back(shared_ptr<Plugin>(&plugin));
>> -}
>> + string name = option.name;
>> + if (name == "framerate" || name == "fps") {
>> + settings.framerate = IntOption(option, error, 1, 240);
>> + return true;
>> + }
>> + if (name == "quality") {
>> + settings.quality = IntOption(option, error, 0, 100);
>> + return true;
>> + }
>> + if (name == "avg_bitrate" || name == "bitrate") {
>> + settings.avg_bitrate = IntOption(option, error, 10000, 32000000,
>> true);
>> + return true;
>> + }
>> + if (name == "max_bitrate") {
>> + settings.max_bitrate = IntOption(option, error, 10000, 32000000,
>> true);
>> + return true;
>> + }
>>
>> -const ConfigureOption* ConcreteAgent::Options() const
>> -{
>> - static_assert(sizeof(ConcreteConfigureOption) ==
>> sizeof(ConfigureOption),
>> - "ConcreteConfigureOption should be binary compatible with
>> ConfigureOption");
>> - return static_cast<const ConfigureOption*>(&options[0]);
>> + 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)
>> @@ -81,7 +98,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;
>> }
>>
>> @@ -103,7 +121,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());
>> @@ -113,6 +131,7 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>> if (plugin.first == DontUse) {
>> break;
>> }
>> + ApplyOptions(plugin.second.get());
>> FrameCapture *capture = plugin.second->CreateCapture();
>> if (capture) {
>> return capture;
>> @@ -120,3 +139,35 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>> }
>> return nullptr;
>> }
>> +
>> +void ConcreteAgent::ApplyOptions(Plugin *plugin)
>> +{
>> + bool usage = false;
>> + for (const auto &opt : options) {
>> + string error;
>> + bool known = plugin->ApplyOption(opt, error);
>> + if (!known) {
>> + Settings &settings = plugin->PluginSettings();
>> + known = StandardOption(settings, opt, error);
>
> Can't the agent check for standard before?
As explained above, the idea is that the agent might have special
ways to deal with standard options, e.g. restrictions on the range.
> How the plugin knows the agent changed the settings?
Does it care? Right now, I am not testing nor reporting if the setting was modified.
> And why StandardOption is public if called only by the agent?
As written above, leftover from code evolution, I’ll remove it.
>
>> + }
>> + if (!known) {
>> + syslog(LOG_ERR, "Option %s not recognized by plugin %s",
>> + opt.name, plugin->Name());
>> + usage = true;
>> + }
>> + if (error != "") {
>> + syslog(LOG_ERR, "Plugin %s did not accept setting %s=%s: %s",
>> + plugin->Name(), opt.name, opt.value, error.c_str());
>> + usage = true;
>> + }
>> + }
>> + if (usage)
>> + {
>> + static std:: once_flag once;
>> + std::call_once(once, [plugin]()
>> + {
>> + const char *usage = plugin->Usage();
>> + syslog(LOG_ERR, "Usage: for plugin %s: %s", plugin->Name(),
>> usage);
>> + });
>
> This way the error/warning depends on the order of the plugins.
Hmm. You are right. But if a given plugin receives some incorrect input
repeatedly, I don’t want to flood the syslog. We already have the error
message that tells what option failed.
Thinking about it, I think showing usage information here is wrong,
I’ll remove it. The order of the messages is the order of the plugins
which is not consistent, but that’s another debate.
>
>> + }
>> +}
>> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
>> index 828368b..b3d4e06 100644
>> --- a/src/concrete-agent.hpp
>> +++ b/src/concrete-agent.hpp
>> @@ -12,33 +12,33 @@
>>
>> 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;
>> + bool StandardOption(Settings &settings,
>> + const Option &option,
>> + std::string &error) override;
>> +
>> +public:
>> + // Interface used by the main agent program
>> void AddOption(const char *name, const char *value);
>> + void LoadPlugins(const char *directory);
>> + void ApplyOptions(Plugin *plugin);
>
> Why this is public?
> Is the program dealing directly with plugins? Should not.
You are right.
>
>> FrameCapture *GetBestFrameCapture();
>> - bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
>> +
>> private:
>> void LoadPlugin(const char *plugin_filename);
>> 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..37df01a 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,20 @@ private:
>> class MjpegPlugin final: public Plugin
>> {
>> public:
>> + virtual const char *Name() override;
>> + virtual const char *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, string &error) 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 +107,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 +144,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 +162,18 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>> return info;
>> }
>>
>> +const char *MjpegPlugin::Name()
>> +{
>> + return "MJPEG";
>> +}
>> +
>> +const char *MjpegPlugin::Usage()
>> +{
>> + return
>> + "quality = [0-100] Set picture quality\n"
>> + "framerate = [1-240] Set number of frames per second\n";
>> +}
>> +
>> FrameCapture *MjpegPlugin::CreateCapture()
>> {
>> return new MjpegFrameCapture(settings);
>> @@ -176,33 +184,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, string &error)
>> +{
>> + // This plugin only relies on base options
>> + return false;
>> }
>>
>> static bool
>> @@ -211,12 +206,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);
>> return true;
>> }
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171116/5f282ebc/attachment-0001.html>
More information about the Spice-devel
mailing list