[Spice-devel,6/11] server: Add VP8 support, a video codec preference list and compatibility checks with the Spice client.

Submitted by Francois Gouget on May 13, 2015, 8:28 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Francois Gouget May 13, 2015, 8:28 p.m.
Preferences can be expressed by sending a semicolon-separated 
encoder:codec list. For instance spice:mjpeg for the original video 
encoder, and gstreamer:mjpeg for the new one.
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.
---
 server/gstreamer_encoder.c |  75 +++++++++++++++------
 server/mjpeg_encoder.c     |   6 +-
 server/red_dispatcher.c    | 159 ++++++++++++++++++++++++++++++++++++++++-----
 server/red_dispatcher.h    |   8 +++
 server/red_worker.c        |  62 +++++++++++++++---
 server/red_worker.h        |  18 +++++
 server/reds.c              |   8 +++
 server/spice-server.h      |   1 +
 server/spice-server.syms   |   1 +
 server/video_encoder.h     |  16 +++--
 10 files changed, 301 insertions(+), 53 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
index ef71b73..dcebc95 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 ! ffmpegcolorspace ! ffenc_mjpeg name=encoder ! appsink name=sink", NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
+    desc = g_strdup_printf("appsrc name=src is-live=1 ! ffmpegcolorspace ! %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,18 @@  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);
+    g_object_set(G_OBJECT(encoder->gstenc), "max-latency", 0, NULL);
+    g_object_set(G_OBJECT(encoder->gstenc), "max-keyframe-distance", 0, NULL);
+    g_object_set(G_OBJECT(encoder->gstenc), "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);
@@ -210,16 +217,18 @@  static int construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
     return TRUE;
 }
 
-static int reconfigure_pipeline(GstEncoder *encoder)
+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) {
-        spice_warning("GStreamer error: the pipeline reconfiguration failed");
+    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
+        /* vp8enc gets confused if we try to reconfigure the pipeline */
         reset_pipeline(encoder);
-        return FALSE;
     }
-    return TRUE;
+    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... */
+    }
 }
 
 static inline uint8_t *get_image_line(SpiceChunks *chunks, size_t *offset,
@@ -250,7 +259,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;
@@ -298,8 +307,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);
 
@@ -364,13 +382,13 @@  static int gst_encoder_encode_frame(GstEncoder *encoder,
         encoder->spice_format = bitmap->format;
         encoder->width = width;
         encoder->height = height;
-        if (encoder->pipeline && !reconfigure_pipeline(encoder))
-            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+        if (encoder->pipeline)
+            reconfigure_pipeline(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;
@@ -413,11 +431,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);
 
@@ -428,6 +459,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 f5f3e52..89ffa72 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"
@@ -204,43 +205,151 @@  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 uint32_t num_video_codecs = 0;
+
+/* 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 = codecs;
 
-    if (num_renderers == RED_MAX_RENDERERS || !(inf = find_renderer(name))) {
-        return FALSE;
+    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) ||
+            !video_encoder_procs[encoder])
+            convert_failed = TRUE;
+
+        free(encoder_name);
+        free(codec_name);
+
+        if (convert_failed)
+            return FALSE;
+
+        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));
+    red_dispatcher_on_vc_change();
+
     return TRUE;
 }
 
@@ -783,6 +892,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;
@@ -1084,6 +1209,8 @@  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));
+    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..8e94e26 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..93d8141 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,13 @@  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);
+
+    return red_dispatcher_set_video_codecs(video_codecs) ? 0 : -1;
+}
+
 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 d5f7fb8..422b6c3 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,22 +149,25 @@  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);
 

Comments

On Wed, May 13, 2015 at 10:28 PM, Francois Gouget <fgouget@codeweavers.com>
wrote:

> Preferences can be expressed by sending a semicolon-separated
> encoder:codec list. For instance spice:mjpeg for the original video
> encoder, and gstreamer:mjpeg for the new one.
> 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.
> ---
>  server/gstreamer_encoder.c |  75 +++++++++++++++------
>  server/mjpeg_encoder.c     |   6 +-
>  server/red_dispatcher.c    | 159
> ++++++++++++++++++++++++++++++++++++++++-----
>  server/red_dispatcher.h    |   8 +++
>  server/red_worker.c        |  62 +++++++++++++++---
>  server/red_worker.h        |  18 +++++
>  server/reds.c              |   8 +++
>  server/spice-server.h      |   1 +
>  server/spice-server.syms   |   1 +
>  server/video_encoder.h     |  16 +++--
>  10 files changed, 301 insertions(+), 53 deletions(-)
>
> diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
> index ef71b73..dcebc95 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
> ! ffmpegcolorspace ! ffenc_mjpeg name=encoder ! appsink name=sink", NULL,
> GST_PARSE_FLAG_FATAL_ERRORS, &err);
> +    desc = g_strdup_printf("appsrc name=src is-live=1 ! ffmpegcolorspace
> ! %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,18 @@ 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);
> +    g_object_set(G_OBJECT(encoder->gstenc), "max-latency", 0, NULL);
> +    g_object_set(G_OBJECT(encoder->gstenc), "max-keyframe-distance", 0,
> NULL);
> +    g_object_set(G_OBJECT(encoder->gstenc), "lag-in-frames", 0, NULL);
> +
>

Those paremeters do not exist with all encoders, notably ffenc_mjpeg. We
could rather use or define encodebin profiles.


>      /* 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);
> @@ -210,16 +217,18 @@ static int construct_pipeline(GstEncoder *encoder,
> const SpiceBitmap *bitmap)
>      return TRUE;
>  }
>
> -static int reconfigure_pipeline(GstEncoder *encoder)
> +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) {
> -        spice_warning("GStreamer error: the pipeline reconfiguration
> failed");
> +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> +        /* vp8enc gets confused if we try to reconfigure the pipeline */
>

You'll easily reach this condition with CAP_SIZED_STREAM streams. Tbh, I
don't think the video stream should be able to change every frame, but it's
something we need to fix in the video handling/detection code first.

         reset_pipeline(encoder);
> -        return FALSE;
>      }
> -    return TRUE;
> +    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... */
> +    }
>  }
>
>  static inline uint8_t *get_image_line(SpiceChunks *chunks, size_t *offset,
> @@ -250,7 +259,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;
> @@ -298,8 +307,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;
> +    }
>

What is this supposed to change? Did you consider appsrc do-timestamp
instead?


>      GST_BUFFER_OFFSET(buffer) = encoder->frame++;
>      gst_buffer_set_caps(buffer, encoder->src_caps);
>
> @@ -364,13 +382,13 @@ static int gst_encoder_encode_frame(GstEncoder
> *encoder,
>          encoder->spice_format = bitmap->format;
>          encoder->width = width;
>          encoder->height = height;
> -        if (encoder->pipeline && !reconfigure_pipeline(encoder))
> -            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +        if (encoder->pipeline)
> +            reconfigure_pipeline(encoder);
>

Why this change?


>      }
>      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;
> @@ -413,11 +431,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);
>
> @@ -428,6 +459,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 f5f3e52..89ffa72 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"
> @@ -204,43 +205,151 @@ 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 uint32_t num_video_codecs = 0;
> +
> +/* 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 = codecs;
>
> -    if (num_renderers == RED_MAX_RENDERERS || !(inf =
> find_renderer(name))) {
> -        return FALSE;
> +    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) ||
> +            !video_encoder_procs[encoder])
> +            convert_failed = TRUE;
> +
> +        free(encoder_name);
> +        free(codec_name);
> +
> +        if (convert_failed)
> +            return FALSE;
> +
> +        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));
> +    red_dispatcher_on_vc_change();
> +
>      return TRUE;
>  }
>
> @@ -783,6 +892,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;
> @@ -1084,6 +1209,8 @@ 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));
> +    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..8e94e26 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!
> +     */
>

You may disable video streaming in this case? (worker->streaming_video ==
STREAM_VIDEO_OFF)


>      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..93d8141 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,13 @@ 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);
> +
> +    return red_dispatcher_set_video_codecs(video_codecs) ? 0 : -1;
> +}
> +
>  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 d5f7fb8..422b6c3 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,22 +149,25 @@ 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);
>
> --
> 2.1.4
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On Tue, 19 May 2015, Marc-André Lureau wrote:
[...]
> > +    /* 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);
> > +    g_object_set(G_OBJECT(encoder->gstenc), "max-latency", 0, NULL);
> > +    g_object_set(G_OBJECT(encoder->gstenc), "max-keyframe-distance", 0, NULL);
> > +    g_object_set(G_OBJECT(encoder->gstenc), "lag-in-frames", 0, NULL);
> > +
> 
> Those paremeters do not exist with all encoders, notably ffenc_mjpeg.

I'll tweak it so we only set the properties supported by the current 
encoder.


> We could rather use or define encodebin profiles.

See discussion in the previous email.


[...]
> > +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> > +        /* vp8enc gets confused if we try to reconfigure the pipeline */
> >          reset_pipeline(encoder);
> 
> You'll easily reach this condition with CAP_SIZED_STREAM streams.

It's ok. reset_pipeline() frees the current pipeline, causing 
gst_encoder_encode_frame() to call construct_pipeline() as if this was 
the stream's first frame.


[...]
> >      /* 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;
> > +    }
> 
> What is this supposed to change? Did you consider appsrc do-timestamp
> instead?

It's a hack to get both the mjpeg end vp8 encoders working.

Using 'appsrc do-timestamp=1' works for the mjpeg encoder but causes us 
to block indefinitely in gst_app_sink_pull_buffer() for the vp8 encoder 
(independently of is-live), for an unknown reason.

My suspicion is that the frame gets dropped, presumably by vp8enc 
itself, maybe due to pipeline latency issues.

I'm also not sure yet of what the best approach is for timestamping and 
setting the framerate in the appsrc caps and their impact on the 
bitrate.

One option is to advertise the outgoing framerate, the target bitrate 
and use do-timestamping. The outgoing framerate, e.g. 10fps, may be 
lower than the source framerate, e.g. 24 fps, which implies dropping 
frames before passing them off to the pipeline. This means some frames 
will be less than 1/10th second apart and, if I remember correctly, 
this causes the pipeline to drop them which is not what we want (and 
again causes a freeze in gst_app_sink_pull_buffer()).

Another option is to set the bitrate and framerate to get the desired 
compressed buffer size. Then the stream's bitrate can be further reduced 
by controlling the outgoing frame rate. In this case I'm not sure using 
real timestamps makes sense.


> > @@ -364,13 +382,13 @@ static int gst_encoder_encode_frame(GstEncoder
> > *encoder,
> >          encoder->spice_format = bitmap->format;
> >          encoder->width = width;
> >          encoder->height = height;
> > -        if (encoder->pipeline && !reconfigure_pipeline(encoder))
> > -            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> > +        if (encoder->pipeline)
> > +            reconfigure_pipeline(encoder);
> 
> Why this change?

reconfigure_pipeline() used to return false if it failed to reconfigure 
the pipeline. With the old code this would cause us to return 
FRAME_UNSUPPORTED. But in fact in such a case the existing pipeline has 
been freed and all we need to do is to rebuild it from scratch which is 
what we already do a few lines down.

This should maybe have been part of patch 5 though 
reconfigure_pipeline() does not fail with ffenc_mjpeg.


> > +        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!
> > +     */
> 
> You may disable video streaming in this case? (worker->streaming_video ==
> STREAM_VIDEO_OFF)

The administrator could certainly set this option.

But as for gracefully handling this failure without configuration, as 
far as I can tell, by the time red_display_create_stream() fails it's 
too late to set streaming_video.