[RFC,spice-server,1/3] stream-channel: Add preferred video codec capability

Submitted by Kevin Pouget on Aug. 6, 2019, 3:34 p.m.

Details

Message ID 20190806153453.20616-2-kpouget@redhat.com
State Superseded
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Kevin Pouget Aug. 6, 2019, 3:34 p.m.
This patch enables the SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE
capability for the stream-channel.

video_stream_parse_preferred_codecs: new function for parsing the
SPICE protocol message. This code used to be inside
dcc_handle_preferred_video_codec_type.

struct StreamChannelClient::client_preferred_video_codecs: new field.

Signed-off-by: Kevin Pouget <kpouget@redhat.com>
---
 server/dcc.c            | 30 +-----------------------------
 server/stream-channel.c | 40 ++++++++++++++++++++++++++++++++++++++++
 server/video-stream.c   | 36 ++++++++++++++++++++++++++++++++++++
 server/video-stream.h   |  1 +
 4 files changed, 78 insertions(+), 29 deletions(-)

--
2.21.0

Patch hide | download patch | download mbox

diff --git a/server/dcc.c b/server/dcc.c
index 21e8598e..538f1f51 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1158,38 +1158,10 @@  static void on_display_video_codecs_update(GObject *gobject, GParamSpec *pspec,
 static int dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,
                                                  SpiceMsgcDisplayPreferredVideoCodecType *msg)
 {
-    gint i, len;
-    gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
-    GArray *client;
-
     g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);

-    /* set default to a big and positive number */
-    memset(indexes, 0x7f, sizeof(indexes));
-
-    for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
-        gint video_codec = msg->codecs[i];
-
-        if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
-            video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
-            spice_debug("Client has sent unknown video-codec (value %d at index %d). "
-                        "Ignoring as server can't handle it",
-                         video_codec, i);
-            continue;
-        }
-
-        if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
-            continue;
-        }
-
-        len++;
-        indexes[video_codec] = len;
-    }
-    client = g_array_sized_new(FALSE, FALSE, sizeof(gint), SPICE_VIDEO_CODEC_TYPE_ENUM_END);
-    g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
-
     g_clear_pointer(&dcc->priv->client_preferred_video_codecs, g_array_unref);
-    dcc->priv->client_preferred_video_codecs = client;
+    dcc->priv->client_preferred_video_codecs = video_stream_parse_preferred_codecs(msg);

     /* New client preference */
     dcc_update_preferred_video_codecs(dcc);
diff --git a/server/stream-channel.c b/server/stream-channel.c
index 7953018e..fa4804f1 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -22,6 +22,7 @@ 
 #include <spice/stream-device.h>

 #include "red-channel-client.h"
+#include "red-client.h"
 #include "stream-channel.h"
 #include "reds.h"
 #include "common-graphics-channel.h"
@@ -52,6 +53,9 @@  struct StreamChannelClient {
     /* current video stream id, <0 if not initialized or
      * we are not sending a stream */
     int stream_id;
+    /* Array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements, with the client
+     * preference order (index) as value */
+    GArray *client_preferred_video_codecs;
 };

 struct StreamChannelClientClass {
@@ -114,6 +118,8 @@  typedef struct StreamDataItem {
 #define PRIMARY_SURFACE_ID 0

 static void stream_channel_client_on_disconnect(RedChannelClient *rcc);
+static bool stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
+                                                      SpiceMsgcDisplayPreferredVideoCodecType *msg);

 RECORDER(stream_channel_data, 32, "Stream channel data packet");

@@ -324,6 +330,10 @@  handle_message(RedChannelClient *rcc, uint16_t type, uint32_t size, void *msg)
     case SPICE_MSGC_DISPLAY_GL_DRAW_DONE:
         /* client should not send this message */
         return false;
+    case SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE:
+        return stream_channel_handle_preferred_video_codec_type(rcc,
+            (SpiceMsgcDisplayPreferredVideoCodecType *)msg);
+        return false;
     default:
         return red_channel_client_handle_message(rcc, type, size, msg);
     }
@@ -390,6 +400,20 @@  stream_channel_get_supported_codecs(StreamChannel *channel, uint8_t *out_codecs)
     return num;
 }

+static bool
+stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
+                                                 SpiceMsgcDisplayPreferredVideoCodecType *msg)
+{
+    StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
+
+    g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
+
+    g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
+    scc->client_preferred_video_codecs = video_stream_parse_preferred_codecs(msg);
+
+    return TRUE;
+}
+
 static void
 stream_channel_connect(RedChannel *red_channel, RedClient *red_client, RedStream *stream,
                        int migration, RedChannelCapabilities *caps)
@@ -448,10 +472,25 @@  stream_channel_constructed(GObject *object)

     red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
     red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
+    red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE);

     reds_register_channel(reds, red_channel);
 }

+static void
+stream_channel_finalize(GObject *object)
+{
+    StreamChannel *self = STREAM_CHANNEL(object);
+    RedChannelClient *rcc;
+
+    FOREACH_CLIENT(self, rcc) {
+        StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
+        g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
+    }
+
+    G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
+}
+
 static void
 stream_channel_class_init(StreamChannelClass *klass)
 {
@@ -459,6 +498,7 @@  stream_channel_class_init(StreamChannelClass *klass)
     RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);

     object_class->constructed = stream_channel_constructed;
+    object_class->finalize = stream_channel_finalize;

     channel_class->parser = spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
     channel_class->handle_message = handle_message;
diff --git a/server/video-stream.c b/server/video-stream.c
index 6aa859a0..a61c8ab2 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -20,6 +20,7 @@ 
 #include "display-channel-private.h"
 #include "main-channel-client.h"
 #include "red-client.h"
+#include "stream-channel.h"

 #define FPS_TEST_INTERVAL 1
 #define FOREACH_STREAMS(display, item)                  \
@@ -468,6 +469,41 @@  static bool video_stream_add_frame(DisplayChannel *display,
     return FALSE;
 }

+/* Returns an array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements,
+ * with the client preference order (index) as value */
+GArray *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType *msg)
+{
+    gint i, len;
+    gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
+    GArray *client;
+
+    /* set default to a big and positive number */
+    memset(indexes, 0x7f, sizeof(indexes));
+
+    for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
+        gint video_codec = msg->codecs[i];
+
+        if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
+            video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
+            spice_debug("Client has sent unknown video-codec (value %d at index %d). "
+                        "Ignoring as server can't handle it",
+                         video_codec, i);
+            continue;
+        }
+
+        if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
+            continue;
+        }
+
+        len++;
+        indexes[video_codec] = len;
+    }
+    client = g_array_sized_new(FALSE, FALSE, sizeof(gint), SPICE_VIDEO_CODEC_TYPE_ENUM_END);
+    g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
+
+    return client;
+}
+
 /* TODO: document the difference between the 2 functions below */
 void video_stream_trace_update(DisplayChannel *display, Drawable *drawable)
 {
diff --git a/server/video-stream.h b/server/video-stream.h
index 46b076fd..73435515 100644
--- a/server/video-stream.h
+++ b/server/video-stream.h
@@ -147,6 +147,7 @@  void video_stream_detach_and_stop(DisplayChannel *display);
 void video_stream_trace_add_drawable(DisplayChannel *display, Drawable *item);
 void video_stream_detach_behind(DisplayChannel *display, QRegion *region,
                                 Drawable *drawable);
+GArray *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType *msg);

 void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent *agent);
 void video_stream_agent_stop(VideoStreamAgent *agent);

Comments

> 
> This patch enables the SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE
> capability for the stream-channel.
> 
> video_stream_parse_preferred_codecs: new function for parsing the
> SPICE protocol message. This code used to be inside
> dcc_handle_preferred_video_codec_type.
> 
> struct StreamChannelClient::client_preferred_video_codecs: new field.
> 
> Signed-off-by: Kevin Pouget <kpouget@redhat.com>
> ---
>  server/dcc.c            | 30 +-----------------------------
>  server/stream-channel.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  server/video-stream.c   | 36 ++++++++++++++++++++++++++++++++++++
>  server/video-stream.h   |  1 +
>  4 files changed, 78 insertions(+), 29 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index 21e8598e..538f1f51 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -1158,38 +1158,10 @@ static void on_display_video_codecs_update(GObject
> *gobject, GParamSpec *pspec,
>  static int dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,
>                                                   SpiceMsgcDisplayPreferredVideoCodecType
>                                                   *msg)
>  {
> -    gint i, len;
> -    gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
> -    GArray *client;
> -
>      g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
> 
> -    /* set default to a big and positive number */
> -    memset(indexes, 0x7f, sizeof(indexes));
> -
> -    for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
> -        gint video_codec = msg->codecs[i];
> -
> -        if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
> -            video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> -            spice_debug("Client has sent unknown video-codec (value %d at
> index %d). "
> -                        "Ignoring as server can't handle it",
> -                         video_codec, i);
> -            continue;
> -        }
> -
> -        if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> -            continue;
> -        }
> -
> -        len++;
> -        indexes[video_codec] = len;
> -    }
> -    client = g_array_sized_new(FALSE, FALSE, sizeof(gint),
> SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> -    g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> -
>      g_clear_pointer(&dcc->priv->client_preferred_video_codecs,
>      g_array_unref);
> -    dcc->priv->client_preferred_video_codecs = client;
> +    dcc->priv->client_preferred_video_codecs =
> video_stream_parse_preferred_codecs(msg);
> 
>      /* New client preference */
>      dcc_update_preferred_video_codecs(dcc);
> diff --git a/server/stream-channel.c b/server/stream-channel.c
> index 7953018e..fa4804f1 100644
> --- a/server/stream-channel.c
> +++ b/server/stream-channel.c
> @@ -22,6 +22,7 @@
>  #include <spice/stream-device.h>
> 
>  #include "red-channel-client.h"
> +#include "red-client.h"

Why red-client ?? Should not be video-stream.h ?

>  #include "stream-channel.h"
>  #include "reds.h"
>  #include "common-graphics-channel.h"
> @@ -52,6 +53,9 @@ struct StreamChannelClient {
>      /* current video stream id, <0 if not initialized or
>       * we are not sending a stream */
>      int stream_id;
> +    /* Array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements, with the client
> +     * preference order (index) as value */
> +    GArray *client_preferred_video_codecs;

I think that currently a static array takes less space, but it's just my
paranoia.

>  };
> 
>  struct StreamChannelClientClass {
> @@ -114,6 +118,8 @@ typedef struct StreamDataItem {
>  #define PRIMARY_SURFACE_ID 0
> 
>  static void stream_channel_client_on_disconnect(RedChannelClient *rcc);
> +static bool
> stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
> +
> SpiceMsgcDisplayPreferredVideoCodecType
> *msg);
> 

Minor: Not correctly indented

>  RECORDER(stream_channel_data, 32, "Stream channel data packet");
> 
> @@ -324,6 +330,10 @@ handle_message(RedChannelClient *rcc, uint16_t type,
> uint32_t size, void *msg)
>      case SPICE_MSGC_DISPLAY_GL_DRAW_DONE:
>          /* client should not send this message */
>          return false;
> +    case SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE:
> +        return stream_channel_handle_preferred_video_codec_type(rcc,
> +            (SpiceMsgcDisplayPreferredVideoCodecType *)msg);
> +        return false;

Second return after a return is not much useful, some code analysers
could also complain about it.

>      default:
>          return red_channel_client_handle_message(rcc, type, size, msg);
>      }
> @@ -390,6 +400,20 @@ stream_channel_get_supported_codecs(StreamChannel
> *channel, uint8_t *out_codecs)
>      return num;
>  }
> 
> +static bool
> +stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
> +
> SpiceMsgcDisplayPreferredVideoCodecType
> *msg)
> +{
> +    StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
> +
> +    g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);

I know it's not a regression, but document for g_return_val_if_fail states
"Verifies that the expression expr , usually representing a precondition, evaluates to TRUE"
but this is not a precondition, this is checking a value from network.

> +
> +    g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
> +    scc->client_preferred_video_codecs =
> video_stream_parse_preferred_codecs(msg);
> +
> +    return TRUE;

I would use true/false with bool

> +}
> +
>  static void
>  stream_channel_connect(RedChannel *red_channel, RedClient *red_client,
>  RedStream *stream,
>                         int migration, RedChannelCapabilities *caps)
> @@ -448,10 +472,25 @@ stream_channel_constructed(GObject *object)
> 
>      red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>      red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> +    red_channel_set_cap(red_channel,
> SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE);
> 
>      reds_register_channel(reds, red_channel);
>  }
> 
> +static void
> +stream_channel_finalize(GObject *object)
> +{
> +    StreamChannel *self = STREAM_CHANNEL(object);
> +    RedChannelClient *rcc;
> +
> +    FOREACH_CLIENT(self, rcc) {
> +        StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
> +        g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
> +    }

Each object should free itself, this should be in
stream_channel_client_finalize, otherwise code will leak when client disconnects.

> +
> +    G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
> +}
> +
>  static void
>  stream_channel_class_init(StreamChannelClass *klass)
>  {
> @@ -459,6 +498,7 @@ stream_channel_class_init(StreamChannelClass *klass)
>      RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> 
>      object_class->constructed = stream_channel_constructed;
> +    object_class->finalize = stream_channel_finalize;
> 
>      channel_class->parser =
>      spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
>      channel_class->handle_message = handle_message;
> diff --git a/server/video-stream.c b/server/video-stream.c
> index 6aa859a0..a61c8ab2 100644
> --- a/server/video-stream.c
> +++ b/server/video-stream.c
> @@ -20,6 +20,7 @@
>  #include "display-channel-private.h"
>  #include "main-channel-client.h"
>  #include "red-client.h"
> +#include "stream-channel.h"
> 

Why this?

>  #define FPS_TEST_INTERVAL 1
>  #define FOREACH_STREAMS(display, item)                  \
> @@ -468,6 +469,41 @@ static bool video_stream_add_frame(DisplayChannel
> *display,
>      return FALSE;
>  }
> 
> +/* Returns an array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements,
> + * with the client preference order (index) as value */
> +GArray
> *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType
> *msg)
> +{
> +    gint i, len;

gnot ga gbig gfun gof gall gthese gg gstrings

> +    gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
> +    GArray *client;
> +
> +    /* set default to a big and positive number */
> +    memset(indexes, 0x7f, sizeof(indexes));
> +
> +    for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
> +        gint video_codec = msg->codecs[i];
> +
> +        if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
> +            video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> +            spice_debug("Client has sent unknown video-codec (value %d at
> index %d). "
> +                        "Ignoring as server can't handle it",
> +                         video_codec, i);
> +            continue;
> +        }
> +
> +        if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> +            continue;
> +        }
> +
> +        len++;
> +        indexes[video_codec] = len;
> +    }
> +    client = g_array_sized_new(FALSE, FALSE, sizeof(gint),
> SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> +    g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> +
> +    return client;
> +}
> +
>  /* TODO: document the difference between the 2 functions below */
>  void video_stream_trace_update(DisplayChannel *display, Drawable *drawable)
>  {
> diff --git a/server/video-stream.h b/server/video-stream.h
> index 46b076fd..73435515 100644
> --- a/server/video-stream.h
> +++ b/server/video-stream.h
> @@ -147,6 +147,7 @@ void video_stream_detach_and_stop(DisplayChannel
> *display);
>  void video_stream_trace_add_drawable(DisplayChannel *display, Drawable
>  *item);
>  void video_stream_detach_behind(DisplayChannel *display, QRegion *region,
>                                  Drawable *drawable);
> +GArray
> *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType
> *msg);
> 
>  void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent
>  *agent);
>  void video_stream_agent_stop(VideoStreamAgent *agent);

Frediano

On Fri, Aug 23, 2019 at 11:32 AM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> On Wed, Aug 14, 2019 at 09:08:51AM -0400, Frediano Ziglio wrote:
> > > +/* Returns an array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements,
> > > + * with the client preference order (index) as value */
> > > +GArray
> > > *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType
> > > *msg)
> > > +{
> > > +    gint i, len;
> >
> > gnot ga gbig gfun gof gall gthese gg gstrings
>
> Haha, you brought something like that in the past too. What can
> we do about it? I don't even think much, if I'm working on code
> with glib/gobject I g-prefix types too. I would say it makes
> sense when interacting with the its libraries, random e.g:
>
>     glong g_utf8_strlen (const gchar *p, gssize max);
>
> I'm raising this question/email so we can improve this somehow
> (checkpatch.pl git-hooks and/or coding style standards) and make
> it clear for contributors/reviewers without much time loss in
> discussion.

 Hello,

for this particular functions (and overall this patch), I didn't write
anything new, everything is copied or moved from the display-channel
preferred codec code, so these gints are already in the codebase!
but besides that, I agree that it should be used only to interact with
glib functions, which is not the case for this gint i, and a few
others in this function! after a quick glance, I think multiple g*
types are wrongly used; I'll fix that when I'll update the patch.

regards,

Kevin
Hello,

On Wed, Aug 14, 2019 at 3:08 PM Frediano Ziglio <fziglio@redhat.com> wrote:
>
> >
> > This patch enables the SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE
> > capability for the stream-channel.
> >
> > video_stream_parse_preferred_codecs: new function for parsing the
> > SPICE protocol message. This code used to be inside
> > dcc_handle_preferred_video_codec_type.
> >
> > struct StreamChannelClient::client_preferred_video_codecs: new field.
> >
> > Signed-off-by: Kevin Pouget <kpouget@redhat.com>
> > ---
> >  server/dcc.c            | 30 +-----------------------------
> >  server/stream-channel.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  server/video-stream.c   | 36 ++++++++++++++++++++++++++++++++++++
> >  server/video-stream.h   |  1 +
> >  4 files changed, 78 insertions(+), 29 deletions(-)
> >
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 21e8598e..538f1f51 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -1158,38 +1158,10 @@ static void on_display_video_codecs_update(GObject
> > *gobject, GParamSpec *pspec,
> >  static int dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,
> >                                                   SpiceMsgcDisplayPreferredVideoCodecType
> >                                                   *msg)
> >  {
> > -    gint i, len;
> > -    gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
> > -    GArray *client;
> > -
> >      g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
> >
> > -    /* set default to a big and positive number */
> > -    memset(indexes, 0x7f, sizeof(indexes));
> > -
> > -    for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
> > -        gint video_codec = msg->codecs[i];
> > -
> > -        if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
> > -            video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> > -            spice_debug("Client has sent unknown video-codec (value %d at
> > index %d). "
> > -                        "Ignoring as server can't handle it",
> > -                         video_codec, i);
> > -            continue;
> > -        }
> > -
> > -        if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> > -            continue;
> > -        }
> > -
> > -        len++;
> > -        indexes[video_codec] = len;
> > -    }
> > -    client = g_array_sized_new(FALSE, FALSE, sizeof(gint),
> > SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > -    g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > -
> >      g_clear_pointer(&dcc->priv->client_preferred_video_codecs,
> >      g_array_unref);
> > -    dcc->priv->client_preferred_video_codecs = client;
> > +    dcc->priv->client_preferred_video_codecs =
> > video_stream_parse_preferred_codecs(msg);
> >
> >      /* New client preference */
> >      dcc_update_preferred_video_codecs(dcc);
> > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > index 7953018e..fa4804f1 100644
> > --- a/server/stream-channel.c
> > +++ b/server/stream-channel.c
> > @@ -22,6 +22,7 @@
> >  #include <spice/stream-device.h>
> >
> >  #include "red-channel-client.h"
> > +#include "red-client.h"
>
> Why red-client ?? Should not be video-stream.h ?

this include allows me to convert this "reds = red_client_get_server(client)"

> stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc, ...) {
>    RedClient *client = red_channel_client_get_client(rcc);
>    RedsState *reds = red_client_get_server(client);
>    main_dispatcher_reset_stream_channel(reds_get_main_dispatcher(reds), channel_id);

> >  #include "stream-channel.h"
> >  #include "reds.h"
> >  #include "common-graphics-channel.h"
> > @@ -52,6 +53,9 @@ struct StreamChannelClient {
> >      /* current video stream id, <0 if not initialized or
> >       * we are not sending a stream */
> >      int stream_id;
> > +    /* Array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements, with the client
> > +     * preference order (index) as value */
> > +    GArray *client_preferred_video_codecs;
>
> I think that currently a static array takes less space, but it's just my
> paranoia.

you mean to change the type to "SpiceVideoCodecType
__name__[SPICE_VIDEO_CODEC_TYPE_ENUM_END]"?
yes, could be, but I think what ever we change on this side, we should
change it on host-encoding side, for consistency.
I'm not sure it's worth the effort for saving a few bytes of glib wrapper?

> >  };
> >
> >  struct StreamChannelClientClass {
> > @@ -114,6 +118,8 @@ typedef struct StreamDataItem {
> >  #define PRIMARY_SURFACE_ID 0
> >
> >  static void stream_channel_client_on_disconnect(RedChannelClient *rcc);
> > +static bool
> > stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
> > +
> > SpiceMsgcDisplayPreferredVideoCodecType
> > *msg);
> >
>
> Minor: Not correctly indented

this was on purpose, as the "correct" alignment of 'msg' would go > 100chars
I put a new line between "<return type> <function name>"

> >  RECORDER(stream_channel_data, 32, "Stream channel data packet");
> >
> > @@ -324,6 +330,10 @@ handle_message(RedChannelClient *rcc, uint16_t type,
> > uint32_t size, void *msg)
> >      case SPICE_MSGC_DISPLAY_GL_DRAW_DONE:
> >          /* client should not send this message */
> >          return false;
> > +    case SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE:
> > +        return stream_channel_handle_preferred_video_codec_type(rcc,
> > +            (SpiceMsgcDisplayPreferredVideoCodecType *)msg);
> > +        return false;
>
> Second return after a return is not much useful, some code analysers
> could also complain about it.

yes, this is fully useless, I cancelled the second return

> >      default:
> >          return red_channel_client_handle_message(rcc, type, size, msg);
> >      }
> > @@ -390,6 +400,20 @@ stream_channel_get_supported_codecs(StreamChannel
> > *channel, uint8_t *out_codecs)
> >      return num;
> >  }
> >
> > +static bool
> > +stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
> > +
> > SpiceMsgcDisplayPreferredVideoCodecType
> > *msg)
> > +{
> > +    StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
> > +
> > +    g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
>
> I know it's not a regression, but document for g_return_val_if_fail states
> "Verifies that the expression expr , usually representing a precondition, evaluates to TRUE"
> but this is not a precondition, this is checking a value from network.

I don't know, it actually looks like a precondition to me, even if
it's a RPC call.
but the type of num_of_codecs is uint8_t, so >=0.

I can replace this with
> if (msg->num_of_codecs == 0) {
>     return true;
> }

> > +
> > +    g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
> > +    scc->client_preferred_video_codecs =
> > video_stream_parse_preferred_codecs(msg);
> > +
> > +    return TRUE;
>
> I would use true/false with bool

sure

> > +}
> > +
> >  static void
> >  stream_channel_connect(RedChannel *red_channel, RedClient *red_client,
> >  RedStream *stream,
> >                         int migration, RedChannelCapabilities *caps)
> > @@ -448,10 +472,25 @@ stream_channel_constructed(GObject *object)
> >
> >      red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> >      red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> > +    red_channel_set_cap(red_channel,
> > SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE);
> >
> >      reds_register_channel(reds, red_channel);
> >  }
> >
> > +static void
> > +stream_channel_finalize(GObject *object)
> > +{
> > +    StreamChannel *self = STREAM_CHANNEL(object);
> > +    RedChannelClient *rcc;
> > +
> > +    FOREACH_CLIENT(self, rcc) {
> > +        StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
> > +        g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
> > +    }
>
> Each object should free itself, this should be in
> stream_channel_client_finalize, otherwise code will leak when client disconnects.

I see, I confused myself with the channel_class vs channel_client_class.
Fixed now

> > +
> > +    G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
> > +}
> > +
> >  static void
> >  stream_channel_class_init(StreamChannelClass *klass)
> >  {
> > @@ -459,6 +498,7 @@ stream_channel_class_init(StreamChannelClass *klass)
> >      RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> >
> >      object_class->constructed = stream_channel_constructed;
> > +    object_class->finalize = stream_channel_finalize;
> >
> >      channel_class->parser =
> >      spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
> >      channel_class->handle_message = handle_message;
> > diff --git a/server/video-stream.c b/server/video-stream.c
> > index 6aa859a0..a61c8ab2 100644
> > --- a/server/video-stream.c
> > +++ b/server/video-stream.c
> > @@ -20,6 +20,7 @@
> >  #include "display-channel-private.h"
> >  #include "main-channel-client.h"
> >  #include "red-client.h"
> > +#include "stream-channel.h"
> >
>
> Why this?

removed

> >  #define FPS_TEST_INTERVAL 1
> >  #define FOREACH_STREAMS(display, item)                  \
> > @@ -468,6 +469,41 @@ static bool video_stream_add_frame(DisplayChannel
> > *display,
> >      return FALSE;
> >  }
> >
> > +/* Returns an array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements,
> > + * with the client preference order (index) as value */
> > +GArray
> > *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType
> > *msg)
> > +{
> > +    gint i, len;
>
> gnot ga gbig gfun gof gall gthese gg gstrings

fixed, this is a code moved from elsewhere, but it's worth fixing the
types as glib is not involved with this variables

> GArray *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType *msg)
> {
>    int i, len;
>    int indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
>    GArray *client;
>
>    /* set default to a big and positive number */
>    memset(indexes, 0x7f, sizeof(indexes));
>
>    for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
>        SpiceVideoCodecType video_codec = msg->codecs[i];
>
>        if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
>            video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
>            spice_debug("Client has sent unknown video-codec (value %d at index %d). "
>                        "Ignoring as server can't handle it",
>                         video_codec, i);
>            continue;
>        }
>
>        if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
>            continue;
>        }
>
>        len++;
>        indexes[video_codec] = len;
>    }
>    client = g_array_sized_new(FALSE, FALSE, sizeof(int), SPICE_VIDEO_CODEC_TYPE_ENUM_END);
>    g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
>
>    return client;
> }

---

but now, I'm not sure what to do with this Preferred Video Codec list.
This patch only sets the StreamChannelClient attribute:

> scc->client_preferred_video_codecs = video_stream_parse_preferred_codecs(msg);

so it should be discussed in with this email:

> [RFC spice-server 3/3] streaming: Restart guest video streams on video-codec changes


thanks,

Kevin

> > +    gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
> > +    GArray *client;
> > +
> > +    /* set default to a big and positive number */
> > +    memset(indexes, 0x7f, sizeof(indexes));
> > +
> > +    for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
> > +        gint video_codec = msg->codecs[i];
> > +
> > +        if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
> > +            video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> > +            spice_debug("Client has sent unknown video-codec (value %d at
> > index %d). "
> > +                        "Ignoring as server can't handle it",
> > +                         video_codec, i);
> > +            continue;
> > +        }
> > +
> > +        if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> > +            continue;
> > +        }
> > +
> > +        len++;
> > +        indexes[video_codec] = len;
> > +    }
> > +    client = g_array_sized_new(FALSE, FALSE, sizeof(gint),
> > SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > +    g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > +
> > +    return client;
> > +}
> > +
> >  /* TODO: document the difference between the 2 functions below */
> >  void video_stream_trace_update(DisplayChannel *display, Drawable *drawable)
> >  {
> > diff --git a/server/video-stream.h b/server/video-stream.h
> > index 46b076fd..73435515 100644
> > --- a/server/video-stream.h
> > +++ b/server/video-stream.h
> > @@ -147,6 +147,7 @@ void video_stream_detach_and_stop(DisplayChannel
> > *display);
> >  void video_stream_trace_add_drawable(DisplayChannel *display, Drawable
> >  *item);
> >  void video_stream_detach_behind(DisplayChannel *display, QRegion *region,
> >                                  Drawable *drawable);
> > +GArray
> > *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType
> > *msg);
> >
> >  void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent
> >  *agent);
> >  void video_stream_agent_stop(VideoStreamAgent *agent);
>
> Frediano