[spice-streaming-agent,2/3] Rework option handling

Submitted by Christophe de Dinechin on Nov. 14, 2017, 2:49 p.m.

Details

Message ID 20171114144943.19816-3-christophe@dinechin.org
State New
Headers show
Series "Rework option handling" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Nov. 14, 2017, 2:49 p.m.
From: Christophe de Dinechin <dinechin@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@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(-)

Patch hide | download patch | download mbox

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;
 }
 

Comments

> 
> From: Christophe de Dinechin <dinechin@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@redhat.com>

This change is neither ABI not API compatible. Why?

> ---
>  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?

> +/*!
> + * 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 to
change them? How to notify the plugin of changes?

> +     */
> +    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 this
and not standard? Looks like like an error. I think should be ignored.

> +    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?

> @@ -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 return
these informations.

>  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
> On 14 Nov 2017, at 16:41, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@redhat.com <mailto:dinechin@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@redhat.com <mailto:dinechin@redhat.com>>
> 
> 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 to
> change 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 this
> and 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 return
> these 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
> On 14 Nov 2017, at 16:53, Christophe de Dinechin <christophe.de.dinechin@gmail.com> wrote:
> 
> 
> 
>> On 14 Nov 2017, at 16:41, Frediano Ziglio <fziglio@redhat.com <mailto:fziglio@redhat.com>> wrote:
>> 
>>> 
>>> From: Christophe de Dinechin <dinechin@redhat.com <mailto:dinechin@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@redhat.com <mailto:dinechin@redhat.com>>
>> 
>> 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)

BTW, to clarify. I had considered adding something like:

	unsigned reserved[4];

to the settings.

The problem with that simplistic approach is that this means we can only
add settings that can safely be ignored, or have a 0 default value, or
something like that. None of the settings that I put there have this property,
so I believe it is unreasonable to expect we would be lucky next time.

So why have a shared Settings structure to start with? Consider a future
evolution of the agent where we want to be able to adjust frames-per-second
to deal with a client-side overload. We need a way to update the framerate
dynamically, and since the agent would make the request, it has to be
done in a way that is, if possible, independent from the plugin.

The settings I selected initially were the ones I thought would be helpful
in order to adjust the QoS later, and that I experimentally was able
to adjust dynamically on at least some of the existing plugins.

> 
>> 
>>> +/*!
>>> + * 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 to
>> change 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 this
>> and 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 return
>> these 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
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> From: Christophe de Dinechin <dinechin@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@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.

>   */
>  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;
>  }
>
> On 15 Nov 2017, at 13:55, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@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@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@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
> > On 15 Nov 2017, at 13:55, Frediano Ziglio < fziglio@redhat.com > wrote:
> 

> > > From: Christophe de Dinechin < dinechin@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@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.

Let me clarify one point. I agree the options interface lacks some feature. Changing 
them dynamically will be necessary. 
On the other hand the interface you are proposing is adding a lot of constrains which 
can be hard to bend in the future. As said having a fixed Settings structure is hard to 
extend. More that this having a method returning a reference to it force the Plugin 
to have a field containing it making it a strong ABI. A workaround would be to 
have a base class for it and allocate from the Agent. But I don't thinkthis will get so far. 
It looks that your interface is forcing both Agent and Plugin to handle the Settings 
structure. This looks messy to me, the responsibilities should not be shared that 
easily. I don't really understand why an option should be really shared. 
The agent should propose a value, the Plugin can accept and use, ignore or use 
a more sensible data. For instance if I say to a Plugin "I want 9600 bps" the Plugin 
should fallback to some more sensible value. I don't see the reason for the Agent to 
know the internal value (beside debugging purposes). 
The ApplyOption API push each option from the Agent to the Plugin. It looks 
like a property settings in some way. But what happens if some values are correlated? 
Let suppose I have (not in this case) a width and height options and the object needs 
to resize some internal memory based on width and height. Should the resize happens 
changing width or height? Or should the object just cache the values, mark as changed 
and resize the memory lazily? Would not be better to pass all options I want to change? 

> > 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.

On Unix is a good idea to have the exceptions classes not with restricted 
visibility. On Windows C++ is in the system ABI and this is supported by 
design. 

> (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

I don't see any global... and no reason to add a field to Plugin either. 

> 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.

I the agent restrict the range should craft a better option value and pass to the 
Plugin. If the Plugin has a better value should just use. 

> > 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.

So, the Agent can change internal settings of the Plugin and the Plugin does not need 
to know? 

> > 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.

But is neither an error nor a warning. 
If pluginA supports optionA and pluginB supports optionB to avoid these message 
I cannot use neither optionA nor optionB that is you basically cannot have options 
for the plugins but only standard options. 
Unless you want to have specific plugin options, in this case you can say that if 
optionX for pluginX is not supported by pluginX you give an error/warning 
while optionX for pluginX is ignored by pluginY. 

> 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.

Mumble... maybe we should have some documentation on responsibility. 

> > > 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;
> > 
> 
> > > }
> > 
> 

Frediano
Sorry for cutting out a lot of context, but this email appeared as HTML in my inbox.


> On 16 Nov 2017, at 14:34, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
> Let me clarify one point. I agree the options interface lacks some feature. Changing
> them dynamically will be necessary.
> On the other hand the interface you are proposing is adding a lot of constrains which
> can be hard to bend in the future. As said having a fixed Settings structure is hard to
> extend.

It will break the ABI. Is that such a big deal? We can even preserve compatibility
by having something like:

	struct SettingsV2 : Settings
	{
		new fields here
	};

and restrict access to the new fields to v2 plugins. So I don’t think it’s a big problem.


> More that this having a method returning a reference to it force the Plugin
> to have a field containing it making it a strong ABI.

No. The plugin could dynamically allocate it, e.g. to share it with another plugin
if two plugins are dealing with the same card. You’d obviously have to be a bit careful
in resource allocation, but I don’t see the interface as precluding any of that.


> A workaround would be to have a base class for it and allocate from the Agent.
> But I don't think this will get so far.

Wouldn’t that be over-engineering, given the problem at hand?


> It looks that your interface is forcing both Agent and Plugin to handle the Settings
> structure. This looks messy to me, the responsibilities should not be shared that
> easily. I don't really understand why an option should be really shared.

This is really on purpose. I try to clarify in the commit message for v2. The purpose
of Settings is to hold those specific settings that the agent will need to be able
to tweak in a common way.

The alternative would be to add these settings to the ABI for plugins, i.e. adding
something like:

	virtual bool set_framerate(int framerate) = 0;
	virtual bool set_quality(int quality) = 0;

That works, but is less efficient on several levels, and does not bring that much
of a benefit, since changing these methods would break the ABI anyway.

> The agent should propose a value, the Plugin can accept and use, ignore or use
> a more sensible data.

Yes. The proposed interface makes this possible. “Use a more sensible data” is
the reason I decided to give plugins “first dib” on all settings, including common ones.


> For instance if I say to a Plugin "I want 9600 bps" the Plugin
> should fallback to some more sensible value. I don't see the reason for the Agent to
> know the internal value (beside debugging purposes).

Besides the fact that it keeps the implementation simpler, here are additional reasons:

- The agent can report current settings
- The agent can keep stats on how the settings evolve
- The agent can see what values the plugin actually decided to commit (see “hysteresis” below)
- The agent easily knows if one setting impacted another (see “coupling” below)
- The settings are an atomic unit, not spread across multiple get/set (related to previous ones)


> The ApplyOption API push each option from the Agent to the Plugin. It looks
> like a property settings in some way. But what happens if some values are correlated?
> Let suppose I have (not in this case) a width and height options and the object needs
> to resize some internal memory based on width and height. Should the resize happens
> changing width or height? Or should the object just cache the values, mark as changed
> and resize the memory lazily? Would not be better to pass all options I want to change?

We do have one case of such coupling that I think exists today.
I added a “quality” setting, stating that it is some normalized
quality value from 1 to 100. My idea behind that was to have a knob that users
could tweak if they are willing to trade off some quality for something else.

Now, if a user sets quality to 50, for example, maybe the agent can someday
translate that into “That’s so many FPS, this bandwidth, etc”.
So user or agent would pass quality=50, and if you ask to read back, you’d have
non-default values for framerate, max_bitrate and max_bitrate.

If you are using an encoder that has more parameters, it may decide
that quality=50 means something else than what the agent would pick
up by default. The interface still allows the plugin to make that decision
based on the current state and the incoming request.

Finally, it could be perfectly legitimate for this kind of coupling to
have some hysteresis, e.g. have an invariant like
max_bitrate < 10 * quality. In that case, starting with
max_bitrate=100 and setting quality to 50 does not change it,
but if you start with max_bitrate=1000, then you end up with
max_bitrate=500.

On the other hand, sending multiple options at once makes the
semantics in such coupled cases much more complex. You
have to decide what takes precedence if you pass multiple options,
whether it’s coupled ones (e.g. bitrate=100k quality=5), conflicting
requirements (e.g. bitrate=100k bitrate=200k), or whether
there are differences between receiving W and H together or
in separate requests.

Because the semantics of the options themselves are more complicated,
the actual complexity when dealing with coupling or hysteresis will
be masked behind a complexity just dealing with options allocation,
insertion, deletion, replacement, etc.

> I don't see any global... and no reason to add a field to Plugin either.

It’s not there today, but if I want to be able to call Agent::IntOption from
the plugin, I need to have a reference or pointer to the actual agent.
I don’t want a global, so I chose an Agent pointer in the plugin.


> I the agent restrict the range should craft a better option value and pass to the
> Plugin. If the Plugin has a better value should just use.

What does “just use” mean? Options would be in a list of (name,value) pairs?
So that would mean lookup to find any existing option and remove them, then
insert a new option with a text representation of the new settings? Is that what
you had in mind? Or did you think of something else I did not understand?


> So, the Agent can change internal settings of the Plugin and the Plugin does not need
> to know?

In my dynamic adjustment experiments with the flight recorder,
the plugin simply keeps current settings and desired settings.
If they differ, then at a time that is safe for the specific device,
it updates the settings. For some systems, that may be the right
time for lazy evaluation of coupling, hysteresis, etc. 

There is no real need for a specific flag if a simple “if (settings != applied)”
does the trick. But nothing prevents you from
implementing ApplyOption in such a way that it sets a flag or
delivers some notification.

(contrast to a bunch of “set_xyz()” methods).

> But is neither an error nor a warning.
> If pluginA supports optionA and pluginB supports optionB to avoid these message
> I cannot use neither optionA nor optionB that is you basically cannot have options
> for the plugins but only standard options.

First, notice that right now we only apply options for the highest ranking plugins,
so we do expect to have plugin-specific options be dealt with by higher-ranking
plugins, which would then be selected and the option is as a result never seen
by lower ranking plugins. In other words, right now, plugin-specific options work
despite the fact that lower-ranking plugins don’t support them, because we exit
the frame capture creation loop as soon as we find a compatible plugin.

Second, as I mentioned earlier, I don’t think our code is currently very robust
to multiple plugins, for other reasons. Notably, if two plugins report as hardware
accelerated, there is no real guarantee as to which one comes first after the sort.
Only once we have addressed that can we really discuss the semantics of
options accepted by one plugin but rejected by another.


> Unless you want to have specific plugin options, in this case you can say that if
> optionX for pluginX is not supported by pluginX you give an error/warning
> while optionX for pluginX is ignored by pluginY.

Say that you have a gloptron card in your system, so you pass
the gloptron_schmuck=27 option. If for some reason your gloptron plugin
does not load and the MJPEG plugin ends up getting that option, don’t
you want a warning? Clearly, you did not intend to talk to the MJPEG
plugin if you passed a gloptron option.

We also discussed the idea of making it explicit that some option
was addressed to some specific card. If we have different vendors or
names, we could go with “gloptron_schmuck” and have non-gloptron
plugins just ignore anything that begins with a prefix like that.

But that’s the tip of the iceberg. How do we address a specific plugin
if the installation contains 3 gloptron plugins, 4 smurf plugins, and your
system has 2 gloptron cards (different plugins) and a smurf one,
that all suport the same codecs?

I think that attempting to send an option to a specific plugin that is
not the naturally highest ranking in the list is a complicated problem,
and I did not attempt to address it here.

Note that as currently implemented, all unknown or invalid options have
no other effect than a log message, which I think is the correct approach.

> Mumble... maybe we should have some documentation on responsibility.

Let’s get started 


Thanks
Christophe