[Spice-devel] [PATCH spice-streaming-agent 1/3] Don't tag the plugin interface as 1.0 just yet

[Christophe de Dinechin]

> On 15 Nov 2017, at 10:52, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> On 14 Nov 2017, at 16:57, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> 
> On 14 Nov 2017, at 16:22, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> 
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
> include/spice-streaming-agent/plugin.hpp | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp
> b/include/spice-streaming-agent/plugin.hpp
> index 727cb3b..607fabf 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -26,7 +26,7 @@ class FrameCapture;
>  * where MM is major and mm is the minor, can be easily expanded
>  * using more bits in the future
>  */
> -enum Constants : unsigned { PluginVersion = 0x100u };
> +enum Constants : unsigned { PluginVersion = 0x001u };
> 
> enum Ranks : unsigned {
>     /// this plugin should not be used
> 
> Nack... ABI not compatible
> 
> Precisely. We should not have started at 1.0. I think it’s still time to fix it.
> 
> Christophe
> Going backward? Would not be easier to get to 1.1 instead?
> Also 0x1 does not fit with the comment above.
> 
> Yes. I think that what we build presently should be labelled as 0.01, not 1.0.
> Unless you consider this ABI / API to be good enough to be labelled 1.0,
> which I do not, because it still lacks some very basic amenities.
> You acked 0x100 so now is quite too late to get back.

It’s too late on what basis? If “ack” means “I think the code is perfect and will never need to be changed”,
don’t expect me to ever ack anything again ;-)

Indeed, I did not pay enough attention to the fact hat you had selected 1.0 as
the version number. I cared more about the fact that there was a version number check
and the beginning of an API. I cared about the good in this interface more than
I cared about what I disliked about it, though I don’t think I was entirely silent about
some shortcomings I had seen either. So they should not come as a surprise.

My understanding of this version number is that it’s designed to check for ABI compatibility
once the product is released. At that time, and at that time only, will we have 1.0 and will
ABI checks have to be consistent.

Until then, I think it’s premature, and we should not artificially constrain ourselves
with ABI compatibility. I don’t actually expect the changes I submitted to be the last ones
to the ABI before we release, see below for more examples.


> If you consider that not "fixed" we can keep the number, if we consider fixed
> we'll bump it.
> Now, it’s only an internal number, so I don’t mind much going to 2.0 instead
> if you prefer. It cannot be 1.1, because that would be seen as compatible.
> But I thought 0.01 was fairer to the actual state of the API, while also
> indicating a compatibility break.
> I would try to extend maintaining API/ABI if possible. As far as I can see
> your mainly concerns are lack of some features.

No, it’s not just a lack of features, it’s really the way the ABI “thinks” about settings.
It was not too bad for a prototype, so it was worth acking. But it’s IMO not good enough
for a product. As the proposal discussed here shows, it lacks:

- What is needed to show per-plugin usage information
- Capability to dynamically adjust settings
- A way to share deal with settings shared by multiple plugins

We discussed that in the past several time, so I believe you knew
that these topics were a concern fo me.

Of course, this might be fixable in an incremental way, but that would only render
the API / ABI messy already in release 1.0, which I advocate against. For example,
that would force me to have two ways to apply settings, one for initial options,
one for later options. Why would I want to make the interface messy that early in
the game?

But my concern about options are not all there is to it, by far. This is why I expect
further changes before the first release. Among other things, just off the top of my
head:

- The error reporting right now looks fragile and ad-hoc.
- Logging is not abstracted nor encapsulated, can’t be configured
- I told you I believe the current sorting of plugins is incorrect because
it is not deterministic if multiple cards return the same value, and it
has to be able to report card initialization issues vs card capabilities
(you have a “todo” in some of the Rank methods regarding init
of the card, so you know that last one)
- No way to retrieve statistics or QoS info from the plugin
- No way for the plugin to be smart about adjusting quality
(a big factor in putting common quality parameters in a shared
Settings structure, BTW, otherwise how would the agent tell
the plugins “please lower the bandwidth” or “adjust the FPS”?)
- No way to cleanly restart the plugin in case of error
[Remember our discussion about my practical issues with Reset() for the recorder]

So really, I believe it is very premature and dangerous to treat this ABI/API as set in stone.
And this is in no way diminishing the work you did designing it, which I believe was
quite a fast-paced march forward from where we started. But there are still issues,
and we need to feel free to address them without saying “no, that’s the way it is, too late”.


> For an actual release, we can skip 1.0 if we fear there is any risk because
> at some point we used 1.0.


> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel