[Spice-devel,v6,08/26] server: Add VP8 support to the GStreamer encoder

Submitted by Francois Gouget on Oct. 14, 2015, 3:32 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Francois Gouget Oct. 14, 2015, 3:32 p.m.
The Spice server administrator can specify the preferred encoder and
codec preferences to optimize for CPU or bandwidth usage. Preferences
are described in a semi-colon separated list of encoder:codec pairs.
For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer
VP8 video encoder and the original MJPEG video encoder as a fallback.
The server has a default preference list which can also be selected by
specifying 'auto' as the preferences list.

The server then picks a codec supported by the client based on the
following new client 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.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 configure.ac               |   4 ++
 server/gstreamer_encoder.c |  80 +++++++++++++++++++---
 server/mjpeg_encoder.c     |   5 +-
 server/red_dispatcher.c    | 165 ++++++++++++++++++++++++++++++++++++++++-----
 server/red_dispatcher.h    |   8 +++
 server/red_worker.c        |  80 ++++++++++++++++++----
 server/red_worker.h        |  18 +++++
 server/reds.c              |  12 ++++
 server/spice-server.h      |   1 +
 server/spice-server.syms   |   1 +
 server/video_encoder.h     |  14 +++-
 11 files changed, 345 insertions(+), 43 deletions(-)

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index 76e2ad7..3c1882f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -141,6 +141,10 @@  AC_SUBST([SPICE_PROTOCOL_MIN_VER])
 PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.22])
 AS_VAR_APPEND([SPICE_REQUIRES], [" glib-2.0 >= 2.22"])
 
+AC_CHECK_LIB(glib-2.0, g_get_num_processors,
+             AC_DEFINE([HAVE_G_GET_NUMPROCESSORS], 1, [Defined if we have g_get_num_processors()]),,
+             $GLIB2_LIBS)
+
 PKG_CHECK_MODULES(PIXMAN, pixman-1 >= 0.17.7)
 AC_SUBST(PIXMAN_CFLAGS)
 AC_SUBST(PIXMAN_LIBS)
diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
index bf66e99..765b31b 100644
--- a/server/gstreamer_encoder.c
+++ b/server/gstreamer_encoder.c
@@ -184,13 +184,33 @@  static void set_appsrc_caps(SpiceGstEncoder *encoder)
 static gboolean construct_pipeline(SpiceGstEncoder *encoder,
                                    const SpiceBitmap *bitmap)
 {
+    const gchar* gstenc_name;
+    switch (encoder->base.codec_type)
+    {
+    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
+        gstenc_name = "avenc_mjpeg";
+        break;
+    case SPICE_VIDEO_CODEC_TYPE_VP8:
+        gstenc_name = "vp8enc";
+        break;
+    default:
+        /* gstreamer_encoder_new() should have rejected this codec type */
+        spice_warning("unsupported codec type %d", encoder->base.codec_type);
+        return FALSE;
+    }
+
     GError *err = NULL;
-    const gchar *desc = "appsrc name=src format=2 do-timestamp=true ! videoconvert ! avenc_mjpeg name=encoder ! appsink name=sink";
+    gchar *desc = g_strdup_printf("appsrc name=src format=2 do-timestamp=true ! videoconvert ! %s name=encoder ! appsink name=sink", gstenc_name);
     spice_debug("GStreamer pipeline: %s", desc);
     encoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
+    g_free(desc);
     if (!encoder->pipeline || err) {
         spice_warning("GStreamer error: %s", err->message);
         g_clear_error(&err);
+        if (encoder->pipeline) {
+            gst_object_unref(encoder->pipeline);
+            encoder->pipeline = NULL;
+        }
         return FALSE;
     }
     encoder->appsrc = GST_APP_SRC(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "src"));
@@ -199,17 +219,46 @@  static gboolean construct_pipeline(SpiceGstEncoder *encoder,
 
     /* Configure the encoder bitrate, frame latency, etc. */
     adjust_bit_rate(encoder);
-    g_object_set(G_OBJECT(encoder->gstenc),
-                 "bitrate", encoder->bit_rate,
-                 "max-threads", 1, /* zero-frame latency */
-                 NULL);
+    switch (encoder->base.codec_type) {
+    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
+        g_object_set(G_OBJECT(encoder->gstenc),
+                     "bitrate", encoder->bit_rate,
+                     "max-threads", 1, /* zero-frame latency */
+                     NULL);
+        break;
+    case SPICE_VIDEO_CODEC_TYPE_VP8: {
+        /* See http://www.webmproject.org/docs/encoder-parameters/ */
+#ifdef HAVE_G_GET_NUMPROCESSORS
+        int core_count = g_get_num_processors();
+#else
+        int core_count = 1;
+#endif
+        g_object_set(G_OBJECT(encoder->gstenc),
+                     "resize-allowed", TRUE, /* for very low bit rates */
+                     "target-bitrate", encoder->bit_rate,
+                     "end-usage", 1, /* CBR */
+                     "lag-in-frames", 0, /* zero-frame latency */
+                     "error-resilient", 1, /* for client frame drops */
+                     "deadline", 1000000 / get_source_fps(encoder) / 2, /* usec */
+                     "threads", core_count - 1,
+                     NULL);
+        break;
+        }
+    default:
+        /* gstreamer_encoder_new() should have rejected this codec type */
+        spice_warning("unknown encoder type %d", encoder->base.codec_type);
+        reset_pipeline(encoder);
+        return FALSE;
+    }
 
     /* Set the source caps */
     set_appsrc_caps(encoder);
 
-    /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
-    spice_debug("removing the pipeline clock");
-    gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
+    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG) {
+        /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
+        spice_debug("removing the pipeline clock");
+        gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
+    }
 
     /* Start playing */
     spice_debug("setting state to PLAYING");
@@ -225,6 +274,12 @@  static gboolean construct_pipeline(SpiceGstEncoder *encoder,
 /* A helper for gst_encoder_encode_frame(). */
 static void reconfigure_pipeline(SpiceGstEncoder *encoder)
 {
+    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
+        /* vp8enc gets confused if we try to reconfigure the pipeline */
+        reset_pipeline(encoder);
+        return;
+    }
+
     if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE) {
         spice_debug("GStreamer error: could not pause the pipeline, rebuilding it instead");
         reset_pipeline(encoder);
@@ -458,13 +513,19 @@  static void gst_encoder_get_stats(VideoEncoder *video_encoder,
     }
 }
 
-VideoEncoder *gstreamer_encoder_new(uint64_t starting_bit_rate,
+VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType codec_type,
+                                    uint64_t starting_bit_rate,
                                     VideoEncoderRateControlCbs *cbs,
                                     void *cbs_opaque)
 {
     SpiceGstEncoder *encoder;
 
     spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
+    if (codec_type != SPICE_VIDEO_CODEC_TYPE_MJPEG &&
+        codec_type != SPICE_VIDEO_CODEC_TYPE_VP8) {
+        spice_warning("unsupported codec type %d", codec_type);
+        return NULL;
+    }
 
     GError *err = NULL;
     if (!gst_init_check(NULL, NULL, &err)) {
@@ -480,6 +541,7 @@  VideoEncoder *gstreamer_encoder_new(uint64_t starting_bit_rate,
     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;
 
     if (cbs) {
         encoder->cbs = *cbs;
diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index c0fb7c6..74ddf0b 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -1349,10 +1349,12 @@  static void mjpeg_encoder_get_stats(VideoEncoder *video_encoder,
     stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
 }
 
-VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
+VideoEncoder *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
+                                uint64_t starting_bit_rate,
                                 VideoEncoderRateControlCbs *cbs,
                                 void *cbs_opaque)
 {
+    spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);
     spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
 
     MJpegEncoder *encoder = spice_new0(MJpegEncoder, 1);
@@ -1363,6 +1365,7 @@  VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
     encoder->base.notify_server_frame_drop = &mjpeg_encoder_notify_server_frame_drop;
     encoder->base.get_bit_rate = &mjpeg_encoder_get_bit_rate;
     encoder->base.get_stats = &mjpeg_encoder_get_stats;
+    encoder->base.codec_type = codec_type;
     encoder->first_frame = TRUE;
     encoder->rate_control.byte_rate = starting_bit_rate / 8;
     encoder->starting_bit_rate = starting_bit_rate;
diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index b11cd42..75cc53d 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"
@@ -205,12 +206,27 @@  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"},
@@ -222,26 +238,120 @@  static RendererInfo renderers_info[] = {
 static uint32_t renderers[RED_RENDERER_LAST];
 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;
-        }
-        inf++;
+    uint32_t renderer;
+
+    if (num_renderers == RED_RENDERER_LAST ||
+        !name_to_enum(renderer_names, name, &renderer)) {
+        return FALSE;
     }
-    return NULL;
+    renderers[num_renderers++] = renderer;
+    return TRUE;
 }
 
-int red_dispatcher_add_renderer(const char *name)
+static const EnumNames video_encoder_names[] = {
+    {0, "spice"},
+    {1, "gstreamer"},
+    {0, NULL},
+};
+
+static new_video_encoder_t video_encoder_procs[] = {
+    &mjpeg_encoder_new,
+#ifdef HAVE_GSTREAMER_1_0
+    &gstreamer_encoder_new,
+#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)
 {
-    RendererInfo *inf;
+    while (*codecs == ';') {
+        codecs++;
+    }
+    if (!*codecs) {
+        return NULL;
+    }
+    int n;
+    *encoder = *codec = NULL;
+    if (sscanf(codecs, "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n", encoder, codec, &n) != 2) {
+        spice_warning("spice: invalid encoder:codec value at %s", codecs);
+        return strchr(codecs, ';');
+    }
+    return codecs + n;
+}
 
-    if (num_renderers == RED_RENDERER_LAST || !(inf = find_renderer(name))) {
-        return FALSE;
+int red_dispatcher_set_video_codecs(const char *codecs)
+{
+    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 (strcmp(codecs, "auto") == 0) {
+        codecs = VIDEO_ENCODER_DEFAULT_PREFERENCE;
+    }
+
+    c = codecs;
+    count = 0;
+    while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
+        int skip = FALSE;
+        if (!encoder_name || !codec_name) {
+            skip = TRUE;
+
+        } else if (!name_to_enum(video_encoder_names, encoder_name, &encoder)) {
+            spice_warning("spice: unknown video encoder %s", encoder_name);
+            skip = TRUE;
+
+        } else if (!name_to_enum(video_codec_names, codec_name, &type) ||
+                   !name_to_enum(video_cap_names, codec_name, &cap)) {
+            spice_warning("spice: unknown video codec %s", codec_name);
+            skip = TRUE;
+
+        } else if (!video_encoder_procs[encoder]) {
+            spice_warning("spice: unsupported video encoder %s", encoder_name);
+            skip = TRUE;
+        }
+
+        free(encoder_name);
+        free(codec_name);
+        if (skip) {
+            continue;
+        }
+
+        if (count == RED_MAX_VIDEO_CODECS) {
+            spice_warning("spice: cannot add more than %d video codec preferences", count);
+            break;
+        }
+        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;
 }
 
@@ -785,6 +895,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;
@@ -1092,6 +1218,11 @@  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(VIDEO_ENCODER_DEFAULT_PREFERENCE);
+    }
+    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 320b7e3..82aed9f 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 f2a4ee6..177ae1f 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
@@ -990,6 +991,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;
@@ -3068,21 +3071,45 @@  static void red_stream_update_client_playback_latency(void *opaque, uint32_t del
 }
 
 /* A helper for red_display_create_stream(). */
-static VideoEncoder* red_display_create_video_encoder(uint64_t starting_bit_rate,
+static VideoEncoder* red_display_create_video_encoder(DisplayChannelClient *dcc,
+                                                      uint64_t starting_bit_rate,
                                                       VideoEncoderRateControlCbs *cbs,
                                                       void *cbs_opaque)
 {
-#ifdef HAVE_GSTREAMER_1_0
-    VideoEncoder* video_encoder = gstreamer_encoder_new(starting_bit_rate, cbs, cbs_opaque);
-    if (video_encoder) {
-        return video_encoder;
+    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;
+        }
     }
-#endif
-    /* Use the builtin MJPEG video encoder as a fallback */
-    return mjpeg_encoder_new(starting_bit_rate, cbs, cbs_opaque);
+
+    /* 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 mjpeg_encoder_new(SPICE_VIDEO_CODEC_TYPE_MJPEG, starting_bit_rate, cbs, cbs_opaque);
+    }
+
+    return NULL;
 }
 
-static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
+static int red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
 {
     StreamAgent *agent = &dcc->stream_agents[get_stream_id(dcc->common.worker, stream)];
 
@@ -3108,10 +3135,15 @@  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 = red_display_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 = red_display_create_video_encoder(0, NULL, NULL);
+        agent->video_encoder = red_display_create_video_encoder(dcc, 0, NULL, NULL);
     }
+    if (agent->video_encoder == NULL) {
+        stream->refs--;
+        return FALSE;
+    }
+
     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)) {
@@ -3129,6 +3161,7 @@  static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
         agent->stats.start = stream->current->red_drawable->mm_time;
     }
 #endif
+    return TRUE;
 }
 
 static void red_create_stream(RedWorker *worker, Drawable *drawable)
@@ -3163,7 +3196,12 @@  static void red_create_stream(RedWorker *worker, Drawable *drawable)
     worker->streams_size_total += stream->width * stream->height;
     worker->stream_count++;
     WORKER_FOREACH_DCC_SAFE(worker, dcc_ring_item, next, dcc) {
-        red_display_create_stream(dcc, stream);
+        if (!red_display_create_stream(dcc, stream)) {
+            drawable->stream = NULL;
+            stream->current = NULL;
+            red_stop_stream(worker, stream);
+            return;
+        }
     }
     spice_debug("stream %d %dx%d (%d, %d) (%d, %d)", (int)(stream - worker->streams_buf), stream->width,
                 stream->height, stream->dest_area.left, stream->dest_area.top,
@@ -3178,7 +3216,10 @@  static void red_disply_start_streams(DisplayChannelClient *dcc)
 
     while ((item = ring_next(ring, item))) {
         Stream *stream = SPICE_CONTAINEROF(item, Stream, link);
-        red_display_create_stream(dcc, stream);
+        if (!red_display_create_stream(dcc, stream)) {
+            red_stop_stream(dcc->common.worker, stream);
+            return;
+        }
     }
 }
 
@@ -8928,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;
@@ -11760,6 +11801,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;
@@ -12089,6 +12139,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 ca8aadb..1fdcdb3 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_LAST
 };
 
+#define RED_MAX_VIDEO_CODECS 8
+
+typedef struct RedVideoCodec {
+    new_video_encoder_t 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 5d2ad9b..174f103 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3689,6 +3689,18 @@  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 c2ff61d..198dcf0 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 d65e14d..8da649c 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 8036c3c..8fdbf1d 100644
--- a/server/video_encoder.h
+++ b/server/video_encoder.h
@@ -119,6 +119,9 @@  struct VideoEncoder {
      *              statistics.
      */
     void (*get_stats)(VideoEncoder *encoder, VideoEncoderStats *stats);
+
+    /* The codec being used by the video encoder */
+    SpiceVideoCodecType codec_type;
 };
 
 
@@ -148,6 +151,7 @@  typedef struct VideoEncoderRateControlCbs {
 
 /* Instantiates the video encoder.
  *
+ * @codec_type:        The codec to use.
  * @starting_bit_rate: An initial estimate of the available stream bit rate or
  *                     zero if the client does not support rate control.
  * @cbs:               A set of callback methods to be used for rate control.
@@ -155,13 +159,19 @@  typedef struct VideoEncoderRateControlCbs {
  * @return:            A pointer to a structure implementing the VideoEncoder
  *                     methods.
  */
-VideoEncoder* mjpeg_encoder_new(uint64_t starting_bit_rate,
+typedef VideoEncoder* (*new_video_encoder_t)(SpiceVideoCodecType codec_type, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque);
+
+VideoEncoder* mjpeg_encoder_new(SpiceVideoCodecType codec_type,
+                                uint64_t starting_bit_rate,
                                 VideoEncoderRateControlCbs *cbs,
                                 void *cbs_opaque);
 #ifdef HAVE_GSTREAMER_1_0
-VideoEncoder* gstreamer_encoder_new(uint64_t starting_bit_rate,
+VideoEncoder* gstreamer_encoder_new(SpiceVideoCodecType codec_type,
+                                    uint64_t starting_bit_rate,
                                     VideoEncoderRateControlCbs *cbs,
                                     void *cbs_opaque);
 #endif
 
+#define VIDEO_ENCODER_DEFAULT_PREFERENCE "spice:mjpeg;gstreamer:mjpeg;gstreamer:vp8"
+
 #endif

Comments

On Wed, Oct 14, 2015 at 05:32:03PM +0200, Francois Gouget wrote:
> The Spice server administrator can specify the preferred encoder and
> codec preferences to optimize for CPU or bandwidth usage. Preferences
> are described in a semi-colon separated list of encoder:codec pairs.
> For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer
> VP8 video encoder and the original MJPEG video encoder as a fallback.
> The server has a default preference list which can also be selected by
> specifying 'auto' as the preferences list.

Note: All this paragraph describes a very different thing than what the
shortlog says "server: Add VP8 support to the GStreamer encoder". Imo
they are 2 distinct things, the addition of
spice_server_set_video_codecs() and the addition of vp8 support to the
gstreamer encoder, ie they should be (at least) 2 different commits. It
might even make sense to move the client capability checks to a 3rd
commit.

When adding use of SPICE_DISPLAY_CAP_MULTI_CODEC , I would raise the
spice-protocol requirement in configure.ac to 0.12.11 (and do a
post-release bump in spice-protocol git).

> 
> The server then picks a codec supported by the client based on the
> following new client 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.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  configure.ac               |   4 ++
>  server/gstreamer_encoder.c |  80 +++++++++++++++++++---
>  server/mjpeg_encoder.c     |   5 +-
>  server/red_dispatcher.c    | 165 ++++++++++++++++++++++++++++++++++++++++-----
>  server/red_dispatcher.h    |   8 +++
>  server/red_worker.c        |  80 ++++++++++++++++++----
>  server/red_worker.h        |  18 +++++
>  server/reds.c              |  12 ++++
>  server/spice-server.h      |   1 +
>  server/spice-server.syms   |   1 +
>  server/video_encoder.h     |  14 +++-
>  11 files changed, 345 insertions(+), 43 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 76e2ad7..3c1882f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -141,6 +141,10 @@ AC_SUBST([SPICE_PROTOCOL_MIN_VER])
>  PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.22])
>  AS_VAR_APPEND([SPICE_REQUIRES], [" glib-2.0 >= 2.22"])
>  
> +AC_CHECK_LIB(glib-2.0, g_get_num_processors,
> +             AC_DEFINE([HAVE_G_GET_NUMPROCESSORS], 1, [Defined if we have g_get_num_processors()]),,
> +             $GLIB2_LIBS)
> +
>  PKG_CHECK_MODULES(PIXMAN, pixman-1 >= 0.17.7)
>  AC_SUBST(PIXMAN_CFLAGS)
>  AC_SUBST(PIXMAN_LIBS)
> diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
> index bf66e99..765b31b 100644
> --- a/server/gstreamer_encoder.c
> +++ b/server/gstreamer_encoder.c
> @@ -184,13 +184,33 @@ static void set_appsrc_caps(SpiceGstEncoder *encoder)
>  static gboolean construct_pipeline(SpiceGstEncoder *encoder,
>                                     const SpiceBitmap *bitmap)
>  {
> +    const gchar* gstenc_name;
> +    switch (encoder->base.codec_type)
> +    {
> +    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> +        gstenc_name = "avenc_mjpeg";
> +        break;
> +    case SPICE_VIDEO_CODEC_TYPE_VP8:
> +        gstenc_name = "vp8enc";
> +        break;
> +    default:
> +        /* gstreamer_encoder_new() should have rejected this codec type */
> +        spice_warning("unsupported codec type %d", encoder->base.codec_type);
> +        return FALSE;
> +    }
> +
>      GError *err = NULL;
> -    const gchar *desc = "appsrc name=src format=2 do-timestamp=true ! videoconvert ! avenc_mjpeg name=encoder ! appsink name=sink";
> +    gchar *desc = g_strdup_printf("appsrc name=src format=2 do-timestamp=true ! videoconvert ! %s name=encoder ! appsink name=sink", gstenc_name);
>      spice_debug("GStreamer pipeline: %s", desc);
>      encoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
> +    g_free(desc);
>      if (!encoder->pipeline || err) {
>          spice_warning("GStreamer error: %s", err->message);
>          g_clear_error(&err);
> +        if (encoder->pipeline) {
> +            gst_object_unref(encoder->pipeline);
> +            encoder->pipeline = NULL;
> +        }

I think this unref could go to the previous commit?

>          return FALSE;
>      }
>      encoder->appsrc = GST_APP_SRC(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "src"));
> @@ -199,17 +219,46 @@ static gboolean construct_pipeline(SpiceGstEncoder *encoder,
>  
>      /* Configure the encoder bitrate, frame latency, etc. */
>      adjust_bit_rate(encoder);
> -    g_object_set(G_OBJECT(encoder->gstenc),
> -                 "bitrate", encoder->bit_rate,
> -                 "max-threads", 1, /* zero-frame latency */
> -                 NULL);
> +    switch (encoder->base.codec_type) {
> +    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> +        g_object_set(G_OBJECT(encoder->gstenc),
> +                     "bitrate", encoder->bit_rate,
> +                     "max-threads", 1, /* zero-frame latency */
> +                     NULL);
> +        break;
> +    case SPICE_VIDEO_CODEC_TYPE_VP8: {
> +        /* See http://www.webmproject.org/docs/encoder-parameters/ */
> +#ifdef HAVE_G_GET_NUMPROCESSORS
> +        int core_count = g_get_num_processors();
> +#else
> +        int core_count = 1;
> +#endif
> +        g_object_set(G_OBJECT(encoder->gstenc),
> +                     "resize-allowed", TRUE, /* for very low bit rates */
> +                     "target-bitrate", encoder->bit_rate,
> +                     "end-usage", 1, /* CBR */
> +                     "lag-in-frames", 0, /* zero-frame latency */
> +                     "error-resilient", 1, /* for client frame drops */
> +                     "deadline", 1000000 / get_source_fps(encoder) / 2, /* usec */
> +                     "threads", core_count - 1,
> +                     NULL);
> +        break;
> +        }
> +    default:
> +        /* gstreamer_encoder_new() should have rejected this codec type */
> +        spice_warning("unknown encoder type %d", encoder->base.codec_type);
> +        reset_pipeline(encoder);
> +        return FALSE;
> +    }
>  
>      /* Set the source caps */
>      set_appsrc_caps(encoder);
>  
> -    /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
> -    spice_debug("removing the pipeline clock");
> -    gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
> +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG) {
> +        /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
> +        spice_debug("removing the pipeline clock");
> +        gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
> +    }

Maybe this can go to the case SPICE_VIDEO_CODEC_TYPE_MJPEG above?

>  
>      /* Start playing */
>      spice_debug("setting state to PLAYING");
> @@ -225,6 +274,12 @@ static gboolean construct_pipeline(SpiceGstEncoder *encoder,
>  /* A helper for gst_encoder_encode_frame(). */
>  static void reconfigure_pipeline(SpiceGstEncoder *encoder)
>  {
> +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> +        /* vp8enc gets confused if we try to reconfigure the pipeline */
> +        reset_pipeline(encoder);
> +        return;
> +    }

Any bug reference for this one? Just add it to the comment if there is
such a bug, if not, that's fine.

> +
>      if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE) {
>          spice_debug("GStreamer error: could not pause the pipeline, rebuilding it instead");
>          reset_pipeline(encoder);
> @@ -458,13 +513,19 @@ static void gst_encoder_get_stats(VideoEncoder *video_encoder,
>      }
>  }
>  
> -VideoEncoder *gstreamer_encoder_new(uint64_t starting_bit_rate,
> +VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType codec_type,
> +                                    uint64_t starting_bit_rate,
>                                      VideoEncoderRateControlCbs *cbs,
>                                      void *cbs_opaque)
>  {
>      SpiceGstEncoder *encoder;
>  
>      spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
> +    if (codec_type != SPICE_VIDEO_CODEC_TYPE_MJPEG &&
> +        codec_type != SPICE_VIDEO_CODEC_TYPE_VP8) {
> +        spice_warning("unsupported codec type %d", codec_type);
> +        return NULL;
> +    }
>  
>      GError *err = NULL;
>      if (!gst_init_check(NULL, NULL, &err)) {
> @@ -480,6 +541,7 @@ VideoEncoder *gstreamer_encoder_new(uint64_t starting_bit_rate,
>      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;
>  
>      if (cbs) {
>          encoder->cbs = *cbs;
> diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
> index c0fb7c6..74ddf0b 100644
> --- a/server/mjpeg_encoder.c
> +++ b/server/mjpeg_encoder.c
> @@ -1349,10 +1349,12 @@ static void mjpeg_encoder_get_stats(VideoEncoder *video_encoder,
>      stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
>  }
>  
> -VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
> +VideoEncoder *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> +                                uint64_t starting_bit_rate,
>                                  VideoEncoderRateControlCbs *cbs,
>                                  void *cbs_opaque)
>  {
> +    spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);

I'd make that g_return_val_if_fail(codec_type ==
SPICE_VIDEO_CODEC_TYPE_MJPEG);

>      spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
>  
>      MJpegEncoder *encoder = spice_new0(MJpegEncoder, 1);
> @@ -1363,6 +1365,7 @@ VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
>      encoder->base.notify_server_frame_drop = &mjpeg_encoder_notify_server_frame_drop;
>      encoder->base.get_bit_rate = &mjpeg_encoder_get_bit_rate;
>      encoder->base.get_stats = &mjpeg_encoder_get_stats;
> +    encoder->base.codec_type = codec_type;
>      encoder->first_frame = TRUE;
>      encoder->rate_control.byte_rate = starting_bit_rate / 8;
>      encoder->starting_bit_rate = starting_bit_rate;
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index b11cd42..75cc53d 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"
> @@ -205,12 +206,27 @@ 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;
> +}
> +

For what it's worth, glib enums give you some similar functionality, but
requires a bit of plumbing before you can benefit from it ;) (in other
words, not asking you to make that change, just a random comment).

> +static const EnumNames renderer_names[] = {
>      {RED_RENDERER_SW, "sw"},
>  #ifdef USE_OPENGL
>      {RED_RENDERER_OGL_PBUF, "oglpbuf"},
> @@ -222,26 +238,120 @@ static RendererInfo renderers_info[] = {
>  static uint32_t renderers[RED_RENDERER_LAST];
>  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;
> -        }
> -        inf++;
> +    uint32_t renderer;
> +
> +    if (num_renderers == RED_RENDERER_LAST ||
> +        !name_to_enum(renderer_names, name, &renderer)) {
> +        return FALSE;
>      }
> -    return NULL;
> +    renderers[num_renderers++] = renderer;
> +    return TRUE;
>  }
>  
> -int red_dispatcher_add_renderer(const char *name)
> +static const EnumNames video_encoder_names[] = {
> +    {0, "spice"},
> +    {1, "gstreamer"},
> +    {0, NULL},
> +};
> +
> +static new_video_encoder_t video_encoder_procs[] = {
> +    &mjpeg_encoder_new,
> +#ifdef HAVE_GSTREAMER_1_0
> +    &gstreamer_encoder_new,
> +#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)
>  {
> -    RendererInfo *inf;
> +    while (*codecs == ';') {
> +        codecs++;
> +    }
> +    if (!*codecs) {
> +        return NULL;
> +    }
> +    int n;
> +    *encoder = *codec = NULL;
> +    if (sscanf(codecs, "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n", encoder, codec, &n) != 2) {
> +        spice_warning("spice: invalid encoder:codec value at %s", codecs);
> +        return strchr(codecs, ';');
> +    }
> +    return codecs + n;
> +}
>  
> -    if (num_renderers == RED_RENDERER_LAST || !(inf = find_renderer(name))) {
> -        return FALSE;
> +int red_dispatcher_set_video_codecs(const char *codecs)
> +{
> +    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 (strcmp(codecs, "auto") == 0) {
> +        codecs = VIDEO_ENCODER_DEFAULT_PREFERENCE;
> +    }
> +
> +    c = codecs;
> +    count = 0;
> +    while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {

Might be simpler to parse with g_strsplit() and strstr or such, but your
way is ok (though took me a while to find what sscanf(%m) is doing!)

> +        int skip = FALSE;
> +        if (!encoder_name || !codec_name) {
> +            skip = TRUE;
> +
> +        } else if (!name_to_enum(video_encoder_names, encoder_name, &encoder)) {
> +            spice_warning("spice: unknown video encoder %s", encoder_name);
> +            skip = TRUE;
> +
> +        } else if (!name_to_enum(video_codec_names, codec_name, &type) ||
> +                   !name_to_enum(video_cap_names, codec_name, &cap)) {
> +            spice_warning("spice: unknown video codec %s", codec_name);
> +            skip = TRUE;
> +
> +        } else if (!video_encoder_procs[encoder]) {
> +            spice_warning("spice: unsupported video encoder %s", encoder_name);
> +            skip = TRUE;
> +        }
> +
> +        free(encoder_name);
> +        free(codec_name);
> +        if (skip) {
> +            continue;
> +        }
> +
> +        if (count == RED_MAX_VIDEO_CODECS) {
> +            spice_warning("spice: cannot add more than %d video codec preferences", count);
> +            break;
> +        }
> +        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;
>  }
>  
> @@ -785,6 +895,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;
> @@ -1092,6 +1218,11 @@ 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(VIDEO_ENCODER_DEFAULT_PREFERENCE);
> +    }
> +    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 320b7e3..82aed9f 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 f2a4ee6..177ae1f 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
> @@ -990,6 +991,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;
> @@ -3068,21 +3071,45 @@ static void red_stream_update_client_playback_latency(void *opaque, uint32_t del
>  }
>  
>  /* A helper for red_display_create_stream(). */
> -static VideoEncoder* red_display_create_video_encoder(uint64_t starting_bit_rate,
> +static VideoEncoder* red_display_create_video_encoder(DisplayChannelClient *dcc,
> +                                                      uint64_t starting_bit_rate,
>                                                        VideoEncoderRateControlCbs *cbs,
>                                                        void *cbs_opaque)
>  {
> -#ifdef HAVE_GSTREAMER_1_0
> -    VideoEncoder* video_encoder = gstreamer_encoder_new(starting_bit_rate, cbs, cbs_opaque);
> -    if (video_encoder) {
> -        return video_encoder;
> +    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;
> +        }
>      }
> -#endif
> -    /* Use the builtin MJPEG video encoder as a fallback */
> -    return mjpeg_encoder_new(starting_bit_rate, cbs, cbs_opaque);
> +
> +    /* 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 mjpeg_encoder_new(SPICE_VIDEO_CODEC_TYPE_MJPEG, starting_bit_rate, cbs, cbs_opaque);
> +    }
> +
> +    return NULL;
>  }
>  
> -static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
> +static int red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
>  {
>      StreamAgent *agent = &dcc->stream_agents[get_stream_id(dcc->common.worker, stream)];
>  
> @@ -3108,10 +3135,15 @@ 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 = red_display_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 = red_display_create_video_encoder(0, NULL, NULL);
> +        agent->video_encoder = red_display_create_video_encoder(dcc, 0, NULL, NULL);
>      }
> +    if (agent->video_encoder == NULL) {
> +        stream->refs--;
> +        return FALSE;
> +    }
> +

Have you tested this error path? What happens when we destroy the stream
this way? bad rendering, or just fall back to no streaming?

>      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)) {
> @@ -3129,6 +3161,7 @@ static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
>          agent->stats.start = stream->current->red_drawable->mm_time;
>      }
>  #endif
> +    return TRUE;
>  }
>  
>  static void red_create_stream(RedWorker *worker, Drawable *drawable)
> @@ -3163,7 +3196,12 @@ static void red_create_stream(RedWorker *worker, Drawable *drawable)
>      worker->streams_size_total += stream->width * stream->height;
>      worker->stream_count++;
>      WORKER_FOREACH_DCC_SAFE(worker, dcc_ring_item, next, dcc) {
> -        red_display_create_stream(dcc, stream);
> +        if (!red_display_create_stream(dcc, stream)) {
> +            drawable->stream = NULL;
> +            stream->current = NULL;
> +            red_stop_stream(worker, stream);
> +            return;
> +        }
>      }
>      spice_debug("stream %d %dx%d (%d, %d) (%d, %d)", (int)(stream - worker->streams_buf), stream->width,
>                  stream->height, stream->dest_area.left, stream->dest_area.top,
> @@ -3178,7 +3216,10 @@ static void red_disply_start_streams(DisplayChannelClient *dcc)
>  
>      while ((item = ring_next(ring, item))) {
>          Stream *stream = SPICE_CONTAINEROF(item, Stream, link);
> -        red_display_create_stream(dcc, stream);
> +        if (!red_display_create_stream(dcc, stream)) {
> +            red_stop_stream(dcc->common.worker, stream);
> +            return;
> +        }
>      }
>  }
>  
> @@ -8928,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;
> @@ -11760,6 +11801,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;
> @@ -12089,6 +12139,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 ca8aadb..1fdcdb3 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_LAST
>  };
>  
> +#define RED_MAX_VIDEO_CODECS 8
> +
> +typedef struct RedVideoCodec {
> +    new_video_encoder_t 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 5d2ad9b..174f103 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3689,6 +3689,18 @@ 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 c2ff61d..198dcf0 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 d65e14d..8da649c 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;

This needs to go to a new section at the bottom of that file:

SPICE_SERVER_0.12.7 {
global:
    spice_server_set_video_codecs;
} SPICE_SERVER_0.12.6;

Christophe


> diff --git a/server/video_encoder.h b/server/video_encoder.h
> index 8036c3c..8fdbf1d 100644
> --- a/server/video_encoder.h
> +++ b/server/video_encoder.h
> @@ -119,6 +119,9 @@ struct VideoEncoder {
>       *              statistics.
>       */
>      void (*get_stats)(VideoEncoder *encoder, VideoEncoderStats *stats);
> +
> +    /* The codec being used by the video encoder */
> +    SpiceVideoCodecType codec_type;
>  };
>  
>  
> @@ -148,6 +151,7 @@ typedef struct VideoEncoderRateControlCbs {
>  
>  /* Instantiates the video encoder.
>   *
> + * @codec_type:        The codec to use.
>   * @starting_bit_rate: An initial estimate of the available stream bit rate or
>   *                     zero if the client does not support rate control.
>   * @cbs:               A set of callback methods to be used for rate control.
> @@ -155,13 +159,19 @@ typedef struct VideoEncoderRateControlCbs {
>   * @return:            A pointer to a structure implementing the VideoEncoder
>   *                     methods.
>   */
> -VideoEncoder* mjpeg_encoder_new(uint64_t starting_bit_rate,
> +typedef VideoEncoder* (*new_video_encoder_t)(SpiceVideoCodecType codec_type, uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque);
> +
> +VideoEncoder* mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> +                                uint64_t starting_bit_rate,
>                                  VideoEncoderRateControlCbs *cbs,
>                                  void *cbs_opaque);
>  #ifdef HAVE_GSTREAMER_1_0
> -VideoEncoder* gstreamer_encoder_new(uint64_t starting_bit_rate,
> +VideoEncoder* gstreamer_encoder_new(SpiceVideoCodecType codec_type,
> +                                    uint64_t starting_bit_rate,
>                                      VideoEncoderRateControlCbs *cbs,
>                                      void *cbs_opaque);
>  #endif
>  
> +#define VIDEO_ENCODER_DEFAULT_PREFERENCE "spice:mjpeg;gstreamer:mjpeg;gstreamer:vp8"
> +
>  #endif
> -- 
> 2.6.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, 22 Oct 2015, Christophe Fergeau wrote:

> On Wed, Oct 14, 2015 at 05:32:03PM +0200, Francois Gouget wrote:
> > The Spice server administrator can specify the preferred encoder and
> > codec preferences to optimize for CPU or bandwidth usage. Preferences
> > are described in a semi-colon separated list of encoder:codec pairs.
> > For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer
> > VP8 video encoder and the original MJPEG video encoder as a fallback.
> > The server has a default preference list which can also be selected by
> > specifying 'auto' as the preferences list.
> 
> Note: All this paragraph describes a very different thing than what the
> shortlog says "server: Add VP8 support to the GStreamer encoder". Imo
> they are 2 distinct things, the addition of
> spice_server_set_video_codecs() and the addition of vp8 support to the
> gstreamer encoder, ie they should be (at least) 2 different commits. It
> might even make sense to move the client capability checks to a 3rd
> commit.

These are technically independent but conceptually they all depend on 
each other:
 * Adding VP8 support without a way to request using VP8 instead of 
   MJPEG would be equivalent to adding dead code. Furthermore if 
   compiling in GStreamer support forced use of VP8 then that server 
   would be incompatible with most clients so checking the client caps 
   is really necessary.

 * Committing a way to pick the encoder and codec when the only 
   codec is MJPEG does not make sense either. This could degenerate to 
   code that only lets the administrator pick the encoder (i.e. builting 
   or gstreamer), but the next patch would have to add support for the 
   codec and would essentially be just as big.

 * Committing the CAPS checking code first could work, though it would 
   be somewhat pointless and would not even shave a couple of lines off 
   this one.


> When adding use of SPICE_DISPLAY_CAP_MULTI_CODEC , I would raise the
> spice-protocol requirement in configure.ac to 0.12.11 (and do a
> post-release bump in spice-protocol git).

I have bumped the requirement to 0.12.11 in configure.ac but I believe 
the post-release bump in spice-protocol git has to be left to you.


> >      /* Start playing */
> >      spice_debug("setting state to PLAYING");
> > @@ -225,6 +274,12 @@ static gboolean construct_pipeline(SpiceGstEncoder *encoder,
> >  /* A helper for gst_encoder_encode_frame(). */
> >  static void reconfigure_pipeline(SpiceGstEncoder *encoder)
> >  {
> > +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> > +        /* vp8enc gets confused if we try to reconfigure the pipeline */
> > +        reset_pipeline(encoder);
> > +        return;
> > +    }
> 
> Any bug reference for this one? Just add it to the comment if there is
> such a bug, if not, that's fine.

vp8enc ignores our attempt to change the appsrc caps so that when it 
gets a buffer of the wrong size we get stuck. Given that VP8 performs 
inter-frame compression I cannot totally blame it for not liking the 
video format to change mid-stream. So while I'd prefer it if it could 
deal with this case, I'm not sure this warrants a vp8enc bug report. In 
any case this requires a workaround for now. I tried to make the comment 
more explicit.

Here are the corresponding log traces:

(/usr/bin/Xorg:26752): Spice-Debug **: gstreamer_encoder.c:415:gst_encoder_encode_frame: video format change: width 0 -> 640, height 0 -> 360, format 0 -> 9
[...]
(/usr/bin/Xorg:26752): Spice-Debug **: gstreamer_encoder.c:415:gst_encoder_encode_frame: video format change: width 640 -> 764, height 360 -> 448, format 9 -> 9
[...]
0:00:00.343781738 26752 0x7f9768544050 DEBUG          basetransform gstbasetransform.c:624:gst_base_transform_transform_size:<ffmpegcsp0> asked to transform size 1369088 for caps video/x-raw-rgb, bpp=(int)32, depth=(int)24, width=(int)640, height=(int)360, endianness=(int)4321, red_mask=(int)65280, green_mask=(int)16711680, blue_mask=(int)-16777216, framerate=(fraction)30/1 to size for caps video/x-raw-yuv, format=(fourcc)I420, width=(int)640, height=(int)360, framerate=(fraction)30/1 in direction SINK
** (Xorg:26752): WARNING **: ffmpegcsp0: size 1369088 is not a multiple of unit size 921600

See also:
http://lists.freedesktop.org/archives/spice-devel/2015-August/021274.html



> > @@ -1349,10 +1349,12 @@ static void mjpeg_encoder_get_stats(VideoEncoder *video_encoder,
> >      stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
> >  }
> >  
> > -VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
> > +VideoEncoder *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> > +                                uint64_t starting_bit_rate,
> >                                  VideoEncoderRateControlCbs *cbs,
> >                                  void *cbs_opaque)
> >  {
> > +    spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);
> 
> I'd make that g_return_val_if_fail(codec_type ==
> SPICE_VIDEO_CODEC_TYPE_MJPEG);

mjpeg_encoder.c does not depend on glib.h currently. The g_return_xxx() 
functions are also very rarely used in the Spice server: only twice in 
reds_stream.c. Furthermore calling mjpeg_encoder_new() for anything but 
the MJPEG codec can only result from a coding error. So I think 
spice_assert() is appropriate here.

See also:
http://lists.freedesktop.org/archives/spice-devel/2015-July/021161.html


> > +int red_dispatcher_set_video_codecs(const char *codecs)
> > +{
> > +    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 (strcmp(codecs, "auto") == 0) {
> > +        codecs = VIDEO_ENCODER_DEFAULT_PREFERENCE;
> > +    }
> > +
> > +    c = codecs;
> > +    count = 0;
> > +    while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
> 
> Might be simpler to parse with g_strsplit() and strstr or such, but your
> way is ok (though took me a while to find what sscanf(%m) is doing!)

I was initially using a more manual way of parsing the string but Victor 
Toso recommended using sscanf which did lead to shorter code:
http://lists.freedesktop.org/archives/spice-devel/2015-July/021161.html


> > @@ -3108,10 +3135,15 @@ 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 = red_display_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 = red_display_create_video_encoder(0, NULL, NULL);
> > +        agent->video_encoder = red_display_create_video_encoder(dcc, 0, NULL, NULL);
> >      }
> > +    if (agent->video_encoder == NULL) {
> > +        stream->refs--;
> > +        return FALSE;
> > +    }
> > +
> 
> Have you tested this error path? What happens when we destroy the stream
> this way? bad rendering, or just fall back to no streaming?

If I remember correctly (I tested this months ago), it goes back to no 
streaming. I don't remember if I got it to stop trying to initiate the 
stream over and over again though.
On Mon, Oct 26, 2015 at 09:06:03PM +0100, Francois Gouget wrote:
> On Thu, 22 Oct 2015, Christophe Fergeau wrote:
> 
> > On Wed, Oct 14, 2015 at 05:32:03PM +0200, Francois Gouget wrote:
> > > The Spice server administrator can specify the preferred encoder and
> > > codec preferences to optimize for CPU or bandwidth usage. Preferences
> > > are described in a semi-colon separated list of encoder:codec pairs.
> > > For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer
> > > VP8 video encoder and the original MJPEG video encoder as a fallback.
> > > The server has a default preference list which can also be selected by
> > > specifying 'auto' as the preferences list.
> > 
> > Note: All this paragraph describes a very different thing than what the
> > shortlog says "server: Add VP8 support to the GStreamer encoder". Imo
> > they are 2 distinct things, the addition of
> > spice_server_set_video_codecs() and the addition of vp8 support to the
> > gstreamer encoder, ie they should be (at least) 2 different commits. It
> > might even make sense to move the client capability checks to a 3rd
> > commit.
> 
> These are technically independent but conceptually they all depend on 
> each other:
>  * Adding VP8 support without a way to request using VP8 instead of 
>    MJPEG would be equivalent to adding dead code. Furthermore if 
>    compiling in GStreamer support forced use of VP8 then that server 
>    would be incompatible with most clients so checking the client caps 
>    is really necessary.
> 
>  * Committing a way to pick the encoder and codec when the only 
>    codec is MJPEG does not make sense either. This could degenerate to 
>    code that only lets the administrator pick the encoder (i.e. builting 
>    or gstreamer), but the next patch would have to add support for the 
>    codec and would essentially be just as big.
> 

I would add the whole parsing code from red_dispatcher.c in separate
commit(s). They are quite pointless without VP8 support, but this is
still some preparatory work which is logically independant from the VP8
support. It's better to have it separate imo, better when reviewing,
better when bisecting, ...

>  * Committing the CAPS checking code first could work, though it would 
>    be somewhat pointless and would not even shave a couple of lines off 
>    this one.
> 
> 
> > When adding use of SPICE_DISPLAY_CAP_MULTI_CODEC , I would raise the
> > spice-protocol requirement in configure.ac to 0.12.11 (and do a
> > post-release bump in spice-protocol git).
> 
> I have bumped the requirement to 0.12.11 in configure.ac but I believe 
> the post-release bump in spice-protocol git has to be left to you.

It was actually done a while ago in spice-protocol git
http://cgit.freedesktop.org/spice/spice-protocol/commit/?id=017ddbe7a7c73816f068527531

> 
> > > @@ -1349,10 +1349,12 @@ static void mjpeg_encoder_get_stats(VideoEncoder *video_encoder,
> > >      stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
> > >  }
> > >  
> > > -VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
> > > +VideoEncoder *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> > > +                                uint64_t starting_bit_rate,
> > >                                  VideoEncoderRateControlCbs *cbs,
> > >                                  void *cbs_opaque)
> > >  {
> > > +    spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);
> > 
> > I'd make that g_return_val_if_fail(codec_type ==
> > SPICE_VIDEO_CODEC_TYPE_MJPEG);
> 
> mjpeg_encoder.c does not depend on glib.h currently. The g_return_xxx() 
> functions are also very rarely used in the Spice server: only twice in 
> reds_stream.c. Furthermore calling mjpeg_encoder_new() for anything but 
> the MJPEG codec can only result from a coding error. So I think 
> spice_assert() is appropriate here.


spice_return_if_fail() can be used then. xx_return_if_fail() are used to
indicate programming errors, ie if they are triggered, then the code is
buggy. spice-server being a library, the less assert() it contains, the
better. So if the rest of the code can handle it just fine, better to
tag programming errors with xx_return_if_fail() rather than
assert'ing().

Christophe