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

[Frediano Ziglio]

> 
> Hi,
> 
> On Tue, Nov 14, 2017 at 04:41:46PM +0100, Christophe de Dinechin wrote:
> > 
> > 
> > > On 14 Nov 2017, at 16:22, Frediano Ziglio  wrote:
> > > 
> > >> 
> > >> From: Christophe de Dinechin 
> > >> 
> > >> Signed-off-by: Christophe de Dinechin 
> > >> ---
> > >> 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Victor 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  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> 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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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  wrote:
> 
> On 14 Nov 2017, at 16:57, Frediano Ziglio  wrote:
> 
> 
> On 14 Nov 2017, at 16:22, Frediano Ziglio  wrote:
> 
> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
> 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 

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

[Frediano Ziglio]

> On 14 Nov 2017, at 16:57, Frediano Ziglio < fzig...@redhat.com > wrote:
> > > > On 14 Nov 2017, at 16:22, Frediano Ziglio < fzig...@redhat.com > wrote:
> > > 
> > 
> 

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

> > > > > Signed-off-by: Christophe de Dinechin < dinec...@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 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

> On 14 Nov 2017, at 16:57, Frediano Ziglio  wrote:
> 
> 
> On 14 Nov 2017, at 16:22, Frediano Ziglio  > wrote:
> 
> 
> From: Christophe de Dinechin  >
> 
> Signed-off-by: Christophe de Dinechin  >
> ---
> 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 
> https://lists.freedesktop.org/mailman/listinfo/spice-devel 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> > On 14 Nov 2017, at 16:22, Frediano Ziglio < fzig...@redhat.com > wrote:
> 

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

> > > Signed-off-by: Christophe de Dinechin < dinec...@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 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

> On 14 Nov 2017, at 16:22, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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 
> https://lists.freedesktop.org/mailman/listinfo/spice-devel 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel