Re: [Spice-devel] [PATCH spice-gtk 1/2] Change the setting of the images cache size and the glz window size

[Yonit Halperin]

Thanks, will send a corrected version.
Yonit.

On 01/18/2012 04:41 PM, Alon Levy wrote:

On Mon, Jan 16, 2012 at 06:15:08PM +0200, Yonit Halperin wrote:

Set the default sizes to be the same as in the old linux spice client.
cache_size=20M pixels (instead of 32M), window_size=8M pixels for a 64MB
dev ram (instead of 16M pixels).
---
  gtk/channel-display-priv.h |2 -
  gtk/channel-display.c  |   22 +++---
  gtk/channel-main.c |1 +
  gtk/spice-session-priv.h   |   12 ++
  gtk/spice-session.c|   90 +++-
  5 files changed, 117 insertions(+), 10 deletions(-)

diff --git a/gtk/channel-display-priv.h b/gtk/channel-display-priv.h
index 332d271..eca1787 100644
--- a/gtk/channel-display-priv.h
+++ b/gtk/channel-display-priv.h
@@ -36,8 +36,6 @@

  G_BEGIN_DECLS

-#define DISPLAY_PIXMAP_CACHE (1024 * 1024 * 32)
-#define GLZ_WINDOW_SIZE  (1024 * 1024 * 16)

  typedef struct display_surface {
  RingItemlink;
diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index ec42829..2473ac6 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -766,13 +766,21 @@ static void emit_invalidate(SpiceChannel *channel, 
SpiceRect *bbox)
  static void spice_display_channel_up(SpiceChannel *channel)
  {
  SpiceMsgOut *out;
-SpiceMsgcDisplayInit init = {
-.pixmap_cache_id= 1,
-.pixmap_cache_size  = DISPLAY_PIXMAP_CACHE,
-.glz_dictionary_id  = 1,
-.glz_dictionary_window_size = GLZ_WINDOW_SIZE,
-};
-
+SpiceSession *s = spice_channel_get_session(channel);
+SpiceMsgcDisplayInit init;
+int cache_size;
+int glz_window_size;
+
+g_object_get(s,
+ cache-size,cache_size,
+ glz-window-size,glz_window_size,
+ NULL);
+SPICE_DEBUG(%s: cache_size %d, glz_window_size %d (bytes), __FUNCTION__,
+cache_size, glz_window_size);
+init.pixmap_cache_id = 1;
+init.glz_dictionary_id = 1;
+init.pixmap_cache_size = cache_size / 4; /* pixels */
+init.glz_dictionary_window_size = glz_window_size / 4; /* pixels */
  out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_INIT);
  out-marshallers-msgc_display_init(out-marshaller,init);
  spice_msg_out_send_internal(out);
diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index ebf660f..b2df547 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -1128,6 +1128,7 @@ static void main_handle_init(SpiceChannel *channel, 
SpiceMsgIn *in)
 init-current_mouse_mode);

  spice_session_set_mm_time(session, init-multi_media_time);
+spice_session_set_caches_hints(session, init-ram_hint, 
init-display_channels_hint);

  c-agent_tokens = init-agent_tokens;
  if (init-agent_connected)
diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
index 5df1182..430f4a4 100644
--- a/gtk/spice-session-priv.h
+++ b/gtk/spice-session-priv.h
@@ -27,6 +27,10 @@

  G_BEGIN_DECLS

+#define MAX_IMAGES_CACHE_SIZE (1024 * 1024 * 80)


Why 80MB? why not just 0x or any other meaninglessly high number
- we will fail when trying to allocate if the number is too large
   anyway.


+#define MIN_GLZ_WINDOW_SIZE (1024 * 1024 * 12)
+#define MAX_GLZ_WINDOW_SIZE MIN((LZ_MAX_WINDOW_SIZE * 4), 1024 * 1024 * 64)
+
  struct _SpiceSessionPrivate {
  char  *host;
  char  *port;
@@ -84,6 +88,11 @@ struct _SpiceSessionPrivate {
  display_cache images;
  display_cache palettes;
  SpiceGlzDecoderWindow *glz_window;
+int   images_cache_size;
+int   glz_window_size;
+uint32_t  pci_ram_size;
+uint32_t  display_channels_count;
+

  /* associated objects */
  SpiceAudio*audio_manager;
@@ -120,6 +129,9 @@ const gchar* spice_session_get_cert_subject(SpiceSession 
*session);
  const gchar* spice_session_get_ciphers(SpiceSession *session);
  const gchar* spice_session_get_ca_file(SpiceSession *session);

+void spice_session_set_caches_hints(SpiceSession *session,
+uint32_t pci_ram_size,
+uint32_t display_channels_count);
  void spice_session_get_caches(SpiceSession *session,
display_cache **images,
display_cache **palettes,
diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index 79ba1b6..54aeabd 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -101,6 +101,8 @@ enum {
  PROP_DISABLE_EFFECTS,
  PROP_COLOR_DEPTH,
  PROP_READ_ONLY,
+PROP_CACHE_SIZE,
+PROP_GLZ_WINDOW_SIZE,
  };

  /* signals */
@@ -407,7 +409,13 @@ static void spice_session_get_property(GObject*gobject,
  break;
  case PROP_READ_ONLY:
  g_value_set_boolean(value, s-read_only);
-   break;
+   break;

indentation error


+case

[Spice-devel] [PATCH spice-gtk v2 1/2] Change the setting of the images cache size and the glz window size

[Yonit Halperin]

Set the default sizes to be the same as in the old linux spice client.
cache_size=20M pixels (instead of 32M), window_size=8M pixels for a 64MB
dev ram (instead of 16M pixels).
---
 gtk/channel-display-priv.h |2 -
 gtk/channel-display.c  |   22 +++---
 gtk/channel-main.c |1 +
 gtk/spice-session-priv.h   |   12 ++
 gtk/spice-session.c|   90 +++-
 5 files changed, 117 insertions(+), 10 deletions(-)

diff --git a/gtk/channel-display-priv.h b/gtk/channel-display-priv.h
index 332d271..eca1787 100644
--- a/gtk/channel-display-priv.h
+++ b/gtk/channel-display-priv.h
@@ -36,8 +36,6 @@
 
 G_BEGIN_DECLS
 
-#define DISPLAY_PIXMAP_CACHE (1024 * 1024 * 32)
-#define GLZ_WINDOW_SIZE  (1024 * 1024 * 16)
 
 typedef struct display_surface {
 RingItemlink;
diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index ec42829..2473ac6 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -766,13 +766,21 @@ static void emit_invalidate(SpiceChannel *channel, 
SpiceRect *bbox)
 static void spice_display_channel_up(SpiceChannel *channel)
 {
 SpiceMsgOut *out;
-SpiceMsgcDisplayInit init = {
-.pixmap_cache_id= 1,
-.pixmap_cache_size  = DISPLAY_PIXMAP_CACHE,
-.glz_dictionary_id  = 1,
-.glz_dictionary_window_size = GLZ_WINDOW_SIZE,
-};
-
+SpiceSession *s = spice_channel_get_session(channel);
+SpiceMsgcDisplayInit init;
+int cache_size;
+int glz_window_size;
+
+g_object_get(s,
+ cache-size, cache_size,
+ glz-window-size, glz_window_size,
+ NULL);
+SPICE_DEBUG(%s: cache_size %d, glz_window_size %d (bytes), __FUNCTION__,
+cache_size, glz_window_size);
+init.pixmap_cache_id = 1;
+init.glz_dictionary_id = 1;
+init.pixmap_cache_size = cache_size / 4; /* pixels */
+init.glz_dictionary_window_size = glz_window_size / 4; /* pixels */
 out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_INIT);
 out-marshallers-msgc_display_init(out-marshaller, init);
 spice_msg_out_send_internal(out);
diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index ebf660f..b2df547 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -1128,6 +1128,7 @@ static void main_handle_init(SpiceChannel *channel, 
SpiceMsgIn *in)
init-current_mouse_mode);
 
 spice_session_set_mm_time(session, init-multi_media_time);
+spice_session_set_caches_hints(session, init-ram_hint, 
init-display_channels_hint);
 
 c-agent_tokens = init-agent_tokens;
 if (init-agent_connected)
diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
index 5df1182..a814cfe 100644
--- a/gtk/spice-session-priv.h
+++ b/gtk/spice-session-priv.h
@@ -27,6 +27,10 @@
 
 G_BEGIN_DECLS
 
+#define IMAGES_CACHE_SIZE_DEFAULT (1024 * 1024 * 80)
+#define MIN_GLZ_WINDOW_SIZE_DEFAULT (1024 * 1024 * 12)
+#define MAX_GLZ_WINDOW_SIZE_DEFAULT MIN((LZ_MAX_WINDOW_SIZE * 4), 1024 * 1024 
* 64)
+
 struct _SpiceSessionPrivate {
 char  *host;
 char  *port;
@@ -84,6 +88,11 @@ struct _SpiceSessionPrivate {
 display_cache images;
 display_cache palettes;
 SpiceGlzDecoderWindow *glz_window;
+int   images_cache_size;
+int   glz_window_size;
+uint32_t  pci_ram_size;
+uint32_t  display_channels_count;
+
 
 /* associated objects */
 SpiceAudio*audio_manager;
@@ -120,6 +129,9 @@ const gchar* spice_session_get_cert_subject(SpiceSession 
*session);
 const gchar* spice_session_get_ciphers(SpiceSession *session);
 const gchar* spice_session_get_ca_file(SpiceSession *session);
 
+void spice_session_set_caches_hints(SpiceSession *session,
+uint32_t pci_ram_size,
+uint32_t display_channels_count);
 void spice_session_get_caches(SpiceSession *session,
   display_cache **images,
   display_cache **palettes,
diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index 79ba1b6..d36709e 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -101,6 +101,8 @@ enum {
 PROP_DISABLE_EFFECTS,
 PROP_COLOR_DEPTH,
 PROP_READ_ONLY,
+PROP_CACHE_SIZE,
+PROP_GLZ_WINDOW_SIZE,
 };
 
 /* signals */
@@ -407,7 +409,13 @@ static void spice_session_get_property(GObject*gobject,
 break;
 case PROP_READ_ONLY:
 g_value_set_boolean(value, s-read_only);
-   break;
+break;
+case PROP_CACHE_SIZE:
+g_value_set_int(value, s-images_cache_size);
+break;
+case PROP_GLZ_WINDOW_SIZE:
+g_value_set_int(value, s-glz_window_size);
+break;
 default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
break;
@@ -508,6 +516,12 @@ static void 

[Spice-devel] [PATCH spice-gtk v2 2/2] Add command line options for setting the cache size and the glz window size

[Yonit Halperin]

This options will help us tune and find the optimal values.
---
 gtk/spice-option.c |   10 ++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gtk/spice-option.c b/gtk/spice-option.c
index 394a07d..d466f94 100644
--- a/gtk/spice-option.c
+++ b/gtk/spice-option.c
@@ -36,6 +36,8 @@ static char *usbredir_filter = NULL;
 static gboolean smartcard = FALSE;
 static gboolean disable_audio = FALSE;
 static gboolean disable_usbredir = FALSE;
+static gint cache_size = 0;
+static gint glz_window_size = 0;
 
 static void option_version(void)
 {
@@ -84,6 +86,10 @@ GOptionGroup* spice_get_option_group(void)
   N_(Enable Spice-GTK debugging), NULL },
 { spice-gtk-version, '\0', G_OPTION_FLAG_NO_ARG, 
G_OPTION_ARG_CALLBACK, option_version,
   N_(Display Spice-GTK version information), NULL },
+{ spice-cache-size, '\0', 0, G_OPTION_ARG_INT, cache_size,
+  N_(Image cache size), N_(bytes) },
+{ spice-glz-window-size, '\0', 0, G_OPTION_ARG_INT, glz_window_size,
+  N_(Glz compression history size), N_(bytes) },
 { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
 };
 GOptionGroup *grp;
@@ -146,4 +152,8 @@ void spice_set_session_option(SpiceSession *session)
 g_object_set(session, enable-usbredir, FALSE, NULL);
 if (disable_audio)
 g_object_set(session, enable-audio, FALSE, NULL);
+if (cache_size)
+g_object_set(session, cache-size, cache_size, NULL);
+if (glz_window_size)
+g_object_set(session, glz_window_size, glz_window_size, NULL);
 }
-- 
1.7.6.4

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

Re: [Spice-devel] [PATCH spice-gtk v2 1/2] Change the setting of the images cache size and the glz window size

[Yonit Halperin]

Hi,

On 01/23/2012 12:36 PM, Hans de Goede wrote:

Hi,

I've various remarks, see my comments inline.

On 01/22/2012 09:12 AM, Yonit Halperin wrote:

Set the default sizes to be the same as in the old linux spice client.
cache_size=20M pixels (instead of 32M), window_size=8M pixels for a 64MB
dev ram (instead of 16M pixels).
---
gtk/channel-display-priv.h | 2 -
gtk/channel-display.c | 22 +++---
gtk/channel-main.c | 1 +
gtk/spice-session-priv.h | 12 ++
gtk/spice-session.c | 90 +++-
5 files changed, 117 insertions(+), 10 deletions(-)

diff --git a/gtk/channel-display-priv.h b/gtk/channel-display-priv.h
index 332d271..eca1787 100644
--- a/gtk/channel-display-priv.h
+++ b/gtk/channel-display-priv.h
@@ -36,8 +36,6 @@

G_BEGIN_DECLS

-#define DISPLAY_PIXMAP_CACHE (1024 * 1024 * 32)
-#define GLZ_WINDOW_SIZE (1024 * 1024 * 16)

typedef struct display_surface {
RingItem link;
diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index ec42829..2473ac6 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -766,13 +766,21 @@ static void emit_invalidate(SpiceChannel
*channel, SpiceRect *bbox)
static void spice_display_channel_up(SpiceChannel *channel)
{
SpiceMsgOut *out;
- SpiceMsgcDisplayInit init = {
- .pixmap_cache_id = 1,
- .pixmap_cache_size = DISPLAY_PIXMAP_CACHE,
- .glz_dictionary_id = 1,
- .glz_dictionary_window_size = GLZ_WINDOW_SIZE,
- };
-
+ SpiceSession *s = spice_channel_get_session(channel);
+ SpiceMsgcDisplayInit init;
+ int cache_size;
+ int glz_window_size;
+
+ g_object_get(s,
+ cache-size,cache_size,
+ glz-window-size,glz_window_size,
+ NULL);
+ SPICE_DEBUG(%s: cache_size %d, glz_window_size %d (bytes),
__FUNCTION__,
+ cache_size, glz_window_size);
+ init.pixmap_cache_id = 1;
+ init.glz_dictionary_id = 1;
+ init.pixmap_cache_size = cache_size / 4; /* pixels */
+ init.glz_dictionary_window_size = glz_window_size / 4; /* pixels */
out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_INIT);
out-marshallers-msgc_display_init(out-marshaller,init);
spice_msg_out_send_internal(out);
diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index ebf660f..b2df547 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -1128,6 +1128,7 @@ static void main_handle_init(SpiceChannel
*channel, SpiceMsgIn *in)
init-current_mouse_mode);

spice_session_set_mm_time(session, init-multi_media_time);
+ spice_session_set_caches_hints(session, init-ram_hint,
init-display_channels_hint);

c-agent_tokens = init-agent_tokens;
if (init-agent_connected)
diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
index 5df1182..a814cfe 100644
--- a/gtk/spice-session-priv.h
+++ b/gtk/spice-session-priv.h
@@ -27,6 +27,10 @@

G_BEGIN_DECLS

+#define IMAGES_CACHE_SIZE_DEFAULT (1024 * 1024 * 80)
+#define MIN_GLZ_WINDOW_SIZE_DEFAULT (1024 * 1024 * 12)
+#define MAX_GLZ_WINDOW_SIZE_DEFAULT MIN((LZ_MAX_WINDOW_SIZE * 4),
1024 * 1024 * 64)
+


Are these glz window size min and max values only for the default
calculation, or
should the same min / max also be applied in
spice_session_set_property() ? If they
also should be applied in spice_session_set_property, I suggest dropping
the
_DEFAULT, and adding checks for them there. If they should not be
applied in
spice_session_set_property(), I would like to see some additional
MIN/MAX defines
which are used to clamp the input value in spice_session_set_property(),
likewise I
would like to see some min / max defines for the image cache size and the
PROP_CACHE_SIZE input value clamped to these in
spice_session_set_property().


The min/max default values are used only for the default calculation.
In g_object_class_install_property I set the min/max value for both the 
cache and dictionary. Their minimal size is 0. The maximal size for the 
cache is G_MAXINT and for the dictionary it is the maximal value that 
can be processed by the lz code:  LZ_MAX_WINDOW_SIZE * 4 ( 
LZ_MAX_WINDOW_SIZE is in pixels). These limits will allow us to test a 
variety of values. Once we have conclusion for the optimal sizes 
(depending on different conditions), we can set stricter limits.

Do you want me to add explicit defines for these limits?


struct _SpiceSessionPrivate {
char *host;
char *port;
@@ -84,6 +88,11 @@ struct _SpiceSessionPrivate {
display_cache images;
display_cache palettes;
SpiceGlzDecoderWindow *glz_window;
+ int images_cache_size;
+ int glz_window_size;
+ uint32_t pci_ram_size;
+ uint32_t display_channels_count;
+

/* associated objects */
SpiceAudio *audio_manager;
@@ -120,6 +129,9 @@ const gchar*
spice_session_get_cert_subject(SpiceSession *session);
const gchar* spice_session_get_ciphers(SpiceSession *session);
const gchar* spice_session_get_ca_file(SpiceSession *session);

+void spice_session_set_caches_hints(SpiceSession *session,
+ uint32_t pci_ram_size,
+ uint32_t display_channels_count);
void spice_session_get_caches(SpiceSession *session,
display_cache **images,
display_cache **palettes,
diff --git a/gtk/spice

Re: [Spice-devel] RFC: remove Adler checksum and zlib header/trailer from zlib compressed images

[Yonit Halperin]

On 02/06/2012 09:02 AM, Yaniv Kaul wrote:

On 01/29/2012 11:29 AM, Alon Levy wrote:

On Sun, Jan 29, 2012 at 09:28:34AM +0200, Yaniv Kaul wrote:

These small changes to server and (gtk) client seem to work, and
remove both the zlib header/trailer from the deflate stream as well
as the Adler checksum, which removes several bytes from the stream
as well as speedup (unnoticeable) of the compression/decompression
due to the removal of the checksum calculation.

Problem is - not sure how to test it - don't know how to get an
image once with and once without the patch - and ensure it's the
same image.
The fact that the first image is never zlib compressed complicates
things. I could not reliably get a 2nd image sent to the client
which looks the same as one in a different connection.

Nevertheless it does no harm - and images pass well - verified by
connecting with a modified Spice GTK client to a modified Spice
server, as well as looking at the network capture via wireshark.

It's of course missing the display channel specific capability
negotiation for this change.


I'm afraid I could not really find where to perform the display channel
specific capability negotiation. :(
Anyone?


Hi,

red_dispatcher.c/red_dispatcher_set_display_peer receives the client's caps.
They should be sent to red_worker.c/handle_dev_display_connect (need to 
change RedWorkerMessageDisplayConnect payload to contain them).

caps are tested via red_channel_client_test_remote_cap.
The negotiation should be in red_worker.c/handle_new_display_channel.

The display channel server caps should be set in 
red_worker.c/display_channel_create via red_channel.set_cap.


Cheers,
Yonit.

TIA,
Y.


Comments are welcome.

Other then that adding a link to the docs page of
inlateInit2/deflateInit2 in the commit message would be nice for those
seeking to find the explanation for the magic numbers, no comment,
thanks for doing this!

If you want a repeatable test you can write one or use one of the
existing artificial tests in server/tests.


Client change:
diff --git a/gtk/decode-zlib.c b/gtk/decode-zlib.c
index a692020..997f350 100644
--- a/gtk/decode-zlib.c
+++ b/gtk/decode-zlib.c
@@ -63,7 +63,7 @@ SpiceZlibDecoder *zlib_decoder_new(void)
d-_z_strm.opaque = Z_NULL;
d-_z_strm.next_in = Z_NULL;
d-_z_strm.avail_in = 0;
- z_ret = inflateInit(d-_z_strm);
+ z_ret = inflateInit2(d-_z_strm, -15);
if (z_ret != Z_OK) {
g_warning(zlib decoder init failed, error %d, z_ret);
goto fail;


server change:
diff --git a/server/zlib_encoder.c b/server/zlib_encoder.c
index c51466b..66a039a 100644
--- a/server/zlib_encoder.c
+++ b/server/zlib_encoder.c
@@ -47,7 +47,7 @@ ZlibEncoder*
zlib_encoder_create(ZlibEncoderUsrContext *usr, int level)
enc-strm.zfree = Z_NULL;
enc-strm.opaque = Z_NULL;

- z_ret = deflateInit(enc-strm, level);
+ z_ret = deflateInit2(enc-strm, level, Z_DEFLATED, -15, 8,
Z_DEFAULT_STRATEGY);
enc-last_level = level;
if (z_ret != Z_OK) {
red_printf(zlib error);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


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


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

[Spice-devel] [PATCH] server: support IPV6 addresses in channel events sent to qemu

[Yonit Halperin]

RHBZ #788444

CC: Gerd Hoffmann kra...@redhat.com

Signed-off-by: Gerd Hoffmann kra...@redhat.com
Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 server/reds.c  |   21 +
 server/spice.h |6 ++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 2492a89..828ba65 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2005,7 +2005,7 @@ static void reds_get_spice_ticket(RedLinkInfo *link)
 
 #if HAVE_SASL
 static char *addr_to_string(const char *format,
-struct sockaddr *sa,
+struct sockaddr_storage *sa,
 socklen_t salen) {
 char *addr;
 char host[NI_MAXHOST];
@@ -2013,7 +2013,7 @@ static char *addr_to_string(const char *format,
 int err;
 size_t addrlen;
 
-if ((err = getnameinfo(sa, salen,
+if ((err = getnameinfo((struct sockaddr *)sa, salen,
host, sizeof(host),
serv, sizeof(serv),
NI_NUMERICHOST | NI_NUMERICSERV)) != 0) {
@@ -2402,11 +2402,13 @@ static void reds_start_auth_sasl(RedLinkInfo *link)
 RedsSASL *sasl = link-stream-sasl;
 
 /* Get local  remote client addresses in form  IPADDR;PORT */
-if (!(localAddr = addr_to_string(%s;%s, link-stream-info.laddr, 
link-stream-info.llen))) {
+if (!(localAddr = addr_to_string(%s;%s, link-stream-info.laddr_ext,
+  link-stream-info.llen_ext))) {
 goto error;
 }
 
-if (!(remoteAddr = addr_to_string(%s;%s, link-stream-info.paddr, 
link-stream-info.plen))) {
+if (!(remoteAddr = addr_to_string(%s;%s, link-stream-info.paddr_ext,
+   link-stream-info.plen_ext))) {
 free(localAddr);
 goto error;
 }
@@ -2717,10 +2719,21 @@ static RedLinkInfo *reds_init_client_connection(int 
socket)
 
 stream-socket = socket;
 /* gather info + send event */
+
+/* deprecated fields. Filling them for backward compatibility */
 stream-info.llen = sizeof(stream-info.laddr);
 stream-info.plen = sizeof(stream-info.paddr);
 getsockname(stream-socket, (struct sockaddr*)(stream-info.laddr), 
stream-info.llen);
 getpeername(stream-socket, (struct sockaddr*)(stream-info.paddr), 
stream-info.plen);
+
+stream-info.flags |= SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT;
+stream-info.llen_ext = sizeof(stream-info.laddr_ext);
+stream-info.plen_ext = sizeof(stream-info.paddr_ext);
+getsockname(stream-socket, (struct sockaddr*)(stream-info.laddr_ext),
+stream-info.llen_ext);
+getpeername(stream-socket, (struct sockaddr*)(stream-info.paddr_ext),
+stream-info.plen_ext);
+
 reds_stream_channel_event(stream, SPICE_CHANNEL_EVENT_CONNECTED);
 
 openssl_init(link);
diff --git a/server/spice.h b/server/spice.h
index c582e6c..7397655 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -54,6 +54,7 @@ typedef struct SpiceCoreInterface SpiceCoreInterface;
 #define SPICE_CHANNEL_EVENT_DISCONNECTED  3
 
 #define SPICE_CHANNEL_EVENT_FLAG_TLS  (1  0)
+#define SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT (1  1)
 
 typedef struct SpiceWatch SpiceWatch;
 typedef void (*SpiceWatchFunc)(int fd, int event, void *opaque);
@@ -66,9 +67,14 @@ typedef struct SpiceChannelEventInfo {
 int type;
 int id;
 int flags;
+/* deprecated, can't hold ipv6 addresses, kept for backward compatibility 
*/
 struct sockaddr laddr;
 struct sockaddr paddr;
 socklen_t llen, plen;
+/* should be used if (flags  SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) */
+struct sockaddr_storage laddr_ext;
+struct sockaddr_storage paddr_ext;
+socklen_t llen_ext, plen_ext;
 } SpiceChannelEventInfo;
 
 struct SpiceCoreInterface {
-- 
1.7.7.6

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

[Spice-devel] [PATCH] spice: support ipv6 channel address in monitor events and in spice info

[Yonit Halperin]

RHBZ #788444

CC: Gerd Hoffmann kra...@redhat.com

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 ui/spice-core.c |   37 -
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 5639c6f..60fd6c3 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -220,10 +220,23 @@ static void channel_event(int event, 
SpiceChannelEventInfo *info)
 }
 
 client = qdict_new();
-add_addr_info(client, info-paddr, info-plen);
-
 server = qdict_new();
-add_addr_info(server, info-laddr, info-llen);
+
+#ifdef SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT
+if (info-flags  SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
+add_addr_info(client, (struct sockaddr *)info-paddr_ext,
+  info-plen_ext);
+add_addr_info(server, (struct sockaddr *)info-laddr_ext,
+  info-llen_ext);
+} else {
+fprintf(stderr, spice: %s, extended address is expected\n,
+__func__);
+#endif
+add_addr_info(client, info-paddr, info-plen);
+add_addr_info(server, info-laddr, info-llen);
+#ifdef SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT
+}
+#endif
 
 if (event == SPICE_CHANNEL_EVENT_INITIALIZED) {
 qdict_put(server, auth, qstring_from_str(auth));
@@ -376,16 +389,30 @@ static SpiceChannelList *qmp_query_spice_channels(void)
 QTAILQ_FOREACH(item, channel_list, link) {
 SpiceChannelList *chan;
 char host[NI_MAXHOST], port[NI_MAXSERV];
+struct sockaddr *paddr;
+socklen_t plen;
 
 chan = g_malloc0(sizeof(*chan));
 chan-value = g_malloc0(sizeof(*chan-value));
 
-getnameinfo(item-info-paddr, item-info-plen,
+#ifdef SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT
+if (item-info-flags  SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
+paddr = (struct sockaddr *)item-info-paddr_ext;
+plen = item-info-plen_ext;
+} else {
+#endif
+paddr = item-info-paddr;
+plen = item-info-plen;
+#ifdef SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT
+}
+#endif
+
+getnameinfo(paddr, plen,
 host, sizeof(host), port, sizeof(port),
 NI_NUMERICHOST | NI_NUMERICSERV);
 chan-value-host = g_strdup(host);
 chan-value-port = g_strdup(port);
-chan-value-family = 
g_strdup(inet_strfamily(item-info-paddr.sa_family));
+chan-value-family = g_strdup(inet_strfamily(paddr-sa_family));
 
 chan-value-connection_id = item-info-connection_id;
 chan-value-channel_type = item-info-type;
-- 
1.7.7.6

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

Re: [Spice-devel] spice configuration - problem with connection

[Yonit Halperin]

On 02/11/2012 01:29 PM, Daniel Parnak wrote:

Hello,

I want to test spice on my virtual machines but I have problem.
I create 2 virtual machines (one for server, one for client) on VMware
Workstation 8 and I run on them Fedora-16-x86_64-Live-Desktop.

# On server I do:
yum -y install qemu-kvm libvirt python-virtinst bridge-utils
systemctl start libvirtd.service
chkconfig libvirtd on
yum -y install spice-server spice-protocol
qemu-img create /tmp/fedora.qcow 8G
qemu -cdrom /dev/cdrom -hda /tmp/fedora.qcow -boot d -net nic -net user
-m 1024

# Then after run virtual machine I start spice
qemu -spice port=5930
Hi, you should add the -spice spice-params at the same command you 
run the vm. No need for 2 different `qemu` runs.
In addition, you probably also want to add to spice-params 
,disable-ticketing and to qemu params -vga qxl.


# On client's machine I install spice client
yum -y install spice-client spice-protocol
spicec -h 192.168.163.128 -p 5930

And I receive warning:
failed to connect: no route to host (113)

I can ping server and host. Tcpdump shows that when I want to connect
via spicec packages are sent, and host receive it.

What is wrong? And how can I resolve this problem?

Greetings,
Daniel


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


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

Re: [Spice-devel] spice problem with firewall or natting

[Yonit Halperin]

Hi,
On 02/09/2012 05:40 AM, abdulkader wrote:

hai,

I installed rhev-v and everything working perfect. inside network, I can
access spice desktop. but when I am trying to connect to desktop from
outside(natted public IP, my firewall ASA) through user portal, spice
showing starting screen, but it is showing error.

please help on this matter

Can you attach the client and server logs?
The client's log is at $home/.spicec/spicec.log (or at %APPDATA%/spicec/ 
on a windows client)


The server log is located on the host at /var/log/libvirt/qemu/vm-name.log

Cheers,
Yonit.


regards,

AbdulKader



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


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

Re: [Spice-devel] Untested idea: enlarge the TCP send buffer (server), receive buffer (client)

[Yonit Halperin]

On 02/13/2012 10:47 PM, Yaniv Kaul wrote:

For both LAN (high bandwidth, high performance) and WAN (high latency)
perhaps it may be worthwhile to increase (via socket options) the TCP
receive (on the client) and send (on the server) buffers for the display
channel? It will cause:
- bigger TCP window (which is good for both cases above)
- some waste of memory (negligible, I think it's enough to increase to
256K or so for each).


Hi,
I've started investigating it a bit, but from spice network captures I 
didn't see that the window size is a limiting factor. In all of the 
scenarios I saw, the windows was big enough during all the session . I 
don't recall many cases in which the available window size was close to 
zero. Still, I didn't completed testing this and we do plan to add 
options to configure the window size and test how it affects different 
scenarios.



I suspect it might make a difference especially in video streaming.


btw, I've started to work on moving the video to udp.


It's a simple patch (I hope - via setsockopt() ) and does not require
feature negotiation, but I'm not sure I have the tools to test such change.

Thoughts?
Y.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


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

[Spice-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

RHBZ #790083

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 hw/qxl.c |   17 -
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index bc03c1d..a2a3380 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1478,14 +1478,21 @@ static void qxl_vm_change_state_handler(void *opaque, 
int running,
  * called
  */
  qxl_update_irq(qxl);
-} else if (qxl-mode == QXL_MODE_NATIVE) {
-/* dirty all vram (which holds surfaces) and devram (primary surface)
+} else {
+/* dirty all vram (which holds surfaces) and the primary surface
  * to make sure they are saved */
 /* FIXME #1: should go out during live stage */
 /* FIXME #2: we only need to save the areas which are actually used */
-qxl_set_dirty(qxl-vram_bar, 0, qxl-vram_size);
-qxl_set_dirty(qxl-vga.vram, qxl-shadow_rom.draw_area_offset,
-  qxl-shadow_rom.surface0_area_size);
+switch (qxl-mode) {
+case QXL_MODE_NATIVE:
+qxl_set_dirty(qxl-vram_bar, 0, qxl-vram_size);
+case QXL_MODE_COMPAT:
+qxl_set_dirty(qxl-vga.vram, qxl-shadow_rom.draw_area_offset,
+  qxl-shadow_rom.surface0_area_size);
+break;
+default:
+break;
+}
 }
 }
 
-- 
1.7.7.6

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

Re: [Spice-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

On 02/14/2012 10:35 AM, Gerd Hoffmann wrote:

On 02/14/12 09:10, Yonit Halperin wrote:

RHBZ #790083

Signed-off-by: Yonit Halperinyhalp...@redhat.com


You are doing two things in one patch:  (a) fix the compat mode bug,
which also matches the patch description, and (b) skip vram when it is
unused (in compat mode).

I'd love to see (b) done in a different way: simply walk all surfaces
and tag them dirty.  Will have the same effect for compat mode (no
surfaces used -  nothing tagged dirty) and additionally it will (in
native mode) only migrate over the vram areas which are actually filled
with surfaces.

I can do it, by retrieving the surfaces addresses from the tracked guest 
commands. However, if we already do it, it would be even better if we 
just dirty only the areas that are actually modified by the update_area 
calls. The problem is that (1) spice-server updates surfaces also 
without request from driver. We can add a cb to the interface or use the 
async_complete cb with a special flag (2) async_complete cb is called 
from spice server context. We can add a pipe for update_area dirty 
events, and make sure that it is handled, before migration moves from 
the live stage.


Yonit.

thanks,
   Gerd


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

Re: [Spice-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

On 02/14/2012 11:10 AM, Yonit Halperin wrote:

On 02/14/2012 10:35 AM, Gerd Hoffmann wrote:

On 02/14/12 09:10, Yonit Halperin wrote:

RHBZ #790083

Signed-off-by: Yonit Halperinyhalp...@redhat.com


You are doing two things in one patch: (a) fix the compat mode bug,
which also matches the patch description, and (b) skip vram when it is
unused (in compat mode).

I'd love to see (b) done in a different way: simply walk all surfaces
and tag them dirty. Will have the same effect for compat mode (no
surfaces used - nothing tagged dirty) and additionally it will (in
native mode) only migrate over the vram areas which are actually filled
with surfaces.


I can do it, by retrieving the surfaces addresses from the tracked guest
commands. However, if we already do it, it would be even better if we
just dirty only the areas that are actually modified by the update_area
calls. The problem is that (1) spice-server updates surfaces also
without request from driver. We can add a cb to the interface or use the
async_complete cb with a special flag (2) async_complete cb is called
from spice server context. We can add a pipe for update_area dirty
events, and make sure that it is handled, before migration moves from
the live stage.
Giving it another thought, I don't really like it because surfaces are 
destroyed with high frequency. So it will be a waste.

So doing it just before migration sounds better.


Yonit.

thanks,
Gerd


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


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

Re: [Spice-devel] [PATCH] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

On 02/14/2012 11:24 AM, Gerd Hoffmann wrote:

   Hi,


I can do it, by retrieving the surfaces addresses from the tracked guest
commands.


Exactly.


However, if we already do it, it would be even better if we
just dirty only the areas that are actually modified by the update_area
calls. The problem is that (1) spice-server updates surfaces also
without request from driver.


On worker-stop() for example, which renderes all outstanding commands
so all state is flushed to the surfaces (and thereby device memory).
This is done on vm_stop too, so I wouldn't be surprised if most surfaces
are dirtied anyway at this point.  Getting notifications about
spice-server touching surfaces doesn't buy us much then.
We also render drawables that are not hidden by other ones, when we lack 
free memory in the driver.
In addition inter-surfaces dependencies are handled in a brute-force 
manner by rendering old drawables (I plan to improve this). But anyway,
as I mentioned it the preceding email, since most of the surfaces are 
destroyed (at least with the current drivers), I prefer not to dirty 
them live. So I'll make the change you originally suggested.


Regards,
Yonit.



cheers,
   Gerd



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

[Spice-devel] [PATCH v2 1/2] qxl: set only off-screen surfaces dirty instead of the whole vram

[Yonit Halperin]

We used to assure the guest surfaces were saved before migration by
setting the whole vram dirty. This patch sets dirty only the areas
that are actually used in the vram.

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 hw/qxl.c |   53 -
 1 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index bc03c1d..df55de1 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1006,7 +1006,7 @@ static void qxl_reset_surfaces(PCIQXLDevice *d)
 qxl_spice_destroy_surfaces(d, QXL_SYNC);
 }
 
-/* called from spice server thread context only */
+/* can be also called from spice server thread context */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
 {
 uint64_t phys   = le64_to_cpu(pqxl);
@@ -1465,6 +1465,46 @@ static void qxl_hw_text_update(void *opaque, 
console_ch_t *chardata)
 }
 }
 
+static void qxl_dirty_surfaces(PCIQXLDevice *qxl)
+{
+intptr_t vram_start;
+int i;
+
+if (qxl-mode != QXL_MODE_NATIVE) {
+return;
+}
+
+/* dirty the primary surface */
+qxl_set_dirty(qxl-vga.vram, qxl-shadow_rom.draw_area_offset,
+  qxl-shadow_rom.surface0_area_size);
+
+vram_start =  (intptr_t)memory_region_get_ram_ptr(qxl-vram_bar);
+
+/* dirty the off-screen surfaces */
+for (i = 0; i  NUM_SURFACES; i++) {
+QXLSurfaceCmd *cmd;
+intptr_t surface_offset;
+int surface_size;
+
+if (qxl-guest_surfaces.cmds[i] == 0) {
+continue;
+}
+
+cmd = qxl_phys2virt(qxl, qxl-guest_surfaces.cmds[i],
+MEMSLOT_GROUP_GUEST);
+assert(cmd-type == QXL_SURFACE_CMD_CREATE);
+surface_offset = (intptr_t)qxl_phys2virt(qxl,
+ cmd-u.surface_create.data,
+ MEMSLOT_GROUP_GUEST);
+surface_offset -= vram_start;
+surface_size = cmd-u.surface_create.height *
+   abs(cmd-u.surface_create.stride);
+dprint(qxl, 3, %s: dirty surface %d, offset %d, size %d\n, __func__,
+   i, (int)surface_offset, surface_size);
+qxl_set_dirty(qxl-vram_bar, surface_offset, surface_size);
+}
+}
+
 static void qxl_vm_change_state_handler(void *opaque, int running,
 RunState state)
 {
@@ -1478,14 +1518,9 @@ static void qxl_vm_change_state_handler(void *opaque, 
int running,
  * called
  */
  qxl_update_irq(qxl);
-} else if (qxl-mode == QXL_MODE_NATIVE) {
-/* dirty all vram (which holds surfaces) and devram (primary surface)
- * to make sure they are saved */
-/* FIXME #1: should go out during live stage */
-/* FIXME #2: we only need to save the areas which are actually used */
-qxl_set_dirty(qxl-vram_bar, 0, qxl-vram_size);
-qxl_set_dirty(qxl-vga.vram, qxl-shadow_rom.draw_area_offset,
-  qxl-shadow_rom.surface0_area_size);
+} else {
+/* make sure surfaces are saved before migration */
+qxl_dirty_surfaces(qxl);
 }
 }
 
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 2/2] qxl: make sure primary surface is saved on migration also in compat mode

[Yonit Halperin]

RHBZ #790083

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 hw/qxl.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index df55de1..10137f9 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1470,7 +1470,7 @@ static void qxl_dirty_surfaces(PCIQXLDevice *qxl)
 intptr_t vram_start;
 int i;
 
-if (qxl-mode != QXL_MODE_NATIVE) {
+if (qxl-mode != QXL_MODE_NATIVE  qxl-mode != QXL_MODE_COMPAT) {
 return;
 }
 
-- 
1.7.7.6

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

Re: [Spice-devel] Performance expectations

[Yonit Halperin]

On 02/15/2012 01:16 AM, Kai Meyer wrote:

Help me set my expectations straight. Should I be able to use spice from
inside a VM on my local machine to view video streaming services like
Youtube, Hulu, and Netflix? I have RHEL 6 Workstation for my host, and
Win7 Pro as my guest. I've installed the qxl driver in the VM, and used
Virt-manager to configure the VM to use Spice as my Display and Video
Model is qxl.

You should be able to do it. But it depends on you network conditions 
(client to host).


Yonit.

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


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

Re: [Spice-devel] [PATCH 1/3] README: make a note of SPICE_DEBUG_ALLOW_MC

[Yonit Halperin]

Ack series.

Thanks,
Yonit.

On 02/15/2012 03:09 PM, Alon Levy wrote:

---
  README |5 +
  1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/README b/README
index a27dc06..e146a95 100644
--- a/README
+++ b/README
@@ -79,4 +79,9 @@ version 2.1 of the License, or (at your option) any later 
version.
  Please see the COPYING file for the complete LGPLv2+ license
  terms, or visithttp://www.gnu.org/licenses/.

+Experimental Features
+-
+To enable multiple client connections, set:
+SPICE_DEBUG_ALLOW_MC=1
+
  -- End of readme


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

Re: [Spice-devel] Performance expectations

[Yonit Halperin]

Hi,
On 02/15/2012 06:22 PM, Kai Meyer wrote:



On 02/15/2012 05:19 AM, Yonit Halperin wrote:

On 02/15/2012 01:16 AM, Kai Meyer wrote:

Help me set my expectations straight. Should I be able to use spice from
inside a VM on my local machine to view video streaming services like
Youtube, Hulu, and Netflix? I have RHEL 6 Workstation for my host, and
Win7 Pro as my guest. I've installed the qxl driver in the VM, and used
Virt-manager to configure the VM to use Spice as my Display and Video
Model is qxl.


You should be able to do it. But it depends on you network conditions
(client to host).

Yonit.

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




This is a VM on my local machine, so I'm effectively connecting to
localhost. Network conditions should be the least concern, yet playback
is terrible.


Which spice client do you run?
Another possible reason for the slow playback is that Rhel 6 doesn't 
have libjpeg-turbo.


Yonit.

-Kai Meyer


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

Re: [Spice-devel] [PATCH] server: spice_qxl_update_area_dirty_async

[Yonit Halperin]

Hi,
On 02/19/2012 02:53 PM, Alon Levy wrote:

On Sun, Feb 19, 2012 at 02:37:16PM +0200, Alon Levy wrote:

Bumps spice to 0.9.2, since it adds a new symbol. This will be 0.8.4
in 0.8 branch - the syms file is updated to contain 0.8.3 and 0.8.4.


Forgot to change this comment. I'll drop the 0.10.1-0.10.2 change since
Marc Andre sent a patch that does that already.



Add an asynchronous version of update_area that updates the dirty
rectangles, similarily to the sync one. The existing async version
doesn't pass the dirty list at all, which is good for
QXL_IO_UPDATE_AREA, but bad for the monitor command screen_dump and
vnc/sdl interoperability.

(1) It would be nice to explain why the call to the sync version caused 
deadlock.


(2) Is it necessary to add another async version of update_area? Can't 
we use the update_area_complete callback instead?


Regards,
Yonit.

RHBZ: 747011
FDBZ: 41622
---
  configure.ac |2 +-
  server/red_dispatcher.c  |   42 +++
  server/red_dispatcher.h  |9 +++
  server/red_worker.c  |   60 +-
  server/red_worker.h  |3 ++
  server/spice-server.syms |5 
  server/spice.h   |6 -
  7 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1c15e74..a617096 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,7 +2,7 @@ AC_PREREQ([2.57])

  m4_define([SPICE_MAJOR], 0)
  m4_define([SPICE_MINOR], 10)
-m4_define([SPICE_MICRO], 1)
+m4_define([SPICE_MICRO], 2)

  AC_INIT(spice, [SPICE_MAJOR.SPICE_MINOR.SPICE_MICRO], [], spice)

diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index 321232b..5b8097c 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -334,6 +334,28 @@ static void red_dispatcher_update_area_async(RedDispatcher 
*dispatcher,
  payload);
  }

+static void red_dispatcher_update_area_dirty_async(RedDispatcher *dispatcher,
+   uint32_t surface_id,
+   struct QXLRect *qxl_area,
+   struct QXLRect *qxl_dirty_rects,
+   uint32_t num_dirty_rects,
+   uint32_t clear_dirty_region,
+   uint64_t cookie)
+{
+RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC;
+RedWorkerMessageUpdateDirtyAsync payload;
+
+payload.base.cmd = async_command_alloc(dispatcher, message, cookie);
+payload.surface_id = surface_id;
+payload.qxl_area = *qxl_area;
+payload.qxl_dirty_rects = qxl_dirty_rects;
+payload.num_dirty_rects = num_dirty_rects;
+payload.clear_dirty_region = clear_dirty_region;
+dispatcher_send_message(dispatcher-dispatcher,
+message,
+payload);
+}
+
  static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
 QXLRect *qxl_area, QXLRect 
*qxl_dirty_rects,
 uint32_t num_dirty_rects, uint32_t 
clear_dirty_region)
@@ -870,6 +892,17 @@ void spice_qxl_loadvm_commands(QXLInstance *instance, 
struct QXLCommandExt *ext,
  }

  SPICE_GNUC_VISIBLE
+void spice_qxl_update_area_dirty_async(QXLInstance *instance, uint32_t 
surface_id,
+   struct QXLRect *qxl_area, struct 
QXLRect *dirty_rects,
+   uint32_t num_dirty_rects, uint32_t 
clear_dirty_region,
+   uint64_t cookie)
+{
+red_dispatcher_update_area_dirty_async(instance-st-dispatcher, 
surface_id, qxl_area,
+   dirty_rects, num_dirty_rects, 
clear_dirty_region,
+   cookie);
+}
+
+SPICE_GNUC_VISIBLE
  void spice_qxl_update_area_async(QXLInstance *instance, uint32_t surface_id, 
QXLRect *qxl_area,
   uint32_t clear_dirty_region, uint64_t cookie)
  {
@@ -926,10 +959,11 @@ void red_dispatcher_async_complete(struct RedDispatcher 
*dispatcher,
  pthread_mutex_unlock(dispatcher-async_lock);
  switch (async_command-message) {
  case RED_WORKER_MESSAGE_UPDATE_ASYNC:
-break;
+case RED_WORKER_MESSAGE_UPDATE_DIRTY_ASYNC:
  case RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC:
-break;
  case RED_WORKER_MESSAGE_DESTROY_SURFACES_ASYNC:
+case RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC:
+case RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC:
  break;
  case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC:
  red_dispatcher_create_primary_surface_complete(dispatcher);
@@ -937,10 +971,6 @@ void red_dispatcher_async_complete(struct RedDispatcher 
*dispatcher,
  case RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE_ASYNC:
  red_dispatcher_destroy_primary_surface_complete(dispatcher);
  

Re: [Spice-devel] Performance expectations

[Yonit Halperin]

On 02/21/2012 07:49 PM, Kai Meyer wrote:



On 02/20/2012 05:19 AM, Yonit Halperin wrote:

Hi,
On 02/15/2012 06:22 PM, Kai Meyer wrote:



On 02/15/2012 05:19 AM, Yonit Halperin wrote:

On 02/15/2012 01:16 AM, Kai Meyer wrote:

Help me set my expectations straight. Should I be able to use spice
from
inside a VM on my local machine to view video streaming services like
Youtube, Hulu, and Netflix? I have RHEL 6 Workstation for my host, and
Win7 Pro as my guest. I've installed the qxl driver in the VM, and
used
Virt-manager to configure the VM to use Spice as my Display and Video
Model is qxl.


You should be able to do it. But it depends on you network conditions
(client to host).

Yonit.

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




This is a VM on my local machine, so I'm effectively connecting to
localhost. Network conditions should be the least concern, yet playback
is terrible.


Which spice client do you run?
Another possible reason for the slow playback is that Rhel 6 doesn't
have libjpeg-turbo.

Yonit.

-Kai Meyer




I'm using the spice client that comes in the default RHEL 6
repositories. libjpeg-turbo is not installed as a system library
anywhere I can find. If I am cpu-bound, it seems like it would be the
qemu process running the VM. My spicec process does not utilize more
than 50% of a cpu (as reported by top).

Can you please send the qemu log file (/var/log/libvirt/qemu/vm_name)?

Thanks,
Yonit.

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

Re: [Spice-devel] Performance expectations

[Yonit Halperin]

On 02/21/2012 09:56 PM, Kai Meyer wrote:



On 02/21/2012 12:34 PM, Yonit Halperin wrote:

On 02/21/2012 07:49 PM, Kai Meyer wrote:



On 02/20/2012 05:19 AM, Yonit Halperin wrote:

Hi,
On 02/15/2012 06:22 PM, Kai Meyer wrote:



On 02/15/2012 05:19 AM, Yonit Halperin wrote:

On 02/15/2012 01:16 AM, Kai Meyer wrote:

Help me set my expectations straight. Should I be able to use spice
from
inside a VM on my local machine to view video streaming services
like
Youtube, Hulu, and Netflix? I have RHEL 6 Workstation for my
host, and
Win7 Pro as my guest. I've installed the qxl driver in the VM, and
used
Virt-manager to configure the VM to use Spice as my Display and
Video
Model is qxl.


You should be able to do it. But it depends on you network conditions
(client to host).

Yonit.

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




This is a VM on my local machine, so I'm effectively connecting to
localhost. Network conditions should be the least concern, yet
playback
is terrible.


Which spice client do you run?
Another possible reason for the slow playback is that Rhel 6 doesn't
have libjpeg-turbo.

Yonit.

-Kai Meyer




I'm using the spice client that comes in the default RHEL 6
repositories. libjpeg-turbo is not installed as a system library
anywhere I can find. If I am cpu-bound, it seems like it would be the
qemu process running the VM. My spicec process does not utilize more
than 50% of a cpu (as reported by top).

Can you please send the qemu log file (/var/log/libvirt/qemu/vm_name)?

Thanks,
Yonit.


Ya, here's a pastebin:
http://pastebin.com/EaHRcYaG


Thanks,
Can you try the following:
- Use rtl8139 for the Nic model instead of virtio and compare to your 
previous experience?


- You can also try watch locally saved movie instead of using a web service

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

Re: [Spice-devel] RFC: remove Adler checksum and zlib header/trailer from zlib compressed images

[Yonit Halperin]

On 02/23/2012 08:46 PM, Yaniv Kaul wrote:

On 02/06/2012 09:43 AM, Yonit Halperin wrote:

On 02/06/2012 09:02 AM, Yaniv Kaul wrote:

On 01/29/2012 11:29 AM, Alon Levy wrote:

On Sun, Jan 29, 2012 at 09:28:34AM +0200, Yaniv Kaul wrote:

These small changes to server and (gtk) client seem to work, and
remove both the zlib header/trailer from the deflate stream as well
as the Adler checksum, which removes several bytes from the stream
as well as speedup (unnoticeable) of the compression/decompression
due to the removal of the checksum calculation.

Problem is - not sure how to test it - don't know how to get an
image once with and once without the patch - and ensure it's the
same image.
The fact that the first image is never zlib compressed complicates
things. I could not reliably get a 2nd image sent to the client
which looks the same as one in a different connection.

Nevertheless it does no harm - and images pass well - verified by
connecting with a modified Spice GTK client to a modified Spice
server, as well as looking at the network capture via wireshark.

It's of course missing the display channel specific capability
negotiation for this change.


I'm afraid I could not really find where to perform the display channel
specific capability negotiation. :(
Anyone?


Hi,

red_dispatcher.c/red_dispatcher_set_display_peer receives the client's
caps.
They should be sent to red_worker.c/handle_dev_display_connect (need
to change RedWorkerMessageDisplayConnect payload to contain them).
caps are tested via red_channel_client_test_remote_cap.
The negotiation should be in red_worker.c/handle_new_display_channel.

The display channel server caps should be set in
red_worker.c/display_channel_create via red_channel.set_cap.

Cheers,
Yonit.


1. Client: I could not find how to do it in spice-gtk. I assume it's in
gtk/channel-display.c , under spice_display_channel_init(), and I assume
via spice_channel_set_capability(), but I could not get a hold of a
SpiceChannel object to pass to that function. ?
You should use the macro SPIC 
spice_channel_set_capability(SPICE_CHANNEL(channel), cap)

You can see  an example in spice_main_channel_init

2. Server: looks like zlib is init'ed via red_worker.c/red_init_zlib(),
which is called from red_worker.c/red_worker_main() , so anything after
that is a bit too late. Unless I re-init zlib (which looks like a light
operation) on ever connect.

The zlib encoder can be moved from the worker to display channel client. 
With multi clients, different clients may have different caps. Btw, for 
now we encode each message for each client, instead of reusing the 
already encoded msg. We plan to change this, and than the encoder will 
be moved to groups of channel client that share the same encoders.
The initialization and release of the encoder can be in the 
connect/disconnect of the display channel client ( 
handle_new_display_channel/display_channel_client_on_disconnect).

Thanks,
Y.


TIA,

Y.


Comments are welcome.

Other then that adding a link to the docs page of
inlateInit2/deflateInit2 in the commit message would be nice for those
seeking to find the explanation for the magic numbers, no comment,
thanks for doing this!

If you want a repeatable test you can write one or use one of the
existing artificial tests in server/tests.


Client change:
diff --git a/gtk/decode-zlib.c b/gtk/decode-zlib.c
index a692020..997f350 100644
--- a/gtk/decode-zlib.c
+++ b/gtk/decode-zlib.c
@@ -63,7 +63,7 @@ SpiceZlibDecoder *zlib_decoder_new(void)
d-_z_strm.opaque = Z_NULL;
d-_z_strm.next_in = Z_NULL;
d-_z_strm.avail_in = 0;
- z_ret = inflateInit(d-_z_strm);
+ z_ret = inflateInit2(d-_z_strm, -15);
if (z_ret != Z_OK) {
g_warning(zlib decoder init failed, error %d, z_ret);
goto fail;


server change:
diff --git a/server/zlib_encoder.c b/server/zlib_encoder.c
index c51466b..66a039a 100644
--- a/server/zlib_encoder.c
+++ b/server/zlib_encoder.c
@@ -47,7 +47,7 @@ ZlibEncoder*
zlib_encoder_create(ZlibEncoderUsrContext *usr, int level)
enc-strm.zfree = Z_NULL;
enc-strm.opaque = Z_NULL;

- z_ret = deflateInit(enc-strm, level);
+ z_ret = deflateInit2(enc-strm, level, Z_DEFLATED, -15, 8,
Z_DEFAULT_STRATEGY);
enc-last_level = level;
if (z_ret != Z_OK) {
red_printf(zlib error);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


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






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

[Spice-devel] [PATCH 1/2] client: keyboard - add mapping for volume keys

[Yonit Halperin]

Add support for sending volume keys scancodes to the guest
RHBZ #552539

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 client/inputs_channel.cpp |3 +++
 client/red_key.h  |3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/client/inputs_channel.cpp b/client/inputs_channel.cpp
index b6f0220..c148eff 100644
--- a/client/inputs_channel.cpp
+++ b/client/inputs_channel.cpp
@@ -561,7 +561,10 @@ void InputsChannel::init_scan_table()
 init_escape_scan_code(REDKEY_ESCAPE_BASE);
 init_escape_scan_code(REDKEY_PAD_ENTER);
 init_escape_scan_code(REDKEY_R_CTRL);
+init_escape_scan_code(REDKEY_MUTE);
 init_escape_scan_code(REDKEY_FAKE_L_SHIFT);
+init_escape_scan_code(REDKEY_VOLUME_DOWN);
+init_escape_scan_code(REDKEY_VOLUME_UP);
 init_escape_scan_code(REDKEY_PAD_DIVIDE);
 init_escape_scan_code(REDKEY_FAKE_R_SHIFT);
 init_escape_scan_code(REDKEY_CTRL_PRINT_SCREEN);
diff --git a/client/red_key.h b/client/red_key.h
index ea3396a..3789c9a 100644
--- a/client/red_key.h
+++ b/client/red_key.h
@@ -121,7 +121,10 @@ enum RedKey {
 REDKEY_ESCAPE_BASE = 0x100,
 REDKEY_PAD_ENTER = REDKEY_ESCAPE_BASE + 0x1c,
 REDKEY_R_CTRL = REDKEY_ESCAPE_BASE + 0x1d,
+REDKEY_MUTE = REDKEY_ESCAPE_BASE + 0x20,
 REDKEY_FAKE_L_SHIFT = REDKEY_ESCAPE_BASE + 0x2a,
+REDKEY_VOLUME_DOWN = REDKEY_ESCAPE_BASE + 0x2e,
+REDKEY_VOLUME_UP = REDKEY_ESCAPE_BASE + 0x30,
 REDKEY_PAD_DIVIDE = REDKEY_ESCAPE_BASE + 0x35,
 REDKEY_FAKE_R_SHIFT = REDKEY_ESCAPE_BASE + 0x36,
 REDKEY_CTRL_PRINT_SCREEN = REDKEY_ESCAPE_BASE + 0x37,
-- 
1.7.7.6

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

[Spice-devel] [PATCH 2/2] client X11: support volume keys when evdev is in use

[Yonit Halperin]

Add support for sending volume keys scancodes to the guest
RHBZ #552539

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 client/x11/red_window.cpp |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/client/x11/red_window.cpp b/client/x11/red_window.cpp
index b16249e..fda90d5 100644
--- a/client/x11/red_window.cpp
+++ b/client/x11/red_window.cpp
@@ -186,6 +186,9 @@ enum EvdevKeyCode {
 EVDEV_KEYCODE_PAGE_DOWN,
 EVDEV_KEYCODE_INSERT,
 EVDEV_KEYCODE_DELETE,
+EVDEV_KEYCODE_MUTE = 121,
+EVDEV_KEYCODE_VOLUME_DOWN = 122,
+EVDEV_KEYCODE_VOLUME_UP = 123,
 EVDEV_KEYCODE_PAUSE = 127,
 EVDEV_KEYCODE_HANGUL = 130,
 EVDEV_KEYCODE_HANGUL_HANJA,
@@ -456,6 +459,9 @@ static void init_evdev_map()
 {
 #define KEYMAP(key_code, red_key)  keycode_map[EVDEV_##key_code] = red_key
 INIT_MAP;
+KEYMAP(KEYCODE_MUTE, REDKEY_MUTE);
+KEYMAP(KEYCODE_VOLUME_DOWN, REDKEY_VOLUME_DOWN);
+KEYMAP(KEYCODE_VOLUME_UP, REDKEY_VOLUME_UP);
 #undef KEYMAP
 }
 
-- 
1.7.7.6

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

Re: [Spice-devel] Cross-device access in SPICE server

[Yonit Halperin]

On 03/02/2012 09:06 PM, Noel Van Hook wrote:

I was wondering if anyone had any experience or insight into a way for
a worker in the server to get access to data from the other workers?
Specifically I am looking into some optimization that would require a
worker to have access to the surfaces stored in other workers.
Is there an easy way for the worker threads to access the other
workers?  I'm referring to the worker-surfaces[] array in
red_worker.c

Thanks,
Noel



Hi,

Currently the different workers share the pixmaps' cache (and also the 
glz dictionary). I.e., if an image is in use in more than one device, it 
will be sent to the client only once and will enter the shared cache.
The surfaces themselves are not shared, but we plan to change the driver 
to use only one device for multiple monitors, and then maybe this will 
change.
Can you be more specific about what you are trying to achieve? This will 
help to give you more details.


Regards,
Yonit.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Cross-device access in SPICE server

[Yonit Halperin]

On 03/05/2012 06:40 PM, Noel Van Hook wrote:

On Sun, Mar 4, 2012 at 12:29 PM, Yonit Halperinyhalp...@redhat.com  wrote:

On 03/02/2012 09:06 PM, Noel Van Hook wrote:


I was wondering if anyone had any experience or insight into a way for
a worker in the server to get access to data from the other workers?
Specifically I am looking into some optimization that would require a
worker to have access to the surfaces stored in other workers.
Is there an easy way for the worker threads to access the other
workers?  I'm referring to the worker-surfaces[] array in
red_worker.c

Thanks,
Noel



Hi,

Currently the different workers share the pixmaps' cache (and also the glz
dictionary). I.e., if an image is in use in more than one device, it will be
sent to the client only once and will enter the shared cache.
The surfaces themselves are not shared, but we plan to change the driver to
use only one device for multiple monitors, and then maybe this will change.
Can you be more specific about what you are trying to achieve? This will
help to give you more details.


We are trying to me the X driver XRandR compliant in such a way that
we don't break other drivers (such as the Windows driver).
We can collect all the heads together and make them look like a single
device, but copy operations that go from one device to the other give
us problems. Currently we are trying to modify the server to be able
to access a source surface which is located on another device.

Note that making the surfaces accessible from all workers (or display 
channels in the client side) is not enough, You should also be careful 
with synchronizing the workers: If you have a qxl-1 command that access 
surface S on qxl-2, all qxl-2 commands that preceded this qxl-1 command 
(and modified the corresponding are on surface S), should be executed 
before this qxl-1 command.


Yonit.

Noel




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

[Spice-devel] seamless migration with spice

[Yonit Halperin]

Hi,

We would like to implement seamless migration for Spice, i.e., keeping 
the currently opened spice client session valid after migration.
Today, the spice client establishes the connection to the destination 
before migration starts, and when migration completes, the client's 
session is moved to the destination, but all the session data is being 
reset.


We face 2 main challenges when coming to implement seamless migration:

(1) Spice client must establish the connection to the destination before 
the spice password expires. However, during migration, qemu main loop is 
not processed, and when migration completes, the password might have 
already expired.


Today we solve this by the async command client_migrate_info, which is 
expected to be called before migration starts. The command is completed

once spice client has connected to the destination (or a timeout).

Since async monitor commands are no longer supported, we are looking for 
a new solution.
The straightforward solution would be to process the main loop on the 
destination side during migration.


(2) In order to restore the source-client spice session in the 
destination, we need to pass data from the source to the destination.

Example for such data: in flight copy paste data, in flight usb data
We want to pass the data from the source spice server to the 
destination, via Spice client. This introduces a possible race: after 
migration completes, the source qemu can be killed before the 
spice-server completes transferring the migration data to the client.


Possible solutions:
- Have an async migration state notifiers. The migration state will 
change after all the notifiers complete callbacks are called.
- libvirt will wait for qmp event corresponding to spice completing its 
migration, and only then will kill the source qemu process.


Any thoughts?

Thanks,
Yonit.

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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi.
On 03/11/2012 05:36 PM, Anthony Liguori wrote:

On 03/11/2012 10:25 AM, Alon Levy wrote:

On Sun, Mar 11, 2012 at 09:18:17AM -0500, Anthony Liguori wrote:

On 03/11/2012 08:16 AM, Yonit Halperin wrote:

Hi,

We would like to implement seamless migration for Spice, i.e.,
keeping the
currently opened spice client session valid after migration.
Today, the spice client establishes the connection to the
destination before
migration starts, and when migration completes, the client's session
is moved to
the destination, but all the session data is being reset.

We face 2 main challenges when coming to implement seamless migration:

(1) Spice client must establish the connection to the destination
before the
spice password expires. However, during migration, qemu main loop is
not
processed, and when migration completes, the password might have
already expired.

Today we solve this by the async command client_migrate_info, which
is expected
to be called before migration starts. The command is completed
once spice client has connected to the destination (or a timeout).

Since async monitor commands are no longer supported, we are looking
for a new
solution.


We need to fix async monitor commands. Luiz sent a note our to
qemu-devel recently on this topic.

I'm not sure we'll get there for 1.1 but if we do a 3 month release
cycle for 1.2, then that's a pretty reasonable target IMHO.


What about the second part? it's independant of the async issue.


Isn't this a client problem? The client has this state, no?


No, part of the data is server specific.


If the state is stored in the server, wouldn't it be marshaled as part
of the server's migration state?

We currently don't restore the server state. That is the problem we want 
to solve.
I meant that the server state can be marshaled from the source to the 
client, and from the client to the destination. The client serves as the 
mediator.

Another option that we thought about was using save/load vmstate.

Regards,
Yonit.

I read that as the client needs to marshal it's own local state in the
session and restore it in the new session.





Regards,

Anthony Liguori





Regards,

Anthony Liguori


The straightforward solution would be to process the main loop on the
destination side during migration.

(2) In order to restore the source-client spice session in the
destination, we
need to pass data from the source to the destination.
Example for such data: in flight copy paste data, in flight usb data
We want to pass the data from the source spice server to the
destination, via
Spice client. This introduces a possible race: after migration
completes, the
source qemu can be killed before the spice-server completes
transferring the
migration data to the client.

Possible solutions:
- Have an async migration state notifiers. The migration state will
change after
all the notifiers complete callbacks are called.
- libvirt will wait for qmp event corresponding to spice completing its
migration, and only then will kill the source qemu process.

Any thoughts?

Thanks,
Yonit.








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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

On 03/12/2012 11:46 AM, Gerd Hoffmann wrote:

   Hi,


The problem with (b) is, that iirc the way b was implemented in the past
was still the big blob approach, but then pass the blob through the client,
which means an evil client could modify it, causing all sorts of
interesting
behavior inside spice-server. Since we're re-implementing this to me the
send a blob through the client approach is simply not acceptable from a
security pov, also see my previous mail in this thread.


Agree.  It should be a normal spice message which goes through the spice
marshaller for parsing  sanity checking.


I disagree. Note that there is more info to send over then just which
surfaces / images are cached by the client. There also is things like
partial complete agent channel messages, including how much bytes must
be read
to complete the command, etc.


Is there a complete list of the session state we need to save?


IMHO (b) would only be acceptable if the data send through the client stops
becoming a blob.


Using (a) to send a blob isn't better.


Gerd/Hans,

Can you explain/exemplify, why sending data as a blob (either by (a) or 
(b)), that is verified only by the two ends that actually use it, is a 
problem?
Lets say the client/qemu are completely aware of the migration data, 
what prevent it from harming it then?



Instead the client could simply send a list of all
surface ids,
etc. which it has cached after it connects to / starts using the new
host. Note
that the old hosts needs to send nothing for this, this is info the
client already
has, also removing the need for synchronization.


Yes, some session state is known to the client anyway so we don't need a
source-  target channel for them.


As for certain other
data, such
as (but not limited to) partially parsed agent messages, these should be
send through the regular vmstate methods IMHO.


That isn't easy to handle via vmstate, at least as soon as this goes
beyond a fixed number of fields aka 'migrate over this struct for me'.
Think multiple spice clients connected at the same time.


1) Do (a), sending everything that way
2) Do (a) sending non client state that way; and
let the client send state like which surfaces it has cached
when the new session starts.


I think we have to look at each piece of state information needed by the
target and look how to handle this best.

cheers,
   Gerd

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


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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi,
On 03/12/2012 03:50 PM, Gerd Hoffmann wrote:

   Hi,


Can you explain/exemplify, why sending data as a blob (either by (a) or
(b)), that is verified only by the two ends that actually use it, is a
problem?


It tends to be not very robust.  Especially when the creating/parsing is
done ad-hoc and the format changes now and then due to more info needing
to be stored later on.  The qemu migration format which has almost no
structure breaks now and then because of that.  Thus I'd prefer to not
go down this route when creating something new.

cheers,
   Gerd


Exposing spice server internals to the client/qemu seems to me more 
vulnerable then sending it as a blob. Nonetheless, it introduces more 
complexity to backward compatibility support and it will need to involve 
not only the capabilities/versions of the server but also those of the 
qemu/client. Which reminds me, that we also need capabilities 
negotiation for the migration protocol between the src and the destination.


Regards,
Yonit.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi,
On 03/13/2012 08:40 AM, Gerd Hoffmann wrote:

On 03/12/12 19:45, Yonit Halperin wrote:

Hi,
On 03/12/2012 03:50 PM, Gerd Hoffmann wrote:

Hi,


Can you explain/exemplify, why sending data as a blob (either by (a) or
(b)), that is verified only by the two ends that actually use it, is a
problem?


It tends to be not very robust.  Especially when the creating/parsing is
done ad-hoc and the format changes now and then due to more info needing
to be stored later on.  The qemu migration format which has almost no
structure breaks now and then because of that.  Thus I'd prefer to not
go down this route when creating something new.

cheers,
Gerd


Exposing spice server internals to the client/qemu seems to me more
vulnerable then sending it as a blob.


That also depends on what and how much we need to transfer.


Nonetheless, it introduces more
complexity to backward compatibility support and it will need to involve
not only the capabilities/versions of the server but also those of the
qemu/client


Backward compatibility isn't that easy both ways.

It is not easy when you have 2 components, and it is much less easy when 
you have 3 or 4 components. So why make it more complicated if you can 
avoid it. Especially since there is no functional reason for making the 
qemu/client capabilities/versions dependent on the server internal data.

.Which reminds me, that we also need capabilities
negotiation for the migration protocol between the src and the destination.


If this is a hard requirement then using the vmstate channel isn't going
to work.  The vmstate is a one-way channel, no way to negotiate anything
between source and target.


We can do this via the client.

Regards,
Yonit.

cheers,
   Gerd


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

[Spice-devel] log file for spice-gtk

[Yonit Halperin]

Hi,

Are there any plans to have a log file for spice-gtk?
I think that it is very important to have logging with configurable 
debug level via a configuration file. It will be helpful for live 
debugging of user problems that are hard to reproduce.


I also saw there was some discussion about .spicec directory. It is used 
by the old client for logs and also for holding the certificate files. 
(In windows machine it is in %APPDATA%/spicec)


Regards,
Yonit.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] log file for spice-gtk

[Yonit Halperin]

On 03/15/2012 01:25 PM, Marc-André Lureau wrote:

On Thu, Mar 15, 2012 at 11:39 AM, Alon Levyal...@redhat.com  wrote:

OK, so I think there is place to make remote-viewer produce a log file
by default.


The caller of virt-viewer/spice-gtk can redirect logging to files. By
experience, I'd say this is the right solution, as it avoid filling
disk or getting complicated logging (it's not difficult to get several
megabytes in several files...)

There is no need to dump everything. For this aim we should use log 
levels, which should be configurable via a configuration file or 
something similar.

1. For any hard to reproduce error there will be a log file by default.


What do you mean by default? There are very good reasons why logging
files aren't used, not even by default. There are tons of things
missing anyway, typically how/where/who compiled the project, the
gazillions of depedencies information and their own logging etc..
spice-gtk/remote-viewer probably account for 0.01% of the code that is
actually run.. If we want Spice domain logging, then we should dump
the traffic, at least we would be able to reproduce something
eventually..

And in the end, 99% of the bugs are solved with: backtrace +
reproducer. Not logging.

I beg to differ you. From my experience, spice client logs are very 
helpful. With the minimal information spice-client log holds, we could 
help many users. Sometimes you cannot reproduce problems at your own 
environment, and you can't ask the user to use backtrace.

2. Users who were used to spicec.log will have a remote-viewer.log


Why? Who is used to spicec.log? there isn't such thing anymore, and it
hasn't been specified what is spicec.log.
Well, there used to be such thing for a while, and we did use it. QE 
also knows about it...



I haven't noticed any easy way to set a fd different then stderr for
debugging output though, maybe someone can help?


By stderr redirection, if you really want to (for example, when the
reproducer is only on one machine)



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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

On 03/13/2012 09:40 AM, Gerd Hoffmann wrote:

   Hi,


It is not easy when you have 2 components, and it is much less easy when
you have 3 or 4 components. So why make it more complicated if you can
avoid it. Especially since there is no functional reason for making the
qemu/client capabilities/versions dependent on the server internal data.


qemu has ways to handle compatibility in the vmstate format.  We can use
those capabilities.  That of course requires exposing the structs to be
saved to qemu and adds some complexity to the qemu-  spice interface.

What session state is needed by the target?
What of this can be negotiated between client and target host without
bothering the source?
What needs be transfered from source to target, either directly or via
client?


If this is a hard requirement then using the vmstate channel isn't going
to work.  The vmstate is a one-way channel, no way to negotiate anything
between source and target.


We can do this via the client.


Then you can send the actual state via client too.
Out-of-band negotiation for the blob send via vmstate scares me.

Can we please start with a look at which state we actually have to send
over?

Ok, I can take the display and sound channels.
Alon, can you take the smartcard?
Hans, spicevmc?
Arnon, the main channel, mainly the agent stuff?

Thanks,
Yonit.



cheers,
   Gerd


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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi,
On 03/15/2012 02:36 PM, Hans de Goede wrote:

Hi,

On 03/15/2012 01:11 PM, Yonit Halperin wrote:

On 03/13/2012 09:40 AM, Gerd Hoffmann wrote:

Hi,


It is not easy when you have 2 components, and it is much less easy
when
you have 3 or 4 components. So why make it more complicated if you can
avoid it. Especially since there is no functional reason for making the
qemu/client capabilities/versions dependent on the server internal
data.


qemu has ways to handle compatibility in the vmstate format. We can use
those capabilities. That of course requires exposing the structs to be
saved to qemu and adds some complexity to the qemu- spice interface.

What session state is needed by the target?
What of this can be negotiated between client and target host without
bothering the source?
What needs be transfered from source to target, either directly or via
client?


If this is a hard requirement then using the vmstate channel isn't
going
to work. The vmstate is a one-way channel, no way to negotiate
anything
between source and target.


We can do this via the client.


Then you can send the actual state via client too.
Out-of-band negotiation for the blob send via vmstate scares me.

Can we please start with a look at which state we actually have to send
over?

Ok, I can take the display and sound channels.
Alon, can you take the smartcard?
Hans, spicevmc?


Easy, the spicevmc channel has no state which needs to be migrated, except
for things in the red_channel_client base class:

1) Partially received spice messages
2) Possible pending pipe items when writes from qemu - client have
blocked.

I assume that the red_channel_client base class will handle migrating them,
if we migrate them at all.

Instead of migrating we could:
For 1. expect the client to stop sending new messages at a certain point
during the migration, and ensure we've processed any pending messages
after this point.

For 2. we could flush pending items and set a flag to stop channel
implementations from queuing new ones, at which point for spicevmc the
data will get queued inside qemu and migrating it no longer is
a spice-server problem to migrate it (and we need migration support for
the data possibly queued inside qemu anyways).


We have an implementation for this: After migration had completed,
each spice-server channel sent MSG_MIGRATE to the corresponding client 
channel. The msg was sent after all the pending msgs to the client had 
already been sent.
In response, the client sent SPICE_MSGC_MIGRATE_FLUSH_MARK to the 
server, after it completed sending all its pending messages.
Then the blob data transfer and completion of socket switching has 
occurred.


Regarding the usb data in the server that should be flushed to qemu: we 
need to save it after the source vm is stopped. So I think it is too 
late for flushing it to qemu, unless you refereed to the special vmstate 
we will have for spice, if we go in that solution direction.


Cheers,
Yonit.

Regards,

Hans


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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi,
On 03/15/2012 04:23 PM, Hans de Goede wrote:

Hi,

On 03/15/2012 03:07 PM, Yonit Halperin wrote:

Hi,
On 03/15/2012 02:36 PM, Hans de Goede wrote:

Hi,

On 03/15/2012 01:11 PM, Yonit Halperin wrote:

On 03/13/2012 09:40 AM, Gerd Hoffmann wrote:

Hi,


It is not easy when you have 2 components, and it is much less easy
when
you have 3 or 4 components. So why make it more complicated if you
can
avoid it. Especially since there is no functional reason for
making the
qemu/client capabilities/versions dependent on the server internal
data.


qemu has ways to handle compatibility in the vmstate format. We can
use
those capabilities. That of course requires exposing the structs to be
saved to qemu and adds some complexity to the qemu- spice interface.

What session state is needed by the target?
What of this can be negotiated between client and target host without
bothering the source?
What needs be transfered from source to target, either directly or via
client?


If this is a hard requirement then using the vmstate channel isn't
going
to work. The vmstate is a one-way channel, no way to negotiate
anything
between source and target.


We can do this via the client.


Then you can send the actual state via client too.
Out-of-band negotiation for the blob send via vmstate scares me.

Can we please start with a look at which state we actually have to
send
over?

Ok, I can take the display and sound channels.
Alon, can you take the smartcard?
Hans, spicevmc?


Easy, the spicevmc channel has no state which needs to be migrated,
except
for things in the red_channel_client base class:

1) Partially received spice messages
2) Possible pending pipe items when writes from qemu - client have
blocked.

I assume that the red_channel_client base class will handle migrating
them,
if we migrate them at all.

Instead of migrating we could:
For 1. expect the client to stop sending new messages at a certain point
during the migration, and ensure we've processed any pending messages
after this point.

For 2. we could flush pending items and set a flag to stop channel
implementations from queuing new ones, at which point for spicevmc the
data will get queued inside qemu and migrating it no longer is
a spice-server problem to migrate it (and we need migration support for
the data possibly queued inside qemu anyways).


We have an implementation for this: After migration had completed,
each spice-server channel sent MSG_MIGRATE to the corresponding client
channel. The msg was sent after all the pending msgs to the client had
already been sent.
In response, the client sent SPICE_MSGC_MIGRATE_FLUSH_MARK to the
server, after it completed sending all its pending messages.


Yes, that is exactly what I thought we did but I was too lazy to check the
source :) So that would mean that other then assuring no data gets
queued up in spicevmc after sending the MSG_MIGRATE to the client (see
below),
no changes are needed to spicevmc, as it is essentially stateless.


Then the blob data transfer and completion of socket switching has
occurred.

Regarding the usb data in the server that should be flushed to qemu:
we need to save it after the source vm is stopped. So I think it is
too late for flushing it to qemu, unless you refereed to the special
vmstate we will have for spice, if we go in that solution direction.


I was talking about the other direction, so data queued in qemu which
should be flushed
to the server (and then forwarded to the client). IOW what I mean is that
after the spice-server channel sent MSG_MIGRATE, it should no longer
read data
from the qemu-chardev, even if it receives a chardev wakeup from qemu,
leaving the
data inside qemu to be migrated using qemu's standard migration mechanism.
However, since data from the client can reach spicevmc server channel 
after savevm was called, we will have to migrate this data by ourself.

Unless, we manage to flush the connection before savevm is performed.

Regards,
Yonit.



Regards,

Hans


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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi,
On 03/15/2012 04:07 PM, Yonit Halperin wrote:

Hi,
On 03/15/2012 02:36 PM, Hans de Goede wrote:

Hi,

On 03/15/2012 01:11 PM, Yonit Halperin wrote:

On 03/13/2012 09:40 AM, Gerd Hoffmann wrote:

Hi,


It is not easy when you have 2 components, and it is much less easy
when
you have 3 or 4 components. So why make it more complicated if you can
avoid it. Especially since there is no functional reason for making
the
qemu/client capabilities/versions dependent on the server internal
data.


qemu has ways to handle compatibility in the vmstate format. We can use
those capabilities. That of course requires exposing the structs to be
saved to qemu and adds some complexity to the qemu- spice interface.

What session state is needed by the target?
What of this can be negotiated between client and target host without
bothering the source?
What needs be transfered from source to target, either directly or via
client?


If this is a hard requirement then using the vmstate channel isn't
going
to work. The vmstate is a one-way channel, no way to negotiate
anything
between source and target.


We can do this via the client.


Then you can send the actual state via client too.
Out-of-band negotiation for the blob send via vmstate scares me.

Can we please start with a look at which state we actually have to send
over?

Ok, I can take the display and sound channels.

Display channel
---
(A) cache
Cache migration is a bit tricky since the cache is shared between the 
display channels, and each display channel can be in different state wrt 
migration. The possible states are: (1) Source still sends pending 
messages  (2) migration transition - messages are accumulate in the pipe 
(3) Dest send display messages.


We can either store and migrate the cache, or choose to reset it.
In the extinct spice seamless migration solution, the cache was reset. 
For implementing this approach, I think that the first display channel 
that handles migration can freeze the source cache, and send 
SPICE_MSG_DISPLAY_INVAL_ALL_PIXMAPS to the client (together with the 
corresponding wait list - i.e., other display channels' message 
serials we should wait for before resetting the cache).
In the old solution, resetting the client side cache was performed only 
after the channel that freezed the cached completely switched to the 
destination. This required migrating the wait list and the last 
message serial. Then, the freezer channel sent the 
SPICE_MSG_DISPLAY_INVAL_ALL_PIXMAPS with the MAX(migrated_wait_list, 
current_cache_wait_list_serial).
I'm not sure why the old solution initiated the reset from the 
destination and not from the source. Maybe for a case that for some 
reason the client stayed connected to the source and the vm was started 
on the source???


Of course, resetting the cache has the obvious consequence of resending 
images and rebuilding the cache.


If we choose to restore the complete cache on the destination side we 
need to:

(1) freeze the cache
(2) send the cache to the destination. The cache holds the ids of the 
images in stored in the client side cache, and the lru list of them.
In addition, for each such image we store the serial of the last message 
that accessed it from each display channel.

(3) start the destination cache in freeze mode
(4) Unfreeze the cache after it is restored from the migtation data.

In any case, the migration data should also hold the cache size (which 
is set by the client upon connection initialization).


(B) Glz dictionary
The dictionary is also shared between the display channels. It holds 
references to qxl images.
As in the old implementation, I think we should reset it after 
migration. Unlike the cache, the client doesn't need to know anything 
about it. The only date that should be migrated to the destination 
server are (1) the dictionary size (also set by the client upon connection)
(2) the last image id in the dictionary (otherwise we should have a 
message for resetting the dictionary on the client side).


(C) Surfaces:
Again, 2 options:
(1) Not migrate anything related to the client's off-screen surfaces. 
Consequence: we might send the client off-screen surfaces that we have 
already sent.
(2) Migrate the list of surfaces that the client holds and their lossy 
regions (or just the regions extents, for simplicity).



(D) In order to promise that in flight data from/to the src server won't 
get lost we still need to assure that the src server is not killed 
before spice completes its work - and then we are back to the original 
problem that started this thread. This is relevant to other channels as 
well, e.g., spicevmc.



Sound channels:
---
There is a 16K buffer in the record channel. However, since it can be 
overwritten by newer samples anyhow, I don't think it is necessary to 
migrate it.
The old solution migrated the record start time, and also the time its 
mode change (celt/raw), but I don't find any use for it.







Alon, can you

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi,
On 03/15/2012 04:23 PM, Hans de Goede wrote:

Hi,

On 03/15/2012 03:07 PM, Yonit Halperin wrote:

Hi,
On 03/15/2012 02:36 PM, Hans de Goede wrote:

Hi,

On 03/15/2012 01:11 PM, Yonit Halperin wrote:

On 03/13/2012 09:40 AM, Gerd Hoffmann wrote:

Hi,


It is not easy when you have 2 components, and it is much less easy
when
you have 3 or 4 components. So why make it more complicated if you
can
avoid it. Especially since there is no functional reason for
making the
qemu/client capabilities/versions dependent on the server internal
data.


qemu has ways to handle compatibility in the vmstate format. We can
use
those capabilities. That of course requires exposing the structs to be
saved to qemu and adds some complexity to the qemu- spice interface.

What session state is needed by the target?
What of this can be negotiated between client and target host without
bothering the source?
What needs be transfered from source to target, either directly or via
client?


If this is a hard requirement then using the vmstate channel isn't
going
to work. The vmstate is a one-way channel, no way to negotiate
anything
between source and target.


We can do this via the client.


Then you can send the actual state via client too.
Out-of-band negotiation for the blob send via vmstate scares me.

Can we please start with a look at which state we actually have to
send
over?

Ok, I can take the display and sound channels.
Alon, can you take the smartcard?
Hans, spicevmc?


Easy, the spicevmc channel has no state which needs to be migrated,
except
for things in the red_channel_client base class:

1) Partially received spice messages
2) Possible pending pipe items when writes from qemu - client have
blocked.

I assume that the red_channel_client base class will handle migrating
them,
if we migrate them at all.

Instead of migrating we could:
For 1. expect the client to stop sending new messages at a certain point
during the migration, and ensure we've processed any pending messages
after this point.

For 2. we could flush pending items and set a flag to stop channel
implementations from queuing new ones, at which point for spicevmc the
data will get queued inside qemu and migrating it no longer is
a spice-server problem to migrate it (and we need migration support for
the data possibly queued inside qemu anyways).


We have an implementation for this: After migration had completed,
each spice-server channel sent MSG_MIGRATE to the corresponding client
channel. The msg was sent after all the pending msgs to the client had
already been sent.
In response, the client sent SPICE_MSGC_MIGRATE_FLUSH_MARK to the
server, after it completed sending all its pending messages.


Yes, that is exactly what I thought we did but I was too lazy to check the
source :) So that would mean that other then assuring no data gets
queued up in spicevmc after sending the MSG_MIGRATE to the client (see
below),
no changes are needed to spicevmc, as it is essentially stateless.

Indeed, however, in order for this to work, we still need to make sure 
the source qemu is not killed before we complete this.


Cheers,
Yonit.


Then the blob data transfer and completion of socket switching has
occurred.

Regarding the usb data in the server that should be flushed to qemu:
we need to save it after the source vm is stopped. So I think it is
too late for flushing it to qemu, unless you refereed to the special
vmstate we will have for spice, if we go in that solution direction.


I was talking about the other direction, so data queued in qemu which
should be flushed
to the server (and then forwarded to the client). IOW what I mean is that
after the spice-server channel sent MSG_MIGRATE, it should no longer
read data
from the qemu-chardev, even if it receives a chardev wakeup from qemu,
leaving the
data inside qemu to be migrated using qemu's standard migration mechanism.

Regards,

Hans


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

Re: [Spice-devel] [bug spice-gtk] spice-gtk memory leak

[Yonit Halperin]

On 03/16/2012 08:13 PM, Marc-André Lureau wrote:



- Mensaje original -

Also, running spicy under valgrind leak check tool shows quite
clearly
that there is nothing being leaked directly from spice-gtk, it
seems.

As I told you off-line, It happened to me that remote-viewer
(spice-gtk client) ended up in 1.3GB, Unfortunately I am not able to
reproduce but I am getting 400-500MB and massif does not show
anything to me :-/.


Well, if you get 400Mb of resident memory, I would really like to see the 
massif profile, please send it!
I couldn't get it above 350Mb here, from which150Mb were image cache


Hi,

The default size of the cache is 80MB (20 Mega pixels). It shouldn't 
have reached 150MB. I've tracked the add/remove events from the cache, 
and it looks stable around 20 Mega pixels.

I don't manage to get more than 150MB resident memory for the remote-viewer.
Marian, can you describe how you manage to reach more the 400MB? 
Multiple monitors? How long was the remote viewer alive? What guest 
operations did you do? Does it happen if on the qemu side you set 
image-compression=quic (instead of auto-glz)?


Thanks,
Yonit.






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


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

Re: [Spice-devel] [Qemu-devel] seamless migration with spice

[Yonit Halperin]

Hi,
On 03/20/2012 03:58 PM, Gerd Hoffmann wrote:

   Hi,


We can either store and migrate the cache, or choose to reset it.
In the extinct spice seamless migration solution, the cache was reset.


Hmm, this makes me wonder what the main advantage of seamless migration
used to be?  image cache was reset, surfaces didn't exist back then.  So
any image data must be retransmitted anyway, correct?

I guess the main advantage for the display channel was that you could 
make sure all the pending messages from the source have reached the 
client, so you can continue the session with the destination without 
sending the client the primary surface.



For implementing this approach, I think that the first display channel
that handles migration can freeze the source cache, and send
SPICE_MSG_DISPLAY_INVAL_ALL_PIXMAPS to the client (together with the
corresponding wait list - i.e., other display channels' message
serials we should wait for before resetting the cache).


Looks sane to me.


In the old solution, resetting the client side cache was performed only
after the channel that freezed the cached completely switched to the
destination. This required migrating the wait list and the last
message serial. Then, the freezer channel sent the
SPICE_MSG_DISPLAY_INVAL_ALL_PIXMAPS with the MAX(migrated_wait_list,
current_cache_wait_list_serial).
I'm not sure why the old solution initiated the reset from the
destination and not from the source. Maybe for a case that for some
reason the client stayed connected to the source and the vm was started
on the source???


Could be.  Nowdays qemu informs spice-server whenever the migration was
successful or not, so this should not be needed any more.


If we choose to restore the complete cache on the destination side we
need to:
(1) freeze the cache
(2) send the cache to the destination. The cache holds the ids of the
images in stored in the client side cache, and the lru list of them.
In addition, for each such image we store the serial of the last message
that accessed it from each display channel.
(3) start the destination cache in freeze mode
(4) Unfreeze the cache after it is restored from the migtation data.


Ok.


In any case, the migration data should also hold the cache size (which
is set by the client upon connection initialization).


Why?  When the client sets it on connection initialization the dest host
should know it already ...

I think that when migrating the channels we can skip the initialization 
of the channels, and use the same setting.

about it. The only date that should be migrated to the destination
server are (1) the dictionary size (also set by the client upon connection)


Same here, not needed I think.


(2) the last image id in the dictionary (otherwise we should have a
message for resetting the dictionary on the client side).


A message would need to be simliar to
SPICE_MSG_DISPLAY_INVAL_ALL_PIXMAPS for display channel syncronization,
correct?

Yes, but it can be simpler for the dictionary, sending the last_image_id 
should be enough.

(C) Surfaces:
Again, 2 options:
(1) Not migrate anything related to the client's off-screen surfaces.
Consequence: we might send the client off-screen surfaces that we have
already sent.
(2) Migrate the list of surfaces that the client holds and their lossy
regions (or just the regions extents, for simplicity).


Do we have any stats on image / surface usage?

I'd expect image cache tends to hold short-living images, whereas
surfaces tends to hold long-living ones, so preserving surfaces is more
important than keeping the image cache.  That expectation isn't backed
by any real data through, and it probably also depends on the guest os
and driver version ...

Well, I wish it was. Unfortunately, almost all of the surfaces, both for 
Windows 7 and Linux guests, are short living. Moreover, a major part of 
the surfaces are not even used for any drawing on the primary surface, 
and just make us call update_area (instead of just rendering them on the 
guest, like it was when they were guest-managed bitmaps). Font-smoothing 
is one of the triggers of this behavior.
I hope this will change with Xrender and when we upgrade the Windows 
driver and support Dircet and 3D.

I have an old email with stats about surfaces. I'll send it to you.

(D) In order to promise that in flight data from/to the src server won't
get lost we still need to assure that the src server is not killed
before spice completes its work - and then we are back to the original
problem that started this thread.


I think this is needed no matter which way the migration state travels,
correct?

If we take the vmstate approach, we can use the pre_save callback for 
sending all the pending data, and post_load cb for completing the client 
switch from the src to the destination. But it will be ugly as long as 
these callbacks are synchronous (at least the pre_save one).


Regards,
Yonit.

cheers,
   Gerd



___

Re: [Spice-devel] [PATCH spice-gtk] RFC: always release shm of primary surfaces

[Yonit Halperin]

On 03/21/2012 08:53 PM, Marc-André Lureau wrote:

Note: I can't reproduce the leak, even after dozen of resolution switch.

Always delete shared memory segment of primary surfaces when
destroying its canvas.


Ack. Tested.
Just notice that it is not only for the primary surface, but for every 
surface.

---
  gtk/channel-display.c |6 +-
  1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index 152d6a7..8269705 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -705,6 +705,7 @@ static void destroy_canvas(display_surface *surface)
  #ifdef HAVE_SYS_SHM_H
  else {
  shmdt(surface-data);
+shmctl(surface-shmid, IPC_RMID, 0);
  }
  #endif
  surface-shmid = -1;
@@ -815,11 +816,6 @@ static void display_handle_mode(SpiceChannel *channel, 
SpiceMsgIn *in)
  surface-size= surface-height * surface-stride;
  surface-primary = true;
  create_canvas(channel, surface);
-#ifdef HAVE_SYS_SHM_H
-if (surface-shmid != -1) {
-shmctl(surface-shmid, IPC_RMID, 0);
-}
-#endif
So for very old spice-server and for the old qxl driver, there wouldn't 
have been a leak (since the primary surface was created with 
SPICE_MSG_DISPLAY_MODE msg, which is no longer used).

  }

  /* coroutine context */


Regards,
Yonit.

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

Re: [Spice-devel] SPICE_CHANNEL_TUNNEL

[Yonit Halperin]

Hi,
On 03/23/2012 11:59 AM, Charles.Tsai-蔡清海-研究發展部 wrote:

Alon,

I read the spice code and found a spice tunnel channel there. What is 
the purpose of the tunnel channel? Is it for network tunneling?
What is the use case for tunnel channel in spice?
The tunnel was intended for redirecting a virtual network in the guest 
to the client. Its main use was for network printer redirection.
The idea was to have a dedicated nic in the guest whose packets where 
handled by a library based on slirp (it is still downloadable from 
spice-space.org). Instead of using BSD sockets,  you could register 
other socket callbacks to this library, e.g., callbacks in the tunnel 
worker. These callbacks forwarded the connections meta data and the 
application layer of the packets to the client.


Regards,
Yonit.

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


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

[Spice-devel] [PATCH spice-protocol] controller: add COLOR_DEPTH and DISABLE_EFFECTS messages

[Yonit Halperin]

rhbz #787447

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 spice/controller_prot.h |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/spice/controller_prot.h b/spice/controller_prot.h
index a60b122..f7b1f26 100644
--- a/spice/controller_prot.h
+++ b/spice/controller_prot.h
@@ -75,6 +75,9 @@ enum {
 
 CONTROLLER_ENABLE_SMARTCARD,
 
+CONTROLLER_COLOR_DEPTH,
+CONTROLLER_DISABLE_EFFECTS,
+
 //spice client - extrenal app
 CONTROLLER_MENU_ITEM_CLICK = 1001,
 };
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-common] updated submodule spice-protocol

[Yonit Halperin]

added the disable-effects and color-depth options to the controller
Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 spice-protocol |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/spice-protocol b/spice-protocol
index 32da251..8cf92f0 16
--- a/spice-protocol
+++ b/spice-protocol
@@ -1 +1 @@
-Subproject commit 32da251a6572e3463cff040d106bb47a04e5a905
+Subproject commit 8cf92f042312e50b2ff186b28356053aeac9e04c
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-gtk] controller: add support for DISABLE_EFFECTS and COLOR_DEPTH

[Yonit Halperin]

rhbz #787449

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 data/spice-protocol.vapi   |3 +++
 gtk/controller/controller.vala |9 +
 spice-common   |2 +-
 3 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/data/spice-protocol.vapi b/data/spice-protocol.vapi
index 6626176..4b88175 100644
--- a/data/spice-protocol.vapi
+++ b/data/spice-protocol.vapi
@@ -64,6 +64,9 @@ namespace SpiceProtocol {
 
//spice client - external app
MENU_ITEM_CLICK,
+
+   COLOR_DEPTH,
+   DISABLE_EFFECTS,
}
 
[CCode (cname = unsigned int, cprefix = CONTROLLER_, 
has_type_id = false)]
diff --git a/gtk/controller/controller.vala b/gtk/controller/controller.vala
index c6cf984..0e35d0c 100644
--- a/gtk/controller/controller.vala
+++ b/gtk/controller/controller.vala
@@ -41,6 +41,8 @@ public class Controller: Object {
public SpiceCtrl.Menu? menu  { private set; get; }
public bool enable_smartcard { private set; get; }
public bool send_cad { private set; get; }
+   public string[] disable_effects {private set; get; }
+   public uint32 color_depth {private set; get; }
 
public signal void do_connect ();
public signal void show ();
@@ -142,6 +144,13 @@ public class Controller: Object {
hotkeys = str;
break;
 
+case SpiceProtocol.Controller.MsgId.COLOR_DEPTH:
+color_depth = v.value;
+break;
+case SpiceProtocol.Controller.MsgId.DISABLE_EFFECTS:
+disable_effects = str.split(,);
+break;
+
case SpiceProtocol.Controller.MsgId.CONNECT:
do_connect ();
break;
diff --git a/spice-common b/spice-common
index e3f6941..f3a0657 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit e3f6941895085c7138abcb49a98572ea1479ac1a
+Subproject commit f3a065706178d4087ba25cd1a7e28ae311b640f1
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-xpi] add support for DisableEffects and ColorDepth

[Yonit Halperin]

rhbz #747313

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 SpiceXPI/src/plugin/nsISpicec.idl|2 ++
 SpiceXPI/src/plugin/nsScriptablePeer.cpp |   16 +++-
 SpiceXPI/src/plugin/nsScriptablePeer.h   |2 ++
 SpiceXPI/src/plugin/plugin.cpp   |   26 ++
 SpiceXPI/src/plugin/plugin.h |   10 ++
 data/test.html   |   12 
 spice-protocol   |2 +-
 7 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/SpiceXPI/src/plugin/nsISpicec.idl 
b/SpiceXPI/src/plugin/nsISpicec.idl
index 5fc4a29..d3d0699 100644
--- a/SpiceXPI/src/plugin/nsISpicec.idl
+++ b/SpiceXPI/src/plugin/nsISpicec.idl
@@ -61,6 +61,8 @@ interface nsISpicec : nsISupports {
 attribute unsigned short UsbListenPort;
 attribute boolean UsbAutoShare;
 attribute boolean Smartcard;
+attribute string ColorDepth;
+attribute string DisableEffects;
 
 void connect();
 void show();
diff --git a/SpiceXPI/src/plugin/nsScriptablePeer.cpp 
b/SpiceXPI/src/plugin/nsScriptablePeer.cpp
index 394ced8..caab9b7 100644
--- a/SpiceXPI/src/plugin/nsScriptablePeer.cpp
+++ b/SpiceXPI/src/plugin/nsScriptablePeer.cpp
@@ -79,6 +79,8 @@ NPIdentifier 
ScriptablePluginObject::m_id_no_taskmgr_execution;
 NPIdentifier ScriptablePluginObject::m_id_send_ctrlaltdel;
 NPIdentifier ScriptablePluginObject::m_id_usb_listen_port;
 NPIdentifier ScriptablePluginObject::m_id_usb_auto_share;
+NPIdentifier ScriptablePluginObject::m_id_color_depth;
+NPIdentifier ScriptablePluginObject::m_id_disable_effects;
 NPIdentifier ScriptablePluginObject::m_id_connect;
 NPIdentifier ScriptablePluginObject::m_id_show;
 NPIdentifier ScriptablePluginObject::m_id_disconnect;
@@ -129,6 +131,8 @@ void ScriptablePluginObject::Init()
 m_id_send_ctrlaltdel = NPN_GetStringIdentifier(SendCtrlAltDelete);
 m_id_usb_listen_port = NPN_GetStringIdentifier(UsbListenPort);
 m_id_usb_auto_share = NPN_GetStringIdentifier(UsbAutoShare);
+m_id_color_depth = NPN_GetStringIdentifier(ColorDepth);
+m_id_disable_effects = NPN_GetStringIdentifier(DisableEffects);
 m_id_connect = NPN_GetStringIdentifier(connect);
 m_id_show = NPN_GetStringIdentifier(show);
 m_id_disconnect = NPN_GetStringIdentifier(disconnect);
@@ -170,7 +174,9 @@ bool ScriptablePluginObject::HasProperty(NPIdentifier name)
name == m_id_no_taskmgr_execution ||
name == m_id_send_ctrlaltdel ||
name == m_id_usb_listen_port ||
-   name == m_id_usb_auto_share);
+   name == m_id_usb_auto_share ||
+   name == m_id_color_depth ||
+   name == m_id_disable_effects);
 }
 
 bool ScriptablePluginObject::GetProperty(NPIdentifier name, NPVariant *result)
@@ -220,6 +226,10 @@ bool ScriptablePluginObject::GetProperty(NPIdentifier 
name, NPVariant *result)
 INT32_TO_NPVARIANT(m_plugin-GetUsbListenPort(), *result);
 else if (name == m_id_usb_auto_share)
 BOOLEAN_TO_NPVARIANT(m_plugin-GetUsbAutoShare(), *result);
+else if (name == m_id_color_depth)
+STRINGZ_TO_NPVARIANT(m_plugin-GetColorDepth(), *result);
+else if (name == m_id_disable_effects)
+STRINGZ_TO_NPVARIANT(m_plugin-GetDisableEffects(), *result);
 else
 return false;
 
@@ -296,6 +306,10 @@ bool ScriptablePluginObject::SetProperty(NPIdentifier 
name, const NPVariant *val
 m_plugin-SetUsbListenPort(val);
 else if (name == m_id_usb_auto_share)
 m_plugin-SetUsbAutoShare(boolean);
+else if (name == m_id_color_depth)
+m_plugin-SetColorDepth(str.c_str());
+else if (name == m_id_disable_effects)
+m_plugin-SetDisableEffects(str.c_str());
 else
 return false;
 
diff --git a/SpiceXPI/src/plugin/nsScriptablePeer.h 
b/SpiceXPI/src/plugin/nsScriptablePeer.h
index 469a05e..44bd53c 100644
--- a/SpiceXPI/src/plugin/nsScriptablePeer.h
+++ b/SpiceXPI/src/plugin/nsScriptablePeer.h
@@ -96,6 +96,8 @@ private:
 static NPIdentifier m_id_send_ctrlaltdel;
 static NPIdentifier m_id_usb_listen_port;
 static NPIdentifier m_id_usb_auto_share;
+static NPIdentifier m_id_color_depth;
+static NPIdentifier m_id_disable_effects;
 static NPIdentifier m_id_connect;
 static NPIdentifier m_id_show;
 static NPIdentifier m_id_disconnect;
diff --git a/SpiceXPI/src/plugin/plugin.cpp b/SpiceXPI/src/plugin/plugin.cpp
index 25a098b..5596609 100644
--- a/SpiceXPI/src/plugin/plugin.cpp
+++ b/SpiceXPI/src/plugin/plugin.cpp
@@ -225,6 +225,8 @@ NPBool nsPluginInstance::init(NPWindow *aWindow)
 m_dynamic_menu.clear();
 m_number_of_monitors.clear();
 m_guest_host_name.clear();
+m_color_depth.clear();
+m_disable_effects.clear();
 
 m_fullscreen = PR_FALSE;
 m_smartcard = PR_FALSE;
@@ -482,6 +484,28 @@ void nsPluginInstance::SetUsbAutoShare(PRBool 
aUsbAutoShare)
 // when fixed in RHEVM
 }
 
+/* attribute string ColorDepth; */
+char

[Spice-devel] [PATCH virt-viewer] spice/controller: add support for the properties disable-effects color-depth

[Yonit Halperin]

rhbz #808986

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 src/remote-viewer.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/src/remote-viewer.c b/src/remote-viewer.c
index 7dda7fe..5f7685f 100644
--- a/src/remote-viewer.c
+++ b/src/remote-viewer.c
@@ -583,7 +583,9 @@ spice_ctrl_notified(SpiceCtrlController *ctrl,
 g_str_equal(pspec-name, port) ||
 g_str_equal(pspec-name, password) ||
 g_str_equal(pspec-name, ca-file) ||
-g_str_equal(pspec-name, enable-smartcard)) {
+g_str_equal(pspec-name, enable-smartcard) ||
+g_str_equal(pspec-name, color-depth) ||
+g_str_equal(pspec-name, disable-effects)) {
 g_object_set_property(G_OBJECT(session), pspec-name, value);
 } else if (g_str_equal(pspec-name, sport)) {
 g_object_set_property(G_OBJECT(session), tls-port, value);
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-common 5/6] video streaming: add support for frames of different sizes

[Yonit Halperin]

Add SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, for stream_data message
that also contains the size and destination box of the data.
The server can send such messages only to clients with
SPICE_DISPLAY_CAP_SIZED_STREAM.

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 common/messages.h |   15 ++-
 spice.proto   |   19 ---
 spice1.proto  |8 ++--
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/common/messages.h b/common/messages.h
index 58e8bee..f7bc32a 100644
--- a/common/messages.h
+++ b/common/messages.h
@@ -307,13 +307,26 @@ typedef struct SpiceMsgDisplayStreamCreate {
 SpiceClip clip;
 } SpiceMsgDisplayStreamCreate;
 
-typedef struct SpiceMsgDisplayStreamData {
+typedef struct SpiceStreamDataHeader {
 uint32_t id;
 uint32_t multi_media_time;
+} SpiceStreamDataHeader;
+
+typedef struct SpiceMsgDisplayStreamData {
+SpiceStreamDataHeader base;
 uint32_t data_size;
 uint8_t data[0];
 } SpiceMsgDisplayStreamData;
 
+typedef struct SpiceMsgDisplayStreamDataSized {
+SpiceStreamDataHeader base;
+uint32_t width;
+uint32_t height;
+SpiceRect dest;
+uint32_t data_size;
+uint8_t data[0];
+} SpiceMsgDisplayStreamDataSized;
+
 typedef struct SpiceMsgDisplayStreamClip {
 uint32_t id;
 SpiceClip clip;
diff --git a/spice.proto b/spice.proto
index 513fe87..71be9ac 100644
--- a/spice.proto
+++ b/spice.proto
@@ -591,6 +591,11 @@ struct String {
 } u @anon;
 };
 
+struct StreamDataHeader {
+   uint32 id;
+   uint32 multi_media_time;
+};
+
 channel DisplayChannel : BaseChannel {
  server:
 message {
@@ -637,10 +642,9 @@ channel DisplayChannel : BaseChannel {
 } stream_create = 122;
 
 message {
-   uint32 id;
-   uint32 multi_media_time;
+   StreamDataHeader base;
uint32 data_size;
-   uint8 data[data_size] @end  @nomarshal;
+   uint8 data[data_size] @end @nomarshal;
 } stream_data;
 
 message {
@@ -785,6 +789,15 @@ channel DisplayChannel : BaseChannel {
uint32 surface_id;
 } @ctype(SpiceMsgSurfaceDestroy) surface_destroy;
 
+message {
+   StreamDataHeader base;
+   uint32 width;
+   uint32 height;
+   Rect dest;
+   uint32 data_size;
+   uint8 data[data_size] @end @nomarshal;
+} stream_data_sized;
+
  client:
 message {
uint8 pixmap_cache_id;
diff --git a/spice1.proto b/spice1.proto
index fa2524b..2ed1058 100644
--- a/spice1.proto
+++ b/spice1.proto
@@ -533,6 +533,11 @@ struct String {
 } u @anon;
 };
 
+struct StreamDataHeader {
+   uint32 id;
+   uint32 multi_media_time;
+};
+
 channel DisplayChannel : BaseChannel {
  server:
 message {
@@ -580,8 +585,7 @@ channel DisplayChannel : BaseChannel {
 } stream_create = 122;
 
 message {
-   uint32 id;
-   uint32 multi_media_time;
+   StreamDataHeader base;
uint32 data_size;
uint32 pad_size @zero;
uint8 data[data_size] @end  @nomarshal;
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 01/11] server/red_worker: fix dump_bitmap

[Yonit Halperin]

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 server/red_worker.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 07782c8..5350195 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, 
SpiceBitmap *bitmap, uint32_t group_i
 }
 /* writing the data */
 for (i = 0; i  bitmap-data-num_chunks; i++) {
+int j;
 SpiceChunk *chunk = bitmap-data-chunk[i];
 int num_lines = chunk-len / bitmap-stride;
-for (i = 0; i  num_lines; i++) {
-dump_line(f, chunk-data + (i * bitmap-stride), n_pixel_bits, 
bitmap-x, row_size);
+for (j = 0; j  num_lines; j++) {
+dump_line(f, chunk-data + (j * bitmap-stride), n_pixel_bits, 
bitmap-x, row_size);
 }
 }
 fclose(f);
-- 
1.7.7.6

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

Re: [Spice-devel] [PATCH spice-common 2/6] rect: add rect_get_area

[Yonit Halperin]

On 04/08/2012 07:36 PM, Marc-André Lureau wrote:



- Mensaje original -


Signed-off-by: Yonit Halperinyhalp...@redhat.com
---
  common/rect.h |   10 ++
  1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/common/rect.h b/common/rect.h
index 655e9e8..0e4b01e 100644
--- a/common/rect.h
+++ b/common/rect.h
@@ -80,6 +80,11 @@ static INLINE int rect_contains(const SpiceRect
*big, const SpiceRect *small)
 big-top= small-top  big-bottom= small-bottom;
  }

+static INLINE int rect_get_area(const SpiceRect *r)
+{
+return (r-right - r-left) * (r-bottom - r-top);
+}
+
  SPICE_END_DECLS

  #ifdef __cplusplus
@@ -124,6 +129,11 @@ static inline int rect_contains(const SpiceRect
big, const SpiceRect  small)
  return rect_contains(big,small);
  }

+static inline int rect_get_area(const SpiceRect  r)
+{
+return rect_contains(r);
+}


Looks like a typo.
Sorry, squashed the fix for this copy-paste bug into another patch by 
mistake. Will resend.



  #endif /* __cplusplus */

  #endif
--
1.7.7.6

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



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

Re: [Spice-devel] [PATCH spice-common 4/6] Update spice-protocol submodule

[Yonit Halperin]

On 04/08/2012 09:44 PM, Alon Levy wrote:

On Sun, Apr 08, 2012 at 06:42:36PM +0300, Yonit Halperin wrote:

For STREAM_DATA_SIZED and QOS_QUERY messages.

Signed-off-by: Yonit Halperinyhalp...@redhat.com
---
  common/rect.h  |2 +-
  spice-protocol |2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/rect.h b/common/rect.h
index 0e4b01e..a9c1b08 100644
--- a/common/rect.h
+++ b/common/rect.h
@@ -131,7 +131,7 @@ static inline int rect_contains(const SpiceRect  big, const 
SpiceRect  small)

  static inline int rect_get_area(const SpiceRect  r)
  {
-return rect_contains(r);
+return rect_get_area(r);


This isn't mentioned in the commit message.


squashed the fix for this copy-paste bug into another patch by mistake. 
Will resend.



  }

  #endif /* __cplusplus */
diff --git a/spice-protocol b/spice-protocol
index 2d24f61..3d53775 16
--- a/spice-protocol
+++ b/spice-protocol
@@ -1 +1 @@
-Subproject commit 2d24f61aae4f92746940fd1c0daf546c7a51dae8
+Subproject commit 3d53775685071891ee2ed253c620f57dc090711f
--
1.7.7.6

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


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

Re: [Spice-devel] [PATCH spice-common 4/6] Update spice-protocol submodule

[Yonit Halperin]

On 04/09/2012 09:49 AM, Hans de Goede wrote:

Hi,

On 04/08/2012 08:44 PM, Alon Levy wrote:

On Sun, Apr 08, 2012 at 06:42:36PM +0300, Yonit Halperin wrote:

For STREAM_DATA_SIZED and QOS_QUERY messages.

Signed-off-by: Yonit Halperinyhalp...@redhat.com
---
common/rect.h | 2 +-
spice-protocol | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/rect.h b/common/rect.h
index 0e4b01e..a9c1b08 100644
--- a/common/rect.h
+++ b/common/rect.h
@@ -131,7 +131,7 @@ static inline int rect_contains(const SpiceRect
big, const SpiceRect small)

static inline int rect_get_area(const SpiceRect r)
{
- return rect_contains(r);
+ return rect_get_area(r);


This isn't mentioned in the commit message.


And it looks like it turns this function into a function recursing into
itself, unless
we're relying on c++ function overloading here, but common is supposed
to be C-only.

Likewise the function prototype looks like it is using c++ conventions,
but again common
is supposed to be C only.

This file has
#ifdef __cplusplus where all the c functions are overloaded with 
reference data types. The c++ client use this.


Regards,

Hans


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

Re: [Spice-devel] [PATCH spice-protocol 2/2] Add qos related messages

[Yonit Halperin]

On 04/10/2012 12:13 PM, Marc-André Lureau wrote:

On Sun, Apr 8, 2012 at 5:42 PM, Yonit Halperinyhalp...@redhat.com  wrote:

If the client's channel has SPICE_COMMON_CAP_QOS_QUERY,
the server's channel can send SPICE_MSG_QOS_QUERY to the client.
In response, the client is expected to send back SPICE_MSG_QOS_ACK
immediately after it receives the message following the query, and
before handling the message. The server can deduce
the network condition using the ack arrival time.


Can it deduce other things than latency with this query?

You can deduce the bit rate, the ack is expected  only after receiving 
the message following the QOS_QUERY. So you can send the QOS_QUERY just 
before you are about to send a large message.


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

Re: [Spice-devel] [PATCH spice-protocol 1/2] video streaming: add support for frames of different sizes

[Yonit Halperin]

On 04/10/2012 12:22 PM, Marc-André Lureau wrote:

On Sun, Apr 8, 2012 at 5:42 PM, Yonit Halperinyhalp...@redhat.com  wrote:

Add SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, for stream_data message
that also contains the size and destination box of the data.
The server can send such messages only to clients with
SPICE_DISPLAY_CAP_SIZED_STREAM.


What is the image format, is it jpeg?


Yes, the same as all the stream.

Can you precise what width/height refer to?

The message contains the destination rectangle of the data, and the 
width and hight of the compressed image. I can add more details to the 
commit message.


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

Re: [Spice-devel] [PATCH spice-protocol 1/2] video streaming: add support for frames of different sizes

[Yonit Halperin]

On 04/10/2012 01:12 PM, Christophe Fergeau wrote:

On Sun, Apr 08, 2012 at 06:42:10PM +0300, Yonit Halperin wrote:

Add SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, for stream_data message
that also contains the size and destination box of the data.
The server can send such messages only to clients with
SPICE_DISPLAY_CAP_SIZED_STREAM.


Will every image of a video stream be sent with its width/height attached?
Or will width/height only be sent when it changes?
I sent SPICE_MSG_DISPLAY_STREAM_DATA_SIZED only for images whose 
destination box is bigger and contain the corresponding stream. The rest 
of the stream is sent as usual (via SPICE_MSG_DISPLAY_STREAM_DATA)


Yonit.


Christophe


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

Re: [Spice-devel] [PATCH spice 04/11] client: handle the new SpiceMsgDisplayStreamData (common/messages.h)

[Yonit Halperin]

On 04/10/2012 01:31 PM, Christophe Fergeau wrote:

On Sun, Apr 08, 2012 at 06:43:13PM +0300, Yonit Halperin wrote:

SpiceMsgDisplayStreamData now contains SpiceStreamDataHeader,
which is shared with SpiceMsgDisplayStreamDataSized.

Signed-off-by: Yonit Halperinyhalp...@redhat.com
---
  client/display_channel.cpp |6 --
  1 files changed, 4 insertions(+), 2 deletions(-)


I don't see a spice-protocol/spice-common update there, this means
compilation will be broken before or after this change.

The spice-common commit was part of patch 3 (d36f538). So the client 
build breaks after patch 3. I didn't want to put the sever  client 
patches in the same commit.

Christophe





diff --git a/client/display_channel.cpp b/client/display_channel.cpp
index ebeacd2..17bdf6a 100644
--- a/client/display_channel.cpp
+++ b/client/display_channel.cpp
@@ -1419,7 +1419,7 @@ void 
DisplayChannel::handle_stream_data(RedPeer::InMessage* message)
  SpiceMsgDisplayStreamData* stream_data = 
(SpiceMsgDisplayStreamData*)message-data();
  VideoStream* stream;

-if (stream_data-id= _streams.size() || !(stream = 
_streams[stream_data-id])) {
+if (stream_data-base.id= _streams.size() || !(stream = 
_streams[stream_data-base.id])) {
  THROW(invalid stream);
  }

@@ -1427,7 +1427,9 @@ void 
DisplayChannel::handle_stream_data(RedPeer::InMessage* message)
  THROW(access violation);
  }

-stream-push_data(stream_data-multi_media_time, stream_data-data_size, 
stream_data-data);
+stream-push_data(stream_data-base.multi_media_time,
+  stream_data-data_size,
+  stream_data-data);
  }

  void DisplayChannel::handle_stream_clip(RedPeer::InMessage* message)
--
1.7.7.6

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


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

Re: [Spice-devel] [PATCH spice 10/11] server/video-streams: adjust mjpeg quality and frame rate according to the current bit rate

[Yonit Halperin]

On 04/10/2012 03:48 PM, Christophe Fergeau wrote:

On Sun, Apr 08, 2012 at 06:43:19PM +0300, Yonit Halperin wrote:

Previously, the mjpeg quality was always 70. The frame rate was tuned
according to the frames' congestion in the pipe.
This patch sets the mjpeg quality and frame rate according
to the compressed size of the frames and the currently available bit
rate.
The compression size is estimated for different jpeg qualities,
and the bit rate is evaluated using qos queries (see red_channel).
The bit rate and compression size are monitored for major changes, and
when they occur, the mjpeg settings are re-evaluated.
In addition, the settings are fine-tuned correspondingly to the frames
pipe congestion.

Signed-off-by: Yonit Halperinyhalp...@redhat.com
---
  server/red_worker.c |  385 ++-
  1 files changed, 347 insertions(+), 38 deletions(-)


Sorry, but NACK because of the red_worker.c size increase and
$ wc -l red_worker.c
11341 red_worker.c

I'm under the impression that the 'jpeg' struct and the methods
manipulating it could be moved to a separate file.


Maybe I can move it to the mjpeg_encoder. But before I refactor this,
I would like to first get a review on the content itself.

Yonit.


Christophe



diff --git a/server/red_worker.c b/server/red_worker.c
index a9942cf..92e1197 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -114,7 +114,8 @@
  #define RED_STREAM_MIN_SIZE (96 * 96)

  #define FPS_TEST_INTERVAL 1
-#define MAX_FPS 30
+#define STREAM_MAX_FPS 30
+#define STREAM_MIN_FPS 1

  //best bit rate per pixel base on 1300 bps for frame size 720x576 pixels 
and 25 fps
  #define BEST_BIT_RATE_PER_PIXEL 38
@@ -396,6 +397,13 @@ struct Stream {
  int bit_rate;
  };

+#define STREAM_JPEG_QUALITY_SAMPLE_NUM 4
+static const int stream_jpeg_quality_samples[STREAM_JPEG_QUALITY_SAMPLE_NUM] = 
{15, 25, 50, 70};
+
+#define STREAM_FRAME_SIZE_CHANGE_TH 1.5
+#define STREAM_BIT_RATE_CHANGE_TH 1.25
+#define STREAM_AVERAGE_SIZE_WINDOW 3
+
  typedef struct StreamAgent {
  QRegion vis_region;
  PipeItem create_item;
@@ -403,6 +411,34 @@ typedef struct StreamAgent {
  Stream *stream;
  uint64_t last_send_time;

+/*
+  Adjusting the stream jpeg quality and frame rate (fps):
+  When during_sampling=TRUE, we compress different frames with different
+  jpeg quality. By using (1) the resulting compression ratio, (2) the 
current
+  channel bandwidth, and (3) the existance of other streams,
+  we evaulate the max frame frequency for the stream with the given 
quality,
+  and we choose the highest quality that will allow a reasonable frame 
rate.
+  during_sampling is set for new streams and also when the bandwidth and/or
+  average frame size significantly change.
+*/
+struct {
+int quality_id;
+uint64_t quality_sample_size[STREAM_JPEG_QUALITY_SAMPLE_NUM];
+int during_sampling;
+/* low limit for the the current sampling */
+int min_sample_quality_id;
+int min_sample_quality_fps; // min fps for the given quality
+/* tracking the best sampled fps so far */
+int max_sampled_fps;
+int max_sampled_fps_quality_id;
+int byte_rate;
+/* tracking the average frame size with the current jpeg quality */
+uint64_t size_sum;
+int size_summed_count;
+uint64_t recent_size_sum;
+int recent_size_summed_count;
+} jpeg;
+
  int frames;
  int drops;
  int fps;
@@ -938,6 +974,7 @@ typedef struct RedWorker {
  Ring streams;
  ItemTrace items_trace[NUM_TRACE_ITEMS];
  uint32_t next_item_trace;
+uint64_t streams_size_total;

  QuicData quic_data;
  QuicContext *quic;
@@ -2512,6 +2549,7 @@ static void red_attach_stream(RedWorker *worker, Drawable 
*drawable, Stream *str
  region_clone(agent-vis_region,drawable-tree_item.base.rgn);
  push_stream_clip_by_drawable(dcc, agent, drawable);
  }
+agent-frames++;
  }
  }

@@ -2522,6 +2560,7 @@ static void red_stop_stream(RedWorker *worker, Stream 
*stream)

  spice_assert(ring_item_is_linked(stream-link));
  spice_assert(!stream-current);
+spice_debug(id %ld, stream - worker-streams_buf);
  WORKER_FOREACH_DCC(worker, item, dcc) {
  StreamAgent *stream_agent;
  stream_agent =dcc-stream_agents[stream - worker-streams_buf];
@@ -2530,6 +2569,7 @@ static void red_stop_stream(RedWorker *worker, Stream 
*stream)
  stream-refs++;
  
red_channel_client_pipe_add(dcc-common.base,stream_agent-destroy_item);
  }
+worker-streams_size_total -= stream-width * stream-height;
  ring_remove(stream-link);
  red_release_stream(worker, stream);
  }
@@ -2741,11 +2781,10 @@ static int get_minimal_bit_rate(RedWorker *worker, int 
width, int height)
  return ret;
  }

-static void red_display_create_stream(DisplayChannelClient *dcc

Re: [Spice-devel] [PATCH spice 03/11] server: video streaming: add support for frames of different sizes

[Yonit Halperin]

Hi,
please wait with the review for this one. I may have a better way to 
identify such frames, using the current tree. If it works, I will send 
it in v2.


On 04/08/2012 06:43 PM, Yonit Halperin wrote:

When playing a youtube video on Windows guest, the driver sometimes sends
images which contain the video frames, but also other parts of the
screen (e.g., the youtube process bar). In order to prevent glitches, we send 
these
images as part of the stream, using SPICE_MSG_DISPLAY_STREAM_DATA_SIZED.

This patch includes and update to the spice-common submodule.

Signed-off-by: Yonit Halperinyhalp...@redhat.com
---
  server/mjpeg_encoder.c |   15 ++---
  server/mjpeg_encoder.h |3 +-
  server/red_worker.c|  183 +---
  spice-common   |2 +-
  4 files changed, 151 insertions(+), 52 deletions(-)

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 4692315..b3685f8 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -25,8 +25,6 @@
  #includejpeglib.h

  struct MJpegEncoder {
-int width;
-int height;
  uint8_t *row;
  int first_frame;
  int quality;
@@ -38,15 +36,13 @@ struct MJpegEncoder {
  void (*pixel_converter)(uint8_t *src, uint8_t *dest);
  };

-MJpegEncoder *mjpeg_encoder_new(int width, int height)
+MJpegEncoder *mjpeg_encoder_new()
  {
  MJpegEncoder *enc;

  enc = spice_new0(MJpegEncoder, 1);

  enc-first_frame = TRUE;
-enc-width = width;
-enc-height = height;
  enc-quality = 70;
  enc-cinfo.err = jpeg_std_error(enc-jerr);
  jpeg_create_compress(enc-cinfo);
@@ -200,6 +196,7 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
  /* end of code from libjpeg */

  int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
+  int width, int height,
uint8_t **dest, size_t *dest_len)
  {
  encoder-cinfo.in_color_space   = JCS_RGB;
@@ -233,9 +230,9 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, 
SpiceBitmapFmt format,
  }

  if ((encoder-pixel_converter != NULL)  (encoder-row == NULL)) {
-unsigned int stride = encoder-width * 3;
+unsigned int stride = width * 3;
  /* check for integer overflow */
-if (stride  encoder-width) {
+if (stride  width) {
  return FALSE;
  }
  encoder-row = spice_malloc(stride);
@@ -243,8 +240,8 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, 
SpiceBitmapFmt format,

  spice_jpeg_mem_dest(encoder-cinfo, dest, dest_len);

-encoder-cinfo.image_width  = encoder-width;
-encoder-cinfo.image_height = encoder-height;
+encoder-cinfo.image_width  = width;
+encoder-cinfo.image_height = height;
  jpeg_set_defaults(encoder-cinfo);
  encoder-cinfo.dct_method   = JDCT_IFAST;
  jpeg_set_quality(encoder-cinfo, encoder-quality, TRUE);
diff --git a/server/mjpeg_encoder.h b/server/mjpeg_encoder.h
index c43827f..3a005b7 100644
--- a/server/mjpeg_encoder.h
+++ b/server/mjpeg_encoder.h
@@ -23,11 +23,12 @@

  typedef struct MJpegEncoder MJpegEncoder;

-MJpegEncoder *mjpeg_encoder_new(int width, int height);
+MJpegEncoder *mjpeg_encoder_new();
  void mjpeg_encoder_destroy(MJpegEncoder *encoder);

  uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder *encoder);
  int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
+  int width, int height,
uint8_t **dest, size_t *dest_len);
  int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, uint8_t *src_pixels,
size_t image_width);
diff --git a/server/red_worker.c b/server/red_worker.c
index b2fa348..fd0e32e 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -374,6 +374,12 @@ typedef struct ImageItem {

  typedef struct Drawable Drawable;

+enum {
+STREAM_FRAME_NONE,
+STREAM_FRAME_NATIVE,
+STREAM_FRAME_CONTAINER,
+};
+
  typedef struct Stream Stream;
  struct Stream {
  uint8_t refs;
@@ -784,6 +790,7 @@ struct Drawable {
  int gradual_frames_count;
  int last_gradual_frame;
  Stream *stream;
+Stream *sized_stream;
  int streamable;
  BitmapGradualType copy_bitmap_graduality;
  uint32_t group_id;
@@ -2773,7 +2780,7 @@ static void red_create_stream(RedWorker *worker, Drawable 
*drawable)
  stream_width = src_rect-right - src_rect-left;
  stream_height = src_rect-bottom - src_rect-top;

-stream-mjpeg_encoder = mjpeg_encoder_new(stream_width, stream_height);
+stream-mjpeg_encoder = mjpeg_encoder_new();

  ring_add(worker-streams,stream-link);
  stream-current = drawable;
@@ -2850,34 +2857,63 @@ static inline int __red_is_next_stream_frame(RedWorker 
*worker,
   const int other_src_height,
   const SpiceRect *other_dest

Re: [Spice-devel] [PATCH spice-gtk] Set new cert-subject when switching host

[Yonit Halperin]

Ack
On 04/15/2012 10:32 PM, Marc-André Lureau wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=802574
---
  gtk/channel-main.c |9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index 8d0a809..9de51d7 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -1706,8 +1706,8 @@ static void main_handle_migrate_switch_host(SpiceChannel 
*channel, SpiceMsgIn *i
  g_return_if_fail(subject[mig-cert_subject_size - 1] == '\0');
  }

-SPICE_DEBUG(migrate_switch %s %d %d,
-host, mig-port, mig-sport);
+SPICE_DEBUG(migrate_switch %s %d %d %s,
+host, mig-port, mig-sport, subject);

  if (c-switch_host_delayed_id != 0) {
  g_warning(Switching host already in progress, aborting it);
@@ -1717,7 +1717,10 @@ static void main_handle_migrate_switch_host(SpiceChannel 
*channel, SpiceMsgIn *i

  session = spice_channel_get_session(channel);
  spice_session_set_migration_state(session, 
SPICE_SESSION_MIGRATION_SWITCHING);
-g_object_set(session, host, host, NULL);
+g_object_set(session,
+ host, host,
+ cert-subject, subject,
+ NULL);
  spice_session_set_port(session, mig-port, FALSE);
  spice_session_set_port(session, mig-sport, TRUE);



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

Re: [Spice-devel] [PATCH spice-gtk] Clear cache and glz dictionnary on switch-host

[Yonit Halperin]

Ack
On 04/15/2012 11:01 PM, Marc-André Lureau wrote:

If we don't clear the glz dictionnary, this might lead to
corrupted/invalid dictionnary and invalid memory allocation due
unbounded increase of dictionnary size
---
  gtk/spice-session.c |8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index d30d089..02b35f3 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -1122,9 +1122,9 @@ gboolean 
spice_session_get_client_provided_socket(SpiceSession *session)
  }

  G_GNUC_INTERNAL
-void spice_session_switching_disconnect(SpiceSession *session)
+void spice_session_switching_disconnect(SpiceSession *self)
  {
-SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
+SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(self);
  struct channel *item;
  RingItem *ring, *next;

@@ -1141,6 +1141,10 @@ void spice_session_switching_disconnect(SpiceSession 
*session)
  }

  g_warn_if_fail(!ring_is_empty(s-channels)); /* ring_get_length() == 1 */
+
+spice_session_palettes_clear(self);
+spice_session_images_clear(self);
+glz_decoder_window_clear(s-glz_window);
  }

  G_GNUC_INTERNAL


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

[Spice-devel] [PATCH v2 spice-protocol 0/2] video streaming enhancements and bandwidth monitoring

[Yonit Halperin]

Diff from v1:
- spice-protocol: more details in commit message
- spice-common:
- squashed the copy-paste fix for rect_get_area to the right patch
- removed the ring_add_tail patch
- spice:
- refactored the mjpeg quality/fps tuning; I moved it to mjpeg_encoder, and
  also made some small modifications to the tuning process.
- added more comments to the qos query timeout in red_channel
- spice_timer_queue: thread safr
- rearranged the patches that introduce STREAM_DATA_SIZED
- replaced sprintf with snprintf
- spice-gtk:
  update the spice-common submodule references

Regards,
Yonit.

Yonit Halperin (2):
  video streaming: add support for frames of different sizes
  Add qos related messages

 spice/enums.h|3 +++
 spice/protocol.h |5 +
 2 files changed, 8 insertions(+), 0 deletions(-)

-- 
1.7.7.6

Hi,

This patch series includes also patches for spice-common, spice, and spice-gtk.

Till now, the quality of the mjpeg stream was constant (70). However, when 
spice used ffmpeg's mjpeg,
it had employed its bit rate control, which eventually affected the stream 
quality.
This patch series introduces:
(1) Support for periodically monitoring the available bit rate of a channel.
The monitoring is based on the time it takes till a large enough message, 
that has been sent from 
the server, is received by the client (actually, till the time it takes the 
server to receive the ack for it). Thus,
this kind of monitoring suits channels that are used to pass large messages 
from the server to the client.
(2) Dynamically adjusting the video quality and frame rate, according to the 
available bit rate and the
current compression ratio for different jpeg qualities (might change 
significantly when the video scene
properties changes).
(3) Fix for playback glitches with youtube on different browsers in Windows7:
allow stream frames to be bigger and contain the original stream.


TODO:
- Measure latency. The current bandwidth monitoring will classify high-latency 
+ high-bandwidth connection as low-bandwidth ones.
- Remove the initial bandwidth measurement in the main_channel (PING/PONG).
- Remove red_worker.c unused bit-rate setting code, which was used for ffmpeg
- Video tearing in spice-gtk. Sounds like it might happen due to screen
  updates that take place at the same time the canvas is being updated.

I decided not to send it (yet?), but I also worked on monitoring the time it 
takes the client to ack windows of messages (with SPICE_MSGC_ACK), 
when those messages are sent relatively fluently, and when we aggregate several 
windows
if the total size of the messages is too small.
The time that passes since a window starts till we receive an ack for it 
includes both the 
preparation time of the messages in the server, and the time it takes to the 
client to process the messages.
Thus, comparing this time to the QOS_QUERY time, could have given us some sort 
of estimation for the
cpu state of the client and server. 

Yonit Halperin (2):
  video streaming: add support for frames of different sizes
  Add qos related messages

 spice/enums.h|3 +++
 spice/protocol.h |5 +
 2 files changed, 8 insertions(+), 0 deletions(-)

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

[Spice-devel] [PATCH v2 spice-protocol 1/2] video streaming: add support for frames of different sizes

[Yonit Halperin]

Add SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, for stream_data message
that in addition to the mjpeg data, also contains the
(1) width and height of the compressed frame.
(2) the destination box of the frame.
The server can send such messages only to clients with
SPICE_DISPLAY_CAP_SIZED_STREAM.

When playing a youtube video on Windows guest, the driver sometimes sends
images which contain the video frames, but also other parts of the
screen (e.g., the youtube process bar). In order to prevent glitches, we send 
these
images as part of the stream, using SPICE_MSG_DISPLAY_STREAM_DATA_SIZED.
---
 spice/enums.h|1 +
 spice/protocol.h |4 
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/spice/enums.h b/spice/enums.h
index d2dbfd0..802e1ee 100644
--- a/spice/enums.h
+++ b/spice/enums.h
@@ -415,6 +415,7 @@ enum {
 SPICE_MSG_DISPLAY_DRAW_ALPHA_BLEND,
 SPICE_MSG_DISPLAY_SURFACE_CREATE,
 SPICE_MSG_DISPLAY_SURFACE_DESTROY,
+SPICE_MSG_DISPLAY_STREAM_DATA_SIZED,
 
 SPICE_MSG_END_DISPLAY
 };
diff --git a/spice/protocol.h b/spice/protocol.h
index e3342cb..ceba2a1 100644
--- a/spice/protocol.h
+++ b/spice/protocol.h
@@ -122,6 +122,10 @@ enum {
 SPICE_MAIN_CAP_NAME_AND_UUID,
 };
 
+enum {
+SPICE_DISPLAY_CAP_SIZED_STREAM,
+};
+
 #include spice/end-packed.h
 
 #endif /* _H_SPICE_PROTOCOL */
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice-protocol 2/2] Add qos related messages

[Yonit Halperin]

If the client's channel has SPICE_COMMON_CAP_QOS_QUERY,
the server's channel can send SPICE_MSG_QOS_QUERY to the client.
In response, the client is expected to send back SPICE_MSG_QOS_ACK
immediately after it receives the message following the query, and
before handling this message. The server can deduce
the network condition using the ack arrival time.
---
 spice/enums.h|2 ++
 spice/protocol.h |1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/spice/enums.h b/spice/enums.h
index 802e1ee..601d799 100644
--- a/spice/enums.h
+++ b/spice/enums.h
@@ -343,6 +343,7 @@ enum {
 SPICE_MSG_DISCONNECTING,
 SPICE_MSG_NOTIFY,
 SPICE_MSG_LIST,
+SPICE_MSG_QOS_QUERY,
 };
 
 enum {
@@ -352,6 +353,7 @@ enum {
 SPICE_MSGC_MIGRATE_FLUSH_MARK,
 SPICE_MSGC_MIGRATE_DATA,
 SPICE_MSGC_DISCONNECTING,
+SPICE_MSGC_QOS_ACK,
 };
 
 enum {
diff --git a/spice/protocol.h b/spice/protocol.h
index ceba2a1..2ee551a 100644
--- a/spice/protocol.h
+++ b/spice/protocol.h
@@ -56,6 +56,7 @@ enum {
 SPICE_COMMON_CAP_AUTH_SPICE,
 SPICE_COMMON_CAP_AUTH_SASL,
 SPICE_COMMON_CAP_MINI_HEADER,
+SPICE_COMMON_CAP_QOS_QUERY,
 };
 
 typedef struct SPICE_ATTR_PACKED SpiceLinkMess {
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice-common 1/5] rect: add rect_contains

[Yonit Halperin]

---
 common/rect.h |   11 +++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/common/rect.h b/common/rect.h
index a63d785..655e9e8 100644
--- a/common/rect.h
+++ b/common/rect.h
@@ -74,6 +74,12 @@ static INLINE int rect_is_same_size(const SpiceRect *r1, 
const SpiceRect *r2)
r1-bottom - r1-top == r2-bottom - r2-top;
 }
 
+static INLINE int rect_contains(const SpiceRect *big, const SpiceRect *small)
+{
+return big-left = small-left  big-right = small-right 
+   big-top = small-top  big-bottom = small-bottom;
+}
+
 SPICE_END_DECLS
 
 #ifdef __cplusplus
@@ -113,6 +119,11 @@ static inline int rect_is_same_size(const SpiceRect r1, 
const SpiceRect r2)
 return rect_is_same_size(r1, r2);
 }
 
+static inline int rect_contains(const SpiceRect big, const SpiceRect small)
+{
+return rect_contains(big, small);
+}
+
 #endif /* __cplusplus */
 
 #endif
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice-common 2/5] rect: add rect_get_area

[Yonit Halperin]

---
 common/rect.h |   10 ++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/common/rect.h b/common/rect.h
index 655e9e8..a9c1b08 100644
--- a/common/rect.h
+++ b/common/rect.h
@@ -80,6 +80,11 @@ static INLINE int rect_contains(const SpiceRect *big, const 
SpiceRect *small)
big-top = small-top  big-bottom = small-bottom;
 }
 
+static INLINE int rect_get_area(const SpiceRect *r)
+{
+return (r-right - r-left) * (r-bottom - r-top);
+}
+
 SPICE_END_DECLS
 
 #ifdef __cplusplus
@@ -124,6 +129,11 @@ static inline int rect_contains(const SpiceRect big, 
const SpiceRect small)
 return rect_contains(big, small);
 }
 
+static inline int rect_get_area(const SpiceRect r)
+{
+return rect_get_area(r);
+}
+
 #endif /* __cplusplus */
 
 #endif
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice-common 4/5] video streaming: add support for frames of different sizes

[Yonit Halperin]

Add SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, for stream_data message
that also contains the size and destination box of the data.
The server can send such messages only to clients with
SPICE_DISPLAY_CAP_SIZED_STREAM.
---
 common/messages.h |   15 ++-
 spice.proto   |   19 ---
 spice1.proto  |8 ++--
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/common/messages.h b/common/messages.h
index 58e8bee..f7bc32a 100644
--- a/common/messages.h
+++ b/common/messages.h
@@ -307,13 +307,26 @@ typedef struct SpiceMsgDisplayStreamCreate {
 SpiceClip clip;
 } SpiceMsgDisplayStreamCreate;
 
-typedef struct SpiceMsgDisplayStreamData {
+typedef struct SpiceStreamDataHeader {
 uint32_t id;
 uint32_t multi_media_time;
+} SpiceStreamDataHeader;
+
+typedef struct SpiceMsgDisplayStreamData {
+SpiceStreamDataHeader base;
 uint32_t data_size;
 uint8_t data[0];
 } SpiceMsgDisplayStreamData;
 
+typedef struct SpiceMsgDisplayStreamDataSized {
+SpiceStreamDataHeader base;
+uint32_t width;
+uint32_t height;
+SpiceRect dest;
+uint32_t data_size;
+uint8_t data[0];
+} SpiceMsgDisplayStreamDataSized;
+
 typedef struct SpiceMsgDisplayStreamClip {
 uint32_t id;
 SpiceClip clip;
diff --git a/spice.proto b/spice.proto
index 513fe87..71be9ac 100644
--- a/spice.proto
+++ b/spice.proto
@@ -591,6 +591,11 @@ struct String {
 } u @anon;
 };
 
+struct StreamDataHeader {
+   uint32 id;
+   uint32 multi_media_time;
+};
+
 channel DisplayChannel : BaseChannel {
  server:
 message {
@@ -637,10 +642,9 @@ channel DisplayChannel : BaseChannel {
 } stream_create = 122;
 
 message {
-   uint32 id;
-   uint32 multi_media_time;
+   StreamDataHeader base;
uint32 data_size;
-   uint8 data[data_size] @end  @nomarshal;
+   uint8 data[data_size] @end @nomarshal;
 } stream_data;
 
 message {
@@ -785,6 +789,15 @@ channel DisplayChannel : BaseChannel {
uint32 surface_id;
 } @ctype(SpiceMsgSurfaceDestroy) surface_destroy;
 
+message {
+   StreamDataHeader base;
+   uint32 width;
+   uint32 height;
+   Rect dest;
+   uint32 data_size;
+   uint8 data[data_size] @end @nomarshal;
+} stream_data_sized;
+
  client:
 message {
uint8 pixmap_cache_id;
diff --git a/spice1.proto b/spice1.proto
index fa2524b..2ed1058 100644
--- a/spice1.proto
+++ b/spice1.proto
@@ -533,6 +533,11 @@ struct String {
 } u @anon;
 };
 
+struct StreamDataHeader {
+   uint32 id;
+   uint32 multi_media_time;
+};
+
 channel DisplayChannel : BaseChannel {
  server:
 message {
@@ -580,8 +585,7 @@ channel DisplayChannel : BaseChannel {
 } stream_create = 122;
 
 message {
-   uint32 id;
-   uint32 multi_media_time;
+   StreamDataHeader base;
uint32 data_size;
uint32 pad_size @zero;
uint8 data[data_size] @end  @nomarshal;
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice-common 5/5] Add qos related messages

[Yonit Halperin]

If the client's channel has SPICE_COMMON_CAP_QOS_QUERY,
the server's channel can send SPICE_MSG_QOS_QUERY to the client.
In response, the client is expected to send back SPICE_MSG_QOS_ACK
immediately after it receives the message following the query, and
before handling the message. The server can deduce
the network conditions using the ack arrival time.
---
 spice.proto |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/spice.proto b/spice.proto
index 71be9ac..4b622d5 100644
--- a/spice.proto
+++ b/spice.proto
@@ -135,6 +135,8 @@ channel BaseChannel {
 
 Data list; /* the msg body is SpiceSubMessageList */
 
+Empty qos_query;
+
  client:
 message {
uint32 generation;
@@ -155,6 +157,8 @@ channel BaseChannel {
uint64 time_stamp;
link_err reason;
 } @ctype(SpiceMsgDisconnect) disconnecting;
+
+Empty qos_ack;
 };
 
 struct ChannelId {
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice 01/11] server/red_worker: fix dump_bitmap

[Yonit Halperin]

---
 server/red_worker.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 07782c8..5350195 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, 
SpiceBitmap *bitmap, uint32_t group_i
 }
 /* writing the data */
 for (i = 0; i  bitmap-data-num_chunks; i++) {
+int j;
 SpiceChunk *chunk = bitmap-data-chunk[i];
 int num_lines = chunk-len / bitmap-stride;
-for (i = 0; i  num_lines; i++) {
-dump_line(f, chunk-data + (i * bitmap-stride), n_pixel_bits, 
bitmap-x, row_size);
+for (j = 0; j  num_lines; j++) {
+dump_line(f, chunk-data + (j * bitmap-stride), n_pixel_bits, 
bitmap-x, row_size);
 }
 }
 fclose(f);
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice 02/11] server/red_worker: dump_bitmap: add surface_id to the bitmap file name

[Yonit Halperin]

Also replaced sprintf with snprintf
---
 server/red_worker.c |   14 --
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 5350195..99a8948 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1014,7 +1014,7 @@ static void 
cursor_channel_client_release_item_after_push(CursorChannelClient *c
 static void red_wait_pipe_item_sent(RedChannelClient *rcc, PipeItem *item);
 
 #ifdef DUMP_BITMAP
-static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t 
group_id);
+static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t 
group_id, int surface_id);
 #endif
 
 /*
@@ -6368,7 +6368,9 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, 
SpiceMarshaller *m,
 case SPICE_IMAGE_TYPE_BITMAP: {
 SpiceBitmap *bitmap = image.u.bitmap;
 #ifdef DUMP_BITMAP
-dump_bitmap(display_channel-common.worker, simage-u.bitmap, 
drawable-group_id);
+dump_bitmap(display_channel-common.worker, simage-u.bitmap,
+drawable-group_id,
+drawable-surface_id);
 #endif
 /* Images must be added to the cache only after they are compressed
in order to prevent starvation in the client between pixmap_cache 
and
@@ -11235,12 +11237,12 @@ static void dump_line(FILE *f, uint8_t* line, 
uint16_t n_pixel_bits, int width,
 }
 
 #define RAM_PATH /tmp/tmpfs
-
-static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t 
group_id)
+#define DUMP_BITMAP_FILE_NAME_LEN 200
+static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t 
group_id, int surface_id)
 {
 static uint32_t file_id = 0;
+static char file_str[DUMP_BITMAP_FILE_NAME_LEN];
 
-char file_str[200];
 int rgb = TRUE;
 uint16_t n_pixel_bits;
 SpicePalette *plt = NULL;
@@ -11303,7 +11305,7 @@ static void dump_bitmap(RedWorker *worker, SpiceBitmap 
*bitmap, uint32_t group_i
 file_size = bitmap_data_offset + (bitmap-y * row_size);
 
 id = ++file_id;
-sprintf(file_str, %s/%u.bmp, RAM_PATH, id);
+sprintf(file_str, DUMP_BITMAP_FILE_NAME_LEN, %s/%u-%d.bmp, RAM_PATH, id, 
surface_id);
 
 f = fopen(file_str, wb);
 if (!f) {
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice 03/11] Update the spice-common submodule

[Yonit Halperin]

spice-common changes: STREAM_DATA_SIZED message was added in order to support
video streams with frames that their size is different from the initial size
that the stream was created with.

This patch also includes server and client adjustments to the new
SpiceMsgDisplayStreamData.
---
 client/display_channel.cpp |6 --
 server/red_worker.c|4 ++--
 spice-common   |2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/client/display_channel.cpp b/client/display_channel.cpp
index ebeacd2..17bdf6a 100644
--- a/client/display_channel.cpp
+++ b/client/display_channel.cpp
@@ -1419,7 +1419,7 @@ void 
DisplayChannel::handle_stream_data(RedPeer::InMessage* message)
 SpiceMsgDisplayStreamData* stream_data = 
(SpiceMsgDisplayStreamData*)message-data();
 VideoStream* stream;
 
-if (stream_data-id = _streams.size() || !(stream = 
_streams[stream_data-id])) {
+if (stream_data-base.id = _streams.size() || !(stream = 
_streams[stream_data-base.id])) {
 THROW(invalid stream);
 }
 
@@ -1427,7 +1427,9 @@ void 
DisplayChannel::handle_stream_data(RedPeer::InMessage* message)
 THROW(access violation);
 }
 
-stream-push_data(stream_data-multi_media_time, stream_data-data_size, 
stream_data-data);
+stream-push_data(stream_data-base.multi_media_time,
+  stream_data-data_size,
+  stream_data-data);
 }
 
 void DisplayChannel::handle_stream_clip(RedPeer::InMessage* message)
diff --git a/server/red_worker.c b/server/red_worker.c
index 99a8948..34e6a04 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -8063,8 +8063,8 @@ static inline int 
red_marshall_stream_data(RedChannelClient *rcc,
 
 SpiceMsgDisplayStreamData stream_data;
 
-stream_data.id = stream - worker-streams_buf;
-stream_data.multi_media_time = drawable-red_drawable-mm_time;
+stream_data.base.id = stream - worker-streams_buf;
+stream_data.base.multi_media_time = drawable-red_drawable-mm_time;
 stream_data.data_size = n;
 spice_marshall_msg_display_stream_data(base_marshaller, stream_data);
 spice_marshaller_add_ref(base_marshaller,
diff --git a/spice-common b/spice-common
index 1b41d15..005f433 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit 1b41d15a99dfcddb99975d17cfbcd61d8870a887
+Subproject commit 005f433769e90a4be32302cc90aca3d34919f9d5
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice 04/11] server: video streaming: add support for frames of different sizes

[Yonit Halperin]

When playing a youtube video on Windows guest, the driver sometimes sends
images which contain the video frames, but also other parts of the
screen (e.g., the youtube process bar). In order to prevent glitches, we send 
these
images as part of the stream, using SPICE_MSG_DISPLAY_STREAM_DATA_SIZED.
---
 server/mjpeg_encoder.c |   15 ++---
 server/mjpeg_encoder.h |3 +-
 server/red_worker.c|  183 +---
 3 files changed, 150 insertions(+), 51 deletions(-)

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 4692315..b3685f8 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -25,8 +25,6 @@
 #include jpeglib.h
 
 struct MJpegEncoder {
-int width;
-int height;
 uint8_t *row;
 int first_frame;
 int quality;
@@ -38,15 +36,13 @@ struct MJpegEncoder {
 void (*pixel_converter)(uint8_t *src, uint8_t *dest);
 };
 
-MJpegEncoder *mjpeg_encoder_new(int width, int height)
+MJpegEncoder *mjpeg_encoder_new()
 {
 MJpegEncoder *enc;
 
 enc = spice_new0(MJpegEncoder, 1);
 
 enc-first_frame = TRUE;
-enc-width = width;
-enc-height = height;
 enc-quality = 70;
 enc-cinfo.err = jpeg_std_error(enc-jerr);
 jpeg_create_compress(enc-cinfo);
@@ -200,6 +196,7 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
 /* end of code from libjpeg */
 
 int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
+  int width, int height,
   uint8_t **dest, size_t *dest_len)
 {
 encoder-cinfo.in_color_space   = JCS_RGB;
@@ -233,9 +230,9 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, 
SpiceBitmapFmt format,
 }
 
 if ((encoder-pixel_converter != NULL)  (encoder-row == NULL)) {
-unsigned int stride = encoder-width * 3;
+unsigned int stride = width * 3;
 /* check for integer overflow */
-if (stride  encoder-width) {
+if (stride  width) {
 return FALSE;
 }
 encoder-row = spice_malloc(stride);
@@ -243,8 +240,8 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, 
SpiceBitmapFmt format,
 
 spice_jpeg_mem_dest(encoder-cinfo, dest, dest_len);
 
-encoder-cinfo.image_width  = encoder-width;
-encoder-cinfo.image_height = encoder-height;
+encoder-cinfo.image_width  = width;
+encoder-cinfo.image_height = height;
 jpeg_set_defaults(encoder-cinfo);
 encoder-cinfo.dct_method   = JDCT_IFAST;
 jpeg_set_quality(encoder-cinfo, encoder-quality, TRUE);
diff --git a/server/mjpeg_encoder.h b/server/mjpeg_encoder.h
index c43827f..3a005b7 100644
--- a/server/mjpeg_encoder.h
+++ b/server/mjpeg_encoder.h
@@ -23,11 +23,12 @@
 
 typedef struct MJpegEncoder MJpegEncoder;
 
-MJpegEncoder *mjpeg_encoder_new(int width, int height);
+MJpegEncoder *mjpeg_encoder_new();
 void mjpeg_encoder_destroy(MJpegEncoder *encoder);
 
 uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder *encoder);
 int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
+  int width, int height,
   uint8_t **dest, size_t *dest_len);
 int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, uint8_t *src_pixels,
   size_t image_width);
diff --git a/server/red_worker.c b/server/red_worker.c
index 34e6a04..c66e397 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -374,6 +374,12 @@ typedef struct ImageItem {
 
 typedef struct Drawable Drawable;
 
+enum {
+STREAM_FRAME_NONE,
+STREAM_FRAME_NATIVE,
+STREAM_FRAME_CONTAINER,
+};
+
 typedef struct Stream Stream;
 struct Stream {
 uint8_t refs;
@@ -784,6 +790,7 @@ struct Drawable {
 int gradual_frames_count;
 int last_gradual_frame;
 Stream *stream;
+Stream *sized_stream;
 int streamable;
 BitmapGradualType copy_bitmap_graduality;
 uint32_t group_id;
@@ -2773,7 +2780,7 @@ static void red_create_stream(RedWorker *worker, Drawable 
*drawable)
 stream_width = src_rect-right - src_rect-left;
 stream_height = src_rect-bottom - src_rect-top;
 
-stream-mjpeg_encoder = mjpeg_encoder_new(stream_width, stream_height);
+stream-mjpeg_encoder = mjpeg_encoder_new();
 
 ring_add(worker-streams, stream-link);
 stream-current = drawable;
@@ -2850,34 +2857,63 @@ static inline int __red_is_next_stream_frame(RedWorker 
*worker,
  const int other_src_height,
  const SpiceRect *other_dest,
  const red_time_t other_time,
- const Stream *stream)
+ const Stream *stream,
+ int container_candidate_allowed)
 {
 RedDrawable *red_drawable;
+int is_frame_container = FALSE;
 
 if (candidate-creation_time - 

[Spice-devel] [PATCH v2 spice 06/11] server/red_worker: assign timer callbacks to worker_core, using spice_timer_queue

[Yonit Halperin]

display channel: Supplying timeouts interface to red_channel, in order to allow
periodic network monitoring (see next 2 patches).
---
 server/red_worker.c |   14 ++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index c66e397..7c41e08 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -78,6 +78,7 @@
 #include red_dispatcher.h
 #include dispatcher.h
 #include main_channel.h
+#include spice_timer_queue.h
 
 //#define COMPRESS_STAT
 //#define DUMP_BITMAP
@@ -9704,6 +9705,11 @@ static void worker_watch_remove(SpiceWatch *watch)
 }
 
 SpiceCoreInterface worker_core = {
+.timer_add = spice_timer_queue_add,
+.timer_start = spice_timer_set,
+.timer_cancel = spice_timer_cancel,
+.timer_remove = spice_timer_remove,
+
 .watch_update_mask = worker_watch_update_mask,
 .watch_add = worker_watch_add,
 .watch_remove = worker_watch_remove,
@@ -11224,6 +11230,10 @@ static void red_init(RedWorker *worker, WorkerInitData 
*init_data)
 spice_warn_if(init_data-n_surfaces  NUM_SURFACES);
 worker-n_surfaces = init_data-n_surfaces;
 
+if (!spice_timer_queue_create()) {
+spice_error(failed to create timer queue);
+}
+
 message = RED_WORKER_MESSAGE_READY;
 write_message(worker-channel, message);
 }
@@ -11257,10 +11267,14 @@ void *red_worker_main(void *arg)
 worker.event_timeout = INF_EVENT_WAIT;
 for (;;) {
 int i, num_events;
+unsigned int timers_queue_timeout;
 
+timers_queue_timeout = spice_timer_queue_get_timeout_ms();
 worker.event_timeout = MIN(red_get_streams_timout(worker), 
worker.event_timeout);
+worker.event_timeout = MIN(timers_queue_timeout, worker.event_timeout);
 num_events = poll(worker.poll_fds, MAX_EVENT_SOURCES, 
worker.event_timeout);
 red_handle_streams_timout(worker);
+spice_timer_queue_cb();
 
 if (worker.display_channel) {
 /* during migration, in the dest, the display channel can be 
initialized
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice 07/11] server/red_channel: support network monitoring

[Yonit Halperin]

If the client's channel has SPICE_COMMON_CAP_QOS_QUERY, the server
channel's can send it SPICE_MSG_QOS_QUERY. The client is supposed to
respond by SPICE_MSG_QOS_ACK right after it receives the one message
that follows SPICE_MSG_QOS_QUERY.
For channels that are characterized mainly by transmissions from the
server to the client, the time it takes to send large enough messages
can be used to calculate the current available bandwidth of the channel.

This patch also contains the corresponding update to the spice-common
submodule.
---
 server/inputs_channel.c|1 +
 server/main_channel.c  |5 +-
 server/red_channel.c   |  122 
 server/red_channel.h   |   23 
 server/red_tunnel_worker.c |1 +
 server/red_worker.c|2 +-
 server/smartcard.c |1 +
 server/spicevmc.c  |1 +
 spice-common   |2 +-
 9 files changed, 154 insertions(+), 4 deletions(-)

diff --git a/server/inputs_channel.c b/server/inputs_channel.c
index ad247f4..cc0db94 100644
--- a/server/inputs_channel.c
+++ b/server/inputs_channel.c
@@ -500,6 +500,7 @@ static void inputs_connect(RedChannel *channel, RedClient 
*client,
   channel,
   client,
   stream,
+  FALSE,
   num_common_caps, 
common_caps,
   num_caps, caps);
 icc-motion_count = 0;
diff --git a/server/main_channel.c b/server/main_channel.c
index 713f121..624b56d 100644
--- a/server/main_channel.c
+++ b/server/main_channel.c
@@ -996,8 +996,9 @@ static MainChannelClient 
*main_channel_client_create(MainChannel *main_chan, Red
 {
 MainChannelClient *mcc = (MainChannelClient*)
  
red_channel_client_create(sizeof(MainChannelClient), main_chan-base,
-   client, stream, 
num_common_caps,
-   common_caps, num_caps, 
caps);
+   client, stream, FALSE,
+   num_common_caps, 
common_caps,
+   num_caps, caps);
 
 mcc-connection_id = connection_id;
 mcc-bitrate_per_sec = ~0;
diff --git a/server/red_channel.c b/server/red_channel.c
index 4858bb5..303f0f7 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -24,11 +24,15 @@
 
 #include stdio.h
 #include stdint.h
+#include sys/ioctl.h
+#include sys/socket.h
 #include netinet/in.h
 #include netinet/tcp.h
 #include fcntl.h
 #include unistd.h
 #include errno.h
+#include time.h
+#include spice/protocol.h
 
 #include common/generated_server_marshallers.h
 #include common/ring.h
@@ -37,6 +41,10 @@
 #include red_channel.h
 #include reds.h
 
+#define NET_MONITOR_TIMEOUT_MS 15000
+#define NET_MONITOR_QOS_QUERY_SIZE 10
+#define NET_MONITOR_QOS_QUERY_SIZE_MIN 25000
+
 static void red_channel_client_event(int fd, int event, void *data);
 static void red_client_add_channel(RedClient *client, RedChannelClient *rcc);
 static void red_client_remove_channel(RedChannelClient *rcc);
@@ -514,8 +522,33 @@ int red_channel_client_test_remote_cap(RedChannelClient 
*rcc, uint32_t cap)
   cap);
 }
 
+static void red_channel_client_net_monitor_qos_timer(void *opaque)
+{
+RedChannelClient *rcc = opaque;
+int *qos_state = rcc-net_monitor.qos.state;
+
+spice_assert(*qos_state != NET_MONITOR_STATE_INVALID);
+
+if (*qos_state == NET_MONITOR_STATE_COMPLETE) {
+*qos_state = NET_MONITOR_STATE_PENDING;
+rcc-net_monitor.qos.size_threshold = NET_MONITOR_QOS_QUERY_SIZE;
+} else {
+spice_assert(*qos_state == NET_MONITOR_STATE_PENDING);
+/*
+ * We have reached the timeout without passing the size threshold.
+ * We decrease the size threshold and timeout period, in order not to 
wait
+ * too long till a qos query is possible.
+ */
+rcc-net_monitor.qos.size_threshold = 
MAX(NET_MONITOR_QOS_QUERY_SIZE_MIN,
+   
(rcc-net_monitor.qos.size_threshold - 1));
+rcc-channel-core-timer_start(rcc-net_monitor.qos.timer, 
NET_MONITOR_TIMEOUT_MS / 2);
+spice_debug(size_threshold=%d, rcc-net_monitor.qos.size_threshold);
+}
+}
+
 RedChannelClient *red_channel_client_create(int size, RedChannel *channel, 
RedClient  *client,
 RedsStream *stream,
+int monitor_net,
 int num_common_caps, uint32_t 
*common_caps,
  

[Spice-devel] [PATCH v2 spice 08/11] server/red_worker: enable periodic network monitoring in the display channel

[Yonit Halperin]

---
 server/red_worker.c |5 -
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 2de96d2..1e74662 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -9719,6 +9719,7 @@ static CommonChannelClient 
*common_channel_client_create(int size,
  CommonChannel *common,
  RedClient *client,
  RedsStream *stream,
+ int monitor_network,
  uint32_t *common_caps,
  int num_common_caps,
  uint32_t *caps,
@@ -9726,7 +9727,7 @@ static CommonChannelClient 
*common_channel_client_create(int size,
 {
 MainChannelClient *mcc = red_client_get_main(client);
 RedChannelClient *rcc =
-red_channel_client_create(size, common-base, client, stream, FALSE,
+red_channel_client_create(size, common-base, client, stream, 
monitor_network,
   num_common_caps, common_caps, num_caps, 
caps);
 CommonChannelClient *common_cc = (CommonChannelClient*)rcc;
 common_cc-worker = common-worker;
@@ -9748,6 +9749,7 @@ DisplayChannelClient 
*display_channel_client_create(CommonChannel *common,
 DisplayChannelClient *dcc =
 (DisplayChannelClient*)common_channel_client_create(
 sizeof(DisplayChannelClient), common, client, stream,
+TRUE,
 common_caps, num_common_caps,
 caps, num_caps);
 
@@ -9767,6 +9769,7 @@ CursorChannelClient 
*cursor_channel_create_rcc(CommonChannel *common,
 CursorChannelClient *ccc =
 (CursorChannelClient*)common_channel_client_create(
 sizeof(CursorChannelClient), common, client, stream,
+FALSE,
 common_caps,
 num_common_caps,
 caps,
-- 
1.7.7.6

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

[Spice-devel] [PATCH v2 spice-gtk 3/3] display: debug video streams frames dropping

[Yonit Halperin]

---
 gtk/channel-display-priv.h |5 +
 gtk/channel-display.c  |   19 +++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/gtk/channel-display-priv.h b/gtk/channel-display-priv.h
index 1969697..8aac8d8 100644
--- a/gtk/channel-display-priv.h
+++ b/gtk/channel-display-priv.h
@@ -71,6 +71,11 @@ typedef struct display_stream {
 GQueue  *msgq;
 guint   timeout;
 SpiceChannel*channel;
+uint32_tnum_drops_arrival;
+uint64_tdrop_arrival_late_time;
+uint32_tnum_drops_play;
+uint64_tdrop_play_late_time;
+uint32_tnum_total_frames;
 } display_stream;
 
 void stream_get_dimensions(display_stream *st, int *width, int *height);
diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index a8e830b..b57e04f 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -981,6 +981,8 @@ static gboolean display_stream_schedule(display_stream *st)
 st-timeout = g_timeout_add(d, (GSourceFunc)display_stream_render, st);
 return TRUE;
 } else {
+st-num_drops_play++;
+st-drop_play_late_time += time - op-multi_media_time;
 in = g_queue_pop_head(st-msgq);
 spice_msg_in_unref(in);
 if (g_queue_get_length(st-msgq) == 0)
@@ -1137,7 +1139,11 @@ static void display_handle_stream_data(SpiceChannel 
*channel, SpiceMsgIn *in)
 op-multi_media_time = mmtime + 100; /* workaround... */
 }
 
+st-num_total_frames++;
+
 if (op-multi_media_time  mmtime) {
+st-num_drops_arrival++;
+st-drop_arrival_late_time += mmtime - op-multi_media_time;
 SPICE_DEBUG(stream data too late by %u ms (ts: %u, mmtime: %u), 
dropin,
 mmtime - op-multi_media_time, op-multi_media_time, 
mmtime);
 return;
@@ -1173,6 +1179,7 @@ static void destroy_stream(SpiceChannel *channel, int id)
 {
 SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)-priv;
 display_stream *st;
+int num_played_frames;
 
 g_return_if_fail(c != NULL);
 g_return_if_fail(c-streams != NULL);
@@ -1181,6 +1188,18 @@ static void destroy_stream(SpiceChannel *channel, int id)
 if (!st)
 return;
 
+num_played_frames = st-num_total_frames - st-num_drops_arrival - 
st-num_drops_play;
+SPICE_DEBUG(strem id %d late-arrival drops %d (avg late time %.2f),
+late-play drops %d (avg late time %.2f), played %f (%d/%d),
+id,
+(int)st-num_drops_arrival,
+st-num_drops_arrival ? (st-drop_arrival_late_time + 0.0) / 
st-num_drops_arrival :
+0.0,
+(int)st-num_drops_play,
+st-num_drops_play ? (st-drop_play_late_time + 0.0) / 
st-num_drops_play : 0,
+st-num_total_frames ? (num_played_frames + 0.0) / 
st-num_total_frames : 0,
+num_played_frames,
+st-num_total_frames);
 switch (st-codec) {
 case SPICE_VIDEO_CODEC_TYPE_MJPEG:
 stream_mjpeg_cleanup(st);
-- 
1.7.7.6

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

Re: [Spice-devel] [PATCH v2 spice 01/11] server/red_worker: fix dump_bitmap

[Yonit Halperin]

On 04/17/2012 01:46 PM, Alon Levy wrote:

On Tue, Apr 17, 2012 at 01:12:26PM +0300, Yonit Halperin wrote:

---
  server/red_worker.c |5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 07782c8..5350195 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -11348,10 +11348,11 @@ static void dump_bitmap(RedWorker *worker, 
SpiceBitmap *bitmap, uint32_t group_i
  }
  /* writing the data */
  for (i = 0; i  bitmap-data-num_chunks; i++) {
+int j;


Missed this in v1, sorry - shouldn't this be defined at the function
start? Not a big deal.

why?



  SpiceChunk *chunk =bitmap-data-chunk[i];
  int num_lines = chunk-len / bitmap-stride;
-for (i = 0; i  num_lines; i++) {
-dump_line(f, chunk-data + (i * bitmap-stride), n_pixel_bits, 
bitmap-x, row_size);
+for (j = 0; j  num_lines; j++) {
+dump_line(f, chunk-data + (j * bitmap-stride), n_pixel_bits, 
bitmap-x, row_size);
  }
  }
  fclose(f);
--
1.7.7.6



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

Re: [Spice-devel] [PATCH v2 spice-protocol 2/2] Add qos related messages

[Yonit Halperin]

On 04/18/2012 08:47 PM, Marc-André Lureau wrote:

Hi

On Wed, Apr 18, 2012 at 7:34 PM, Marc-André Lureau
marcandre.lur...@gmail.com  wrote:

It would be
-  QOS_QUERY(nbytes)
-  nbytes of various messages 1 or n
- QOS_ACK


Further questions about this, how can the server really determine the
bandwidth? (I suppose this is the goal)

There might be data queued on the upstream link, and then latency and
other factors are affecting the result.


The latency factor is in my TODO (see cover letter).
I use ioctl(socket, TIOCOUTQ) in order to find how many bytes are 
pending in the tcp snd buffer, in addition to the size of the message.
So I calculate the bandwidth by (message_size + ioctl(socket, 
TIOCOUTQ))/ack_time.
When I add the latency, it will be (message_size + ioctl(socket, 
TIOCOUTQ)) / (ack_time - latency)


This measurement can be affected by other links, but I think it is what 
we want - the actual bandwidth available for the channel.



It could perhaps be more reliable if the messages would be:

-  QOS_QUERY(nbytes)
-  nbytes of various messages data
- QOS_ACK(N Bps)


Btw, those messages should be symmetric, so we could measure
upload/download (it's always fun to reinvent the wheel!

Currently all the channels are mainly asymmetric (maybe except for the 
usb). Specifically, for the display channel, which is currently the only 
consumer of bandwidth data, the upload statistics are the most important 
ones. In the future we may want to implement it in the client side as 
well (for record channel, and usb), and add the symmetric messages.


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

Re: [Spice-devel] [PATCH v2 spice-protocol 2/2] Add qos related messages

[Yonit Halperin]

On 04/19/2012 02:58 PM, Marc-André Lureau wrote:

On Thu, Apr 19, 2012 at 9:30 AM, Yonit Halperinyhalp...@redhat.com  wrote:

The latency factor is in my TODO (see cover letter).
I use ioctl(socket, TIOCOUTQ) in order to find how many bytes are pending in
the tcp snd buffer, in addition to the size of the message.
So I calculate the bandwidth by (message_size + ioctl(socket,
TIOCOUTQ))/ack_time.
When I add the latency, it will be (message_size + ioctl(socket, TIOCOUTQ))
/ (ack_time - latency)

This measurement can be affected by other links, but I think it is what we
want -  the actual bandwidth available for the channel.


Don't we want to measure only downlink bandwidth/conditon separately
from uplink? What you suggest takes into account a lot of parameters
from both side. I still fail to see what exactly it represents. Imho,
it doesn't need TIOCOUTQ or latency. A top with N bytes sent in
burst is all it takes, no?


We want to measure the effective traffic rate from the server to the 
client. It is not really important if the bottle neck is the uplink of 
the server, or the downlink of the client.
Since the relevant spice channel is mostly uni-directional, and the ack 
message from the client is small, the ping-pong time can give us 
(latency-server + latency-client), and subtracting it from the ack time 
will allow us to measure the traffic rate.
TIOCOUTQ is essential in order to know, as much as you can, how much 
data has been sent to the client since you started the time measuring.

I didn't understand your comment  A top with N bytes sent in
 burst is all it takes, no?
In the future, we plan to have bandwidth shaping capabilities per 
channel (Maybe via libvirt and tc), so it is necessary to measure the 
traffic per channel.


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

Re: [Spice-devel] [PATCH v2 spice-protocol 2/2] Add qos related messages

[Yonit Halperin]

On 04/22/2012 04:23 PM, Marc-André Lureau wrote:

On Sun, Apr 22, 2012 at 10:39 AM, Yonit Halperinyhalp...@redhat.com  wrote:

  A top with N bytes sent


This is the simpler mechanism I have in mind: the client receives a
start mark knowing N bytes are following that were sent directly (in
burst). Once the N bytes are received the client can report the time
it took to receives those, or the bandwidth. Tbh, I am still not
convinced by the proposal you made, to measure somehow only one
direction bandwidth but take overall latency in whatever
state/buffering/timing in both uplink and downlink..


t1-t0 = l1 + f*b + l2
where t0=server_send_time t1=server_receive_ack_time
l1+l2 =up_latency + down_latency = roundtrip time
f=bandwidth
b=sent data size

Of course, this is theoretic, and in reality there are overheads related 
to the protocol, cpu, etc. Thus, the computed f is smaller than the real 
throughput. However, for spice purposes, the exact value is not 
critical, and an estimation that is smaller than the real value is 
preferred over the opposite.
In addition, an estimation of the latency is something we may also want 
to use in the future, e.g., for tuning the size of the playback 
de-jitter buffer, etc.


Regarding your suggestion:
Taking a burst of N bytes is problematic with the current serial 
implementation of the server and the client: (process_message, 
send_message), (proccess_message, send_message), --- 
(receive_message, process_message), (receive_message, process_message)
We can look at the time it took to the client to receive the message 
following the qos message, if its size is big enough to make the size of 
the data already pending in the tcp stack neglectable.




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

[Spice-devel] [PATCH xf86-video-qxl] Do not call update_area when lacking device memory

[Yonit Halperin]

The QXL_IO_NOTIFY_OOM is intended exactly for handling occurrences of
lacking memory. The spice server tries to first release resources that
are no longer in the current tree (and thus, do not need rendering).
It renders drawables only as a last resort. And even then,
it does not update the whole primary surface, but rather renders
the oldest X drawables.
The call to update_area is redundant, and its effect on performance
is noticeable when playing full screen video.

Signed-off-by: Yonit Halperin yhalp...@redhat.com
---
 src/qxl_driver.c |   14 --
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index 5c826f3..8b73dd8 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -294,20 +294,6 @@ qxl_allocnf (qxl_screen_t *qxl, unsigned long size)
 
 while (!(result = qxl_alloc (qxl-mem, size)))
 {
-   struct QXLRam *ram_header = (void *)(
-   (unsigned long)qxl-ram + qxl-rom-ram_header_offset);
-
-   /* Rather than go out of memory, we simply tell the
-* device to dump everything
-*/
-   ram_header-update_area.top = 0;
-   ram_header-update_area.bottom = qxl-virtual_y;
-   ram_header-update_area.left = 0;
-   ram_header-update_area.right = qxl-virtual_x;
-   ram_header-update_surface = 0; /* Only primary for now */
-   
-qxl_update_area(qxl);
-   
 #if 0
ErrorF (eliminated memory (%d)\n, nth_oom++);
 #endif
-- 
1.7.7.6

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

Re: [Spice-devel] Client Graphics Filtering

[Yonit Halperin]

Hi,

On 04/24/2012 09:12 AM, Alon Levy wrote:

On Tue, Apr 24, 2012 at 09:47:47AM +0800, 蒋媛园 wrote:

  Hi
I want to filter the graphics to the client. For example, only the
graphics of a calculator(in guest OS Win7) are send to the client.
I know that there is always the possibility of falling all the way back to 
CPU drawing to a memory bitmap which is then copied to the client.
Now, I have filtered the drawing commands on device-managed surfaces which 
belong to some app.exe. However I don't know how to control the engine-managed 
surfaces, namely those pre-rendered bitmaps. I didn't find related code in QXL 
driver.
Could you please give me some advice? I really need some help.

I'm not sure I understand exactly what you mean. I guess that by 
engine-managed, you mean gdi bitmaps? When the destination of a drawing 
operation is a gdi bitmap, the client doesn't know about this operation. 
The driver renders the src* of the operation if needed (if it is a 
device managed bitmap), copies the relevant src area to a new gdi 
bitmap, and fallback to a Gdi call, with the copied src and original dest.
If the destination is a device managed bitmap, and the src* is a gdi 
managed bitmap, the gdi bitmap is copied to the device memory, and the 
corresponding qxl drawing command is sent to the server together with 
the reference to the copied bitmap.


* There can be multiple references to bitmaps in one qxl command 
(src/mask/brush), and each one can be a bitmap (gdi/device)


Regards,
Yonit.


Great to hear someone is working on this, I don't have an answer though,
Yonit, any idea maybe?



Thanks in advance
  Best Regards!





网易Lofter，专注兴趣，分享创作！




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

Re: [Spice-devel] [bug] tripping an assert (regularly!) in release_drawable in red_worker.c

[Yonit Halperin]

Hi,
On 05/01/2012 05:39 PM, Nahum Shalman wrote:

I'm much happier about this one since it now happens to me pretty much
every time I use a VM.
Once again, I'm using the latest bits as of a couple days ago with a
modified qemu running under illumos.

I'm reliably tripping an assert in red_worker.c
(it's annoying when that happens... qemu just sort of spins off into
crazy land rather than dumping core and exiting.)

This is the tripped assert:
SpiceWorker-ERROR **: red_worker.c:1736:release_drawable: assertion
`!drawable-stream' failed

I hacked up all the calls to release_drawable to include a matching
assert above the call to isolate which call was tripping it, and it
looks like it's the last one in red_process_drawable

I have a couple of thoughts:

1. Can someone familiar with the (scary) innards of red_worker.c take a
look and see if anything stands out?
I'm doing some changes in this part of the code, but so far I haven't 
tripped into this assert (before applying my changes, and without 
pulling changes from the last week or two). Can you bisect this?

What guest are you running?

Regards,
Yonit.

2. Should we consider beefing up the implementation of red_assert to
have it generate a stack trace and/or have it cause qemu to dump core?
This seems reasonable to me since QEMU seems to go crazy when this happens.

Thanks for your time,
Nahum
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel


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

[Spice-devel] [PATCH spice-common 1/5] rect: add rect_contains

[Yonit Halperin]

---
 common/rect.h |   11 +++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/common/rect.h b/common/rect.h
index a63d785..655e9e8 100644
--- a/common/rect.h
+++ b/common/rect.h
@@ -74,6 +74,12 @@ static INLINE int rect_is_same_size(const SpiceRect *r1, 
const SpiceRect *r2)
r1-bottom - r1-top == r2-bottom - r2-top;
 }
 
+static INLINE int rect_contains(const SpiceRect *big, const SpiceRect *small)
+{
+return big-left = small-left  big-right = small-right 
+   big-top = small-top  big-bottom = small-bottom;
+}
+
 SPICE_END_DECLS
 
 #ifdef __cplusplus
@@ -113,6 +119,11 @@ static inline int rect_is_same_size(const SpiceRect r1, 
const SpiceRect r2)
 return rect_is_same_size(r1, r2);
 }
 
+static inline int rect_contains(const SpiceRect big, const SpiceRect small)
+{
+return rect_contains(big, small);
+}
+
 #endif /* __cplusplus */
 
 #endif
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-common 3/5] rect: add rect_debug

[Yonit Halperin]

---
 common/rect.h |   11 +++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/common/rect.h b/common/rect.h
index a9c1b08..f8bacf1 100644
--- a/common/rect.h
+++ b/common/rect.h
@@ -21,6 +21,7 @@
 
 #include spice/macros.h
 #include draw.h
+#include log.h
 
 SPICE_BEGIN_DECLS
 
@@ -85,6 +86,11 @@ static INLINE int rect_get_area(const SpiceRect *r)
 return (r-right - r-left) * (r-bottom - r-top);
 }
 
+static INLINE void rect_debug(const SpiceRect *r)
+{
+spice_debug((%d, %d) (%d, %d), r-left, r-top, r-right, r-bottom);
+}
+
 SPICE_END_DECLS
 
 #ifdef __cplusplus
@@ -134,6 +140,11 @@ static inline int rect_get_area(const SpiceRect r)
 return rect_get_area(r);
 }
 
+static inline void rect_debug(const SpiceRect r)
+{
+rect_debug(r);
+}
+
 #endif /* __cplusplus */
 
 #endif
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-common 4/5] region: add region_extents

[Yonit Halperin]

---
 common/region.c |   11 +++
 common/region.h |2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/common/region.c b/common/region.c
index 30a11e3..cd9ade0 100644
--- a/common/region.c
+++ b/common/region.c
@@ -386,6 +386,17 @@ void region_ret_rects(const QRegion *rgn, SpiceRect 
*rects, uint32_t num_rects)
 }
 }
 
+void region_extents(const QRegion *rgn, SpiceRect *r)
+{
+pixman_box32_t *extents;
+
+extents = pixman_region32_extents((pixman_region32_t *)rgn);
+
+r-left = extents-x1;
+r-top = extents-y1;
+r-right = extents-x2;
+r-bottom = extents-y2;
+}
 
 int region_is_equal(const QRegion *rgn1, const QRegion *rgn2)
 {
diff --git a/common/region.h b/common/region.h
index 3eed547..4619406 100644
--- a/common/region.h
+++ b/common/region.h
@@ -41,6 +41,7 @@ void region_destroy(QRegion *rgn);
 void region_clone(QRegion *dest, const QRegion *src);
 SpiceRect *region_dup_rects(const QRegion *rgn, uint32_t *num_rects);
 void region_ret_rects(const QRegion *rgn, SpiceRect *rects, uint32_t 
num_rects);
+void region_extents(const QRegion *rgn, SpiceRect *r);
 
 int region_test(const QRegion *rgn, const QRegion *other_rgn, int query);
 int region_is_valid(const QRegion *rgn);
@@ -61,6 +62,7 @@ void region_remove(QRegion *rgn, const SpiceRect *r);
 
 void region_offset(QRegion *rgn, int32_t dx, int32_t dy);
 
+
 void region_dump(const QRegion *rgn, const char *prefix);
 
 SPICE_END_DECLS
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-common 5/5] video streaming: add support for frames of different sizes

[Yonit Halperin]

rhbz #813826, #815426

Add SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, for stream_data message
that also contains the size and destination box of the data.
The server can send such messages only to clients with
SPICE_DISPLAY_CAP_SIZED_STREAM.
---
 common/messages.h |   15 ++-
 spice-protocol|2 +-
 spice.proto   |   19 ---
 spice1.proto  |8 ++--
 4 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/common/messages.h b/common/messages.h
index 106a8d3..e3677d1 100644
--- a/common/messages.h
+++ b/common/messages.h
@@ -308,13 +308,26 @@ typedef struct SpiceMsgDisplayStreamCreate {
 SpiceClip clip;
 } SpiceMsgDisplayStreamCreate;
 
-typedef struct SpiceMsgDisplayStreamData {
+typedef struct SpiceStreamDataHeader {
 uint32_t id;
 uint32_t multi_media_time;
+} SpiceStreamDataHeader;
+
+typedef struct SpiceMsgDisplayStreamData {
+SpiceStreamDataHeader base;
 uint32_t data_size;
 uint8_t data[0];
 } SpiceMsgDisplayStreamData;
 
+typedef struct SpiceMsgDisplayStreamDataSized {
+SpiceStreamDataHeader base;
+uint32_t width;
+uint32_t height;
+SpiceRect dest;
+uint32_t data_size;
+uint8_t data[0];
+} SpiceMsgDisplayStreamDataSized;
+
 typedef struct SpiceMsgDisplayStreamClip {
 uint32_t id;
 SpiceClip clip;
diff --git a/spice-protocol b/spice-protocol
index 2d24f61..bf0daeb 16
--- a/spice-protocol
+++ b/spice-protocol
@@ -1 +1 @@
-Subproject commit 2d24f61aae4f92746940fd1c0daf546c7a51dae8
+Subproject commit bf0daeb3a3c834133e781439f76b1c702470581b
diff --git a/spice.proto b/spice.proto
index 513fe87..71be9ac 100644
--- a/spice.proto
+++ b/spice.proto
@@ -591,6 +591,11 @@ struct String {
 } u @anon;
 };
 
+struct StreamDataHeader {
+   uint32 id;
+   uint32 multi_media_time;
+};
+
 channel DisplayChannel : BaseChannel {
  server:
 message {
@@ -637,10 +642,9 @@ channel DisplayChannel : BaseChannel {
 } stream_create = 122;
 
 message {
-   uint32 id;
-   uint32 multi_media_time;
+   StreamDataHeader base;
uint32 data_size;
-   uint8 data[data_size] @end  @nomarshal;
+   uint8 data[data_size] @end @nomarshal;
 } stream_data;
 
 message {
@@ -785,6 +789,15 @@ channel DisplayChannel : BaseChannel {
uint32 surface_id;
 } @ctype(SpiceMsgSurfaceDestroy) surface_destroy;
 
+message {
+   StreamDataHeader base;
+   uint32 width;
+   uint32 height;
+   Rect dest;
+   uint32 data_size;
+   uint8 data[data_size] @end @nomarshal;
+} stream_data_sized;
+
  client:
 message {
uint8 pixmap_cache_id;
diff --git a/spice1.proto b/spice1.proto
index fa2524b..2ed1058 100644
--- a/spice1.proto
+++ b/spice1.proto
@@ -533,6 +533,11 @@ struct String {
 } u @anon;
 };
 
+struct StreamDataHeader {
+   uint32 id;
+   uint32 multi_media_time;
+};
+
 channel DisplayChannel : BaseChannel {
  server:
 message {
@@ -580,8 +585,7 @@ channel DisplayChannel : BaseChannel {
 } stream_create = 122;
 
 message {
-   uint32 id;
-   uint32 multi_media_time;
+   StreamDataHeader base;
uint32 data_size;
uint32 pad_size @zero;
uint8 data[data_size] @end  @nomarshal;
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 01/18] Update the spice-common submodule

[Yonit Halperin]

We need some rect/region getters methods that were added
---
 spice-common |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/spice-common b/spice-common
index 6af29a9..178c7ea 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit 6af29a97ac2d4e88e1391dd1b4697f32138ce4df
+Subproject commit 178c7eaff6fa45b9051bb4d3cf90f45ea9319f83
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 02/18] server/red_worker: add get_stream_id

[Yonit Halperin]

---
 server/red_worker.c |   27 ---
 1 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 07782c8..09a9357 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -2485,6 +2485,11 @@ static void red_display_release_stream_clip(RedWorker 
*worker, StreamClipItem *i
 }
 }
 
+static inline int get_stream_id(RedWorker *worker, Stream *stream)
+{
+return (int)(stream - worker-streams_buf);
+}
+
 static void red_attach_stream(RedWorker *worker, Drawable *drawable, Stream 
*stream)
 {
 DisplayChannelClient *dcc;
@@ -2498,7 +2503,7 @@ static void red_attach_stream(RedWorker *worker, Drawable 
*drawable, Stream *str
 stream-last_time = drawable-creation_time;
 
 WORKER_FOREACH_DCC(worker, item, dcc) {
-agent = dcc-stream_agents[stream - worker-streams_buf];
+agent = dcc-stream_agents[get_stream_id(worker, stream)];
 if (!region_is_equal(agent-vis_region, 
drawable-tree_item.base.rgn)) {
 region_destroy(agent-vis_region);
 region_clone(agent-vis_region, drawable-tree_item.base.rgn);
@@ -2516,7 +2521,7 @@ static void red_stop_stream(RedWorker *worker, Stream 
*stream)
 spice_assert(!stream-current);
 WORKER_FOREACH_DCC(worker, item, dcc) {
 StreamAgent *stream_agent;
-stream_agent = dcc-stream_agents[stream - worker-streams_buf];
+stream_agent = dcc-stream_agents[get_stream_id(worker, stream)];
 region_clear(stream_agent-vis_region);
 spice_assert(!pipe_item_is_linked(stream_agent-destroy_item));
 stream-refs++;
@@ -2578,7 +2583,7 @@ static void red_detach_streams_behind(RedWorker *worker, 
QRegion *region)
 item = ring_next(ring, item);
 
 WORKER_FOREACH_DCC(worker, dcc_ring_item, dcc) {
-StreamAgent *agent = dcc-stream_agents[stream - 
worker-streams_buf];
+StreamAgent *agent = dcc-stream_agents[get_stream_id(worker, 
stream)];
 if (region_intersects(agent-vis_region, region)) {
 region_clear(agent-vis_region);
 push_stream_clip(dcc, agent);
@@ -2628,7 +2633,7 @@ static void red_streams_update_clip(RedWorker *worker, 
Drawable *drawable)
 }
 
 WORKER_FOREACH_DCC(worker, dcc_ring_item, dcc) {
-agent = dcc-stream_agents[stream - worker-streams_buf];
+agent = dcc-stream_agents[get_stream_id(worker, stream)];
 
 if (region_intersects(agent-vis_region, 
drawable-tree_item.base.rgn)) {
 region_exclude(agent-vis_region, 
drawable-tree_item.base.rgn);
@@ -2735,7 +2740,7 @@ static int get_minimal_bit_rate(RedWorker *worker, int 
width, int height)
 
 static void red_display_create_stream(DisplayChannelClient *dcc, Stream 
*stream)
 {
-StreamAgent *agent = dcc-stream_agents[stream - 
dcc-common.worker-streams_buf];
+StreamAgent *agent = dcc-stream_agents[get_stream_id(dcc-common.worker, 
stream)];
 
 stream-refs++;
 spice_assert(region_is_empty(agent-vis_region));
@@ -2927,7 +2932,7 @@ static inline void pre_stream_item_swap(RedWorker 
*worker, Stream *stream)
 return;
 }
 
-index = stream - worker-streams_buf;
+index = get_stream_id(worker, stream);
 DRAWABLE_FOREACH_DPI(stream-current, ring_item, dpi) {
 dcc = dpi-dcc;
 if (!display_channel_client_is_low_bandwidth(dcc)) {
@@ -8036,7 +8041,7 @@ static inline int 
red_marshall_stream_data(RedChannelClient *rcc,
 return FALSE;
 }
 
-StreamAgent *agent = dcc-stream_agents[stream - worker-streams_buf];
+StreamAgent *agent = dcc-stream_agents[get_stream_id(worker, stream)];
 uint64_t time_now = red_now();
 size_t outbuf_size;
 if (time_now - agent-last_send_time  (1000 * 1000 * 1000) / agent-fps) {
@@ -8061,7 +8066,7 @@ static inline int 
red_marshall_stream_data(RedChannelClient *rcc,
 
 SpiceMsgDisplayStreamData stream_data;
 
-stream_data.id = stream - worker-streams_buf;
+stream_data.id = get_stream_id(worker, stream);
 stream_data.multi_media_time = drawable-red_drawable-mm_time;
 stream_data.data_size = n;
 spice_marshall_msg_display_stream_data(base_marshaller, stream_data);
@@ -8361,7 +8366,7 @@ static void 
red_display_marshall_stream_start(RedChannelClient *rcc,
 SpiceClipRects clip_rects;
 
 stream_create.surface_id = 0;
-stream_create.id = agent - dcc-stream_agents;
+stream_create.id = get_stream_id(dcc-common.worker, stream);
 stream_create.flags = stream-top_down ? SPICE_STREAM_FLAGS_TOP_DOWN : 0;
 stream_create.codec_type = SPICE_VIDEO_CODEC_TYPE_MJPEG;
 
@@ -8397,7 +8402,7 @@ static void 
red_display_marshall_stream_clip(RedChannelClient *rcc,
 red_channel_client_init_send_data(rcc, SPICE_MSG_DISPLAY_STREAM_CLIP, 
item-base);
 SpiceMsgDisplayStreamClip stream_clip;
 
-stream_clip.id = agent - dcc-stream_agents;
+stream_clip.id = 

[Spice-devel] [PATCH spice 03/18] server/red_worker/video: maintain visible region and clip region for streams

[Yonit Halperin]

Differentiate between the clipping of the video stream, and the region
that currently displays fragments of the video stream (henceforth,
vis_region). The latter equals or contains the former one. For example,
let c1 be the clip area at time t1, and c2 be the clip area at time t2,
where t1  t2. If c1 contains c2, and at least part of c1/c2, hasn't been
covered by a non-video images, vis_region will contain c2, and also the part
of c1/c2 that still displays fragments of the video.
When we consider if a stream should be upgraded (1), due to its area
being used by a rendering operation, or due to stopping the video, we
should take into account the vis_region, and not the clip region (next
patch: not upgrade by the last frame, but rather by vis_region).
This fix will be more necessary when sized frames are introduced (see the
following patches). Then, the vis_region might be larger
than the last frame, and contain it, more frequently than before.

(1) upgrading a stream stands for sending its last frame losslessly. Or more
precisely, lossless resending of all the currently displayed lossy areas, that 
were
sent as part of the stream.
---
 server/red_worker.c |   39 +++
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 09a9357..bab4b93 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -390,7 +390,15 @@ struct Stream {
 };
 
 typedef struct StreamAgent {
-QRegion vis_region;
+QRegion vis_region; /* the part of the surface area that is currently 
occupied by video
+   fragments */
+QRegion clip;   /* the current video clipping. It can be different 
from vis_region:
+   for example, let c1 be the clip area at time t1, 
and c2
+   be the clip area at time t2, where t1  t2. If c1 
contains c2, and
+   at least part of c1/c2, hasn't been covered by a 
non-video images,
+   vis_region will contain c2 and also the part of 
c1/c2 that still
+   displays fragments of the video */
+
 PipeItem create_item;
 PipeItem destroy_item;
 Stream *stream;
@@ -2468,11 +2476,11 @@ static void push_stream_clip(DisplayChannelClient* dcc, 
StreamAgent *agent)
 }
 item-clip_type = SPICE_CLIP_TYPE_RECTS;
 
-n_rects = pixman_region32_n_rects(agent-vis_region);
+n_rects = pixman_region32_n_rects(agent-clip);
 
 item-rects = spice_malloc_n_m(n_rects, sizeof(SpiceRect), 
sizeof(SpiceClipRects));
 item-rects-num_rects = n_rects;
-region_ret_rects(agent-vis_region, item-rects-rects, n_rects);
+region_ret_rects(agent-clip, item-rects-rects, n_rects);
 red_channel_client_pipe_add(dcc-common.base, (PipeItem *)item);
 }
 
@@ -2493,7 +2501,6 @@ static inline int get_stream_id(RedWorker *worker, Stream 
*stream)
 static void red_attach_stream(RedWorker *worker, Drawable *drawable, Stream 
*stream)
 {
 DisplayChannelClient *dcc;
-StreamAgent *agent;
 RingItem *item;
 
 spice_assert(!drawable-stream  !stream-current);
@@ -2503,10 +2510,12 @@ static void red_attach_stream(RedWorker *worker, 
Drawable *drawable, Stream *str
 stream-last_time = drawable-creation_time;
 
 WORKER_FOREACH_DCC(worker, item, dcc) {
-agent = dcc-stream_agents[get_stream_id(worker, stream)];
-if (!region_is_equal(agent-vis_region, 
drawable-tree_item.base.rgn)) {
-region_destroy(agent-vis_region);
-region_clone(agent-vis_region, drawable-tree_item.base.rgn);
+StreamAgent *agent = dcc-stream_agents[get_stream_id(worker, 
stream)];
+
+region_or(agent-vis_region, drawable-tree_item.base.rgn);
+if (!region_is_equal(agent-clip, drawable-tree_item.base.rgn)) {
+region_destroy(agent-clip);
+region_clone(agent-clip, drawable-tree_item.base.rgn);
 push_stream_clip_by_drawable(dcc, agent, drawable);
 }
 }
@@ -2523,6 +2532,7 @@ static void red_stop_stream(RedWorker *worker, Stream 
*stream)
 StreamAgent *stream_agent;
 stream_agent = dcc-stream_agents[get_stream_id(worker, stream)];
 region_clear(stream_agent-vis_region);
+region_clear(stream_agent-clip);
 spice_assert(!pipe_item_is_linked(stream_agent-destroy_item));
 stream-refs++;
 red_channel_client_pipe_add(dcc-common.base, 
stream_agent-destroy_item);
@@ -2585,7 +2595,8 @@ static void red_detach_streams_behind(RedWorker *worker, 
QRegion *region)
 WORKER_FOREACH_DCC(worker, dcc_ring_item, dcc) {
 StreamAgent *agent = dcc-stream_agents[get_stream_id(worker, 
stream)];
 if (region_intersects(agent-vis_region, region)) {
-region_clear(agent-vis_region);
+/* hiding the stream at the client side at once */
+region_clear(agent-clip);
 

[Spice-devel] [PATCH spice 04/18] server/red_worker/video: upgrade stream by a screenshot instead of the last frame, if needed

[Yonit Halperin]

Upgrading a stream: When the stream's visible region is bigger than the one of 
the last
frame, we will send an updated screenshot of the visible region, instead
of sending the last frame losslessly.
---
 server/red_worker.c |   92 +++---
 1 files changed, 71 insertions(+), 21 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index bab4b93..a008172 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1006,6 +1006,8 @@ static void 
display_channel_push_release(DisplayChannelClient *dcc, uint8_t type
 static void red_display_release_stream_clip(RedWorker *worker, StreamClipItem 
*item);
 static int 
red_display_free_some_independent_glz_drawables(DisplayChannelClient *dcc);
 static void red_display_free_glz_drawable(DisplayChannelClient *dcc, 
RedGlzDrawable *drawable);
+static ImageItem *red_add_surface_area_image(DisplayChannelClient *dcc, int 
surface_id,
+ SpiceRect *area, PipeItem *pos, 
int can_lossy);
 static void reset_rate(DisplayChannelClient *dcc, StreamAgent *stream_agent);
 static BitmapGradualType _get_bitmap_graduality_level(RedWorker *worker, 
SpiceBitmap *bitmap,
   uint32_t group_id);
@@ -2541,25 +2543,54 @@ static void red_stop_stream(RedWorker *worker, Stream 
*stream)
 red_release_stream(worker, stream);
 }
 
-static int drawable_is_linked(Drawable *drawable)
+static int red_display_drawable_is_in_pipe(DisplayChannelClient *dcc, Drawable 
*drawable)
 {
-return !ring_is_empty(drawable-pipes);
+DrawablePipeItem *dpi;
+RingItem *dpi_link;
+
+DRAWABLE_FOREACH_DPI(drawable, dpi_link, dpi) {
+if (dpi-dcc == dcc) {
+return TRUE;
+}
+}
+
+return FALSE;
 }
 
-static inline void red_detach_stream_gracefully(RedWorker *worker, Stream 
*stream)
+/*
+ * after red_display_detach_stream_gracefully is called for all the display 
channel clients,
+ * red_detach_stream should be called. See comment (1).
+ */
+static inline void red_display_detach_stream_gracefully(DisplayChannelClient 
*dcc, Stream *stream)
 {
-RedChannel *channel;
-RingItem *item;
-RedChannelClient *rcc;
-DisplayChannelClient *dcc;
+int stream_id = get_stream_id(dcc-common.worker, stream);
+StreamAgent *agent = dcc-stream_agents[stream_id];
 
-spice_assert(stream-current);
-WORKER_FOREACH_DCC(worker, item, dcc) {
+/* stopping the client from playing older frames at once*/
+region_clear(agent-clip);
+push_stream_clip(dcc, agent);
+
+if (region_is_empty(agent-vis_region)) {
+spice_debug(stream %d: vis region empty, stream_id);
+return;
+}
+
+if (stream-current 
+region_contains(stream-current-tree_item.base.rgn, 
agent-vis_region)) {
+RedChannel *channel;
+RedChannelClient *rcc;
 UpgradeItem *upgrade_item;
 int n_rects;
-if (drawable_is_linked(stream-current)) {
-continue;
+
+/* (1) The caller should detach the drawable from the stream. This will
+ * lead to sending the drawable losslessly, as an ordinary drawable. */
+if (red_display_drawable_is_in_pipe(dcc, stream-current)) {
+spice_debug(stream %d: upgrade by linked drawable. box ==, 
stream_id);
+rect_debug(stream-current-red_drawable-bbox);
+return;
 }
+spice_debug(stream %d: upgrade by drawable. box ==, stream_id);
+rect_debug(stream-current-red_drawable-bbox);
 rcc = dcc-common.base;
 channel = rcc-channel;
 upgrade_item = spice_new(UpgradeItem, 1);
@@ -2574,8 +2605,31 @@ static inline void 
red_detach_stream_gracefully(RedWorker *worker, Stream *strea
 region_ret_rects(upgrade_item-drawable-tree_item.base.rgn,
  upgrade_item-rects-rects, n_rects);
 red_channel_client_pipe_add(rcc, upgrade_item-base);
+
+} else {
+SpiceRect upgrade_area;
+
+region_extents(agent-vis_region, upgrade_area);
+spice_debug(stream %d: upgrade by screenshot. has current %d. box 
==,
+stream_id, stream-current != NULL);
+rect_debug(upgrade_area);
+red_update_area(dcc-common.worker, upgrade_area, 0);
+red_add_surface_area_image(dcc, 0, upgrade_area, NULL, FALSE);
+}
+
+}
+
+static inline void red_detach_stream_gracefully(RedWorker *worker, Stream 
*stream)
+{
+RingItem *item;
+DisplayChannelClient *dcc;
+
+WORKER_FOREACH_DCC(worker, item, dcc) {
+red_display_detach_stream_gracefully(dcc, stream);
+}
+if (stream-current) {
+red_detach_stream(worker, stream);
 }
-red_detach_stream(worker, stream);
 }
 
 // region should be a primary surface region
@@ -2594,24 +2648,20 @@ static void red_detach_streams_behind(RedWorker 
*worker, QRegion *region)
 
 WORKER_FOREACH_DCC(worker, dcc_ring_item, 

[Spice-devel] [PATCH spice 05/18] server/red_worker/video: don't detach a drawable from a stream due to it being rendered.

[Yonit Halperin]

The previous patch took care that streams will be upgraded by a surface
screenshot and not the last frame, if necessary. Thus, there is no
more a reason for detaching drawables from streams when they are
rendered. Moreover, detaching such drawables can cause glitches, in
case they are still in the pipe, and red_update_area is called
frequently and leads to stream frames rendering.
---
 server/red_worker.c |   16 
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index a008172..a3dc8fc 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1743,9 +1743,12 @@ static inline void release_drawable(RedWorker *worker, 
Drawable *drawable)
 RingItem *item, *next;
 
 if (!--drawable-refs) {
-spice_assert(!drawable-stream);
 spice_assert(!drawable-tree_item.shadow);
 spice_assert(ring_is_empty(drawable-pipes));
+
+if (drawable-stream) {
+red_detach_stream(worker, drawable-stream);
+}
 region_destroy(drawable-tree_item.base.rgn);
 
 remove_drawable_dependencies(worker, drawable);
@@ -1846,9 +1849,7 @@ static inline void current_remove_drawable(RedWorker 
*worker, Drawable *item)
 if (item-tree_item.effect != QXL_EFFECT_OPAQUE) {
 worker-transparent_count--;
 }
-if (item-stream) {
-red_detach_stream(worker, item-stream);
-} else {
+if (!item-stream) {
 red_add_item_trace(worker, item);
 }
 remove_shadow(worker, item-tree_item);
@@ -2741,9 +2742,7 @@ static inline void red_handle_streams_timout(RedWorker 
*worker)
 Stream *stream = SPICE_CONTAINEROF(item, Stream, link);
 item = ring_next(ring, item);
 if (now = (stream-last_time + RED_STREAM_TIMOUT)) {
-if (stream-current) {
-red_detach_stream_gracefully(worker, stream);
-}
+red_detach_stream_gracefully(worker, stream);
 red_stop_stream(worker, stream);
 }
 }
@@ -4504,7 +4503,8 @@ static void red_update_area(RedWorker *worker, const 
SpiceRect *area, int surfac
 #ifdef ACYCLIC_SURFACE_DEBUG
 int gn;
 #endif
-
+spice_debug(surface %d: area ==, surface_id);
+rect_debug(area);
 surface = worker-surfaces[surface_id];
 
 last = NULL;
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 06/18] Update the spice-common submodule

[Yonit Halperin]

spice-common changes: STREAM_DATA_SIZED message was added in order to support
video streams with frames that their size is different from the initial size
that the stream was created with.

This patch also includes server and client adjustments to the new
SpiceMsgDisplayStreamData.
---
 client/display_channel.cpp |6 --
 server/red_worker.c|4 ++--
 spice-common   |2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/client/display_channel.cpp b/client/display_channel.cpp
index ebeacd2..17bdf6a 100644
--- a/client/display_channel.cpp
+++ b/client/display_channel.cpp
@@ -1419,7 +1419,7 @@ void 
DisplayChannel::handle_stream_data(RedPeer::InMessage* message)
 SpiceMsgDisplayStreamData* stream_data = 
(SpiceMsgDisplayStreamData*)message-data();
 VideoStream* stream;
 
-if (stream_data-id = _streams.size() || !(stream = 
_streams[stream_data-id])) {
+if (stream_data-base.id = _streams.size() || !(stream = 
_streams[stream_data-base.id])) {
 THROW(invalid stream);
 }
 
@@ -1427,7 +1427,9 @@ void 
DisplayChannel::handle_stream_data(RedPeer::InMessage* message)
 THROW(access violation);
 }
 
-stream-push_data(stream_data-multi_media_time, stream_data-data_size, 
stream_data-data);
+stream-push_data(stream_data-base.multi_media_time,
+  stream_data-data_size,
+  stream_data-data);
 }
 
 void DisplayChannel::handle_stream_clip(RedPeer::InMessage* message)
diff --git a/server/red_worker.c b/server/red_worker.c
index a3dc8fc..4d7f89b 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -8131,8 +8131,8 @@ static inline int 
red_marshall_stream_data(RedChannelClient *rcc,
 
 SpiceMsgDisplayStreamData stream_data;
 
-stream_data.id = get_stream_id(worker, stream);
-stream_data.multi_media_time = drawable-red_drawable-mm_time;
+stream_data.base.id = get_stream_id(worker, stream);
+stream_data.base.multi_media_time = drawable-red_drawable-mm_time;
 stream_data.data_size = n;
 spice_marshall_msg_display_stream_data(base_marshaller, stream_data);
 spice_marshaller_add_ref(base_marshaller,
diff --git a/spice-common b/spice-common
index 178c7ea..22fc0b0 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit 178c7eaff6fa45b9051bb4d3cf90f45ea9319f83
+Subproject commit 22fc0b0145876b90385c1c88923bcd72a6380812
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 07/18] server/red_worker.c/video: add support for frames of different sizes

[Yonit Halperin]

rhbz #813826

When playing a youtube video on Windows guest, the driver sometimes sends
images which contain the video frames, but also other parts of the
screen (e.g., the youtube process bar). In order to prevent glitches, we send 
these
images as part of the stream, using SPICE_MSG_DISPLAY_STREAM_DATA_SIZED.
---
 server/mjpeg_encoder.c |   15 ++--
 server/mjpeg_encoder.h |3 +-
 server/red_worker.c|  208 
 3 files changed, 165 insertions(+), 61 deletions(-)

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 4692315..b3685f8 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -25,8 +25,6 @@
 #include jpeglib.h
 
 struct MJpegEncoder {
-int width;
-int height;
 uint8_t *row;
 int first_frame;
 int quality;
@@ -38,15 +36,13 @@ struct MJpegEncoder {
 void (*pixel_converter)(uint8_t *src, uint8_t *dest);
 };
 
-MJpegEncoder *mjpeg_encoder_new(int width, int height)
+MJpegEncoder *mjpeg_encoder_new()
 {
 MJpegEncoder *enc;
 
 enc = spice_new0(MJpegEncoder, 1);
 
 enc-first_frame = TRUE;
-enc-width = width;
-enc-height = height;
 enc-quality = 70;
 enc-cinfo.err = jpeg_std_error(enc-jerr);
 jpeg_create_compress(enc-cinfo);
@@ -200,6 +196,7 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
 /* end of code from libjpeg */
 
 int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
+  int width, int height,
   uint8_t **dest, size_t *dest_len)
 {
 encoder-cinfo.in_color_space   = JCS_RGB;
@@ -233,9 +230,9 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, 
SpiceBitmapFmt format,
 }
 
 if ((encoder-pixel_converter != NULL)  (encoder-row == NULL)) {
-unsigned int stride = encoder-width * 3;
+unsigned int stride = width * 3;
 /* check for integer overflow */
-if (stride  encoder-width) {
+if (stride  width) {
 return FALSE;
 }
 encoder-row = spice_malloc(stride);
@@ -243,8 +240,8 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, 
SpiceBitmapFmt format,
 
 spice_jpeg_mem_dest(encoder-cinfo, dest, dest_len);
 
-encoder-cinfo.image_width  = encoder-width;
-encoder-cinfo.image_height = encoder-height;
+encoder-cinfo.image_width  = width;
+encoder-cinfo.image_height = height;
 jpeg_set_defaults(encoder-cinfo);
 encoder-cinfo.dct_method   = JDCT_IFAST;
 jpeg_set_quality(encoder-cinfo, encoder-quality, TRUE);
diff --git a/server/mjpeg_encoder.h b/server/mjpeg_encoder.h
index c43827f..3a005b7 100644
--- a/server/mjpeg_encoder.h
+++ b/server/mjpeg_encoder.h
@@ -23,11 +23,12 @@
 
 typedef struct MJpegEncoder MJpegEncoder;
 
-MJpegEncoder *mjpeg_encoder_new(int width, int height);
+MJpegEncoder *mjpeg_encoder_new();
 void mjpeg_encoder_destroy(MJpegEncoder *encoder);
 
 uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder *encoder);
 int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
+  int width, int height,
   uint8_t **dest, size_t *dest_len);
 int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, uint8_t *src_pixels,
   size_t image_width);
diff --git a/server/red_worker.c b/server/red_worker.c
index 4d7f89b..306b316 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -374,6 +374,12 @@ typedef struct ImageItem {
 
 typedef struct Drawable Drawable;
 
+enum {
+STREAM_FRAME_NONE,
+STREAM_FRAME_NATIVE,
+STREAM_FRAME_CONTAINER,
+};
+
 typedef struct Stream Stream;
 struct Stream {
 uint8_t refs;
@@ -792,6 +798,7 @@ struct Drawable {
 int gradual_frames_count;
 int last_gradual_frame;
 Stream *stream;
+Stream *sized_stream;
 int streamable;
 BitmapGradualType copy_bitmap_graduality;
 uint32_t group_id;
@@ -994,7 +1001,7 @@ static void red_update_area(RedWorker *worker, const 
SpiceRect *area, int surfac
 static void red_release_cursor(RedWorker *worker, CursorItem *cursor);
 static inline void release_drawable(RedWorker *worker, Drawable *item);
 static void red_display_release_stream(RedWorker *worker, StreamAgent *agent);
-static inline void red_detach_stream(RedWorker *worker, Stream *stream);
+static inline void red_detach_stream(RedWorker *worker, Stream *stream, int 
detach_sized);
 static void red_stop_stream(RedWorker *worker, Stream *stream);
 static inline void red_stream_maintenance(RedWorker *worker, Drawable 
*candidate, Drawable *sect);
 static inline void display_begin_send_message(RedChannelClient *rcc);
@@ -1747,7 +1754,7 @@ static inline void release_drawable(RedWorker *worker, 
Drawable *drawable)
 spice_assert(ring_is_empty(drawable-pipes));
 
 if (drawable-stream) {
-red_detach_stream(worker, drawable-stream);
+red_detach_stream(worker, 

[Spice-devel] [PATCH spice 08/18] server/red_worker/video: don't override the clip in areas that belong only to sized frames

[Yonit Halperin]

---
 server/red_worker.c |   42 --
 1 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 306b316..18d6309 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -2454,31 +2454,6 @@ static StreamClipItem 
*__new_stream_clip(DisplayChannelClient* dcc, StreamAgent
 return item;
 }
 
-static void push_stream_clip_by_drawable(DisplayChannelClient* dcc, 
StreamAgent *agent,
- Drawable *drawable)
-{
-StreamClipItem *item = __new_stream_clip(dcc, agent);
-int n_rects;
-
-if (!item) {
-spice_critical(alloc failed);
-}
-
-if (drawable-red_drawable-clip.type == SPICE_CLIP_TYPE_NONE) {
-item-rects = NULL;
-item-clip_type = SPICE_CLIP_TYPE_NONE;
-item-rects = NULL;
-} else {
-item-clip_type = SPICE_CLIP_TYPE_RECTS;
-n_rects = pixman_region32_n_rects(drawable-tree_item.base.rgn);
-
-item-rects = spice_malloc_n_m(n_rects, sizeof(SpiceRect), 
sizeof(SpiceClipRects));
-item-rects-num_rects = n_rects;
-region_ret_rects(drawable-tree_item.base.rgn, item-rects-rects, 
n_rects);
-}
-red_channel_client_pipe_add(dcc-common.base, (PipeItem *)item);
-}
-
 static void push_stream_clip(DisplayChannelClient* dcc, StreamAgent *agent)
 {
 StreamClipItem *item = __new_stream_clip(dcc, agent);
@@ -2523,13 +2498,20 @@ static void red_attach_stream(RedWorker *worker, 
Drawable *drawable, Stream *str
 stream-last_time = drawable-creation_time;
 
 WORKER_FOREACH_DCC(worker, item, dcc) {
-StreamAgent *agent = dcc-stream_agents[get_stream_id(worker, 
stream)];
+StreamAgent *agent;
+QRegion clip_in_draw_dest;
 
+agent = dcc-stream_agents[get_stream_id(worker, stream)];
 region_or(agent-vis_region, drawable-tree_item.base.rgn);
-if (!region_is_equal(agent-clip, drawable-tree_item.base.rgn)) {
-region_destroy(agent-clip);
-region_clone(agent-clip, drawable-tree_item.base.rgn);
-push_stream_clip_by_drawable(dcc, agent, drawable);
+
+region_init(clip_in_draw_dest);
+region_add(clip_in_draw_dest, drawable-red_drawable-bbox);
+region_and(clip_in_draw_dest, agent-clip);
+
+if (!region_is_equal(clip_in_draw_dest, 
drawable-tree_item.base.rgn)) {
+region_remove(agent-clip, drawable-red_drawable-bbox);
+region_or(agent-clip, drawable-tree_item.base.rgn);
+push_stream_clip(dcc, agent);
 }
 }
 }
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 10/18] server/tests/test_display_base: fix two int to pointer cast warnings

[Yonit Halperin]

From: Alon Levy al...@redhat.com

---
 server/tests/test_display_base.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/tests/test_display_base.c b/server/tests/test_display_base.c
index e376195..d060f3f 100644
--- a/server/tests/test_display_base.c
+++ b/server/tests/test_display_base.c
@@ -533,7 +533,7 @@ static void do_wakeup(void *opaque)
 
 static void release_resource(QXLInstance *qin, struct QXLReleaseInfoExt 
release_info)
 {
-QXLCommandExt *ext = (unsigned long)release_info.info-id;
+QXLCommandExt *ext = (QXLCommandExt *)(unsigned long)release_info.info-id;
 //printf(%s\n, __func__);
 ASSERT(release_info.group_id == MEM_SLOT_GROUP_ID);
 switch (ext-cmd.type) {
@@ -544,7 +544,7 @@ static void release_resource(QXLInstance *qin, struct 
QXLReleaseInfoExt release_
 free(ext);
 break;
 case QXL_CMD_CURSOR: {
-QXLCursorCmd *cmd = (unsigned long)ext-cmd.data;
+QXLCursorCmd *cmd = (QXLCursorCmd *)(unsigned long)ext-cmd.data;
 if (cmd-type == QXL_CURSOR_SET) {
 free(cmd);
 }
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 11/18] server/tests: add test_get_width/test_get_height

[Yonit Halperin]

From: Alon Levy al...@redhat.com

---
 server/tests/test_display_base.c |   16 
 server/tests/test_display_base.h |3 +++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/server/tests/test_display_base.c b/server/tests/test_display_base.c
index d060f3f..fd9a37e 100644
--- a/server/tests/test_display_base.c
+++ b/server/tests/test_display_base.c
@@ -47,6 +47,9 @@ static void test_spice_destroy_update(SimpleSpiceUpdate 
*update)
 free(update);
 }
 
+static uint32_t test_width;
+static uint32_t test_height;
+
 #define DEFAULT_WIDTH 640
 #define DEFAULT_HEIGHT 320
 
@@ -320,9 +323,22 @@ static void create_primary_surface(QXLWorker *worker, 
uint32_t width,
 surface.mem= (uint64_t)g_primary_surface;
 surface.group_id   = MEM_SLOT_GROUP_ID;
 
+test_width = width;
+test_height = height;
+
 qxl_worker-create_primary_surface(qxl_worker, 0, surface);
 }
 
+uint32_t test_get_width(void)
+{
+return test_width;
+}
+
+uint32_t test_get_height(void)
+{
+return test_height;
+}
+
 QXLDevMemSlot slot = {
 .slot_group_id = MEM_SLOT_GROUP_ID,
 .slot_id = 0,
diff --git a/server/tests/test_display_base.h b/server/tests/test_display_base.h
index fa9fd18..6922d9b 100644
--- a/server/tests/test_display_base.h
+++ b/server/tests/test_display_base.h
@@ -36,6 +36,9 @@ void test_set_command_list(Command *command, int 
num_commands);
 void test_add_display_interface(SpiceServer *server);
 SpiceServer* test_init(SpiceCoreInterface* core);
 
+uint32_t test_get_width(void);
+uint32_t test_get_height(void);
+
 void spice_test_config_parse_args(int argc, char **argv);
 
 #endif /* __TEST_DISPLAY_BASE_H__ */
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 13/18] server/tests: add SIMPLE_DRAW_SOLID and SIMPLE_DRAW_BITMAP

[Yonit Halperin]

From: Alon Levy al...@redhat.com

---
 server/tests/test_display_base.c |   43 -
 server/tests/test_display_base.h |2 +
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/server/tests/test_display_base.c b/server/tests/test_display_base.c
index f7b85c1..b43859c 100644
--- a/server/tests/test_display_base.c
+++ b/server/tests/test_display_base.c
@@ -191,6 +191,27 @@ SimpleSpiceUpdate 
*test_spice_create_update_from_bitmap(uint32_t surface_id,
 return update;
 }
 
+static SimpleSpiceUpdate *test_spice_create_update_solid(uint32_t surface_id, 
QXLRect bbox, uint32_t color)
+{
+uint8_t *bitmap;
+uint32_t *dst;
+uint32_t bw;
+uint32_t bh;
+int i;
+
+bw = bbox.right - bbox.left;
+bh = bbox.bottom - bbox.top;
+
+bitmap = malloc(bw * bh * 4);
+dst = (uint32_t *)bitmap;
+
+for (i = 0 ; i  bh * bw ; ++i, ++dst) {
+*dst = color;
+}
+
+return test_spice_create_update_from_bitmap(surface_id, bbox, bitmap);
+}
+
 static SimpleSpiceUpdate *test_spice_create_update_draw(uint32_t surface_id, 
int t)
 {
 int top, left;
@@ -468,6 +489,8 @@ static void produce_command(void)
 
 /* Drawing commands, they all push a command to the command ring */
 case SIMPLE_COPY_BITS:
+case SIMPLE_DRAW_SOLID:
+case SIMPLE_DRAW_BITMAP:
 case SIMPLE_DRAW: {
 SimpleSpiceUpdate *update;
 
@@ -481,12 +504,20 @@ static void produce_command(void)
 }
 
 switch (command-command) {
-case SIMPLE_COPY_BITS:
-update = test_spice_create_update_copy_bits(0);
-break;
-case SIMPLE_DRAW:
-update = test_spice_create_update_draw(0, path.t);
-break;
+case SIMPLE_COPY_BITS:
+update = test_spice_create_update_copy_bits(0);
+break;
+case SIMPLE_DRAW:
+update = test_spice_create_update_draw(0, path.t);
+break;
+case SIMPLE_DRAW_BITMAP:
+update = 
test_spice_create_update_from_bitmap(command-bitmap.surface_id,
+command-bitmap.bbox, command-bitmap.bitmap);
+break;
+case SIMPLE_DRAW_SOLID:
+update = 
test_spice_create_update_solid(command-solid.surface_id,
+command-solid.bbox, command-solid.color);
+break;
 }
 push_command(update-ext);
 break;
diff --git a/server/tests/test_display_base.h b/server/tests/test_display_base.h
index b769721..3b24641 100644
--- a/server/tests/test_display_base.h
+++ b/server/tests/test_display_base.h
@@ -17,6 +17,8 @@ typedef enum {
 PATH_PROGRESS,
 SIMPLE_CREATE_SURFACE,
 SIMPLE_DRAW,
+SIMPLE_DRAW_BITMAP,
+SIMPLE_DRAW_SOLID,
 SIMPLE_COPY_BITS,
 SIMPLE_DESTROY_SURFACE,
 SIMPLE_UPDATE,
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 14/18] server/tests/test_display_streaming: update to create sized frames

[Yonit Halperin]

From: Alon Levy al...@redhat.com

---
 server/tests/test_display_streaming.c |   44 +++-
 1 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/server/tests/test_display_streaming.c 
b/server/tests/test_display_streaming.c
index 1b81d76..b4fe013 100644
--- a/server/tests/test_display_streaming.c
+++ b/server/tests/test_display_streaming.c
@@ -5,14 +5,37 @@
  */
 
 #include config.h
+#include stdio.h
+#include string.h
+#include assert.h
+#include unistd.h
 #include test_display_base.h
 
-int simple_commands[] = {
-SIMPLE_DRAW,
-SIMPLE_UPDATE,
-PATH_PROGRESS,
-SIMPLE_CREATE_SURFACE,
-SIMPLE_DESTROY_SURFACE,
+static int sized;
+
+void create_update(Command *command)
+{
+static int count = 0;
+CommandDrawSolid *cmd = command-solid;
+cmd-surface_id = 0;
+
+cmd-bbox.left = 0;
+cmd-bbox.right = test_get_width();
+cmd-bbox.top = 0;
+cmd-color = 0x00 + ((count * 10) % 256);
+assert(test_get_height()  50);
+cmd-bbox.bottom = test_get_height() - 50;
+if (count  20) {
+} else if (sized  count % 5 == 0) {
+cmd-bbox.bottom = test_get_height();
+cmd-color = 0xff;
+}
+count++;
+printf(%d %d\n, count, cmd-bbox.bottom);
+}
+
+static Command commands[] = {
+{SIMPLE_DRAW_SOLID, create_update},
 };
 
 SpiceCoreInterface *core;
@@ -20,12 +43,19 @@ SpiceServer *server;
 
 int main(int argc, char **argv)
 {
+int i;
 spice_test_config_parse_args(argc, argv);
+sized = 0;
+for (i = 1 ; i  argc; ++i) {
+if (strcmp(argv[i], sized) == 0) {
+sized = 1;
+}
+}
 core = basic_event_loop_init();
 server = test_init(core);
 spice_server_set_streaming_video(server, SPICE_STREAM_VIDEO_ALL);
 test_add_display_interface(server);
-test_set_simple_command_list(simple_commands, COUNT(simple_commands));
+test_set_command_list(commands, COUNT(commands));
 basic_event_loop_mainloop();
 return 0;
 }
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice 15/18] server/tests: add clip to SIMPLE_DRAW_BITMAP

[Yonit Halperin]

---
 server/tests/test_display_base.c |   34 --
 server/tests/test_display_base.h |2 ++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/server/tests/test_display_base.c b/server/tests/test_display_base.c
index b43859c..79781f7 100644
--- a/server/tests/test_display_base.c
+++ b/server/tests/test_display_base.c
@@ -43,6 +43,9 @@ static void test_spice_destroy_update(SimpleSpiceUpdate 
*update)
 if (!update) {
 return;
 }
+if (update-drawable.clip.type != SPICE_CLIP_TYPE_NONE) {
+free((uint8_t*)update-drawable.clip.data);
+}
 free(update-bitmap);
 free(update);
 }
@@ -143,9 +146,12 @@ static void draw_pos(int t, int *x, int *y)
 #endif
 }
 
-/* bitmap is freeds, so it must be allocated with malloc */
+/* bitmap and rects are freed, so they must be allocated with malloc */
 SimpleSpiceUpdate *test_spice_create_update_from_bitmap(uint32_t surface_id,
-QXLRect bbox, uint8_t *bitmap)
+QXLRect bbox,
+uint8_t *bitmap,
+uint32_t 
num_clip_rects,
+QXLRect *clip_rects)
 {
 SimpleSpiceUpdate *update;
 QXLDrawable *drawable;
@@ -163,7 +169,22 @@ SimpleSpiceUpdate 
*test_spice_create_update_from_bitmap(uint32_t surface_id,
 drawable-surface_id  = surface_id;
 
 drawable-bbox= bbox;
-drawable-clip.type   = SPICE_CLIP_TYPE_NONE;
+if (num_clip_rects == 0) {
+drawable-clip.type   = SPICE_CLIP_TYPE_NONE;
+} else {
+QXLClipRects *cmd_clip;
+
+cmd_clip = calloc(sizeof(QXLClipRects) + 
num_clip_rects*sizeof(QXLRect), 1);
+cmd_clip-num_rects = num_clip_rects;
+cmd_clip-chunk.data_size = num_clip_rects*sizeof(QXLRect);
+cmd_clip-chunk.prev_chunk = cmd_clip-chunk.next_chunk = 0;
+memcpy(cmd_clip + 1, clip_rects, cmd_clip-chunk.data_size);
+
+drawable-clip.type = SPICE_CLIP_TYPE_RECTS;
+drawable-clip.data = (intptr_t)cmd_clip;
+
+free(clip_rects);
+}
 drawable-effect  = QXL_EFFECT_OPAQUE;
 simple_set_release_info(drawable-release_info, (intptr_t)update);
 drawable-type= QXL_DRAW_COPY;
@@ -209,7 +230,7 @@ static SimpleSpiceUpdate 
*test_spice_create_update_solid(uint32_t surface_id, QX
 *dst = color;
 }
 
-return test_spice_create_update_from_bitmap(surface_id, bbox, bitmap);
+return test_spice_create_update_from_bitmap(surface_id, bbox, bitmap, 0, 
NULL);
 }
 
 static SimpleSpiceUpdate *test_spice_create_update_draw(uint32_t surface_id, 
int t)
@@ -249,7 +270,7 @@ static SimpleSpiceUpdate 
*test_spice_create_update_draw(uint32_t surface_id, int
 
 bbox.left = left; bbox.top = top;
 bbox.right = left + bw; bbox.bottom = top + bh;
-return test_spice_create_update_from_bitmap(surface_id, bbox, bitmap);
+return test_spice_create_update_from_bitmap(surface_id, bbox, bitmap, 0, 
NULL);
 }
 
 static SimpleSpiceUpdate *test_spice_create_update_copy_bits(uint32_t 
surface_id)
@@ -512,7 +533,8 @@ static void produce_command(void)
 break;
 case SIMPLE_DRAW_BITMAP:
 update = 
test_spice_create_update_from_bitmap(command-bitmap.surface_id,
-command-bitmap.bbox, command-bitmap.bitmap);
+command-bitmap.bbox, command-bitmap.bitmap,
+command-bitmap.num_clip_rects, 
command-bitmap.clip_rects);
 break;
 case SIMPLE_DRAW_SOLID:
 update = 
test_spice_create_update_solid(command-solid.surface_id,
diff --git a/server/tests/test_display_base.h b/server/tests/test_display_base.h
index 3b24641..1421c1c 100644
--- a/server/tests/test_display_base.h
+++ b/server/tests/test_display_base.h
@@ -35,6 +35,8 @@ typedef struct CommandDrawBitmap {
 QXLRect bbox;
 uint8_t *bitmap;
 uint32_t surface_id;
+uint32_t num_clip_rects;
+QXLRect *clip_rects;
 } CommandDrawBitmap;
 
 typedef struct CommandDrawSolid {
-- 
1.7.7.6

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