Re: [Spice-devel] [PATCH spice-server v2 1/2] Introduce some macros to help declaring new GObject

[Frediano Ziglio]

> 
> On Thu, Sep 07, 2017 at 03:58:21AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Wed, Sep 06, 2017 at 03:42:29PM +0100, Frediano Ziglio wrote:
> > > > The macros will implement most of the boilerplate needed
> > > > to declare an object.
> > > > Their usage are similar to GLib G_DECLARE_*_TYPE macros.
> > > 
> > > Can we/should we use the GLib provided macros when they are available,
> > > and copy/paste the GLib implementation in a -compat.h header for older
> > > systems. The GLib macros were introduced in GLib 2.43.4
> > > 
> > > Christophe
> > > 
> > 
> > That's fine. But our objects are currently neither final neither interfaces
> > so we can't use any of them.
> 
> MainChannel, CursorChannel, ... could be considered final, RedChannel,
> ... could most likely use G_DECLARE_DERIVABLE_TYPE. But the GLib macros might
> not exactly match what we are doing (naming, private data, ..).
> 
> Christophe
> 

Actually GLib macros are not C99 compatibles which I don't like that
much.
They also rely on 2.38 version for private support while we require
just 2.28 (there's no fields in the structure, all are private and
there's guarantee that private data have a fixed offset).
Another difference is that they require a MODULE while in most cases
we don't use it. Maybe we should have always a module (would be RED
in our case) but looks like lot of sed in the code which won't help
the history much.
If in the future we decide to move to the official macros I think
this is a step forward and helps adding new classes and be more
consistent.

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

[Spice-devel] [spice-server] channel: Remove unused red_channel_min_pipe_size

[Christophe Fergeau]

This could have been removed as part of 6e6126e024.

Signed-off-by: Christophe Fergeau 
---
 server/red-channel.c | 13 -
 server/red-channel.h |  2 --
 2 files changed, 15 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 5d832c32a..97be2f2bd 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -616,19 +616,6 @@ uint32_t red_channel_max_pipe_size(RedChannel *channel)
 return pipe_size;
 }
 
-uint32_t red_channel_min_pipe_size(RedChannel *channel)
-{
-RedChannelClient *rcc;
-uint32_t pipe_size = ~0;
-
-FOREACH_CLIENT(channel, rcc) {
-uint32_t new_size;
-new_size = red_channel_client_get_pipe_size(rcc);
-pipe_size = MIN(pipe_size, new_size);
-}
-return pipe_size == ~0 ? 0 : pipe_size;
-}
-
 uint32_t red_channel_sum_pipes_size(RedChannel *channel)
 {
 RedChannelClient *rcc;
diff --git a/server/red-channel.h b/server/red-channel.h
index e8feea2c9..55cfa7e1d 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -203,8 +203,6 @@ void red_channel_connect(RedChannel *channel, RedClient 
*client,
 
 /* return the sum of all the rcc pipe size */
 uint32_t red_channel_max_pipe_size(RedChannel *channel);
-/* return the min size of all the rcc pipe */
-uint32_t red_channel_min_pipe_size(RedChannel *channel);
 /* return the max size of all the rcc pipe */
 uint32_t red_channel_sum_pipes_size(RedChannel *channel);
 
-- 
2.13.5

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

Re: [Spice-devel] [PATCH spice-html5 1/4] spiceconn: Add function to test channel capabilities

[Frediano Ziglio]

> I agree, I will use your first suggestion.

> Tomáš

As a note, I'm not a Javascript expert as you, I just tested my suggestion with 
a small test 
script. 

Frediano 

> 2017-09-07 10:31 GMT+02:00 Frediano Ziglio < fzig...@redhat.com > :

> > >
> 
> > > This will be used in other commits.
> 
> > > ---
> 
> > > spiceconn.js | 19 +++
> 
> > > 1 file changed, 19 insertions(+)
> 
> > >
> 
> > > diff --git a/spiceconn.js b/spiceconn.js
> 
> > > index 33e7388..78d5820 100644
> 
> > > --- a/spiceconn.js
> 
> > > +++ b/spiceconn.js
> 
> > > @@ -243,6 +243,9 @@ SpiceConn.prototype =
> 
> > > else if (this.state == "link")
> 
> > > {
> 
> > > this.reply_link = new SpiceLinkReply(mb);
> 
> > > + this.common_caps = this.reply_link.common_caps;
> 
> > > + this.channel_caps = this.reply_link.channel_caps;
> 
> > > +
> 
> > > // FIXME - Screen the caps - require minihdr at least, right?
> 
> > > if (this.reply_link.error)
> 
> > > {
> 
> > > @@ -495,6 +498,22 @@ SpiceConn.prototype =
> 
> > > var e = new Error("Connection timed out.");
> 
> > > this.report_error(e);
> 
> > > },
> 
> > > +
> 
> > > + test_capability: function(caps, cap)
> 
> > > + {
> 
> > > + var ret = (caps >> cap) & 1;
> 
> > > + return ret;
> 
> > > + },
> 
> > > +
> 

> > This will work till cap is < 32, maybe safer to use a
> 

> > return (caps[cap >> 5] >> (cap & 31)) & 1;
> 

> > Or maybe put a comment where capabilities constants are defined
> 
> > to change this function when we reach 32.
> 

> > > + channel_test_capability: function(cap)
> 
> > > + {
> 
> > > + return this.test_capability(this.channel_caps, cap);
> 
> > > + },
> 
> > > +
> 
> > > + channel_test_common_capability: function(cap)
> 
> > > + {
> 
> > > + return this.test_capability(this.common_caps, cap);
> 
> > > + }
> 
> > > }
> 
> > >
> 
> > > function spiceconn_timeout(sc)
> 

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

Re: [Spice-devel] [PATCH spice-server 1/2] gl: fix client mouse mode

[Marc-André Lureau]

- Original Message -
> 
> 
> > Hi
> > 
> > - Original Message -
> > > Since 2.8, QEMU now longer creates QXL primary surfaces when using GL.
> > > This change broke client-side mouse mode, because Spice server relies on
> > > primary surface conditions.
> > > 
> > > When GL is enabled, use GL scanout informations.
> > > Mouse mode is always client when GL surfaces are used.
> > > 
> > > This patch and most of the message are based on a patch from
> > > Marc-André Lureau, just moving responsibility from reds to RedQxl.
> > 
> > My patch was updating reds_update_client_mouse_allowed() which I think was
> > a
> > better place to do cursor business.
> > 
> > Furthermore, it didn't mess with QXL/2D state.
> > 
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/red-qxl.c | 7 +++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > > index b556428d2..a003052ca 100644
> > > --- a/server/red-qxl.c
> > > +++ b/server/red-qxl.c
> > > @@ -873,6 +873,13 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
> > >  /* FIXME: find a way to coallesce all pending SCANOUTs */
> > >  dispatcher_send_message(qxl_state->dispatcher,
> > >  RED_WORKER_MESSAGE_GL_SCANOUT, );
> > > +
> > > +qxl_state->x_res = width;
> > > +qxl_state->y_res = height;
> > > +qxl_state->use_hardware_cursor = TRUE;
> > > +qxl_state->primary_active = TRUE;
> > 
> > I didn't need to touch qxl-state. Not sure what would be the side effect of
> > all this changes, and how they related to the problem.
> >  
> > > +
> > > +reds_update_client_mouse_allowed(qxl_state->reds);
> > >  }
> > >  
> > >  SPICE_GNUC_VISIBLE
> 
> Looks like you keep saying the same comments and I keep answering
> the same things.

You said:

"In all versions Qemu create a new primary surface before sending any 2D command
(tested and checked code).
As current version destroy the surface I doubt in the future will send 2D
command before creating the primary surface again (would be an error).

I think this state transitions should be documented in some way."

This doesn't explain why you mess with those qxl/2d values in the first place 
and what it has to do with cursor.

> I already explained my reasoning and you had never been able to
> raise any issue on this approach while you never been able to
> confute mine.
> 
> Frediano
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-html5 4/4] Display: Add support for the VP9 codec type

[Tomáš Bohdálek]

OK, I wil change it.

Tomáš

2017-09-07 11:39 GMT+02:00 Frediano Ziglio :

>
>
> Hi,
>
> I don't think so. Why do you think it would be better?
>
> Tomáš
>
> Is easier to extend when new codecs are added
>
> Frediano
>
> 2017-09-07 10:50 GMT+02:00 Frediano Ziglio :
>
>> >
>> > ---
>> >  display.js   | 26 --
>> >  spiceconn.js |  2 ++
>> >  webm.js  | 12 ++--
>> >  3 files changed, 32 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/display.js b/display.js
>> > index 0868f91..abd5b1a 100644
>> > --- a/display.js
>> > +++ b/display.js
>> > @@ -543,7 +543,8 @@ SpiceDisplayConn.prototype.process_channel_message
>> =
>> > function(msg)
>> >  else
>> >  this.streams[m.id] = m;
>> >
>> > -if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
>> > +if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
>> > +m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
>> >  {
>> >  var media = new MediaSource();
>> >  var v = document.createElement("video");
>> > @@ -606,7 +607,8 @@ SpiceDisplayConn.prototype.process_channel_message
>> =
>> > function(msg)
>> >  if (this.streams[m.base.id].codec_type ===
>> >  SPICE_VIDEO_CODEC_TYPE_MJPEG)
>> >  process_mjpeg_stream_data(this, m, time_until_due);
>> >
>> > -if (this.streams[m.base.id].codec_type ===
>> > SPICE_VIDEO_CODEC_TYPE_VP8)
>> > +if (this.streams[m.base.id].codec_type ===
>> > SPICE_VIDEO_CODEC_TYPE_VP8 ||
>> > +this.streams[m.base.id].codec_type ===
>> > SPICE_VIDEO_CODEC_TYPE_VP9)
>> >  process_video_stream_data(this.streams[m.base.id], m);
>> >
>> >  return true;
>> > @@ -640,7 +642,8 @@ SpiceDisplayConn.prototype.process_channel_message
>> =
>> > function(msg)
>> >  var m = new SpiceMsgDisplayStreamDestroy(msg.data);
>> >  STREAM_DEBUG > 0 && console.log(this.type + ":
>> MsgStreamDestroy id"
>> >  + m.id);
>> >
>> > -if (this.streams[m.id].codec_type ==
>> SPICE_VIDEO_CODEC_TYPE_VP8)
>> > +if (this.streams[m.id].codec_type ==
>> SPICE_VIDEO_CODEC_TYPE_VP8 ||
>> > +this.streams[m.id].codec_type ==
>> SPICE_VIDEO_CODEC_TYPE_VP9)
>> >  {
>> >  document.getElementById(this.
>> parent.screen_id).removeChild(this.streams[m.id].video);
>> >  this.streams[m.id].source_buffer = null;
>> > @@ -1036,14 +1039,24 @@ function handle_video_source_open(e)
>> >  {
>> >  var stream = this.stream;
>> >  var p = this.spiceconn;
>> > +var codec_type;
>> >
>> >  if (stream.source_buffer)
>> >  return;
>> >
>> > -var s = this.addSourceBuffer(SPICE_VP8_CODEC);
>> > +if (this.stream.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
>> > +{
>> > +codec_type = SPICE_VP8_CODEC;
>> > +}
>> > +else
>> > +{
>> > +codec_type = SPICE_VP9_CODEC;
>> > +}
>> > +
>>
>> Would not be better to use a switch ?
>>
>> > +var s = this.addSourceBuffer(codec_type);
>> >  if (! s)
>> >  {
>> > -p.log_err('Codec ' + SPICE_VP8_CODEC + ' not available.');
>> > +p.log_err('Codec ' + codec_type + ' not available.');
>> >  return;
>> >  }
>> >
>> > @@ -1054,7 +1067,8 @@ function handle_video_source_open(e)
>> >  listen_for_video_events(stream);
>> >
>> >  var h = new webm_Header();
>> > -var te = new webm_VideoTrackEntry(this.stream.stream_width,
>> > this.stream.stream_height);
>> > +var te = new webm_VideoTrackEntry(this.stream.stream_width,
>> > this.stream.stream_height,
>> > +  this.stream.codec_type);
>> >  var t = new webm_Tracks(te);
>> >
>> >  var mb = new ArrayBuffer(h.buffer_size() + t.buffer_size())
>> > diff --git a/spiceconn.js b/spiceconn.js
>> > index 78d5820..4c7bcaf 100644
>> > --- a/spiceconn.js
>> > +++ b/spiceconn.js
>> > @@ -147,6 +147,8 @@ SpiceConn.prototype =
>> >  (1 << SPICE_DISPLAY_CAP_CODEC_MJPEG);
>> >  if ('MediaSource' in window &&
>> >  MediaSource.isTypeSupported(SPICE_VP8_CODEC))
>> >  caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP8);
>> > +if ('MediaSource' in window &&
>> > MediaSource.isTypeSupported(SPICE_VP9_CODEC))
>> > +caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP9);
>> >  msg.channel_caps.push(caps);
>> >  }
>> >
>> > diff --git a/webm.js b/webm.js
>> > index 789da14..c697135 100644
>> > --- a/webm.js
>> > +++ b/webm.js
>> > @@ -88,6 +88,7 @@ var EXPECTED_PACKET_DURATION= 10;
>> >  var GAP_DETECTION_THRESHOLD = 50;
>> >
>> >  var SPICE_VP8_CODEC = 'video/webm;
>> codecs="vp8"';
>> > +var SPICE_VP9_CODEC = 'video/webm;
>> codecs="vp9"';
>> >
>> >  /*--
>> --
>> >  **  

Re: [Spice-devel] [PATCH spice-server 1/2] gl: fix client mouse mode

[Frediano Ziglio]

> 
> - Original Message -
> > 
> > 
> > > Hi
> > > 
> > > - Original Message -
> > > > Since 2.8, QEMU now longer creates QXL primary surfaces when using GL.
> > > > This change broke client-side mouse mode, because Spice server relies
> > > > on
> > > > primary surface conditions.
> > > > 
> > > > When GL is enabled, use GL scanout informations.
> > > > Mouse mode is always client when GL surfaces are used.
> > > > 
> > > > This patch and most of the message are based on a patch from
> > > > Marc-André Lureau, just moving responsibility from reds to RedQxl.
> > > 
> > > My patch was updating reds_update_client_mouse_allowed() which I think
> > > was
> > > a
> > > better place to do cursor business.
> > > 
> > > Furthermore, it didn't mess with QXL/2D state.
> > > 
> > > > 
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  server/red-qxl.c | 7 +++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > > > index b556428d2..a003052ca 100644
> > > > --- a/server/red-qxl.c
> > > > +++ b/server/red-qxl.c
> > > > @@ -873,6 +873,13 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
> > > >  /* FIXME: find a way to coallesce all pending SCANOUTs */
> > > >  dispatcher_send_message(qxl_state->dispatcher,
> > > >  RED_WORKER_MESSAGE_GL_SCANOUT, );
> > > > +
> > > > +qxl_state->x_res = width;
> > > > +qxl_state->y_res = height;
> > > > +qxl_state->use_hardware_cursor = TRUE;
> > > > +qxl_state->primary_active = TRUE;
> > > 
> > > I didn't need to touch qxl-state. Not sure what would be the side effect
> > > of
> > > all this changes, and how they related to the problem.
> > >  
> > > > +
> > > > +reds_update_client_mouse_allowed(qxl_state->reds);
> > > >  }
> > > >  
> > > >  SPICE_GNUC_VISIBLE
> > 
> > Looks like you keep saying the same comments and I keep answering
> > the same things.
> 
> You said:
> 
> "In all versions Qemu create a new primary surface before sending any 2D
> command
> (tested and checked code).
> As current version destroy the surface I doubt in the future will send 2D
> command before creating the primary surface again (would be an error).
> 
> I think this state transitions should be documented in some way."
> 
> This doesn't explain why you mess with those qxl/2d values in the first place
> and what it has to do with cursor.
> 

So now the size of the display is a cursor attribute in your opinion ?
I think you miss all February discussion
https://lists.freedesktop.org/archives/spice-devel/2017-February/date.html

> > I already explained my reasoning and you had never been able to
> > raise any issue on this approach while you never been able to
> > confute mine.
> > 

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

Re: [Spice-devel] [PATCH spice-html5 1/4] spiceconn: Add function to test channel capabilities

[Tomáš Bohdálek]

I agree, I will use your first suggestion.

Tomáš


2017-09-07 10:31 GMT+02:00 Frediano Ziglio :

> >
> > This will be used in other commits.
> > ---
> >  spiceconn.js | 19 +++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/spiceconn.js b/spiceconn.js
> > index 33e7388..78d5820 100644
> > --- a/spiceconn.js
> > +++ b/spiceconn.js
> > @@ -243,6 +243,9 @@ SpiceConn.prototype =
> >  else if (this.state == "link")
> >  {
> >  this.reply_link = new SpiceLinkReply(mb);
> > +this.common_caps = this.reply_link.common_caps;
> > +this.channel_caps = this.reply_link.channel_caps;
> > +
> >   // FIXME - Screen the caps - require minihdr at least,
> right?
> >  if (this.reply_link.error)
> >  {
> > @@ -495,6 +498,22 @@ SpiceConn.prototype =
> >  var e = new Error("Connection timed out.");
> >  this.report_error(e);
> >  },
> > +
> > +test_capability: function(caps, cap)
> > +{
> > +var ret = (caps >> cap) & 1;
> > +return ret;
> > +},
> > +
>
> This will work till cap is < 32, maybe safer to use a
>
>return (caps[cap >> 5] >> (cap & 31)) & 1;
>
> Or maybe put a comment where capabilities constants are defined
> to change this function when we reach 32.
>
> > +channel_test_capability: function(cap)
> > +{
> > +return this.test_capability(this.channel_caps, cap);
> > +},
> > +
> > +channel_test_common_capability: function(cap)
> > +{
> > +return this.test_capability(this.common_caps, cap);
> > +}
> >  }
> >
> >  function spiceconn_timeout(sc)
>
> Frediano
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] dcc: Make dcc_stop static

[Uri Lublin]

On 09/07/2017 12:17 PM, Frediano Ziglio wrote:

Just used by dcc_on_disconnect.

Signed-off-by: Frediano Ziglio 


Acked-by: Uri Lublin 


---
  server/dcc.c | 2 +-
  server/dcc.h | 1 -
  2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index 3fb598ff9..1e0ba790f 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -611,7 +611,7 @@ static void dcc_destroy_stream_agents(DisplayChannelClient 
*dcc)
  }
  }
  
-void dcc_stop(DisplayChannelClient *dcc)

+static void dcc_stop(DisplayChannelClient *dcc)
  {
  DisplayChannel *dc = DCC_TO_DC(dcc);
  
diff --git a/server/dcc.h b/server/dcc.h

index e34e83631..e0cfaecfa 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -138,7 +138,6 @@ DisplayChannelClient*  dcc_new  
 (DisplayCha

spice_wan_compression_t jpeg_state,

spice_wan_compression_t zlib_glz_state);
  void   dcc_start 
(DisplayChannelClient *dcc);
-void   dcc_stop  
(DisplayChannelClient *dcc);
  bool   dcc_handle_message
(RedChannelClient *rcc,

uint16_t type,

uint32_t size, void *msg);

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

[Spice-devel] [PATCH spice-html5 v2 4/4] Display: Add support for the VP9 codec type

[Tomáš Bohdálek]

---
 display.js   | 27 +--
 spiceconn.js |  2 ++
 webm.js  | 13 +++--
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/display.js b/display.js
index 0868f91..d711ced 100644
--- a/display.js
+++ b/display.js
@@ -543,7 +543,8 @@ SpiceDisplayConn.prototype.process_channel_message = 
function(msg)
 else
 this.streams[m.id] = m;
 
-if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
+m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
 {
 var media = new MediaSource();
 var v = document.createElement("video");
@@ -606,7 +607,8 @@ SpiceDisplayConn.prototype.process_channel_message = 
function(msg)
 if (this.streams[m.base.id].codec_type === 
SPICE_VIDEO_CODEC_TYPE_MJPEG)
 process_mjpeg_stream_data(this, m, time_until_due);
 
-if (this.streams[m.base.id].codec_type === SPICE_VIDEO_CODEC_TYPE_VP8)
+if (this.streams[m.base.id].codec_type === SPICE_VIDEO_CODEC_TYPE_VP8 
||
+this.streams[m.base.id].codec_type === SPICE_VIDEO_CODEC_TYPE_VP9)
 process_video_stream_data(this.streams[m.base.id], m);
 
 return true;
@@ -640,7 +642,8 @@ SpiceDisplayConn.prototype.process_channel_message = 
function(msg)
 var m = new SpiceMsgDisplayStreamDestroy(msg.data);
 STREAM_DEBUG > 0 && console.log(this.type + ": MsgStreamDestroy id" + 
m.id);
 
-if (this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+if (this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
+this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
 {
 
document.getElementById(this.parent.screen_id).removeChild(this.streams[m.id].video);
 this.streams[m.id].source_buffer = null;
@@ -1036,14 +1039,25 @@ function handle_video_source_open(e)
 {
 var stream = this.stream;
 var p = this.spiceconn;
+var codec_type = this.stream.codec_type;
 
 if (stream.source_buffer)
 return;
 
-var s = this.addSourceBuffer(SPICE_VP8_CODEC);
+switch (codec_type)
+{
+case SPICE_VIDEO_CODEC_TYPE_VP8:
+codec_type = SPICE_VP8_CODEC;
+break;
+case SPICE_VIDEO_CODEC_TYPE_VP9:
+codec_type = SPICE_VP9_CODEC;
+break;
+}
+
+var s = this.addSourceBuffer(codec_type);
 if (! s)
 {
-p.log_err('Codec ' + SPICE_VP8_CODEC + ' not available.');
+p.log_err('Codec ' + codec_type + ' not available.');
 return;
 }
 
@@ -1054,7 +1068,8 @@ function handle_video_source_open(e)
 listen_for_video_events(stream);
 
 var h = new webm_Header();
-var te = new webm_VideoTrackEntry(this.stream.stream_width, 
this.stream.stream_height);
+var te = new webm_VideoTrackEntry(this.stream.stream_width, 
this.stream.stream_height,
+  this.stream.codec_type);
 var t = new webm_Tracks(te);
 
 var mb = new ArrayBuffer(h.buffer_size() + t.buffer_size())
diff --git a/spiceconn.js b/spiceconn.js
index 78d5820..4c7bcaf 100644
--- a/spiceconn.js
+++ b/spiceconn.js
@@ -147,6 +147,8 @@ SpiceConn.prototype =
 (1 << SPICE_DISPLAY_CAP_CODEC_MJPEG);
 if ('MediaSource' in window && 
MediaSource.isTypeSupported(SPICE_VP8_CODEC))
 caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP8);
+if ('MediaSource' in window && 
MediaSource.isTypeSupported(SPICE_VP9_CODEC))
+caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP9);
 msg.channel_caps.push(caps);
 }
 
diff --git a/webm.js b/webm.js
index 789da14..4447195 100644
--- a/webm.js
+++ b/webm.js
@@ -88,6 +88,7 @@ var EXPECTED_PACKET_DURATION= 10;
 var GAP_DETECTION_THRESHOLD = 50;
 
 var SPICE_VP8_CODEC = 'video/webm; codecs="vp8"';
+var SPICE_VP9_CODEC = 'video/webm; codecs="vp9"';
 
 /*
 **  EBML utility functions
@@ -467,7 +468,7 @@ webm_AudioTrackEntry.prototype =
 },
 }
 
-function webm_VideoTrackEntry(width, height)
+function webm_VideoTrackEntry(width, height, codec_type)
 {
 this.id = WEBM_TRACK_ENTRY;
 this.number = 1;
@@ -482,8 +483,16 @@ function webm_VideoTrackEntry(width, height)
 this.codec_decode_all = 0; // fixme - check
 this.seek_pre_roll = 0; // 8000; // fixme - check
 this.codec_delay =   8000; // Must match codec_private.preskip
-this.codec_id = "V_VP8";
 this.video = new webm_Video(width, height);
+switch (codec_type)
+{
+case SPICE_VIDEO_CODEC_TYPE_VP8:
+this.codec_id = "V_VP8";
+break;
+case SPICE_VIDEO_CODEC_TYPE_VP9:
+this.codec_id = "V_VP9";
+break;
+}
 }
 
 webm_VideoTrackEntry.prototype =
-- 
2.9.5

[Spice-devel] [PATCH spice-server] dcc: Make dcc_stop static

[Frediano Ziglio]

Just used by dcc_on_disconnect.

Signed-off-by: Frediano Ziglio 
---
 server/dcc.c | 2 +-
 server/dcc.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index 3fb598ff9..1e0ba790f 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -611,7 +611,7 @@ static void dcc_destroy_stream_agents(DisplayChannelClient 
*dcc)
 }
 }
 
-void dcc_stop(DisplayChannelClient *dcc)
+static void dcc_stop(DisplayChannelClient *dcc)
 {
 DisplayChannel *dc = DCC_TO_DC(dcc);
 
diff --git a/server/dcc.h b/server/dcc.h
index e34e83631..e0cfaecfa 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -138,7 +138,6 @@ DisplayChannelClient*  dcc_new  
 (DisplayCha
   
spice_wan_compression_t jpeg_state,
   
spice_wan_compression_t zlib_glz_state);
 void   dcc_start 
(DisplayChannelClient *dcc);
-void   dcc_stop  
(DisplayChannelClient *dcc);
 bool   dcc_handle_message
(RedChannelClient *rcc,
   uint16_t 
type,
   uint32_t 
size, void *msg);
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 1/2] reds-stream: Allows to change core interface

[Frediano Ziglio]

When a stream is moved from the main thread to a
secondary one the events are potentially registered
using a different core interface. This cause memory
corruption accessing the watch registered in RedsStream.
This patch allows to always use the right interface.

Signed-off-by: Frediano Ziglio 
---
 server/reds-stream.c | 18 +-
 server/reds-stream.h |  1 +
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/server/reds-stream.c b/server/reds-stream.c
index 2ea547563..e5336265e 100644
--- a/server/reds-stream.c
+++ b/server/reds-stream.c
@@ -98,6 +98,7 @@ struct RedsStreamPrivate {
 ssize_t (*writev)(RedsStream *s, const struct iovec *iov, int iovcnt);
 
 RedsState *reds;
+SpiceCoreInterfaceInternal *core;
 };
 
 static ssize_t stream_write_cb(RedsStream *s, const void *buf, size_t size)
@@ -170,7 +171,7 @@ static ssize_t stream_ssl_read_cb(RedsStream *s, void *buf, 
size_t size)
 void reds_stream_remove_watch(RedsStream* s)
 {
 if (s->watch) {
-reds_core_watch_remove(s->priv->reds, s->watch);
+s->priv->core->watch_remove(s->priv->core, s->watch);
 s->watch = NULL;
 }
 }
@@ -418,6 +419,7 @@ RedsStream *reds_stream_new(RedsState *reds, int socket)
 stream->priv = (RedsStreamPrivate *)(stream+1);
 stream->priv->info = spice_new0(SpiceChannelEventInfo, 1);
 stream->priv->reds = reds;
+stream->priv->core = reds_get_core_interface(reds);
 reds_stream_set_socket(stream, socket);
 
 stream->priv->read = stream_read_cb;
@@ -427,6 +429,12 @@ RedsStream *reds_stream_new(RedsState *reds, int socket)
 return stream;
 }
 
+void reds_stream_set_core_interface(RedsStream *stream, 
SpiceCoreInterfaceInternal *core)
+{
+reds_stream_remove_watch(stream);
+stream->priv->core = core;
+}
+
 bool reds_stream_is_ssl(RedsStream *stream)
 {
 return (stream->priv->ssl != NULL);
@@ -511,7 +519,7 @@ static void async_read_handler(G_GNUC_UNUSED int fd,
 {
 AsyncRead *async = (AsyncRead *)data;
 RedsStream *stream = async->stream;
-RedsState *reds = stream->priv->reds;
+SpiceCoreInterfaceInternal *core = stream->priv->core;
 
 for (;;) {
 int n = async->end - async->now;
@@ -523,9 +531,9 @@ static void async_read_handler(G_GNUC_UNUSED int fd,
 switch (err) {
 case EAGAIN:
 if (!stream->watch) {
-stream->watch = reds_core_watch_add(reds, stream->socket,
-SPICE_WATCH_EVENT_READ,
-async_read_handler, 
async);
+stream->watch = core->watch_add(core, stream->socket,
+SPICE_WATCH_EVENT_READ,
+async_read_handler, async);
 }
 return;
 case EINTR:
diff --git a/server/reds-stream.h b/server/reds-stream.h
index 2dc39c23c..55a82745e 100644
--- a/server/reds-stream.h
+++ b/server/reds-stream.h
@@ -67,6 +67,7 @@ void reds_stream_remove_watch(RedsStream* s);
 void reds_stream_set_channel(RedsStream *stream, int connection_id,
  int channel_type, int channel_id);
 RedsStream *reds_stream_new(RedsState *reds, int socket);
+void reds_stream_set_core_interface(RedsStream *stream, 
SpiceCoreInterfaceInternal *core);
 bool reds_stream_is_ssl(RedsStream *stream);
 RedsStreamSslStatus reds_stream_ssl_accept(RedsStream *stream);
 int reds_stream_enable_ssl(RedsStream *stream, SSL_CTX *ctx);
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 2/2] Fix crash attempting to connect duplicate channels

[Frediano Ziglio]

You could easily trigger this issue using multiple monitors and a
modified spice-gtk client with this patch:

  --- a/src/channel-main.c
  +++ b/src/channel-main.c
  @@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
   {
   g_return_val_if_fail(c != NULL, FALSE);

  +if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
   spice_channel_new(c->session, c->type, c->id);

   g_object_unref(c->session);

which cause a crash like

(process:28742): Spice-WARNING **: Failed to create channel client: Client 
0x40246f5d0: duplicate channel type 2 id 0
2017-08-24 09:36:57.451+: shutting down, reason=crashed

RedChannelClient is an GInitable type, which means that the object is
constructed, and then the _init() function is called, which can fail.
If the _init() fails, the newly-created object will be destroyed. As
part of _init(), we add a new watch for the stream using the core
interface that is associated with the channel. After adding the watch,
our rcc creation fails (due to duplicate ID), and the rcc object is
unreffed. This results in a call to reds_stream_free() (since the rcc
now owns the stream). But in reds_stream_free, we were trying to remove
the watch from the core interface associated with the RedsState. For
most channels, these two core interfaces are equivalent. But for the
Display and Cursor channels, it is the core Glib-based interface
associated with the RedWorker.

The watch in RedsStream by default is bound to the Qemu provided
SpiceCoreInterface while RedChannelClient bound it to Glib one causing
the crash when the watch is deleted from RedsStream. Change the bound
interface.

Signed-off-by: Frediano Ziglio 
---
Changes since v2:
- improved commit message (Jonathon)
---
 server/red-channel-client.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 655e41c73..35bb8d922 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -339,6 +339,16 @@ red_channel_client_finalize(GObject *object)
 {
 RedChannelClient *self = RED_CHANNEL_CLIENT(object);
 
+SpiceCoreInterfaceInternal *core = 
red_channel_get_core_interface(self->priv->channel);
+if (self->priv->latency_monitor.timer) {
+core->timer_remove(core, self->priv->latency_monitor.timer);
+self->priv->latency_monitor.timer = NULL;
+}
+if (self->priv->connectivity_monitor.timer) {
+core->timer_remove(core, self->priv->connectivity_monitor.timer);
+self->priv->connectivity_monitor.timer = NULL;
+}
+
 reds_stream_free(self->priv->stream);
 self->priv->stream = NULL;
 
@@ -921,12 +931,14 @@ static gboolean 
red_channel_client_initable_init(GInitable *initable,
 }
 
 core = red_channel_get_core_interface(self->priv->channel);
-if (self->priv->stream)
+if (self->priv->stream) {
+reds_stream_set_core_interface(self->priv->stream, core);
 self->priv->stream->watch =
 core->watch_add(core, self->priv->stream->socket,
 SPICE_WATCH_EVENT_READ,
 red_channel_client_event,
 self);
+}
 
 if (self->priv->monitor_latency
 && reds_stream_get_family(self->priv->stream) != AF_UNIX) {
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 0/2] Fix crash attempting to connect duplicate channels

[Frediano Ziglio]

This is a condition a client can potentially trigger connecting to an
already connected channel.

Changes since v2:
- extended commit message.

Frediano Ziglio (2):
  reds-stream: Allows to change core interface
  Fix crash attempting to connect duplicate channels

 server/red-channel-client.c | 14 +-
 server/reds-stream.c| 18 +-
 server/reds-stream.h|  1 +
 3 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.13.5

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

Re: [Spice-devel] [PATCH spice-html5 4/4] Display: Add support for the VP9 codec type

[Frediano Ziglio]

> Hi,

> I don't think so. Why do you think it would be better?

> Tomáš

Is easier to extend when new codecs are added 

Frediano 

> 2017-09-07 10:50 GMT+02:00 Frediano Ziglio < fzig...@redhat.com > :

> > >
> 
> > > ---
> 
> > > display.js | 26 --
> 
> > > spiceconn.js | 2 ++
> 
> > > webm.js | 12 ++--
> 
> > > 3 files changed, 32 insertions(+), 8 deletions(-)
> 
> > >
> 
> > > diff --git a/display.js b/display.js
> 
> > > index 0868f91..abd5b1a 100644
> 
> > > --- a/display.js
> 
> > > +++ b/display.js
> 
> > > @@ -543,7 +543,8 @@ SpiceDisplayConn.prototype.process_channel_message =
> 
> > > function(msg)
> 
> > > else
> 
> > > this.streams[ m.id ] = m;
> 
> > >
> 
> > > - if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> 
> > > + if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
> 
> > > + m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
> 
> > > {
> 
> > > var media = new MediaSource();
> 
> > > var v = document.createElement("video");
> 
> > > @@ -606,7 +607,8 @@ SpiceDisplayConn.prototype.process_channel_message =
> 
> > > function(msg)
> 
> > > if (this.streams[ m.base.id ].codec_type ===
> 
> > > SPICE_VIDEO_CODEC_TYPE_MJPEG)
> 
> > > process_mjpeg_stream_data(this, m, time_until_due);
> 
> > >
> 
> > > - if (this.streams[ m.base.id ].codec_type ===
> 
> > > SPICE_VIDEO_CODEC_TYPE_VP8)
> 
> > > + if (this.streams[ m.base.id ].codec_type ===
> 
> > > SPICE_VIDEO_CODEC_TYPE_VP8 ||
> 
> > > + this.streams[ m.base.id ].codec_type ===
> 
> > > SPICE_VIDEO_CODEC_TYPE_VP9)
> 
> > > process_video_stream_data(this.streams[ m.base.id ], m);
> 
> > >
> 
> > > return true;
> 
> > > @@ -640,7 +642,8 @@ SpiceDisplayConn.prototype.process_channel_message =
> 
> > > function(msg)
> 
> > > var m = new SpiceMsgDisplayStreamDestroy(msg.data);
> 
> > > STREAM_DEBUG > 0 && console.log(this.type + ": MsgStreamDestroy id"
> 
> > > + m.id );
> 
> > >
> 
> > > - if (this.streams[ m.id ].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> 
> > > + if (this.streams[ m.id ].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
> 
> > > + this.streams[ m.id ].codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
> 
> > > {
> 
> > > document.getElementById(this.parent.screen_id).removeChild(this.streams[
> > > m.id ].video);
> 
> > > this.streams[ m.id ].source_buffer = null;
> 
> > > @@ -1036,14 +1039,24 @@ function handle_video_source_open(e)
> 
> > > {
> 
> > > var stream = this.stream;
> 
> > > var p = this.spiceconn;
> 
> > > + var codec_type;
> 
> > >
> 
> > > if (stream.source_buffer)
> 
> > > return;
> 
> > >
> 
> > > - var s = this.addSourceBuffer(SPICE_VP8_CODEC);
> 
> > > + if (this.stream.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> 
> > > + {
> 
> > > + codec_type = SPICE_VP8_CODEC;
> 
> > > + }
> 
> > > + else
> 
> > > + {
> 
> > > + codec_type = SPICE_VP9_CODEC;
> 
> > > + }
> 
> > > +
> 

> > Would not be better to use a switch ?
> 

> > > + var s = this.addSourceBuffer(codec_type);
> 
> > > if (! s)
> 
> > > {
> 
> > > - p.log_err('Codec ' + SPICE_VP8_CODEC + ' not available.');
> 
> > > + p.log_err('Codec ' + codec_type + ' not available.');
> 
> > > return;
> 
> > > }
> 
> > >
> 
> > > @@ -1054,7 +1067,8 @@ function handle_video_source_open(e)
> 
> > > listen_for_video_events(stream);
> 
> > >
> 
> > > var h = new webm_Header();
> 
> > > - var te = new webm_VideoTrackEntry(this.stream.stream_width,
> 
> > > this.stream.stream_height);
> 
> > > + var te = new webm_VideoTrackEntry(this.stream.stream_width,
> 
> > > this.stream.stream_height,
> 
> > > + this.stream.codec_type);
> 
> > > var t = new webm_Tracks(te);
> 
> > >
> 
> > > var mb = new ArrayBuffer(h.buffer_size() + t.buffer_size())
> 
> > > diff --git a/spiceconn.js b/spiceconn.js
> 
> > > index 78d5820..4c7bcaf 100644
> 
> > > --- a/spiceconn.js
> 
> > > +++ b/spiceconn.js
> 
> > > @@ -147,6 +147,8 @@ SpiceConn.prototype =
> 
> > > (1 << SPICE_DISPLAY_CAP_CODEC_MJPEG);
> 
> > > if ('MediaSource' in window &&
> 
> > > MediaSource.isTypeSupported(SPICE_VP8_CODEC))
> 
> > > caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP8);
> 
> > > + if ('MediaSource' in window &&
> 
> > > MediaSource.isTypeSupported(SPICE_VP9_CODEC))
> 
> > > + caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP9);
> 
> > > msg.channel_caps.push(caps);
> 
> > > }
> 
> > >
> 
> > > diff --git a/webm.js b/webm.js
> 
> > > index 789da14..c697135 100644
> 
> > > --- a/webm.js
> 
> > > +++ b/webm.js
> 
> > > @@ -88,6 +88,7 @@ var EXPECTED_PACKET_DURATION = 10;
> 
> > > var GAP_DETECTION_THRESHOLD = 50;
> 
> > >
> 
> > > var SPICE_VP8_CODEC = 'video/webm; codecs="vp8"';
> 
> > > +var SPICE_VP9_CODEC = 'video/webm; codecs="vp9"';
> 
> > >
> 
> > > /*
> 
> > > ** EBML utility functions
> 
> > > @@ -467,7 +468,7 @@ webm_AudioTrackEntry.prototype =
> 
> > > },
> 
> > > }
> 
> > >
> 
> > > -function webm_VideoTrackEntry(width, height)
> 
> > > +function webm_VideoTrackEntry(width, height, codec_type)
> 
> > > {
> 
> > > this.id = 

[Spice-devel] [PATCH spice-server 6/6] main-dispatcher: Avoid type conversion in dispatcher_handle_read

[Frediano Ziglio]

Pass proper type to callback to avoid having to convert to
the right type for each call.

Signed-off-by: Frediano Ziglio 
---
This make dispatcher_handle_read identical to handle_dev_input.
---
 server/main-dispatcher.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index 019bf4d3f..71f8f65d9 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -280,9 +280,9 @@ void main_dispatcher_client_disconnect(MainDispatcher 
*self, RedClient *client)
 
 static void dispatcher_handle_read(int fd, int event, void *opaque)
 {
-MainDispatcher *self = opaque;
+Dispatcher *dispatcher = opaque;
 
-dispatcher_handle_recv_read(DISPATCHER(self));
+dispatcher_handle_recv_read(dispatcher);
 }
 
 /*
@@ -311,7 +311,7 @@ void main_dispatcher_constructed(GObject *object)
 self->priv->core->watch_add(self->priv->core,
 dispatcher_get_recv_fd(DISPATCHER(self)),
 SPICE_WATCH_EVENT_READ, 
dispatcher_handle_read,
-self);
+DISPATCHER(self));
 dispatcher_register_handler(DISPATCHER(self), 
MAIN_DISPATCHER_CHANNEL_EVENT,
 main_dispatcher_handle_channel_event,
 sizeof(MainDispatcherChannelEventMessage), 0 
/* no ack */);
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 0/6] Minor Dispatcher related changes

[Frediano Ziglio]

Looking at Jonathon work on Dispatcher had some minor changes.
Some looks not related to Dispatcher but but are close to it.

Frediano Ziglio (6):
  red-qxl: Unify red_qxl_use_client_monitors_config and
red_qxl_client_monitors_config
  red-qxl: Move QXLInterface wrappers together
  spice-qxl: Add version information
  red-qxl: Avoid to use AsyncCommand for GL_DRAW_ASYNC message
  dispatcher: Remove "opaque" property
  main-dispatcher: Avoid type conversion in dispatcher_handle_read

 server/dispatcher.c  | 22 ++
 server/dispatcher.h  |  2 +-
 server/main-dispatcher.c |  6 ++---
 server/red-qxl.c | 58 +++-
 server/red-qxl.h |  1 -
 server/reds.c|  2 +-
 server/spice-qxl.h   |  6 +++--
 7 files changed, 39 insertions(+), 58 deletions(-)

-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 5/6] dispatcher: Remove "opaque" property

[Frediano Ziglio]

Is supposed to be used during initialization but is never
used.

Signed-off-by: Frediano Ziglio 
---
 server/dispatcher.c | 22 ++
 server/dispatcher.h |  2 +-
 server/red-qxl.c|  2 +-
 3 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/server/dispatcher.c b/server/dispatcher.c
index 6f2c4d85e..9e02d901b 100644
--- a/server/dispatcher.c
+++ b/server/dispatcher.c
@@ -66,8 +66,7 @@ struct DispatcherPrivate {
 
 enum {
 PROP_0,
-PROP_MAX_MESSAGE_TYPE,
-PROP_OPAQUE
+PROP_MAX_MESSAGE_TYPE
 };
 
 static void
@@ -83,9 +82,6 @@ dispatcher_get_property(GObject*object,
 case PROP_MAX_MESSAGE_TYPE:
 g_value_set_uint(value, self->priv->max_message_type);
 break;
-case PROP_OPAQUE:
-g_value_set_pointer(value, self->priv->opaque);
-break;
 default:
 G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
 }
@@ -104,9 +100,6 @@ dispatcher_set_property(GObject  *object,
 case PROP_MAX_MESSAGE_TYPE:
 self->priv->max_message_type = g_value_get_uint(value);
 break;
-case PROP_OPAQUE:
-dispatcher_set_opaque(self, g_value_get_pointer(value));
-break;
 default:
 G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
 }
@@ -168,15 +161,6 @@ dispatcher_class_init(DispatcherClass *klass)
   G_PARAM_STATIC_STRINGS |
   G_PARAM_READWRITE |
   G_PARAM_CONSTRUCT_ONLY));
-g_object_class_install_property(object_class,
-PROP_OPAQUE,
-g_param_spec_pointer("opaque",
- "opaque",
- "User data to pass to 
callbacks",
- 
G_PARAM_STATIC_STRINGS |
- G_PARAM_READWRITE |
- G_PARAM_CONSTRUCT));
-
 }
 
 static void
@@ -186,11 +170,10 @@ dispatcher_init(Dispatcher *self)
 }
 
 Dispatcher *
-dispatcher_new(size_t max_message_type, void *opaque)
+dispatcher_new(size_t max_message_type)
 {
 return g_object_new(TYPE_DISPATCHER,
 "max-message-type", (guint) max_message_type,
-"opaque", opaque,
 NULL);
 }
 
@@ -419,7 +402,6 @@ static void setup_dummy_signal_handler(void)
 void dispatcher_set_opaque(Dispatcher *self, void *opaque)
 {
 self->priv->opaque = opaque;
-g_object_notify(G_OBJECT(self), "opaque");
 }
 
 int dispatcher_get_recv_fd(Dispatcher *dispatcher)
diff --git a/server/dispatcher.h b/server/dispatcher.h
index 97b01de9c..00a828bdb 100644
--- a/server/dispatcher.h
+++ b/server/dispatcher.h
@@ -49,7 +49,7 @@ struct DispatcherClass
 
 GType dispatcher_get_type(void) G_GNUC_CONST;
 
-Dispatcher *dispatcher_new(size_t max_message_type, void *opaque);
+Dispatcher *dispatcher_new(size_t max_message_type);
 
 
 typedef void (*dispatcher_handle_message)(void *opaque,
diff --git a/server/red-qxl.c b/server/red-qxl.c
index b1804cf17..d876dc079 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -939,7 +939,7 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl)
 qxl_state->qxl = qxl;
 pthread_mutex_init(_state->scanout_mutex, NULL);
 qxl_state->scanout.drm_dma_buf_fd = -1;
-qxl_state->dispatcher = dispatcher_new(RED_WORKER_MESSAGE_COUNT, NULL);
+qxl_state->dispatcher = dispatcher_new(RED_WORKER_MESSAGE_COUNT);
 qxl_state->qxl_worker.major_version = SPICE_INTERFACE_QXL_MAJOR;
 qxl_state->qxl_worker.minor_version = SPICE_INTERFACE_QXL_MINOR;
 qxl_state->qxl_worker.wakeup = qxl_worker_wakeup;
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 3/6] spice-qxl: Add version information

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/spice-qxl.h | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/server/spice-qxl.h b/server/spice-qxl.h
index b8910bf47..04c657c58 100644
--- a/server/spice-qxl.h
+++ b/server/spice-qxl.h
@@ -197,12 +197,14 @@ struct QXLInterface {
 void (*update_area_complete)(QXLInstance *qin, uint32_t surface_id,
  struct QXLRect *updated_rects,
  uint32_t num_updated_rects);
+/* Available since version 3.2 */
 void (*set_client_capabilities)(QXLInstance *qin,
 uint8_t client_present,
 uint8_t caps[SPICE_CAPABILITIES_SIZE]);
-/* returns 1 if the interface is supported, 0 otherwise.
+/* Returns 1 if the interface is supported, 0 otherwise.
  * if monitors_config is NULL nothing is done except reporting the
- * return code. */
+ * return code.
+ * Available since version 3.3 */
 int (*client_monitors_config)(QXLInstance *qin,
   VDAgentMonitorsConfig *monitors_config);
 };
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 1/6] red-qxl: Unify red_qxl_use_client_monitors_config and red_qxl_client_monitors_config

[Frediano Ziglio]

These 2 functions were doing the same stuff, calling
client_monitors_config callback in QXLInterface.
The only difference was that red_qxl_use_client_monitors_config
used a NULL value.
Added the check for proper version, QXLInstance before 3.3
did not have this callback.

Signed-off-by: Frediano Ziglio 
---
 server/red-qxl.c | 10 ++
 server/red-qxl.h |  1 -
 server/reds.c|  2 +-
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/server/red-qxl.c b/server/red-qxl.c
index b556428d2..54d642ad9 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -195,17 +195,11 @@ static void red_qxl_update_area(QXLState *qxl_state, 
uint32_t surface_id,
 );
 }
 
-gboolean red_qxl_use_client_monitors_config(QXLInstance *qxl)
-{
-return (red_qxl_check_qxl_version(qxl, 3, 3) &&
-qxl_get_interface(qxl)->client_monitors_config &&
-qxl_get_interface(qxl)->client_monitors_config(qxl, NULL));
-}
-
 gboolean red_qxl_client_monitors_config(QXLInstance *qxl,
 VDAgentMonitorsConfig *monitors_config)
 {
-return (qxl_get_interface(qxl)->client_monitors_config &&
+return (red_qxl_check_qxl_version(qxl, 3, 3) &&
+qxl_get_interface(qxl)->client_monitors_config &&
 qxl_get_interface(qxl)->client_monitors_config(qxl, monitors_config));
 }
 
diff --git a/server/red-qxl.h b/server/red-qxl.h
index f925f065b..5d57780a9 100644
--- a/server/red-qxl.h
+++ b/server/red-qxl.h
@@ -37,7 +37,6 @@ void red_qxl_start(QXLInstance *qxl);
 uint32_t red_qxl_get_ram_size(QXLInstance *qxl);
 void red_qxl_async_complete(QXLInstance *qxl, AsyncCommand *async_command);
 struct Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl);
-gboolean red_qxl_use_client_monitors_config(QXLInstance *qxl);
 gboolean red_qxl_client_monitors_config(QXLInstance *qxl, 
VDAgentMonitorsConfig *monitors_config);
 gboolean red_qxl_get_primary_active(QXLInstance *qxl);
 gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res, gint 
*y_res);
diff --git a/server/reds.c b/server/reds.c
index b6ecc6c69..24ec2bdde 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -4401,7 +4401,7 @@ static gboolean reds_use_client_monitors_config(RedsState 
*reds)
 }
 
 FOREACH_QXL_INSTANCE(reds, qxl) {
-if (!red_qxl_use_client_monitors_config(qxl))
+if (!red_qxl_client_monitors_config(qxl, NULL))
 return FALSE;
 }
 return TRUE;
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 2/6] red-qxl: Move QXLInterface wrappers together

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-qxl.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/server/red-qxl.c b/server/red-qxl.c
index 54d642ad9..bec063c17 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -638,18 +638,6 @@ static void qxl_worker_loadvm_commands(QXLWorker 
*qxl_worker,
 red_qxl_loadvm_commands(qxl_state, ext, count);
 }
 
-void red_qxl_attach_worker(QXLInstance *qxl)
-{
-QXLInterface *interface = qxl_get_interface(qxl);
-interface->attache_worker(qxl, >st->qxl_worker);
-}
-
-void red_qxl_set_compression_level(QXLInstance *qxl, int level)
-{
-QXLInterface *interface = qxl_get_interface(qxl);
-interface->set_compression_level(qxl, level);
-}
-
 uint32_t red_qxl_get_ram_size(QXLInstance *qxl)
 {
 QXLDevInitInfo qxl_info;
@@ -1073,6 +1061,18 @@ RedsState* red_qxl_get_server(QXLState *qxl_state)
 return qxl_state->reds;
 }
 
+void red_qxl_attach_worker(QXLInstance *qxl)
+{
+QXLInterface *interface = qxl_get_interface(qxl);
+interface->attache_worker(qxl, >st->qxl_worker);
+}
+
+void red_qxl_set_compression_level(QXLInstance *qxl, int level)
+{
+QXLInterface *interface = qxl_get_interface(qxl);
+interface->set_compression_level(qxl, level);
+}
+
 void red_qxl_get_init_info(QXLInstance *qxl, QXLDevInitInfo *info)
 {
 QXLInterface *interface = qxl_get_interface(qxl);
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 4/6] red-qxl: Avoid to use AsyncCommand for GL_DRAW_ASYNC message

[Frediano Ziglio]

AsyncCommand is used to handle asynchronous messages from the
dispatcher.
GL_DRAW_ASYNC is mainly using it to store the cookie.

The value of GL_DRAW_COOKIE_INVALID was choosen to allow implementing
cookies (which basically are handles) either using indexes (where 0 is
valid) or pointers (where 0 is invalid). Currently Qemu uses pointers.

Signed-off-by: Frediano Ziglio 
---
 server/red-qxl.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/server/red-qxl.c b/server/red-qxl.c
index bec063c17..b1804cf17 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -62,9 +62,11 @@ struct QXLState {
 
 pthread_mutex_t scanout_mutex;
 SpiceMsgDisplayGlScanoutUnix scanout;
-struct AsyncCommand *gl_draw_async;
+uint64_t gl_draw_cookie;
 };
 
+#define GL_DRAW_COOKIE_INVALID (~((uint64_t) 0))
+
 int red_qxl_check_qxl_version(QXLInstance *qxl, int major, int minor)
 {
 int qxl_major = qxl_get_interface(qxl)->base.major_version;
@@ -833,7 +835,7 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
 spice_return_if_fail(qxl != NULL);
 
 QXLState *qxl_state = qxl->st;
-spice_return_if_fail(qxl_state->gl_draw_async == NULL);
+spice_return_if_fail(qxl_state->gl_draw_cookie == GL_DRAW_COOKIE_INVALID);
 
 pthread_mutex_lock(_state->scanout_mutex);
 
@@ -877,13 +879,15 @@ void spice_qxl_gl_draw_async(QXLInstance *qxl,
 spice_return_if_fail(qxl != NULL);
 qxl_state = qxl->st;
 if (qxl_state->scanout.drm_dma_buf_fd == -1) {
+QXLInterface *interface = qxl_get_interface(qxl);
+
 spice_warning("called spice_qxl_gl_draw_async without a buffer");
-red_qxl_async_complete(qxl, async_command_alloc(qxl_state, message, 
cookie));
+interface->async_complete(qxl, cookie);
 return;
 }
-spice_return_if_fail(qxl_state->gl_draw_async == NULL);
+spice_return_if_fail(qxl_state->gl_draw_cookie == GL_DRAW_COOKIE_INVALID);
 
-qxl_state->gl_draw_async = async_command_alloc(qxl_state, message, cookie);
+qxl_state->gl_draw_cookie = cookie;
 dispatcher_send_message(qxl_state->dispatcher, message, );
 }
 
@@ -899,7 +903,6 @@ void red_qxl_async_complete(QXLInstance *qxl, AsyncCommand 
*async_command)
 case RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC:
 case RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC:
 case RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC:
-case RED_WORKER_MESSAGE_GL_DRAW_ASYNC:
 break;
 case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC:
 red_qxl_create_primary_surface_complete(qxl->st);
@@ -916,10 +919,11 @@ void red_qxl_async_complete(QXLInstance *qxl, 
AsyncCommand *async_command)
 
 void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
 {
+QXLInterface *interface = qxl_get_interface(qxl);
 /* this reset before usage prevent a possible race condition */
-struct AsyncCommand *async = qxl->st->gl_draw_async;
-qxl->st->gl_draw_async = NULL;
-red_qxl_async_complete(qxl, async);
+uint64_t cookie = qxl->st->gl_draw_cookie;
+qxl->st->gl_draw_cookie = GL_DRAW_COOKIE_INVALID;
+interface->async_complete(qxl, cookie);
 }
 
 void red_qxl_init(RedsState *reds, QXLInstance *qxl)
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 1/2] gl: fix client mouse mode

[Frediano Ziglio]

Since 2.8, QEMU now longer creates QXL primary surfaces when using GL.
This change broke client-side mouse mode, because Spice server relies on
primary surface conditions.

When GL is enabled, use GL scanout informations.
Mouse mode is always client when GL surfaces are used.

This patch and most of the message are based on a patch from
Marc-André Lureau, just moving responsibility from reds to RedQxl.

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

diff --git a/server/red-qxl.c b/server/red-qxl.c
index b556428d2..a003052ca 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -873,6 +873,13 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
 /* FIXME: find a way to coallesce all pending SCANOUTs */
 dispatcher_send_message(qxl_state->dispatcher,
 RED_WORKER_MESSAGE_GL_SCANOUT, );
+
+qxl_state->x_res = width;
+qxl_state->y_res = height;
+qxl_state->use_hardware_cursor = TRUE;
+qxl_state->primary_active = TRUE;
+
+reds_update_client_mouse_allowed(qxl_state->reds);
 }
 
 SPICE_GNUC_VISIBLE
-- 
2.13.5

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

[Spice-devel] [PATCH spice-server 2/2] red-qxl: Rename primary_active to display_active

[Frediano Ziglio]

To avoid confusion use the more generic "display" instead of "primary".
"primary" is used in the code widely to mean the primary surface.
In the past (before 3D support) having a primary surface mean both
the availability of something to render (that is the device was enabled
and ready) and the presence of a canvas to draw on.
As RedQxl is just interested in the state and size of the display
use "display" as terminology.

Signed-off-by: Frediano Ziglio 
---
 server/red-qxl.c | 12 ++--
 server/red-qxl.h |  2 +-
 server/reds.c|  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/server/red-qxl.c b/server/red-qxl.c
index a003052ca..1c3714565 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -51,7 +51,7 @@ struct QXLState {
 QXLInstance *qxl;
 Dispatcher *dispatcher;
 uint32_t pending;
-int primary_active;
+bool display_active;
 int x_res;
 int y_res;
 int use_hardware_cursor;
@@ -320,7 +320,7 @@ static void 
red_qxl_destroy_primary_surface_complete(QXLState *qxl_state)
 qxl_state->x_res = 0;
 qxl_state->y_res = 0;
 qxl_state->use_hardware_cursor = FALSE;
-qxl_state->primary_active = FALSE;
+qxl_state->display_active = false;
 
 reds_update_client_mouse_allowed(qxl_state->reds);
 }
@@ -373,7 +373,7 @@ static void 
red_qxl_create_primary_surface_complete(QXLState *qxl_state)
 qxl_state->x_res = surface->width;
 qxl_state->y_res = surface->height;
 qxl_state->use_hardware_cursor = surface->mouse_mode;
-qxl_state->primary_active = TRUE;
+qxl_state->display_active = true;
 
 reds_update_client_mouse_allowed(qxl_state->reds);
 memset(_state->surface_create, 0, sizeof(QXLDevSurfaceCreate));
@@ -877,7 +877,7 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
 qxl_state->x_res = width;
 qxl_state->y_res = height;
 qxl_state->use_hardware_cursor = TRUE;
-qxl_state->primary_active = TRUE;
+qxl_state->display_active = true;
 
 reds_update_client_mouse_allowed(qxl_state->reds);
 }
@@ -1029,9 +1029,9 @@ void red_qxl_clear_pending(QXLState *qxl_state, int 
pending)
 clear_bit(pending, _state->pending);
 }
 
-gboolean red_qxl_get_primary_active(QXLInstance *qxl)
+bool red_qxl_is_display_active(QXLInstance *qxl)
 {
-return qxl->st->primary_active;
+return qxl->st->display_active;
 }
 
 gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res, gint 
*y_res)
diff --git a/server/red-qxl.h b/server/red-qxl.h
index f925f065b..4a0bab83b 100644
--- a/server/red-qxl.h
+++ b/server/red-qxl.h
@@ -39,7 +39,7 @@ void red_qxl_async_complete(QXLInstance *qxl, AsyncCommand 
*async_command);
 struct Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl);
 gboolean red_qxl_use_client_monitors_config(QXLInstance *qxl);
 gboolean red_qxl_client_monitors_config(QXLInstance *qxl, 
VDAgentMonitorsConfig *monitors_config);
-gboolean red_qxl_get_primary_active(QXLInstance *qxl);
+bool red_qxl_is_display_active(QXLInstance *qxl);
 gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res, gint 
*y_res);
 SpiceMsgDisplayGlScanoutUnix *red_qxl_get_gl_scanout(QXLInstance *qxl);
 void red_qxl_put_gl_scanout(QXLInstance *qxl, SpiceMsgDisplayGlScanoutUnix 
*scanout);
diff --git a/server/reds.c b/server/reds.c
index b6ecc6c69..aff70768e 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -4374,7 +4374,7 @@ void reds_update_client_mouse_allowed(RedsState *reds)
 
 allow_now = TRUE;
 FOREACH_QXL_INSTANCE(reds, qxl) {
-if (red_qxl_get_primary_active(qxl)) {
+if (red_qxl_is_display_active(qxl)) {
 allow_now = red_qxl_get_allow_client_mouse(qxl, _res, 
_res);
 break;
 }
-- 
2.13.5

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

Re: [Spice-devel] [PATCH spice-server 1/2] gl: fix client mouse mode

[Frediano Ziglio]

> Hi
> 
> - Original Message -
> > Since 2.8, QEMU now longer creates QXL primary surfaces when using GL.
> > This change broke client-side mouse mode, because Spice server relies on
> > primary surface conditions.
> > 
> > When GL is enabled, use GL scanout informations.
> > Mouse mode is always client when GL surfaces are used.
> > 
> > This patch and most of the message are based on a patch from
> > Marc-André Lureau, just moving responsibility from reds to RedQxl.
> 
> My patch was updating reds_update_client_mouse_allowed() which I think was a
> better place to do cursor business.
> 
> Furthermore, it didn't mess with QXL/2D state.
> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/red-qxl.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index b556428d2..a003052ca 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -873,6 +873,13 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
> >  /* FIXME: find a way to coallesce all pending SCANOUTs */
> >  dispatcher_send_message(qxl_state->dispatcher,
> >  RED_WORKER_MESSAGE_GL_SCANOUT, );
> > +
> > +qxl_state->x_res = width;
> > +qxl_state->y_res = height;
> > +qxl_state->use_hardware_cursor = TRUE;
> > +qxl_state->primary_active = TRUE;
> 
> I didn't need to touch qxl-state. Not sure what would be the side effect of
> all this changes, and how they related to the problem.
>  
> > +
> > +reds_update_client_mouse_allowed(qxl_state->reds);
> >  }
> >  
> >  SPICE_GNUC_VISIBLE

Looks like you keep saying the same comments and I keep answering
the same things.
I already explained my reasoning and you had never been able to
raise any issue on this approach while you never been able to
confute mine.

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

Re: [Spice-devel] [PATCH spice-html5 1/4] spiceconn: Add function to test channel capabilities

[Frediano Ziglio]

> 
> This will be used in other commits.
> ---
>  spiceconn.js | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/spiceconn.js b/spiceconn.js
> index 33e7388..78d5820 100644
> --- a/spiceconn.js
> +++ b/spiceconn.js
> @@ -243,6 +243,9 @@ SpiceConn.prototype =
>  else if (this.state == "link")
>  {
>  this.reply_link = new SpiceLinkReply(mb);
> +this.common_caps = this.reply_link.common_caps;
> +this.channel_caps = this.reply_link.channel_caps;
> +
>   // FIXME - Screen the caps - require minihdr at least, right?
>  if (this.reply_link.error)
>  {
> @@ -495,6 +498,22 @@ SpiceConn.prototype =
>  var e = new Error("Connection timed out.");
>  this.report_error(e);
>  },
> +
> +test_capability: function(caps, cap)
> +{
> +var ret = (caps >> cap) & 1;
> +return ret;
> +},
> +

This will work till cap is < 32, maybe safer to use a

   return (caps[cap >> 5] >> (cap & 31)) & 1;

Or maybe put a comment where capabilities constants are defined
to change this function when we reach 32.

> +channel_test_capability: function(cap)
> +{
> +return this.test_capability(this.channel_caps, cap);
> +},
> +
> +channel_test_common_capability: function(cap)
> +{
> +return this.test_capability(this.common_caps, cap);
> +}
>  }
>  
>  function spiceconn_timeout(sc)

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

Re: [Spice-devel] [PATCH spice-html5 4/4] Display: Add support for the VP9 codec type

[Frediano Ziglio]

> 
> ---
>  display.js   | 26 --
>  spiceconn.js |  2 ++
>  webm.js  | 12 ++--
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/display.js b/display.js
> index 0868f91..abd5b1a 100644
> --- a/display.js
> +++ b/display.js
> @@ -543,7 +543,8 @@ SpiceDisplayConn.prototype.process_channel_message =
> function(msg)
>  else
>  this.streams[m.id] = m;
>  
> -if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> +if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
> +m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
>  {
>  var media = new MediaSource();
>  var v = document.createElement("video");
> @@ -606,7 +607,8 @@ SpiceDisplayConn.prototype.process_channel_message =
> function(msg)
>  if (this.streams[m.base.id].codec_type ===
>  SPICE_VIDEO_CODEC_TYPE_MJPEG)
>  process_mjpeg_stream_data(this, m, time_until_due);
>  
> -if (this.streams[m.base.id].codec_type ===
> SPICE_VIDEO_CODEC_TYPE_VP8)
> +if (this.streams[m.base.id].codec_type ===
> SPICE_VIDEO_CODEC_TYPE_VP8 ||
> +this.streams[m.base.id].codec_type ===
> SPICE_VIDEO_CODEC_TYPE_VP9)
>  process_video_stream_data(this.streams[m.base.id], m);
>  
>  return true;
> @@ -640,7 +642,8 @@ SpiceDisplayConn.prototype.process_channel_message =
> function(msg)
>  var m = new SpiceMsgDisplayStreamDestroy(msg.data);
>  STREAM_DEBUG > 0 && console.log(this.type + ": MsgStreamDestroy id"
>  + m.id);
>  
> -if (this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> +if (this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
> +this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
>  {
>  
> document.getElementById(this.parent.screen_id).removeChild(this.streams[m.id].video);
>  this.streams[m.id].source_buffer = null;
> @@ -1036,14 +1039,24 @@ function handle_video_source_open(e)
>  {
>  var stream = this.stream;
>  var p = this.spiceconn;
> +var codec_type;
>  
>  if (stream.source_buffer)
>  return;
>  
> -var s = this.addSourceBuffer(SPICE_VP8_CODEC);
> +if (this.stream.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> +{
> +codec_type = SPICE_VP8_CODEC;
> +}
> +else
> +{
> +codec_type = SPICE_VP9_CODEC;
> +}
> +

Would not be better to use a switch ?

> +var s = this.addSourceBuffer(codec_type);
>  if (! s)
>  {
> -p.log_err('Codec ' + SPICE_VP8_CODEC + ' not available.');
> +p.log_err('Codec ' + codec_type + ' not available.');
>  return;
>  }
>  
> @@ -1054,7 +1067,8 @@ function handle_video_source_open(e)
>  listen_for_video_events(stream);
>  
>  var h = new webm_Header();
> -var te = new webm_VideoTrackEntry(this.stream.stream_width,
> this.stream.stream_height);
> +var te = new webm_VideoTrackEntry(this.stream.stream_width,
> this.stream.stream_height,
> +  this.stream.codec_type);
>  var t = new webm_Tracks(te);
>  
>  var mb = new ArrayBuffer(h.buffer_size() + t.buffer_size())
> diff --git a/spiceconn.js b/spiceconn.js
> index 78d5820..4c7bcaf 100644
> --- a/spiceconn.js
> +++ b/spiceconn.js
> @@ -147,6 +147,8 @@ SpiceConn.prototype =
>  (1 << SPICE_DISPLAY_CAP_CODEC_MJPEG);
>  if ('MediaSource' in window &&
>  MediaSource.isTypeSupported(SPICE_VP8_CODEC))
>  caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP8);
> +if ('MediaSource' in window &&
> MediaSource.isTypeSupported(SPICE_VP9_CODEC))
> +caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP9);
>  msg.channel_caps.push(caps);
>  }
>  
> diff --git a/webm.js b/webm.js
> index 789da14..c697135 100644
> --- a/webm.js
> +++ b/webm.js
> @@ -88,6 +88,7 @@ var EXPECTED_PACKET_DURATION= 10;
>  var GAP_DETECTION_THRESHOLD = 50;
>  
>  var SPICE_VP8_CODEC = 'video/webm; codecs="vp8"';
> +var SPICE_VP9_CODEC = 'video/webm; codecs="vp9"';
>  
>  
> /*
>  **  EBML utility functions
> @@ -467,7 +468,7 @@ webm_AudioTrackEntry.prototype =
>  },
>  }
>  
> -function webm_VideoTrackEntry(width, height)
> +function webm_VideoTrackEntry(width, height, codec_type)
>  {
>  this.id = WEBM_TRACK_ENTRY;
>  this.number = 1;
> @@ -482,8 +483,15 @@ function webm_VideoTrackEntry(width, height)
>  this.codec_decode_all = 0; // fixme - check
>  this.seek_pre_roll = 0; // 8000; // fixme - check
>  this.codec_delay =   8000; // Must match codec_private.preskip
> -this.codec_id = "V_VP8";
>  this.video = new webm_Video(width, height);
> +if (codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> +  

Re: [Spice-devel] [spice-server] channel: Remove unused red_channel_min_pipe_size

[Frediano Ziglio]

> 
> This could have been removed as part of 6e6126e024.
> 

Symmetric with max but I don't see the purpose, easy to
add again if needed.

Acked-by: Frediano Ziglio 

Frediano

> Signed-off-by: Christophe Fergeau 
> ---
>  server/red-channel.c | 13 -
>  server/red-channel.h |  2 --
>  2 files changed, 15 deletions(-)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 5d832c32a..97be2f2bd 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -616,19 +616,6 @@ uint32_t red_channel_max_pipe_size(RedChannel *channel)
>  return pipe_size;
>  }
>  
> -uint32_t red_channel_min_pipe_size(RedChannel *channel)
> -{
> -RedChannelClient *rcc;
> -uint32_t pipe_size = ~0;
> -
> -FOREACH_CLIENT(channel, rcc) {
> -uint32_t new_size;
> -new_size = red_channel_client_get_pipe_size(rcc);
> -pipe_size = MIN(pipe_size, new_size);
> -}
> -return pipe_size == ~0 ? 0 : pipe_size;
> -}
> -
>  uint32_t red_channel_sum_pipes_size(RedChannel *channel)
>  {
>  RedChannelClient *rcc;
> diff --git a/server/red-channel.h b/server/red-channel.h
> index e8feea2c9..55cfa7e1d 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -203,8 +203,6 @@ void red_channel_connect(RedChannel *channel, RedClient
> *client,
>  
>  /* return the sum of all the rcc pipe size */
>  uint32_t red_channel_max_pipe_size(RedChannel *channel);
> -/* return the min size of all the rcc pipe */
> -uint32_t red_channel_min_pipe_size(RedChannel *channel);
>  /* return the max size of all the rcc pipe */
>  uint32_t red_channel_sum_pipes_size(RedChannel *channel);
>  
> --
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 1/2] Introduce some macros to help declaring new GObject

[Christophe Fergeau]

On Thu, Sep 07, 2017 at 03:58:21AM -0400, Frediano Ziglio wrote:
> > 
> > On Wed, Sep 06, 2017 at 03:42:29PM +0100, Frediano Ziglio wrote:
> > > The macros will implement most of the boilerplate needed
> > > to declare an object.
> > > Their usage are similar to GLib G_DECLARE_*_TYPE macros.
> > 
> > Can we/should we use the GLib provided macros when they are available,
> > and copy/paste the GLib implementation in a -compat.h header for older
> > systems. The GLib macros were introduced in GLib 2.43.4
> > 
> > Christophe
> > 
> 
> That's fine. But our objects are currently neither final neither interfaces
> so we can't use any of them.

MainChannel, CursorChannel, ... could be considered final, RedChannel,
... could most likely use G_DECLARE_DERIVABLE_TYPE. But the GLib macros might
not exactly match what we are doing (naming, private data, ..).

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

Re: [Spice-devel] [PATCH spice-server 1/2] gl: fix client mouse mode

[Marc-André Lureau]

Hi

- Original Message -
> Since 2.8, QEMU now longer creates QXL primary surfaces when using GL.
> This change broke client-side mouse mode, because Spice server relies on
> primary surface conditions.
> 
> When GL is enabled, use GL scanout informations.
> Mouse mode is always client when GL surfaces are used.
> 
> This patch and most of the message are based on a patch from
> Marc-André Lureau, just moving responsibility from reds to RedQxl.

My patch was updating reds_update_client_mouse_allowed() which I think was a 
better place to do cursor business.

Furthermore, it didn't mess with QXL/2D state.

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-qxl.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index b556428d2..a003052ca 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -873,6 +873,13 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
>  /* FIXME: find a way to coallesce all pending SCANOUTs */
>  dispatcher_send_message(qxl_state->dispatcher,
>  RED_WORKER_MESSAGE_GL_SCANOUT, );
> +
> +qxl_state->x_res = width;
> +qxl_state->y_res = height;
> +qxl_state->use_hardware_cursor = TRUE;
> +qxl_state->primary_active = TRUE;

I didn't need to touch qxl-state. Not sure what would be the side effect of all 
this changes, and how they related to the problem.
 
> +
> +reds_update_client_mouse_allowed(qxl_state->reds);
>  }
>  
>  SPICE_GNUC_VISIBLE
> --
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 2/6] red-qxl: Move QXLInterface wrappers together

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Thu, Sep 07, 2017 at 12:40:22PM +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-qxl.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 54d642ad9..bec063c17 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -638,18 +638,6 @@ static void qxl_worker_loadvm_commands(QXLWorker 
> *qxl_worker,
>  red_qxl_loadvm_commands(qxl_state, ext, count);
>  }
>  
> -void red_qxl_attach_worker(QXLInstance *qxl)
> -{
> -QXLInterface *interface = qxl_get_interface(qxl);
> -interface->attache_worker(qxl, >st->qxl_worker);
> -}
> -
> -void red_qxl_set_compression_level(QXLInstance *qxl, int level)
> -{
> -QXLInterface *interface = qxl_get_interface(qxl);
> -interface->set_compression_level(qxl, level);
> -}
> -
>  uint32_t red_qxl_get_ram_size(QXLInstance *qxl)
>  {
>  QXLDevInitInfo qxl_info;
> @@ -1073,6 +1061,18 @@ RedsState* red_qxl_get_server(QXLState *qxl_state)
>  return qxl_state->reds;
>  }
>  
> +void red_qxl_attach_worker(QXLInstance *qxl)
> +{
> +QXLInterface *interface = qxl_get_interface(qxl);
> +interface->attache_worker(qxl, >st->qxl_worker);
> +}
> +
> +void red_qxl_set_compression_level(QXLInstance *qxl, int level)
> +{
> +QXLInterface *interface = qxl_get_interface(qxl);
> +interface->set_compression_level(qxl, level);
> +}
> +
>  void red_qxl_get_init_info(QXLInstance *qxl, QXLDevInitInfo *info)
>  {
>  QXLInterface *interface = qxl_get_interface(qxl);
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 3/6] spice-qxl: Add version information

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Thu, Sep 07, 2017 at 12:40:23PM +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/spice-qxl.h | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> index b8910bf47..04c657c58 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -197,12 +197,14 @@ struct QXLInterface {
>  void (*update_area_complete)(QXLInstance *qin, uint32_t surface_id,
>   struct QXLRect *updated_rects,
>   uint32_t num_updated_rects);
> +/* Available since version 3.2 */
>  void (*set_client_capabilities)(QXLInstance *qin,
>  uint8_t client_present,
>  uint8_t caps[SPICE_CAPABILITIES_SIZE]);
> -/* returns 1 if the interface is supported, 0 otherwise.
> +/* Returns 1 if the interface is supported, 0 otherwise.
>   * if monitors_config is NULL nothing is done except reporting the
> - * return code. */
> + * return code.
> + * Available since version 3.3 */
>  int (*client_monitors_config)(QXLInstance *qin,
>VDAgentMonitorsConfig *monitors_config);
>  };
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 5/6] dispatcher: Remove "opaque" property

[Jonathon Jongsma]

On Thu, 2017-09-07 at 16:50 +0200, Christophe Fergeau wrote:
> Acked-by: Christophe Fergeau 
> 
> On Thu, Sep 07, 2017 at 12:40:25PM +0100, Frediano Ziglio wrote:
> > Is supposed to be used during initialization but is never
> > used.
> > 

Hmm, I find this commit log to be a bit misleading. It implies that the
the Dispatcher no longer has an 'opaque' property. In reality, it still
has this property, it just is no longer a "GObject property" but is
only a simple struct member. 'opaque' is definitely used. It is passed
to all of the message handlers. It was never set in the constructor,
but later we call dispatcher_set_opaque() to set this value. 


> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/dispatcher.c | 22 ++
> >  server/dispatcher.h |  2 +-
> >  server/red-qxl.c|  2 +-
> >  3 files changed, 4 insertions(+), 22 deletions(-)
> > 
> > diff --git a/server/dispatcher.c b/server/dispatcher.c
> > index 6f2c4d85e..9e02d901b 100644
> > --- a/server/dispatcher.c
> > +++ b/server/dispatcher.c
> > @@ -66,8 +66,7 @@ struct DispatcherPrivate {
> >  
> >  enum {
> >  PROP_0,
> > -PROP_MAX_MESSAGE_TYPE,
> > -PROP_OPAQUE
> > +PROP_MAX_MESSAGE_TYPE
> >  };
> >  
> >  static void
> > @@ -83,9 +82,6 @@ dispatcher_get_property(GObject*object,
> >  case PROP_MAX_MESSAGE_TYPE:
> >  g_value_set_uint(value, self->priv->max_message_type);
> >  break;
> > -case PROP_OPAQUE:
> > -g_value_set_pointer(value, self->priv->opaque);
> > -break;
> >  default:
> >  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > pspec);
> >  }
> > @@ -104,9 +100,6 @@ dispatcher_set_property(GObject  *object,
> >  case PROP_MAX_MESSAGE_TYPE:
> >  self->priv->max_message_type =
> > g_value_get_uint(value);
> >  break;
> > -case PROP_OPAQUE:
> > -dispatcher_set_opaque(self,
> > g_value_get_pointer(value));
> > -break;
> >  default:
> >  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > pspec);
> >  }
> > @@ -168,15 +161,6 @@ dispatcher_class_init(DispatcherClass *klass)
> >G_PARAM_STAT
> > IC_STRINGS |
> >G_PARAM_READ
> > WRITE |
> >G_PARAM_CONS
> > TRUCT_ONLY));
> > -g_object_class_install_property(object_class,
> > -PROP_OPAQUE,
> > -g_param_spec_pointer("opaque",
> > - "opaque",
> > - "User
> > data to pass to callbacks",
> > - G_PARAM_S
> > TATIC_STRINGS |
> > - G_PARAM_R
> > EADWRITE |
> > - G_PARAM_C
> > ONSTRUCT));
> > -
> >  }
> >  
> >  static void
> > @@ -186,11 +170,10 @@ dispatcher_init(Dispatcher *self)
> >  }
> >  
> >  Dispatcher *
> > -dispatcher_new(size_t max_message_type, void *opaque)
> > +dispatcher_new(size_t max_message_type)
> >  {
> >  return g_object_new(TYPE_DISPATCHER,
> >  "max-message-type", (guint)
> > max_message_type,
> > -"opaque", opaque,
> >  NULL);
> >  }
> >  
> > @@ -419,7 +402,6 @@ static void setup_dummy_signal_handler(void)
> >  void dispatcher_set_opaque(Dispatcher *self, void *opaque)
> >  {
> >  self->priv->opaque = opaque;
> > -g_object_notify(G_OBJECT(self), "opaque");
> >  }
> >  
> >  int dispatcher_get_recv_fd(Dispatcher *dispatcher)
> > diff --git a/server/dispatcher.h b/server/dispatcher.h
> > index 97b01de9c..00a828bdb 100644
> > --- a/server/dispatcher.h
> > +++ b/server/dispatcher.h
> > @@ -49,7 +49,7 @@ struct DispatcherClass
> >  
> >  GType dispatcher_get_type(void) G_GNUC_CONST;
> >  
> > -Dispatcher *dispatcher_new(size_t max_message_type, void *opaque);
> > +Dispatcher *dispatcher_new(size_t max_message_type);
> >  
> >  
> >  typedef void (*dispatcher_handle_message)(void *opaque,
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index b1804cf17..d876dc079 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -939,7 +939,7 @@ void red_qxl_init(RedsState *reds, QXLInstance
> > *qxl)
> >  qxl_state->qxl = qxl;
> >  pthread_mutex_init(_state->scanout_mutex, NULL);
> >  qxl_state->scanout.drm_dma_buf_fd = -1;
> > -qxl_state->dispatcher =
> > dispatcher_new(RED_WORKER_MESSAGE_COUNT, NULL);
> > +qxl_state->dispatcher =
> > dispatcher_new(RED_WORKER_MESSAGE_COUNT);
> >  qxl_state->qxl_worker.major_version =
> > SPICE_INTERFACE_QXL_MAJOR;
> >  

Re: [Spice-devel] [PATCH spice-server v2 3/3] Fix crash attempting to connect duplicate channels

[Jonathon Jongsma]

On Thu, 2017-09-07 at 03:50 -0400, Frediano Ziglio wrote:
> > 
> > On Wed, 2017-08-30 at 10:36 +0100, Frediano Ziglio wrote:
> > > You could easily trigger this issue using multiple monitors and
> > > a modified spice-gtk client with this patch:
> > > 
> > > --- a/src/channel-main.c
> > > +++ b/src/channel-main.c
> > > @@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t
> > > *c)
> > >  {
> > >  g_return_val_if_fail(c != NULL, FALSE);
> > > 
> > > +if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
> > >  spice_channel_new(c->session, c->type, c->id);
> > > 
> > >  g_object_unref(c->session);
> > > 
> > > 
> > > which cause a crash like
> > > 
> > > (process:28742): Spice-WARNING **: Failed to create channel
> > > client:
> > > Client 0x40246f5d0: duplicate channel type 2 id 0
> > > 2017-08-24 09:36:57.451+: shutting down, reason=crashed
> > 
> > It took me quite a while to figure out how this crash is related to
> > the
> > patch below. Perhaps it needs additional detail in the commit log?
> > As
> > far as I can tell, it's like this:
> > 
> > RedChannelClient is an GInitable type, which means that the object
> > is
> > constructed, and then the _init() function is called, which can
> > fail.
> > If the _init() fails, the newly-created object will be destroyed.
> > As
> > part of _init(), we add a new watch for the stream using the core
> > interface that is associated with the channel. After adding the
> > watch,
> > our rcc creation fails (due to duplicate ID), and the rcc object is
> > unreffed. This results in a call to reds_stream_free() (since the
> > rcc
> > now owns the stream). But in reds_stream_free, we were trying to
> > remove
> > the watch from the core interface associated with the RedsState.
> > For
> > most channels, these two core interfaces are equivalent. But for
> > the
> > Display and Cursor channels, it is the core Glib-based interface
> > associated with the RedWorker.
> > 
> 
> You already know where this paragraph will end, right ?
> Yes!

fine with me ;)

> 
> > Do I understand it correctly?
> > 
> > > 
> > > The watch in RedsStream by default is bound to the Qemu
> > > provided SpiceCoreInterface while RedChannelClient bound it
> > > to Glib one causing the crash when the watch is deleted from
> > > RedsStream. Change the bound interface.
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/red-channel-client.c | 14 +-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/server/red-channel-client.c b/server/red-channel-
> > > client.c
> > > index 145ba43f..acc2b4eb 100644
> > > --- a/server/red-channel-client.c
> > > +++ b/server/red-channel-client.c
> > > @@ -339,6 +339,16 @@ red_channel_client_finalize(GObject *object)
> > >  {
> > >  RedChannelClient *self = RED_CHANNEL_CLIENT(object);
> > >  
> > > +SpiceCoreInterfaceInternal *core =
> > > red_channel_get_core_interface(self->priv->channel);
> > > +if (self->priv->latency_monitor.timer) {
> > > +core->timer_remove(core, self->priv-
> > > >latency_monitor.timer);
> > > +self->priv->latency_monitor.timer = NULL;
> > > +}
> > > +if (self->priv->connectivity_monitor.timer) {
> > > +core->timer_remove(core, self->priv-
> > > > connectivity_monitor.timer);
> > > 
> > > +self->priv->connectivity_monitor.timer = NULL;
> > > +}
> > > +
> > 
> > I'm a little bit confused here. Is this part related at all to the
> > crash in the commit message? It doesn't seem to be.
> > 
> 
> Yes, in case monitoring is enabled (currently DCC) the monitoring
> timer is registered causing a use-after-free too.
> Not both timers are needed but I think that a finalize is more
> safer and robust if it handles any possible status.
> 
> > >  reds_stream_free(self->priv->stream);
> > >  self->priv->stream = NULL;
> > >  
> > > @@ -921,12 +931,14 @@ static gboolean
> > > red_channel_client_initable_init(GInitable *initable,
> > >  }
> > >  
> > >  core = red_channel_get_core_interface(self->priv->channel);
> > > -if (self->priv->stream)
> > > +if (self->priv->stream) {
> > > +reds_stream_set_core_interface(self->priv->stream,
> > > core);
> > >  self->priv->stream->watch =
> > >  core->watch_add(core, self->priv->stream->socket,
> > >  SPICE_WATCH_EVENT_READ,
> > >  red_channel_client_event,
> > >  self);
> > > +}
> > 
> > It seems a little bit weird that the RedChannelClient is poking
> > into
> > the stream structure to set stream->watch...
> > 
> 
> Not a regression of this patch. I found safer making sure that
> RedsStream can release the watch safely as the code already
> automatically do it. The previous patch instead added a "watch"
> in RCC but was more intrusive.

Yes, this comment was not about your patch, just a general comment
about the code. I 

Re: [Spice-devel] [PATCH spice-server 1/6] red-qxl: Unify red_qxl_use_client_monitors_config and red_qxl_client_monitors_config

[Christophe Fergeau]

On Thu, Sep 07, 2017 at 12:40:21PM +0100, Frediano Ziglio wrote:
> These 2 functions were doing the same stuff, calling
> client_monitors_config callback in QXLInterface.
> The only difference was that red_qxl_use_client_monitors_config
> used a NULL value.
> Added the check for proper version, QXLInstance before 3.3
> did not have this callback.

Having separate methods to check if client monitors config is supported,
and to do the actual call to the appropriate callback makes sense to me
(ie current code does not seem too bad)
However, in this case the only caller of
red_qxl_use_client_monitors_config is reds_use_client_monitors_config,
so  this is not going to make a big difference.

Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-qxl.c | 10 ++
>  server/red-qxl.h |  1 -
>  server/reds.c|  2 +-
>  3 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index b556428d2..54d642ad9 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -195,17 +195,11 @@ static void red_qxl_update_area(QXLState *qxl_state, 
> uint32_t surface_id,
>  );
>  }
>  
> -gboolean red_qxl_use_client_monitors_config(QXLInstance *qxl)
> -{
> -return (red_qxl_check_qxl_version(qxl, 3, 3) &&
> -qxl_get_interface(qxl)->client_monitors_config &&
> -qxl_get_interface(qxl)->client_monitors_config(qxl, NULL));
> -}
> -
>  gboolean red_qxl_client_monitors_config(QXLInstance *qxl,
>  VDAgentMonitorsConfig 
> *monitors_config)
>  {
> -return (qxl_get_interface(qxl)->client_monitors_config &&
> +return (red_qxl_check_qxl_version(qxl, 3, 3) &&
> +qxl_get_interface(qxl)->client_monitors_config &&
>  qxl_get_interface(qxl)->client_monitors_config(qxl, 
> monitors_config));
>  }
>  
> diff --git a/server/red-qxl.h b/server/red-qxl.h
> index f925f065b..5d57780a9 100644
> --- a/server/red-qxl.h
> +++ b/server/red-qxl.h
> @@ -37,7 +37,6 @@ void red_qxl_start(QXLInstance *qxl);
>  uint32_t red_qxl_get_ram_size(QXLInstance *qxl);
>  void red_qxl_async_complete(QXLInstance *qxl, AsyncCommand *async_command);
>  struct Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl);
> -gboolean red_qxl_use_client_monitors_config(QXLInstance *qxl);
>  gboolean red_qxl_client_monitors_config(QXLInstance *qxl, 
> VDAgentMonitorsConfig *monitors_config);
>  gboolean red_qxl_get_primary_active(QXLInstance *qxl);
>  gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res, gint 
> *y_res);
> diff --git a/server/reds.c b/server/reds.c
> index b6ecc6c69..24ec2bdde 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -4401,7 +4401,7 @@ static gboolean 
> reds_use_client_monitors_config(RedsState *reds)
>  }
>  
>  FOREACH_QXL_INSTANCE(reds, qxl) {
> -if (!red_qxl_use_client_monitors_config(qxl))
> +if (!red_qxl_client_monitors_config(qxl, NULL))
>  return FALSE;
>  }
>  return TRUE;
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-server v2] gl: fix client mouse mode

[Frediano Ziglio]

Since 2.8, QEMU now longer creates QXL primary surfaces when using GL.
This change broke client-side mouse mode, because Spice server relies on
primary surface conditions.

When GL is enabled, use GL scanout informations.
Mouse mode is always client when GL surfaces are used.

This patch and most of the message are based on a patch from
Marc-André Lureau, just moving responsibility from reds to RedQxl.

Signed-off-by: Frediano Ziglio 
---
 server/red-qxl.c | 31 +--
 server/red-qxl.h |  3 +--
 server/reds.c|  3 +--
 3 files changed, 23 insertions(+), 14 deletions(-)

Changes since v1:
- do not change qxl_state resolution.

diff --git a/server/red-qxl.c b/server/red-qxl.c
index b556428d2..93e7eb7b8 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -903,6 +903,8 @@ void spice_qxl_gl_draw_async(QXLInstance *qxl,
 
 qxl_state->gl_draw_async = async_command_alloc(qxl_state, message, cookie);
 dispatcher_send_message(qxl_state->dispatcher, message, );
+
+reds_update_client_mouse_allowed(qxl_state->reds);
 }
 
 void red_qxl_async_complete(QXLInstance *qxl, AsyncCommand *async_command)
@@ -1022,20 +1024,29 @@ void red_qxl_clear_pending(QXLState *qxl_state, int 
pending)
 clear_bit(pending, _state->pending);
 }
 
-gboolean red_qxl_get_primary_active(QXLInstance *qxl)
+bool red_qxl_get_allow_client_mouse(QXLInstance *qxl, int *x_res, int *y_res, 
int *allow_now)
 {
-return qxl->st->primary_active;
-}
+// try to get resolution from 3D
+SpiceMsgDisplayGlScanoutUnix *gl;
+if ((gl = red_qxl_get_gl_scanout(qxl))) {
+*x_res = gl->width;
+*y_res = gl->height;
+*allow_now = TRUE;
+red_qxl_put_gl_scanout(qxl, gl);
+return true;
+}
+
+// check for 2D
+if (!qxl->st->primary_active) {
+return false;
+}
 
-gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res, gint 
*y_res)
-{
 if (qxl->st->use_hardware_cursor) {
-if (x_res)
-*x_res = qxl->st->x_res;
-if (y_res)
-*y_res = qxl->st->y_res;
+*x_res = qxl->st->x_res;
+*y_res = qxl->st->y_res;
 }
-return qxl->st->use_hardware_cursor;
+*allow_now = qxl->st->use_hardware_cursor;
+return true;
 }
 
 void red_qxl_on_ic_change(QXLInstance *qxl, SpiceImageCompression ic)
diff --git a/server/red-qxl.h b/server/red-qxl.h
index f925f065b..503ba223d 100644
--- a/server/red-qxl.h
+++ b/server/red-qxl.h
@@ -39,8 +39,7 @@ void red_qxl_async_complete(QXLInstance *qxl, AsyncCommand 
*async_command);
 struct Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl);
 gboolean red_qxl_use_client_monitors_config(QXLInstance *qxl);
 gboolean red_qxl_client_monitors_config(QXLInstance *qxl, 
VDAgentMonitorsConfig *monitors_config);
-gboolean red_qxl_get_primary_active(QXLInstance *qxl);
-gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res, gint 
*y_res);
+bool red_qxl_get_allow_client_mouse(QXLInstance *qxl, int *x_res, int *y_res, 
int *allow_now);
 SpiceMsgDisplayGlScanoutUnix *red_qxl_get_gl_scanout(QXLInstance *qxl);
 void red_qxl_put_gl_scanout(QXLInstance *qxl, SpiceMsgDisplayGlScanoutUnix 
*scanout);
 void red_qxl_gl_draw_async_complete(QXLInstance *qxl);
diff --git a/server/reds.c b/server/reds.c
index b6ecc6c69..2f4f12ab9 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -4374,8 +4374,7 @@ void reds_update_client_mouse_allowed(RedsState *reds)
 
 allow_now = TRUE;
 FOREACH_QXL_INSTANCE(reds, qxl) {
-if (red_qxl_get_primary_active(qxl)) {
-allow_now = red_qxl_get_allow_client_mouse(qxl, _res, 
_res);
+if (red_qxl_get_allow_client_mouse(qxl, _res, _res, 
_now)) {
 break;
 }
 }
-- 
2.13.5

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

Re: [Spice-devel] [PATCH spice-server 5/6] dispatcher: Remove "opaque" property

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Thu, Sep 07, 2017 at 12:40:25PM +0100, Frediano Ziglio wrote:
> Is supposed to be used during initialization but is never
> used.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/dispatcher.c | 22 ++
>  server/dispatcher.h |  2 +-
>  server/red-qxl.c|  2 +-
>  3 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index 6f2c4d85e..9e02d901b 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -66,8 +66,7 @@ struct DispatcherPrivate {
>  
>  enum {
>  PROP_0,
> -PROP_MAX_MESSAGE_TYPE,
> -PROP_OPAQUE
> +PROP_MAX_MESSAGE_TYPE
>  };
>  
>  static void
> @@ -83,9 +82,6 @@ dispatcher_get_property(GObject*object,
>  case PROP_MAX_MESSAGE_TYPE:
>  g_value_set_uint(value, self->priv->max_message_type);
>  break;
> -case PROP_OPAQUE:
> -g_value_set_pointer(value, self->priv->opaque);
> -break;
>  default:
>  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
>  }
> @@ -104,9 +100,6 @@ dispatcher_set_property(GObject  *object,
>  case PROP_MAX_MESSAGE_TYPE:
>  self->priv->max_message_type = g_value_get_uint(value);
>  break;
> -case PROP_OPAQUE:
> -dispatcher_set_opaque(self, g_value_get_pointer(value));
> -break;
>  default:
>  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
>  }
> @@ -168,15 +161,6 @@ dispatcher_class_init(DispatcherClass *klass)
>G_PARAM_STATIC_STRINGS 
> |
>G_PARAM_READWRITE |
>
> G_PARAM_CONSTRUCT_ONLY));
> -g_object_class_install_property(object_class,
> -PROP_OPAQUE,
> -g_param_spec_pointer("opaque",
> - "opaque",
> - "User data to pass 
> to callbacks",
> - 
> G_PARAM_STATIC_STRINGS |
> - G_PARAM_READWRITE |
> - G_PARAM_CONSTRUCT));
> -
>  }
>  
>  static void
> @@ -186,11 +170,10 @@ dispatcher_init(Dispatcher *self)
>  }
>  
>  Dispatcher *
> -dispatcher_new(size_t max_message_type, void *opaque)
> +dispatcher_new(size_t max_message_type)
>  {
>  return g_object_new(TYPE_DISPATCHER,
>  "max-message-type", (guint) max_message_type,
> -"opaque", opaque,
>  NULL);
>  }
>  
> @@ -419,7 +402,6 @@ static void setup_dummy_signal_handler(void)
>  void dispatcher_set_opaque(Dispatcher *self, void *opaque)
>  {
>  self->priv->opaque = opaque;
> -g_object_notify(G_OBJECT(self), "opaque");
>  }
>  
>  int dispatcher_get_recv_fd(Dispatcher *dispatcher)
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index 97b01de9c..00a828bdb 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -49,7 +49,7 @@ struct DispatcherClass
>  
>  GType dispatcher_get_type(void) G_GNUC_CONST;
>  
> -Dispatcher *dispatcher_new(size_t max_message_type, void *opaque);
> +Dispatcher *dispatcher_new(size_t max_message_type);
>  
>  
>  typedef void (*dispatcher_handle_message)(void *opaque,
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index b1804cf17..d876dc079 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -939,7 +939,7 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl)
>  qxl_state->qxl = qxl;
>  pthread_mutex_init(_state->scanout_mutex, NULL);
>  qxl_state->scanout.drm_dma_buf_fd = -1;
> -qxl_state->dispatcher = dispatcher_new(RED_WORKER_MESSAGE_COUNT, NULL);
> +qxl_state->dispatcher = dispatcher_new(RED_WORKER_MESSAGE_COUNT);
>  qxl_state->qxl_worker.major_version = SPICE_INTERFACE_QXL_MAJOR;
>  qxl_state->qxl_worker.minor_version = SPICE_INTERFACE_QXL_MINOR;
>  qxl_state->qxl_worker.wakeup = qxl_worker_wakeup;
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 6/6] main-dispatcher: Avoid type conversion in dispatcher_handle_read

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Thu, Sep 07, 2017 at 12:40:26PM +0100, Frediano Ziglio wrote:
> Pass proper type to callback to avoid having to convert to
> the right type for each call.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> This make dispatcher_handle_read identical to handle_dev_input.
> ---
>  server/main-dispatcher.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> index 019bf4d3f..71f8f65d9 100644
> --- a/server/main-dispatcher.c
> +++ b/server/main-dispatcher.c
> @@ -280,9 +280,9 @@ void main_dispatcher_client_disconnect(MainDispatcher 
> *self, RedClient *client)
>  
>  static void dispatcher_handle_read(int fd, int event, void *opaque)
>  {
> -MainDispatcher *self = opaque;
> +Dispatcher *dispatcher = opaque;
>  
> -dispatcher_handle_recv_read(DISPATCHER(self));
> +dispatcher_handle_recv_read(dispatcher);
>  }
>  
>  /*
> @@ -311,7 +311,7 @@ void main_dispatcher_constructed(GObject *object)
>  self->priv->core->watch_add(self->priv->core,
>  dispatcher_get_recv_fd(DISPATCHER(self)),
>  SPICE_WATCH_EVENT_READ, 
> dispatcher_handle_read,
> -self);
> +DISPATCHER(self));
>  dispatcher_register_handler(DISPATCHER(self), 
> MAIN_DISPATCHER_CHANNEL_EVENT,
>  main_dispatcher_handle_channel_event,
>  sizeof(MainDispatcherChannelEventMessage), 0 
> /* no ack */);
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [RFC PATCH spice-server v3 10/20] stream-channel: Allows to register callback to get new stream request

[Christophe Fergeau]

On Fri, Aug 25, 2017 at 12:22:41PM -0400, Frediano Ziglio wrote:
> > 
> > On Fri, 2017-08-25 at 05:39 -0400, Frediano Ziglio wrote:
> > > > 
> > > > I had a similar comment in a different patch during the last
> > > > version of
> > > > the series, but I personally would prefer a signal to handle this
> > > > situation. For example, StreamChannel could have a "supported-
> > > > codecs"
> > > > property, Then when a new client connected, or a client
> > > > disconnected,
> > > > it would update this property. The StreamDevice would connect to
> > > > the
> > > > "notify::supported-codecs" signal, and then it would start and stop
> > > > the
> > > > stream as necessary. I think that encapsulates the logic of
> > > > starting
> > > > and stopping the stream a little bit better within the device class
> > > > as
> > > > well.
> > > > 
> > > > After writing the above paragraph, I did a little more
> > > > investigation
> > > > and I noticed that DisplayChannel already has a similar design --
> > > > it
> > > > has a "video-codecs" property.
> > > > 
> > > > Reviewed-by: Jonathon Jongsma 
> > > > 
> > > 
> > > As long as I can have type safety, static binding and
> > > I don't have to write too much I'm not against properties
> > > and signal.
> > > 
> > > As far as I know this is not possible with GObject,
> > > at least not in the current C implementation.
> > > 
> > > Frediano
> > 
> > Yes, there is a tradeoff between type-safety and convenience when
> > you're using C. But we already have plenty of signals and properties in
> > other parts of the code. Are you suggesting that we should not have any
> > of these in the code because they're not fully type-safe? And if not:
> > why should we treat this class differently and refuse to use them here
> > when we already use them (for good reasons) elsewhere?
> > 
> 
> Callbacks are managed in spice-sever in lot of different way
> - normal functions + data (like this);
> - passing list of functions;
> - overriding methods in GObjects;
> - signals (very few!).
> 
> We limit the usage of property to the constructor case with few
> exceptions and signals are limited to "sin" and "video-codecs".

This limited usage is mainly because we did not get to it yet :)
So far a lot of the focus was on splitting the code in smaller/more
manageable classes, for that we chose to use GObject, and to use
GObject, we had to change some of the code to use properties.
The rest of the code did not change, as the focus really was on
introducing a meaningful split in objects.
But every time I see a foo_on_frobnicate() method, my reaction is always
"I really should look if it makes sense to replace this with signals"

Yes they could have more type safety, yes they could be faster. I don't
expect we'll have tons of signals in the code, and we know we have to be
careful when we see one, so I don't expect a lot of issues with them.

We really should be using signals instead of our homegrown solution when
appropriate. Not a perfect tool, but the one which is used in our
codebase.

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

Re: [Spice-devel] [PATCH spice-server v4] Add documentation for Dispatcher

[Frediano Ziglio]

> 
> ---
> Changes since v3:
>  - removed documentation for 'opaque' argument from dispatcher_new()
> 
>  server/dispatcher.h | 95
>  -
>  1 file changed, 80 insertions(+), 15 deletions(-)
> 
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index ab7713845..1aaba13bc 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -35,6 +35,18 @@ typedef struct Dispatcher Dispatcher;
>  typedef struct DispatcherClass DispatcherClass;
>  typedef struct DispatcherPrivate DispatcherPrivate;
>  
> +/* A Dispatcher provides inter-thread communication by serializing messages.
> + * Currently the Dispatcher uses a unix socket (socketpair) for dispatching
> the
> + * messages.
> + *
> + * Message types are identified by a unique integer value and must first be
> + * registered with the class (see dispatcher_register_handler()) before they
> + * can be sent. Sending threads can send a message using the
> + * dispatcher_send_message() function. The receiving thread can monitor the
> + * dispatcher's 'receive' file descriptor (see dispatcher_get_recv_fd()) for
> + * activity and should call dispatcher_handle_recv_read() to process
> incoming
> + * messages.
> + */
>  struct Dispatcher
>  {
>  GObject parent;
> @@ -49,26 +61,52 @@ struct DispatcherClass
>  
>  GType dispatcher_get_type(void) G_GNUC_CONST;
>  
> +/* dispatcher_new
> + *
> + * Create a new Dispatcher object
> + *
> + * @max_message_type:   indicates the number of unique message types that
> can
> + *  be handled by this dispatcher. Each message type is
> + *  identified by an integer value between 0 and
> + *  max_message_type-1.
> + */
>  Dispatcher *dispatcher_new(size_t max_message_type);
>  
>  
> +/* The function signature for handlers of a specific message type */
>  typedef void (*dispatcher_handle_message)(void *opaque,
>void *payload);
>  
> +/* The signature for a function that handles all messages (see
> + * dispatcher_register_universal_handler()) */
>  typedef void (*dispatcher_handle_any_message)(void *opaque,
>uint32_t message_type,
>void *payload);
>  
> -/*
> - * dispatcher_send_message
> +/* dispatcher_send_message
> + *
> + * Sends a message to the receiving thread. The message type must have been
> + * registered first (see dispatcher_register_handler()).  @payload must be a
> + * buffer of the same size as the size registered for @message_type
> + *
> + * If the sent message is a message type requires an ACK, this function will
> + * block until it receives an ACK from the receiving thread.
> + *
>   * @message_type: message type
>   * @payload:  payload
>   */
>  void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
>   void *payload);
>  
> -/*
> - * dispatcher_register_handler
> +/* dispatcher_register_handler
> + *
> + * This function registers a message type with the dispatcher, and registers
> + * @handler as the function that will handle incoming messages of this type.
> + * If @ack is true, the dispatcher will also send an ACK in response to the
> + * message after the message has been passed to the handler. You can only
> + * register a given message type once. For example, you cannot register two
> + * different handlers for the same message type with different @ack values.
> + *
>   * @dispatcher: dispatcher
>   * @messsage_type:  message type
>   * @handler:message handler
> @@ -79,32 +117,59 @@ void dispatcher_register_handler(Dispatcher *dispatcher,
> uint32_t message_type,
>   dispatcher_handle_message handler, size_t
>   size,
>   bool ack);
>  
> -/*
> - * Hack to allow red_record to see the message being sent so it can record
> - * it to file.
> +/* dispatcher_register_universal_handler
> + *
> + * Register a universal handler that will be called when *any* message is
> + * received by the dispatcher. When a message is received, this handler will
> be
> + * called first. If the received message type was registered via
> + * dispatcher_register_handler(), the message-specific handler will then be
> + * called. Only one universal handler can be registered. This feature can be
> + * used to record all messages to a file for replay and debugging.
> + *
> + * @dispatcher: dispatcher
> + * @handler:a handler function
>   */
>  void dispatcher_register_universal_handler(Dispatcher *dispatcher,
>  dispatcher_handle_any_message handler);
>  
> -/*
> - *  dispatcher_handle_recv_read
> - *  @dispatcher: Dispatcher instance
> +/* dispatcher_handle_recv_read
> + *
> + * A convenience function that is intended to be called by the receiving
> thread
> + * to 

[Spice-devel] [PATCH spice-server v3 3/3] Add documentation for Dispatcher

[Jonathon Jongsma]

---
Changes since v2:
 - rebased on master
 - moved handle_async_done removal hunk to first patch

 server/dispatcher.h | 97 -
 1 file changed, 82 insertions(+), 15 deletions(-)

diff --git a/server/dispatcher.h b/server/dispatcher.h
index ab7713845..72f81c1f1 100644
--- a/server/dispatcher.h
+++ b/server/dispatcher.h
@@ -35,6 +35,18 @@ typedef struct Dispatcher Dispatcher;
 typedef struct DispatcherClass DispatcherClass;
 typedef struct DispatcherPrivate DispatcherPrivate;
 
+/* A Dispatcher provides inter-thread communication by serializing messages.
+ * Currently the Dispatcher uses a unix socket (socketpair) for dispatching the
+ * messages.
+ *
+ * Message types are identified by a unique integer value and must first be
+ * registered with the class (see dispatcher_register_handler()) before they
+ * can be sent. Sending threads can send a message using the
+ * dispatcher_send_message() function. The receiving thread can monitor the
+ * dispatcher's 'receive' file descriptor (see dispatcher_get_recv_fd()) for
+ * activity and should call dispatcher_handle_recv_read() to process incoming
+ * messages.
+ */
 struct Dispatcher
 {
 GObject parent;
@@ -49,26 +61,54 @@ struct DispatcherClass
 
 GType dispatcher_get_type(void) G_GNUC_CONST;
 
+/* dispatcher_new
+ *
+ * Create a new Dispatcher object
+ *
+ * @max_message_type:   indicates the number of unique message types that can
+ *  be handled by this dispatcher. Each message type is
+ *  identified by an integer value between 0 and
+ *  max_message_type-1.
+ * @opaque: an arbitrary pointer that will be passed as the first
+ *  argument to any registered handler functions
+ */
 Dispatcher *dispatcher_new(size_t max_message_type);
 
 
+/* The function signature for handlers of a specific message type */
 typedef void (*dispatcher_handle_message)(void *opaque,
   void *payload);
 
+/* The signature for a function that handles all messages (see
+ * dispatcher_register_universal_handler()) */
 typedef void (*dispatcher_handle_any_message)(void *opaque,
   uint32_t message_type,
   void *payload);
 
-/*
- * dispatcher_send_message
+/* dispatcher_send_message
+ *
+ * Sends a message to the receiving thread. The message type must have been
+ * registered first (see dispatcher_register_handler()).  @payload must be a
+ * buffer of the same size as the size registered for @message_type
+ *
+ * If the sent message is a message type requires an ACK, this function will
+ * block until it receives an ACK from the receiving thread.
+ *
  * @message_type: message type
  * @payload:  payload
  */
 void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
  void *payload);
 
-/*
- * dispatcher_register_handler
+/* dispatcher_register_handler
+ *
+ * This function registers a message type with the dispatcher, and registers
+ * @handler as the function that will handle incoming messages of this type.
+ * If @ack is true, the dispatcher will also send an ACK in response to the
+ * message after the message has been passed to the handler. You can only
+ * register a given message type once. For example, you cannot register two
+ * different handlers for the same message type with different @ack values.
+ *
  * @dispatcher: dispatcher
  * @messsage_type:  message type
  * @handler:message handler
@@ -79,32 +119,59 @@ void dispatcher_register_handler(Dispatcher *dispatcher, 
uint32_t message_type,
  dispatcher_handle_message handler, size_t 
size,
  bool ack);
 
-/*
- * Hack to allow red_record to see the message being sent so it can record
- * it to file.
+/* dispatcher_register_universal_handler
+ *
+ * Register a universal handler that will be called when *any* message is
+ * received by the dispatcher. When a message is received, this handler will be
+ * called first. If the received message type was registered via
+ * dispatcher_register_handler(), the message-specific handler will then be
+ * called. Only one universal handler can be registered. This feature can be
+ * used to record all messages to a file for replay and debugging.
+ *
+ * @dispatcher: dispatcher
+ * @handler:a handler function
  */
 void dispatcher_register_universal_handler(Dispatcher *dispatcher,
 dispatcher_handle_any_message handler);
 
-/*
- *  dispatcher_handle_recv_read
- *  @dispatcher: Dispatcher instance
+/* dispatcher_handle_recv_read
+ *
+ * A convenience function that is intended to be called by the receiving thread
+ * to handle all incoming messages and execute any handlers for those messages.
+ * This function will handle all incoming messages 

[Spice-devel] [PATCH spice-server v3 1/3] Dispatcher: remove async_done callback

[Jonathon Jongsma]

This callback was only executed for message types that were registered
with DISPATCHER_ASYNC ack type. However, the async_done handler was
called immediately after the message-specific handler and was called in
the same thread, so the async_done stuff can just as easily be done from
within the message-specific handler. This allows to simplify the
dispatcher_register_handler() method to simply require a boolean
argument for whether the message type requires an ACK or not.

Signed-off-by: Jonathon Jongsma 
---
Changes since v2:
 - rebased on master
 - moved handle_async_done removal hunk from documentation patch
 - fixed handle_dev_monitors_config_async so that it doesn't return
   early without calling red_qxl_async_complete()

 server/dispatcher.c |  19 ++---
 server/dispatcher.h |  30 +--
 server/red-worker.c | 108 +---
 3 files changed, 58 insertions(+), 99 deletions(-)

diff --git a/server/dispatcher.c b/server/dispatcher.c
index 9e02d901b..7fb706f8b 100644
--- a/server/dispatcher.c
+++ b/server/dispatcher.c
@@ -40,7 +40,7 @@ static void setup_dummy_signal_handler(void);
 
 typedef struct DispatcherMessage {
 size_t size;
-int ack;
+bool ack;
 dispatcher_handle_message handler;
 } DispatcherMessage;
 
@@ -60,7 +60,6 @@ struct DispatcherPrivate {
 void *payload; /* allocated as max of message sizes */
 size_t payload_size; /* used to track realloc calls */
 void *opaque;
-dispatcher_handle_async_done handle_async_done;
 dispatcher_handle_any_message any_handler;
 };
 
@@ -286,14 +285,12 @@ static int dispatcher_handle_single_read(Dispatcher 
*dispatcher)
 } else {
 spice_printerr("error: no handler for message type %d", type);
 }
-if (msg->ack == DISPATCHER_ACK) {
+if (msg->ack) {
 if (write_safe(dispatcher->priv->recv_fd,
(uint8_t*), sizeof(ack)) == -1) {
 spice_printerr("error writing ack for message %d", type);
 /* TODO: close socketpair? */
 }
-} else if (msg->ack == DISPATCHER_ASYNC && 
dispatcher->priv->handle_async_done) {
-dispatcher->priv->handle_async_done(dispatcher->priv->opaque, type, 
payload);
 }
 return 1;
 }
@@ -329,7 +326,7 @@ void dispatcher_send_message(Dispatcher *dispatcher, 
uint32_t message_type,
message_type);
 goto unlock;
 }
-if (msg->ack == DISPATCHER_ACK) {
+if (msg->ack) {
 if (read_safe(send_fd, (uint8_t*), sizeof(ack), 1) == -1) {
 spice_printerr("error: failed to read ack");
 } else if (ack != ACK) {
@@ -342,17 +339,9 @@ unlock:
 pthread_mutex_unlock(>priv->lock);
 }
 
-void dispatcher_register_async_done_callback(
-Dispatcher *dispatcher,
-dispatcher_handle_async_done handler)
-{
-assert(dispatcher->priv->handle_async_done == NULL);
-dispatcher->priv->handle_async_done = handler;
-}
-
 void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t message_type,
  dispatcher_handle_message handler,
- size_t size, int ack)
+ size_t size, bool ack)
 {
 DispatcherMessage *msg;
 
diff --git a/server/dispatcher.h b/server/dispatcher.h
index 00a828bdb..ab7713845 100644
--- a/server/dispatcher.h
+++ b/server/dispatcher.h
@@ -59,11 +59,6 @@ typedef void (*dispatcher_handle_any_message)(void *opaque,
   uint32_t message_type,
   void *payload);
 
-typedef void (*dispatcher_handle_async_done)(void *opaque,
- uint32_t message_type,
- void *payload);
-
-
 /*
  * dispatcher_send_message
  * @message_type: message type
@@ -72,38 +67,17 @@ typedef void (*dispatcher_handle_async_done)(void *opaque,
 void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
  void *payload);
 
-enum {
-DISPATCHER_NONE = 0,
-DISPATCHER_ACK,
-DISPATCHER_ASYNC
-};
-
 /*
  * dispatcher_register_handler
  * @dispatcher: dispatcher
  * @messsage_type:  message type
  * @handler:message handler
  * @size:   message size. Each type has a fixed associated size.
- * @ack:One of DISPATCHER_NONE, DISPATCHER_ACK, DISPATCHER_ASYNC.
- *  DISPATCHER_NONE - only send the message
- *  DISPATCHER_ACK - send an ack after the message
- *  DISPATCHER_ASYNC - call send an ack. This is per message 
type - you can't send the
- *  same message type with and without. Register two different
- *  messages if that is what you want.
+ * @ack:whether the dispatcher should send an ACK to the sender
  */

[Spice-devel] [PATCH spice-server v3 2/3] MainDispatcher: use correct argument type

[Jonathon Jongsma]

For dispatcher_register_handler(), use 'false' instead of 0 since the
last argument is a bool type now.

Signed-off-by: Jonathon Jongsma 
---
Changes since v2:
 - new patch

 server/main-dispatcher.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index 71f8f65d9..43f8b79a7 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -314,16 +314,16 @@ void main_dispatcher_constructed(GObject *object)
 DISPATCHER(self));
 dispatcher_register_handler(DISPATCHER(self), 
MAIN_DISPATCHER_CHANNEL_EVENT,
 main_dispatcher_handle_channel_event,
-sizeof(MainDispatcherChannelEventMessage), 0 
/* no ack */);
+sizeof(MainDispatcherChannelEventMessage), 
false);
 dispatcher_register_handler(DISPATCHER(self), 
MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
 main_dispatcher_handle_migrate_complete,
-
sizeof(MainDispatcherMigrateSeamlessDstCompleteMessage), 0 /* no ack */);
+
sizeof(MainDispatcherMigrateSeamlessDstCompleteMessage), false);
 dispatcher_register_handler(DISPATCHER(self), 
MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
 main_dispatcher_handle_mm_time_latency,
-sizeof(MainDispatcherMmTimeLatencyMessage), 0 
/* no ack */);
+sizeof(MainDispatcherMmTimeLatencyMessage), 
false);
 dispatcher_register_handler(DISPATCHER(self), 
MAIN_DISPATCHER_CLIENT_DISCONNECT,
 main_dispatcher_handle_client_disconnect,
-sizeof(MainDispatcherClientDisconnectMessage), 
0 /* no ack */);
+sizeof(MainDispatcherClientDisconnectMessage), 
false);
 }
 
 static void main_dispatcher_finalize(GObject *object)
-- 
2.13.3

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

[Spice-devel] [PATCH spice-server v4] Add documentation for Dispatcher

[Jonathon Jongsma]

---
Changes since v3:
 - removed documentation for 'opaque' argument from dispatcher_new()

 server/dispatcher.h | 95 -
 1 file changed, 80 insertions(+), 15 deletions(-)

diff --git a/server/dispatcher.h b/server/dispatcher.h
index ab7713845..1aaba13bc 100644
--- a/server/dispatcher.h
+++ b/server/dispatcher.h
@@ -35,6 +35,18 @@ typedef struct Dispatcher Dispatcher;
 typedef struct DispatcherClass DispatcherClass;
 typedef struct DispatcherPrivate DispatcherPrivate;
 
+/* A Dispatcher provides inter-thread communication by serializing messages.
+ * Currently the Dispatcher uses a unix socket (socketpair) for dispatching the
+ * messages.
+ *
+ * Message types are identified by a unique integer value and must first be
+ * registered with the class (see dispatcher_register_handler()) before they
+ * can be sent. Sending threads can send a message using the
+ * dispatcher_send_message() function. The receiving thread can monitor the
+ * dispatcher's 'receive' file descriptor (see dispatcher_get_recv_fd()) for
+ * activity and should call dispatcher_handle_recv_read() to process incoming
+ * messages.
+ */
 struct Dispatcher
 {
 GObject parent;
@@ -49,26 +61,52 @@ struct DispatcherClass
 
 GType dispatcher_get_type(void) G_GNUC_CONST;
 
+/* dispatcher_new
+ *
+ * Create a new Dispatcher object
+ *
+ * @max_message_type:   indicates the number of unique message types that can
+ *  be handled by this dispatcher. Each message type is
+ *  identified by an integer value between 0 and
+ *  max_message_type-1.
+ */
 Dispatcher *dispatcher_new(size_t max_message_type);
 
 
+/* The function signature for handlers of a specific message type */
 typedef void (*dispatcher_handle_message)(void *opaque,
   void *payload);
 
+/* The signature for a function that handles all messages (see
+ * dispatcher_register_universal_handler()) */
 typedef void (*dispatcher_handle_any_message)(void *opaque,
   uint32_t message_type,
   void *payload);
 
-/*
- * dispatcher_send_message
+/* dispatcher_send_message
+ *
+ * Sends a message to the receiving thread. The message type must have been
+ * registered first (see dispatcher_register_handler()).  @payload must be a
+ * buffer of the same size as the size registered for @message_type
+ *
+ * If the sent message is a message type requires an ACK, this function will
+ * block until it receives an ACK from the receiving thread.
+ *
  * @message_type: message type
  * @payload:  payload
  */
 void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
  void *payload);
 
-/*
- * dispatcher_register_handler
+/* dispatcher_register_handler
+ *
+ * This function registers a message type with the dispatcher, and registers
+ * @handler as the function that will handle incoming messages of this type.
+ * If @ack is true, the dispatcher will also send an ACK in response to the
+ * message after the message has been passed to the handler. You can only
+ * register a given message type once. For example, you cannot register two
+ * different handlers for the same message type with different @ack values.
+ *
  * @dispatcher: dispatcher
  * @messsage_type:  message type
  * @handler:message handler
@@ -79,32 +117,59 @@ void dispatcher_register_handler(Dispatcher *dispatcher, 
uint32_t message_type,
  dispatcher_handle_message handler, size_t 
size,
  bool ack);
 
-/*
- * Hack to allow red_record to see the message being sent so it can record
- * it to file.
+/* dispatcher_register_universal_handler
+ *
+ * Register a universal handler that will be called when *any* message is
+ * received by the dispatcher. When a message is received, this handler will be
+ * called first. If the received message type was registered via
+ * dispatcher_register_handler(), the message-specific handler will then be
+ * called. Only one universal handler can be registered. This feature can be
+ * used to record all messages to a file for replay and debugging.
+ *
+ * @dispatcher: dispatcher
+ * @handler:a handler function
  */
 void dispatcher_register_universal_handler(Dispatcher *dispatcher,
 dispatcher_handle_any_message handler);
 
-/*
- *  dispatcher_handle_recv_read
- *  @dispatcher: Dispatcher instance
+/* dispatcher_handle_recv_read
+ *
+ * A convenience function that is intended to be called by the receiving thread
+ * to handle all incoming messages and execute any handlers for those messages.
+ * This function will handle all incoming messages until there is no more data
+ * to read, so multiple handlers may be executed from a single call to
+ * dispatcher_handle_recv_read().
+ *
+ * @dispatcher: 

Re: [Spice-devel] [PATCH spice-server 4/6] red-qxl: Avoid to use AsyncCommand for GL_DRAW_ASYNC message

[Jonathon Jongsma]

OK. The invalid cookie value is a bit odd, but looks safe. 

Acked-by: Jonathon Jongsma 


On Thu, 2017-09-07 at 12:40 +0100, Frediano Ziglio wrote:
> AsyncCommand is used to handle asynchronous messages from the
> dispatcher.
> GL_DRAW_ASYNC is mainly using it to store the cookie.
> 
> The value of GL_DRAW_COOKIE_INVALID was choosen to allow implementing
> cookies (which basically are handles) either using indexes (where 0
> is
> valid) or pointers (where 0 is invalid). Currently Qemu uses
> pointers.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-qxl.c | 22 +-
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index bec063c17..b1804cf17 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -62,9 +62,11 @@ struct QXLState {
>  
>  pthread_mutex_t scanout_mutex;
>  SpiceMsgDisplayGlScanoutUnix scanout;
> -struct AsyncCommand *gl_draw_async;
> +uint64_t gl_draw_cookie;
>  };
>  
> +#define GL_DRAW_COOKIE_INVALID (~((uint64_t) 0))
> +
>  int red_qxl_check_qxl_version(QXLInstance *qxl, int major, int
> minor)
>  {
>  int qxl_major = qxl_get_interface(qxl)->base.major_version;
> @@ -833,7 +835,7 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
>  spice_return_if_fail(qxl != NULL);
>  
>  QXLState *qxl_state = qxl->st;
> -spice_return_if_fail(qxl_state->gl_draw_async == NULL);
> +spice_return_if_fail(qxl_state->gl_draw_cookie ==
> GL_DRAW_COOKIE_INVALID);
>  
>  pthread_mutex_lock(_state->scanout_mutex);
>  
> @@ -877,13 +879,15 @@ void spice_qxl_gl_draw_async(QXLInstance *qxl,
>  spice_return_if_fail(qxl != NULL);
>  qxl_state = qxl->st;
>  if (qxl_state->scanout.drm_dma_buf_fd == -1) {
> +QXLInterface *interface = qxl_get_interface(qxl);
> +
>  spice_warning("called spice_qxl_gl_draw_async without a
> buffer");
> -red_qxl_async_complete(qxl, async_command_alloc(qxl_state,
> message, cookie));
> +interface->async_complete(qxl, cookie);
>  return;
>  }
> -spice_return_if_fail(qxl_state->gl_draw_async == NULL);
> +spice_return_if_fail(qxl_state->gl_draw_cookie ==
> GL_DRAW_COOKIE_INVALID);
>  
> -qxl_state->gl_draw_async = async_command_alloc(qxl_state,
> message, cookie);
> +qxl_state->gl_draw_cookie = cookie;
>  dispatcher_send_message(qxl_state->dispatcher, message, );
>  }
>  
> @@ -899,7 +903,6 @@ void red_qxl_async_complete(QXLInstance *qxl,
> AsyncCommand *async_command)
>  case RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC:
>  case RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC:
>  case RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC:
> -case RED_WORKER_MESSAGE_GL_DRAW_ASYNC:
>  break;
>  case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC:
>  red_qxl_create_primary_surface_complete(qxl->st);
> @@ -916,10 +919,11 @@ void red_qxl_async_complete(QXLInstance *qxl,
> AsyncCommand *async_command)
>  
>  void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
>  {
> +QXLInterface *interface = qxl_get_interface(qxl);
>  /* this reset before usage prevent a possible race condition */
> -struct AsyncCommand *async = qxl->st->gl_draw_async;
> -qxl->st->gl_draw_async = NULL;
> -red_qxl_async_complete(qxl, async);
> +uint64_t cookie = qxl->st->gl_draw_cookie;
> +qxl->st->gl_draw_cookie = GL_DRAW_COOKIE_INVALID;
> +interface->async_complete(qxl, cookie);
>  }
>  
>  void red_qxl_init(RedsState *reds, QXLInstance *qxl)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-server] RedChannel: Remove obsolete TODO comment

[Jonathon Jongsma]

---
 server/red-channel.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/server/red-channel.h b/server/red-channel.h
index 55cfa7e1d..b5510c301 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -160,9 +160,6 @@ bool red_channel_test_remote_cap(RedChannel *channel, 
uint32_t cap);
 /* should be called when a new channel is ready to send messages */
 void red_channel_init_outgoing_messages_window(RedChannel *channel);
 
-// TODO: add back the channel_pipe_add functionality - by adding reference 
counting
-// to the RedPipeItem.
-
 // helper to push a new item to all channels
 typedef RedPipeItem *(*new_pipe_item_t)(RedChannelClient *rcc, void *data, int 
num);
 int red_channel_pipes_new_add(RedChannel *channel, new_pipe_item_t creator, 
void *data);
-- 
2.13.3

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

[Spice-devel] [PATCH spice-html5 4/4] Display: Add support for the VP9 codec type

[Tomáš Bohdálek]

---
 display.js   | 26 --
 spiceconn.js |  2 ++
 webm.js  | 12 ++--
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/display.js b/display.js
index 0868f91..abd5b1a 100644
--- a/display.js
+++ b/display.js
@@ -543,7 +543,8 @@ SpiceDisplayConn.prototype.process_channel_message = 
function(msg)
 else
 this.streams[m.id] = m;
 
-if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+if (m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
+m.codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
 {
 var media = new MediaSource();
 var v = document.createElement("video");
@@ -606,7 +607,8 @@ SpiceDisplayConn.prototype.process_channel_message = 
function(msg)
 if (this.streams[m.base.id].codec_type === 
SPICE_VIDEO_CODEC_TYPE_MJPEG)
 process_mjpeg_stream_data(this, m, time_until_due);
 
-if (this.streams[m.base.id].codec_type === SPICE_VIDEO_CODEC_TYPE_VP8)
+if (this.streams[m.base.id].codec_type === SPICE_VIDEO_CODEC_TYPE_VP8 
||
+this.streams[m.base.id].codec_type === SPICE_VIDEO_CODEC_TYPE_VP9)
 process_video_stream_data(this.streams[m.base.id], m);
 
 return true;
@@ -640,7 +642,8 @@ SpiceDisplayConn.prototype.process_channel_message = 
function(msg)
 var m = new SpiceMsgDisplayStreamDestroy(msg.data);
 STREAM_DEBUG > 0 && console.log(this.type + ": MsgStreamDestroy id" + 
m.id);
 
-if (this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+if (this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
+this.streams[m.id].codec_type == SPICE_VIDEO_CODEC_TYPE_VP9)
 {
 
document.getElementById(this.parent.screen_id).removeChild(this.streams[m.id].video);
 this.streams[m.id].source_buffer = null;
@@ -1036,14 +1039,24 @@ function handle_video_source_open(e)
 {
 var stream = this.stream;
 var p = this.spiceconn;
+var codec_type;
 
 if (stream.source_buffer)
 return;
 
-var s = this.addSourceBuffer(SPICE_VP8_CODEC);
+if (this.stream.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+{
+codec_type = SPICE_VP8_CODEC;
+}
+else
+{
+codec_type = SPICE_VP9_CODEC;
+}
+
+var s = this.addSourceBuffer(codec_type);
 if (! s)
 {
-p.log_err('Codec ' + SPICE_VP8_CODEC + ' not available.');
+p.log_err('Codec ' + codec_type + ' not available.');
 return;
 }
 
@@ -1054,7 +1067,8 @@ function handle_video_source_open(e)
 listen_for_video_events(stream);
 
 var h = new webm_Header();
-var te = new webm_VideoTrackEntry(this.stream.stream_width, 
this.stream.stream_height);
+var te = new webm_VideoTrackEntry(this.stream.stream_width, 
this.stream.stream_height,
+  this.stream.codec_type);
 var t = new webm_Tracks(te);
 
 var mb = new ArrayBuffer(h.buffer_size() + t.buffer_size())
diff --git a/spiceconn.js b/spiceconn.js
index 78d5820..4c7bcaf 100644
--- a/spiceconn.js
+++ b/spiceconn.js
@@ -147,6 +147,8 @@ SpiceConn.prototype =
 (1 << SPICE_DISPLAY_CAP_CODEC_MJPEG);
 if ('MediaSource' in window && 
MediaSource.isTypeSupported(SPICE_VP8_CODEC))
 caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP8);
+if ('MediaSource' in window && 
MediaSource.isTypeSupported(SPICE_VP9_CODEC))
+caps |= (1 << SPICE_DISPLAY_CAP_CODEC_VP9);
 msg.channel_caps.push(caps);
 }
 
diff --git a/webm.js b/webm.js
index 789da14..c697135 100644
--- a/webm.js
+++ b/webm.js
@@ -88,6 +88,7 @@ var EXPECTED_PACKET_DURATION= 10;
 var GAP_DETECTION_THRESHOLD = 50;
 
 var SPICE_VP8_CODEC = 'video/webm; codecs="vp8"';
+var SPICE_VP9_CODEC = 'video/webm; codecs="vp9"';
 
 /*
 **  EBML utility functions
@@ -467,7 +468,7 @@ webm_AudioTrackEntry.prototype =
 },
 }
 
-function webm_VideoTrackEntry(width, height)
+function webm_VideoTrackEntry(width, height, codec_type)
 {
 this.id = WEBM_TRACK_ENTRY;
 this.number = 1;
@@ -482,8 +483,15 @@ function webm_VideoTrackEntry(width, height)
 this.codec_decode_all = 0; // fixme - check
 this.seek_pre_roll = 0; // 8000; // fixme - check
 this.codec_delay =   8000; // Must match codec_private.preskip
-this.codec_id = "V_VP8";
 this.video = new webm_Video(width, height);
+if (codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+{
+this.codec_id = "V_VP8";
+}
+else
+{
+this.codec_id = "V_VP9";
+}
 }
 
 webm_VideoTrackEntry.prototype =
-- 
2.9.5

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

[Spice-devel] [PATCH spice-html5 3/4] Display: Implement change preferred video codec type message

[Tomáš Bohdálek]

---
 display.js  | 16 
 enums.js| 10 ++
 spicemsg.js | 24 
 3 files changed, 50 insertions(+)

diff --git a/display.js b/display.js
index 60c79b4..0868f91 100644
--- a/display.js
+++ b/display.js
@@ -1261,3 +1261,19 @@ SpiceDisplayConn.prototype.change_preferred_compression 
= function(compression_i
 msg.build_msg(SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION, compression);
 this.send_msg(msg);
 }
+
+SpiceDisplayConn.prototype.change_preferred_video_codec_type = 
function(video_codecs)
+{
+var ch = this.channel_type();
+if (!this.channel_test_capability(SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE))
+{
+this.log_warn(ch + " does not have capability to change the preferred 
video codec type");
+return;
+}
+
+var msg = new SpiceMiniData();
+var video_codec_type = new 
SpiceMsgcDisplayPreferredVideoCodecType(video_codecs);
+
+msg.build_msg(SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE, 
video_codec_type);
+this.send_msg(msg);
+}
diff --git a/enums.js b/enums.js
index b37cb20..b69bf3e 100644
--- a/enums.js
+++ b/enums.js
@@ -135,6 +135,8 @@ var SPICE_MSG_DISPLAY_STREAM_ACTIVATE_REPORT = 319;
 var SPICE_MSGC_DISPLAY_INIT = 101;
 var SPICE_MSGC_DISPLAY_STREAM_REPORT= 102;
 var SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION = 103;
+var SPICE_MSGC_DISPLAY_GL_DRAW_DONE = 104;
+var SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE = 105;
 
 var SPICE_MSG_INPUTS_INIT   = 101;
 var SPICE_MSG_INPUTS_KEY_MODIFIERS  = 102;
@@ -197,6 +199,9 @@ var SPICE_DISPLAY_CAP_GL_SCANOUT  = 7;
 var SPICE_DISPLAY_CAP_MULTI_CODEC = 8;
 var SPICE_DISPLAY_CAP_CODEC_MJPEG = 9;
 var SPICE_DISPLAY_CAP_CODEC_VP8   = 10;
+var SPICE_DISPLAY_CAP_CODEC_H264  = 11;
+var SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE = 12;
+var SPICE_DISPLAY_CAP_CODEC_VP9   = 13;
 
 var SPICE_AUDIO_DATA_MODE_INVALID   = 0;
 var SPICE_AUDIO_DATA_MODE_RAW   = 1;
@@ -231,6 +236,11 @@ var SPICE_MOUSE_MODE_SERVER = (1 << 0),
 var SPICE_CLIP_TYPE_NONE= 0;
 var SPICE_CLIP_TYPE_RECTS   = 1;
 
+var SPICE_VIDEO_CODEC_TYPE_MJPEG= 1,
+SPICE_VIDEO_CODEC_TYPE_VP8  = 2,
+SPICE_VIDEO_CODEC_TYPE_H264 = 3,
+SPICE_VIDEO_CODEC_TYPE_VP9  = 4;
+
 var SPICE_IMAGE_TYPE_BITMAP = 0;
 var SPICE_IMAGE_TYPE_QUIC   = 1;
 var SPICE_IMAGE_TYPE_RESERVED   = 2;
diff --git a/spicemsg.js b/spicemsg.js
index 8a9eeed..dd684b7 100644
--- a/spicemsg.js
+++ b/spicemsg.js
@@ -1316,3 +1316,27 @@ SpiceMsgcDisplayPreferredCompression.prototype =
 return 1;
 }
 }
+
+function SpiceMsgcDisplayPreferredVideoCodecType(codecs)
+{
+this.codecs = codecs;
+this.num_of_codecs = codecs.length;
+}
+
+SpiceMsgcDisplayPreferredVideoCodecType.prototype =
+{
+to_buffer: function(a, at)
+{
+at = at || 0;
+var dv = new SpiceDataView(a);
+dv.setUint8(at, this.num_of_codecs, true); at += 1;
+for (var i = 0; i < this.codecs.length; i++)
+dv.setUint8(at + i, this.codecs[i], true);
+at += this.num_of_codecs;
+return at;
+},
+buffer_size: function()
+{
+return this.num_of_codecs + 1;
+}
+}
-- 
2.9.5

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

[Spice-devel] [PATCH spice-html5 1/4] spiceconn: Add function to test channel capabilities

[Tomáš Bohdálek]

This will be used in other commits.
---
 spiceconn.js | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/spiceconn.js b/spiceconn.js
index 33e7388..78d5820 100644
--- a/spiceconn.js
+++ b/spiceconn.js
@@ -243,6 +243,9 @@ SpiceConn.prototype =
 else if (this.state == "link")
 {
 this.reply_link = new SpiceLinkReply(mb);
+this.common_caps = this.reply_link.common_caps;
+this.channel_caps = this.reply_link.channel_caps;
+
  // FIXME - Screen the caps - require minihdr at least, right?
 if (this.reply_link.error)
 {
@@ -495,6 +498,22 @@ SpiceConn.prototype =
 var e = new Error("Connection timed out.");
 this.report_error(e);
 },
+
+test_capability: function(caps, cap)
+{
+var ret = (caps >> cap) & 1;
+return ret;
+},
+
+channel_test_capability: function(cap)
+{
+return this.test_capability(this.channel_caps, cap);
+},
+
+channel_test_common_capability: function(cap)
+{
+return this.test_capability(this.common_caps, cap);
+}
 }
 
 function spiceconn_timeout(sc)
-- 
2.9.5

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

[Spice-devel] [PATCH spice-html5 2/4] Display: Implement change preferred compression message

[Tomáš Bohdálek]

---
 display.js  | 16 
 enums.js| 10 ++
 spicemsg.js | 20 
 3 files changed, 46 insertions(+)

diff --git a/display.js b/display.js
index 7719b23..60c79b4 100644
--- a/display.js
+++ b/display.js
@@ -1245,3 +1245,19 @@ function listen_for_video_events(stream)
 if (STREAM_DEBUG > 1)
 video_2_events.forEach(video_debug_listen_for_one_event, stream.video);
 }
+
+SpiceDisplayConn.prototype.change_preferred_compression = 
function(compression_id)
+{
+var ch = this.channel_type();
+if (!this.channel_test_capability(SPICE_DISPLAY_CAP_PREF_COMPRESSION))
+{
+this.log_warn(ch + " does not have capability to change the preferred 
compression");
+return;
+}
+
+var msg = new SpiceMiniData();
+var compression = new SpiceMsgcDisplayPreferredCompression(compression_id);
+
+msg.build_msg(SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION, compression);
+this.send_msg(msg);
+}
diff --git a/enums.js b/enums.js
index b6e013c..b37cb20 100644
--- a/enums.js
+++ b/enums.js
@@ -134,6 +134,7 @@ var SPICE_MSG_DISPLAY_STREAM_ACTIVATE_REPORT = 319;
 
 var SPICE_MSGC_DISPLAY_INIT = 101;
 var SPICE_MSGC_DISPLAY_STREAM_REPORT= 102;
+var SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION = 103;
 
 var SPICE_MSG_INPUTS_INIT   = 101;
 var SPICE_MSG_INPUTS_KEY_MODIFIERS  = 102;
@@ -243,6 +244,15 @@ var SPICE_IMAGE_TYPE_FROM_CACHE_LOSSLESS = 106;
 var SPICE_IMAGE_TYPE_ZLIB_GLZ_RGB   = 107;
 var SPICE_IMAGE_TYPE_JPEG_ALPHA = 108;
 
+var SPICE_IMAGE_COMPRESSION_INVALID = 0,
+SPICE_IMAGE_COMPRESSION_OFF = 1,
+SPICE_IMAGE_COMPRESSION_AUTO_GLZ = 2,
+SPICE_IMAGE_COMPRESSION_AUTO_LZ = 3,
+SPICE_IMAGE_COMPRESSION_QUIC= 4,
+SPICE_IMAGE_COMPRESSION_GLZ = 5,
+SPICE_IMAGE_COMPRESSION_LZ  = 6,
+SPICE_IMAGE_COMPRESSION_LZ4 = 7;
+
 var SPICE_IMAGE_FLAGS_CACHE_ME = (1 << 0),
 SPICE_IMAGE_FLAGS_HIGH_BITS_SET = (1 << 1),
 SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME = (1 << 2);
diff --git a/spicemsg.js b/spicemsg.js
index 3619996..8a9eeed 100644
--- a/spicemsg.js
+++ b/spicemsg.js
@@ -1296,3 +1296,23 @@ SpiceMsgPortInit.prototype =
 this.name = a.slice(offset, offset + namesize - 1);
 }
 }
+
+function SpiceMsgcDisplayPreferredCompression(compression)
+{
+this.image_compression = compression;
+}
+
+SpiceMsgcDisplayPreferredCompression.prototype =
+{
+to_buffer: function(a, at)
+{
+at = at || 0;
+var dv = new SpiceDataView(a);
+dv.setUint8(at, this.image_compression, true); at += 1;
+return at;
+},
+buffer_size: function()
+{
+return 1;
+}
+}
-- 
2.9.5

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

[Spice-devel] [PATCH spice-html5 0/4] display: Implement preferred compression messages

[Tomáš Bohdálek]

These patches implement spice messages for changing image and video compression.
The last patch adds support for VP9 decoding.

Tomáš Bohdálek (4):
  spiceconn: Add function to test channel capabilities
  Display: Implement change preferred compression message
  Display: Implement change preferred video codec type message
  Display: Add support for the VP9 codec type

 display.js   | 58 --
 enums.js | 20 
 spiceconn.js | 21 +
 spicemsg.js  | 44 
 webm.js  | 12 ++--
 5 files changed, 147 insertions(+), 8 deletions(-)

-- 
2.9.5

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

Re: [Spice-devel] [PATCH spice-server v2 10/15] test-display-base: Always compile with AUTOMATED_TESTS enabled

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Wed, Sep 06, 2017 at 05:26:54PM +0100, Frediano Ziglio wrote:
> There's no need to not compile this feature, it just enable
> a parameters which must be passed in order to change test
> behaviour.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  configure.ac | 17 -
>  server/tests/Makefile.am |  4 
>  server/tests/test-display-base.c |  6 --
>  3 files changed, 27 deletions(-)
> 
> Changes since v1:
> - remove configure stuff.
> 
> diff --git a/configure.ac b/configure.ac
> index 7d8afd572..483dbfdf8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -122,12 +122,6 @@ if test "x$have_gstreamer_0_10" = "xyes" || test 
> "x$have_gstreamer_1_0" = "xyes"
>  AC_SUBST(ORC_LIBS)
>  fi
>  
> -AC_ARG_ENABLE([automated_tests],
> -  AS_HELP_STRING([--enable-automated-tests], [Enable automated 
> tests using spicy-screenshot (part of spice-gtk)]),,
> -  [enable_automated_tests="no"])
> -AS_IF([test x"$enable_automated_tests" != "xno"], 
> [enable_automated_tests="yes"])
> -AM_CONDITIONAL(HAVE_AUTOMATED_TESTS, test "x$enable_automated_tests" != 
> "xno")
> -
>  dnl Check for the presence of Valgrind and do the plumbing to allow
>  dnl the running of "make check-valgrind".
>  AX_VALGRIND_DFLT(memcheck, on)
> @@ -214,16 +208,6 @@ AC_SUBST(JPEG_LIBS)
>  AC_CHECK_LIB(z, deflate, Z_LIBS='-lz', AC_MSG_ERROR([zlib not found]))
>  AC_SUBST(Z_LIBS)
>  
> -if test "x$enable_automated_tests" = "xyes"; then
> -AC_MSG_CHECKING([for spicy-screenshot])
> -spicy-screenshot --help >/dev/null 2>&1
> -if test $? -ne 0 ; then
> -AC_MSG_RESULT([not found])
> -AC_MSG_ERROR([spicy-screenshot was not found, this module is part of 
> spice-gtk and is required to compile this package])
> -fi
> -AC_MSG_RESULT([found])
> -fi
> -
>  
>  AC_ARG_ENABLE([manual],
> AS_HELP_STRING([--enable-manual=@<:@auto/yes/no@:>@],
> @@ -327,7 +311,6 @@ AC_MSG_NOTICE([
>  Smartcard:${have_smartcard}
>  GStreamer:${enable_gstreamer}
>  SASL support: ${have_sasl}
> -Automated tests:  ${enable_automated_tests}
>  Manual:   ${have_asciidoc}
>  
>  Now type 'make' to build $PACKAGE
> diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> index 17414f657..b64add5f5 100644
> --- a/server/tests/Makefile.am
> +++ b/server/tests/Makefile.am
> @@ -24,10 +24,6 @@ AM_CPPFLAGS =  \
>   $(WARN_CFLAGS)  \
>   $(NULL)
>  
> -if HAVE_AUTOMATED_TESTS
> -AM_CPPFLAGS += -DAUTOMATED_TESTS
> -endif
> -
>  noinst_LIBRARIES = libtest.a
>  
>  libtest_a_SOURCES =  \
> diff --git a/server/tests/test-display-base.c 
> b/server/tests/test-display-base.c
> index 768b8986f..e27dcf951 100644
> --- a/server/tests/test-display-base.c
> +++ b/server/tests/test-display-base.c
> @@ -948,11 +948,7 @@ static void init_automated(void)
>  static __attribute__((noreturn))
>  void usage(const char *argv0, const int exitcode)
>  {
> -#ifdef AUTOMATED_TESTS
>  const char *autoopt=" [--automated-tests]";
> -#else
> -const char *autoopt="";
> -#endif
>  
>  printf("usage: %s%s\n", argv0, autoopt);
>  exit(exitcode);
> @@ -961,9 +957,7 @@ void usage(const char *argv0, const int exitcode)
>  void spice_test_config_parse_args(int argc, char **argv)
>  {
>  struct option options[] = {
> -#ifdef AUTOMATED_TESTS
>  {"automated-tests", no_argument, _automated_tests, 1},
> -#endif
>  {NULL, 0, NULL, 0},
>  };
>  int option_index;
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 14/15] tests: Make test-two-servers work

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Wed, Sep 06, 2017 at 05:26:55PM +0100, Frediano Ziglio wrote:
> This test runs 2 spice server in one program.
> Use two different tcp port to be able to connect to both servers.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/tests/test-display-base.c | 8 ++--
>  server/tests/test-display-base.h | 1 +
>  server/tests/test-two-servers.c  | 2 +-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> - use test_new_with_port;
> - moved thread part to another patch.
> 
> diff --git a/server/tests/test-display-base.c 
> b/server/tests/test-display-base.c
> index e27dcf951..289aa9840 100644
> --- a/server/tests/test-display-base.c
> +++ b/server/tests/test-display-base.c
> @@ -900,9 +900,8 @@ void test_set_command_list(Test *test, Command *commands, 
> int num_commands)
>  }
>  
>  
> -Test *test_new(SpiceCoreInterface *core)
> +Test* test_new_with_port(SpiceCoreInterface* core, int port)
>  {
> -int port = 5912;
>  Test *test = spice_new0(Test, 1);
>  SpiceServer* server = spice_server_new();
>  
> @@ -926,6 +925,11 @@ Test *test_new(SpiceCoreInterface *core)
>  return test;
>  }
>  
> +Test *test_new(SpiceCoreInterface *core)
> +{
> +return test_new_with_port(core, 5912);
> +}
> +
>  void test_destroy(Test *test)
>  {
>  spice_server_destroy(test->server);
> diff --git a/server/tests/test-display-base.h 
> b/server/tests/test-display-base.h
> index 1a4f20c5b..a80f03e78 100644
> --- a/server/tests/test-display-base.h
> +++ b/server/tests/test-display-base.h
> @@ -134,6 +134,7 @@ void test_set_simple_command_list(Test *test, const int 
> *command, int num_comman
>  void test_set_command_list(Test *test, Command *command, int num_commands);
>  void test_add_display_interface(Test *test);
>  void test_add_agent_interface(SpiceServer *server); // TODO - Test *test
> +Test* test_new_with_port(SpiceCoreInterface* core, int port);
>  Test* test_new(SpiceCoreInterface* core);
>  void test_destroy(Test *test);
>  
> diff --git a/server/tests/test-two-servers.c b/server/tests/test-two-servers.c
> index 40a0e5717..92935528e 100644
> --- a/server/tests/test-two-servers.c
> +++ b/server/tests/test-two-servers.c
> @@ -42,7 +42,7 @@ int main(void)
>  
>  core = basic_event_loop_init();
>  t1 = test_new(core);
> -t2 = test_new(core);
> +t2 = test_new_with_port(core, 5913);
>  //spice_server_set_image_compression(server, 
> SPICE_IMAGE_COMPRESSION_OFF);
>  test_add_display_interface(t1);
>  test_add_display_interface(t2);
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 03/15] test-display-base: Avoid usage after free when the wakeup timer is freed

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Wed, Sep 06, 2017 at 05:26:53PM +0100, Frediano Ziglio wrote:
> The wakeup timer is used by the worker thread and by the
> main thread.
> Destroying the object before destroying the worker thread
> can lead to use after free.
> Destroying the worker thread first makes sure we don't race.
> This is detected easily when compiling the test with address sanitizer.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/tests/test-display-base.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Changes since v1:
> - update commit message and shortlog.
> 
> diff --git a/server/tests/test-display-base.c 
> b/server/tests/test-display-base.c
> index 14311dbc2..c35eec1da 100644
> --- a/server/tests/test-display-base.c
> +++ b/server/tests/test-display-base.c
> @@ -921,8 +921,10 @@ Test *test_new(SpiceCoreInterface *core)
>  
>  void test_destroy(Test *test)
>  {
> -test->core->timer_remove(test->wakeup_timer);
>  spice_server_destroy(test->server);
> +// this timer is used by spice server so
> +// avoid to free it while is running
> +test->core->timer_remove(test->wakeup_timer);
>  free(test->commands);
>  free(test);
>  }
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 3/3] Fix crash attempting to connect duplicate channels

[Frediano Ziglio]

> 
> On Wed, 2017-08-30 at 10:36 +0100, Frediano Ziglio wrote:
> > You could easily trigger this issue using multiple monitors and
> > a modified spice-gtk client with this patch:
> > 
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
> >  {
> >  g_return_val_if_fail(c != NULL, FALSE);
> > 
> > +if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
> >  spice_channel_new(c->session, c->type, c->id);
> > 
> >  g_object_unref(c->session);
> > 
> > 
> > which cause a crash like
> > 
> > (process:28742): Spice-WARNING **: Failed to create channel client:
> > Client 0x40246f5d0: duplicate channel type 2 id 0
> > 2017-08-24 09:36:57.451+: shutting down, reason=crashed
> 
> It took me quite a while to figure out how this crash is related to the
> patch below. Perhaps it needs additional detail in the commit log? As
> far as I can tell, it's like this:
> 
> RedChannelClient is an GInitable type, which means that the object is
> constructed, and then the _init() function is called, which can fail.
> If the _init() fails, the newly-created object will be destroyed. As
> part of _init(), we add a new watch for the stream using the core
> interface that is associated with the channel. After adding the watch,
> our rcc creation fails (due to duplicate ID), and the rcc object is
> unreffed. This results in a call to reds_stream_free() (since the rcc
> now owns the stream). But in reds_stream_free, we were trying to remove
> the watch from the core interface associated with the RedsState. For
> most channels, these two core interfaces are equivalent. But for the
> Display and Cursor channels, it is the core Glib-based interface
> associated with the RedWorker.
> 

You already know where this paragraph will end, right ?
Yes!

> Do I understand it correctly?
> 
> > 
> > The watch in RedsStream by default is bound to the Qemu
> > provided SpiceCoreInterface while RedChannelClient bound it
> > to Glib one causing the crash when the watch is deleted from
> > RedsStream. Change the bound interface.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/red-channel-client.c | 14 +-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-
> > client.c
> > index 145ba43f..acc2b4eb 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -339,6 +339,16 @@ red_channel_client_finalize(GObject *object)
> >  {
> >  RedChannelClient *self = RED_CHANNEL_CLIENT(object);
> >  
> > +SpiceCoreInterfaceInternal *core =
> > red_channel_get_core_interface(self->priv->channel);
> > +if (self->priv->latency_monitor.timer) {
> > +core->timer_remove(core, self->priv->latency_monitor.timer);
> > +self->priv->latency_monitor.timer = NULL;
> > +}
> > +if (self->priv->connectivity_monitor.timer) {
> > +core->timer_remove(core, self->priv-
> > >connectivity_monitor.timer);
> > +self->priv->connectivity_monitor.timer = NULL;
> > +}
> > +
> 
> I'm a little bit confused here. Is this part related at all to the
> crash in the commit message? It doesn't seem to be.
> 

Yes, in case monitoring is enabled (currently DCC) the monitoring
timer is registered causing a use-after-free too.
Not both timers are needed but I think that a finalize is more
safer and robust if it handles any possible status.

> >  reds_stream_free(self->priv->stream);
> >  self->priv->stream = NULL;
> >  
> > @@ -921,12 +931,14 @@ static gboolean
> > red_channel_client_initable_init(GInitable *initable,
> >  }
> >  
> >  core = red_channel_get_core_interface(self->priv->channel);
> > -if (self->priv->stream)
> > +if (self->priv->stream) {
> > +reds_stream_set_core_interface(self->priv->stream, core);
> >  self->priv->stream->watch =
> >  core->watch_add(core, self->priv->stream->socket,
> >  SPICE_WATCH_EVENT_READ,
> >  red_channel_client_event,
> >  self);
> > +}
> 
> It seems a little bit weird that the RedChannelClient is poking into
> the stream structure to set stream->watch...
> 

Not a regression of this patch. I found safer making sure that
RedsStream can release the watch safely as the code already
automatically do it. The previous patch instead added a "watch"
in RCC but was more intrusive.

> What about moving this whole section down below the call to
> red_client_add_channel() (which is the call that can fail) in order to
> avoid creating the watch at all when our rcc creation fails?
> 

I can do but I prefer a more robust code instead of having
finalize implementation depending on red_channel_client_initable_init
implementation.
I won't be surprised if moving these watch/timers could lead to
different regressions. Registering to RedClient (which runs 

Re: [Spice-devel] [PATCH spice-server v2 1/2] Introduce some macros to help declaring new GObject

[Christophe Fergeau]

On Wed, Sep 06, 2017 at 03:42:29PM +0100, Frediano Ziglio wrote:
> The macros will implement most of the boilerplate needed
> to declare an object.
> Their usage are similar to GLib G_DECLARE_*_TYPE macros.

Can we/should we use the GLib provided macros when they are available,
and copy/paste the GLib implementation in a -compat.h header for older
systems. The GLib macros were introduced in GLib 2.43.4

Christophe

> 
> Signed-off-by: Frediano Ziglio 
> ---
> Changes since v1:
> - use SPICE_ prefix instead of GOBJECT_;
> - if RED_ prefix is used use this prefix for all GLib
>   macros.
> ---
>  server/red-common.h | 60 
> +
>  1 file changed, 60 insertions(+)
> 
> diff --git a/server/red-common.h b/server/red-common.h
> index 9ff1fd9b5..19bb3fb6e 100644
> --- a/server/red-common.h
> +++ b/server/red-common.h
> @@ -93,4 +93,64 @@ typedef struct GListIter {
>  #define GLIST_FOREACH_REVERSED(_list, _type, _data) \
>  GLIST_FOREACH_GENERIC(_list, G_PASTE(_iter_, __LINE__), _type, _data, 
> prev)
>  
> +#define SPICE_DECLARE_TYPE_GENERIC(ModuleObjType, ModuleObjName, 
> module_obj_name, MODULE, OBJ_NAME) \
> +typedef ModuleObjType ModuleObjName; \
> +typedef struct ModuleObjName ## Class ModuleObjName ## Class; \
> +typedef struct ModuleObjName ## Private ModuleObjName ## Private; \
> +GType module_obj_name ## _get_type(void) G_GNUC_CONST; \
> +static inline ModuleObjName *G_PASTE(MODULE,OBJ_NAME)(void *obj) \
> +{ return G_TYPE_CHECK_INSTANCE_CAST(obj, \
> + module_obj_name ## _get_type(), ModuleObjName); } \
> +static inline ModuleObjName ## Class 
> *G_PASTE(G_PASTE(MODULE,OBJ_NAME),_CLASS)(void *klass) \
> +{ return G_TYPE_CHECK_CLASS_CAST(klass, \
> + module_obj_name ## _get_type(), ModuleObjName ## Class); } \
> +static inline gboolean G_PASTE(G_PASTE(MODULE,IS_),OBJ_NAME)(void *obj) \
> +{ return G_TYPE_CHECK_INSTANCE_TYPE(obj, module_obj_name ## 
> _get_type()); } \
> +static inline gboolean 
> G_PASTE(G_PASTE(G_PASTE(MODULE,IS_),OBJ_NAME),_CLASS)(void *klass) \
> +{ return G_TYPE_CHECK_CLASS_TYPE((klass), module_obj_name ## 
> _get_type()); } \
> +static inline ModuleObjName ## Class 
> *G_PASTE(G_PASTE(MODULE,OBJ_NAME),_GET_CLASS)(void *obj) \
> +{ return G_TYPE_INSTANCE_GET_CLASS(obj, \
> + module_obj_name ## _get_type(), ModuleObjName ## Class); }
> +
> +#define SPICE_DECLARE_EMPTY_MODULE
> +
> +/* Helper to declare a GObject type
> + *
> + * @ModuleObjType used to specify the structure type in case is not
> + *the same as the @ModuleObjName. For instance can be
> + *"struct MyObjectDefinition"
> + * @ModuleObjName type identifier like MyObject
> + * @module_obj_name   method prefix like my_object (no need to add the
> + *underscore)
> + * @OBJ_NAME  macro common part like MY_OBJECT
> + */
> +#define SPICE_DECLARE_TYPE_STRUCT(ModuleObjType, ModuleObjName, 
> module_obj_name, OBJ_NAME) \
> +SPICE_DECLARE_TYPE_GENERIC(ModuleObjType, ModuleObjName, 
> module_obj_name, \
> + SPICE_DECLARE_EMPTY_MODULE, OBJ_NAME)
> +
> +/* Helper to declare a GObject type
> + *
> + * Same as SPICE_DECLARE_TYPE_STRUCT but the macro names
> + * will have the "RED_" prefix.
> + */
> +#define SPICE_DECLARE_RED_TYPE_STRUCT(ModuleObjType, ModuleObjName, 
> module_obj_name, OBJ_NAME) \
> +SPICE_DECLARE_TYPE_GENERIC(ModuleObjType, ModuleObjName, 
> module_obj_name, RED_, OBJ_NAME)
> +
> +/* Helper to declare a GObject type
> + *
> + * Same as SPICE_DECLARE_TYPE_STRUCT but the structure is the same
> + * as the type, most declarations use this convention.
> + */
> +#define SPICE_DECLARE_TYPE(ModuleObjName, module_obj_name, OBJ_NAME) \
> +SPICE_DECLARE_TYPE_GENERIC(struct ModuleObjName, ModuleObjName, 
> module_obj_name, \
> + SPICE_DECLARE_EMPTY_MODULE, OBJ_NAME)
> +
> +/* Helper to declare a GObject type
> + *
> + * Same as SPICE_DECLARE_TYPE but the macro names
> + * will have the "RED_" prefix.
> + */
> +#define SPICE_DECLARE_RED_TYPE(ModuleObjName, module_obj_name, OBJ_NAME) \
> +SPICE_DECLARE_TYPE_GENERIC(struct ModuleObjName, ModuleObjName, 
> module_obj_name, RED_, OBJ_NAME)
> +
>  #endif /* RED_COMMON_H_ */
> -- 
> 2.13.5
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 1/2] Introduce some macros to help declaring new GObject

[Frediano Ziglio]

> 
> On Wed, Sep 06, 2017 at 03:42:29PM +0100, Frediano Ziglio wrote:
> > The macros will implement most of the boilerplate needed
> > to declare an object.
> > Their usage are similar to GLib G_DECLARE_*_TYPE macros.
> 
> Can we/should we use the GLib provided macros when they are available,
> and copy/paste the GLib implementation in a -compat.h header for older
> systems. The GLib macros were introduced in GLib 2.43.4
> 
> Christophe
> 

That's fine. But our objects are currently neither final neither interfaces
so we can't use any of them.
I changed the field names and fields order to better match GLib versions.

Frediano

> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > Changes since v1:
> > - use SPICE_ prefix instead of GOBJECT_;
> > - if RED_ prefix is used use this prefix for all GLib
> >   macros.
> > ---
> >  server/red-common.h | 60
> >  +
> >  1 file changed, 60 insertions(+)
> > 
> > diff --git a/server/red-common.h b/server/red-common.h
> > index 9ff1fd9b5..19bb3fb6e 100644
> > --- a/server/red-common.h
> > +++ b/server/red-common.h
> > @@ -93,4 +93,64 @@ typedef struct GListIter {
> >  #define GLIST_FOREACH_REVERSED(_list, _type, _data) \
> >  GLIST_FOREACH_GENERIC(_list, G_PASTE(_iter_, __LINE__), _type, _data,
> >  prev)
> >  
> > +#define SPICE_DECLARE_TYPE_GENERIC(ModuleObjType, ModuleObjName,
> > module_obj_name, MODULE, OBJ_NAME) \
> > +typedef ModuleObjType ModuleObjName; \
> > +typedef struct ModuleObjName ## Class ModuleObjName ## Class; \
> > +typedef struct ModuleObjName ## Private ModuleObjName ## Private; \
> > +GType module_obj_name ## _get_type(void) G_GNUC_CONST; \
> > +static inline ModuleObjName *G_PASTE(MODULE,OBJ_NAME)(void *obj) \
> > +{ return G_TYPE_CHECK_INSTANCE_CAST(obj, \
> > + module_obj_name ## _get_type(), ModuleObjName); } \
> > +static inline ModuleObjName ## Class
> > *G_PASTE(G_PASTE(MODULE,OBJ_NAME),_CLASS)(void *klass) \
> > +{ return G_TYPE_CHECK_CLASS_CAST(klass, \
> > + module_obj_name ## _get_type(), ModuleObjName ## Class); } \
> > +static inline gboolean G_PASTE(G_PASTE(MODULE,IS_),OBJ_NAME)(void
> > *obj) \
> > +{ return G_TYPE_CHECK_INSTANCE_TYPE(obj, module_obj_name ##
> > _get_type()); } \
> > +static inline gboolean
> > G_PASTE(G_PASTE(G_PASTE(MODULE,IS_),OBJ_NAME),_CLASS)(void *klass) \
> > +{ return G_TYPE_CHECK_CLASS_TYPE((klass), module_obj_name ##
> > _get_type()); } \
> > +static inline ModuleObjName ## Class
> > *G_PASTE(G_PASTE(MODULE,OBJ_NAME),_GET_CLASS)(void *obj) \
> > +{ return G_TYPE_INSTANCE_GET_CLASS(obj, \
> > + module_obj_name ## _get_type(), ModuleObjName ## Class); }
> > +
> > +#define SPICE_DECLARE_EMPTY_MODULE
> > +
> > +/* Helper to declare a GObject type
> > + *
> > + * @ModuleObjType used to specify the structure type in case is not
> > + *the same as the @ModuleObjName. For instance can be
> > + *"struct MyObjectDefinition"
> > + * @ModuleObjName type identifier like MyObject
> > + * @module_obj_name   method prefix like my_object (no need to add the
> > + *underscore)
> > + * @OBJ_NAME  macro common part like MY_OBJECT
> > + */
> > +#define SPICE_DECLARE_TYPE_STRUCT(ModuleObjType, ModuleObjName,
> > module_obj_name, OBJ_NAME) \
> > +SPICE_DECLARE_TYPE_GENERIC(ModuleObjType, ModuleObjName,
> > module_obj_name, \
> > + SPICE_DECLARE_EMPTY_MODULE, OBJ_NAME)
> > +
> > +/* Helper to declare a GObject type
> > + *
> > + * Same as SPICE_DECLARE_TYPE_STRUCT but the macro names
> > + * will have the "RED_" prefix.
> > + */
> > +#define SPICE_DECLARE_RED_TYPE_STRUCT(ModuleObjType, ModuleObjName,
> > module_obj_name, OBJ_NAME) \
> > +SPICE_DECLARE_TYPE_GENERIC(ModuleObjType, ModuleObjName,
> > module_obj_name, RED_, OBJ_NAME)
> > +
> > +/* Helper to declare a GObject type
> > + *
> > + * Same as SPICE_DECLARE_TYPE_STRUCT but the structure is the same
> > + * as the type, most declarations use this convention.
> > + */
> > +#define SPICE_DECLARE_TYPE(ModuleObjName, module_obj_name, OBJ_NAME) \
> > +SPICE_DECLARE_TYPE_GENERIC(struct ModuleObjName, ModuleObjName,
> > module_obj_name, \
> > + SPICE_DECLARE_EMPTY_MODULE, OBJ_NAME)
> > +
> > +/* Helper to declare a GObject type
> > + *
> > + * Same as SPICE_DECLARE_TYPE but the macro names
> > + * will have the "RED_" prefix.
> > + */
> > +#define SPICE_DECLARE_RED_TYPE(ModuleObjName, module_obj_name, OBJ_NAME) \
> > +SPICE_DECLARE_TYPE_GENERIC(struct ModuleObjName, ModuleObjName,
> > module_obj_name, RED_, OBJ_NAME)
> > +
> >  #endif /* RED_COMMON_H_ */
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 1/2] Dispatcher: remove async_done callback

[Frediano Ziglio]

> 
> This callback was only executed for message types that were registered
> with DISPATCHER_ASYNC ack type. However, the async_done handler was
> called immediately after the message-specific handler and was called in
> the same thread, so the async_done stuff can just as easily be done from
> within the message-specific handler. This allows to simplify the
> dispatcher_register_handler() method to simply require a boolean
> argument for whether the message type requires an ACK or not.
> ---
> 
> I notice Frediano kept the enum type, but in this patch I changed it to
> a simple boolean. Is there any reason to keep the enum?
> 

Main reason was faster to not change :-)
Somebody could complain that reading the code a "DISPATCHER_ACK" make
understand the purpose more then a "true" but I'm fine with the
true. Note that on main-dispatcher the register is called with
a "0 /* no ack */" parameter. I would surely change (follow up).

>  server/dispatcher.c | 19 +++---
>  server/dispatcher.h | 25 ++
>  server/red-worker.c | 99
>  +
>  3 files changed, 53 insertions(+), 90 deletions(-)
> 
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index 6f2c4d85e..97ebe32a6 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -40,7 +40,7 @@ static void setup_dummy_signal_handler(void);
>  
>  typedef struct DispatcherMessage {
>  size_t size;

OT and paranoid. If we use uint32_t we reduce the structure to
16 bytes (64 bit) which lead to use indexing like [base+16*index]
on Intel so more optimizations.

> -int ack;
> +bool ack;
>  dispatcher_handle_message handler;
>  } DispatcherMessage;
>  
> @@ -60,7 +60,6 @@ struct DispatcherPrivate {
>  void *payload; /* allocated as max of message sizes */
>  size_t payload_size; /* used to track realloc calls */
>  void *opaque;
> -dispatcher_handle_async_done handle_async_done;
>  dispatcher_handle_any_message any_handler;
>  };
>  
> @@ -303,14 +302,12 @@ static int dispatcher_handle_single_read(Dispatcher
> *dispatcher)
>  } else {
>  spice_printerr("error: no handler for message type %d", type);
>  }
> -if (msg->ack == DISPATCHER_ACK) {
> +if (msg->ack) {
>  if (write_safe(dispatcher->priv->recv_fd,
> (uint8_t*), sizeof(ack)) == -1) {
>  spice_printerr("error writing ack for message %d", type);
>  /* TODO: close socketpair? */
>  }
> -} else if (msg->ack == DISPATCHER_ASYNC &&
> dispatcher->priv->handle_async_done) {
> -dispatcher->priv->handle_async_done(dispatcher->priv->opaque, type,
> payload);
>  }
>  return 1;
>  }
> @@ -346,7 +343,7 @@ void dispatcher_send_message(Dispatcher *dispatcher,
> uint32_t message_type,
> message_type);
>  goto unlock;
>  }
> -if (msg->ack == DISPATCHER_ACK) {
> +if (msg->ack) {
>  if (read_safe(send_fd, (uint8_t*), sizeof(ack), 1) == -1) {
>  spice_printerr("error: failed to read ack");
>  } else if (ack != ACK) {
> @@ -359,17 +356,9 @@ unlock:
>  pthread_mutex_unlock(>priv->lock);
>  }
>  
> -void dispatcher_register_async_done_callback(
> -Dispatcher *dispatcher,
> -dispatcher_handle_async_done
> handler)
> -{
> -assert(dispatcher->priv->handle_async_done == NULL);
> -dispatcher->priv->handle_async_done = handler;
> -}
> -
>  void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t
>  message_type,
>   dispatcher_handle_message handler,
> - size_t size, int ack)
> + size_t size, bool ack)
>  {
>  DispatcherMessage *msg;
>  
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index 97b01de9c..eb93c1358 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -72,38 +72,17 @@ typedef void (*dispatcher_handle_async_done)(void
> *opaque,
>  void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
>   void *payload);
>  
> -enum {
> -DISPATCHER_NONE = 0,
> -DISPATCHER_ACK,
> -DISPATCHER_ASYNC
> -};
> -
>  /*
>   * dispatcher_register_handler
>   * @dispatcher: dispatcher
>   * @messsage_type:  message type
>   * @handler:message handler
>   * @size:   message size. Each type has a fixed associated size.
> - * @ack:One of DISPATCHER_NONE, DISPATCHER_ACK,
> DISPATCHER_ASYNC.
> - *  DISPATCHER_NONE - only send the message
> - *  DISPATCHER_ACK - send an ack after the message
> - *  DISPATCHER_ASYNC - call send an ack. This is per message
> type - you can't send the
> - *  same message type with and without. Register two
> different
> - *  messages if that is what you 

Re: [Spice-devel] [PATCH spice-server 2/2] Add documentation for Dispatcher

[Frediano Ziglio]

> 
> ---
>  server/dispatcher.h | 102
>  +---
>  1 file changed, 82 insertions(+), 20 deletions(-)
> 
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index eb93c1358..ed3ecf6bb 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -35,6 +35,18 @@ typedef struct Dispatcher Dispatcher;
>  typedef struct DispatcherClass DispatcherClass;
>  typedef struct DispatcherPrivate DispatcherPrivate;
>  
> +/* A Dispatcher provides inter-thread communication by serializing messages.
> + * Currently the Dispatcher uses a unix socket (socketpair) for dispatching
> the
> + * messages.
> + *
> + * Message types are identified by a unique integer value and must first be
> + * registered with the class (see dispatcher_register_handler()) before they
> + * can be sent. Sending threads can send a message using the
> + * dispatcher_send_message() function. The receiving thread can monitor the
> + * dispatcher's 'receive' file descriptor (see dispatcher_get_recv_fd()) for
> + * activity and should call dispatcher_handle_recv_read() to process
> incoming
> + * messages.
> + */
>  struct Dispatcher
>  {
>  GObject parent;
> @@ -49,31 +61,54 @@ struct DispatcherClass
>  
>  GType dispatcher_get_type(void) G_GNUC_CONST;
>  
> +/* dispatcher_new
> + *
> + * Create a new Dispatcher object
> + *
> + * @max_message_type:   indicates the number of unique message types that
> can
> + *  be handled by this dispatcher. Each message type is
> + *  identified by an integer value between 0 and
> + *  max_message_type-1.
> + * @opaque: an arbitrary pointer that will be passed as the
> first
> + *  argument to any registered handler functions
> + */
>  Dispatcher *dispatcher_new(size_t max_message_type, void *opaque);
>  
>  
> +/* The function signature for handlers of a specific message type */
>  typedef void (*dispatcher_handle_message)(void *opaque,
>void *payload);
>  
> +/* The signature for a function that handles all messages (see
> + * dispatcher_register_universal_handler()) */
>  typedef void (*dispatcher_handle_any_message)(void *opaque,
>uint32_t message_type,
>void *payload);
>  
> -typedef void (*dispatcher_handle_async_done)(void *opaque,
> - uint32_t message_type,
> - void *payload);
> -
> -

This should go to previous patch.

> -/*
> - * dispatcher_send_message
> +/* dispatcher_send_message
> + *
> + * Sends a message to the receiving thread. The message type must have been
> + * registered first (see dispatcher_register_handler()).  @payload must be a
> + * buffer of the same size as the size registered for @message_type
> + *
> + * If the sent message is a message type requires an ACK, this function will
> + * block until it receives an ACK from the receiving thread.
> + *
>   * @message_type: message type
>   * @payload:  payload
>   */
>  void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
>   void *payload);
>  
> -/*
> - * dispatcher_register_handler
> +/* dispatcher_register_handler
> + *
> + * This function registers a message type with the dispatcher, and registers
> + * @handler as the function that will handle incoming messages of this type.
> + * If @ack is true, the dispatcher will also send an ACK in response to the
> + * message after the message has been passed to the handler. You can only
> + * register a given message type once. For example, you cannot register two
> + * different handlers for the same message type with different @ack values.
> + *
>   * @dispatcher: dispatcher
>   * @messsage_type:  message type
>   * @handler:message handler
> @@ -84,32 +119,59 @@ void dispatcher_register_handler(Dispatcher *dispatcher,
> uint32_t message_type,
>   dispatcher_handle_message handler, size_t
>   size,
>   bool ack);
>  
> -/*
> - * Hack to allow red_record to see the message being sent so it can record
> - * it to file.
> +/* dispatcher_register_universal_handler
> + *
> + * Register a universal handler that will be called when *any* message is
> + * received by the dispatcher. When a message is received, this handler will
> be
> + * called first. If the received message type was registered via
> + * dispatcher_register_handler(), the message-specific handler will then be
> + * called. Only one universal handler can be registered. This feature can be
> + * used to record all messages to a file for replay and debugging.
> + *
> + * @dispatcher: dispatcher
> + * @handler:a handler function
>   */
>  void