From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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.
*/
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.
+ */
+ 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;
/*!
- * 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;
+
};
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)
+{
+ 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);
+ }
+ 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);
+ });
+ }
+}
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);
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;
}
--
2.13.5 (Apple Git-94)
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel