Re: [PATCH spice-streaming-agent 2/3] Rework option handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> 
> Several functional objectives:
> 
> 1. Be able to share some common options, e.g. fps
> 2. Prepare for the possibility to update options on the fly
> 
> Also get rid of the null-terminated C++ vector
> 
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>

This change is neither ABI not API compatible. Why?

> ---
>  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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]