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>
This change is neither ABI not API compatible. Why?
Compatible with what? We are still building the very first versions of this agent, and nobody but us has tested it yet.
So I think it’s still the right time to fix things that are broken. In particular the fact that the curent API does not make it possible to adjust settings on the fly. Half of the experiment I have been doing can’t be done with the curent API. ---
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
+};
+
How are you going to extend this?
If we see other shared settings, now is a good time to think about them. Later additions to these settings would require an ABI break.
(Of note, we have the same issue with the command line options too)
+/*!
+ * 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
For which usage? The result is not const, should we expect tochange them? How to notify the plugin of changes?
See actual call. The usage is to let the agent manage options for plugins without knowing the details of their settings.
Ideally, I would have preferred a base class to deal with that, but that does not work, because the base class code is in the agent and the derived class in dynamically loaded plugins, so dlopen fails. + */
+ 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;
+ }
+ }
How do you deal with options supported by another plugin but not thisand not standard? Looks like like an error. I think should be ignored.
This is why it goes to syslog but does not throw. Maybe log a warning instead of an error? + 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);
don't get changes here. Just spaces?
Yes. Grouping all image parameters together as a result of not-published intermediate stages on this code. @@ -166,6 +162,18 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
return info;
}
+const char *MjpegPlugin::Name()
+{
+ return "MJPEG";
+}
+
Yes, name is really helpful.+const char *MjpegPlugin::Usage()
+{
+ return
+ "quality = [0-100] Set picture quality\n"
+ "framerate = [1-240] Set number of frames per second\n";
+}
+
Wondering about the indentation here ("quality" and " = [").There should be a standard maybe. Or a structured way to returnthese informations.
I considered returning an array and letting the caller do the formatting. And I think you are right, it’s the way to go. I’ll do that. 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;
}
Frediano
|