>
> 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.
Not clear how and why.
> */
> 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.
Why not using exceptions instead of a value passed as reference?
> + */
> + 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.
> /*!
> - * 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?
> };
>
> 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?
> +{
> + 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?
How the plugin knows the agent changed the settings?
And why StandardOption is public if called only by the agent?
> + }
> + 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.
> + }
> +}
> 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.
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel