[Spice-devel] [PATCH spice-gtk] Fix docs for SpiceFileTransferTask::progress

[Jonathon Jongsma]

This property actually represents a fractional value from 0 to 1.0, not
a percentage between 0 and 100.
---
 src/spice-file-transfer-task.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index c414e40..a830126 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -509,7 +509,7 @@ gboolean 
spice_file_transfer_task_is_completed(SpiceFileTransferTask *self)
  * Convenience function for retrieving the current progress of this file
  * transfer task.
  *
- * Returns: A percentage value between 0 and 100
+ * Returns: A fractional value between 0 and 1.0
  *
  * Since: 0.31
  **/
@@ -757,7 +757,7 @@ 
spice_file_transfer_task_class_init(SpiceFileTransferTaskClass *klass)
  * SpiceFileTransferTask:progress:
  *
  * The current state of the file transfer. This value indicates a
- * percentage, and ranges from 0 to 100. Listen for change notifications on
+ * fraction, and ranges from 0 to 1.0. Listen for change notifications on
  * this property to be updated whenever the file transfer progress changes.
  *
  * Since: 0.31
@@ -766,7 +766,7 @@ 
spice_file_transfer_task_class_init(SpiceFileTransferTaskClass *klass)
 g_param_spec_double("progress",
 "Progress",
 "The percentage of the 
file transferred",
-0.0, 100.0, 0.0,
+0.0, 1.0, 0.0,
 G_PARAM_READABLE |
 
G_PARAM_STATIC_STRINGS));
 
-- 
2.7.4

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

[Spice-devel] [PATCH spice-gtk 1/2] Improve file transfer error messages

[Jonathon Jongsma]

In preparation for potentially displaying error messages to a user in a
UI, I thought I'd improve the messages slightly.
---
 src/channel-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/channel-main.c b/src/channel-main.c
index a074928..1ea0f71 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1854,11 +1854,11 @@ static void 
main_agent_handle_xfer_status(SpiceMainChannel *channel,
 return;
 case VD_AGENT_FILE_XFER_STATUS_CANCELLED:
 error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
-"transfer is cancelled by spice agent");
+"The spice agent cancelled the file transfer");
 break;
 case VD_AGENT_FILE_XFER_STATUS_ERROR:
 error = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
-"some errors occurred in the spice agent");
+"The spice agent reported an error during the file 
transfer");
 break;
 case VD_AGENT_FILE_XFER_STATUS_SUCCESS:
 break;
-- 
2.7.4

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

[Spice-devel] [PATCH spice-gtk 2/2] Add ability to get sizes from SpiceFileTransferTask

[Jonathon Jongsma]

If a client is handling multiple SpiceFileTransferTasks at one time,
it's not currently possible to provide a single overall progress to the
user. The only information that the client can get is the percentage
progress. This patch adds two new properties:
 - total-bytes: the size of the file transfer task in bytes
 - transferred-bytes: the number of bytes already transferred

This allows a client UI to calculate the combined progress for all
ongoing transfer tasks and present it to the user. Two convenience
functions were added to retrieve these values:
 - spice_file_transfer_task_get_total_bytes()
 - spice_file_transfer_task_get_transferred_bytes()
---
 src/channel-main.c  |  4 ++--
 src/map-file|  2 ++
 src/spice-file-transfer-task-priv.h |  2 --
 src/spice-file-transfer-task.c  | 48 +
 src/spice-file-transfer-task.h  |  2 ++
 src/spice-glib-sym-file |  2 ++
 6 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/src/channel-main.c b/src/channel-main.c
index 1ea0f71..419b0c2 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -2976,8 +2976,8 @@ static void 
file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
  * remaining bytes from transfer-size in order to keep the coherence of
  * the information we provide to the user (total-sent and 
transfer-size)
  * in the progress-callback */
-guint64 file_size = spice_file_transfer_task_get_file_size(xfer_task);
-guint64 bytes_read = 
spice_file_transfer_task_get_bytes_read(xfer_task);
+guint64 file_size = 
spice_file_transfer_task_get_total_bytes(xfer_task);
+guint64 bytes_read = 
spice_file_transfer_task_get_transferred_bytes(xfer_task);
 xfer_op->stats.transfer_size -= (file_size - bytes_read);
 if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
 xfer_op->stats.cancelled++;
diff --git a/src/map-file b/src/map-file
index 8618f6e..3d92153 100644
--- a/src/map-file
+++ b/src/map-file
@@ -37,6 +37,8 @@ spice_display_set_grab_keys;
 spice_file_transfer_task_cancel;
 spice_file_transfer_task_get_filename;
 spice_file_transfer_task_get_progress;
+spice_file_transfer_task_get_total_bytes;
+spice_file_transfer_task_get_transferred_bytes;
 spice_file_transfer_task_get_type;
 spice_get_option_group;
 spice_gl_scanout_free;
diff --git a/src/spice-file-transfer-task-priv.h 
b/src/spice-file-transfer-task-priv.h
index a7b58d8..253b3c5 100644
--- a/src/spice-file-transfer-task-priv.h
+++ b/src/spice-file-transfer-task-priv.h
@@ -50,8 +50,6 @@ gssize 
spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
 GAsyncResult *result,
 char **buffer,
 GError **error);
-guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask *self);
-guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask *self);
 gboolean spice_file_transfer_task_is_completed(SpiceFileTransferTask *self);
 
 G_END_DECLS
diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
index e7f50da..c414e40 100644
--- a/src/spice-file-transfer-task.c
+++ b/src/spice-file-transfer-task.c
@@ -73,6 +73,8 @@ enum {
 PROP_TASK_CHANNEL,
 PROP_TASK_CANCELLABLE,
 PROP_TASK_FILE,
+PROP_TASK_TOTAL_BYTES,
+PROP_TASK_TRANSFERRED_BYTES,
 PROP_TASK_PROGRESS,
 };
 
@@ -152,6 +154,7 @@ static void spice_file_transfer_task_query_info_cb(GObject 
*obj,
 
 /* SpiceFileTransferTask's init is done, handshake for file-transfer will
  * start soon. First "progress" can be emitted ~ 0% */
+g_object_notify(G_OBJECT(self), "total-bytes");
 g_object_notify(G_OBJECT(self), "progress");
 
 g_task_return_pointer(task, info, g_object_unref);
@@ -238,6 +241,7 @@ static void spice_file_transfer_task_read_stream_cb(GObject 
*source_object,
 }
 }
 
+g_object_notify(G_OBJECT(self), "transferred-bytes");
 g_task_return_int(task, nbytes);
 g_object_unref(task);
 }
@@ -349,15 +353,13 @@ GCancellable 
*spice_file_transfer_task_get_cancellable(SpiceFileTransferTask *se
 return self->cancellable;
 }
 
-G_GNUC_INTERNAL
-guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask *self)
+guint64 spice_file_transfer_task_get_total_bytes(SpiceFileTransferTask *self)
 {
 g_return_val_if_fail(self != NULL, 0);
 return self->file_size;
 }
 
-G_GNUC_INTERNAL
-guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask *self)
+guint64 spice_file_transfer_task_get_transferred_bytes(SpiceFileTransferTask 
*self)
 {
 g_return_val_if_fail(self != NULL, 0);
 return self->read_bytes;
@@ -570,6 +572,12 @@ spice_file_transfer_task_get_property(GObject *object,
 case PROP_TASK_FILE:
 g_value_set_object(value, self->file);
 break;
+ 

Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

[Frediano Ziglio]

> 
> On Thu, 11 Aug 2016, Daniel P. Berrange wrote:
> 
> > On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote:
> > > 
> > > The following patch broke compilation of xf86-video-qxl:
> > > 
> > > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb
> > > Author: Christophe Fergeau 
> > > Date:   Thu Mar 5 15:28:22 2015 +0100
> > > 
> > > Mark unused public API methods/code as deprecated
> > > 
> > > Acked-by: Jonathon Jongsma 
> > > 
> > > 
> > > The reason is it introduces a #include  in spice-server.h which
> > > is a *public* header! This means any application that needs to include
> > > spice-server.h, like xf86-video-qxl, must now check for GLib even if
> > > they don't use it, like xf86-video-qxl.
> > 
> > They shouldn't have to check for it explicitly. The spice-server.pc
> > pkgconfig file should have been updated in that commit so that glib-2.0
> > is listed under "Requires" instead of "Requires.private", then applications
> > using spice-server would not have broken, as pkg-config would have just
> > "done the right thing" and automatically added -I/path/to/glib/headers.
> 
> Even so, while being Spice tradition, the naming conflicts and code
> duplication should be fixed. So I think my patchset still stands and
> removing the need for this glib.h include is a nice side benefit.
> 

I agree on removing the header dependency.
Patch is small and make sense instead of adding the dependency to all
projects just using the headers.

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

Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

[Francois Gouget]

On Thu, 11 Aug 2016, Daniel P. Berrange wrote:

> On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote:
> > 
> > The following patch broke compilation of xf86-video-qxl:
> > 
> > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb
> > Author: Christophe Fergeau 
> > Date:   Thu Mar 5 15:28:22 2015 +0100
> > 
> > Mark unused public API methods/code as deprecated
> > 
> > Acked-by: Jonathon Jongsma 
> > 
> > 
> > The reason is it introduces a #include  in spice-server.h which 
> > is a *public* header! This means any application that needs to include 
> > spice-server.h, like xf86-video-qxl, must now check for GLib even if 
> > they don't use it, like xf86-video-qxl.
> 
> They shouldn't have to check for it explicitly. The spice-server.pc
> pkgconfig file should have been updated in that commit so that glib-2.0
> is listed under "Requires" instead of "Requires.private", then applications
> using spice-server would not have broken, as pkg-config would have just
> "done the right thing" and automatically added -I/path/to/glib/headers.

Even so, while being Spice tradition, the naming conflicts and code 
duplication should be fixed. So I think my patchset still stands and 
removing the need for this glib.h include is a nice side benefit.

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

Re: [Spice-devel] [vdagent-win v2 2/2] vdagent-win: start vdagent with lock info from session

[Victor Toso]

Hi,

On Thu, Aug 11, 2016 at 10:27:40AM -0400, Frediano Ziglio wrote:
> > 
> > Commit 5907b6cbb5c724f9729da59a644271b4258d122e started to handle
> > Lock/Unlock events from Session at VDAgent. Although that works just
> > fine, it does not cover all the situations as pointed by Andrei at [0]
> > and I quote:
> > 
> > > It fails for next test-case:
> > >
> > > * Connect with RV to VM
> > > * Lock VM (ctrl-alt-del)
> > > * Close RV
> > > * Connect with RV to the VM again
> > > * Do not unlock session. DnD files from client to locked VM.
> > >
> > > Result : Files are copied to VM.
> > 
> > [0] https://bugzilla.redhat.com/show_bug.cgi?id=1323628#c6
> > 
> > To solve this situation, we need VDService to inform VDAgent if
> > Session is Locked or not upon its initialization.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1323628
> > Signed-off-by: Victor Toso 
> > Reported-by: Andrei Stepanov 
> > ---
> >  common/vdcommon.h   |  1 +
> >  vdagent/vdagent.cpp | 16 +++-
> >  vdservice/vdservice.cpp | 15 +++
> >  3 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/common/vdcommon.h b/common/vdcommon.h
> > index f4859e2..8539c38 100644
> > --- a/common/vdcommon.h
> > +++ b/common/vdcommon.h
> > @@ -35,6 +35,7 @@ typedef CRITICAL_SECTION mutex_t;
> >  
> >  #define VD_AGENT_REGISTRY_KEY "SOFTWARE\\Red Hat\\Spice\\vdagent\\"
> >  #define VD_AGENT_STOP_EVENT   TEXT("Global\\vdagent_stop_event")
> > +#define VD_AGENT_SESSION_LOCKED_EVENT
> > TEXT("Global\\vdagent_session_locked_event")
> >  
> >  #if defined __GNUC__
> >  #define ALIGN_GCC __attribute__ ((packed))
> > diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> > index d3cbd5f..06d6574 100644
> > --- a/vdagent/vdagent.cpp
> > +++ b/vdagent/vdagent.cpp
> > @@ -142,6 +142,7 @@ private:
> >  DWORD _input_time;
> >  HANDLE _control_event;
> >  HANDLE _stop_event;
> > +HANDLE _session_locked_event;
> >  VDAgentMessage* _in_msg;
> >  uint32_t _in_msg_pos;
> >  bool _pending_input;
> > @@ -202,6 +203,7 @@ VDAgent::VDAgent()
> >  , _input_time (0)
> >  , _control_event (NULL)
> >  , _stop_event (NULL)
> > +, _session_locked_event (NULL)
> >  , _in_msg (NULL)
> >  , _in_msg_pos (0)
> >  , _pending_input (false)
> > @@ -308,6 +310,7 @@ bool VDAgent::run()
> >  return false;
> >  }
> >  _stop_event = OpenEvent(SYNCHRONIZE, FALSE, VD_AGENT_STOP_EVENT);
> > +_session_locked_event = OpenEvent(SYNCHRONIZE, FALSE,
> > VD_AGENT_SESSION_LOCKED_EVENT);
> >  memset(&wcls, 0, sizeof(wcls));
> >  wcls.lpfnWndProc = &VDAgent::wnd_proc;
> >  wcls.lpszClassName = VD_AGENT_WINCLASS_NAME;
> > @@ -482,7 +485,7 @@ void VDAgent::input_desktop_message_loop()
> >  
> >  void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
> >  {
> > -HANDLE events[2];
> > +HANDLE events[3];
> >  DWORD event_count = 1;
> >  DWORD wait_ret;
> >  DWORD action;
> > @@ -490,6 +493,7 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> > wake_mask)
> >  enum {
> >  CONTROL_ACTION,
> >  STOP_ACTION,
> > +SESSION_LOCKED_ACTION,
> >  } actions[G_N_ELEMENTS(events)];
> >  
> >  events[0] = _control_event;
> > @@ -500,6 +504,12 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> > wake_mask)
> >  event_count++;
> >  }
> >  
> > +if (_session_locked_event) {
> > +events[event_count] = _session_locked_event;
> > +actions[event_count] = SESSION_LOCKED_ACTION;
> > +event_count++;
> > +}
> > +
> >  wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout,
> >  wake_mask, MWMO_ALERTABLE);
> >  if (wait_ret == WAIT_OBJECT_0 + event_count) {
> >  while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
> > @@ -524,6 +534,10 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> > wake_mask)
> >  vd_printf("%s: received stop event", __func__);
> >  _running = false;
> >  break;
> > +case SESSION_LOCKED_ACTION:
> > +vd_printf("%s: received session locked event", __func__);
> > +_session_is_locked = true;
> > +break;
> >  default:
> >  vd_printf("%s: action not handled (%lu)", __func__, action);
> >  _running = false;
> > diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> > index 89e0dbb..1f53d97 100644
> > --- a/vdservice/vdservice.cpp
> > +++ b/vdservice/vdservice.cpp
> > @@ -93,6 +93,7 @@ private:
> >  PROCESS_INFORMATION _agent_proc_info;
> >  HANDLE _control_event;
> >  HANDLE _agent_stop_event;
> > +HANDLE _session_locked_event;
> >  HANDLE* _events;
> >  TCHAR _agent_path[MAX_PATH];
> >  VDControlQueue _control_queue;
> > @@ -105,6 +106,7 @@ private:
> >  int _system_version;
> >  bool _agent_alive;
> >  bool _running;
> > +bool _session_is_locked;
> >  VDLog* _log;
> >  unsigned _events_c

Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

[Daniel P. Berrange]

On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote:
> 
> The following patch broke compilation of xf86-video-qxl:
> 
> commit b41220b1441b8adea6db9a98e9da1b43a8f426bb
> Author: Christophe Fergeau 
> Date:   Thu Mar 5 15:28:22 2015 +0100
> 
> Mark unused public API methods/code as deprecated
> 
> Acked-by: Jonathon Jongsma 
> 
> 
> The reason is it introduces a #include  in spice-server.h which 
> is a *public* header! This means any application that needs to include 
> spice-server.h, like xf86-video-qxl, must now check for GLib even if 
> they don't use it, like xf86-video-qxl.

They shouldn't have to check for it explicitly. The spice-server.pc
pkgconfig file should have been updated in that commit so that glib-2.0
is listed under "Requires" instead of "Requires.private", then applications
using spice-server would not have broken, as pkg-config would have just
"done the right thing" and automatically added -I/path/to/glib/headers.

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

[Spice-devel] [client 3/3] spice-gtk: Use spice-protocol's SPICE_GNUC_DEPRECATED* macros

[Francois Gouget]

- spice-protocol's macros have been improved to automatically work with
  GLib if available so there is no longer any justification for
  duplicating them in spice-gtk.
- This gets rid of spice-gtk's SPICE_DEPRECATED macro which was causing
  a naming conflict with the corresponding spice-protocol compilation
  directive.
- Finally replace SPICE_NO_DEPRECATED with spice-protocol's
  SPICE_DEPRECATED macro which has the exact same meaning despite the
  missing NO.

Signed-off-by: Francois Gouget 
---
 src/Makefile.am  |  2 +-
 src/channel-main.h   |  8 
 src/spice-audio.h|  2 +-
 src/spice-channel.h  |  5 +++--
 src/spice-util.h | 20 ++--
 src/usb-device-manager.h |  2 +-
 6 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index a820259..dddf9e3 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -77,7 +77,7 @@ KEYMAP_GEN = $(srcdir)/keymap-gen.pl
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
-DG_LOG_DOMAIN=\"GSpice\"   \
-   -DSPICE_NO_DEPRECATED   \
+   -DSPICE_DEPRECATED  \
-DSPICE_GTK_LOCALEDIR=\"${SPICE_GTK_LOCALEDIR}\"\
-DPNP_IDS=\""$(PNP_IDS)"\"  \
-DUSB_IDS=\""$(USB_IDS)"\"  \
diff --git a/src/channel-main.h b/src/channel-main.h
index 3fe8df1..3d17793 100644
--- a/src/channel-main.h
+++ b/src/channel-main.h
@@ -101,13 +101,13 @@ gboolean spice_main_file_copy_finish(SpiceMainChannel 
*channel,
 void spice_main_request_mouse_mode(SpiceMainChannel *channel, int mode);
 
 #ifndef SPICE_DISABLE_DEPRECATED
-SPICE_DEPRECATED_FOR(spice_main_clipboard_selection_grab)
+SPICE_GNUC_DEPRECATED_FOR(spice_main_clipboard_selection_grab)
 void spice_main_clipboard_grab(SpiceMainChannel *channel, guint32 *types, int 
ntypes);
-SPICE_DEPRECATED_FOR(spice_main_clipboard_selection_release)
+SPICE_GNUC_DEPRECATED_FOR(spice_main_clipboard_selection_release)
 void spice_main_clipboard_release(SpiceMainChannel *channel);
-SPICE_DEPRECATED_FOR(spice_main_clipboard_selection_notify)
+SPICE_GNUC_DEPRECATED_FOR(spice_main_clipboard_selection_notify)
 void spice_main_clipboard_notify(SpiceMainChannel *channel, guint32 type, 
const guchar *data, size_t size);
-SPICE_DEPRECATED_FOR(spice_main_clipboard_selection_request)
+SPICE_GNUC_DEPRECATED_FOR(spice_main_clipboard_selection_request)
 void spice_main_clipboard_request(SpiceMainChannel *channel, guint32 type);
 #endif
 
diff --git a/src/spice-audio.h b/src/spice-audio.h
index 01f564a..6bd9a55 100644
--- a/src/spice-audio.h
+++ b/src/spice-audio.h
@@ -104,7 +104,7 @@ GType spice_audio_get_type(void);
 SpiceAudio* spice_audio_get(SpiceSession *session, GMainContext *context);
 
 #ifndef SPICE_DISABLE_DEPRECATED
-SPICE_DEPRECATED_FOR(spice_audio_get)
+SPICE_GNUC_DEPRECATED_FOR(spice_audio_get)
 SpiceAudio* spice_audio_new(SpiceSession *session, GMainContext *context, 
const char *name);
 #endif
 
diff --git a/src/spice-channel.h b/src/spice-channel.h
index 3b0bffb..aa0d9cb 100644
--- a/src/spice-channel.h
+++ b/src/spice-channel.h
@@ -24,6 +24,7 @@
 
 #include 
 
+#include 
 #include "spice-types.h"
 #include "spice-glib-enums.h"
 #include "spice-util.h"
@@ -136,9 +137,9 @@ gboolean spice_channel_test_common_capability(SpiceChannel 
*channel, guint32 cap
 void spice_channel_flush_async(SpiceChannel *channel, GCancellable 
*cancellable, GAsyncReadyCallback callback, gpointer user_data);
 gboolean spice_channel_flush_finish(SpiceChannel *channel, GAsyncResult 
*result, GError **error);
 #ifndef SPICE_DISABLE_DEPRECATED
-SPICE_DEPRECATED
+SPICE_GNUC_DEPRECATED
 void spice_channel_set_capability(SpiceChannel *channel, guint32 cap);
-SPICE_DEPRECATED
+SPICE_GNUC_DEPRECATED
 void spice_channel_destroy(SpiceChannel *channel);
 #endif
 
diff --git a/src/spice-util.h b/src/spice-util.h
index 88e3a57..7fc4c25 100644
--- a/src/spice-util.h
+++ b/src/spice-util.h
@@ -20,6 +20,8 @@
 
 #include 
 
+#include 
+
 G_BEGIN_DECLS
 
 void spice_util_set_debug(gboolean enabled);
@@ -40,24 +42,6 @@ gchar* spice_uuid_to_string(const guint8 uuid[16]);
 
 #define SPICE_RESERVED_PADDING (10 * sizeof(void*))
 
-/* need to be in a public header */
-#ifndef SPICE_GNUC_DEPRECATED_FOR
-#if__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
-#define SPICE_GNUC_DEPRECATED_FOR(f)\
-  __attribute__((deprecated("Use " #f " instead")))
-#else
-#define SPICE_GNUC_DEPRECATED_FOR(f)G_GNUC_DEPRECATED
-#endif /* __GNUC__ */
-#endif
-
-#ifndef SPICE_NO_DEPRECATED
-#define SPICE_DEPRECATED_FOR(f)  SPICE_GNUC_DEPRECATED_FOR(f)
-#define SPICE_DEPRECATED  G_GNUC_DEPRECATED
-#else
-#define SPICE_DEPRECATED_FOR(f)
-#define SPICE_DEPRECATED
-#endif
-
 G_END_DECLS
 
 #endif /* SPICE_UTIL_H */
diff --g

[Spice-devel] [spice 2/3] server: Use SPICE_GNUC_DEPRECATED to avoid a dependency on glib.h

[Francois Gouget]

spice-server.h cannot include glib.h because it is a public header and 
is used by projects that do not use GLib.

Signed-off-by: Francois Gouget 
---
 server/spice-migration.h |  4 ++--
 server/spice-server.h| 13 ++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/server/spice-migration.h b/server/spice-migration.h
index 9528f0e..58b1cbb 100644
--- a/server/spice-migration.h
+++ b/server/spice-migration.h
@@ -46,8 +46,8 @@ struct SpiceMigrateInstance {
 /* spice switch-host client migration */
 int spice_server_migrate_info(SpiceServer *s, const char* dest,
   int port, int secure_port,
-  const char* cert_subject) G_GNUC_DEPRECATED;
-int spice_server_migrate_switch(SpiceServer *s) G_GNUC_DEPRECATED;
+  const char* cert_subject) SPICE_GNUC_DEPRECATED;
+int spice_server_migrate_switch(SpiceServer *s) SPICE_GNUC_DEPRECATED;
 
 /* spice (semi-)seamless client migration */
 int spice_server_migrate_connect(SpiceServer *s, const char* dest,
diff --git a/server/spice-server.h b/server/spice-server.h
index 6eb1b1d..4d1703b 100644
--- a/server/spice-server.h
+++ b/server/spice-server.h
@@ -22,8 +22,7 @@
 #error "Only spice.h can be included directly."
 #endif
 
-#include 
-
+#include 
 #include "spice-core.h"
 
 /* Don't use features incompatible with a specific spice
@@ -50,7 +49,7 @@ int spice_server_set_compat_version(SpiceServer *s,
 spice_compat_version_t version);
 int spice_server_set_port(SpiceServer *s, int port);
 void spice_server_set_addr(SpiceServer *s, const char *addr, int flags);
-int spice_server_set_listen_socket_fd(SpiceServer *s, int listen_fd) 
G_GNUC_DEPRECATED;
+int spice_server_set_listen_socket_fd(SpiceServer *s, int listen_fd) 
SPICE_GNUC_DEPRECATED;
 int spice_server_set_exit_on_disconnect(SpiceServer *s, int flag);
 int spice_server_set_noauth(SpiceServer *s);
 int spice_server_set_sasl(SpiceServer *s, int enabled);
@@ -104,7 +103,7 @@ int spice_server_set_zlib_glz_compression(SpiceServer *s, 
spice_wan_compression_
 
 int spice_server_set_channel_security(SpiceServer *s, const char *channel, int 
security);
 
-int spice_server_add_renderer(SpiceServer *s, const char *name) 
G_GNUC_DEPRECATED;
+int spice_server_add_renderer(SpiceServer *s, const char *name) 
SPICE_GNUC_DEPRECATED;
 
 enum {
 SPICE_STREAM_VIDEO_INVALID,
@@ -127,8 +126,8 @@ int spice_server_set_agent_mouse(SpiceServer *s, int 
enable);
 int spice_server_set_agent_copypaste(SpiceServer *s, int enable);
 int spice_server_set_agent_file_xfer(SpiceServer *s, int enable);
 
-int spice_server_get_sock_info(SpiceServer *s, struct sockaddr *sa, socklen_t 
*salen) G_GNUC_DEPRECATED;
-int spice_server_get_peer_info(SpiceServer *s, struct sockaddr *sa, socklen_t 
*salen) G_GNUC_DEPRECATED;
+int spice_server_get_sock_info(SpiceServer *s, struct sockaddr *sa, socklen_t 
*salen) SPICE_GNUC_DEPRECATED;
+int spice_server_get_peer_info(SpiceServer *s, struct sockaddr *sa, socklen_t 
*salen) SPICE_GNUC_DEPRECATED;
 
 int spice_server_is_server_mouse(SpiceServer *s);
 
@@ -138,6 +137,6 @@ void spice_server_set_uuid(SpiceServer *s, const uint8_t 
uuid[16]);
 void spice_server_vm_start(SpiceServer *s);
 void spice_server_vm_stop(SpiceServer *s);
 
-int spice_server_get_num_clients(SpiceServer *s) G_GNUC_DEPRECATED;
+int spice_server_get_num_clients(SpiceServer *s) SPICE_GNUC_DEPRECATED;
 
 #endif /* SPICE_SERVER_H_ */
-- 
2.8.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [protocol 1/3] macros: Improve the SPICE_GNUC_DEPRECATED* macros

[Francois Gouget]

If the user specifically requests access to the deprecated APIs by
defining the SPICE_DEPRECATED macro, then turn off the
SPICE_GNUC_DEPRECATED* warnings.
Also automatically use G_GNUC_DEPRECATED if available.
Add SPICE_GNUC_DEPRECATED_FOR().

Signed-off-by: Francois Gouget 
---
 spice/macros.h | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/spice/macros.h b/spice/macros.h
index a0413f3..6e2dbce 100644
--- a/spice/macros.h
+++ b/spice/macros.h
@@ -78,11 +78,25 @@
 #define SPICE_GNUC_NO_INSTRUMENT
 #endif  /* !__GNUC__ */
 
-#if__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1)
-#define SPICE_GNUC_DEPRECATED  __attribute__((__deprecated__))
+#if defined(SPICE_DEPRECATED)
+/* Don't warn when deprecated APIs have been explicitly asked for */
+#  define SPICE_GNUC_DEPRECATED
+#elif   defined(G_GNUC_DEPRECATED)
+#  define SPICE_GNUC_DEPRECATED G_GNUC_DEPRECATED
+#elif   __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1)
+#  define SPICE_GNUC_DEPRECATED  __attribute__((__deprecated__))
 #else
-#define SPICE_GNUC_DEPRECATED
-#endif /* __GNUC__ */
+#  define SPICE_GNUC_DEPRECATED
+#endif  /* __GNUC__ */
+
+#if defined(SPICE_DEPRECATED)
+/* Don't warn when deprecated APIs have been explicitly asked for */
+#  define SPICE_GNUC_DEPRECATED_FOR(f)
+#elif   __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
+#  define SPICE_GNUC_DEPRECATED_FOR(f) __attribute__((deprecated("Use " #f " 
instead")))
+#else
+#  define SPICE_GNUC_DEPRECATED_FOR(f) SPICE_GNUC_DEPRECATED
+#endif  /* __GNUC__ */
 
 #if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 3)
 #  define SPICE_GNUC_MAY_ALIAS __attribute__((may_alias))
-- 
2.8.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

[Francois Gouget]

The following patch broke compilation of xf86-video-qxl:

commit b41220b1441b8adea6db9a98e9da1b43a8f426bb
Author: Christophe Fergeau 
Date:   Thu Mar 5 15:28:22 2015 +0100

Mark unused public API methods/code as deprecated

Acked-by: Jonathon Jongsma 


The reason is it introduces a #include  in spice-server.h which 
is a *public* header! This means any application that needs to include 
spice-server.h, like xf86-video-qxl, must now check for GLib even if 
they don't use it, like xf86-video-qxl.

This does not make sense and thus the glib.h include must be removed. 
However it was added to get G_GNUC_DEPRECATED. Fortunately there is 
SPICE_GNUC_DEPRECATED, or maybe that's SPICE_DEPRECATED? It gets a bit 
confusing from here:

First the macros to tag deprecated functions:
* spice uses G_GNUC_DEPRECATED directly in spice-server.h as seen above.

* spice-gtk defines SPICE_DEPRECATED in spice-util.h as a wrapper around 
  G_GNUC_DEPRECATED, or an empty macro if SPICE_NO_DEPRECATED is 
  defined.

* spice-gtk also has a SPICE_GNUC_DEPRECATED_FOR() macro which is not 
  defined anywhere else and a corresponding wrapper, 
  SPICE_DEPRECATED_FOR(), which is a no-op if SPICE_NO_DEPRECATED is 
  defined.

* spice-protocol defines SPICE_GNUC_DEPRECATED in spice/macro.h but 
  uses the gcc __attribute__ macro directly instead of using 
  G_GNUC_DEPRECATED.


Second the macros to 'enable/disable' deprecated APIs:
* In spice-gtk defining SPICE_NO_DEPRECATED disables the warnings.

* In spice-protocol you must define SPICE_DEPRECATED to get access to 
  the deprecated vd_agent.h VD_AGENT_CLIPBOARD_MAX_SIZE_DEFAULT and 
  VD_AGENT_CLIPBOARD_MAX_SIZE_ENV macros.

Of course spice-gtk's source files use both spice-util.h and vd_agent.h, 
meaning they necessarily get the spice-protocol deprecated macros due to 
the SPICE_DEPRECATED naming conflict.


So here is the proposed solution:

* spice-protocol's SPICE_DEPRECATED is in a public header so keep it as 
  is. Extend it to mean the user wants to use deprecated APIs. This 
  includes:
  - Defining deprecated APIs and macros (as before).
  - Disabling warnings about the use of deprecated APIs so it takes 
over spice-gtk's SPICE_NO_DEPRECATED role.

* Disable spice-protocol's SPICE_GNUC_DEPRECATED warnings when 
  SPICE_DEPRECATED is defined. 

* Add SPICE_GNUC_DEPRECATED_FOR() to spice-protocol next to 
  SPICE_GNUC_DEPRECATED.

* Extend spice-protocol's SPICE_GNUC_DEPRECATED to use G_GNUC_DEPRECATED 
  if available. This makes it a portable drop-in replacement for 
  G_GNUC_DEPRECATED in Spice's code, particularly in spice-server.h.

This implies spice and spice-gtk will need an up-to-date spice-protocol 
to compile.

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

Re: [Spice-devel] [vdagent-win v2 2/2] vdagent-win: start vdagent with lock info from session

[Frediano Ziglio]

> 
> Commit 5907b6cbb5c724f9729da59a644271b4258d122e started to handle
> Lock/Unlock events from Session at VDAgent. Although that works just
> fine, it does not cover all the situations as pointed by Andrei at [0]
> and I quote:
> 
> > It fails for next test-case:
> >
> > * Connect with RV to VM
> > * Lock VM (ctrl-alt-del)
> > * Close RV
> > * Connect with RV to the VM again
> > * Do not unlock session. DnD files from client to locked VM.
> >
> > Result : Files are copied to VM.
> 
> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1323628#c6
> 
> To solve this situation, we need VDService to inform VDAgent if
> Session is Locked or not upon its initialization.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1323628
> Signed-off-by: Victor Toso 
> Reported-by: Andrei Stepanov 
> ---
>  common/vdcommon.h   |  1 +
>  vdagent/vdagent.cpp | 16 +++-
>  vdservice/vdservice.cpp | 15 +++
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/common/vdcommon.h b/common/vdcommon.h
> index f4859e2..8539c38 100644
> --- a/common/vdcommon.h
> +++ b/common/vdcommon.h
> @@ -35,6 +35,7 @@ typedef CRITICAL_SECTION mutex_t;
>  
>  #define VD_AGENT_REGISTRY_KEY "SOFTWARE\\Red Hat\\Spice\\vdagent\\"
>  #define VD_AGENT_STOP_EVENT   TEXT("Global\\vdagent_stop_event")
> +#define VD_AGENT_SESSION_LOCKED_EVENT
> TEXT("Global\\vdagent_session_locked_event")
>  
>  #if defined __GNUC__
>  #define ALIGN_GCC __attribute__ ((packed))
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index d3cbd5f..06d6574 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -142,6 +142,7 @@ private:
>  DWORD _input_time;
>  HANDLE _control_event;
>  HANDLE _stop_event;
> +HANDLE _session_locked_event;
>  VDAgentMessage* _in_msg;
>  uint32_t _in_msg_pos;
>  bool _pending_input;
> @@ -202,6 +203,7 @@ VDAgent::VDAgent()
>  , _input_time (0)
>  , _control_event (NULL)
>  , _stop_event (NULL)
> +, _session_locked_event (NULL)
>  , _in_msg (NULL)
>  , _in_msg_pos (0)
>  , _pending_input (false)
> @@ -308,6 +310,7 @@ bool VDAgent::run()
>  return false;
>  }
>  _stop_event = OpenEvent(SYNCHRONIZE, FALSE, VD_AGENT_STOP_EVENT);
> +_session_locked_event = OpenEvent(SYNCHRONIZE, FALSE,
> VD_AGENT_SESSION_LOCKED_EVENT);
>  memset(&wcls, 0, sizeof(wcls));
>  wcls.lpfnWndProc = &VDAgent::wnd_proc;
>  wcls.lpszClassName = VD_AGENT_WINCLASS_NAME;
> @@ -482,7 +485,7 @@ void VDAgent::input_desktop_message_loop()
>  
>  void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
>  {
> -HANDLE events[2];
> +HANDLE events[3];
>  DWORD event_count = 1;
>  DWORD wait_ret;
>  DWORD action;
> @@ -490,6 +493,7 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>  enum {
>  CONTROL_ACTION,
>  STOP_ACTION,
> +SESSION_LOCKED_ACTION,
>  } actions[G_N_ELEMENTS(events)];
>  
>  events[0] = _control_event;
> @@ -500,6 +504,12 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>  event_count++;
>  }
>  
> +if (_session_locked_event) {
> +events[event_count] = _session_locked_event;
> +actions[event_count] = SESSION_LOCKED_ACTION;
> +event_count++;
> +}
> +
>  wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout,
>  wake_mask, MWMO_ALERTABLE);
>  if (wait_ret == WAIT_OBJECT_0 + event_count) {
>  while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
> @@ -524,6 +534,10 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>  vd_printf("%s: received stop event", __func__);
>  _running = false;
>  break;
> +case SESSION_LOCKED_ACTION:
> +vd_printf("%s: received session locked event", __func__);
> +_session_is_locked = true;
> +break;
>  default:
>  vd_printf("%s: action not handled (%lu)", __func__, action);
>  _running = false;
> diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> index 89e0dbb..1f53d97 100644
> --- a/vdservice/vdservice.cpp
> +++ b/vdservice/vdservice.cpp
> @@ -93,6 +93,7 @@ private:
>  PROCESS_INFORMATION _agent_proc_info;
>  HANDLE _control_event;
>  HANDLE _agent_stop_event;
> +HANDLE _session_locked_event;
>  HANDLE* _events;
>  TCHAR _agent_path[MAX_PATH];
>  VDControlQueue _control_queue;
> @@ -105,6 +106,7 @@ private:
>  int _system_version;
>  bool _agent_alive;
>  bool _running;
> +bool _session_is_locked;
>  VDLog* _log;
>  unsigned _events_count;
>  };
> @@ -128,6 +130,7 @@ VDService::VDService()
>  , _agent_restarts (0)
>  , _agent_alive (false)
>  , _running (false)
> +, _session_is_locked (false)
>  , _log (NULL)
>  , _events_count(0)
>  {
> @@ -135,6 +138,7 @@ VDService::VDService()
>  _system_version = supported_system_version();
>  _contro

Re: [Spice-devel] [vdagent-win v2 1/2] vdagent: rework on event_dispatcher

[Frediano Ziglio]

> 
> As _stop_event is not mandatory, we are initializing the events array
> with something that might be NULL. The problem is with the following
> patch as I'm introducing a new non mandatory event and logic starts to
> get a bit hard to follow.
> 
> So this patch makes explicit if we are setting or receiving a
> _stop_event or not and uses an auxiliary enum and array 'actions' to
> work around this non mandatory events and the return value of
> MsgWaitForMultipleObjectsEx()
> ---
>  vdagent/vdagent.cpp | 39 ++-
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index 959881d..d3cbd5f 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -478,12 +478,27 @@ void VDAgent::input_desktop_message_loop()
>  CloseDesktop(hdesk);
>  }
>  
> +#define G_N_ELEMENTS(arr)  (sizeof (arr) / sizeof ((arr)[0]))
> +

Oh.. you can use SPICE_N_ELEMENTS, should be available

>  void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
>  {
> -HANDLE events[] = {_control_event, _stop_event};
> -DWORD event_count = _stop_event ? 2 : 1;
> +HANDLE events[2];
> +DWORD event_count = 1;
>  DWORD wait_ret;
> +DWORD action;

action should be of enumerator type ...

>  MSG msg;
> +enum {
> +CONTROL_ACTION,
> +STOP_ACTION,
> +} actions[G_N_ELEMENTS(events)];

... I would put here as

} actions[G_N_ELEMENTS(events)], action;

> +
> +events[0] = _control_event;
> +actions[0] = CONTROL_ACTION;
> +if (_stop_event) {
> +events[event_count] = _stop_event;
> +actions[event_count] = STOP_ACTION;
> +event_count++;
> +}
>  
>  wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout,
>  wake_mask, MWMO_ALERTABLE);
>  if (wait_ret == WAIT_OBJECT_0 + event_count) {
> @@ -492,19 +507,25 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>  DispatchMessage(&msg);
>  }
>  return;
> +} else if (wait_ret == WAIT_IO_COMPLETION || wait_ret == WAIT_TIMEOUT) {
> +return;
> +} else if (wait_ret < WAIT_OBJECT_0 || wait_ret > WAIT_OBJECT_0 +
> event_count) {
> +vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret,
> GetLastError());
> +_running = false;
> +return;
>  }
> -switch (wait_ret) {
> -case WAIT_OBJECT_0:
> +
> +action = actions[wait_ret - WAIT_OBJECT_0];
> +switch (action) {
> +case CONTROL_ACTION:
>  handle_control_event();
>  break;
> -case WAIT_OBJECT_0 + 1:
> +case STOP_ACTION:
> +vd_printf("%s: received stop event", __func__);
>  _running = false;
>  break;
> -case WAIT_IO_COMPLETION:
> -case WAIT_TIMEOUT:
> -break;
>  default:
> -vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret,
> GetLastError());
> +vd_printf("%s: action not handled (%lu)", __func__, action);
>  _running = false;
>  }
>  }

Beside these small changes looks fine.

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [PATCH 1/2] Avoids to initialise OpenSSL threading twice

[Pavel Grunt]

Ack

On Thu, 2016-08-11 at 14:22 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 6f88649..f74c8d3 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2792,6 +2792,13 @@ static void openssl_thread_setup(void)
>  {
>  int i;
>  
> +/* Somebody else already setup threading for OpenSSL,
> + * don't do it twice to avoid possible races.
> + */
> +if (CRYPTO_get_locking_callback() != NULL) {
> +return;
> +}
> +
>  lock_cs = OPENSSL_malloc(CRYPTO_num_locks() * sizeof(pthread_mutex_t));
>  
>  for (i = 0; i < CRYPTO_num_locks(); i++) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2] gstreamer: Use a dummy format to make sure format is always defined.

[Pavel Grunt]

On Thu, 2016-08-11 at 14:22 +0100, Frediano Ziglio wrote:
> This avoid a check for NULL.
> Also will be used to catch invalid values when table will be extended.
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/gstreamer-encoder.c | 36 +---
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> Changes from v1:
> - missed a check (actually was in another patch I had).
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 18cc6a7..d575c67 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -747,31 +747,36 @@ static inline void
> server_increase_bit_rate(SpiceGstEncoder *encoder,
>  
>  /* -- GStreamer pipeline -- */
>  
> +/* See GStreamer's part-mediatype-video-raw.txt and
> + * section-types-definitions.html documents.
> + */
> +static const SpiceFormatForGStreamer format_map[] =  {
> +/* First item is invalid.
> + * It's located first so the loop catch invalid values.
> + */
> +{SPICE_BITMAP_FMT_INVALID, "", 0, 0, 0, 0, 0, 0},
> +{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> +{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0, 0x7C00},
> +/* TODO: Test the other formats */
> +{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> +{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
> +};
> +#define GSTREAMER_FORMAT_INVALID (&format_map[0])
> +
>  /* A helper for spice_gst_encoder_encode_frame() */
>  static const SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
>  {
> -/* See GStreamer's part-mediatype-video-raw.txt and
> - * section-types-definitions.html documents.
> - */
> -static const SpiceFormatForGStreamer format_map[] =  {
> -{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> -{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0,
> 0x7C00},
> -/* TODO: Test the other formats */
> -{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> -{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00,
> 0xff},
> -};
> -
>  int i;
>  for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
>  if (format_map[i].spice_format == format) {
> -if (i > 1) {
> +if (i > 2) {
>  spice_warning("The %d format has not been tested yet",
> format);
>  }
>  return &format_map[i];
>  }
>  }
>  
> -return NULL;
> +return GSTREAMER_FORMAT_INVALID;
>  }
>  
>  static void set_appsrc_caps(SpiceGstEncoder *encoder)
> @@ -1457,7 +1462,7 @@ static int spice_gst_encoder_encode_frame(VideoEncoder
> *video_encoder,
>  encoder->width, width, encoder->height, height,
>  encoder->spice_format, bitmap->format);
>  encoder->format = map_format(bitmap->format);
> -if (!encoder->format) {
> +if (encoder->format == GSTREAMER_FORMAT_INVALID) {
>  spice_warning("unable to map format type %d", bitmap->format);
>  encoder->errors = 4;
>  return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> @@ -1663,7 +1668,7 @@ static void spice_gst_encoder_get_stats(VideoEncoder
> *video_encoder,
>  VideoEncoderStats *stats)
>  {
>  SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
> -uint64_t raw_bit_rate = encoder->width * encoder->height * (encoder-
> >format ? encoder->format->bpp : 0) * get_source_fps(encoder);
> +uint64_t raw_bit_rate = encoder->width * encoder->height * encoder-
> >format->bpp * get_source_fps(encoder);
>  
>  spice_return_if_fail(stats != NULL);
>  stats->starting_bit_rate = encoder->starting_bit_rate;
> @@ -1712,6 +1717,7 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType
> codec_type,
>  encoder->starting_bit_rate = starting_bit_rate;
>  encoder->bitmap_ref = bitmap_ref;
>  encoder->bitmap_unref = bitmap_unref;
> +encoder->format = GSTREAMER_FORMAT_INVALID;
>  g_mutex_init(&encoder->outbuf_mutex);
>  g_cond_init(&encoder->outbuf_cond);
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win v1 1/2] file-transfer: fix typos

[Frediano Ziglio]

> 
> Hi,
> 
> On Thu, Aug 11, 2016 at 08:56:11AM -0400, Frediano Ziglio wrote:
> > > 
> > > ---
> > >  vdagent/file_xfer.cpp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> > > index 58d3a31..cbf7e5e 100644
> > > --- a/vdagent/file_xfer.cpp
> > > +++ b/vdagent/file_xfer.cpp
> > > @@ -86,7 +86,7 @@ void
> > > FileXfer::handle_start(VDAgentFileXferStartMessage*
> > > start,
> > >  
> > >  wlen = _tcslen(file_path);
> > >  // make sure we have enough space
> > > -// (1 char for separator, 1 char for filename and 1 char for NUL
> > > terminator)
> > > +// (1 char for separator, 1 char for filename and 1 char for NULL
> > > terminator)
> >
> > Ehm.. no, a string is NUL terminated, is not an array of pointers
> > but an array of characters and NUL is the character with 0 encoding :)
> >
> > Actually I had a doubt about EBCDIC, from
> > https://it.wikipedia.org/wiki/EBCDIC
> > looks like NUL is 0 even on EBCDIC.
> 
> Ah, sure. I removed this one from the patch. Thanks!
> 
> >
> > >  if (wlen + 3 >= MAX_PATH) {
> > >  vd_printf("error: file too long %ls\\%s", file_path, file_name);
> > >  return;
> > > @@ -159,7 +159,7 @@ void
> > > FileXfer::handle_status(VDAgentFileXferStatusMessage* status)
> > >  
> > >  vd_printf("id %u result %u", status->id, status->result);
> > >  if (status->result != VD_AGENT_FILE_XFER_STATUS_CANCELLED) {
> > > -vd_printf("only cancel is premitted");
> > > +vd_printf("only cancel is permitted");
> > >  return;
> > >  }
> > >  iter = _tasks.find(status->id);
> > 
> > Frediano
> 

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [vdagent-win v1 1/2] file-transfer: fix typos

[Victor Toso]

Hi,

On Thu, Aug 11, 2016 at 08:56:11AM -0400, Frediano Ziglio wrote:
> > 
> > ---
> >  vdagent/file_xfer.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> > index 58d3a31..cbf7e5e 100644
> > --- a/vdagent/file_xfer.cpp
> > +++ b/vdagent/file_xfer.cpp
> > @@ -86,7 +86,7 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage*
> > start,
> >  
> >  wlen = _tcslen(file_path);
> >  // make sure we have enough space
> > -// (1 char for separator, 1 char for filename and 1 char for NUL
> > terminator)
> > +// (1 char for separator, 1 char for filename and 1 char for NULL
> > terminator)
>
> Ehm.. no, a string is NUL terminated, is not an array of pointers
> but an array of characters and NUL is the character with 0 encoding :)
>
> Actually I had a doubt about EBCDIC, from https://it.wikipedia.org/wiki/EBCDIC
> looks like NUL is 0 even on EBCDIC.

Ah, sure. I removed this one from the patch. Thanks!

>
> >  if (wlen + 3 >= MAX_PATH) {
> >  vd_printf("error: file too long %ls\\%s", file_path, file_name);
> >  return;
> > @@ -159,7 +159,7 @@ void
> > FileXfer::handle_status(VDAgentFileXferStatusMessage* status)
> >  
> >  vd_printf("id %u result %u", status->id, status->result);
> >  if (status->result != VD_AGENT_FILE_XFER_STATUS_CANCELLED) {
> > -vd_printf("only cancel is premitted");
> > +vd_printf("only cancel is permitted");
> >  return;
> >  }
> >  iter = _tasks.find(status->id);
> 
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v2] gstreamer: Use a dummy format to make sure format is always defined.

[Frediano Ziglio]

This avoid a check for NULL.
Also will be used to catch invalid values when table will be extended.

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

Changes from v1:
- missed a check (actually was in another patch I had).

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 18cc6a7..d575c67 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -747,31 +747,36 @@ static inline void 
server_increase_bit_rate(SpiceGstEncoder *encoder,
 
 /* -- GStreamer pipeline -- */
 
+/* See GStreamer's part-mediatype-video-raw.txt and
+ * section-types-definitions.html documents.
+ */
+static const SpiceFormatForGStreamer format_map[] =  {
+/* First item is invalid.
+ * It's located first so the loop catch invalid values.
+ */
+{SPICE_BITMAP_FMT_INVALID, "", 0, 0, 0, 0, 0, 0},
+{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
+{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0, 0x7C00},
+/* TODO: Test the other formats */
+{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
+{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
+};
+#define GSTREAMER_FORMAT_INVALID (&format_map[0])
+
 /* A helper for spice_gst_encoder_encode_frame() */
 static const SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
 {
-/* See GStreamer's part-mediatype-video-raw.txt and
- * section-types-definitions.html documents.
- */
-static const SpiceFormatForGStreamer format_map[] =  {
-{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
-{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0, 
0x7C00},
-/* TODO: Test the other formats */
-{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
-{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
-};
-
 int i;
 for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
 if (format_map[i].spice_format == format) {
-if (i > 1) {
+if (i > 2) {
 spice_warning("The %d format has not been tested yet", format);
 }
 return &format_map[i];
 }
 }
 
-return NULL;
+return GSTREAMER_FORMAT_INVALID;
 }
 
 static void set_appsrc_caps(SpiceGstEncoder *encoder)
@@ -1457,7 +1462,7 @@ static int spice_gst_encoder_encode_frame(VideoEncoder 
*video_encoder,
 encoder->width, width, encoder->height, height,
 encoder->spice_format, bitmap->format);
 encoder->format = map_format(bitmap->format);
-if (!encoder->format) {
+if (encoder->format == GSTREAMER_FORMAT_INVALID) {
 spice_warning("unable to map format type %d", bitmap->format);
 encoder->errors = 4;
 return VIDEO_ENCODER_FRAME_UNSUPPORTED;
@@ -1663,7 +1668,7 @@ static void spice_gst_encoder_get_stats(VideoEncoder 
*video_encoder,
 VideoEncoderStats *stats)
 {
 SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
-uint64_t raw_bit_rate = encoder->width * encoder->height * 
(encoder->format ? encoder->format->bpp : 0) * get_source_fps(encoder);
+uint64_t raw_bit_rate = encoder->width * encoder->height * 
encoder->format->bpp * get_source_fps(encoder);
 
 spice_return_if_fail(stats != NULL);
 stats->starting_bit_rate = encoder->starting_bit_rate;
@@ -1712,6 +1717,7 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType 
codec_type,
 encoder->starting_bit_rate = starting_bit_rate;
 encoder->bitmap_ref = bitmap_ref;
 encoder->bitmap_unref = bitmap_unref;
+encoder->format = GSTREAMER_FORMAT_INVALID;
 g_mutex_init(&encoder->outbuf_mutex);
 g_cond_init(&encoder->outbuf_cond);
 
-- 
2.7.4

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

[Spice-devel] [PATCH 1/2] Avoids to initialise OpenSSL threading twice

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index 6f88649..f74c8d3 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2792,6 +2792,13 @@ static void openssl_thread_setup(void)
 {
 int i;
 
+/* Somebody else already setup threading for OpenSSL,
+ * don't do it twice to avoid possible races.
+ */
+if (CRYPTO_get_locking_callback() != NULL) {
+return;
+}
+
 lock_cs = OPENSSL_malloc(CRYPTO_num_locks() * sizeof(pthread_mutex_t));
 
 for (i = 0; i < CRYPTO_num_locks(); i++) {
-- 
2.7.4

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

[Spice-devel] [PATCH 2/2] OpenSSL from 1.1.0 is thread safe by default

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index f74c8d3..c3780e0 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2771,6 +2771,7 @@ static int ssl_password_cb(char *buf, int size, int 
flags, void *userdata)
 return (strlen(pass));
 }
 
+#if OPENSSL_VERSION_NUMBER < 0x101FL
 static unsigned long pthreads_thread_id(void)
 {
 unsigned long ret;
@@ -2808,6 +2809,11 @@ static void openssl_thread_setup(void)
 CRYPTO_set_id_callback(pthreads_thread_id);
 CRYPTO_set_locking_callback(pthreads_locking_callback);
 }
+#else
+static inline void openssl_thread_setup(void)
+{
+}
+#endif
 
 static gpointer openssl_global_init(gpointer arg)
 {
-- 
2.7.4

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

Re: [Spice-devel] [PATCH 2/7] gstreamer: Reduce #ifdef

[Pavel Grunt]

The version with #ifdef is more clear/readable for me

Pavel

On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/gstreamer-encoder.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index c8d7d88..cdd9696 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -1240,6 +1240,14 @@ static void clear_zero_copy_queue(SpiceGstEncoder
> *encoder, gboolean unref_queue
>  {
>  /* Nothing to do */
>  }
> +
> +static inline int zero_copy(SpiceGstEncoder *encoder,
> +const SpiceBitmap *bitmap, gpointer
> bitmap_opaque,
> +GstBuffer *buffer, uint32_t *chunk_index,
> +uint32_t *chunk_offset, uint32_t *len)
> +{
> +return TRUE;
> +}
>  #endif
>  
>  /* A helper for push_raw_frame() */
> @@ -1353,19 +1361,17 @@ static int push_raw_frame(SpiceGstEncoder *encoder,
>  } else {
>  /* We can copy the bitmap chunk by chunk */
>  uint32_t chunk_index = 0;
> -#ifdef DO_ZERO_COPY
>  if (!zero_copy(encoder, bitmap, bitmap_opaque, buffer, &chunk_index,
> &chunk_offset, &len)) {
>  gst_buffer_unref(buffer);
>  return VIDEO_ENCODER_FRAME_UNSUPPORTED;
>  }
> +
>  /* len now contains the remaining number of bytes to copy.
>   * However we must avoid any write to the GstBuffer object as it
>   * would cause a copy of the read-only memory objects we just added.
>   * Fortunately we can append extra writable memory objects instead.
>   */
> -#endif
> -
>  if (len) {
>  uint8_t *dst = allocate_and_map_memory(len, &map, buffer);
>  if (!dst) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win v1 1/2] file-transfer: fix typos

[Frediano Ziglio]

> 
> ---
>  vdagent/file_xfer.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> index 58d3a31..cbf7e5e 100644
> --- a/vdagent/file_xfer.cpp
> +++ b/vdagent/file_xfer.cpp
> @@ -86,7 +86,7 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage*
> start,
>  
>  wlen = _tcslen(file_path);
>  // make sure we have enough space
> -// (1 char for separator, 1 char for filename and 1 char for NUL
> terminator)
> +// (1 char for separator, 1 char for filename and 1 char for NULL
> terminator)

Ehm.. no, a string is NUL terminated, is not an array of pointers
but an array of characters and NUL is the character with 0 encoding :)

Actually I had a doubt about EBCDIC, from https://it.wikipedia.org/wiki/EBCDIC
looks like NUL is 0 even on EBCDIC.

>  if (wlen + 3 >= MAX_PATH) {
>  vd_printf("error: file too long %ls\\%s", file_path, file_name);
>  return;
> @@ -159,7 +159,7 @@ void
> FileXfer::handle_status(VDAgentFileXferStatusMessage* status)
>  
>  vd_printf("id %u result %u", status->id, status->result);
>  if (status->result != VD_AGENT_FILE_XFER_STATUS_CANCELLED) {
> -vd_printf("only cancel is premitted");
> +vd_printf("only cancel is permitted");
>  return;
>  }
>  iter = _tasks.find(status->id);

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

Re: [Spice-devel] [PATCH 7/7] Avoids to initialise OpenSSL threading twice

[Pavel Grunt]

On Thu, 2016-08-11 at 08:42 -0400, Frediano Ziglio wrote:
> > 
> > 
> > Hi Frediano,
> > 
> > did you notice any issues ?
> > 
> > CRYPTO_get_locking_callback is kinda deprecated/not needed/noop in recent
> > openssl:
> > https://github.com/openssl/openssl/commit/2e52e7df518d80188c865ea3f7bb3526d1
> > 4b0c
> > 08
> > 
> 
> Wonderful, finally some sane improvements about threading and library!
> This is really a good news!
> Hope they also fix the leaks one day of another.
> Didn't notice any issues, just preventing problems if another library
> linked to the same program decide the use the same functions.
> I think the patch is safe in any case.
yes, of course it is safe.
> As a future work we should detect OpenSSL version and avoid to
> initialize threads as useless.
I think it is worth adding a comment that is going to be fixed in openssl1.1

Pavel

> 
> Frediano
> 
> > 
> > Pavel
> > 
> > On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/reds.c | 7 +++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 6f88649..f74c8d3 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -2792,6 +2792,13 @@ static void openssl_thread_setup(void)
> > >  {
> > >  int i;
> > >  
> > > +/* Somebody else already setup threading for OpenSSL,
> > > + * don't do it twice to avoid possible races.
> > > + */
> > > +if (CRYPTO_get_locking_callback() != NULL) {
> > > +return;
> > > +}
> > > +
> > >  lock_cs = OPENSSL_malloc(CRYPTO_num_locks() *
> > >  sizeof(pthread_mutex_t));
> > >  
> > >  for (i = 0; i < CRYPTO_num_locks(); i++) {
> > 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [vdagent-win v1 2/2] vdagent: remove whitespaces

[Victor Toso]

---
 vdagent/display_setting.cpp | 10 +-
 vdagent/display_setting.h   |  2 +-
 vdagent/file_xfer.cpp   |  4 ++--
 vdagent/vdagent.cpp |  6 +++---
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/vdagent/display_setting.cpp b/vdagent/display_setting.cpp
index c261b67..a2ada04 100644
--- a/vdagent/display_setting.cpp
+++ b/vdagent/display_setting.cpp
@@ -149,7 +149,7 @@ DWORD DisplaySetting::get_user_process_id()
 vd_printf("ProcessIdToSessionId for explorer failed %lu", 
GetLastError());
 break;
 }
-
+
 if (explorer_session_id == agent_session_id) {
 explorer_pid = proc_entry.th32ProcessID;
 break;
@@ -397,7 +397,7 @@ bool DisplaySetting::disable_animation()
 win_animation.cbSize = sizeof(ANIMATIONINFO);
 win_animation.iMinAnimate = 0;
 
-if (SystemParametersInfoA(SPI_SETANIMATION, sizeof(ANIMATIONINFO), 
+if (SystemParametersInfoA(SPI_SETANIMATION, sizeof(ANIMATIONINFO),
   &win_animation, 0)) {
 vd_printf("disable window animation: success");
 } else {
@@ -455,7 +455,7 @@ bool DisplaySetting::reload_win_animation(HKEY 
desktop_reg_key)
 active_win_animation.cbSize = sizeof(ANIMATIONINFO);
 active_win_animation.iMinAnimate = 1;
 
-if (SystemParametersInfoA(SPI_SETANIMATION, sizeof(ANIMATIONINFO), 
+if (SystemParametersInfoA(SPI_SETANIMATION, sizeof(ANIMATIONINFO),
   &active_win_animation, 0)) {
 vd_printf("reload window animation: success");
 return false;
@@ -488,12 +488,12 @@ bool DisplaySetting::reload_ui_effects(HKEY 
desktop_reg_key)
 vd_printf("RegQueryValueEx(UserPreferencesMask) : fail %ld", status);
 return false;
 }
-
+
 if (value_type != REG_BINARY) {
 vd_printf("bad UserPreferencesMask value type %lu (expected 
REG_BINARY)", value_type);
 return false;
 }
-
+
 vd_printf("UserPreferencesMask = %lx %lx", ui_mask[0], ui_mask[1]);
 
 ret &= set_bool_system_parameter_info(SPI_SETUIEFFECTS, ui_mask[0] & 
0x8000);
diff --git a/vdagent/display_setting.h b/vdagent/display_setting.h
index 8c8cdb1..ccb2e84 100644
--- a/vdagent/display_setting.h
+++ b/vdagent/display_setting.h
@@ -22,7 +22,7 @@
 
 class DisplaySettingOptions {
 public:
-DisplaySettingOptions() 
+DisplaySettingOptions()
 : _disable_wallpaper (false)
 , _disable_font_smoothing (false)
 , _disable_animation (false) {}
diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
index cbf7e5e..2f92a18 100644
--- a/vdagent/file_xfer.cpp
+++ b/vdagent/file_xfer.cpp
@@ -127,7 +127,7 @@ bool FileXfer::handle_data(VDAgentFileXferDataMessage* data,
 if (task->pos > task->size) {
 vd_printf("file xfer is longer than expected");
 goto fin;
-}  
+}
 if (!WriteFile(task->handle, data->data, (DWORD)data->size,
&written, NULL) || written != data->size) {
 vd_printf("file write failed %lu", GetLastError());
@@ -157,7 +157,7 @@ void FileXfer::handle_status(VDAgentFileXferStatusMessage* 
status)
 FileXferTasks::iterator iter;
 FileXferTask* task;
 
-vd_printf("id %u result %u", status->id, status->result); 
+vd_printf("id %u result %u", status->id, status->result);
 if (status->result != VD_AGENT_FILE_XFER_STATUS_CANCELLED) {
 vd_printf("only cancel is permitted");
 return;
diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index 959881d..c67f30c 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -129,7 +129,7 @@ private:
 static VDAgent* _singleton;
 HWND _hwnd;
 HWND _hwnd_next_viewer;
-HMODULE _user_lib; 
+HMODULE _user_lib;
 PCLIPBOARD_OP _add_clipboard_listener;
 PCLIPBOARD_OP _remove_clipboard_listener;
 int _system_version;
@@ -983,7 +983,7 @@ void VDAgent::on_clipboard_grab()
 while ((format = EnumClipboardFormats(format))) {
 vd_printf("Unsupported clipboard format %u", format);
 }
-}  
+}
 }
 
 // In delayed rendering, Windows requires us to SetClipboardData before we 
return from
@@ -1268,7 +1268,7 @@ void VDAgent::dispatch_message(VDAgentMessage* msg, 
uint32_t port)
 handle_clipboard((VDAgentClipboard*)msg->data, msg->size - 
sizeof(VDAgentClipboard));
 break;
 case VD_AGENT_CLIPBOARD_GRAB:
-handle_clipboard_grab((VDAgentClipboardGrab*)msg->data, msg->size);

+handle_clipboard_grab((VDAgentClipboardGrab*)msg->data, msg->size);
 break;
 case VD_AGENT_CLIPBOARD_REQUEST:
 res = handle_clipboard_request((VDAgentClipboardRequest*)msg->data);
-- 
2.7.4

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

[Spice-devel] [vdagent-win v1 1/2] file-transfer: fix typos

[Victor Toso]

---
 vdagent/file_xfer.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
index 58d3a31..cbf7e5e 100644
--- a/vdagent/file_xfer.cpp
+++ b/vdagent/file_xfer.cpp
@@ -86,7 +86,7 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage* 
start,
 
 wlen = _tcslen(file_path);
 // make sure we have enough space
-// (1 char for separator, 1 char for filename and 1 char for NUL 
terminator)
+// (1 char for separator, 1 char for filename and 1 char for NULL 
terminator)
 if (wlen + 3 >= MAX_PATH) {
 vd_printf("error: file too long %ls\\%s", file_path, file_name);
 return;
@@ -159,7 +159,7 @@ void FileXfer::handle_status(VDAgentFileXferStatusMessage* 
status)
 
 vd_printf("id %u result %u", status->id, status->result); 
 if (status->result != VD_AGENT_FILE_XFER_STATUS_CANCELLED) {
-vd_printf("only cancel is premitted");
+vd_printf("only cancel is permitted");
 return;
 }
 iter = _tasks.find(status->id);
-- 
2.7.4

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

Re: [Spice-devel] [PATCH 7/7] Avoids to initialise OpenSSL threading twice

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> did you notice any issues ?
> 
> CRYPTO_get_locking_callback is kinda deprecated/not needed/noop in recent
> openssl:
> https://github.com/openssl/openssl/commit/2e52e7df518d80188c865ea3f7bb3526d14b0c
> 08
> 

Wonderful, finally some sane improvements about threading and library!
This is really a good news!
Hope they also fix the leaks one day of another.
Didn't notice any issues, just preventing problems if another library
linked to the same program decide the use the same functions.
I think the patch is safe in any case.
As a future work we should detect OpenSSL version and avoid to
initialize threads as useless.

Frediano

> Pavel
> 
> On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/reds.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 6f88649..f74c8d3 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2792,6 +2792,13 @@ static void openssl_thread_setup(void)
> >  {
> >  int i;
> >  
> > +/* Somebody else already setup threading for OpenSSL,
> > + * don't do it twice to avoid possible races.
> > + */
> > +if (CRYPTO_get_locking_callback() != NULL) {
> > +return;
> > +}
> > +
> >  lock_cs = OPENSSL_malloc(CRYPTO_num_locks() *
> >  sizeof(pthread_mutex_t));
> >  
> >  for (i = 0; i < CRYPTO_num_locks(); i++) {
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [vdagent-win v2 1/2] vdagent: rework on event_dispatcher

[Victor Toso]

As _stop_event is not mandatory, we are initializing the events array
with something that might be NULL. The problem is with the following
patch as I'm introducing a new non mandatory event and logic starts to
get a bit hard to follow.

So this patch makes explicit if we are setting or receiving a
_stop_event or not and uses an auxiliary enum and array 'actions' to
work around this non mandatory events and the return value of
MsgWaitForMultipleObjectsEx()
---
 vdagent/vdagent.cpp | 39 ++-
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index 959881d..d3cbd5f 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -478,12 +478,27 @@ void VDAgent::input_desktop_message_loop()
 CloseDesktop(hdesk);
 }
 
+#define G_N_ELEMENTS(arr)  (sizeof (arr) / sizeof ((arr)[0]))
+
 void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
 {
-HANDLE events[] = {_control_event, _stop_event};
-DWORD event_count = _stop_event ? 2 : 1;
+HANDLE events[2];
+DWORD event_count = 1;
 DWORD wait_ret;
+DWORD action;
 MSG msg;
+enum {
+CONTROL_ACTION,
+STOP_ACTION,
+} actions[G_N_ELEMENTS(events)];
+
+events[0] = _control_event;
+actions[0] = CONTROL_ACTION;
+if (_stop_event) {
+events[event_count] = _stop_event;
+actions[event_count] = STOP_ACTION;
+event_count++;
+}
 
 wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout, 
wake_mask, MWMO_ALERTABLE);
 if (wait_ret == WAIT_OBJECT_0 + event_count) {
@@ -492,19 +507,25 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD 
wake_mask)
 DispatchMessage(&msg);
 }
 return;
+} else if (wait_ret == WAIT_IO_COMPLETION || wait_ret == WAIT_TIMEOUT) {
+return;
+} else if (wait_ret < WAIT_OBJECT_0 || wait_ret > WAIT_OBJECT_0 + 
event_count) {
+vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret, 
GetLastError());
+_running = false;
+return;
 }
-switch (wait_ret) {
-case WAIT_OBJECT_0:
+
+action = actions[wait_ret - WAIT_OBJECT_0];
+switch (action) {
+case CONTROL_ACTION:
 handle_control_event();
 break;
-case WAIT_OBJECT_0 + 1:
+case STOP_ACTION:
+vd_printf("%s: received stop event", __func__);
 _running = false;
 break;
-case WAIT_IO_COMPLETION:
-case WAIT_TIMEOUT:
-break;
 default:
-vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret, 
GetLastError());
+vd_printf("%s: action not handled (%lu)", __func__, action);
 _running = false;
 }
 }
-- 
2.7.4

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

[Spice-devel] [vdagent-win v2 2/2] vdagent-win: start vdagent with lock info from session

[Victor Toso]

Commit 5907b6cbb5c724f9729da59a644271b4258d122e started to handle
Lock/Unlock events from Session at VDAgent. Although that works just
fine, it does not cover all the situations as pointed by Andrei at [0]
and I quote:

> It fails for next test-case:
>
> * Connect with RV to VM
> * Lock VM (ctrl-alt-del)
> * Close RV
> * Connect with RV to the VM again
> * Do not unlock session. DnD files from client to locked VM.
>
> Result : Files are copied to VM.

[0] https://bugzilla.redhat.com/show_bug.cgi?id=1323628#c6

To solve this situation, we need VDService to inform VDAgent if
Session is Locked or not upon its initialization.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1323628
Signed-off-by: Victor Toso 
Reported-by: Andrei Stepanov 
---
 common/vdcommon.h   |  1 +
 vdagent/vdagent.cpp | 16 +++-
 vdservice/vdservice.cpp | 15 +++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/common/vdcommon.h b/common/vdcommon.h
index f4859e2..8539c38 100644
--- a/common/vdcommon.h
+++ b/common/vdcommon.h
@@ -35,6 +35,7 @@ typedef CRITICAL_SECTION mutex_t;
 
 #define VD_AGENT_REGISTRY_KEY "SOFTWARE\\Red Hat\\Spice\\vdagent\\"
 #define VD_AGENT_STOP_EVENT   TEXT("Global\\vdagent_stop_event")
+#define VD_AGENT_SESSION_LOCKED_EVENT 
TEXT("Global\\vdagent_session_locked_event")
 
 #if defined __GNUC__
 #define ALIGN_GCC __attribute__ ((packed))
diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index d3cbd5f..06d6574 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -142,6 +142,7 @@ private:
 DWORD _input_time;
 HANDLE _control_event;
 HANDLE _stop_event;
+HANDLE _session_locked_event;
 VDAgentMessage* _in_msg;
 uint32_t _in_msg_pos;
 bool _pending_input;
@@ -202,6 +203,7 @@ VDAgent::VDAgent()
 , _input_time (0)
 , _control_event (NULL)
 , _stop_event (NULL)
+, _session_locked_event (NULL)
 , _in_msg (NULL)
 , _in_msg_pos (0)
 , _pending_input (false)
@@ -308,6 +310,7 @@ bool VDAgent::run()
 return false;
 }
 _stop_event = OpenEvent(SYNCHRONIZE, FALSE, VD_AGENT_STOP_EVENT);
+_session_locked_event = OpenEvent(SYNCHRONIZE, FALSE, 
VD_AGENT_SESSION_LOCKED_EVENT);
 memset(&wcls, 0, sizeof(wcls));
 wcls.lpfnWndProc = &VDAgent::wnd_proc;
 wcls.lpszClassName = VD_AGENT_WINCLASS_NAME;
@@ -482,7 +485,7 @@ void VDAgent::input_desktop_message_loop()
 
 void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
 {
-HANDLE events[2];
+HANDLE events[3];
 DWORD event_count = 1;
 DWORD wait_ret;
 DWORD action;
@@ -490,6 +493,7 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD 
wake_mask)
 enum {
 CONTROL_ACTION,
 STOP_ACTION,
+SESSION_LOCKED_ACTION,
 } actions[G_N_ELEMENTS(events)];
 
 events[0] = _control_event;
@@ -500,6 +504,12 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD 
wake_mask)
 event_count++;
 }
 
+if (_session_locked_event) {
+events[event_count] = _session_locked_event;
+actions[event_count] = SESSION_LOCKED_ACTION;
+event_count++;
+}
+
 wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout, 
wake_mask, MWMO_ALERTABLE);
 if (wait_ret == WAIT_OBJECT_0 + event_count) {
 while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
@@ -524,6 +534,10 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD 
wake_mask)
 vd_printf("%s: received stop event", __func__);
 _running = false;
 break;
+case SESSION_LOCKED_ACTION:
+vd_printf("%s: received session locked event", __func__);
+_session_is_locked = true;
+break;
 default:
 vd_printf("%s: action not handled (%lu)", __func__, action);
 _running = false;
diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
index 89e0dbb..1f53d97 100644
--- a/vdservice/vdservice.cpp
+++ b/vdservice/vdservice.cpp
@@ -93,6 +93,7 @@ private:
 PROCESS_INFORMATION _agent_proc_info;
 HANDLE _control_event;
 HANDLE _agent_stop_event;
+HANDLE _session_locked_event;
 HANDLE* _events;
 TCHAR _agent_path[MAX_PATH];
 VDControlQueue _control_queue;
@@ -105,6 +106,7 @@ private:
 int _system_version;
 bool _agent_alive;
 bool _running;
+bool _session_is_locked;
 VDLog* _log;
 unsigned _events_count;
 };
@@ -128,6 +130,7 @@ VDService::VDService()
 , _agent_restarts (0)
 , _agent_alive (false)
 , _running (false)
+, _session_is_locked (false)
 , _log (NULL)
 , _events_count(0)
 {
@@ -135,6 +138,7 @@ VDService::VDService()
 _system_version = supported_system_version();
 _control_event = CreateEvent(NULL, FALSE, FALSE, NULL);
 _agent_stop_event = CreateEvent(NULL, FALSE, FALSE, VD_AGENT_STOP_EVENT);
+_session_locked_event = CreateEvent(NULL, FALSE, FALSE, 
VD_AGENT_SESSION_LOCKED_EVENT);
 _agent_path[0] = wchar_t('\0');
 MUTEX_INIT(_agent_mutex);
 M

Re: [Spice-devel] [PATCH 7/7] Avoids to initialise OpenSSL threading twice

[Pavel Grunt]

Hi Frediano,

did you notice any issues ?

CRYPTO_get_locking_callback is kinda deprecated/not needed/noop in recent
openssl:
https://github.com/openssl/openssl/commit/2e52e7df518d80188c865ea3f7bb3526d14b0c
08

Pavel

On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 6f88649..f74c8d3 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2792,6 +2792,13 @@ static void openssl_thread_setup(void)
>  {
>  int i;
>  
> +/* Somebody else already setup threading for OpenSSL,
> + * don't do it twice to avoid possible races.
> + */
> +if (CRYPTO_get_locking_callback() != NULL) {
> +return;
> +}
> +
>  lock_cs = OPENSSL_malloc(CRYPTO_num_locks() * sizeof(pthread_mutex_t));
>  
>  for (i = 0; i < CRYPTO_num_locks(); i++) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 5/7] gstreamer: Peephole optimisation for SpiceFormatForGStreamer

[Pavel Grunt]

On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> Reduce structure length using static allocated string inside the
> structure.
> This will also avoid using .data.rel.ro section and relocations
> reducing even more library size.
ok
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/gstreamer-encoder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 78d4f74..1221612 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -40,7 +40,7 @@
>  
>  typedef struct {
>  SpiceBitmapFmt spice_format;
> -const char *format;
> +char format[8];
>  uint32_t bpp;
>  uint32_t depth;
>  uint32_t endianness;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 4/7] gstreamer: Use a dummy format to make sure format is always defined.

[Pavel Grunt]

On Thu, 2016-08-11 at 13:38 +0200, Pavel Grunt wrote:
> Hi Frediano,
> 
> ack and one suggestion below
> 
> On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> > 
> > This avoid a check for NULL.

There is one more check in spice_gst_encoder_encode_frame()

> > Also will be used to catch invalid values when table will be extended.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/gstreamer-encoder.c | 34 --
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index 7376d2b..78d4f74 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -747,31 +747,36 @@ static inline void
> > server_increase_bit_rate(SpiceGstEncoder *encoder,
> >  
> >  /* -- GStreamer pipeline -- */
> >  
> > +/* See GStreamer's part-mediatype-video-raw.txt and
> > + * section-types-definitions.html documents.
> > + */
> > +static const SpiceFormatForGStreamer format_map[] =  {
> > +/* First item is invalid.
> > + * It's located first so the loop catch invalid values.
> > + */
> > +{SPICE_BITMAP_FMT_INVALID, "", 0, 0, 0, 0, 0, 0},
> > +{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff,
> > 0xff00},
> > +{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0,
> > 0x7C00},
> > +/* TODO: Test the other formats */
> > +{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff,
> > 0xff00},
> > +{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
> > +};
> > +#define GSTREAMER_FORMAT_INVALID (&format_map[0])
> we can have
>    #define GSTREAMER_FORMAT_TESTED 2
> 
> > 
> > +
> >  /* A helper for spice_gst_encoder_encode_frame() */
> >  static const SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
> >  {
> > -/* See GStreamer's part-mediatype-video-raw.txt and
> > - * section-types-definitions.html documents.
> > - */
> > -static const SpiceFormatForGStreamer format_map[] =  {
> > -{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff,
> > 0xff00},
> > -{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0,
> > 0x7C00},
> > -/* TODO: Test the other formats */
> > -{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00,
> > 0xff,
> > 0xff00},
> > -{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00,
> > 0xff},
> > -};
> > -
> >  int i;
> >  for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
> >  if (format_map[i].spice_format == format) {
> > -if (i > 1) {
> > +if (i > 2) {
> and
>                if (i > GSTREAMER_FORMAT_TESTED)
> > 
> >  spice_warning("The %d format has not been tested yet",
> > format);
> >  }
> >  return &format_map[i];
> >  }
> >  }
> >  
> > -return NULL;
> > +return GSTREAMER_FORMAT_INVALID;
> >  }
> >  
> >  static void set_appsrc_caps(SpiceGstEncoder *encoder)
> > @@ -1669,7 +1674,7 @@ static void spice_gst_encoder_get_stats(VideoEncoder
> > *video_encoder,
> >  VideoEncoderStats *stats)
> >  {
> >  SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
> > -uint64_t raw_bit_rate = encoder->width * encoder->height * (encoder-
> > > 
> > > format ? encoder->format->bpp : 0) * get_source_fps(encoder);
> > +uint64_t raw_bit_rate = encoder->width * encoder->height * encoder-
> > > 
> > > format->bpp * get_source_fps(encoder);
> >  
> >  spice_return_if_fail(stats != NULL);
> >  stats->starting_bit_rate = encoder->starting_bit_rate;
> > @@ -1718,6 +1723,7 @@ VideoEncoder
> > *gstreamer_encoder_new(SpiceVideoCodecType
> > codec_type,
> >  encoder->starting_bit_rate = starting_bit_rate;
> >  encoder->bitmap_ref = bitmap_ref;
> >  encoder->bitmap_unref = bitmap_unref;
> > +encoder->format = GSTREAMER_FORMAT_INVALID;
> >  g_mutex_init(&encoder->outbuf_mutex);
> >  g_cond_init(&encoder->outbuf_cond);
> >  
> Pavel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH win-vdagent v6 2/3] Encapsulating XPDM implementation

[Sameeh Jubran]

On Thu, Aug 11, 2016 at 1:23 PM, Frediano Ziglio  wrote:

> >
> > The Direct3D 9 API operates on either the Windows XP display driver
> > model (XPDM) or the Windows Vista display driver model (WDDM), depending
> > on the operating system installed.
> >
> > This patch encapsulates the current XPDM interface implementation into
> > XPDMInterface class which inherits DisplayConfig class. This patch
> > makes it easier to introduce WDDM interface to Vdagent in future
> > patches.
> >
> > Based on a patch by Sandy Stutsman 
> >
> > Signed-off-by: Dmitry Fleytman 
> > Signed-off-by: Sameeh Jubran 
> > ---
> >  vdagent/desktop_layout.cpp| 148 +++---
> --
> >  vdagent/desktop_layout.h  |   9 +-
> >  vdagent/display_configuration.cpp | 174
> >  ++
> >  vdagent/display_configuration.h   |  36 
> >  4 files changed, 245 insertions(+), 122 deletions(-)
> >
> > diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> > index a7666ca..9c3e873 100644
> > --- a/vdagent/desktop_layout.cpp
> > +++ b/vdagent/desktop_layout.cpp
> > @@ -18,6 +18,7 @@
> >  #include 
> >  #include 
> >  #include "desktop_layout.h"
> > +#include "display_configuration.h"
> >  #include "vdlog.h"
> >
> >  #ifdef __MINGW32__
> > @@ -35,15 +36,17 @@ void DisplayMode::set_res(DWORD width, DWORD height,
> > DWORD depth)
> >  DesktopLayout::DesktopLayout()
> >  : _total_width (0)
> >  , _total_height (0)
> > -, _send_monitors_position(false)
> > +, _display_config (NULL)
> >  {
> >  MUTEX_INIT(_mutex);
> > +_display_config = DisplayConfig::create_config();
> >  get_displays();
> >  }
> >
> >  DesktopLayout::~DesktopLayout()
> >  {
> >  clean_displays();
> > +delete _display_config;
> >  }
> >
> >  void DesktopLayout::get_displays()
> > @@ -59,6 +62,7 @@ void DesktopLayout::get_displays()
> >  unlock();
> >  return;
> >  }
> > +_display_config->update_config_path();
> >  clean_displays();
> >  ZeroMemory(&dev_info, sizeof(dev_info));
> >  dev_info.cb = sizeof(dev_info);
> > @@ -82,12 +86,13 @@ void DesktopLayout::get_displays()
> >  _displays[i] = NULL;
> >  }
> >  }
> > -attached = !!(dev_info.StateFlags &
> > DISPLAY_DEVICE_ATTACHED_TO_DESKTOP);
> > +attached = _display_config->is_attached(&dev_info);
> > +
> >  EnumDisplaySettings(dev_info.DeviceName, ENUM_CURRENT_SETTINGS,
> >  &mode);
> >  _displays[display_id] = new DisplayMode(mode.dmPosition.x,
> >  mode.dmPosition.y,
> >  mode.dmPelsWidth,
> >  mode.dmPelsHeight,
> >  mode.dmBitsPerPel,
> >  attached);
> > -update_monitor_config(dev_info.DeviceName,
> _displays[display_id]);
> > +_display_config->update_monitor_config(dev_info.DeviceName,
> > _displays[display_id], &mode);
> >  }
> >  normalize_displays_pos();
> >  unlock();
> > @@ -121,6 +126,7 @@ void DesktopLayout::set_displays()
> >  unlock();
> >  return;
> >  }
> > +_display_config->update_config_path();
> >  ZeroMemory(&dev_info, sizeof(dev_info));
> >  dev_info.cb = sizeof(dev_info);
> >  ZeroMemory(&dev_mode, sizeof(dev_mode));
> > @@ -146,28 +152,32 @@ void DesktopLayout::set_displays()
> >  break;
> >  }
> >  DisplayMode * mode(_displays.at(display_id));
> > -if (!init_dev_mode(dev_info.DeviceName, &dev_mode, mode,
> normal_x,
> > normal_y, true)) {
> > +if (!init_dev_mode(dev_info.DeviceName, &dev_mode, mode)) {
> >  vd_printf("No suitable mode found for display %S",
> >  dev_info.DeviceName);
> >  break;
> >  }
> >  vd_printf("Set display mode %lux%lu", dev_mode.dmPelsWidth,
> >  dev_mode.dmPelsHeight);
> > -LONG ret = ChangeDisplaySettingsEx(dev_info.DeviceName,
> &dev_mode,
> > NULL,
> > -   CDS_UPDATEREGISTRY |
> CDS_NORESET,
> > NULL);
> > -if (ret == DISP_CHANGE_SUCCESSFUL) {
> > +if (_display_config->update_dev_mode_position(dev_info.DeviceNa
> me,
> > &dev_mode,
> > + mode->_pos_x -
> > normal_x,
> > + mode->_pos_y -
> > normal_y)) {
> >  dev_sets++;
> > -update_monitor_config(dev_info.DeviceName, mode);
> > +_display_config->update_monitor_config(dev_info.DeviceName,
> > mode, &dev_mode);
> >  }
> >  if (!is_qxl) {
> >  display_id++;
> >  }
> >  }
> >  if (dev_sets) {
> > -ChangeDisplaySettingsEx(NULL, NULL, NULL, 0, NULL);
> > +_display_config->update_display_se

Re: [Spice-devel] [PATCH 4/7] gstreamer: Use a dummy format to make sure format is always defined.

[Pavel Grunt]

Hi Frediano,

ack and one suggestion below

On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> This avoid a check for NULL.
> Also will be used to catch invalid values when table will be extended.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/gstreamer-encoder.c | 34 --
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 7376d2b..78d4f74 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -747,31 +747,36 @@ static inline void
> server_increase_bit_rate(SpiceGstEncoder *encoder,
>  
>  /* -- GStreamer pipeline -- */
>  
> +/* See GStreamer's part-mediatype-video-raw.txt and
> + * section-types-definitions.html documents.
> + */
> +static const SpiceFormatForGStreamer format_map[] =  {
> +/* First item is invalid.
> + * It's located first so the loop catch invalid values.
> + */
> +{SPICE_BITMAP_FMT_INVALID, "", 0, 0, 0, 0, 0, 0},
> +{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> +{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0, 0x7C00},
> +/* TODO: Test the other formats */
> +{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> +{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
> +};
> +#define GSTREAMER_FORMAT_INVALID (&format_map[0])
we can have
   #define GSTREAMER_FORMAT_TESTED 2

> +
>  /* A helper for spice_gst_encoder_encode_frame() */
>  static const SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
>  {
> -/* See GStreamer's part-mediatype-video-raw.txt and
> - * section-types-definitions.html documents.
> - */
> -static const SpiceFormatForGStreamer format_map[] =  {
> -{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> -{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0,
> 0x7C00},
> -/* TODO: Test the other formats */
> -{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff,
> 0xff00},
> -{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00,
> 0xff},
> -};
> -
>  int i;
>  for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
>  if (format_map[i].spice_format == format) {
> -if (i > 1) {
> +if (i > 2) {
and
               if (i > GSTREAMER_FORMAT_TESTED)
>  spice_warning("The %d format has not been tested yet",
> format);
>  }
>  return &format_map[i];
>  }
>  }
>  
> -return NULL;
> +return GSTREAMER_FORMAT_INVALID;
>  }
>  
>  static void set_appsrc_caps(SpiceGstEncoder *encoder)
> @@ -1669,7 +1674,7 @@ static void spice_gst_encoder_get_stats(VideoEncoder
> *video_encoder,
>  VideoEncoderStats *stats)
>  {
>  SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
> -uint64_t raw_bit_rate = encoder->width * encoder->height * (encoder-
> >format ? encoder->format->bpp : 0) * get_source_fps(encoder);
> +uint64_t raw_bit_rate = encoder->width * encoder->height * encoder-
> >format->bpp * get_source_fps(encoder);
>  
>  spice_return_if_fail(stats != NULL);
>  stats->starting_bit_rate = encoder->starting_bit_rate;
> @@ -1718,6 +1723,7 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType
> codec_type,
>  encoder->starting_bit_rate = starting_bit_rate;
>  encoder->bitmap_ref = bitmap_ref;
>  encoder->bitmap_unref = bitmap_unref;
> +encoder->format = GSTREAMER_FORMAT_INVALID;
>  g_mutex_init(&encoder->outbuf_mutex);
>  g_cond_init(&encoder->outbuf_cond);
>  
Pavel

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

Re: [Spice-devel] [PATCH 6/7] Make process_commands_generation variable type coherent

[Pavel Grunt]

Ack

On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.c | 6 +++---
>  server/display-channel.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 4f52b79..5e0a004 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -24,7 +24,7 @@
>  
>  static void drawable_draw(DisplayChannel *display, Drawable *drawable);
>  static Drawable *display_channel_drawable_try_new(DisplayChannel *display,
> -  int
> process_commands_generation);
> +  uint32_t
> process_commands_generation);
>  
>  uint32_t display_channel_generate_uid(DisplayChannel *display)
>  {
> @@ -1081,7 +1081,7 @@ static void display_channel_add_drawable(DisplayChannel
> *display, Drawable *draw
>  }
>  
>  void display_channel_process_draw(DisplayChannel *display, RedDrawable
> *red_drawable,
> -  int process_commands_generation)
> +  uint32_t process_commands_generation)
>  {
>  Drawable *drawable =
>  display_channel_get_drawable(display, red_drawable->effect,
> red_drawable,
> @@ -1264,7 +1264,7 @@ static void drawables_init(DisplayChannel *display)
>   * @return pointer to uninitialized Drawable or NULL on failure
>   */
>  static Drawable *display_channel_drawable_try_new(DisplayChannel *display,
> -  int
> process_commands_generation)
> +  uint32_t
> process_commands_generation)
>  {
>  Drawable *drawable;
>  
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 52a5783..7b71480 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -270,7 +270,7 @@
> void   display_channel_destroy_surfaces  (DisplayC
> ha
>  uint32_t   display_channel_generate_uid  (Display
> Channel *display);
>  void   display_channel_process_draw  (Display
> Channel *display,
>    RedDraw
> able *red_drawable,
> -  int
> process_commands_generation);
> +  uint32_
> t process_commands_generation);
>  void   display_channel_process_surface_cmd   (Display
> Channel *display,
>    RedSurf
> aceCmd *surface,
>    int
> loadvm);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/7] gstreamer: Fix infinite loop in get_period_bit_rate

[Pavel Grunt]

Ack

On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> This was discovered by chance by me and Uri trying some remote connection
> with slow network (8Mbit) and high latency
> 
>   $ ping 10.10.48.87 -c 3
>   3 packets transmitted, 3 received, 0% packet loss, time 2002ms
>   rtt min/avg/max/mdev = 281.069/316.758/374.413/41.153 ms
> 
> The encoder->history status was (edited for readability):
> 
> (gdb) p ((SpiceGstEncoder*)0x61c000264880)->history_first
> $6 = 29
> (gdb) p ((SpiceGstEncoder*)0x61c000264880)->history_last
> $14 = 28
> (gdb) p ((SpiceGstEncoder*)0x61c000264880)->history
> [0] {mm_time = 11298131, duration = 7391006, size = 5616},
> [1] {mm_time = 11298148, duration = 7373663, size = 5559},
> [2] {mm_time = 11298166, duration = 7052209, size = 5511},
> [3] {mm_time = 11298183, duration = 7006828, size = 5722},
> [4] {mm_time = 11298199, duration = 7433311, size = 5756},
> [5] {mm_time = 11298216, duration = 7134734, size = 5545},
> [6] {mm_time = 11298232, duration = 7436589, size = 5521},
> [7] {mm_time = 11298249, duration = 7152181, size = 5540},
> [8] {mm_time = 11298266, duration = 7181308, size = 7796},
> [9] {mm_time = 11298283, duration = 5053084, size = 50824},
> [10] {mm_time = 11298298, duration = 7472305, size = 7427},
> [11] {mm_time = 11298315, duration = 7385017, size = 6682},
> [12] {mm_time = 11298333, duration = 7287125, size = 6377},
> [13] {mm_time = 11298349, duration = 7191461, size = 6159},
> [14] {mm_time = 11298367, duration = 7104546, size = 6035},
> [15] {mm_time = 11298382, duration = 7266942, size = 5896},
> [16] {mm_time = 11298400, duration = 7108001, size = 5812},
> [17] {mm_time = 11298418, duration = 7020583, size = 5807},
> [18] {mm_time = 11298433, duration = 7066056, size = 5758},
> [19] {mm_time = 11298450, duration = 7052900, size = 5676},
> [20] {mm_time = 11298467, duration = 7248233, size = 5765},
> [21] {mm_time = 11298483, duration = 7077348, size = 5712},
> [22] {mm_time = 11298502, duration = 7495368, size = 5835},
> [23] {mm_time = 11298517, duration = 7068626, size = 5805},
> [24] {mm_time = 11298534, duration = 7060375, size = 5801},
> [25] {mm_time = 11298551, duration = 7020383, size = 5868},
> [26] {mm_time = 11298568, duration = 7248400, size = 5830},
> [27] {mm_time = 11298584, duration = 7001304, size = 6621},
> [28] {mm_time = 11298600, duration = 7311600, size = 6113},
> [29] {mm_time = 11297612, duration = 6999174, size = 5666},  <--- to
> [30] {mm_time = 11297628, duration = 7506688, size = 5502},
> [31] {mm_time = 11297646, duration = 7209494, size = 5687},
> [32] {mm_time = 11297663, duration = 7396429, size = 5724},
> [33] {mm_time = 11297679, duration = 7172624, size = 5839},
> [34] {mm_time = 11297696, duration = 7300811, size = 5645},
> [35] {mm_time = 11297713, duration = 7108985, size = 5553},
> [36] {mm_time = 11297729, duration = 7171701, size = 5774},
> [37] {mm_time = 11297745, duration = 7174018, size = 5637},
> [38] {mm_time = 11297762, duration = 7313549, size = 5655},
> [39] {mm_time = 11297780, duration = 5183985, size = 51014},
> [40] {mm_time = 11297796, duration = 7038329, size = 7374},
> [41] {mm_time = 11297813, duration = 7211506, size = 6585},
> [42] {mm_time = 11297830, duration = 7112690, size = 5729},
> [43] {mm_time = 11297846, duration = 7103074, size = 5761},
> [44] {mm_time = 11297864, duration = 7599826, size = 5661},
> [45] {mm_time = 11297879, duration = 7355392, size = 5351},
> [46] {mm_time = 11297897, duration = 7454367, size = 5488},
> [47] {mm_time = 11297913, duration = 7127145, size = 5573},
> [48] {mm_time = 11297932, duration = 7550098, size = 5447},
> [49] {mm_time = 11297948, duration = 7506884, size = 5809},
> [50] {mm_time = 11297966, duration = 7405712, size = 5783},
> [51] {mm_time = 11297982, duration = 7182025, size = 5599},
> [52] {mm_time = 11298001, duration = 7323887, size = 5817},
> [53] {mm_time = 11298014, duration = 7342091, size = 5575},
> [54] {mm_time = 11298031, duration = 7596319, size = 5739},
> [55] {mm_time = 11298052, duration = 7428169, size = 5669},
> [56] {mm_time = 11298065, duration = 7457282, size = 5732},
> [57] {mm_time = 11298081, duration = 7174029, size = 5541},
> [58] {mm_time = 11298097, duration = 7340512, size = 5680},
> [59] {mm_time = 11298115, duration = 7427439, size = 5701}}
> (gdb) p from
> $16 = 11297544
> (gdb) p to
> $17 = 11297612
> 
> You can see that encoder->history[encoder->history_first].mm_time == to
> cause the check index == encoder->history_first to not be executed.
> 
> This code change was suggested by Uri.
> 
> Reported-by: Uri Lublin 
> Signed-off-by: Frediano Ziglio 
> Tested-by: Uri Lublin 
> ---
>  server/gstreamer-encoder.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index e6790af..c8d7d88 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -434,19 +434,20 @@ static uint64_t get_perio

[Spice-devel] [spice 2/2] streaming: Stop streaming if the client reports a streaming error

[Francois Gouget]

Signed-off-by: Francois Gouget 
---

If there's enough bandwidth it's as if nothing wrong ever happened :-)

 server/dcc.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/server/dcc.c b/server/dcc.c
index d387e8b..b4066f5 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -937,6 +937,15 @@ static int dcc_handle_stream_report(DisplayChannelClient 
*dcc,
 return TRUE;
 }
 
+if (report->num_frames == 0 && report->num_drops == UINT_MAX) {
+spice_warning("stream_report: the client does not support stream %u",
+  report->stream_id);
+/* Stop streaming the video so the client can see it */
+agent->video_encoder->destroy(agent->video_encoder);
+agent->video_encoder = NULL;
+return TRUE;
+}
+
 agent->video_encoder->client_stream_report(agent->video_encoder,
report->num_frames,
report->num_drops,
-- 
2.8.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [client 1/2] streaming: Send a special stream report to signal streaming errors

[Francois Gouget]

Servers that recognize this special report then stop streaming (sending 
regular screen updates instead) while older ones essentially ignore it.

Signed-off-by: Francois Gouget 
---

This patchset is based on Victor Toso's idea [1] of using the stream 
reports to tell the server that, despite expectations, the client cannot 
handle a given stream.

You'll notice that this patch does not directly check for 
create_xxx_decoder() errors. Instead it relies on the previous patchset 
[2] deleting broken streams so that the following messages will run into 
an unknown stream.

Of course this could already happen in case of a malicious server 
sending garbage to the client. So this patchset is quite independent 
from the previous one.

I don't know what the consequences of receiving an unknown message would 
be for the server so I chose to hook into the 
display_handle_stream_activate_report() as that's where we get notified 
that the server supports the stream reports.

Maybe we should send such an error anywhere we receive a message with an 
unknown stream_id. There's really no reason for that to happen in those 
other places though, except if the server does not recognize the initial 
error stream report and continues streaming. In that case sending more 
error messages won't do any good. So sending an error in just this one 
place may make more sense.

We could also add an extra cap to identify servers that recognize this 
special type or stream report. But is it really worth it?

[1] https://lists.freedesktop.org/archives/spice-devel/2016-August/031101.html
[2] https://lists.freedesktop.org/archives/spice-devel/2016-August/031282.html


 src/channel-display.c | 24 +---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/src/channel-display.c b/src/channel-display.c
index 709b3d2..3898f0a 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1530,10 +1530,28 @@ static void 
display_handle_stream_activate_report(SpiceChannel *channel, SpiceMs
 
 g_return_if_fail(c != NULL);
 g_return_if_fail(c->streams != NULL);
-g_return_if_fail(c->nstreams > op->stream_id);
 
-st = c->streams[op->stream_id];
-g_return_if_fail(st != NULL);
+st = op->stream_id < c->nstreams ? c->streams[op->stream_id] : NULL;
+if (st == NULL) {
+SpiceMsgcDisplayStreamReport report;
+SpiceMsgOut *msg;
+
+/* Send a special stream report to indicate there is no such stream */
+spice_printerr("notify the server that stream %u does not exist", 
op->stream_id);
+report.stream_id = op->stream_id;
+report.unique_id = op->unique_id;
+report.start_frame_mm_time = 0;
+report.end_frame_mm_time = 0;
+report.num_frames = 0;
+report.num_drops = UINT_MAX;
+report.last_frame_delay = 0;
+report.audio_delay = 0;
+
+msg = spice_msg_out_new(SPICE_CHANNEL(channel), 
SPICE_MSGC_DISPLAY_STREAM_REPORT);
+msg->marshallers->msgc_display_stream_report(msg->marshaller, &report);
+spice_msg_out_send(msg);
+return;
+}
 
 st->report_is_active = TRUE;
 st->report_id = op->unique_id;
-- 
2.8.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client v3 1/4] streaming: Check the stream id in display_update_stream_report() too

[Frediano Ziglio]

> 
> It's safer and more consistent than assuming the caller has done the
> check already.
> 
> Signed-off-by: Francois Gouget 
Acked-by: Frediano Ziglio 
> ---
>  src/channel-display.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index cf9c583..b4c9ec0 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1222,9 +1222,16 @@ void stream_display_frame(display_stream *st,
> SpiceMsgIn *frame_msg,
>  static void display_update_stream_report(SpiceDisplayChannel *channel,
>  uint32_t stream_id,
>   uint32_t frame_time, int32_t
>   latency)
>  {
> -display_stream *st = channel->priv->streams[stream_id];
> +SpiceDisplayChannelPrivate *c = channel->priv;
> +display_stream *st;
>  guint64 now;
>  
> +g_return_if_fail(c != NULL);
> +g_return_if_fail(c->streams != NULL);
> +g_return_if_fail(c->nstreams > stream_id);
> +
> +st = channel->priv->streams[stream_id];
> +
>  if (!st->report_is_active) {
>  return;
>  }

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

[Spice-devel] [client v3 4/4] streaming: Create the pipeline at the same time as the GStreamer decoder

[Francois Gouget]

This lets create_gstreamer_decoder() fail if it cannot create the
pipeline it needs, allowing the caller to try fallbacks.
This also means the pipeline has the same lifetime as the decoder which
makes it possible to remove a check in queue_frame().

Signed-off-by: Francois Gouget 
---
 src/channel-display-gst.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index c752639..647afc1 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -407,11 +407,6 @@ static void spice_gst_decoder_queue_frame(VideoDecoder 
*video_decoder,
 return;
 }
 
-if (!decoder->pipeline && !create_pipeline(decoder)) {
-stream_dropped_frame_on_playback(decoder->base.stream);
-return;
-}
-
 /* ref() the frame_msg for the buffer */
 spice_msg_in_ref(frame_msg);
 GstBuffer *buffer = 
gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
@@ -463,6 +458,12 @@ VideoDecoder* create_gstreamer_decoder(int codec_type, 
display_stream *stream)
 g_mutex_init(&decoder->queues_mutex);
 decoder->decoding_queue = g_queue_new();
 decoder->display_queue = g_queue_new();
+
+if (!create_pipeline(decoder))
+{
+decoder->base.destroy((VideoDecoder*)decoder);
+decoder = NULL;
+}
 }
 
 return (VideoDecoder*)decoder;
@@ -475,7 +476,7 @@ gboolean gstvideo_has_codec(int codec_type)
 
 VideoDecoder *decoder = create_gstreamer_decoder(codec_type, NULL);
 if (decoder) {
-has_codec = create_pipeline((SpiceGstDecoder*)decoder);
+has_codec = TRUE;
 decoder->destroy(decoder);
 }
 
-- 
2.8.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH win-vdagent v6 2/3] Encapsulating XPDM implementation

[Frediano Ziglio]

> 
> The Direct3D 9 API operates on either the Windows XP display driver
> model (XPDM) or the Windows Vista display driver model (WDDM), depending
> on the operating system installed.
> 
> This patch encapsulates the current XPDM interface implementation into
> XPDMInterface class which inherits DisplayConfig class. This patch
> makes it easier to introduce WDDM interface to Vdagent in future
> patches.
> 
> Based on a patch by Sandy Stutsman 
> 
> Signed-off-by: Dmitry Fleytman 
> Signed-off-by: Sameeh Jubran 
> ---
>  vdagent/desktop_layout.cpp| 148 +++-
>  vdagent/desktop_layout.h  |   9 +-
>  vdagent/display_configuration.cpp | 174
>  ++
>  vdagent/display_configuration.h   |  36 
>  4 files changed, 245 insertions(+), 122 deletions(-)
> 
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> index a7666ca..9c3e873 100644
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include "desktop_layout.h"
> +#include "display_configuration.h"
>  #include "vdlog.h"
>  
>  #ifdef __MINGW32__
> @@ -35,15 +36,17 @@ void DisplayMode::set_res(DWORD width, DWORD height,
> DWORD depth)
>  DesktopLayout::DesktopLayout()
>  : _total_width (0)
>  , _total_height (0)
> -, _send_monitors_position(false)
> +, _display_config (NULL)
>  {
>  MUTEX_INIT(_mutex);
> +_display_config = DisplayConfig::create_config();
>  get_displays();
>  }
>  
>  DesktopLayout::~DesktopLayout()
>  {
>  clean_displays();
> +delete _display_config;
>  }
>  
>  void DesktopLayout::get_displays()
> @@ -59,6 +62,7 @@ void DesktopLayout::get_displays()
>  unlock();
>  return;
>  }
> +_display_config->update_config_path();
>  clean_displays();
>  ZeroMemory(&dev_info, sizeof(dev_info));
>  dev_info.cb = sizeof(dev_info);
> @@ -82,12 +86,13 @@ void DesktopLayout::get_displays()
>  _displays[i] = NULL;
>  }
>  }
> -attached = !!(dev_info.StateFlags &
> DISPLAY_DEVICE_ATTACHED_TO_DESKTOP);
> +attached = _display_config->is_attached(&dev_info);
> +
>  EnumDisplaySettings(dev_info.DeviceName, ENUM_CURRENT_SETTINGS,
>  &mode);
>  _displays[display_id] = new DisplayMode(mode.dmPosition.x,
>  mode.dmPosition.y,
>  mode.dmPelsWidth,
>  mode.dmPelsHeight,
>  mode.dmBitsPerPel,
>  attached);
> -update_monitor_config(dev_info.DeviceName, _displays[display_id]);
> +_display_config->update_monitor_config(dev_info.DeviceName,
> _displays[display_id], &mode);
>  }
>  normalize_displays_pos();
>  unlock();
> @@ -121,6 +126,7 @@ void DesktopLayout::set_displays()
>  unlock();
>  return;
>  }
> +_display_config->update_config_path();
>  ZeroMemory(&dev_info, sizeof(dev_info));
>  dev_info.cb = sizeof(dev_info);
>  ZeroMemory(&dev_mode, sizeof(dev_mode));
> @@ -146,28 +152,32 @@ void DesktopLayout::set_displays()
>  break;
>  }
>  DisplayMode * mode(_displays.at(display_id));
> -if (!init_dev_mode(dev_info.DeviceName, &dev_mode, mode, normal_x,
> normal_y, true)) {
> +if (!init_dev_mode(dev_info.DeviceName, &dev_mode, mode)) {
>  vd_printf("No suitable mode found for display %S",
>  dev_info.DeviceName);
>  break;
>  }
>  vd_printf("Set display mode %lux%lu", dev_mode.dmPelsWidth,
>  dev_mode.dmPelsHeight);
> -LONG ret = ChangeDisplaySettingsEx(dev_info.DeviceName, &dev_mode,
> NULL,
> -   CDS_UPDATEREGISTRY | CDS_NORESET,
> NULL);
> -if (ret == DISP_CHANGE_SUCCESSFUL) {
> +if (_display_config->update_dev_mode_position(dev_info.DeviceName,
> &dev_mode,
> + mode->_pos_x -
> normal_x,
> + mode->_pos_y -
> normal_y)) {
>  dev_sets++;
> -update_monitor_config(dev_info.DeviceName, mode);
> +_display_config->update_monitor_config(dev_info.DeviceName,
> mode, &dev_mode);
>  }
>  if (!is_qxl) {
>  display_id++;
>  }
>  }
>  if (dev_sets) {
> -ChangeDisplaySettingsEx(NULL, NULL, NULL, 0, NULL);
> +_display_config->update_display_settings();
>  normalize_displays_pos();
>  }
>  unlock();
>  }
>  
> +void DesktopLayout::set_position_configurable(bool flag) {
> +_display_config->set_monitors_config(flag);
> +}
> +
>  // Normalize all display positions to non-negative coordinates and update
>  total width and height of
>  // t

[Spice-devel] [client v3 2/4] streaming: Don't crash if no frame was received before closing the stream

[Francois Gouget]

Signed-off-by: Francois Gouget 
---

This could potentially happen if we detect a stream right before it 
ends. But it's mostly useful for the next patch.

 src/channel-display.c | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/src/channel-display.c b/src/channel-display.c
index b4c9ec0..22c54f2 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1434,8 +1434,6 @@ static void destroy_stream(SpiceChannel *channel, int id)
 {
 SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
 display_stream *st;
-guint64 drops_duration_total = 0;
-guint32 num_out_frames;
 int i;
 
 g_return_if_fail(c != NULL);
@@ -1446,20 +1444,22 @@ static void destroy_stream(SpiceChannel *channel, int 
id)
 if (!st)
 return;
 
-num_out_frames = st->num_input_frames - st->arrive_late_count - 
st->num_drops_on_playback;
-CHANNEL_DEBUG(channel, "%s: id=%d #in-frames=%u out/in=%.2f "
-"#drops-on-receive=%u avg-late-time(ms)=%.2f "
-"#drops-on-playback=%u", __FUNCTION__,
-id,
-st->num_input_frames,
-num_out_frames / (double)st->num_input_frames,
-st->arrive_late_count,
-st->arrive_late_count ? st->arrive_late_time / 
((double)st->arrive_late_count): 0,
-st->num_drops_on_playback);
-if (st->num_drops_seqs) {
-CHANNEL_DEBUG(channel, "%s: #drops-sequences=%u ==>", __FUNCTION__, 
st->num_drops_seqs);
-}
-for (i = 0; i < st->num_drops_seqs; i++) {
+if (st->num_input_frames > 0) {
+guint64 drops_duration_total = 0;
+guint32 num_out_frames = st->num_input_frames - st->arrive_late_count 
- st->num_drops_on_playback;
+CHANNEL_DEBUG(channel, "%s: id=%d #in-frames=%u out/in=%.2f "
+"#drops-on-receive=%u avg-late-time(ms)=%.2f "
+"#drops-on-playback=%u", __FUNCTION__,
+id,
+st->num_input_frames,
+num_out_frames / (double)st->num_input_frames,
+st->arrive_late_count,
+st->arrive_late_count ? st->arrive_late_time / 
((double)st->arrive_late_count): 0,
+st->num_drops_on_playback);
+if (st->num_drops_seqs) {
+CHANNEL_DEBUG(channel, "%s: #drops-sequences=%u ==>", 
__FUNCTION__, st->num_drops_seqs);
+}
+for (i = 0; i < st->num_drops_seqs; i++) {
 drops_sequence_stats *stats = 
&g_array_index(st->drops_seqs_stats_arr,
  drops_sequence_stats,
  i);
@@ -1468,9 +1468,10 @@ static void destroy_stream(SpiceChannel *channel, int id)
stats->len,
stats->start_mm_time - 
st->first_frame_mm_time,
stats->duration);
-}
-if (st->num_drops_seqs) {
-CHANNEL_DEBUG(channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" 
==>", __FUNCTION__, drops_duration_total);
+}
+if (st->num_drops_seqs) {
+CHANNEL_DEBUG(channel, "%s: 
drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, 
drops_duration_total);
+}
 }
 
 g_array_free(st->drops_seqs_stats_arr, TRUE);
-- 
2.8.1

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

[Spice-devel] [client v3 3/4] streaming: Don't crash if the stream creation fails

[Francois Gouget]

Note that this implies closing the stream before receiving any frame.

Signed-off-by: Francois Gouget 
---
 src/channel-display.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/channel-display.c b/src/channel-display.c
index 22c54f2..709b3d2 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -109,6 +109,7 @@ static display_surface 
*find_surface(SpiceDisplayChannelPrivate *c, guint32 surf
 static void spice_display_channel_reset(SpiceChannel *channel, gboolean 
migrating);
 static void spice_display_channel_reset_capabilities(SpiceChannel *channel);
 static void destroy_canvas(display_surface *surface);
+static void destroy_stream(SpiceChannel *channel, int id);
 static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer 
data);
 static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
 
@@ -1125,6 +1126,7 @@ static void display_handle_stream_create(SpiceChannel 
*channel, SpiceMsgIn *in)
 }
 if (st->video_decoder == NULL) {
 spice_printerr("could not create a video decoder for codec %u", 
op->codec_type);
+destroy_stream(channel, op->id);
 }
 }
 
@@ -1231,6 +1233,7 @@ static void 
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
 g_return_if_fail(c->nstreams > stream_id);
 
 st = channel->priv->streams[stream_id];
+g_return_if_fail(st != NULL);
 
 if (!st->report_is_active) {
 return;
@@ -1353,6 +1356,7 @@ static void display_handle_stream_data(SpiceChannel 
*channel, SpiceMsgIn *in)
 g_return_if_fail(c->nstreams > op->id);
 
 st =  c->streams[op->id];
+g_return_if_fail(st != NULL);
 mmtime = stream_get_time(st);
 
 if (spice_msg_in_type(in) == SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
@@ -1420,6 +1424,7 @@ static void display_handle_stream_clip(SpiceChannel 
*channel, SpiceMsgIn *in)
 g_return_if_fail(c->nstreams > op->id);
 
 st = c->streams[op->id];
+g_return_if_fail(st != NULL);
 
 if (st->msg_clip) {
 spice_msg_in_unref(st->msg_clip);
-- 
2.8.1

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

[Spice-devel] [client v3 1/4] streaming: Check the stream id in display_update_stream_report() too

[Francois Gouget]

It's safer and more consistent than assuming the caller has done the
check already.

Signed-off-by: Francois Gouget 
---
 src/channel-display.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/channel-display.c b/src/channel-display.c
index cf9c583..b4c9ec0 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1222,9 +1222,16 @@ void stream_display_frame(display_stream *st, SpiceMsgIn 
*frame_msg,
 static void display_update_stream_report(SpiceDisplayChannel *channel, 
uint32_t stream_id,
  uint32_t frame_time, int32_t latency)
 {
-display_stream *st = channel->priv->streams[stream_id];
+SpiceDisplayChannelPrivate *c = channel->priv;
+display_stream *st;
 guint64 now;
 
+g_return_if_fail(c != NULL);
+g_return_if_fail(c->streams != NULL);
+g_return_if_fail(c->nstreams > stream_id);
+
+st = channel->priv->streams[stream_id];
+
 if (!st->report_is_active) {
 return;
 }
-- 
2.8.1

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

Re: [Spice-devel] [PATCH 3/7] gstreamer: Use static compile check

[Pavel Grunt]

On Thu, 2016-08-11 at 09:50 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/gstreamer-encoder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index cdd9696..7376d2b 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -1688,7 +1688,7 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType
> codec_type,
>  bitmap_ref_t bitmap_ref,
>  bitmap_unref_t bitmap_unref)
>  {
> -spice_return_val_if_fail(SPICE_GST_FRAME_STATISTICS_COUNT <=
> SPICE_GST_HISTORY_SIZE, NULL);
> +verify(SPICE_GST_FRAME_STATISTICS_COUNT <= SPICE_GST_HISTORY_SIZE);
>  spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG ||
>   codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
>   codec_type == SPICE_VIDEO_CODEC_TYPE_H264,
> NULL);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 02/11] Move CursorChannelClient to separate file

[Frediano Ziglio]

As a fast look seems that this patch is not updated considering
the encapsulation work on CursorChannel.

> 
> ---
>  server/Makefile.am |   2 +
>  server/cursor-channel-client.c | 120
>  +
>  server/cursor-channel-client.h |  50 +
>  server/cursor-channel.c| 101 --
>  server/cursor-channel.h|  12 +
>  server/red-worker.c|   1 +
>  6 files changed, 186 insertions(+), 100 deletions(-)
>  create mode 100644 server/cursor-channel-client.c
>  create mode 100644 server/cursor-channel-client.h
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index a249046..e1a6b43 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -118,6 +118,8 @@ libserver_la_SOURCES =\
>   red-worker.h\
>   display-channel.c   \
>   display-channel.h   \
> + cursor-channel-client.c \
> + cursor-channel-client.h \
>   cursor-channel.c\
>   cursor-channel.h\
>   red-pipe-item.c \
> diff --git a/server/cursor-channel-client.c b/server/cursor-channel-client.c
> new file mode 100644
> index 000..7dc936b
> --- /dev/null
> +++ b/server/cursor-channel-client.c
> @@ -0,0 +1,120 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2009 Red Hat, Inc.
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see
> .
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include 
> +#endif
> +
> +#include 
> +
> +#include "red-channel-client-private.h"

This "private" header is included a bit too much,
but is not responsibility of this patch to fix it.

> +#include "cache-item.h"
> +#include "cursor-channel.h"
> +
> +#define CLIENT_CURSOR_CACHE_SIZE 256
> +
> +#define CURSOR_CACHE_HASH_SHIFT 8
> +#define CURSOR_CACHE_HASH_SIZE (1 << CURSOR_CACHE_HASH_SHIFT)
> +#define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
> +#define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
> +#define CURSOR_CLIENT_TIMEOUT 300ULL //nano
> +
> +enum {
> +RED_PIPE_ITEM_TYPE_CURSOR = RED_PIPE_ITEM_TYPE_COMMON_LAST,
> +RED_PIPE_ITEM_TYPE_CURSOR_INIT,
> +RED_PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> +};
> +
> +struct CursorChannelClient {
> +RedChannelClient base;
> +
> +RedCacheItem *cursor_cache[CURSOR_CACHE_HASH_SIZE];
> +Ring cursor_cache_lru;
> +long cursor_cache_available;
> +uint32_t cursor_cache_items;
> +};
> +
> +
> +#define RCC_TO_CCC(rcc) ((CursorChannelClient*)rcc)
> +
> +#define CLIENT_CURSOR_CACHE
> +#include "cache-item.tmpl.c"
> +#undef CLIENT_CURSOR_CACHE
> +
> +#ifdef DEBUG_CURSORS
> +static int _cursor_count = 0;
> +#endif
> +
> +void cursor_channel_client_reset_cursor_cache(RedChannelClient *rcc)
> +{
> +red_cursor_cache_reset(RCC_TO_CCC(rcc), CLIENT_CURSOR_CACHE_SIZE);
> +}
> +
> +void cursor_channel_client_on_disconnect(RedChannelClient *rcc)
> +{
> +if (!rcc) {
> +return;
> +}
> +cursor_channel_client_reset_cursor_cache(rcc);
> +}
> +
> +void cursor_channel_client_migrate(RedChannelClient *rcc)
> +{
> +spice_return_if_fail(rcc);
> +
> +red_channel_client_pipe_add_type(rcc,
> RED_PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> +red_channel_client_default_migrate(rcc);
> +}
> +
> +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> RedClient *client, RedsStream *stream,
> +   int mig_target,
> +   uint32_t *common_caps, int
> num_common_caps,
> +   uint32_t *caps, int num_caps)
> +{
> +spice_return_val_if_fail(cursor, NULL);
> +spice_return_val_if_fail(client, NULL);
> +spice_return_val_if_fail(stream, NULL);
> +spice_return_val_if_fail(!num_common_caps || common_caps, NULL);
> +spice_return_val_if_fail(!num_caps || caps, NULL);
> +
> +CursorChannelClient *ccc =
> +
> (CursorChannelClient*)red_channel_client_create(sizeof(CursorChannelClient),
> +RED_CHANNEL(cursor),
> +client, str

[Spice-devel] [PATCH 1/7] gstreamer: Fix infinite loop in get_period_bit_rate

[Frediano Ziglio]

This was discovered by chance by me and Uri trying some remote connection
with slow network (8Mbit) and high latency

  $ ping 10.10.48.87 -c 3
  3 packets transmitted, 3 received, 0% packet loss, time 2002ms
  rtt min/avg/max/mdev = 281.069/316.758/374.413/41.153 ms

The encoder->history status was (edited for readability):

(gdb) p ((SpiceGstEncoder*)0x61c000264880)->history_first
$6 = 29
(gdb) p ((SpiceGstEncoder*)0x61c000264880)->history_last
$14 = 28
(gdb) p ((SpiceGstEncoder*)0x61c000264880)->history
[0] {mm_time = 11298131, duration = 7391006, size = 5616},
[1] {mm_time = 11298148, duration = 7373663, size = 5559},
[2] {mm_time = 11298166, duration = 7052209, size = 5511},
[3] {mm_time = 11298183, duration = 7006828, size = 5722},
[4] {mm_time = 11298199, duration = 7433311, size = 5756},
[5] {mm_time = 11298216, duration = 7134734, size = 5545},
[6] {mm_time = 11298232, duration = 7436589, size = 5521},
[7] {mm_time = 11298249, duration = 7152181, size = 5540},
[8] {mm_time = 11298266, duration = 7181308, size = 7796},
[9] {mm_time = 11298283, duration = 5053084, size = 50824},
[10] {mm_time = 11298298, duration = 7472305, size = 7427},
[11] {mm_time = 11298315, duration = 7385017, size = 6682},
[12] {mm_time = 11298333, duration = 7287125, size = 6377},
[13] {mm_time = 11298349, duration = 7191461, size = 6159},
[14] {mm_time = 11298367, duration = 7104546, size = 6035},
[15] {mm_time = 11298382, duration = 7266942, size = 5896},
[16] {mm_time = 11298400, duration = 7108001, size = 5812},
[17] {mm_time = 11298418, duration = 7020583, size = 5807},
[18] {mm_time = 11298433, duration = 7066056, size = 5758},
[19] {mm_time = 11298450, duration = 7052900, size = 5676},
[20] {mm_time = 11298467, duration = 7248233, size = 5765},
[21] {mm_time = 11298483, duration = 7077348, size = 5712},
[22] {mm_time = 11298502, duration = 7495368, size = 5835},
[23] {mm_time = 11298517, duration = 7068626, size = 5805},
[24] {mm_time = 11298534, duration = 7060375, size = 5801},
[25] {mm_time = 11298551, duration = 7020383, size = 5868},
[26] {mm_time = 11298568, duration = 7248400, size = 5830},
[27] {mm_time = 11298584, duration = 7001304, size = 6621},
[28] {mm_time = 11298600, duration = 7311600, size = 6113},
[29] {mm_time = 11297612, duration = 6999174, size = 5666},  <--- to
[30] {mm_time = 11297628, duration = 7506688, size = 5502},
[31] {mm_time = 11297646, duration = 7209494, size = 5687},
[32] {mm_time = 11297663, duration = 7396429, size = 5724},
[33] {mm_time = 11297679, duration = 7172624, size = 5839},
[34] {mm_time = 11297696, duration = 7300811, size = 5645},
[35] {mm_time = 11297713, duration = 7108985, size = 5553},
[36] {mm_time = 11297729, duration = 7171701, size = 5774},
[37] {mm_time = 11297745, duration = 7174018, size = 5637},
[38] {mm_time = 11297762, duration = 7313549, size = 5655},
[39] {mm_time = 11297780, duration = 5183985, size = 51014},
[40] {mm_time = 11297796, duration = 7038329, size = 7374},
[41] {mm_time = 11297813, duration = 7211506, size = 6585},
[42] {mm_time = 11297830, duration = 7112690, size = 5729},
[43] {mm_time = 11297846, duration = 7103074, size = 5761},
[44] {mm_time = 11297864, duration = 7599826, size = 5661},
[45] {mm_time = 11297879, duration = 7355392, size = 5351},
[46] {mm_time = 11297897, duration = 7454367, size = 5488},
[47] {mm_time = 11297913, duration = 7127145, size = 5573},
[48] {mm_time = 11297932, duration = 7550098, size = 5447},
[49] {mm_time = 11297948, duration = 7506884, size = 5809},
[50] {mm_time = 11297966, duration = 7405712, size = 5783},
[51] {mm_time = 11297982, duration = 7182025, size = 5599},
[52] {mm_time = 11298001, duration = 7323887, size = 5817},
[53] {mm_time = 11298014, duration = 7342091, size = 5575},
[54] {mm_time = 11298031, duration = 7596319, size = 5739},
[55] {mm_time = 11298052, duration = 7428169, size = 5669},
[56] {mm_time = 11298065, duration = 7457282, size = 5732},
[57] {mm_time = 11298081, duration = 7174029, size = 5541},
[58] {mm_time = 11298097, duration = 7340512, size = 5680},
[59] {mm_time = 11298115, duration = 7427439, size = 5701}}
(gdb) p from
$16 = 11297544
(gdb) p to
$17 = 11297612

You can see that encoder->history[encoder->history_first].mm_time == to
cause the check index == encoder->history_first to not be executed.

This code change was suggested by Uri.

Reported-by: Uri Lublin 
Signed-off-by: Frediano Ziglio 
Tested-by: Uri Lublin 
---
 server/gstreamer-encoder.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index e6790af..c8d7d88 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -434,19 +434,20 @@ static uint64_t get_period_bit_rate(SpiceGstEncoder 
*encoder, uint32_t from,
 sum += encoder->history[index].size;
 return (sum - 1) * 8 * MSEC_PER_SEC / (last_mm_time - from);
 
-} else if (index == encoder->history_first) {
+} else if (s

[Spice-devel] [PATCH 7/7] Avoids to initialise OpenSSL threading twice

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index 6f88649..f74c8d3 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2792,6 +2792,13 @@ static void openssl_thread_setup(void)
 {
 int i;
 
+/* Somebody else already setup threading for OpenSSL,
+ * don't do it twice to avoid possible races.
+ */
+if (CRYPTO_get_locking_callback() != NULL) {
+return;
+}
+
 lock_cs = OPENSSL_malloc(CRYPTO_num_locks() * sizeof(pthread_mutex_t));
 
 for (i = 0; i < CRYPTO_num_locks(); i++) {
-- 
2.7.4

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

[Spice-devel] [PATCH 2/7] gstreamer: Reduce #ifdef

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index c8d7d88..cdd9696 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -1240,6 +1240,14 @@ static void clear_zero_copy_queue(SpiceGstEncoder 
*encoder, gboolean unref_queue
 {
 /* Nothing to do */
 }
+
+static inline int zero_copy(SpiceGstEncoder *encoder,
+const SpiceBitmap *bitmap, gpointer bitmap_opaque,
+GstBuffer *buffer, uint32_t *chunk_index,
+uint32_t *chunk_offset, uint32_t *len)
+{
+return TRUE;
+}
 #endif
 
 /* A helper for push_raw_frame() */
@@ -1353,19 +1361,17 @@ static int push_raw_frame(SpiceGstEncoder *encoder,
 } else {
 /* We can copy the bitmap chunk by chunk */
 uint32_t chunk_index = 0;
-#ifdef DO_ZERO_COPY
 if (!zero_copy(encoder, bitmap, bitmap_opaque, buffer, &chunk_index,
&chunk_offset, &len)) {
 gst_buffer_unref(buffer);
 return VIDEO_ENCODER_FRAME_UNSUPPORTED;
 }
+
 /* len now contains the remaining number of bytes to copy.
  * However we must avoid any write to the GstBuffer object as it
  * would cause a copy of the read-only memory objects we just added.
  * Fortunately we can append extra writable memory objects instead.
  */
-#endif
-
 if (len) {
 uint8_t *dst = allocate_and_map_memory(len, &map, buffer);
 if (!dst) {
-- 
2.7.4

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

[Spice-devel] [PATCH 4/7] gstreamer: Use a dummy format to make sure format is always defined.

[Frediano Ziglio]

This avoid a check for NULL.
Also will be used to catch invalid values when table will be extended.

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 34 --
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 7376d2b..78d4f74 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -747,31 +747,36 @@ static inline void 
server_increase_bit_rate(SpiceGstEncoder *encoder,
 
 /* -- GStreamer pipeline -- */
 
+/* See GStreamer's part-mediatype-video-raw.txt and
+ * section-types-definitions.html documents.
+ */
+static const SpiceFormatForGStreamer format_map[] =  {
+/* First item is invalid.
+ * It's located first so the loop catch invalid values.
+ */
+{SPICE_BITMAP_FMT_INVALID, "", 0, 0, 0, 0, 0, 0},
+{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
+{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0, 0x7C00},
+/* TODO: Test the other formats */
+{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
+{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
+};
+#define GSTREAMER_FORMAT_INVALID (&format_map[0])
+
 /* A helper for spice_gst_encoder_encode_frame() */
 static const SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
 {
-/* See GStreamer's part-mediatype-video-raw.txt and
- * section-types-definitions.html documents.
- */
-static const SpiceFormatForGStreamer format_map[] =  {
-{SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
-{SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0, 
0x7C00},
-/* TODO: Test the other formats */
-{SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
-{SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
-};
-
 int i;
 for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
 if (format_map[i].spice_format == format) {
-if (i > 1) {
+if (i > 2) {
 spice_warning("The %d format has not been tested yet", format);
 }
 return &format_map[i];
 }
 }
 
-return NULL;
+return GSTREAMER_FORMAT_INVALID;
 }
 
 static void set_appsrc_caps(SpiceGstEncoder *encoder)
@@ -1669,7 +1674,7 @@ static void spice_gst_encoder_get_stats(VideoEncoder 
*video_encoder,
 VideoEncoderStats *stats)
 {
 SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
-uint64_t raw_bit_rate = encoder->width * encoder->height * 
(encoder->format ? encoder->format->bpp : 0) * get_source_fps(encoder);
+uint64_t raw_bit_rate = encoder->width * encoder->height * 
encoder->format->bpp * get_source_fps(encoder);
 
 spice_return_if_fail(stats != NULL);
 stats->starting_bit_rate = encoder->starting_bit_rate;
@@ -1718,6 +1723,7 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType 
codec_type,
 encoder->starting_bit_rate = starting_bit_rate;
 encoder->bitmap_ref = bitmap_ref;
 encoder->bitmap_unref = bitmap_unref;
+encoder->format = GSTREAMER_FORMAT_INVALID;
 g_mutex_init(&encoder->outbuf_mutex);
 g_cond_init(&encoder->outbuf_cond);
 
-- 
2.7.4

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

[Spice-devel] [PATCH 6/7] Make process_commands_generation variable type coherent

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/display-channel.c | 6 +++---
 server/display-channel.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index 4f52b79..5e0a004 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -24,7 +24,7 @@
 
 static void drawable_draw(DisplayChannel *display, Drawable *drawable);
 static Drawable *display_channel_drawable_try_new(DisplayChannel *display,
-  int 
process_commands_generation);
+  uint32_t 
process_commands_generation);
 
 uint32_t display_channel_generate_uid(DisplayChannel *display)
 {
@@ -1081,7 +1081,7 @@ static void display_channel_add_drawable(DisplayChannel 
*display, Drawable *draw
 }
 
 void display_channel_process_draw(DisplayChannel *display, RedDrawable 
*red_drawable,
-  int process_commands_generation)
+  uint32_t process_commands_generation)
 {
 Drawable *drawable =
 display_channel_get_drawable(display, red_drawable->effect, 
red_drawable,
@@ -1264,7 +1264,7 @@ static void drawables_init(DisplayChannel *display)
  * @return pointer to uninitialized Drawable or NULL on failure
  */
 static Drawable *display_channel_drawable_try_new(DisplayChannel *display,
-  int 
process_commands_generation)
+  uint32_t 
process_commands_generation)
 {
 Drawable *drawable;
 
diff --git a/server/display-channel.h b/server/display-channel.h
index 52a5783..7b71480 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -270,7 +270,7 @@ void   display_channel_destroy_surfaces 
 (DisplayCha
 uint32_t   display_channel_generate_uid  
(DisplayChannel *display);
 void   display_channel_process_draw  
(DisplayChannel *display,
   
RedDrawable *red_drawable,
-  int 
process_commands_generation);
+  uint32_t 
process_commands_generation);
 void   display_channel_process_surface_cmd   
(DisplayChannel *display,
   
RedSurfaceCmd *surface,
   int 
loadvm);
-- 
2.7.4

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

[Spice-devel] [PATCH 5/7] gstreamer: Peephole optimisation for SpiceFormatForGStreamer

[Frediano Ziglio]

Reduce structure length using static allocated string inside the
structure.
This will also avoid using .data.rel.ro section and relocations
reducing even more library size.

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 78d4f74..1221612 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -40,7 +40,7 @@
 
 typedef struct {
 SpiceBitmapFmt spice_format;
-const char *format;
+char format[8];
 uint32_t bpp;
 uint32_t depth;
 uint32_t endianness;
-- 
2.7.4

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

[Spice-devel] [PATCH 3/7] gstreamer: Use static compile check

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index cdd9696..7376d2b 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -1688,7 +1688,7 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType 
codec_type,
 bitmap_ref_t bitmap_ref,
 bitmap_unref_t bitmap_unref)
 {
-spice_return_val_if_fail(SPICE_GST_FRAME_STATISTICS_COUNT <= 
SPICE_GST_HISTORY_SIZE, NULL);
+verify(SPICE_GST_FRAME_STATISTICS_COUNT <= SPICE_GST_HISTORY_SIZE);
 spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG ||
  codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
  codec_type == SPICE_VIDEO_CODEC_TYPE_H264, NULL);
-- 
2.7.4

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