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

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

Details

Message ID 20171114144943.19816-2-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>

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/spice-streaming-agent/plugin.hpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

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

Comments

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

Frediano
> On 14 Nov 2017, at 16:22, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@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

> 
> Frediano
> _______________________________________________
> 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 14 Nov 2017, at 16:22, Frediano Ziglio < fziglio@redhat.com > wrote:
> 

> > > From: Christophe de Dinechin < dinechin@redhat.com >
> > 
> 

> > > Signed-off-by: Christophe de Dinechin < dinechin@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. 

Frediano
> On 14 Nov 2017, at 16:57, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
> 
> On 14 Nov 2017, at 16:22, Frediano Ziglio <fziglio@redhat.com <mailto:fziglio@redhat.com>> wrote:
> 
> 
> From: Christophe de Dinechin <dinechin@redhat.com <mailto:dinechin@redhat.com>>
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com <mailto:dinechin@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.

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.

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@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 14 Nov 2017, at 16:57, Frediano Ziglio < fziglio@redhat.com > wrote:
> > > > On 14 Nov 2017, at 16:22, Frediano Ziglio < fziglio@redhat.com > wrote:
> > > 
> > 
> 

> > > > > From: Christophe de Dinechin < dinechin@redhat.com >
> > > > 
> > > 
> > 
> 

> > > > > Signed-off-by: Christophe de Dinechin < dinechin@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. 
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. 

> 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
> On 15 Nov 2017, at 10:52, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
> On 14 Nov 2017, at 16:57, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
> 
> On 14 Nov 2017, at 16:22, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Tue, Nov 14, 2017 at 04:41:46PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 14 Nov 2017, at 16:22, Frediano Ziglio <fziglio@redhat.com> wrote:
> > 
> >> 
> >> From: Christophe de Dinechin <dinechin@redhat.com>
> >> 
> >> Signed-off-by: Christophe de Dinechin <dinechin@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

Note that spice-gtk is 7 years old, 2399 commits and still on 0.34
release ;)

Shouldn't be a problem to go to 2.0 afterwards, etc.

Cheers,
        toso
> 
> Hi,
> 
> On Tue, Nov 14, 2017 at 04:41:46PM +0100, Christophe de Dinechin wrote:
> > 
> > 
> > > On 14 Nov 2017, at 16:22, Frediano Ziglio <fziglio@redhat.com> wrote:
> > > 
> > >> 
> > >> From: Christophe de Dinechin <dinechin@redhat.com>
> > >> 
> > >> Signed-off-by: Christophe de Dinechin <dinechin@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
> 
> Note that spice-gtk is 7 years old, 2399 commits and still on 0.34
> release ;)
> 
> Shouldn't be a problem to go to 2.0 afterwards, etc.
> 
> Cheers,
>         toso
> 

You are confusing ABI version and software version, spice-gtk is 5.0.0 (ABI).

Frediano