[Spice-devel,7/7] server: Add VP8 support, a video codec preference list and compatibility checks with the Spice client. (take 2)

Submitted by Francois Gouget on May 27, 2015, 1:13 p.m.

Details

Message ID alpine.DEB.2.11.1505271425540.25575@amboise
State New
Headers show

Not browsing as part of any series.

Commit Message

Francois Gouget May 27, 2015, 1:13 p.m.
The video encoder preferences can be expressed by building a semi-colon separated list of encoder:codec pairs. For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer VP8 video encoder first and used the original MJPEG video encoder one as a fallback.
The server has a default preference list which can also be selected by specifying 'auto' as the preferences list.
The client compatibility check relies on the following new capabilities:
 * SPICE_DISPLAY_CAP_MULTI_CODEC which denotes a recent client that supports multiple codecs. This capability is needed to not have to hardcode that MJPEG is supported. This makes it possible to write clients that don't support MJPEG.
 * SPICE_DISPLAY_CAP_CODEC_XXX, where XXX is a supported codec, for now MJPEG and VP8.
---

Changes since take 1:
- Only set the parameters supported by the current encoder.
- reconfigure_pipeline() handling has been fixed in the previous patch.
- The video codecs list now has a more sensible default and it's 
  possible to explicitly request this default by specifying 'auto' in 
  the configuration files.

The buffer timestamping is unchanged because I don't see a better 
solution at this time. 

Note that the xf86-video-qxl, spice-gtk and spice-html5 patches work 
just fine with this new series so I'm not reposting them to limit 
redundant emails. If this series can go in then I can resubmit them. For 
now, anyone who needs them for testing is invited to grab them there:

* [PATCH qxl 7/11] Add SpiceVideoCodecs and $XSPICE_VIDEO_CODECS for specifying video codec preferences.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019770.html

* [PATCH qemu 08/11] Add the ability to specify Spice video codecs.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019771.html

* [PATCH spice-gtk 9/11] Add the ability to use gstreamer for vp8 decoding.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019772.html

* [PATCH spice-html5 10/11] Revise the webm files to more correctly identify audio tracks.
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019773.html

* [PATCH spice-html5 11/12] Add support for the vp8 codec type.
  (this is really patch 11/11)
  http://lists.freedesktop.org/archives/spice-devel/2015-May/019774.html



 server/gstreamer_encoder.c |  68 ++++++++++++++-----
 server/mjpeg_encoder.c     |   6 +-
 server/red_dispatcher.c    | 165 ++++++++++++++++++++++++++++++++++++++++-----
 server/red_dispatcher.h    |   8 +++
 server/red_worker.c        |  62 ++++++++++++++---
 server/red_worker.h        |  18 +++++
 server/reds.c              |  12 ++++
 server/spice-server.h      |   1 +
 server/spice-server.syms   |   1 +
 server/video_encoder.h     |  18 +++--
 10 files changed, 312 insertions(+), 47 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
index 15ddd82..01b4f5d 100644
--- a/server/gstreamer_encoder.c
+++ b/server/gstreamer_encoder.c
@@ -42,6 +42,7 @@  typedef struct {
 
 struct GstEncoder {
     VideoEncoder base;
+    const gchar* gstenc_name;
 
     /* The GStreamer pipeline. If pipeline is NULL then the other pointers are
      * invalid.
@@ -139,11 +140,11 @@  static void adjust_bit_rate(GstEncoder *encoder)
     {
         /* Even MJPEG should achieve good quality with a 10x compression level */
         encoder->bit_rate = raw_bit_rate / 10;
-        spice_debug("capping the bit rate to %.2fMbps for a 10x compression level", ((double)encoder->bit_rate) / 1024 / 1024);
+        spice_debug("capping the %s bit rate to %.2fMbps for a 10x compression level", encoder->gstenc_name, ((double)encoder->bit_rate) / 1024 / 1024);
     }
     else
     {
-        spice_debug("setting the bit rate to %.2fMbps for a %dx compression level", ((double)encoder->bit_rate) / 1024 / 1024, compression);
+        spice_debug("setting the %s bit rate to %.2fMbps for a %dx compression level", encoder->gstenc_name, ((double)encoder->bit_rate) / 1024 / 1024, compression);
     }
 }
 
@@ -177,9 +178,12 @@  static int construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
 {
     GstStateChangeReturn ret;
     GError *err;
+    gchar *desc;
 
     err = NULL;
-    encoder->pipeline = gst_parse_launch_full("appsrc name=src is-live=1 ! autovideoconvert ! ffenc_mjpeg name=encoder ! appsink name=sink", NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
+    desc = g_strdup_printf("appsrc name=src is-live=1 ! autovideoconvert ! %s name=encoder ! appsink name=sink", encoder->gstenc_name);
+    encoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
+    g_free(desc);
     if (!encoder->pipeline)
     {
         spice_warning("GStreamer error: %s", err->message);
@@ -190,15 +194,21 @@  static int construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
     encoder->gstenc = gst_bin_get_by_name(GST_BIN(encoder->pipeline), "encoder");
     encoder->appsink = GST_APP_SINK(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "sink"));
 
+    /* Set the encoder initial bit rate, and ask for a low latency */
+    adjust_bit_rate(encoder);
+    g_object_set(G_OBJECT(encoder->gstenc), "bitrate", encoder->bit_rate, NULL);
+    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+        g_object_set(G_OBJECT(encoder->gstenc),
+                     "max-latency", 0, NULL,
+                     "max-keyframe-distance", 0,
+                     "lag-in-frames", 0,
+                     NULL);
+
     /* Set the source caps */
     encoder->src_caps = NULL;
     if (!set_appsrc_caps(encoder))
         return FALSE;
 
-    /* Set the encoder initial bit rate */
-    adjust_bit_rate(encoder);
-    g_object_set(G_OBJECT(encoder->gstenc), "bitrate", encoder->bit_rate, NULL);
-
     /* Start playing */
     gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
     ret = gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING);
@@ -212,9 +222,13 @@  static int construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
 
 static void reconfigure_pipeline(GstEncoder *encoder)
 {
-    if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE ||
-        !set_appsrc_caps(encoder) ||
-        gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
+    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
+        /* vp8enc gets confused if we try to reconfigure the pipeline */
+        reset_pipeline(encoder);
+    }
+    else if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE ||
+             !set_appsrc_caps(encoder) ||
+             gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
         spice_debug("GStreamer error: the pipeline reconfiguration failed, rebuilding it instead");
         reset_pipeline(encoder); /* we can rebuild it... */
     }
@@ -248,7 +262,7 @@  static inline uint8_t *get_image_line(SpiceChunks *chunks, size_t *offset,
 
 
 static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
-                          const SpiceRect *src, int top_down)
+                          const SpiceRect *src, int top_down, uint32_t frame_time)
 {
     SpiceChunks *chunks;
     uint32_t image_stride;
@@ -296,8 +310,17 @@  static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
     /* The GStreamer buffer timestamps and framerate are irrelevant and would
      * be hard to set right because they can arrive a bit irregularly
      */
-    GST_BUFFER_TIMESTAMP(buffer) = GST_CLOCK_TIME_NONE;
-    GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
+    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
+    {
+        /* FIXME: Maybe try drop-frame = 0 instead? */
+        GST_BUFFER_TIMESTAMP(buffer) = frame_time;
+        GST_BUFFER_DURATION(buffer) = 1;
+    }
+    else
+    {
+        GST_BUFFER_TIMESTAMP(buffer) = GST_CLOCK_TIME_NONE;
+        GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
+    }
     GST_BUFFER_OFFSET(buffer) = encoder->frame++;
     gst_buffer_set_caps(buffer, encoder->src_caps);
 
@@ -368,7 +391,7 @@  static int gst_encoder_encode_frame(GstEncoder *encoder,
     if (!encoder->pipeline && !construct_pipeline(encoder, bitmap))
         return VIDEO_ENCODER_FRAME_DROP;
 
-    rc = push_raw_frame(encoder, bitmap, src, top_down);
+    rc = push_raw_frame(encoder, bitmap, src, top_down, frame_mm_time);
     if (rc == VIDEO_ENCODER_FRAME_ENCODE_DONE)
         rc = pull_compressed_buffer(encoder, outbuf, outbuf_size, data_size);
     return rc;
@@ -411,11 +434,24 @@  void gst_encoder_get_stats(GstEncoder *encoder, VideoEncoderStats *stats)
         stats->avg_quality = 0;
 }
 
-GstEncoder *create_gstreamer_encoder(uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
+GstEncoder *create_gstreamer_encoder(SpiceVideoCodecType codec_type, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
 {
     GstEncoder *encoder;
+    const gchar *gstenc_name;
 
     spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
+    switch (codec_type)
+    {
+    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
+        gstenc_name = "ffenc_mjpeg";
+        break;
+    case SPICE_VIDEO_CODEC_TYPE_VP8:
+        gstenc_name = "vp8enc";
+        break;
+    default:
+        spice_warning("unsupported codec type %d", codec_type);
+        return NULL;
+    }
 
     gst_init(NULL, NULL);
 
@@ -426,6 +462,8 @@  GstEncoder *create_gstreamer_encoder(uint64_t starting_bit_rate, VideoEncoderRat
     encoder->base.notify_server_frame_drop = &gst_encoder_notify_server_frame_drop;
     encoder->base.get_bit_rate = &gst_encoder_get_bit_rate;
     encoder->base.get_stats = &gst_encoder_get_stats;
+    encoder->base.codec_type = codec_type;
+    encoder->gstenc_name = gstenc_name;
     encoder->pipeline = NULL;
 
     if (cbs)
diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index d734520..479e120 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -210,7 +210,8 @@  static inline int rate_control_is_active(MJpegEncoder* encoder)
     return encoder->cbs.get_roundtrip_ms != NULL;
 }
 
-MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
+MJpegEncoder *create_mjpeg_encoder(SpiceVideoCodecType codec_type,
+                                   uint64_t starting_bit_rate,
                                    VideoEncoderRateControlCbs *cbs,
                                    void *cbs_opaque)
 {
@@ -218,6 +219,8 @@  MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
 
     spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
 
+    if (codec_type != SPICE_VIDEO_CODEC_TYPE_MJPEG)
+        return NULL;
     enc = spice_new0(MJpegEncoder, 1);
 
     enc->base.destroy = &mjpeg_encoder_destroy;
@@ -226,6 +229,7 @@  MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
     enc->base.notify_server_frame_drop = &mjpeg_encoder_notify_server_frame_drop;
     enc->base.get_bit_rate = &mjpeg_encoder_get_bit_rate;
     enc->base.get_stats = &mjpeg_encoder_get_stats;
+    enc->base.codec_type = codec_type;
     enc->first_frame = TRUE;
     enc->rate_control.byte_rate = starting_bit_rate / 8;
     enc->starting_bit_rate = starting_bit_rate;
diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index d8389bc..e8a1347 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -27,6 +27,7 @@ 
 #include <sys/socket.h>
 #include <signal.h>
 #include <inttypes.h>
+#include <ctype.h>
 
 #include <spice/qxl_dev.h>
 #include "common/quic.h"
@@ -214,43 +215,155 @@  static void red_dispatcher_cursor_migrate(RedChannelClient *rcc)
                             &payload);
 }
 
-typedef struct RendererInfo {
-    int id;
+typedef struct {
+    uint32_t id;
     const char *name;
-} RendererInfo;
+} EnumNames;
 
-static RendererInfo renderers_info[] = {
+static int name_to_enum(const EnumNames names[], const char *string, uint32_t *id)
+{
+    if (string) {
+        int i = 0;
+        while (names[i].name) {
+            if (strcmp(string, names[i].name) == 0)
+            {
+                *id = names[i].id;
+                return TRUE;
+            }
+            i++;
+        }
+    }
+    return FALSE;
+}
+
+static const EnumNames renderer_names[] = {
     {RED_RENDERER_SW, "sw"},
 #ifdef USE_OPENGL
     {RED_RENDERER_OGL_PBUF, "oglpbuf"},
     {RED_RENDERER_OGL_PIXMAP, "oglpixmap"},
 #endif
-    {RED_RENDERER_INVALID, NULL},
+    {0, NULL},
 };
 
 static uint32_t renderers[RED_MAX_RENDERERS];
 static uint32_t num_renderers = 0;
 
-static RendererInfo *find_renderer(const char *name)
+int red_dispatcher_add_renderer(const char *name)
 {
-    RendererInfo *inf = renderers_info;
-    while (inf->name) {
-        if (strcmp(name, inf->name) == 0) {
-            return inf;
+    uint32_t renderer;
+
+    if (num_renderers == RED_MAX_RENDERERS ||
+        !name_to_enum(renderer_names, name, &renderer))
+        return FALSE;
+    renderers[num_renderers++] = renderer;
+    return TRUE;
+}
+
+static const EnumNames video_encoder_names[] = {
+    {0, "spice"},
+    {1, "gstreamer"},
+    {0, NULL},
+};
+
+static create_video_encoder_proc video_encoder_procs[] = {
+    &create_mjpeg_encoder,
+#ifdef HAVE_GSTREAMER_0_10
+    &create_gstreamer_encoder,
+#else
+    NULL,
+#endif
+};
+
+static const EnumNames video_codec_names[] = {
+    {SPICE_VIDEO_CODEC_TYPE_MJPEG, "mjpeg"},
+    {SPICE_VIDEO_CODEC_TYPE_VP8, "vp8"},
+    {0, NULL},
+};
+
+static const EnumNames video_cap_names[] = {
+    {SPICE_DISPLAY_CAP_CODEC_MJPEG, "mjpeg"},
+    {SPICE_DISPLAY_CAP_CODEC_VP8, "vp8"},
+    {0, NULL},
+};
+
+
+static RedVideoCodec video_codecs[RED_MAX_VIDEO_CODECS];
+static int num_video_codecs = -1;
+
+/* Expected string:  encoder:codec;encoder:codec */
+static const char* parse_video_codecs(const char *codecs, char **encoder,
+                                      char **codec)
+{
+    const char* e;
+    while (*codecs == ';')
+        codecs++;
+    if (!*codecs)
+        return NULL;
+    e = codecs;
+    while (isalnum(*codecs) || *codecs == '_')
+        codecs++;
+    if (*codecs == ':')
+    {
+        const char* c = ++codecs;
+        while (isalnum(*codecs) || *codecs == '_')
+            codecs++;
+        if (!*codecs || *codecs == ';')
+        {
+            int len = c - e - 1;
+            *encoder = malloc(len + 1);
+            strncpy(*encoder, e, len);
+            (*encoder)[len] = '\0';
+
+            len =  codecs - c;
+            *codec = malloc(len + 1);
+            strncpy(*codec, c, len);
+            (*codec)[len] = '\0';
+            return codecs;
         }
-        inf++;
     }
+    spice_warning("spice: invalid encoder:codec value: %s", e);
     return NULL;
 }
 
-int red_dispatcher_add_renderer(const char *name)
+int red_dispatcher_set_video_codecs(const char *codecs)
 {
-    RendererInfo *inf;
+    uint32_t encoder;
+    SpiceVideoCodecType type;
+    uint32_t cap;
+    char *encoder_name, *codec_name;
+    static RedVideoCodec new_codecs[RED_MAX_VIDEO_CODECS];
+    int count;
+    const char* c;
 
-    if (num_renderers == RED_MAX_RENDERERS || !(inf = find_renderer(name))) {
-        return FALSE;
+    if (strcmp(codecs, "auto") == 0)
+        codecs = VIDEO_ENCODER_DEFAULT_PREFERENCE;
+
+    c = codecs;
+    count = 0;
+    while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
+        int convert_failed = FALSE;
+        if (count == RED_MAX_VIDEO_CODECS ||
+            !name_to_enum(video_encoder_names, encoder_name, &encoder) ||
+            !name_to_enum(video_codec_names, codec_name, &type) ||
+            !name_to_enum(video_cap_names, codec_name, &cap))
+            convert_failed = TRUE;
+
+        free(encoder_name);
+        free(codec_name);
+
+        if (convert_failed)
+            return FALSE;
+        if (!video_encoder_procs[encoder])
+            /* A valid but unsupported video encoder, skip it */
+            continue;
+
+        new_codecs[count].create = video_encoder_procs[encoder];
+        new_codecs[count].type = type;
+        new_codecs[count].cap = cap;
+        count++;
     }
-    renderers[num_renderers++] = inf->id;
+    num_video_codecs = count;
+    memcpy(video_codecs, new_codecs, sizeof(video_codecs));
     return TRUE;
 }
 
@@ -793,6 +906,22 @@  void red_dispatcher_on_sv_change(void)
     }
 }
 
+void red_dispatcher_on_vc_change(void)
+{
+    RedWorkerMessageSetVideoCodecs payload;
+    int compression_level = calc_compression_level();
+    RedDispatcher *now = dispatchers;
+    while (now) {
+        now->qxl->st->qif->set_compression_level(now->qxl, compression_level);
+        payload.num_video_codecs = num_video_codecs;
+        payload.video_codecs = video_codecs;
+        dispatcher_send_message(&now->dispatcher,
+                                RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
+                                &payload);
+        now = now->next;
+    }
+}
+
 void red_dispatcher_set_mouse_mode(uint32_t mode)
 {
     RedWorkerMessageSetMouseMode payload;
@@ -1094,6 +1223,10 @@  void red_dispatcher_init(QXLInstance *qxl)
     init_data.pending = &red_dispatcher->pending;
     init_data.num_renderers = num_renderers;
     memcpy(init_data.renderers, renderers, sizeof(init_data.renderers));
+    if (num_video_codecs < 0)
+        red_dispatcher_set_video_codecs("auto");
+    init_data.num_video_codecs = num_video_codecs;
+    memcpy(init_data.video_codecs, video_codecs, sizeof(init_data.video_codecs));
 
     pthread_mutex_init(&red_dispatcher->async_lock, NULL);
     init_data.image_compression = image_compression;
diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
index 907b7c7..eb3aee4 100644
--- a/server/red_dispatcher.h
+++ b/server/red_dispatcher.h
@@ -22,6 +22,7 @@ 
 
 struct RedChannelClient;
 struct RedDispatcher;
+typedef struct RedVideoCodec RedVideoCodec;
 typedef struct AsyncCommand AsyncCommand;
 
 void red_dispatcher_init(QXLInstance *qxl);
@@ -29,11 +30,13 @@  void red_dispatcher_init(QXLInstance *qxl);
 void red_dispatcher_set_mm_time(uint32_t);
 void red_dispatcher_on_ic_change(void);
 void red_dispatcher_on_sv_change(void);
+void red_dispatcher_on_vc_change(void);
 void red_dispatcher_set_mouse_mode(uint32_t mode);
 void red_dispatcher_on_vm_stop(void);
 void red_dispatcher_on_vm_start(void);
 int red_dispatcher_count(void);
 int red_dispatcher_add_renderer(const char *name);
+int red_dispatcher_set_video_codecs(const char* codecs);
 uint32_t red_dispatcher_qxl_ram_size(void);
 int red_dispatcher_qxl_count(void);
 void red_dispatcher_async_complete(struct RedDispatcher *, AsyncCommand *);
@@ -174,6 +177,11 @@  typedef struct RedWorkerMessageSetStreamingVideo {
     uint32_t streaming_video;
 } RedWorkerMessageSetStreamingVideo;
 
+typedef struct RedWorkerMessageSetVideoCodecs {
+    uint32_t num_video_codecs;
+    RedVideoCodec* video_codecs;
+} RedWorkerMessageSetVideoCodecs;
+
 typedef struct RedWorkerMessageSetMouseMode {
     uint32_t mode;
 } RedWorkerMessageSetMouseMode;
diff --git a/server/red_worker.c b/server/red_worker.c
index 19e27c5..b8c2259 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1,6 +1,7 @@ 
 /* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
 /*
    Copyright (C) 2009 Red Hat, Inc.
+   Copyright (C) 2015 Francois Gouget
 
    This library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Lesser General Public
@@ -994,6 +995,8 @@  typedef struct RedWorker {
     uint32_t mouse_mode;
 
     uint32_t streaming_video;
+    uint32_t num_video_codecs;
+    RedVideoCodec video_codecs[RED_MAX_VIDEO_CODECS];
     Stream streams_buf[NUM_STREAMS];
     Stream *free_streams;
     Ring streams;
@@ -3074,10 +3077,41 @@  static void red_stream_update_client_playback_latency(void *opaque, uint32_t del
     main_dispatcher_set_mm_time_latency(agent->dcc->common.base.client, agent->dcc->streams_max_latency);
 }
 
+static VideoEncoder* red_display_create_video_encoder(DisplayChannelClient *dcc, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
+{
+    RedWorker* worker = dcc->common.worker;
+    int i;
+    int client_has_multi_codec = red_channel_client_test_remote_cap(&dcc->common.base, SPICE_DISPLAY_CAP_MULTI_CODEC);
+
+    for (i = 0; i < worker->num_video_codecs; i++)
+    {
+        RedVideoCodec* video_codec = &worker->video_codecs[i];
+        VideoEncoder* video_encoder;
+
+        if (!client_has_multi_codec &&
+            video_codec->type != SPICE_VIDEO_CODEC_TYPE_MJPEG)
+            /* Old clients only support MJPEG */
+            continue;
+
+        if (client_has_multi_codec && !red_channel_client_test_remote_cap(&dcc->common.base, video_codec->cap))
+            /* The client is recent but does not support this codec */
+            continue;
+
+        video_encoder = video_codec->create(video_codec->type, starting_bit_rate, cbs, cbs_opaque);
+        if (video_encoder)
+            return video_encoder;
+    }
+
+    /* Try to use the builtin MJPEG video encoder as a fallback */
+    if (!client_has_multi_codec || red_channel_client_test_remote_cap(&dcc->common.base, SPICE_DISPLAY_CAP_CODEC_MJPEG))
+        return create_mjpeg_encoder(SPICE_VIDEO_CODEC_TYPE_MJPEG, starting_bit_rate, cbs, cbs_opaque);
+
+    return NULL;
+}
+
 static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
 {
     StreamAgent *agent = &dcc->stream_agents[get_stream_id(dcc->common.worker, stream)];
-    create_video_encoder_proc create_video_encoder;
 
     stream->refs++;
     spice_assert(region_is_empty(&agent->vis_region));
@@ -3092,12 +3126,6 @@  static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
     agent->fps = MAX_FPS;
     agent->dcc = dcc;
 
-#ifdef HAVE_GSTREAMER_0_10
-    create_video_encoder = &create_gstreamer_encoder;
-#else
-    create_video_encoder = &create_mjpeg_encoder;
-#endif
-
     if (dcc->use_video_encoder_rate_control) {
         VideoEncoderRateControlCbs video_cbs;
         uint64_t initial_bit_rate;
@@ -3107,10 +3135,13 @@  static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
         video_cbs.update_client_playback_delay = red_stream_update_client_playback_latency;
 
         initial_bit_rate = red_stream_get_initial_bit_rate(dcc, stream);
-        agent->video_encoder = create_video_encoder(initial_bit_rate, &video_cbs, agent);
+        agent->video_encoder = red_display_create_video_encoder(dcc, initial_bit_rate, &video_cbs, agent);
     } else {
-        agent->video_encoder = create_video_encoder(0, NULL, NULL);
+        agent->video_encoder = red_display_create_video_encoder(dcc, 0, NULL, NULL);
     }
+    /* FIXME: We may have failed to create a video encoder which will cause
+     *        a crash!
+     */
     red_channel_client_pipe_add(&dcc->common.base, &agent->create_item);
 
     if (red_channel_client_test_remote_cap(&dcc->common.base, SPICE_DISPLAY_CAP_STREAM_REPORT)) {
@@ -8938,7 +8969,7 @@  static void red_display_marshall_stream_start(RedChannelClient *rcc,
     stream_create.surface_id = 0;
     stream_create.id = get_stream_id(dcc->common.worker, stream);
     stream_create.flags = stream->top_down ? SPICE_STREAM_FLAGS_TOP_DOWN : 0;
-    stream_create.codec_type = SPICE_VIDEO_CODEC_TYPE_MJPEG;
+    stream_create.codec_type = agent->video_encoder->codec_type;
 
     stream_create.src_width = stream->width;
     stream_create.src_height = stream->height;
@@ -11729,6 +11760,15 @@  void handle_dev_set_streaming_video(void *opaque, void *payload)
     }
 }
 
+void handle_dev_set_video_codecs(void *opaque, void *payload)
+{
+    RedWorkerMessageSetVideoCodecs *msg = payload;
+    RedWorker *worker = opaque;
+
+    worker->num_video_codecs = msg->num_video_codecs;
+    memcpy(worker->video_codecs, msg->video_codecs, sizeof(worker->video_codecs));
+}
+
 void handle_dev_set_mouse_mode(void *opaque, void *payload)
 {
     RedWorkerMessageSetMouseMode *msg = payload;
@@ -12035,6 +12075,8 @@  static void red_init(RedWorker *worker, WorkerInitData *init_data)
     worker->jpeg_state = init_data->jpeg_state;
     worker->zlib_glz_state = init_data->zlib_glz_state;
     worker->streaming_video = init_data->streaming_video;
+    worker->num_video_codecs = init_data->num_video_codecs;
+    memcpy(worker->video_codecs, init_data->video_codecs, sizeof(worker->video_codecs));
     worker->driver_cap_monitors_config = 0;
     ring_init(&worker->current_list);
     image_cache_init(&worker->image_cache);
diff --git a/server/red_worker.h b/server/red_worker.h
index 272661f..f732a3e 100644
--- a/server/red_worker.h
+++ b/server/red_worker.h
@@ -21,6 +21,7 @@ 
 #include <unistd.h>
 #include <errno.h>
 #include "red_common.h"
+#include "video_encoder.h"
 
 enum {
     RED_WORKER_PENDING_WAKEUP,
@@ -69,6 +70,7 @@  enum {
 
     RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC,
     RED_WORKER_MESSAGE_DRIVER_UNLOAD,
+    RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
 
     RED_WORKER_MESSAGE_COUNT // LAST
 };
@@ -84,6 +86,20 @@  enum {
     RED_RENDERER_OGL_PIXMAP,
 };
 
+#define RED_MAX_VIDEO_CODECS 8
+
+typedef struct RedVideoCodec {
+    create_video_encoder_proc create;
+    SpiceVideoCodecType type;
+    uint32_t cap;
+} RedVideoCodec;
+
+enum {
+    SPICE_STREAMING_INVALID,
+    SPICE_STREAMING_SPICE,
+    SPICE_STREAMING_GSTREAMER
+};
+
 typedef struct RedDispatcher RedDispatcher;
 
 typedef struct WorkerInitData {
@@ -96,6 +112,8 @@  typedef struct WorkerInitData {
     spice_wan_compression_t jpeg_state;
     spice_wan_compression_t zlib_glz_state;
     int streaming_video;
+    uint32_t num_video_codecs;
+    RedVideoCodec video_codecs[RED_MAX_VIDEO_CODECS];
     uint32_t num_memslots;
     uint32_t num_memslots_groups;
     uint8_t memslot_gen_bits;
diff --git a/server/reds.c b/server/reds.c
index 6d70b68..be8f301 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -111,6 +111,7 @@  static int ticketing_enabled = 1; //Ticketing is enabled by default
 static pthread_mutex_t *lock_cs;
 static long *lock_count;
 uint32_t streaming_video = STREAM_VIDEO_FILTER;
+
 spice_image_compression_t image_compression = SPICE_IMAGE_COMPRESS_AUTO_GLZ;
 spice_wan_compression_t jpeg_state = SPICE_WAN_COMPRESSION_AUTO;
 spice_wan_compression_t zlib_glz_state = SPICE_WAN_COMPRESSION_AUTO;
@@ -3671,6 +3672,17 @@  SPICE_GNUC_VISIBLE int spice_server_set_streaming_video(SpiceServer *s, int valu
     return 0;
 }
 
+SPICE_GNUC_VISIBLE int spice_server_set_video_codecs(SpiceServer *s, const char* video_codecs)
+{
+    spice_assert(reds == s);
+
+    if (!red_dispatcher_set_video_codecs(video_codecs))
+        return -1;
+
+    red_dispatcher_on_vc_change();
+    return 0;
+}
+
 SPICE_GNUC_VISIBLE int spice_server_set_playback_compression(SpiceServer *s, int enable)
 {
     spice_assert(reds == s);
diff --git a/server/spice-server.h b/server/spice-server.h
index bca0da6..7d39e2e 100644
--- a/server/spice-server.h
+++ b/server/spice-server.h
@@ -107,6 +107,7 @@  enum {
 };
 
 int spice_server_set_streaming_video(SpiceServer *s, int value);
+int spice_server_set_video_codecs(SpiceServer *s, const char* video_codecs);
 int spice_server_set_playback_compression(SpiceServer *s, int enable);
 int spice_server_set_agent_mouse(SpiceServer *s, int enable);
 int spice_server_set_agent_copypaste(SpiceServer *s, int enable);
diff --git a/server/spice-server.syms b/server/spice-server.syms
index 2193811..9094a26 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -32,6 +32,7 @@  global:
     spice_server_set_playback_compression;
     spice_server_set_port;
     spice_server_set_streaming_video;
+    spice_server_set_video_codecs;
     spice_server_set_ticket;
     spice_server_set_tls;
     spice_server_set_zlib_glz_compression;
diff --git a/server/video_encoder.h b/server/video_encoder.h
index 70547b0..ab20443 100644
--- a/server/video_encoder.h
+++ b/server/video_encoder.h
@@ -119,6 +119,9 @@  struct VideoEncoder {
      *              statistics.
      */
     void (*get_stats)(VIDEO_ENCODER_T *encoder, VideoEncoderStats *stats);
+
+    /* The codec being used by the video encoder */
+    SpiceVideoCodecType codec_type;
 };
 
 
@@ -146,23 +149,28 @@  typedef struct VideoEncoderRateControlCbs {
 } VideoEncoderRateControlCbs;
 
 
-/* Instantiates the builtin MJPEG video encoder.
+/* Instantiates the a video encoder for the specified codec.
  *
- * @starting_bit_rate: An initial estimate of the available stream bit rate .
+ * @codec_type:        The codec to use.
+ * @starting_bit_rate: An initial estimate of the available stream bit rate.
  * @bit_rate_control:  True if the client supports rate control.
  * @cbs:               A set of callback methods to be used for rate control.
  * @cbs_opaque:        A pointer to be passed to the rate control callbacks.
  * @return:            A pointer to a structure implementing the VideoEncoder
  *                     methods.
  */
-typedef VIDEO_ENCODER_T* (*create_video_encoder_proc)(uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque);
+typedef VIDEO_ENCODER_T* (*create_video_encoder_proc)(SpiceVideoCodecType codec_type, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque);
 
-VIDEO_ENCODER_T* create_mjpeg_encoder(uint64_t starting_bit_rate,
+VIDEO_ENCODER_T* create_mjpeg_encoder(SpiceVideoCodecType codec_type,
+                                      uint64_t starting_bit_rate,
                                       VideoEncoderRateControlCbs *cbs,
                                       void *cbs_opaque);
 
-VIDEO_ENCODER_T* create_gstreamer_encoder(uint64_t starting_bit_rate,
+VIDEO_ENCODER_T* create_gstreamer_encoder(SpiceVideoCodecType codec_type,
+                                          uint64_t starting_bit_rate,
                                           VideoEncoderRateControlCbs *cbs,
                                           void *cbs_opaque);
 
+#define VIDEO_ENCODER_DEFAULT_PREFERENCE "spice:mjpeg;gstreamer:mjpeg;gstreamer:vp8"
+
 #endif

Comments

Il 27/05/2015 15:13, Francois Gouget ha scritto:
> The video encoder preferences can be expressed by building a semi-colon separated list of encoder:codec pairs. For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer VP8 video encoder first and used the original MJPEG video encoder one as a fallback.
> The server has a default preference list which can also be selected by specifying 'auto' as the preferences list.
> The client compatibility check relies on the following new capabilities:
>  * SPICE_DISPLAY_CAP_MULTI_CODEC which denotes a recent client that supports multiple codecs. This capability is needed to not have to hardcode that MJPEG is supported. This makes it possible to write clients that don't support MJPEG.
>  * SPICE_DISPLAY_CAP_CODEC_XXX, where XXX is a supported codec, for now MJPEG and VP8.
> ---
>
> Changes since take 1:
> - Only set the parameters supported by the current encoder.
> - reconfigure_pipeline() handling has been fixed in the previous patch.
> - The video codecs list now has a more sensible default and it's 
>   possible to explicitly request this default by specifying 'auto' in 
>   the configuration files.
>
> The buffer timestamping is unchanged because I don't see a better 
> solution at this time. 
>
> Note that the xf86-video-qxl, spice-gtk and spice-html5 patches work 
> just fine with this new series so I'm not reposting them to limit 
> redundant emails. If this series can go in then I can resubmit them. For 
> now, anyone who needs them for testing is invited to grab them there:
>
> * [PATCH qxl 7/11] Add SpiceVideoCodecs and $XSPICE_VIDEO_CODECS for specifying video codec preferences.
>   http://lists.freedesktop.org/archives/spice-devel/2015-May/019770.html
>
> * [PATCH qemu 08/11] Add the ability to specify Spice video codecs.
>   http://lists.freedesktop.org/archives/spice-devel/2015-May/019771.html

Qemu patch have an error:
http://lists.freedesktop.org/archives/spice-devel/2015-May/019901.html

Thanks for this serie update, I'll try probably tomorrow.

>
> * [PATCH spice-gtk 9/11] Add the ability to use gstreamer for vp8 decoding.
>   http://lists.freedesktop.org/archives/spice-devel/2015-May/019772.html
>
> * [PATCH spice-html5 10/11] Revise the webm files to more correctly identify audio tracks.
>   http://lists.freedesktop.org/archives/spice-devel/2015-May/019773.html
>
> * [PATCH spice-html5 11/12] Add support for the vp8 codec type.
>   (this is really patch 11/11)
>   http://lists.freedesktop.org/archives/spice-devel/2015-May/019774.html
>
>
>
>  server/gstreamer_encoder.c |  68 ++++++++++++++-----
>  server/mjpeg_encoder.c     |   6 +-
>  server/red_dispatcher.c    | 165 ++++++++++++++++++++++++++++++++++++++++-----
>  server/red_dispatcher.h    |   8 +++
>  server/red_worker.c        |  62 ++++++++++++++---
>  server/red_worker.h        |  18 +++++
>  server/reds.c              |  12 ++++
>  server/spice-server.h      |   1 +
>  server/spice-server.syms   |   1 +
>  server/video_encoder.h     |  18 +++--
>  10 files changed, 312 insertions(+), 47 deletions(-)
>
> diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
> index 15ddd82..01b4f5d 100644
> --- a/server/gstreamer_encoder.c
> +++ b/server/gstreamer_encoder.c
> @@ -42,6 +42,7 @@ typedef struct {
>  
>  struct GstEncoder {
>      VideoEncoder base;
> +    const gchar* gstenc_name;
>  
>      /* The GStreamer pipeline. If pipeline is NULL then the other pointers are
>       * invalid.
> @@ -139,11 +140,11 @@ static void adjust_bit_rate(GstEncoder *encoder)
>      {
>          /* Even MJPEG should achieve good quality with a 10x compression level */
>          encoder->bit_rate = raw_bit_rate / 10;
> -        spice_debug("capping the bit rate to %.2fMbps for a 10x compression level", ((double)encoder->bit_rate) / 1024 / 1024);
> +        spice_debug("capping the %s bit rate to %.2fMbps for a 10x compression level", encoder->gstenc_name, ((double)encoder->bit_rate) / 1024 / 1024);
>      }
>      else
>      {
> -        spice_debug("setting the bit rate to %.2fMbps for a %dx compression level", ((double)encoder->bit_rate) / 1024 / 1024, compression);
> +        spice_debug("setting the %s bit rate to %.2fMbps for a %dx compression level", encoder->gstenc_name, ((double)encoder->bit_rate) / 1024 / 1024, compression);
>      }
>  }
>  
> @@ -177,9 +178,12 @@ static int construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
>  {
>      GstStateChangeReturn ret;
>      GError *err;
> +    gchar *desc;
>  
>      err = NULL;
> -    encoder->pipeline = gst_parse_launch_full("appsrc name=src is-live=1 ! autovideoconvert ! ffenc_mjpeg name=encoder ! appsink name=sink", NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
> +    desc = g_strdup_printf("appsrc name=src is-live=1 ! autovideoconvert ! %s name=encoder ! appsink name=sink", encoder->gstenc_name);
> +    encoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
> +    g_free(desc);
>      if (!encoder->pipeline)
>      {
>          spice_warning("GStreamer error: %s", err->message);
> @@ -190,15 +194,21 @@ static int construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
>      encoder->gstenc = gst_bin_get_by_name(GST_BIN(encoder->pipeline), "encoder");
>      encoder->appsink = GST_APP_SINK(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "sink"));
>  
> +    /* Set the encoder initial bit rate, and ask for a low latency */
> +    adjust_bit_rate(encoder);
> +    g_object_set(G_OBJECT(encoder->gstenc), "bitrate", encoder->bit_rate, NULL);
> +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> +        g_object_set(G_OBJECT(encoder->gstenc),
> +                     "max-latency", 0, NULL,
> +                     "max-keyframe-distance", 0,
> +                     "lag-in-frames", 0,
> +                     NULL);
> +
>      /* Set the source caps */
>      encoder->src_caps = NULL;
>      if (!set_appsrc_caps(encoder))
>          return FALSE;
>  
> -    /* Set the encoder initial bit rate */
> -    adjust_bit_rate(encoder);
> -    g_object_set(G_OBJECT(encoder->gstenc), "bitrate", encoder->bit_rate, NULL);
> -
>      /* Start playing */
>      gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
>      ret = gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING);
> @@ -212,9 +222,13 @@ static int construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
>  
>  static void reconfigure_pipeline(GstEncoder *encoder)
>  {
> -    if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE ||
> -        !set_appsrc_caps(encoder) ||
> -        gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
> +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> +        /* vp8enc gets confused if we try to reconfigure the pipeline */
> +        reset_pipeline(encoder);
> +    }
> +    else if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE ||
> +             !set_appsrc_caps(encoder) ||
> +             gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
>          spice_debug("GStreamer error: the pipeline reconfiguration failed, rebuilding it instead");
>          reset_pipeline(encoder); /* we can rebuild it... */
>      }
> @@ -248,7 +262,7 @@ static inline uint8_t *get_image_line(SpiceChunks *chunks, size_t *offset,
>  
>  
>  static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
> -                          const SpiceRect *src, int top_down)
> +                          const SpiceRect *src, int top_down, uint32_t frame_time)
>  {
>      SpiceChunks *chunks;
>      uint32_t image_stride;
> @@ -296,8 +310,17 @@ static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
>      /* The GStreamer buffer timestamps and framerate are irrelevant and would
>       * be hard to set right because they can arrive a bit irregularly
>       */
> -    GST_BUFFER_TIMESTAMP(buffer) = GST_CLOCK_TIME_NONE;
> -    GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
> +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8)
> +    {
> +        /* FIXME: Maybe try drop-frame = 0 instead? */
> +        GST_BUFFER_TIMESTAMP(buffer) = frame_time;
> +        GST_BUFFER_DURATION(buffer) = 1;
> +    }
> +    else
> +    {
> +        GST_BUFFER_TIMESTAMP(buffer) = GST_CLOCK_TIME_NONE;
> +        GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
> +    }
>      GST_BUFFER_OFFSET(buffer) = encoder->frame++;
>      gst_buffer_set_caps(buffer, encoder->src_caps);
>  
> @@ -368,7 +391,7 @@ static int gst_encoder_encode_frame(GstEncoder *encoder,
>      if (!encoder->pipeline && !construct_pipeline(encoder, bitmap))
>          return VIDEO_ENCODER_FRAME_DROP;
>  
> -    rc = push_raw_frame(encoder, bitmap, src, top_down);
> +    rc = push_raw_frame(encoder, bitmap, src, top_down, frame_mm_time);
>      if (rc == VIDEO_ENCODER_FRAME_ENCODE_DONE)
>          rc = pull_compressed_buffer(encoder, outbuf, outbuf_size, data_size);
>      return rc;
> @@ -411,11 +434,24 @@ void gst_encoder_get_stats(GstEncoder *encoder, VideoEncoderStats *stats)
>          stats->avg_quality = 0;
>  }
>  
> -GstEncoder *create_gstreamer_encoder(uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
> +GstEncoder *create_gstreamer_encoder(SpiceVideoCodecType codec_type, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
>  {
>      GstEncoder *encoder;
> +    const gchar *gstenc_name;
>  
>      spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
> +    switch (codec_type)
> +    {
> +    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> +        gstenc_name = "ffenc_mjpeg";
> +        break;
> +    case SPICE_VIDEO_CODEC_TYPE_VP8:
> +        gstenc_name = "vp8enc";
> +        break;
> +    default:
> +        spice_warning("unsupported codec type %d", codec_type);
> +        return NULL;
> +    }
>  
>      gst_init(NULL, NULL);
>  
> @@ -426,6 +462,8 @@ GstEncoder *create_gstreamer_encoder(uint64_t starting_bit_rate, VideoEncoderRat
>      encoder->base.notify_server_frame_drop = &gst_encoder_notify_server_frame_drop;
>      encoder->base.get_bit_rate = &gst_encoder_get_bit_rate;
>      encoder->base.get_stats = &gst_encoder_get_stats;
> +    encoder->base.codec_type = codec_type;
> +    encoder->gstenc_name = gstenc_name;
>      encoder->pipeline = NULL;
>  
>      if (cbs)
> diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
> index d734520..479e120 100644
> --- a/server/mjpeg_encoder.c
> +++ b/server/mjpeg_encoder.c
> @@ -210,7 +210,8 @@ static inline int rate_control_is_active(MJpegEncoder* encoder)
>      return encoder->cbs.get_roundtrip_ms != NULL;
>  }
>  
> -MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
> +MJpegEncoder *create_mjpeg_encoder(SpiceVideoCodecType codec_type,
> +                                   uint64_t starting_bit_rate,
>                                     VideoEncoderRateControlCbs *cbs,
>                                     void *cbs_opaque)
>  {
> @@ -218,6 +219,8 @@ MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
>  
>      spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
>  
> +    if (codec_type != SPICE_VIDEO_CODEC_TYPE_MJPEG)
> +        return NULL;
>      enc = spice_new0(MJpegEncoder, 1);
>  
>      enc->base.destroy = &mjpeg_encoder_destroy;
> @@ -226,6 +229,7 @@ MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
>      enc->base.notify_server_frame_drop = &mjpeg_encoder_notify_server_frame_drop;
>      enc->base.get_bit_rate = &mjpeg_encoder_get_bit_rate;
>      enc->base.get_stats = &mjpeg_encoder_get_stats;
> +    enc->base.codec_type = codec_type;
>      enc->first_frame = TRUE;
>      enc->rate_control.byte_rate = starting_bit_rate / 8;
>      enc->starting_bit_rate = starting_bit_rate;
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index d8389bc..e8a1347 100644
> --- a/server/red_dispatcher.c
> +++ b/server/red_dispatcher.c
> @@ -27,6 +27,7 @@
>  #include <sys/socket.h>
>  #include <signal.h>
>  #include <inttypes.h>
> +#include <ctype.h>
>  
>  #include <spice/qxl_dev.h>
>  #include "common/quic.h"
> @@ -214,43 +215,155 @@ static void red_dispatcher_cursor_migrate(RedChannelClient *rcc)
>                              &payload);
>  }
>  
> -typedef struct RendererInfo {
> -    int id;
> +typedef struct {
> +    uint32_t id;
>      const char *name;
> -} RendererInfo;
> +} EnumNames;
>  
> -static RendererInfo renderers_info[] = {
> +static int name_to_enum(const EnumNames names[], const char *string, uint32_t *id)
> +{
> +    if (string) {
> +        int i = 0;
> +        while (names[i].name) {
> +            if (strcmp(string, names[i].name) == 0)
> +            {
> +                *id = names[i].id;
> +                return TRUE;
> +            }
> +            i++;
> +        }
> +    }
> +    return FALSE;
> +}
> +
> +static const EnumNames renderer_names[] = {
>      {RED_RENDERER_SW, "sw"},
>  #ifdef USE_OPENGL
>      {RED_RENDERER_OGL_PBUF, "oglpbuf"},
>      {RED_RENDERER_OGL_PIXMAP, "oglpixmap"},
>  #endif
> -    {RED_RENDERER_INVALID, NULL},
> +    {0, NULL},
>  };
>  
>  static uint32_t renderers[RED_MAX_RENDERERS];
>  static uint32_t num_renderers = 0;
>  
> -static RendererInfo *find_renderer(const char *name)
> +int red_dispatcher_add_renderer(const char *name)
>  {
> -    RendererInfo *inf = renderers_info;
> -    while (inf->name) {
> -        if (strcmp(name, inf->name) == 0) {
> -            return inf;
> +    uint32_t renderer;
> +
> +    if (num_renderers == RED_MAX_RENDERERS ||
> +        !name_to_enum(renderer_names, name, &renderer))
> +        return FALSE;
> +    renderers[num_renderers++] = renderer;
> +    return TRUE;
> +}
> +
> +static const EnumNames video_encoder_names[] = {
> +    {0, "spice"},
> +    {1, "gstreamer"},
> +    {0, NULL},
> +};
> +
> +static create_video_encoder_proc video_encoder_procs[] = {
> +    &create_mjpeg_encoder,
> +#ifdef HAVE_GSTREAMER_0_10
> +    &create_gstreamer_encoder,
> +#else
> +    NULL,
> +#endif
> +};
> +
> +static const EnumNames video_codec_names[] = {
> +    {SPICE_VIDEO_CODEC_TYPE_MJPEG, "mjpeg"},
> +    {SPICE_VIDEO_CODEC_TYPE_VP8, "vp8"},
> +    {0, NULL},
> +};
> +
> +static const EnumNames video_cap_names[] = {
> +    {SPICE_DISPLAY_CAP_CODEC_MJPEG, "mjpeg"},
> +    {SPICE_DISPLAY_CAP_CODEC_VP8, "vp8"},
> +    {0, NULL},
> +};
> +
> +
> +static RedVideoCodec video_codecs[RED_MAX_VIDEO_CODECS];
> +static int num_video_codecs = -1;
> +
> +/* Expected string:  encoder:codec;encoder:codec */
> +static const char* parse_video_codecs(const char *codecs, char **encoder,
> +                                      char **codec)
> +{
> +    const char* e;
> +    while (*codecs == ';')
> +        codecs++;
> +    if (!*codecs)
> +        return NULL;
> +    e = codecs;
> +    while (isalnum(*codecs) || *codecs == '_')
> +        codecs++;
> +    if (*codecs == ':')
> +    {
> +        const char* c = ++codecs;
> +        while (isalnum(*codecs) || *codecs == '_')
> +            codecs++;
> +        if (!*codecs || *codecs == ';')
> +        {
> +            int len = c - e - 1;
> +            *encoder = malloc(len + 1);
> +            strncpy(*encoder, e, len);
> +            (*encoder)[len] = '\0';
> +
> +            len =  codecs - c;
> +            *codec = malloc(len + 1);
> +            strncpy(*codec, c, len);
> +            (*codec)[len] = '\0';
> +            return codecs;
>          }
> -        inf++;
>      }
> +    spice_warning("spice: invalid encoder:codec value: %s", e);
>      return NULL;
>  }
>  
> -int red_dispatcher_add_renderer(const char *name)
> +int red_dispatcher_set_video_codecs(const char *codecs)
>  {
> -    RendererInfo *inf;
> +    uint32_t encoder;
> +    SpiceVideoCodecType type;
> +    uint32_t cap;
> +    char *encoder_name, *codec_name;
> +    static RedVideoCodec new_codecs[RED_MAX_VIDEO_CODECS];
> +    int count;
> +    const char* c;
>  
> -    if (num_renderers == RED_MAX_RENDERERS || !(inf = find_renderer(name))) {
> -        return FALSE;
> +    if (strcmp(codecs, "auto") == 0)
> +        codecs = VIDEO_ENCODER_DEFAULT_PREFERENCE;
> +
> +    c = codecs;
> +    count = 0;
> +    while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
> +        int convert_failed = FALSE;
> +        if (count == RED_MAX_VIDEO_CODECS ||
> +            !name_to_enum(video_encoder_names, encoder_name, &encoder) ||
> +            !name_to_enum(video_codec_names, codec_name, &type) ||
> +            !name_to_enum(video_cap_names, codec_name, &cap))
> +            convert_failed = TRUE;
> +
> +        free(encoder_name);
> +        free(codec_name);
> +
> +        if (convert_failed)
> +            return FALSE;
> +        if (!video_encoder_procs[encoder])
> +            /* A valid but unsupported video encoder, skip it */
> +            continue;
> +
> +        new_codecs[count].create = video_encoder_procs[encoder];
> +        new_codecs[count].type = type;
> +        new_codecs[count].cap = cap;
> +        count++;
>      }
> -    renderers[num_renderers++] = inf->id;
> +    num_video_codecs = count;
> +    memcpy(video_codecs, new_codecs, sizeof(video_codecs));
>      return TRUE;
>  }
>  
> @@ -793,6 +906,22 @@ void red_dispatcher_on_sv_change(void)
>      }
>  }
>  
> +void red_dispatcher_on_vc_change(void)
> +{
> +    RedWorkerMessageSetVideoCodecs payload;
> +    int compression_level = calc_compression_level();
> +    RedDispatcher *now = dispatchers;
> +    while (now) {
> +        now->qxl->st->qif->set_compression_level(now->qxl, compression_level);
> +        payload.num_video_codecs = num_video_codecs;
> +        payload.video_codecs = video_codecs;
> +        dispatcher_send_message(&now->dispatcher,
> +                                RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
> +                                &payload);
> +        now = now->next;
> +    }
> +}
> +
>  void red_dispatcher_set_mouse_mode(uint32_t mode)
>  {
>      RedWorkerMessageSetMouseMode payload;
> @@ -1094,6 +1223,10 @@ void red_dispatcher_init(QXLInstance *qxl)
>      init_data.pending = &red_dispatcher->pending;
>      init_data.num_renderers = num_renderers;
>      memcpy(init_data.renderers, renderers, sizeof(init_data.renderers));
> +    if (num_video_codecs < 0)
> +        red_dispatcher_set_video_codecs("auto");
> +    init_data.num_video_codecs = num_video_codecs;
> +    memcpy(init_data.video_codecs, video_codecs, sizeof(init_data.video_codecs));
>  
>      pthread_mutex_init(&red_dispatcher->async_lock, NULL);
>      init_data.image_compression = image_compression;
> diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> index 907b7c7..eb3aee4 100644
> --- a/server/red_dispatcher.h
> +++ b/server/red_dispatcher.h
> @@ -22,6 +22,7 @@
>  
>  struct RedChannelClient;
>  struct RedDispatcher;
> +typedef struct RedVideoCodec RedVideoCodec;
>  typedef struct AsyncCommand AsyncCommand;
>  
>  void red_dispatcher_init(QXLInstance *qxl);
> @@ -29,11 +30,13 @@ void red_dispatcher_init(QXLInstance *qxl);
>  void red_dispatcher_set_mm_time(uint32_t);
>  void red_dispatcher_on_ic_change(void);
>  void red_dispatcher_on_sv_change(void);
> +void red_dispatcher_on_vc_change(void);
>  void red_dispatcher_set_mouse_mode(uint32_t mode);
>  void red_dispatcher_on_vm_stop(void);
>  void red_dispatcher_on_vm_start(void);
>  int red_dispatcher_count(void);
>  int red_dispatcher_add_renderer(const char *name);
> +int red_dispatcher_set_video_codecs(const char* codecs);
>  uint32_t red_dispatcher_qxl_ram_size(void);
>  int red_dispatcher_qxl_count(void);
>  void red_dispatcher_async_complete(struct RedDispatcher *, AsyncCommand *);
> @@ -174,6 +177,11 @@ typedef struct RedWorkerMessageSetStreamingVideo {
>      uint32_t streaming_video;
>  } RedWorkerMessageSetStreamingVideo;
>  
> +typedef struct RedWorkerMessageSetVideoCodecs {
> +    uint32_t num_video_codecs;
> +    RedVideoCodec* video_codecs;
> +} RedWorkerMessageSetVideoCodecs;
> +
>  typedef struct RedWorkerMessageSetMouseMode {
>      uint32_t mode;
>  } RedWorkerMessageSetMouseMode;
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 19e27c5..b8c2259 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -1,6 +1,7 @@
>  /* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
>  /*
>     Copyright (C) 2009 Red Hat, Inc.
> +   Copyright (C) 2015 Francois Gouget
>  
>     This library is free software; you can redistribute it and/or
>     modify it under the terms of the GNU Lesser General Public
> @@ -994,6 +995,8 @@ typedef struct RedWorker {
>      uint32_t mouse_mode;
>  
>      uint32_t streaming_video;
> +    uint32_t num_video_codecs;
> +    RedVideoCodec video_codecs[RED_MAX_VIDEO_CODECS];
>      Stream streams_buf[NUM_STREAMS];
>      Stream *free_streams;
>      Ring streams;
> @@ -3074,10 +3077,41 @@ static void red_stream_update_client_playback_latency(void *opaque, uint32_t del
>      main_dispatcher_set_mm_time_latency(agent->dcc->common.base.client, agent->dcc->streams_max_latency);
>  }
>  
> +static VideoEncoder* red_display_create_video_encoder(DisplayChannelClient *dcc, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
> +{
> +    RedWorker* worker = dcc->common.worker;
> +    int i;
> +    int client_has_multi_codec = red_channel_client_test_remote_cap(&dcc->common.base, SPICE_DISPLAY_CAP_MULTI_CODEC);
> +
> +    for (i = 0; i < worker->num_video_codecs; i++)
> +    {
> +        RedVideoCodec* video_codec = &worker->video_codecs[i];
> +        VideoEncoder* video_encoder;
> +
> +        if (!client_has_multi_codec &&
> +            video_codec->type != SPICE_VIDEO_CODEC_TYPE_MJPEG)
> +            /* Old clients only support MJPEG */
> +            continue;
> +
> +        if (client_has_multi_codec && !red_channel_client_test_remote_cap(&dcc->common.base, video_codec->cap))
> +            /* The client is recent but does not support this codec */
> +            continue;
> +
> +        video_encoder = video_codec->create(video_codec->type, starting_bit_rate, cbs, cbs_opaque);
> +        if (video_encoder)
> +            return video_encoder;
> +    }
> +
> +    /* Try to use the builtin MJPEG video encoder as a fallback */
> +    if (!client_has_multi_codec || red_channel_client_test_remote_cap(&dcc->common.base, SPICE_DISPLAY_CAP_CODEC_MJPEG))
> +        return create_mjpeg_encoder(SPICE_VIDEO_CODEC_TYPE_MJPEG, starting_bit_rate, cbs, cbs_opaque);
> +
> +    return NULL;
> +}
> +
>  static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
>  {
>      StreamAgent *agent = &dcc->stream_agents[get_stream_id(dcc->common.worker, stream)];
> -    create_video_encoder_proc create_video_encoder;
>  
>      stream->refs++;
>      spice_assert(region_is_empty(&agent->vis_region));
> @@ -3092,12 +3126,6 @@ static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
>      agent->fps = MAX_FPS;
>      agent->dcc = dcc;
>  
> -#ifdef HAVE_GSTREAMER_0_10
> -    create_video_encoder = &create_gstreamer_encoder;
> -#else
> -    create_video_encoder = &create_mjpeg_encoder;
> -#endif
> -
>      if (dcc->use_video_encoder_rate_control) {
>          VideoEncoderRateControlCbs video_cbs;
>          uint64_t initial_bit_rate;
> @@ -3107,10 +3135,13 @@ static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
>          video_cbs.update_client_playback_delay = red_stream_update_client_playback_latency;
>  
>          initial_bit_rate = red_stream_get_initial_bit_rate(dcc, stream);
> -        agent->video_encoder = create_video_encoder(initial_bit_rate, &video_cbs, agent);
> +        agent->video_encoder = red_display_create_video_encoder(dcc, initial_bit_rate, &video_cbs, agent);
>      } else {
> -        agent->video_encoder = create_video_encoder(0, NULL, NULL);
> +        agent->video_encoder = red_display_create_video_encoder(dcc, 0, NULL, NULL);
>      }
> +    /* FIXME: We may have failed to create a video encoder which will cause
> +     *        a crash!
> +     */
>      red_channel_client_pipe_add(&dcc->common.base, &agent->create_item);
>  
>      if (red_channel_client_test_remote_cap(&dcc->common.base, SPICE_DISPLAY_CAP_STREAM_REPORT)) {
> @@ -8938,7 +8969,7 @@ static void red_display_marshall_stream_start(RedChannelClient *rcc,
>      stream_create.surface_id = 0;
>      stream_create.id = get_stream_id(dcc->common.worker, stream);
>      stream_create.flags = stream->top_down ? SPICE_STREAM_FLAGS_TOP_DOWN : 0;
> -    stream_create.codec_type = SPICE_VIDEO_CODEC_TYPE_MJPEG;
> +    stream_create.codec_type = agent->video_encoder->codec_type;
>  
>      stream_create.src_width = stream->width;
>      stream_create.src_height = stream->height;
> @@ -11729,6 +11760,15 @@ void handle_dev_set_streaming_video(void *opaque, void *payload)
>      }
>  }
>  
> +void handle_dev_set_video_codecs(void *opaque, void *payload)
> +{
> +    RedWorkerMessageSetVideoCodecs *msg = payload;
> +    RedWorker *worker = opaque;
> +
> +    worker->num_video_codecs = msg->num_video_codecs;
> +    memcpy(worker->video_codecs, msg->video_codecs, sizeof(worker->video_codecs));
> +}
> +
>  void handle_dev_set_mouse_mode(void *opaque, void *payload)
>  {
>      RedWorkerMessageSetMouseMode *msg = payload;
> @@ -12035,6 +12075,8 @@ static void red_init(RedWorker *worker, WorkerInitData *init_data)
>      worker->jpeg_state = init_data->jpeg_state;
>      worker->zlib_glz_state = init_data->zlib_glz_state;
>      worker->streaming_video = init_data->streaming_video;
> +    worker->num_video_codecs = init_data->num_video_codecs;
> +    memcpy(worker->video_codecs, init_data->video_codecs, sizeof(worker->video_codecs));
>      worker->driver_cap_monitors_config = 0;
>      ring_init(&worker->current_list);
>      image_cache_init(&worker->image_cache);
> diff --git a/server/red_worker.h b/server/red_worker.h
> index 272661f..f732a3e 100644
> --- a/server/red_worker.h
> +++ b/server/red_worker.h
> @@ -21,6 +21,7 @@
>  #include <unistd.h>
>  #include <errno.h>
>  #include "red_common.h"
> +#include "video_encoder.h"
>  
>  enum {
>      RED_WORKER_PENDING_WAKEUP,
> @@ -69,6 +70,7 @@ enum {
>  
>      RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC,
>      RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> +    RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
>  
>      RED_WORKER_MESSAGE_COUNT // LAST
>  };
> @@ -84,6 +86,20 @@ enum {
>      RED_RENDERER_OGL_PIXMAP,
>  };
>  
> +#define RED_MAX_VIDEO_CODECS 8
> +
> +typedef struct RedVideoCodec {
> +    create_video_encoder_proc create;
> +    SpiceVideoCodecType type;
> +    uint32_t cap;
> +} RedVideoCodec;
> +
> +enum {
> +    SPICE_STREAMING_INVALID,
> +    SPICE_STREAMING_SPICE,
> +    SPICE_STREAMING_GSTREAMER
> +};
> +
>  typedef struct RedDispatcher RedDispatcher;
>  
>  typedef struct WorkerInitData {
> @@ -96,6 +112,8 @@ typedef struct WorkerInitData {
>      spice_wan_compression_t jpeg_state;
>      spice_wan_compression_t zlib_glz_state;
>      int streaming_video;
> +    uint32_t num_video_codecs;
> +    RedVideoCodec video_codecs[RED_MAX_VIDEO_CODECS];
>      uint32_t num_memslots;
>      uint32_t num_memslots_groups;
>      uint8_t memslot_gen_bits;
> diff --git a/server/reds.c b/server/reds.c
> index 6d70b68..be8f301 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -111,6 +111,7 @@ static int ticketing_enabled = 1; //Ticketing is enabled by default
>  static pthread_mutex_t *lock_cs;
>  static long *lock_count;
>  uint32_t streaming_video = STREAM_VIDEO_FILTER;
> +
>  spice_image_compression_t image_compression = SPICE_IMAGE_COMPRESS_AUTO_GLZ;
>  spice_wan_compression_t jpeg_state = SPICE_WAN_COMPRESSION_AUTO;
>  spice_wan_compression_t zlib_glz_state = SPICE_WAN_COMPRESSION_AUTO;
> @@ -3671,6 +3672,17 @@ SPICE_GNUC_VISIBLE int spice_server_set_streaming_video(SpiceServer *s, int valu
>      return 0;
>  }
>  
> +SPICE_GNUC_VISIBLE int spice_server_set_video_codecs(SpiceServer *s, const char* video_codecs)
> +{
> +    spice_assert(reds == s);
> +
> +    if (!red_dispatcher_set_video_codecs(video_codecs))
> +        return -1;
> +
> +    red_dispatcher_on_vc_change();
> +    return 0;
> +}
> +
>  SPICE_GNUC_VISIBLE int spice_server_set_playback_compression(SpiceServer *s, int enable)
>  {
>      spice_assert(reds == s);
> diff --git a/server/spice-server.h b/server/spice-server.h
> index bca0da6..7d39e2e 100644
> --- a/server/spice-server.h
> +++ b/server/spice-server.h
> @@ -107,6 +107,7 @@ enum {
>  };
>  
>  int spice_server_set_streaming_video(SpiceServer *s, int value);
> +int spice_server_set_video_codecs(SpiceServer *s, const char* video_codecs);
>  int spice_server_set_playback_compression(SpiceServer *s, int enable);
>  int spice_server_set_agent_mouse(SpiceServer *s, int enable);
>  int spice_server_set_agent_copypaste(SpiceServer *s, int enable);
> diff --git a/server/spice-server.syms b/server/spice-server.syms
> index 2193811..9094a26 100644
> --- a/server/spice-server.syms
> +++ b/server/spice-server.syms
> @@ -32,6 +32,7 @@ global:
>      spice_server_set_playback_compression;
>      spice_server_set_port;
>      spice_server_set_streaming_video;
> +    spice_server_set_video_codecs;
>      spice_server_set_ticket;
>      spice_server_set_tls;
>      spice_server_set_zlib_glz_compression;
> diff --git a/server/video_encoder.h b/server/video_encoder.h
> index 70547b0..ab20443 100644
> --- a/server/video_encoder.h
> +++ b/server/video_encoder.h
> @@ -119,6 +119,9 @@ struct VideoEncoder {
>       *              statistics.
>       */
>      void (*get_stats)(VIDEO_ENCODER_T *encoder, VideoEncoderStats *stats);
> +
> +    /* The codec being used by the video encoder */
> +    SpiceVideoCodecType codec_type;
>  };
>  
>  
> @@ -146,23 +149,28 @@ typedef struct VideoEncoderRateControlCbs {
>  } VideoEncoderRateControlCbs;
>  
>  
> -/* Instantiates the builtin MJPEG video encoder.
> +/* Instantiates the a video encoder for the specified codec.
>   *
> - * @starting_bit_rate: An initial estimate of the available stream bit rate .
> + * @codec_type:        The codec to use.
> + * @starting_bit_rate: An initial estimate of the available stream bit rate.
>   * @bit_rate_control:  True if the client supports rate control.
>   * @cbs:               A set of callback methods to be used for rate control.
>   * @cbs_opaque:        A pointer to be passed to the rate control callbacks.
>   * @return:            A pointer to a structure implementing the VideoEncoder
>   *                     methods.
>   */
> -typedef VIDEO_ENCODER_T* (*create_video_encoder_proc)(uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque);
> +typedef VIDEO_ENCODER_T* (*create_video_encoder_proc)(SpiceVideoCodecType codec_type, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque);
>  
> -VIDEO_ENCODER_T* create_mjpeg_encoder(uint64_t starting_bit_rate,
> +VIDEO_ENCODER_T* create_mjpeg_encoder(SpiceVideoCodecType codec_type,
> +                                      uint64_t starting_bit_rate,
>                                        VideoEncoderRateControlCbs *cbs,
>                                        void *cbs_opaque);
>  
> -VIDEO_ENCODER_T* create_gstreamer_encoder(uint64_t starting_bit_rate,
> +VIDEO_ENCODER_T* create_gstreamer_encoder(SpiceVideoCodecType codec_type,
> +                                          uint64_t starting_bit_rate,
>                                            VideoEncoderRateControlCbs *cbs,
>                                            void *cbs_opaque);
>  
> +#define VIDEO_ENCODER_DEFAULT_PREFERENCE "spice:mjpeg;gstreamer:mjpeg;gstreamer:vp8"
> +
>  #endif
>> * [PATCH qemu 08/11] Add the ability to specify Spice video codecs.
>>   http://lists.freedesktop.org/archives/spice-devel/2015-May/019771.html
> 
> Qemu patch have an error:
> http://lists.freedesktop.org/archives/spice-devel/2015-May/019901.html

Yes, thanks.  There is an error with spice-html5 as well.

I think I'll plan to resubmit once the server parts are in, and my
patches can be smaller sets.

Cheers,

Jeremy