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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel