[Spice-devel] [PATCH spice-streaming-agent 2/3] Rework option handling

Christophe de Dinechin cdupontd at redhat.com
Wed Nov 15 10:48:01 UTC 2017



> On 14 Nov 2017, at 16:53, Christophe de Dinechin <christophe.de.dinechin at gmail.com> wrote:
> 
> 
> 
>> On 14 Nov 2017, at 16:41, Frediano Ziglio <fziglio at redhat.com <mailto:fziglio at redhat.com>> wrote:
>> 
>>> 
>>> From: Christophe de Dinechin <dinechin at redhat.com <mailto:dinechin at 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 at redhat.com <mailto:dinechin at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171115/8662b6b3/attachment-0001.html>


More information about the Spice-devel mailing list