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

[Christophe de Dinechin]

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