Re: [Spice-devel] Multi-head Spice

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 04:18:12PM -0600, Jonathon Jongsma wrote:
> On Mon, 2017-02-27 at 17:53 +0100, Christophe de Dinechin wrote:
> > I tried to setup a multi-head guest, but I’m having trouble getting
> > multiple displays to work correctly
> > 
> > For Jonathon, the part that I had trouble with was adding “heads =
> > ‘4’" to the video / QXL configuration. I don’t see it documented in
> > https://www.spice-space.org/spice-user-
> > manual.html#_multiple_monitor_support.
> 
> Yep, it's a fairly new feature. I'm working on additional multi-monitor 
> documentation and will be sure to include that.

Yep, we totally missed it when 'heads' support was added :( virt-manager
UI needs to be able to set this too..

> 
> > 
> > For Christophe F, what I get with the configuration file http://paste
> > bin.com/FEXbjaE3 is shown in the following picture https://redskincat
> > .wordpress.com/2017/02/27/learning-more-about-mesa/#jp-carousel-1224.
> > As far as I remember, this is pretty much a default configuration of
> > Fedora 25 after install from the live CD. If I understand correctly,
> > the difference with you is that I actually installed.
> > 
> > Does anybody else see this? If not, what could be wrong with my
> > setup?
> > 
> 
> I don't see anything obviously wrong with the configuration. If you
> close the spice client and immediately re-connect does it still show
> the same thing in both windows?
> 
> Can you capture the debug output of running virt-viewer with the --
> debug and --spice-debug options while you enable the second monitor?
> That might give a clue about what's happening.

Also, what window manager are you using on the client? The window
decorations don't look like default GNOME setup? The linux client is
running on bare-metal, not in an osx VM?

Christophe


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] How to config video and image compression?

[John Y.]

Not locally, the guest run in server A.
I remote control the guest from my laptop.

 Regards,
John

2017-02-28 0:22 GMT+08:00 Uri Lublin :

> On 02/27/2017 04:01 AM, John Y. wrote:
>
>> I'm sorry that I misled you.
>> The guest OS is windows 7 so that I cannot use virgl.
>> I got the configuration from libvirt.org  and I
>> don't know which configuration will work for me, so I pasted all in the
>> mail.
>>
>> Do you mean the default configuration the best suited for my case?
>>
>
>
> If you run your VM "locally" (the server and the client
> are running on the same machine) you probably do not gain
> from image-compression/video-streaming.
>
>
>
>> 2017-02-27 0:58 GMT+08:00 Uri Lublin > >:
>>
>> On 02/23/2017 09:43 AM, John Y. wrote:
>>
>> I learned that video and image compression can be chosen server
>> initiation from
>> https://www.spice-space.org/spice-user-manual.html
>> .
>>
>> How can I change the configuration for the compression ?
>>
>> I want to play 720p video smoothly in the guest(some loss or
>> distortion
>> does not matter).
>> What is the best configuration for me?
>>
>> I got that I can change option of graphics in libvirt like:
>> 
>>   
>>   
>>   
>>   
>>   
>>   
>>   
>>   > rendernode='/dev/dri/by-path/pci-:00:02.0-render'/>
>> 
>>
>> Which option should I change ?
>>
>> Can I also customize more parameter about the vga(such as fps,
>> bitrate
>> ...) ?
>>
>> Regards,
>> John
>>
>>
>>
>> Hi John,
>>
>> What is the guest OS?
>>
>> According to your configuration above, I assume you are
>> running the VM locally on your workstation.
>> Also I see you are trying to use virgl (gl enable=yes)
>> If this is the case, for Linux guests, you need not to worry about
>> image/video compression.
>> Just make sure you are using virtio-vga and that the
>> given 'rendernode' is correct and accessible,
>> and that you have new enough qemu-kvm/spice-server and
>> remote-viewer or virt-manager.
>>
>> Regards,
>> Uri.
>>
>>
>>
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Multi-head Spice

[Jonathon Jongsma]

On Mon, 2017-02-27 at 17:53 +0100, Christophe de Dinechin wrote:
> I tried to setup a multi-head guest, but I’m having trouble getting
> multiple displays to work correctly
> 
> For Jonathon, the part that I had trouble with was adding “heads =
> ‘4’" to the video / QXL configuration. I don’t see it documented in
> https://www.spice-space.org/spice-user-
> manual.html#_multiple_monitor_support.

Yep, it's a fairly new feature. I'm working on additional multi-monitor 
documentation and will be sure to include that.

> 
> For Christophe F, what I get with the configuration file http://paste
> bin.com/FEXbjaE3 is shown in the following picture https://redskincat
> .wordpress.com/2017/02/27/learning-more-about-mesa/#jp-carousel-1224.
> As far as I remember, this is pretty much a default configuration of
> Fedora 25 after install from the live CD. If I understand correctly,
> the difference with you is that I actually installed.
> 
> Does anybody else see this? If not, what could be wrong with my
> setup?
> 

I don't see anything obviously wrong with the configuration. If you
close the spice client and immediately re-connect does it still show
the same thing in both windows?

Can you capture the debug output of running virt-viewer with the --
debug and --spice-debug options while you enable the second monitor?
That might give a clue about what's happening.

Thanks,
Jonathon
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] keymap detection under XWayland broken again ?

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 02:37:37PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 27, 2017 at 01:13:25PM +, Daniel P. Berrange wrote:
> > A long while back, Pavel learnt that under XWayland we needed to
> > use XkbGetMap instead of XkbGetKeyboard:
> > 
> >   commit 95e322a6bbc29dc9c28724fb80706464e276b89f
> >   Author: Pavel Grunt 
> >   Date:   Fri Feb 20 16:19:50 2015 +0100
> > 
> > vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of 
> > XkbGetKeyboard
> > 
> > XkbGetKeyboard does not work in XWayland (bfo#89240).
> > 
> > If I test spice-gtk today, forcing GTK to use X11 as the backend, the
> > keymap detection is broken for XWayland again :-(
> > 
> > $ GDK_BACKEND=x11  ./tools/spicy
> > 
> > (lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping 
> > '(unnamed)'.
> > Please report to gtk-vnc-l...@gnome.org
> > including the following information:
> > 
> >   - Operating system
> >   - GDK build
> >   - X11 Server
> >   - xprop -root
> >   - xdpyinfo
> > 
> > 
> > It seems, at least on my own Fedora 25 machine,  Xwayland is reporting
> > "unnamed" for the keymap instead of "evdev". I'm pretty sure this used
> > to work correctly when Pavel first wrote that changeset above, so I'm
> > wondering if anyone else sees this same behaviour, or if perhaps it is
> > something peculiar to my desktop install/setup. If its not jsut me, then
> > its a change in behaviour of Xwayland we'll need to adapt to. I really
> > hope we don't need to go back to my hack of checking if WAYLAND_DISPLAY
> > env variable exists :-(
> 
> I'm seeing these warnings too with GDK_BACKEND=x11, never tried to
> figure out what was going on, and I had forgotten that Pavel had fixed
> something similar in the past :( So no, it's not just you.

Ok, this problem just became alot more "fun". Initially when logging in
Xwayland correctly reports "evdev" as the keycode mapping. As soon as
you launch firefox, it starts to report "unknown".

So firefox is doing something to the Xwayland server that breaks it :-(

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Multi-head Spice

[Christophe de Dinechin]

I tried to setup a multi-head guest, but I’m having trouble getting multiple 
displays to work correctly

For Jonathon, the part that I had trouble with was adding “heads = ‘4’" to the 
video / QXL configuration. I don’t see it documented in 
https://www.spice-space.org/spice-user-manual.html#_multiple_monitor_support.

For Christophe F, what I get with the configuration file 
http://pastebin.com/FEXbjaE3  is shown in the 
following picture 
https://redskincat.wordpress.com/2017/02/27/learning-more-about-mesa/#jp-carousel-1224
 
.
 As far as I remember, this is pretty much a default configuration of Fedora 25 
after install from the live CD. If I understand correctly, the difference with 
you is that I actually installed.

Does anybody else see this? If not, what could be wrong with my setup?


Christophe





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

Re: [Spice-devel] [PATCH] Remove X == TRUE tests for non-boolean values

[Christophe de Dinechin]

> On 27 Feb 2017, at 17:10, Frediano Ziglio  wrote:
> 
> On 27 Feb 2017, at 13:12, Christophe Fergeau  > wrote:
> 
> On Mon, Feb 27, 2017 at 06:58:47AM -0500, Frediano Ziglio wrote:
> 
> Using X == TRUE is fragile, since the input value is a uint8_t. It would be
> surprising if the value was set to 2 or 0xFF and the test failed.
> 
> Signed-off-by: Christophe de Dinechin  >
> 
> Acked-by: Frediano Ziglio >
> 
> This apparently is going to cause warnings with gcc 7, see
> https://www.redhat.com/archives/libvir-list/2017-February/msg01199.html 
> 
> 
> if (dcc->priv->surface_client_created[surface_id] != 0) {} should work,
> or
> diff --git a/server/dcc-private.h b/server/dcc-private.h
> index 64b32a7..1cf3b0d 100644
> --- a/server/dcc-private.h
> +++ b/server/dcc-private.h
> @@ -51,7 +51,7 @@ struct DisplayChannelClientPrivate
> int num_pixmap_cache_items;
> } send_data;
> 
> -uint8_t surface_client_created[NUM_SURFACES];
> +bool surface_client_created[NUM_SURFACES];
> QRegion surface_client_lossy_region[NUM_SURFACES];
> 
> StreamAgent stream_agents[NUM_STREAMS];
> 
> which seems to be a more accurate type for 'surface_client_created’.
> 
> Looking at the declaration, I just noticed we pre-allocate 1 surfaces. 
> Why is this not dynamic allocation? Is it sane to allocate 250K statically 
> with each DisplayChannelClientPrivate?
> I think mostly old style pool allocations. Considering that usually we use 
> only 1 surface would be indeed much smaller.
> 
> I’m tempted to replace this with a dynamic array of QRegion *, where a 
> non-NULL pointer indicates that the client has been created. OK with that? I 
> expect some operations will be faster because they won’t iterate 
> I would use a new structure like
> 
> typedef struct {
> QRegion region;
> } RedClientSurfaceInfo;
> 
> make source code a bit longer (not the binary) but can be easily extended and 
> it's easier to
> understand a dcc->client_surface_info[i] != NULL than a 
> dcc->surface_client_lossy_region[i] != NULL.

Yes. Agreed.

> 
> over 1 mostly empty surfaces. There will be one extra indirection when 
> accessing the surface_client_lossy region, but x86_64 became pretty darn good 
> at chasing pointers like that thanks to C++ vtables ;-)
> Last sentence is a bit cryptic I think :-)

Sorry. Did not mean to be. What I meant is that chasing pointers like a->b->c 
is required to perform well for virtual function calls in C++ (this->foo() 
translates into this->vptr->vtbl[n]()). At some point, it was a huge 
performance hit for many workloads, because CPUs were not that good at 
prefetching this kind of double-dereferences. But C++ and other similar 
programming techniques with double indirection became so prevalent that CPUs 
are now quite good at it.


> Could be true.
> I don't think the extra indirection is a big deal, it's mostly used on 
> surface/channel creation/destroying.

OK

Christophe

> 
> 
> Christophe
> 
> Chistophe
> 
> 
> 
> 
> ---
> server/dcc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index afe37b1..7cfa72b 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -398,7 +398,7 @@ static void
> add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
> 
> surface_id = drawable->surface_deps[x];
> if (surface_id != -1) {
> -if (dcc->priv->surface_client_created[surface_id] == TRUE) {
> +if (dcc->priv->surface_client_created[surface_id]) {
> continue;
> }
> dcc_create_surface(dcc, surface_id);
> @@ -407,7 +407,7 @@ static void
> add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
> }
> }
> 
> -if (dcc->priv->surface_client_created[drawable->surface_id] == TRUE) {
> +if (dcc->priv->surface_client_created[drawable->surface_id]) {
> return;
> }
> Frediano

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

Re: [Spice-devel] How to config video and image compression?

[Uri Lublin]

On 02/27/2017 04:01 AM, John Y. wrote:

I'm sorry that I misled you.
The guest OS is windows 7 so that I cannot use virgl.
I got the configuration from libvirt.org  and I
don't know which configuration will work for me, so I pasted all in the
mail.

Do you mean the default configuration the best suited for my case?



If you run your VM "locally" (the server and the client
are running on the same machine) you probably do not gain
from image-compression/video-streaming.




2017-02-27 0:58 GMT+08:00 Uri Lublin >:

On 02/23/2017 09:43 AM, John Y. wrote:

I learned that video and image compression can be chosen server
initiation from
https://www.spice-space.org/spice-user-manual.html
.
How can I change the configuration for the compression ?

I want to play 720p video smoothly in the guest(some loss or
distortion
does not matter).
What is the best configuration for me?

I got that I can change option of graphics in libvirt like:

  
  
  
  
  
  
  
  


Which option should I change ?

Can I also customize more parameter about the vga(such as fps,
bitrate
...) ?

Regards,
John



Hi John,

What is the guest OS?

According to your configuration above, I assume you are
running the VM locally on your workstation.
Also I see you are trying to use virgl (gl enable=yes)
If this is the case, for Linux guests, you need not to worry about
image/video compression.
Just make sure you are using virtio-vga and that the
given 'rendernode' is correct and accessible,
and that you have new enough qemu-kvm/spice-server and
remote-viewer or virt-manager.

Regards,
Uri.




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

Re: [Spice-devel] [PATCH] Remove X == TRUE tests for non-boolean values

[Frediano Ziglio]

> > On 27 Feb 2017, at 13:12, Christophe Fergeau < cferg...@redhat.com > wrote:
> 

> > On Mon, Feb 27, 2017 at 06:58:47AM -0500, Frediano Ziglio wrote:
> 

> > > > Using X == TRUE is fragile, since the input value is a uint8_t. It
> > > > would
> > > > be
> > > 
> > 
> 
> > > > surprising if the value was set to 2 or 0xFF and the test failed.
> > > 
> > 
> 

> > > > Signed-off-by: Christophe de Dinechin < dinec...@redhat.com >
> > > 
> > 
> 

> > > Acked-by: Frediano Ziglio < fzig...@redhat.com >
> > 
> 

> > This apparently is going to cause warnings with gcc 7, see
> 
> > https://www.redhat.com/archives/libvir-list/2017-February/msg01199.html
> 

> > if (dcc->priv->surface_client_created[surface_id] != 0) {} should work,
> 
> > or
> 
> > diff --git a/server/dcc-private.h b/server/dcc-private.h
> 
> > index 64b32a7..1cf3b0d 100644
> 
> > --- a/server/dcc-private.h
> 
> > +++ b/server/dcc-private.h
> 
> > @@ -51,7 +51,7 @@ struct DisplayChannelClientPrivate
> 
> > int num_pixmap_cache_items;
> 
> > } send_data;
> 

> > - uint8_t surface_client_created[NUM_SURFACES];
> 
> > + bool surface_client_created[NUM_SURFACES];
> 
> > QRegion surface_client_lossy_region[NUM_SURFACES];
> 

> > StreamAgent stream_agents[NUM_STREAMS];
> 

> > which seems to be a more accurate type for 'surface_client_created’.
> 

> Looking at the declaration, I just noticed we pre-allocate 1 surfaces.
> Why is this not dynamic allocation? Is it sane to allocate 250K statically
> with each DisplayChannelClientPrivate?

I think mostly old style pool allocations. Considering that usually we use only 
1 surface would be indeed much smaller. 

> I’m tempted to replace this with a dynamic array of QRegion *, where a
> non-NULL pointer indicates that the client has been created. OK with that? I
> expect some operations will be faster because they won’t iterate

I would use a new structure like 

typedef struct { 
QRegion region; 
} RedClientSurfaceInfo; 

make source code a bit longer (not the binary) but can be easily extended and 
it's easier to 
understand a dcc->client_surface_info[i] != NULL than a 
dcc->surface_client_lossy_region[i] != NULL. 

> over 1 mostly empty surfaces. There will be one extra indirection when
> accessing the surface_client_lossy region, but x86_64 became pretty darn
> good at chasing pointers like that thanks to C++ vtables ;-)

Last sentence is a bit cryptic I think :-) 
Could be true. 
I don't think the extra indirection is a big deal, it's mostly used on 
surface/channel creation/destroying. 

> Christophe

> > Chistophe
> 

> > > > ---
> > > 
> > 
> 
> > > > server/dcc.c | 4 ++--
> > > 
> > 
> 
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > 
> 

> > > > diff --git a/server/dcc.c b/server/dcc.c
> > > 
> > 
> 
> > > > index afe37b1..7cfa72b 100644
> > > 
> > 
> 
> > > > --- a/server/dcc.c
> > > 
> > 
> 
> > > > +++ b/server/dcc.c
> > > 
> > 
> 
> > > > @@ -398,7 +398,7 @@ static void
> > > 
> > 
> 
> > > > add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
> > > 
> > 
> 

> > > > surface_id = drawable->surface_deps[x];
> > > 
> > 
> 
> > > > if (surface_id != -1) {
> > > 
> > 
> 
> > > > - if (dcc->priv->surface_client_created[surface_id] == TRUE) {
> > > 
> > 
> 
> > > > + if (dcc->priv->surface_client_created[surface_id]) {
> > > 
> > 
> 
> > > > continue;
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 
> > > > dcc_create_surface(dcc, surface_id);
> > > 
> > 
> 
> > > > @@ -407,7 +407,7 @@ static void
> > > 
> > 
> 
> > > > add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 

> > > > - if (dcc->priv->surface_client_created[drawable->surface_id] == TRUE)
> > > > {
> > > 
> > 
> 
> > > > + if (dcc->priv->surface_client_created[drawable->surface_id]) {
> > > 
> > 
> 
> > > > return;
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 

Frediano 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Remove X == TRUE tests for non-boolean values

[Christophe de Dinechin]

> On 27 Feb 2017, at 13:12, Christophe Fergeau  wrote:
> 
> On Mon, Feb 27, 2017 at 06:58:47AM -0500, Frediano Ziglio wrote:
>>> 
>>> Using X == TRUE is fragile, since the input value is a uint8_t. It would be
>>> surprising if the value was set to 2 or 0xFF and the test failed.
>>> 
>>> Signed-off-by: Christophe de Dinechin >> >
>> 
>> Acked-by: Frediano Ziglio >
> 
> This apparently is going to cause warnings with gcc 7, see
> https://www.redhat.com/archives/libvir-list/2017-February/msg01199.html 
> 
> 
> if (dcc->priv->surface_client_created[surface_id] != 0) {} should work,
> or
> diff --git a/server/dcc-private.h b/server/dcc-private.h
> index 64b32a7..1cf3b0d 100644
> --- a/server/dcc-private.h
> +++ b/server/dcc-private.h
> @@ -51,7 +51,7 @@ struct DisplayChannelClientPrivate
> int num_pixmap_cache_items;
> } send_data;
> 
> -uint8_t surface_client_created[NUM_SURFACES];
> +bool surface_client_created[NUM_SURFACES];
> QRegion surface_client_lossy_region[NUM_SURFACES];
> 
> StreamAgent stream_agents[NUM_STREAMS];
> 
> which seems to be a more accurate type for 'surface_client_created’.

Looking at the declaration, I just noticed we pre-allocate 1 surfaces. Why 
is this not dynamic allocation? Is it sane to allocate 250K statically with 
each DisplayChannelClientPrivate?

I’m tempted to replace this with a dynamic array of QRegion *, where a non-NULL 
pointer indicates that the client has been created. OK with that? I expect some 
operations will be faster because they won’t iterate over 1 mostly empty 
surfaces. There will be one extra indirection when accessing the 
surface_client_lossy region, but x86_64 became pretty darn good at chasing 
pointers like that thanks to C++ vtables ;-)


Christophe
> 
> Chistophe
> 
> 
> 
>> 
>>> ---
>>> server/dcc.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/server/dcc.c b/server/dcc.c
>>> index afe37b1..7cfa72b 100644
>>> --- a/server/dcc.c
>>> +++ b/server/dcc.c
>>> @@ -398,7 +398,7 @@ static void
>>> add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
>>> 
>>> surface_id = drawable->surface_deps[x];
>>> if (surface_id != -1) {
>>> -if (dcc->priv->surface_client_created[surface_id] == TRUE) {
>>> +if (dcc->priv->surface_client_created[surface_id]) {
>>> continue;
>>> }
>>> dcc_create_surface(dcc, surface_id);
>>> @@ -407,7 +407,7 @@ static void
>>> add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
>>> }
>>> }
>>> 
>>> -if (dcc->priv->surface_client_created[drawable->surface_id] == TRUE) {
>>> +if (dcc->priv->surface_client_created[drawable->surface_id]) {
>>> return;
>>> }
>>> 
>> 
>> 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 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice (stable)] sound: do not change volume or mute state on migration

[Victor Toso]

From: Victor Toso 

On migration, we are resending the current volume and mute state in
the Guest. If the client user has change its master volume in the
guest it might change the client application volume too and the volume
jump (increase or decrease) might happen on migration.

This patch is a complement of f10de4bc084fcc - Here, volume was
jumping regardless of guest's volume value.

Resolves: rhbz#1425443
Signed-off-by: Victor Toso 
---
 server/snd_worker.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/server/snd_worker.c b/server/snd_worker.c
index 1b9bad5..343a69e 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -560,6 +560,12 @@ static int snd_send_volume(SndChannel *channel, 
SpiceVolumeState *st, int msg)
 SpiceMsgAudioVolume *vol;
 uint8_t c;
 
+/* Never changes volume or mute state on migration */
+if (red_client_during_migrate_at_target(channel->channel_client->client)) {
+spice_debug("Do not change volume during migration");
+return FALSE;
+}
+
 vol = alloca(sizeof (SpiceMsgAudioVolume) +
  st->volume_nchannels * sizeof (uint16_t));
 if (!snd_reset_send_data(channel, msg)) {
@@ -591,6 +597,12 @@ static int snd_send_mute(SndChannel *channel, 
SpiceVolumeState *st, int msg)
 {
 SpiceMsgAudioMute mute;
 
+/* Never changes volume or mute state on migration */
+if (red_client_during_migrate_at_target(channel->channel_client->client)) {
+spice_debug("Do not change mute during migration");
+return FALSE;
+}
+
 if (!snd_reset_send_data(channel, msg)) {
 return FALSE;
 }
-- 
2.9.3

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

[Spice-devel] [spice (stable)] Resending volume value on migration

[Victor Toso]

From: Victor Toso 

Hi,

This patch is actually an improvement of an old patch [0]. It fixes rhbz#1425443
here. I'm sending this for stable branch only as we might have a better way to
do it on the current master as a lot has been improved.

[0] https://lists.freedesktop.org/archives/spice-devel/2015-April/019605.html
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1425443

Let me know if this seems good enough for stable branch.

Victor Toso (1):
  sound: do not change volume or mute state on migration

 server/snd_worker.c | 12 
 1 file changed, 12 insertions(+)

-- 
2.9.3

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

Re: [Spice-devel] keymap detection under XWayland broken again ?

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 01:13:25PM +, Daniel P. Berrange wrote:
> A long while back, Pavel learnt that under XWayland we needed to
> use XkbGetMap instead of XkbGetKeyboard:
> 
>   commit 95e322a6bbc29dc9c28724fb80706464e276b89f
>   Author: Pavel Grunt 
>   Date:   Fri Feb 20 16:19:50 2015 +0100
> 
> vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of XkbGetKeyboard
> 
> XkbGetKeyboard does not work in XWayland (bfo#89240).
> 
> If I test spice-gtk today, forcing GTK to use X11 as the backend, the
> keymap detection is broken for XWayland again :-(
> 
> $ GDK_BACKEND=x11  ./tools/spicy
> 
> (lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping '(unnamed)'.
> Please report to gtk-vnc-l...@gnome.org
> including the following information:
> 
>   - Operating system
>   - GDK build
>   - X11 Server
>   - xprop -root
>   - xdpyinfo
> 
> 
> It seems, at least on my own Fedora 25 machine,  Xwayland is reporting
> "unnamed" for the keymap instead of "evdev". I'm pretty sure this used
> to work correctly when Pavel first wrote that changeset above, so I'm
> wondering if anyone else sees this same behaviour, or if perhaps it is
> something peculiar to my desktop install/setup. If its not jsut me, then
> its a change in behaviour of Xwayland we'll need to adapt to. I really
> hope we don't need to go back to my hack of checking if WAYLAND_DISPLAY
> env variable exists :-(

I'm seeing these warnings too with GDK_BACKEND=x11, never tried to
figure out what was going on, and I had forgotten that Pavel had fixed
something similar in the past :( So no, it's not just you.

Christophe


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] keymap detection under XWayland broken again ?

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 02:24:16PM +0100, Pavel Grunt wrote:
> On Mon, 2017-02-27 at 13:13 +, Daniel P. Berrange wrote:
> > A long while back, Pavel learnt that under XWayland we needed to
> > use XkbGetMap instead of XkbGetKeyboard:
> > 
> >   commit 95e322a6bbc29dc9c28724fb80706464e276b89f
> >   Author: Pavel Grunt 
> >   Date:   Fri Feb 20 16:19:50 2015 +0100
> > 
> > vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of
> > XkbGetKeyboard
> > 
> > XkbGetKeyboard does not work in XWayland (bfo#89240).
> > 
> > If I test spice-gtk today, forcing GTK to use X11 as the backend,
> > the
> > keymap detection is broken for XWayland again :-(
> > 
> > $ GDK_BACKEND=x11  ./tools/spicy
> > 
> > (lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping
> > '(unnamed)'.
> > Please report to gtk-vnc-l...@gnome.org
> > including the following information:
> > 
> >   - Operating system
> >   - GDK build
> >   - X11 Server
> >   - xprop -root
> >   - xdpyinfo
> > 
> > 
> > It seems, at least on my own Fedora 25 machine,  Xwayland is
> > reporting
> > "unnamed" for the keymap instead of "evdev". I'm pretty sure this
> > used
> > to work correctly when Pavel first wrote that changeset above, so
> > I'm
> > wondering if anyone else sees this same behaviour, or if perhaps it
> > is
> > something peculiar to my desktop install/setup. If its not jsut me,
> > then
> > its a change in behaviour of Xwayland we'll need to adapt to.
> 
> There is https://bugzilla.redhat.com/show_bug.cgi?id=1413814 , it is
> about ssh -X from the Wayland client to X11, in fact it does the same
> as your reproducer

Yeah, I'm not using SSH at all - just my local GNOME session with Wayland

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] keymap detection under XWayland broken again ?

[Pavel Grunt]

On Mon, 2017-02-27 at 13:13 +, Daniel P. Berrange wrote:
> A long while back, Pavel learnt that under XWayland we needed to
> use XkbGetMap instead of XkbGetKeyboard:
> 
>   commit 95e322a6bbc29dc9c28724fb80706464e276b89f
>   Author: Pavel Grunt 
>   Date:   Fri Feb 20 16:19:50 2015 +0100
> 
> vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of
> XkbGetKeyboard
> 
> XkbGetKeyboard does not work in XWayland (bfo#89240).
> 
> If I test spice-gtk today, forcing GTK to use X11 as the backend,
> the
> keymap detection is broken for XWayland again :-(
> 
> $ GDK_BACKEND=x11  ./tools/spicy
> 
> (lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping
> '(unnamed)'.
> Please report to gtk-vnc-l...@gnome.org
> including the following information:
> 
>   - Operating system
>   - GDK build
>   - X11 Server
>   - xprop -root
>   - xdpyinfo
> 
> 
> It seems, at least on my own Fedora 25 machine,  Xwayland is
> reporting
> "unnamed" for the keymap instead of "evdev". I'm pretty sure this
> used
> to work correctly when Pavel first wrote that changeset above, so
> I'm
> wondering if anyone else sees this same behaviour, or if perhaps it
> is
> something peculiar to my desktop install/setup. If its not jsut me,
> then
> its a change in behaviour of Xwayland we'll need to adapt to.

There is https://bugzilla.redhat.com/show_bug.cgi?id=1413814 , it is
about ssh -X from the Wayland client to X11, in fact it does the same
as your reproducer

>  I really
> hope we don't need to go back to my hack of checking if
> WAYLAND_DISPLAY
> env variable exists :-(
> 
> Regards,
> Daniel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice v9] dcc: handle preferred video codec message

[Victor Toso]

Hi,

On Mon, Feb 27, 2017 at 08:06:30AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso 
> > 
> > [0] SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE
> > 
> > This message provides a list of video codecs based on client's order
> > of preference.
> > 
> > We duplicate the video codecs array from reds.c and sort it using the
> > order of codecs as reference.
> > 
> > This message will not change an ongoing streaming but it could change
> > newly created streams depending the rank value of each video codec
> > that can be set by spice_server_set_video_codecs()
> > 
> > Signed-off-by: Victor Toso 
> > ---
> >  server/dcc-private.h |   5 ++
> >  server/dcc.c | 132
> >  +++
> >  server/dcc.h |   1 +
> >  server/display-channel.c |   2 +
> >  server/stream.c  |   3 +-
> >  5 files changed, 141 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/dcc-private.h b/server/dcc-private.h
> > index 64b32a7..bf69fbb 100644
> > --- a/server/dcc-private.h
> > +++ b/server/dcc-private.h
> > @@ -51,6 +51,11 @@ struct DisplayChannelClientPrivate
> >  int num_pixmap_cache_items;
> >  } send_data;
> >  
> > +/* Host preferred video-codec order sorted with client preferred */
> > +GArray *preferred_video_codecs;
> > +/* Last client preferred video-codec order */
> > +GArray *client_preferred_video_codecs;
> > +
> >  uint8_t surface_client_created[NUM_SURFACES];
> >  QRegion surface_client_lossy_region[NUM_SURFACES];
> >  
> > diff --git a/server/dcc.c b/server/dcc.c
> > index cf9431a..476f43d 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -39,6 +39,8 @@ enum
> >  PROP_ZLIB_GLZ_STATE
> >  };
> >  
> > +static void on_display_video_codecs_update(GObject *gobject, GParamSpec
> > *pspec, gpointer user_data);
> > +
> >  static void
> >  display_channel_client_get_property(GObject *object,
> >  guint property_id,
> > @@ -99,12 +101,19 @@ display_channel_client_constructed(GObject *object)
> >  dcc_init_stream_agents(self);
> >  
> >  image_encoders_init(>priv->encoders,
> >  _TO_DC(self)->priv->encoder_shared_data);
> > +
> > +g_signal_connect(DCC_TO_DC(self), "notify::video-codecs",
> > + G_CALLBACK(on_display_video_codecs_update), self);
> >  }
> >  
> >  static void
> >  display_channel_client_finalize(GObject *object)
> >  {
> >  DisplayChannelClient *self = DISPLAY_CHANNEL_CLIENT(object);
> > +
> > +g_signal_handlers_disconnect_by_func(DCC_TO_DC(self),
> > on_display_video_codecs_update, self);
> > +g_clear_pointer(>priv->preferred_video_codecs, g_array_unref);
> > +g_clear_pointer(>priv->client_preferred_video_codecs,
> > g_array_unref);
> >  g_free(self->priv);
> >  
> >  G_OBJECT_CLASS(display_channel_client_parent_class)->finalize(object);
> > @@ -1105,6 +1114,126 @@ static int
> > dcc_handle_preferred_compression(DisplayChannelClient *dcc,
> >  return TRUE;
> >  }
> >  
> > +
> > +/* TODO: Client preference should only be considered when host has
> > video-codecs
> > + * with the same priority value. At the moment, the video-codec GArray will
> > be
> > + * sorted following only the client's preference (@user_data)
> > + *
> > + * example:
> > + * host encoding preference: gstreamer:mjpeg;gstreamer:vp8;gstreamer:h264
> > + * client decoding preference: h264, vp9, mjpeg
> > + * result: gstreamer:h264;gstreamer:mjpeg;gstreamer:vp8
> > + */
> > +static gint sort_video_codecs_by_client_preference(gconstpointer a_pointer,
> > +   gconstpointer b_pointer,
> > +   gpointer user_data)
> > +{
> > +const RedVideoCodec *a = a_pointer;
> > +const RedVideoCodec *b = b_pointer;
> > +
> > +if (a->type != b->type) {
> > +guint i;
> > +GArray *client_pref = user_data;
> > +
> > +for (i = 0; i < client_pref->len; i++) {
> > +if (a->type == g_array_index(client_pref, gint, i)) {
> > +return -1;
> > +}
> > +if (b->type == g_array_index(client_pref, gint, i)) {
> > +return 1;
> > +}
> > +}
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +static void dcc_update_preferred_video_codecs(DisplayChannelClient *dcc)
> > +{
> > +guint i;
> > +GArray *video_codecs, *server_codecs;
> > +GString *msg;
> > +
> > +server_codecs = display_channel_get_video_codecs(DCC_TO_DC(dcc));
> > +spice_return_if_fail(server_codecs != NULL);
> > +
> > +/* Copy current host preference */
> > +video_codecs = g_array_sized_new(FALSE, FALSE, sizeof(RedVideoCodec),
> > server_codecs->len);
> > +g_array_append_vals(video_codecs, server_codecs->data,
> > server_codecs->len);
> > +
> > +/* Sort the copy of current host preference based 

[Spice-devel] keymap detection under XWayland broken again ?

[Daniel P. Berrange]

A long while back, Pavel learnt that under XWayland we needed to
use XkbGetMap instead of XkbGetKeyboard:

  commit 95e322a6bbc29dc9c28724fb80706464e276b89f
  Author: Pavel Grunt 
  Date:   Fri Feb 20 16:19:50 2015 +0100

vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of XkbGetKeyboard

XkbGetKeyboard does not work in XWayland (bfo#89240).

If I test spice-gtk today, forcing GTK to use X11 as the backend, the
keymap detection is broken for XWayland again :-(

$ GDK_BACKEND=x11  ./tools/spicy

(lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping '(unnamed)'.
Please report to gtk-vnc-l...@gnome.org
including the following information:

  - Operating system
  - GDK build
  - X11 Server
  - xprop -root
  - xdpyinfo


It seems, at least on my own Fedora 25 machine,  Xwayland is reporting
"unnamed" for the keymap instead of "evdev". I'm pretty sure this used
to work correctly when Pavel first wrote that changeset above, so I'm
wondering if anyone else sees this same behaviour, or if perhaps it is
something peculiar to my desktop install/setup. If its not jsut me, then
its a change in behaviour of Xwayland we'll need to adapt to. I really
hope we don't need to go back to my hack of checking if WAYLAND_DISPLAY
env variable exists :-(

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice v9] dcc: handle preferred video codec message

[Frediano Ziglio]

> 
> From: Victor Toso 
> 
> [0] SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE
> 
> This message provides a list of video codecs based on client's order
> of preference.
> 
> We duplicate the video codecs array from reds.c and sort it using the
> order of codecs as reference.
> 
> This message will not change an ongoing streaming but it could change
> newly created streams depending the rank value of each video codec
> that can be set by spice_server_set_video_codecs()
> 
> Signed-off-by: Victor Toso 
> ---
>  server/dcc-private.h |   5 ++
>  server/dcc.c | 132
>  +++
>  server/dcc.h |   1 +
>  server/display-channel.c |   2 +
>  server/stream.c  |   3 +-
>  5 files changed, 141 insertions(+), 2 deletions(-)
> 
> diff --git a/server/dcc-private.h b/server/dcc-private.h
> index 64b32a7..bf69fbb 100644
> --- a/server/dcc-private.h
> +++ b/server/dcc-private.h
> @@ -51,6 +51,11 @@ struct DisplayChannelClientPrivate
>  int num_pixmap_cache_items;
>  } send_data;
>  
> +/* Host preferred video-codec order sorted with client preferred */
> +GArray *preferred_video_codecs;
> +/* Last client preferred video-codec order */
> +GArray *client_preferred_video_codecs;
> +
>  uint8_t surface_client_created[NUM_SURFACES];
>  QRegion surface_client_lossy_region[NUM_SURFACES];
>  
> diff --git a/server/dcc.c b/server/dcc.c
> index cf9431a..476f43d 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -39,6 +39,8 @@ enum
>  PROP_ZLIB_GLZ_STATE
>  };
>  
> +static void on_display_video_codecs_update(GObject *gobject, GParamSpec
> *pspec, gpointer user_data);
> +
>  static void
>  display_channel_client_get_property(GObject *object,
>  guint property_id,
> @@ -99,12 +101,19 @@ display_channel_client_constructed(GObject *object)
>  dcc_init_stream_agents(self);
>  
>  image_encoders_init(>priv->encoders,
>  _TO_DC(self)->priv->encoder_shared_data);
> +
> +g_signal_connect(DCC_TO_DC(self), "notify::video-codecs",
> + G_CALLBACK(on_display_video_codecs_update), self);
>  }
>  
>  static void
>  display_channel_client_finalize(GObject *object)
>  {
>  DisplayChannelClient *self = DISPLAY_CHANNEL_CLIENT(object);
> +
> +g_signal_handlers_disconnect_by_func(DCC_TO_DC(self),
> on_display_video_codecs_update, self);
> +g_clear_pointer(>priv->preferred_video_codecs, g_array_unref);
> +g_clear_pointer(>priv->client_preferred_video_codecs,
> g_array_unref);
>  g_free(self->priv);
>  
>  G_OBJECT_CLASS(display_channel_client_parent_class)->finalize(object);
> @@ -1105,6 +1114,126 @@ static int
> dcc_handle_preferred_compression(DisplayChannelClient *dcc,
>  return TRUE;
>  }
>  
> +
> +/* TODO: Client preference should only be considered when host has
> video-codecs
> + * with the same priority value. At the moment, the video-codec GArray will
> be
> + * sorted following only the client's preference (@user_data)
> + *
> + * example:
> + * host encoding preference: gstreamer:mjpeg;gstreamer:vp8;gstreamer:h264
> + * client decoding preference: h264, vp9, mjpeg
> + * result: gstreamer:h264;gstreamer:mjpeg;gstreamer:vp8
> + */
> +static gint sort_video_codecs_by_client_preference(gconstpointer a_pointer,
> +   gconstpointer b_pointer,
> +   gpointer user_data)
> +{
> +const RedVideoCodec *a = a_pointer;
> +const RedVideoCodec *b = b_pointer;
> +
> +if (a->type != b->type) {
> +guint i;
> +GArray *client_pref = user_data;
> +
> +for (i = 0; i < client_pref->len; i++) {
> +if (a->type == g_array_index(client_pref, gint, i)) {
> +return -1;
> +}
> +if (b->type == g_array_index(client_pref, gint, i)) {
> +return 1;
> +}
> +}
> +}
> +
> +return 0;
> +}
> +
> +static void dcc_update_preferred_video_codecs(DisplayChannelClient *dcc)
> +{
> +guint i;
> +GArray *video_codecs, *server_codecs;
> +GString *msg;
> +
> +server_codecs = display_channel_get_video_codecs(DCC_TO_DC(dcc));
> +spice_return_if_fail(server_codecs != NULL);
> +
> +/* Copy current host preference */
> +video_codecs = g_array_sized_new(FALSE, FALSE, sizeof(RedVideoCodec),
> server_codecs->len);
> +g_array_append_vals(video_codecs, server_codecs->data,
> server_codecs->len);
> +
> +/* Sort the copy of current host preference based on client's preference
> */
> +g_array_sort_with_data(video_codecs,
> sort_video_codecs_by_client_preference,
> +   dcc->priv->client_preferred_video_codecs);
> +g_clear_pointer(>priv->preferred_video_codecs, g_array_unref);
> +dcc->priv->preferred_video_codecs = video_codecs;
> +
> +msg = 

Re: [Spice-devel] [PATCH spice-server v2] reds-stream: Introduce reds_stream_set_no_delay() helper

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 01:42:37PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 27, 2017 at 12:35:15PM +, Frediano Ziglio wrote:
> > From: Christophe Fergeau 
> > 
> > The code to enable/disable on a TCP socket is duplicated in multiple
> > places in the code base, this commit replaces this duplicated code with
> > a helper in RedsStream.
> > ---
> >  server/common-graphics-channel.c | 15 +++
> >  server/red-channel-client.c  | 17 ++---
> >  server/reds-stream.c | 16 
> >  server/reds-stream.h |  1 +
> >  server/reds.c|  8 +---
> >  server/sound.c   |  8 +---
> >  6 files changed, 24 insertions(+), 41 deletions(-)
> > 
> > Changes since v1:
> > - boolean style updates
> 
> As far as I'm concerned, NACK to these changes.

With that said, I'm happy with preparing a patch cleaning up
reds-stream.[ch] from a bool/gboolean use point of view.

Christophe


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-server v2] reds-stream: Introduce reds_stream_set_no_delay() helper

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 12:35:15PM +, Frediano Ziglio wrote:
> From: Christophe Fergeau 
> 
> The code to enable/disable on a TCP socket is duplicated in multiple
> places in the code base, this commit replaces this duplicated code with
> a helper in RedsStream.
> ---
>  server/common-graphics-channel.c | 15 +++
>  server/red-channel-client.c  | 17 ++---
>  server/reds-stream.c | 16 
>  server/reds-stream.h |  1 +
>  server/reds.c|  8 +---
>  server/sound.c   |  8 +---
>  6 files changed, 24 insertions(+), 41 deletions(-)
> 
> Changes since v1:
> - boolean style updates

As far as I'm concerned, NACK to these changes.

Christophe



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

[Spice-devel] [PATCH spice-server v2] reds-stream: Introduce reds_stream_set_no_delay() helper

[Frediano Ziglio]

From: Christophe Fergeau 

The code to enable/disable on a TCP socket is duplicated in multiple
places in the code base, this commit replaces this duplicated code with
a helper in RedsStream.
---
 server/common-graphics-channel.c | 15 +++
 server/red-channel-client.c  | 17 ++---
 server/reds-stream.c | 16 
 server/reds-stream.h |  1 +
 server/reds.c|  8 +---
 server/sound.c   |  8 +---
 6 files changed, 24 insertions(+), 41 deletions(-)

Changes since v1:
- boolean style updates

diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
index bad6e8c..d04c509 100644
--- a/server/common-graphics-channel.c
+++ b/server/common-graphics-channel.c
@@ -19,9 +19,6 @@
 #endif
 
 #include 
-#include 
-#include 
-#include 
 
 #include "common-graphics-channel.h"
 #include "dcc.h"
@@ -117,24 +114,18 @@ int common_channel_config_socket(RedChannelClient *rcc)
 RedClient *client = red_channel_client_get_client(rcc);
 MainChannelClient *mcc = red_client_get_main(client);
 RedsStream *stream = red_channel_client_get_stream(rcc);
-int delay_val;
-gboolean is_low_bandwidth;
+bool is_low_bandwidth;
 
 // TODO - this should be dynamic, not one time at channel creation
 is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
-delay_val = is_low_bandwidth ? 0 : 1;
 /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
  * on the delayed ack timeout on the other side.
  * Instead of using Nagle's, we need to implement message buffering on
  * the application level.
  * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
  */
-if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, _val,
-   sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
-spice_warning("setsockopt failed, %s", strerror(errno));
-}
-}
+reds_stream_set_no_delay(stream, is_low_bandwidth);
+
 // TODO: move wide/narrow ack setting to red_channel.
 red_channel_client_ack_set_client_window(rcc,
 is_low_bandwidth ?
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index b583da2..ef52701 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -600,13 +600,7 @@ static void red_channel_client_send_ping(RedChannelClient 
*rcc)
 }  else {
 rcc->priv->latency_monitor.tcp_nodelay = delay_val;
 if (!delay_val) {
-delay_val = 1;
-if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, 
TCP_NODELAY, _val,
-   sizeof(delay_val)) == -1) {
-   if (errno != ENOTSUP) {
-spice_warning("setsockopt failed, %s", 
strerror(errno));
-}
-}
+reds_stream_set_no_delay(rcc->priv->stream, true);
 }
 }
 }
@@ -1430,14 +1424,7 @@ static void 
red_channel_client_handle_pong(RedChannelClient *rcc, SpiceMsgPing *
 
 /* set TCP_NODELAY=0, in case we reverted it for the test*/
 if (!rcc->priv->latency_monitor.tcp_nodelay) {
-int delay_val = 0;
-
-if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, TCP_NODELAY, 
_val,
-   sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
-spice_warning("setsockopt failed, %s", strerror(errno));
-}
-}
+reds_stream_set_no_delay(rcc->priv->stream, false);
 }
 
 /*
diff --git a/server/reds-stream.c b/server/reds-stream.c
index d0dadb9..2c6aaba 100644
--- a/server/reds-stream.c
+++ b/server/reds-stream.c
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -256,6 +257,21 @@ int reds_stream_is_plain_unix(const RedsStream *s)
 
 }
 
+bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay)
+{
+int delay_val = no_delay;
+
+if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
+   _val, sizeof(delay_val)) != 0) {
+if (errno != ENOTSUP && errno != ENOPROTOOPT) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+return false;
+}
+}
+
+return true;
+}
+
 int reds_stream_send_msgfd(RedsStream *stream, int fd)
 {
 struct msghdr msgh = { 0, };
diff --git a/server/reds-stream.h b/server/reds-stream.h
index a8d1736..c76014b 100644
--- a/server/reds-stream.h
+++ b/server/reds-stream.h
@@ -74,6 +74,7 @@ int reds_stream_enable_ssl(RedsStream *stream, SSL_CTX *ctx);
 void reds_stream_set_info_flag(RedsStream *stream, unsigned int flag);
 int reds_stream_get_family(const RedsStream *stream);
 int reds_stream_is_plain_unix(const RedsStream *stream);
+bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay);
 int reds_stream_send_msgfd(RedsStream *stream, int fd);
 
 typedef enum 

[Spice-devel] [PATCH spice-server v2] style: Change boolean style convention

[Frediano Ziglio]

Following
https://lists.freedesktop.org/archives/spice-devel/2017-February/035800.html
, the consensus was to use 'bool' rather than 'gboolean', and to use
true/false rather than TRUE/FALSE. New code should attempt to follow
the new style.

Signed-off-by: Frediano Ziglio 
---
 docs/spice_style.txt | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Changes since v1:
- update commit message (Christophe Fergeau)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 4614598..eb0e30e 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -47,10 +47,18 @@ Examples: +
   use `(var == 7)` instead of  `(7 == var)` +
   use `(function(var) > 7)` instead of `(7 < function(var))`
 
-TRUE, FALSE and NULL
+boolean type
+
+Where possible prefer the usage of `bool` type to store boolean.
+
+If not technically possible (ie libJPEG uses `boolean` while GLib uses 
`gboolean`) uses proper library type.
+
+Constants should be consistent with the type used so `true` and `false` for 
`bool` type.
+
+true, false and NULL
 
 
-Use `TRUE`, `FALSE` and `NULL` instead of 1 and 0 in order to improve code 
readability.
+Use `true`, `false` and `NULL` instead of 1 and 0 in order to improve code 
readability.
 
 Static storage initialization
 -
-- 
2.9.3

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

Re: [Spice-devel] [PATCH spice-server] style: Specify boolean style convention

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 07:04:30AM -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Feb 27, 2017 at 06:36:18AM -0500, Frediano Ziglio wrote:
> > > What about:
> > > 
> > > "style: Change boolean style convention
> > > 
> > > Was agreed to use C99 syntax for boolean.
> > > New code should attempt to follow the new style."
> > 
> > "Following
> > https://lists.freedesktop.org/archives/spice-devel/2017-February/035800.html
> > , the consensus was to use 'bool' rather than 'gboolean', and to use
> > true/false rather than TRUE/FALSE. New code should attempt to follow the
> > new style."
> > 
> > Christophe
> > 
> 
> "and to use true/false rather than TRUE/FALSE." is maybe misleading.
> Few places where it's another type should stay consistent with the type
> (unless I didn't understand).

Imo it's fine not to be 100% accurate in the commit log, the actual
change has all the details.

Christophe


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] Remove X == TRUE tests for non-boolean values

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 06:58:47AM -0500, Frediano Ziglio wrote:
> > 
> > Using X == TRUE is fragile, since the input value is a uint8_t. It would be
> > surprising if the value was set to 2 or 0xFF and the test failed.
> > 
> > Signed-off-by: Christophe de Dinechin 
> 
> Acked-by: Frediano Ziglio 

This apparently is going to cause warnings with gcc 7, see
https://www.redhat.com/archives/libvir-list/2017-February/msg01199.html

if (dcc->priv->surface_client_created[surface_id] != 0) {} should work,
or
diff --git a/server/dcc-private.h b/server/dcc-private.h
index 64b32a7..1cf3b0d 100644
--- a/server/dcc-private.h
+++ b/server/dcc-private.h
@@ -51,7 +51,7 @@ struct DisplayChannelClientPrivate
 int num_pixmap_cache_items;
 } send_data;

-uint8_t surface_client_created[NUM_SURFACES];
+bool surface_client_created[NUM_SURFACES];
 QRegion surface_client_lossy_region[NUM_SURFACES];

 StreamAgent stream_agents[NUM_STREAMS];

which seems to be a more accurate type for 'surface_client_created'.

Chistophe



> 
> > ---
> >  server/dcc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/dcc.c b/server/dcc.c
> > index afe37b1..7cfa72b 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -398,7 +398,7 @@ static void
> > add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
> >  
> >  surface_id = drawable->surface_deps[x];
> >  if (surface_id != -1) {
> > -if (dcc->priv->surface_client_created[surface_id] == TRUE) {
> > +if (dcc->priv->surface_client_created[surface_id]) {
> >  continue;
> >  }
> >  dcc_create_surface(dcc, surface_id);
> > @@ -407,7 +407,7 @@ static void
> > add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
> >  }
> >  }
> >  
> > -if (dcc->priv->surface_client_created[drawable->surface_id] == TRUE) {
> > +if (dcc->priv->surface_client_created[drawable->surface_id]) {
> >  return;
> >  }
> >  
> 
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 06:45:21AM -0500, Frediano Ziglio wrote:
> > 
> > Some debug messages were using spice_printerr(), which should be limited
> > to actual errors.
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  server/inputs-channel.c  |  4 ++--
> >  server/main-channel-client.c | 28 ++--
> >  server/main-channel.c| 10 +-
> >  server/red-channel-client.c  |  4 ++--
> >  server/red-client.c  |  4 ++--
> >  server/red-qxl.c | 12 ++--
> >  server/smartcard.c   |  2 +-
> >  server/spicevmc.c|  2 +-
> >  8 files changed, 33 insertions(+), 33 deletions(-)
> > 
> > diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> > index 3672d7e..89fb90e 100644
> > --- a/server/inputs-channel.c
> > +++ b/server/inputs-channel.c
> > @@ -516,7 +516,7 @@ static void inputs_connect(RedChannel *channel, 
> > RedClient
> > *client,
> >  "keyboard channel is insecure");
> >  }
> >  
> > -spice_printerr("inputs channel client create");
> > +spice_debug("inputs channel client create");
> >  rcc = inputs_channel_client_create(channel, client, stream, FALSE,
> > num_common_caps, common_caps,
> > num_caps, caps);
> 
> agreed
> 
> > @@ -714,7 +714,7 @@ int inputs_channel_has_tablet(InputsChannel *inputs)
> >  
> >  void inputs_channel_detach_tablet(InputsChannel *inputs, 
> > SpiceTabletInstance
> >  *tablet)
> >  {
> > -spice_printerr("");
> > +spice_debug("");
> >  inputs->tablet = NULL;
> >  }
> >  
> 
> I vote to remove

I prefer that this is done in a separate patch, would be a good
opportunity to take a look at the spice_debug(NULL) that we have (as
well as an odd-looking spice_debug("%s", "")).

Christophe


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-server] style: Specify boolean style convention

[Frediano Ziglio]

> 
> On Mon, Feb 27, 2017 at 06:36:18AM -0500, Frediano Ziglio wrote:
> > What about:
> > 
> > "style: Change boolean style convention
> > 
> > Was agreed to use C99 syntax for boolean.
> > New code should attempt to follow the new style."
> 
> "Following
> https://lists.freedesktop.org/archives/spice-devel/2017-February/035800.html
> , the consensus was to use 'bool' rather than 'gboolean', and to use
> true/false rather than TRUE/FALSE. New code should attempt to follow the
> new style."
> 
> Christophe
> 

"and to use true/false rather than TRUE/FALSE." is maybe misleading.
Few places where it's another type should stay consistent with the type
(unless I didn't understand).

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 12:44:27PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 27, 2017 at 06:39:14AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Wed, Feb 15, 2017 at 11:24:37AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > 
> > > > > > On 15 Feb 2017, at 12:45, Frediano Ziglio  
> > > > > > wrote:
> > > > > > 
> > > > > >> @@ -134,7 +134,7 @@ static void
> > > > > >> red_qxl_display_migrate(RedChannelClient
> > > > > >> *rcc)
> > > > > >> }
> > > > > >> g_object_get(channel, "channel-type", , "id", , NULL);
> > > > > >> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > > > > >> "dispatcher");
> > > > > >> -spice_printerr("channel type %u id %u", type, id);
> > > > > >> +spice_debug("channel type %u id %u", type, id);
> > > > > >> payload.rcc = rcc;
> > > > > >> dispatcher_send_message(dispatcher,
> > > > > >> RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> > > > > > 
> > > > > > Looks like there's lot of debugging on migration.
> > > > > > Like we are not really sure the code is working.
> > > > > 
> > > > > Maybe someone once needed a lot of debug information, and kept it 
> > > > > around
> > > > > in
> > > > > case things start going south. That probably means we have annotations
> > > > > that
> > > > > may end up being annoying for everybody debugging anything else than
> > > > > migration.
> > > > > 
> > > > 
> > > > I wrote that consideration thinking about customer reports.
> > > > spice_printerr goes in the logs while spice_debug not (by default).
> > > > If we have lot of bugs in migration removing from client logs could
> > > > became an issue for us. It's hard to ask to the customer to enable all
> > > > debugging (too invasive) and even if it could be enabled separately
> > > > it could be too late (as hard to reproduce for the customer) or
> > > > complicated (think about huge setups).
> > > > 
> > > > (CCing Jasa)
> > > 
> > > In my opinion, a library really should not be displaying anything on
> > > stdout by default, and as little as possible on stderr. "We print this
> > > debugging code to stderr because we want it to show up in the log by
> > > default" does not sound very compelling. Why this debugging code and
> > > not eg qxl display debugging code? Next step is suggesting to always
> > > output all debugging code to stderr, but then we'd get too much :)
> > > 
> > > I'd just silence all the debugging code, or remove it, and work on a way
> > > of making it easy to enable/filter/... if what we have now is not good
> > > enough.
> > > 
> > > Christophe
> > > 
> > 
> > Sure, however your comment is going OT.
> > My comments was specific to migration problems.
> > 
> 
> And I don't think debug for migration problems should be anything
> special. It has no good reason to go to stderr.

Agreed, libraries should never unconditionally dump debug output to either
stdout or stderr. This is particularly true if any of the debug logs are
possible to trigger by the guest OS, as it could be used to flood the host
logs. So I agree with Christophe that there should be an explicit opt-in
to turn on debugging output, either via an environment variable, or via
an API call QEMU can make.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] Remove X == TRUE tests for non-boolean values

[Christophe de Dinechin]

Using X == TRUE is fragile, since the input value is a uint8_t. It would be
surprising if the value was set to 2 or 0xFF and the test failed.

Signed-off-by: Christophe de Dinechin 
---
 server/dcc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index afe37b1..7cfa72b 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -398,7 +398,7 @@ static void 
add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
 
 surface_id = drawable->surface_deps[x];
 if (surface_id != -1) {
-if (dcc->priv->surface_client_created[surface_id] == TRUE) {
+if (dcc->priv->surface_client_created[surface_id]) {
 continue;
 }
 dcc_create_surface(dcc, surface_id);
@@ -407,7 +407,7 @@ static void 
add_drawable_surface_images(DisplayChannelClient *dcc, Drawable *dra
 }
 }
 
-if (dcc->priv->surface_client_created[drawable->surface_id] == TRUE) {
+if (dcc->priv->surface_client_created[drawable->surface_id]) {
 return;
 }
 
-- 
2.10.1 (Apple Git-78)

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

Re: [Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 06:39:14AM -0500, Frediano Ziglio wrote:
> > 
> > On Wed, Feb 15, 2017 at 11:24:37AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > 
> > > > > On 15 Feb 2017, at 12:45, Frediano Ziglio  wrote:
> > > > > 
> > > > >> @@ -134,7 +134,7 @@ static void
> > > > >> red_qxl_display_migrate(RedChannelClient
> > > > >> *rcc)
> > > > >> }
> > > > >> g_object_get(channel, "channel-type", , "id", , NULL);
> > > > >> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > > > >> "dispatcher");
> > > > >> -spice_printerr("channel type %u id %u", type, id);
> > > > >> +spice_debug("channel type %u id %u", type, id);
> > > > >> payload.rcc = rcc;
> > > > >> dispatcher_send_message(dispatcher,
> > > > >> RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> > > > > 
> > > > > Looks like there's lot of debugging on migration.
> > > > > Like we are not really sure the code is working.
> > > > 
> > > > Maybe someone once needed a lot of debug information, and kept it around
> > > > in
> > > > case things start going south. That probably means we have annotations
> > > > that
> > > > may end up being annoying for everybody debugging anything else than
> > > > migration.
> > > > 
> > > 
> > > I wrote that consideration thinking about customer reports.
> > > spice_printerr goes in the logs while spice_debug not (by default).
> > > If we have lot of bugs in migration removing from client logs could
> > > became an issue for us. It's hard to ask to the customer to enable all
> > > debugging (too invasive) and even if it could be enabled separately
> > > it could be too late (as hard to reproduce for the customer) or
> > > complicated (think about huge setups).
> > > 
> > > (CCing Jasa)
> > 
> > In my opinion, a library really should not be displaying anything on
> > stdout by default, and as little as possible on stderr. "We print this
> > debugging code to stderr because we want it to show up in the log by
> > default" does not sound very compelling. Why this debugging code and
> > not eg qxl display debugging code? Next step is suggesting to always
> > output all debugging code to stderr, but then we'd get too much :)
> > 
> > I'd just silence all the debugging code, or remove it, and work on a way
> > of making it easy to enable/filter/... if what we have now is not good
> > enough.
> > 
> > Christophe
> > 
> 
> Sure, however your comment is going OT.
> My comments was specific to migration problems.
> 

And I don't think debug for migration problems should be anything
special. It has no good reason to go to stderr.

Christophe


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-server] style: Specify boolean style convention

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 06:36:18AM -0500, Frediano Ziglio wrote:
> What about:
> 
> "style: Change boolean style convention
> 
> Was agreed to use C99 syntax for boolean.
> New code should attempt to follow the new style."

"Following
https://lists.freedesktop.org/archives/spice-devel/2017-February/035800.html
, the consensus was to use 'bool' rather than 'gboolean', and to use
true/false rather than TRUE/FALSE. New code should attempt to follow the
new style."

Christophe


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-server] style: Group together indentation sections

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Thu, Feb 23, 2017 at 05:37:53PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  docs/spice_style.txt | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 99c8390..eb0e30e 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -175,9 +175,12 @@ for (i = 0; i < 10; ++i) {
>  some_func(var, i * i + *ptr, , sizeof(var));
>  }
>  
> +Indentation
> +---
> +
>  [[function_indentation]]
>  Function Indentation
> -
> +
>  
>  * No spaces between function name to left bracket.
>  * Curly bracket start on new line.
> @@ -216,7 +219,7 @@ function(type1 arg1, type2 arg2,
>  
>  
>  Branching indentation
> --
> +~
>  
>  * Add space after a branch keyword and after the right bracket.
>  * Curly bracket starts on the same line the branch starts.
> @@ -283,7 +286,7 @@ default:
>  
>  
>  Types indentation
> --
> +~
>  
>  [source,c]
>  
> @@ -309,7 +312,7 @@ union {
>  
>  
>  Vertical indentation
> -
> +
>  
>  Use one space no tabs and no vertical alignment.
>  [source,c]
> @@ -324,7 +327,7 @@ unsigned long f2(int a, char ch);
>  
>  
>  Multi line macro indentation
> -
> +
>  
>  [source,c]
>  #define MACRO_NAME(a, b, c) {\
> @@ -335,7 +338,7 @@ Multi line macro indentation
>  }
>  
>  Multi line array indentation
> -
> +
>  
>  [source,c]
>  char *array[] = {
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Frediano Ziglio]

> 
> On Wed, Feb 15, 2017 at 11:24:37AM -0500, Frediano Ziglio wrote:
> > > 
> > > 
> > > > On 15 Feb 2017, at 12:45, Frediano Ziglio  wrote:
> > > > 
> > > >> @@ -134,7 +134,7 @@ static void
> > > >> red_qxl_display_migrate(RedChannelClient
> > > >> *rcc)
> > > >> }
> > > >> g_object_get(channel, "channel-type", , "id", , NULL);
> > > >> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > > >> "dispatcher");
> > > >> -spice_printerr("channel type %u id %u", type, id);
> > > >> +spice_debug("channel type %u id %u", type, id);
> > > >> payload.rcc = rcc;
> > > >> dispatcher_send_message(dispatcher,
> > > >> RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> > > > 
> > > > Looks like there's lot of debugging on migration.
> > > > Like we are not really sure the code is working.
> > > 
> > > Maybe someone once needed a lot of debug information, and kept it around
> > > in
> > > case things start going south. That probably means we have annotations
> > > that
> > > may end up being annoying for everybody debugging anything else than
> > > migration.
> > > 
> > 
> > I wrote that consideration thinking about customer reports.
> > spice_printerr goes in the logs while spice_debug not (by default).
> > If we have lot of bugs in migration removing from client logs could
> > became an issue for us. It's hard to ask to the customer to enable all
> > debugging (too invasive) and even if it could be enabled separately
> > it could be too late (as hard to reproduce for the customer) or
> > complicated (think about huge setups).
> > 
> > (CCing Jasa)
> 
> In my opinion, a library really should not be displaying anything on
> stdout by default, and as little as possible on stderr. "We print this
> debugging code to stderr because we want it to show up in the log by
> default" does not sound very compelling. Why this debugging code and
> not eg qxl display debugging code? Next step is suggesting to always
> output all debugging code to stderr, but then we'd get too much :)
> 
> I'd just silence all the debugging code, or remove it, and work on a way
> of making it easy to enable/filter/... if what we have now is not good
> enough.
> 
> Christophe
> 

Sure, however your comment is going OT.
My comments was specific to migration problems.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] style: Specify boolean style convention

[Frediano Ziglio]

What about:

"style: Change boolean style convention

Was agreed to use C99 syntax for boolean.
New code should attempt to follow the new style."

Frediano

> 
> Maybe add a note that this *changes* existing conventions.
> Not sure it's worth doing a global s/TRUE/true though.
> Looks good to me.
> 
> Christophe
> 
> On Thu, Feb 23, 2017 at 05:37:52PM +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  docs/spice_style.txt | 12 ++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > index cd874df..99c8390 100644
> > --- a/docs/spice_style.txt
> > +++ b/docs/spice_style.txt
> > @@ -47,10 +47,18 @@ Examples: +
> >use `(var == 7)` instead of  `(7 == var)` +
> >use `(function(var) > 7)` instead of `(7 < function(var))`
> >  
> > -TRUE, FALSE and NULL
> > +boolean type
> > +
> > +Where possible prefer the usage of `bool` type to store boolean.
> > +
> > +If not technically possible (ie libJPEG uses `boolean` while GLib uses
> > `gboolean`) uses proper library type.
> > +
> > +Constants should be consistent with the type used so `true` and `false`
> > for `bool` type.
> > +
> > +true, false and NULL
> >  
> >  
> > -Use `TRUE`, `FALSE` and `NULL` instead of 1 and 0 in order to improve code
> > readability.
> > +Use `true`, `false` and `NULL` instead of 1 and 0 in order to improve code
> > readability.
> >  
> >  Static storage initialization
> >  -
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 11:24:37AM -0500, Frediano Ziglio wrote:
> > 
> > 
> > > On 15 Feb 2017, at 12:45, Frediano Ziglio  wrote:
> > > 
> > >> @@ -134,7 +134,7 @@ static void red_qxl_display_migrate(RedChannelClient
> > >> *rcc)
> > >> }
> > >> g_object_get(channel, "channel-type", , "id", , NULL);
> > >> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > >> "dispatcher");
> > >> -spice_printerr("channel type %u id %u", type, id);
> > >> +spice_debug("channel type %u id %u", type, id);
> > >> payload.rcc = rcc;
> > >> dispatcher_send_message(dispatcher,
> > >> RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> > > 
> > > Looks like there's lot of debugging on migration.
> > > Like we are not really sure the code is working.
> > 
> > Maybe someone once needed a lot of debug information, and kept it around in
> > case things start going south. That probably means we have annotations that
> > may end up being annoying for everybody debugging anything else than
> > migration.
> > 
> 
> I wrote that consideration thinking about customer reports.
> spice_printerr goes in the logs while spice_debug not (by default).
> If we have lot of bugs in migration removing from client logs could
> became an issue for us. It's hard to ask to the customer to enable all
> debugging (too invasive) and even if it could be enabled separately
> it could be too late (as hard to reproduce for the customer) or
> complicated (think about huge setups).
> 
> (CCing Jasa)

In my opinion, a library really should not be displaying anything on
stdout by default, and as little as possible on stderr. "We print this
debugging code to stderr because we want it to show up in the log by
default" does not sound very compelling. Why this debugging code and
not eg qxl display debugging code? Next step is suggesting to always
output all debugging code to stderr, but then we'd get too much :)

I'd just silence all the debugging code, or remove it, and work on a way
of making it easy to enable/filter/... if what we have now is not good
enough.

Christophe


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-server] style: Specify boolean style convention

[Christophe Fergeau]

Maybe add a note that this *changes* existing conventions.
Not sure it's worth doing a global s/TRUE/true though.
Looks good to me.

Christophe

On Thu, Feb 23, 2017 at 05:37:52PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  docs/spice_style.txt | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index cd874df..99c8390 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -47,10 +47,18 @@ Examples: +
>use `(var == 7)` instead of  `(7 == var)` +
>use `(function(var) > 7)` instead of `(7 < function(var))`
>  
> -TRUE, FALSE and NULL
> +boolean type
> +
> +Where possible prefer the usage of `bool` type to store boolean.
> +
> +If not technically possible (ie libJPEG uses `boolean` while GLib uses 
> `gboolean`) uses proper library type.
> +
> +Constants should be consistent with the type used so `true` and `false` for 
> `bool` type.
> +
> +true, false and NULL
>  
>  
> -Use `TRUE`, `FALSE` and `NULL` instead of 1 and 0 in order to improve code 
> readability.
> +Use `true`, `false` and `NULL` instead of 1 and 0 in order to improve code 
> readability.
>  
>  Static storage initialization
>  -
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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-server 3/4] reds-stream: Introduce reds_stream_set_no_delay() helper

[Christophe Fergeau]

So what is the verdict here? Is everyone fine with keeping consistency
within reds-stream.[ch] which use bool and TRUE/FALSE, or do you want
something different?

Christophe

On Wed, Feb 15, 2017 at 04:04:16PM +0100, Christophe Fergeau wrote:
> On Wed, Feb 15, 2017 at 03:23:36PM +0100, Pavel Grunt wrote:
> > On Wed, 2017-02-15 at 15:16 +0100, Christophe Fergeau wrote:
> > > On Wed, Feb 15, 2017 at 09:14:10AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > On Wed, Feb 15, 2017 at 06:39:13AM -0500, Frediano Ziglio wrote:
> > > > > > 
> > > > > > Patch is good however the use of bool is inconsistent.
> > > > > > Although I _much_ prefer bool/true/false we decided for
> > > > > > gboolean/TRUE/FALSE.
> > > > > 
> > > > > reds-stream.[ch] is already exclusively using bool instead of
> > > > > gboolean,
> > > > > so I'd prefer to stick with this. I have a patch (series) in
> > > > > preparation
> > > > > changing gboolean to bool (with justification), so I would not
> > > > > change
> > > > > reds-stream.[ch] just yet.
> > > > > 
> > > > > Christophe
> > > > > 
> > > > 
> > > > Can you be at least consistent in server/reds-stream.c ?
> > > > 
> > > > bool -> true/false
> > > > 
> > > 
> > > Existing code already uses bool together with TRUE/FALSE, so I would
> > > keep the discussion about TRUE vs true for another time as well ;)
> > > 
> > 
> > I am against mixing, TRUE is gboolean, true is bool
> > 
> 
> I agree it's odd, but this is what reds-stream.c is already doing, I'd
> prefer to keep things consistent for now, even though it's weird.
> 
> > imo it is simple: spice server requires C99 => we have stdbool => we
> > can use it if it has benefits
> 
> I'll send a patch series related to that, imo we talk about that in
> relation with that new series.
> 
> Christophe



> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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] [spice-gtk v1 0/7] clipboard_get_targets() on spice-gtk-session.c

[Victor Toso]

Hi,

On Wed, Feb 22, 2017 at 02:11:05PM +0100, Victor Toso wrote:
> From: Victor Toso 
>
> Hi,
>
> Minor changes that I was doing while playing with clipboard.
> The first one seems important the others might be somewhat an
> improvement or not. Let me know ;)
>
> Cheers,
>
> Victor Toso (7):
>   gtk-session: check if retrieving clipboard data failed
>   gtk-session: initialize array without memset

JFYI, I pushed the first two patches.

>   gtk-session: use clear variable for array's size
>   gtk-session: do an early check of clipboard grab
>   gtk-session: move atom's debug
>   gtk-session: better debug GdkAtoms that are parsed
>   gtk-session: move variables to internal scope
> 
>  src/spice-gtk-session.c | 55 
> -
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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-gtk v3] Switch over to using keycodemapdb submodule

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 11:37:44AM +0100, Pavel Grunt wrote:
> Hello Daniel,
> 
> On Mon, 2017-02-27 at 10:25 +, Daniel P. Berrange wrote:
> > Consume the keymaps.csv file from a git submodule instead of having
> > a private copy. This makes it easier to ensure all users of the
> > keymap
> > (libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a
> > consistent
> > set of data.
> > 
> besides that it also allow us to drop the dependency on perl (also
> perl-Text-CSV is not packaged in some distros)

True yes, I intentionally kept the python code so that it only
used modules base-python installs so we don't rely on external
modules from pypi.

> Are there contributing rules for the keycodemapdb (where to send the
> patches etc.)?

Just send pull requests to the repo is best I think. I don't think we'd
have enough traffic to warrant creating a new mailing list. In any
case I would expect that most bug reports would start off with a mail
and/or bug report to a project using the module. eg a mail on spice-devel,
so there's little point trying to artificially move discussion to a
dedicated list.

I'm happy to add any of the people with experiance of this code to the
admins/committers list of the gitlab project too, so I'm not a potential
bottleneck.

> (For the future) Do you consider adding the vncdisplaykeymap.[ch] to
> the repo ?

I'm unsure of the direction to take for that at this time. It would
certainly be interesting to look at sharing that logic. For sharing
code though, I wonder if its better to create a libgtkkeycodemap.so
library rather than do a sub-module thing for that code too.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3] Switch over to using keycodemapdb submodule

[Pavel Grunt]

Hello Daniel,

On Mon, 2017-02-27 at 10:25 +, Daniel P. Berrange wrote:
> Consume the keymaps.csv file from a git submodule instead of having
> a private copy. This makes it easier to ensure all users of the
> keymap
> (libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a
> consistent
> set of data.
> 
besides that it also allow us to drop the dependency on perl (also
perl-Text-CSV is not packaged in some distros)

Are there contributing rules for the keycodemapdb (where to send the
patches etc.)?

(For the future) Do you consider adding the vncdisplaykeymap.[ch] to
the repo ?

Thanks,
Pavel


> Signed-off-by: Daniel P. Berrange 
> ---
> 
> Changed in v3:
> 
>  - Fix makefile rules to find generator script in VPATH build
> 
>  .gitmodules   |   3 +
>  configure.ac  |  11 --
>  src/Makefile.am   |  30 ++--
>  src/keycodemapdb  |   1 +
>  src/keymap-gen.pl | 214 ---
>  src/keymaps.csv   | 511 -
> -
>  6 files changed, 17 insertions(+), 753 deletions(-)
>  create mode 16 src/keycodemapdb
>  delete mode 100755 src/keymap-gen.pl
>  delete mode 100644 src/keymaps.csv
> 
> diff --git a/.gitmodules b/.gitmodules
> index 0c618ee..82467e4 100644
> --- a/.gitmodules
> +++ b/.gitmodules
> @@ -1,3 +1,6 @@
>  [submodule "spice-common"]
>   path = spice-common
>   url = ../spice-common
> +[submodule "src/keycodemapdb"]
> + path = src/keycodemapdb
> + url = https://gitlab.com/keycodemap/keycodemapdb.git
> diff --git a/configure.ac b/configure.ac
> index 463fbe0..763d14b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
>  dnl
> 
> =
>  dnl Chek optional features
>  
> -srcdir="$(dirname $0)"
> -if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
> -  AC_MSG_CHECKING([for Text::CSV Perl module])
> -  perl -MText::CSV -e "" >/dev/null 2>&1
> -  if test $? -ne 0 ; then
> -AC_MSG_RESULT([not found])
> -AC_MSG_ERROR([Text::CSV Perl module is required to compile this
> package])
> -  fi
> -  AC_MSG_RESULT([found])
> -fi
> -
>  SPICE_GLIB_REQUIRES=""
>  SPICE_GTK_REQUIRES=""
>  
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7542fac..594c0de 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -26,14 +26,13 @@ GLIBGENS =
> \
>   spice-widget-enums.h\
>   $(NULL)
>  
> -CLEANFILES = $(GLIBGENS)
> +CLEANFILES = $(GLIBGENS) $(KEYMAPS)
>  BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
>  
>  EXTRA_DIST = \
> - $(KEYMAPS)  \
>   decode-glz-tmpl.c   \
> - keymap-gen.pl   \
> - keymaps.csv \
> + $(KEYMAP_CSV)   \
> + $(KEYMAP_GEN)   \
>   map-file\
>   spice-glib-sym-file \
>   spice-gtk-sym-file  \
> @@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols
> ${srcdir}/spice-gtk-sym-file
>  GTK_SYMBOLS_FILE = spice-gtk-sym-file
>  endif
>  
> -KEYMAP_GEN = $(srcdir)/keymap-gen.pl
> +KEYMAP_GEN = keycodemapdb/tools/keymap-gen
> +KEYMAP_CSV = keycodemapdb/data/keymaps.csv
>  
>  SPICE_COMMON_CPPFLAGS =  
> \
>   -DSPICE_COMPILATION 
> \
> @@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
>  
>  
>  vncdisplaykeymap.c: $(KEYMAPS)
> +$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
>  
> -$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
> -
> -# Note despite being autogenerated these are not part of
> CLEANFILES, they
> -# are actually a part of EXTRA_DIST to avoid the need for
> perl(Text::CSV) by
> -# end users
>  vncdisplaykeymap_xorgevdev2xtkbd.c:
> - $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev
> xtkbd > $@ || rm $@
> + $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2
> --varname keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV)
> xorgevdev xtkbd > $@ || rm $@
>  
>  vncdisplaykeymap_xorgkbd2xtkbd.c:
> - $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd
> xtkbd > $@ || rm $@
> + $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2
> --varname keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV)
> xorgkbd xtkbd > $@ || rm $@
>  
>  vncdisplaykeymap_xorgxquartz2xtkbd.c:
> - $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz
> xtkbd > $@ || rm $@
> + $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2
> --varname keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV)
> xorgxquartz xtkbd > $@ || rm $@
>  
>  vncdisplaykeymap_xorgxwin2xtkbd.c:
> - $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin
> xtkbd > $@ || rm $@
> + 

[Spice-devel] [PATCH spice-gtk v3] Switch over to using keycodemapdb submodule

[Daniel P. Berrange]

Consume the keymaps.csv file from a git submodule instead of having
a private copy. This makes it easier to ensure all users of the keymap
(libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a consistent
set of data.

Signed-off-by: Daniel P. Berrange 
---

Changed in v3:

 - Fix makefile rules to find generator script in VPATH build

 .gitmodules   |   3 +
 configure.ac  |  11 --
 src/Makefile.am   |  30 ++--
 src/keycodemapdb  |   1 +
 src/keymap-gen.pl | 214 ---
 src/keymaps.csv   | 511 --
 6 files changed, 17 insertions(+), 753 deletions(-)
 create mode 16 src/keycodemapdb
 delete mode 100755 src/keymap-gen.pl
 delete mode 100644 src/keymaps.csv

diff --git a/.gitmodules b/.gitmodules
index 0c618ee..82467e4 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "spice-common"]
path = spice-common
url = ../spice-common
+[submodule "src/keycodemapdb"]
+   path = src/keycodemapdb
+   url = https://gitlab.com/keycodemap/keycodemapdb.git
diff --git a/configure.ac b/configure.ac
index 463fbe0..763d14b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
 dnl =
 dnl Chek optional features
 
-srcdir="$(dirname $0)"
-if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
-  AC_MSG_CHECKING([for Text::CSV Perl module])
-  perl -MText::CSV -e "" >/dev/null 2>&1
-  if test $? -ne 0 ; then
-AC_MSG_RESULT([not found])
-AC_MSG_ERROR([Text::CSV Perl module is required to compile this package])
-  fi
-  AC_MSG_RESULT([found])
-fi
-
 SPICE_GLIB_REQUIRES=""
 SPICE_GTK_REQUIRES=""
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 7542fac..594c0de 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,14 +26,13 @@ GLIBGENS =  \
spice-widget-enums.h\
$(NULL)
 
-CLEANFILES = $(GLIBGENS)
+CLEANFILES = $(GLIBGENS) $(KEYMAPS)
 BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
 
 EXTRA_DIST =   \
-   $(KEYMAPS)  \
decode-glz-tmpl.c   \
-   keymap-gen.pl   \
-   keymaps.csv \
+   $(KEYMAP_CSV)   \
+   $(KEYMAP_GEN)   \
map-file\
spice-glib-sym-file \
spice-gtk-sym-file  \
@@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols 
${srcdir}/spice-gtk-sym-file
 GTK_SYMBOLS_FILE = spice-gtk-sym-file
 endif
 
-KEYMAP_GEN = $(srcdir)/keymap-gen.pl
+KEYMAP_GEN = keycodemapdb/tools/keymap-gen
+KEYMAP_CSV = keycodemapdb/data/keymaps.csv
 
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
@@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
 
 
 vncdisplaykeymap.c: $(KEYMAPS)
+$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
 
-$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
-
-# Note despite being autogenerated these are not part of CLEANFILES, they
-# are actually a part of EXTRA_DIST to avoid the need for perl(Text::CSV) by
-# end users
 vncdisplaykeymap_xorgevdev2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgevdev xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_xorgkbd2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd xtkbd > $@ || rm 
$@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgkbd xtkbd > $@ || rm 
$@
 
 vncdisplaykeymap_xorgxquartz2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz xtkbd > $@ 
|| rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxquartz xtkbd > 
$@ || rm $@
 
 vncdisplaykeymap_xorgxwin2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxwin2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxwin xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_osx2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv osx xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_osx2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) osx xtkbd > $@ || rm $@
 
 vncdisplaykeymap_win322xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv win32 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 

[Spice-devel] [spice v9] dcc: handle preferred video codec message

[Victor Toso]

From: Victor Toso 

[0] SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE

This message provides a list of video codecs based on client's order
of preference.

We duplicate the video codecs array from reds.c and sort it using the
order of codecs as reference.

This message will not change an ongoing streaming but it could change
newly created streams depending the rank value of each video codec
that can be set by spice_server_set_video_codecs()

Signed-off-by: Victor Toso 
---
 server/dcc-private.h |   5 ++
 server/dcc.c | 132 +++
 server/dcc.h |   1 +
 server/display-channel.c |   2 +
 server/stream.c  |   3 +-
 5 files changed, 141 insertions(+), 2 deletions(-)

diff --git a/server/dcc-private.h b/server/dcc-private.h
index 64b32a7..bf69fbb 100644
--- a/server/dcc-private.h
+++ b/server/dcc-private.h
@@ -51,6 +51,11 @@ struct DisplayChannelClientPrivate
 int num_pixmap_cache_items;
 } send_data;
 
+/* Host preferred video-codec order sorted with client preferred */
+GArray *preferred_video_codecs;
+/* Last client preferred video-codec order */
+GArray *client_preferred_video_codecs;
+
 uint8_t surface_client_created[NUM_SURFACES];
 QRegion surface_client_lossy_region[NUM_SURFACES];
 
diff --git a/server/dcc.c b/server/dcc.c
index cf9431a..476f43d 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -39,6 +39,8 @@ enum
 PROP_ZLIB_GLZ_STATE
 };
 
+static void on_display_video_codecs_update(GObject *gobject, GParamSpec 
*pspec, gpointer user_data);
+
 static void
 display_channel_client_get_property(GObject *object,
 guint property_id,
@@ -99,12 +101,19 @@ display_channel_client_constructed(GObject *object)
 dcc_init_stream_agents(self);
 
 image_encoders_init(>priv->encoders, 
_TO_DC(self)->priv->encoder_shared_data);
+
+g_signal_connect(DCC_TO_DC(self), "notify::video-codecs",
+ G_CALLBACK(on_display_video_codecs_update), self);
 }
 
 static void
 display_channel_client_finalize(GObject *object)
 {
 DisplayChannelClient *self = DISPLAY_CHANNEL_CLIENT(object);
+
+g_signal_handlers_disconnect_by_func(DCC_TO_DC(self), 
on_display_video_codecs_update, self);
+g_clear_pointer(>priv->preferred_video_codecs, g_array_unref);
+g_clear_pointer(>priv->client_preferred_video_codecs, g_array_unref);
 g_free(self->priv);
 
 G_OBJECT_CLASS(display_channel_client_parent_class)->finalize(object);
@@ -1105,6 +1114,126 @@ static int 
dcc_handle_preferred_compression(DisplayChannelClient *dcc,
 return TRUE;
 }
 
+
+/* TODO: Client preference should only be considered when host has video-codecs
+ * with the same priority value. At the moment, the video-codec GArray will be
+ * sorted following only the client's preference (@user_data)
+ *
+ * example:
+ * host encoding preference: gstreamer:mjpeg;gstreamer:vp8;gstreamer:h264
+ * client decoding preference: h264, vp9, mjpeg
+ * result: gstreamer:h264;gstreamer:mjpeg;gstreamer:vp8
+ */
+static gint sort_video_codecs_by_client_preference(gconstpointer a_pointer,
+   gconstpointer b_pointer,
+   gpointer user_data)
+{
+const RedVideoCodec *a = a_pointer;
+const RedVideoCodec *b = b_pointer;
+
+if (a->type != b->type) {
+guint i;
+GArray *client_pref = user_data;
+
+for (i = 0; i < client_pref->len; i++) {
+if (a->type == g_array_index(client_pref, gint, i)) {
+return -1;
+}
+if (b->type == g_array_index(client_pref, gint, i)) {
+return 1;
+}
+}
+}
+
+return 0;
+}
+
+static void dcc_update_preferred_video_codecs(DisplayChannelClient *dcc)
+{
+guint i;
+GArray *video_codecs, *server_codecs;
+GString *msg;
+
+server_codecs = display_channel_get_video_codecs(DCC_TO_DC(dcc));
+spice_return_if_fail(server_codecs != NULL);
+
+/* Copy current host preference */
+video_codecs = g_array_sized_new(FALSE, FALSE, sizeof(RedVideoCodec), 
server_codecs->len);
+g_array_append_vals(video_codecs, server_codecs->data, server_codecs->len);
+
+/* Sort the copy of current host preference based on client's preference */
+g_array_sort_with_data(video_codecs, 
sort_video_codecs_by_client_preference,
+   dcc->priv->client_preferred_video_codecs);
+g_clear_pointer(>priv->preferred_video_codecs, g_array_unref);
+dcc->priv->preferred_video_codecs = video_codecs;
+
+msg = g_string_new("Preferred video-codecs:");
+for (i = 0; i < video_codecs->len; i++) {
+RedVideoCodec codec = g_array_index(video_codecs, RedVideoCodec, i);
+g_string_append_printf(msg, " %d", codec.type);
+}
+spice_info("%s", msg->str);
+g_string_free(msg, TRUE);
+}

[Spice-devel] [spice v9] Preferred video codec message (27 Feb)

[Victor Toso]

From: Victor Toso 

Hi,

Finally a new version with rework from previous iteration.
To test it easily, I'm using this branch [0] that introduces a streaming
information in spicy tool (no streaming it sets to None, with vp8 stream
it sets it to "vp8", and so on)

[0]  https://gitlab.com/victortoso/spice-gtk/commits/spicy-tools-label

changes between v8->v9:
* Server might drop the message with newer clients (Pavel)
 - If a new client has a video-codec that server might not know, we were
   dropping the message. Now we are just ignoring it.

* Handling duplicated cases (Frediano)
 - Instead of doing a check on the size of the array, we check if given
   video-codec has been included already.

* Fix the sort (Christophe de Dinechin)
 - Although the order of video-codec was alright it could be the case
   that we could change the encoder order. This is somewhat a minor
   corner case as the only situation we can have this is with
   spice:mjpeg and gstreamer:mjpeg

* Use spice_debug instead of spice_info (Frediano)
* Fixed a few typos (Frediano)
* Code style! Always use brackets (Frediano)
* Use g_array_sized_new() where it fits (Christophe de Dinechin)
* Use g_array_append_vals() where it fits (Christophe de Dinechin)

Victor Toso (1):
  dcc: handle preferred video codec message

 server/dcc-private.h |   5 ++
 server/dcc.c | 132 +++
 server/dcc.h |   1 +
 server/display-channel.c |   2 +
 server/stream.c  |   3 +-
 5 files changed, 141 insertions(+), 2 deletions(-)

-- 
2.9.3

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

Re: [Spice-devel] [client] spicy: Spicy uses GTK+ and must link with it

[Francois Gouget]

On Mon, 27 Feb 2017, Christophe Fergeau wrote:

> On Mon, Feb 27, 2017 at 09:19:15AM +0100, Christophe Fergeau wrote:
> > However, spicy-stats uses glib/gobject, and does not explicitly link
> > with it either, I would have expected something like the patch below, but
> > this can actually be done on top of your patch.
> 
> Ah, never mind, just saw your other patch now.

Well, your patch has a lot more (for spicy-screenshot for instance). I'm  
fine with either I guess.


-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client] spicy: Spicy uses GTK+ and must link with it

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 09:25:35AM +0100, Victor Toso wrote:
> Hi,
> 
> On Mon, Feb 27, 2017 at 09:19:15AM +0100, Christophe Fergeau wrote:
> > On Fri, Feb 24, 2017 at 12:23:23PM +0100, Francois Gouget wrote:
> > > This avoids having the linker complain that gtk_toggle_action_get_type()
> > > or some other GTK+ API is undefined.
> >
> > This kind of issues are indeed seen on some distros, and not others.
> > I believe this is what is described on
> > https://wiki.mageia.org/en/Underlinking_issues_in_packaging
> 
> Ah, thanks for the link above.

https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/
has a nice explanation as well, but this did not help me either to hit
the linking issue ;)

I've pushed the 2 patches for now.

Christophe


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] [client] spicy-stats: spicy-stats uses libgobject so link with it

[Christophe Fergeau]

On Fri, Feb 24, 2017 at 12:26:42PM +0100, Francois Gouget wrote:
> This avoids having the linker complain that g_signal_connect_data() or
> some other GObject API is undefined.

Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Francois Gouget 
> ---
>  tools/Makefile.am | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index e41e0ab5..c80d34ae 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -61,6 +61,7 @@ spicy_stats_SOURCES =   \
>  
>  spicy_stats_LDADD =  \
>   $(top_builddir)/src/libspice-client-glib-2.0.la \
> + $(GOBJECT2_LIBS) \
>   $(NULL)
>  
>  spicy_stats_CPPFLAGS =   \
> -- 
> 2.11.0
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] [client] spicy: Spicy uses GTK+ and must link with it

[Christophe Fergeau]

On Mon, Feb 27, 2017 at 09:19:15AM +0100, Christophe Fergeau wrote:
> However, spicy-stats uses glib/gobject, and does not explicitly link
> with it either, I would have expected something like the patch below, but
> this can actually be done on top of your patch.

Ah, never mind, just saw your other patch now.

Christophe


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] [client] spicy: Spicy uses GTK+ and must link with it

[Victor Toso]

Hi,

On Mon, Feb 27, 2017 at 09:19:15AM +0100, Christophe Fergeau wrote:
> On Fri, Feb 24, 2017 at 12:23:23PM +0100, Francois Gouget wrote:
> > This avoids having the linker complain that gtk_toggle_action_get_type()
> > or some other GTK+ API is undefined.
>
> This kind of issues are indeed seen on some distros, and not others.
> I believe this is what is described on
> https://wiki.mageia.org/en/Underlinking_issues_in_packaging

Ah, thanks for the link above.

>
> spicy directly uses symbols from gtk+, so it should explicitly link
> with it. This wiki page says this should be reproducible using
> -Wl,--as-needed, but this did not make a difference for me as -lgtk-3
> is listed in libspice-client-gtk-3.0.la, and libtool uses this when
> linking spicy.

Here as well.

> However, spicy-stats uses glib/gobject, and does not explicitly link
> with it either, I would have expected something like the patch below, but
> this can actually be done on top of your patch.
>
> Acked-by: Christophe Fergeau 
> even though I would have liked to be able to reproduce the issue :-/

Me too. :(

toso
>
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 0bdb3c5..0b3df78 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -12,6 +12,12 @@ TOOLS_CPPFLAGS = \
> $(SPICE_CFLAGS) \
> $(NULL)
> 
> +TOOLS_LIBS =   \
> +   $(GLIB2_LIBS)   \
> +   $(GOBJECT2_LIBS)\
> +   $(top_builddir)/src/libspice-client-glib-2.0.la \
> +   $(NULL)
> +
>  if WITH_GTK
>  bin_PROGRAMS += spicy
>  TOOLS_CPPFLAGS += $(GTK_CFLAGS)
> @@ -26,8 +32,9 @@ spicy_SOURCES =   \
> $(NULL)
> 
>  spicy_LDADD =  \
> +   $(TOOLS_LIBS)   \
> +   $(GTK_LIBS) \
> $(top_builddir)/src/libspice-client-gtk-3.0.la  \
> -   $(top_builddir)/src/libspice-client-glib-2.0.la \
> $(NULL)
> 
>  # FIXME: GtkAction and lots of GtkUIManager APIs are deprecated
> @@ -44,8 +51,7 @@ spicy_screenshot_SOURCES =\
> $(NULL)
> 
>  spicy_screenshot_LDADD =   \
> -   $(top_builddir)/src/libspice-client-glib-2.0.la \
> -   $(GOBJECT2_LIBS)\
> +   $(TOOLS_LIBS)   \
> $(NULL)
> 
>  spicy_screenshot_CPPFLAGS =\
> @@ -59,7 +65,7 @@ spicy_stats_SOURCES = \
> $(NULL)
> 
>  spicy_stats_LDADD =\
> -   $(top_builddir)/src/libspice-client-glib-2.0.la \
> +   $(TOOLS_LIBS)   \
> $(NULL)
> 
>  spicy_stats_CPPFLAGS = \
> 
> 
> Christophe
> 
> 
> > 
> > Signed-off-by: Francois Gouget 
> > ---
> >  tools/Makefile.am | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index 0bdb3c54..e41e0ab5 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -28,6 +28,7 @@ spicy_SOURCES =   \
> >  spicy_LDADD =  \
> > $(top_builddir)/src/libspice-client-gtk-3.0.la  \
> > $(top_builddir)/src/libspice-client-glib-2.0.la \
> > +   $(GTK_LIBS) \
> > $(NULL)
> >  
> >  # FIXME: GtkAction and lots of GtkUIManager APIs are deprecated
> > -- 
> > 2.11.0
> > 
> > ___
> > 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



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] [client] spicy: Spicy uses GTK+ and must link with it

[Christophe Fergeau]

On Fri, Feb 24, 2017 at 12:23:23PM +0100, Francois Gouget wrote:
> This avoids having the linker complain that gtk_toggle_action_get_type()
> or some other GTK+ API is undefined.

This kind of issues are indeed seen on some distros, and not others.
I believe this is what is described on
https://wiki.mageia.org/en/Underlinking_issues_in_packaging

spicy directly uses symbols from gtk+, so it should explicitly link with
it. This wiki page says this should be reproducible using
-Wl,--as-needed, but this did not make a difference for me as -lgtk-3 is
listed in libspice-client-gtk-3.0.la, and libtool uses this when linking
spicy.

However, spicy-stats uses glib/gobject, and does not explicitly link
with it either, I would have expected something like the patch below, but
this can actually be done on top of your patch.

Acked-by: Christophe Fergeau 
even though I would have liked to be able to reproduce the issue :-/


diff --git a/tools/Makefile.am b/tools/Makefile.am
index 0bdb3c5..0b3df78 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -12,6 +12,12 @@ TOOLS_CPPFLAGS = \
$(SPICE_CFLAGS) \
$(NULL)

+TOOLS_LIBS =   \
+   $(GLIB2_LIBS)   \
+   $(GOBJECT2_LIBS)\
+   $(top_builddir)/src/libspice-client-glib-2.0.la \
+   $(NULL)
+
 if WITH_GTK
 bin_PROGRAMS += spicy
 TOOLS_CPPFLAGS += $(GTK_CFLAGS)
@@ -26,8 +32,9 @@ spicy_SOURCES =   \
$(NULL)

 spicy_LDADD =  \
+   $(TOOLS_LIBS)   \
+   $(GTK_LIBS) \
$(top_builddir)/src/libspice-client-gtk-3.0.la  \
-   $(top_builddir)/src/libspice-client-glib-2.0.la \
$(NULL)

 # FIXME: GtkAction and lots of GtkUIManager APIs are deprecated
@@ -44,8 +51,7 @@ spicy_screenshot_SOURCES =\
$(NULL)

 spicy_screenshot_LDADD =   \
-   $(top_builddir)/src/libspice-client-glib-2.0.la \
-   $(GOBJECT2_LIBS)\
+   $(TOOLS_LIBS)   \
$(NULL)

 spicy_screenshot_CPPFLAGS =\
@@ -59,7 +65,7 @@ spicy_stats_SOURCES = \
$(NULL)

 spicy_stats_LDADD =\
-   $(top_builddir)/src/libspice-client-glib-2.0.la \
+   $(TOOLS_LIBS)   \
$(NULL)

 spicy_stats_CPPFLAGS = \


Christophe


> 
> Signed-off-by: Francois Gouget 
> ---
>  tools/Makefile.am | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 0bdb3c54..e41e0ab5 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -28,6 +28,7 @@ spicy_SOURCES = \
>  spicy_LDADD =\
>   $(top_builddir)/src/libspice-client-gtk-3.0.la  \
>   $(top_builddir)/src/libspice-client-glib-2.0.la \
> + $(GTK_LIBS) \
>   $(NULL)
>  
>  # FIXME: GtkAction and lots of GtkUIManager APIs are deprecated
> -- 
> 2.11.0
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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