Hey,
Mostly looks good, couple of minor comments below,
On Tue, Mar 01, 2016 at 04:51:29PM +0100, Francois Gouget wrote:
> The Spice server administrator can specify the encoder and codec
> preferences to optimize for CPU or bandwidth usage. Preferences are
> described in a semi-colon separated list of encoder:codec pairs.
> The server has a default preference list which can explicitly be
> selected by specifying 'auto'.
>
> 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. Note
> that for now the server only supports the MJPEG codec.
>
> Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> ---
>
> Unlike in the previous patches this is now stored in a GArray.
>
> server/dcc-send.c | 2 +-
> server/display-channel.c | 13 +++-
> server/display-channel.h | 4 ++
> server/gstreamer-encoder.c | 6 +-
> server/mjpeg-encoder.c | 6 +-
> server/red-dispatcher.c | 10 +++
> server/red-dispatcher.h | 7 ++
> server/red-worker.c | 14 ++++
> server/reds-private.h | 1 +
> server/reds.c | 163 ++++++++++++++++++++++++++++++++++++++++-----
> server/reds.h | 1 +
> server/spice-server.h | 8 +++
> server/spice-server.syms | 5 ++
> server/stream.c | 29 ++++++--
> server/video-encoder.h | 20 +++++-
> 15 files changed, 260 insertions(+), 29 deletions(-)
>
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 8cf5a6a..6cf16e2 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -2150,7 +2150,7 @@ static void marshall_stream_start(RedChannelClient *rcc,
> stream_create.surface_id = 0;
> stream_create.id = get_stream_id(DCC_TO_DC(dcc), 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;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 3b5e169..372298e 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -230,6 +230,14 @@ void display_channel_set_stream_video(DisplayChannel *display, int stream_video)
> display->stream_video = stream_video;
> }
>
> +void display_channel_set_video_codecs(DisplayChannel *display, GArray *video_codecs)
> +{
> + spice_return_if_fail(display);
> +
> + g_array_set_size(display->video_codecs, 0);
> + g_array_insert_vals(display->video_codecs, 0, video_codecs->data, video_codecs->len);
> +}
I would do something like
g_array_unref(display->video_codecs);
display->video_codecs = g_array_ref(video_codecs);
> +
> static void stop_streams(DisplayChannel *display)
> {
> Ring *ring = &display->streams;
> @@ -2025,7 +2033,8 @@ static SpiceCanvas *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t su
> return display->surfaces[surface_id].context.canvas;
> }
>
> -DisplayChannel* display_channel_new(RedWorker *worker, int migrate, int stream_video,
> +DisplayChannel* display_channel_new(RedWorker *worker, int migrate,
> + int stream_video, GArray *video_codecs,
> uint32_t n_surfaces)
> {
> DisplayChannel *display;
> @@ -2079,6 +2088,8 @@ DisplayChannel* display_channel_new(RedWorker *worker, int migrate, int stream_v
> drawables_init(display);
> image_cache_init(&display->image_cache);
> display->stream_video = stream_video;
> + display->video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
> + display_channel_set_video_codecs(display, video_codecs);
> display_channel_init_streams(display);
>
> return display;
> diff --git a/server/display-channel.h b/server/display-channel.h
> index cf40edd..e44ca56 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -183,6 +183,7 @@ struct DisplayChannel {
> uint32_t glz_drawable_count;
>
> int stream_video;
> + GArray *video_codecs;
> uint32_t stream_count;
> Stream streams_buf[NUM_STREAMS];
> Stream *free_streams;
> @@ -253,6 +254,7 @@ typedef struct UpgradeItem {
> DisplayChannel* display_channel_new (RedWorker *worker,
> int migrate,
> int stream_video,
> + GArray *video_codecs,
> uint32_t n_surfaces);
> void display_channel_create_surface (DisplayChannel *display, uint32_t surface_id,
> uint32_t width, uint32_t height,
> @@ -274,6 +276,8 @@ void display_channel_update (DisplayCha
> void display_channel_free_some (DisplayChannel *display);
> void display_channel_set_stream_video (DisplayChannel *display,
> int stream_video);
> +void display_channel_set_video_codecs (DisplayChannel *display,
> + GArray *video_codecs);
> int display_channel_get_streams_timeout (DisplayChannel *display);
> void display_channel_compress_stats_print (const DisplayChannel *display);
> void display_channel_compress_stats_reset (DisplayChannel *display);
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 3fb5181..aae47c7 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -497,9 +497,12 @@ static void spice_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)
> {
> + spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG, NULL);
> +
> GError *err = NULL;
> if (!gst_init_check(NULL, NULL, &err)) {
> spice_warning("GStreamer error: %s", err->message);
> @@ -514,6 +517,7 @@ VideoEncoder *gstreamer_encoder_new(uint64_t starting_bit_rate,
> encoder->base.notify_server_frame_drop = &spice_gst_encoder_notify_server_frame_drop;
> encoder->base.get_bit_rate = &spice_gst_encoder_get_bit_rate;
> encoder->base.get_stats = &spice_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 9b2cff9..7e66106 100644
> --- a/server/mjpeg-encoder.c
> +++ b/server/mjpeg-encoder.c
> @@ -1344,17 +1344,21 @@ 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)
> {
> MJpegEncoder *encoder = spice_new0(MJpegEncoder, 1);
>
> + spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG, NULL);
> +
> encoder->base.destroy = &mjpeg_encoder_destroy;
> encoder->base.encode_frame = &mjpeg_encoder_encode_frame;
> encoder->base.client_stream_report = &mjpeg_encoder_client_stream_report;
> 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 c2ca6b6..7930f0d 100644
> --- a/server/red-dispatcher.c
> +++ b/server/red-dispatcher.c
> @@ -1022,6 +1022,16 @@ void red_dispatcher_on_sv_change(RedDispatcher *dispatcher, int sv)
> &payload);
> }
>
> +void red_dispatcher_on_vc_change(RedDispatcher *dispatcher, GArray *video_codecs)
> +{
> + /* this command is synchronous, so it's ok to pass a pointer */
> + RedWorkerMessageSetVideoCodecs payload;
> + payload.video_codecs = video_codecs;
You could g_array_ref(video_codecs); here and unref it in the callback
handling the dispatch message, no need to rely on the fact that it's
synchronous this way.
> + dispatcher_send_message(&dispatcher->dispatcher,
> + RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
> + &payload);
> +}
> +
> void red_dispatcher_set_mouse_mode(RedDispatcher *dispatcher, uint32_t mode)
> {
> RedWorkerMessageSetMouseMode payload;
> diff --git a/server/red-dispatcher.h b/server/red-dispatcher.h
> index 1aa63c2..e707eaa 100644
> --- a/server/red-dispatcher.h
> +++ b/server/red-dispatcher.h
> @@ -29,6 +29,7 @@ void red_dispatcher_init(QXLInstance *qxl);
> void red_dispatcher_set_mm_time(RedDispatcher *dispatcher, uint32_t);
> void red_dispatcher_on_ic_change(RedDispatcher *dispatcher, SpiceImageCompression ic);
> void red_dispatcher_on_sv_change(RedDispatcher *dispatcher, int sv);
> +void red_dispatcher_on_vc_change(RedDispatcher *dispatcher, GArray* video_codecs);
> void red_dispatcher_set_mouse_mode(RedDispatcher *dispatcher, uint32_t mode);
> void red_dispatcher_attach_worker(RedDispatcher *dispatcher);
> void red_dispatcher_set_compression_level(RedDispatcher *dispatcher, int level);
> @@ -93,6 +94,7 @@ enum {
> RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> RED_WORKER_MESSAGE_GL_SCANOUT,
> RED_WORKER_MESSAGE_GL_DRAW_ASYNC,
> + RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
>
> RED_WORKER_MESSAGE_COUNT // LAST
> };
> @@ -230,6 +232,11 @@ typedef struct RedWorkerMessageSetStreamingVideo {
> uint32_t streaming_video;
> } RedWorkerMessageSetStreamingVideo;
>
> +/* this command is synchronous, so it's ok to pass a pointer */
> +typedef struct RedWorkerMessageSetVideoCodecs {
> + GArray* video_codecs;
> +} RedWorkerMessageSetVideoCodecs;
> +
> typedef struct RedWorkerMessageSetMouseMode {
> uint32_t mode;
> } RedWorkerMessageSetMouseMode;
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 771e1eb..fd8eefb 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1074,6 +1074,14 @@ static void handle_dev_set_streaming_video(void *opaque, void *payload)
> display_channel_set_stream_video(worker->display_channel, msg->streaming_video);
> }
>
> +void handle_dev_set_video_codecs(void *opaque, void *payload)
> +{
> + RedWorkerMessageSetVideoCodecs *msg = payload;
> + RedWorker *worker = opaque;
> +
> + display_channel_set_video_codecs(worker->display_channel, msg->video_codecs);
> +}
> +
> static void handle_dev_set_mouse_mode(void *opaque, void *payload)
> {
> RedWorkerMessageSetMouseMode *msg = payload;
> @@ -1347,6 +1355,11 @@ static void register_callbacks(Dispatcher *dispatcher)
> sizeof(RedWorkerMessageSetStreamingVideo),
> DISPATCHER_NONE);
> dispatcher_register_handler(dispatcher,
> + RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
> + handle_dev_set_video_codecs,
> + sizeof(RedWorkerMessageSetVideoCodecs),
> + DISPATCHER_ACK);
> + dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_SET_MOUSE_MODE,
> handle_dev_set_mouse_mode,
> sizeof(RedWorkerMessageSetMouseMode),
> @@ -1533,6 +1546,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher *red_dispatcher)
> worker->cursor_channel = cursor_channel_new(worker);
> // TODO: handle seemless migration. Temp, setting migrate to FALSE
> worker->display_channel = display_channel_new(worker, FALSE, reds_get_streaming_video(reds),
> + reds_get_video_codecs(reds),
> init_info.n_surfaces);
>
> return worker;
> diff --git a/server/reds-private.h b/server/reds-private.h
> index f567929..8416dc1 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -230,6 +230,7 @@ struct RedsState {
>
> gboolean ticketing_enabled;
> uint32_t streaming_video;
> + GArray* video_codecs;
> SpiceImageCompression image_compression;
> spice_wan_compression_t jpeg_state;
> spice_wan_compression_t zlib_glz_state;
> diff --git a/server/reds.c b/server/reds.c
> index e58cec5..e97038b 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -71,6 +71,7 @@
> #include "utils.h"
>
> #include "reds-private.h"
> +#include "video-encoder.h"
>
> static SpiceCoreInterface *core_public = NULL;
>
> @@ -174,6 +175,7 @@ static void reds_char_device_remove_state(RedsState *reds, SpiceCharDeviceState
> static void reds_send_mm_time(RedsState *reds);
> static void reds_on_ic_change(RedsState *reds);
> static void reds_on_sv_change(RedsState *reds);
> +static void reds_on_vc_change(RedsState *reds);
> static void reds_on_vm_stop(RedsState *reds);
> static void reds_on_vm_start(RedsState *reds);
> static void reds_set_mouse_mode(RedsState *reds, uint32_t mode);
> @@ -3424,6 +3426,9 @@ err:
>
> static const char default_renderer[] = "sw";
>
> +#define RED_MAX_VIDEO_CODECS 8
> +static const char default_video_codecs[] = "spice:mjpeg;gstreamer:mjpeg";
> +
> /* new interface */
> SPICE_GNUC_VISIBLE SpiceServer *spice_server_new(void)
> {
> @@ -3446,6 +3451,7 @@ SPICE_GNUC_VISIBLE SpiceServer *spice_server_new(void)
> memset(reds->spice_uuid, 0, sizeof(reds->spice_uuid));
> reds->ticketing_enabled = TRUE; /* ticketing enabled by default */
> reds->streaming_video = SPICE_STREAM_VIDEO_FILTER;
> + reds->video_codecs = g_array_sized_new(FALSE, FALSE, sizeof(RedVideoCodec), RED_MAX_VIDEO_CODECS);
> reds->image_compression = SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
> reds->jpeg_state = SPICE_WAN_COMPRESSION_AUTO;
> reds->zlib_glz_state = SPICE_WAN_COMPRESSION_AUTO;
> @@ -3456,39 +3462,136 @@ SPICE_GNUC_VISIBLE SpiceServer *spice_server_new(void)
> return reds;
> }
>
> -typedef struct RendererInfo {
> - int id;
> +typedef struct {
> + uint32_t id;
> const char *name;
> -} RendererInfo;
> +} EnumNames;
> +
> +static gboolean get_name_index(const EnumNames names[], const char *name, uint32_t *index)
> +{
> + if (name) {
> + int i;
> + for (i = 0; names[i].name; i++) {
> + if (strcmp(name, names[i].name) == 0) {
> + *index = i;
> + return TRUE;
> + }
> + }
> + }
> + return FALSE;
> +}
>
> -static const RendererInfo renderers_info[] = {
> +static const EnumNames renderer_names[] = {
> {RED_RENDERER_SW, "sw"},
> {RED_RENDERER_INVALID, NULL},
> };
>
> -static const RendererInfo *find_renderer(const char *name)
> +static gboolean reds_add_renderer(RedsState *reds, const char *name)
> +{
> + uint32_t index;
> +
> + if (reds->renderers->len == RED_RENDERER_LAST ||
> + !get_name_index(renderer_names, name, &index)) {
> + return FALSE;
> + }
> + g_array_append_val(reds->renderers, renderer_names[index].id);
> + return TRUE;
> +}
> +
> +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 int video_codec_caps[] = {
> + SPICE_DISPLAY_CAP_CODEC_MJPEG,
> + SPICE_DISPLAY_CAP_CODEC_VP8,
> +};
2 VP8 references which belong to a later commit
> +
> +
> +/* Expected string: encoder:codec;encoder:codec */
> +static const char* parse_video_codecs(const char *codecs, char **encoder,
> + char **codec)
> {
> - const RendererInfo *inf = renderers_info;
> - while (inf->name) {
> - if (strcmp(name, inf->name) == 0) {
> - return inf;
> + if (!codecs) {
> + return NULL;
> + }
> + 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) {
> + while (*codecs != '\0' && *codecs != ';') {
> + codecs++;
> }
> - inf++;
> + return codecs;
> }
> - return NULL;
> + return codecs + n;
> }
>
> -static int reds_add_renderer(RedsState *reds, const char *name)
> +static void reds_set_video_codecs(RedsState *reds, const char *codecs)
> {
> - const RendererInfo *inf;
> + char *encoder_name, *codec_name;
>
> - if (reds->renderers->len == RED_RENDERER_LAST || !(inf = find_renderer(name))) {
> - return FALSE;
> + if (strcmp(codecs, "auto") == 0) {
> + codecs = default_video_codecs;
> + }
> +
> + g_array_set_size(reds->video_codecs, 0);
Similarly to what I suggested for display_channel_set_video_codecs(),
I'd just _unref the existing array and create a new one. But that's fine
with me if you prefer to keep things as they are now with 'static'
allocations.
> + const char *c = codecs;
> + while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
> + uint32_t encoder_index, codec_index;
> + if (!encoder_name || !codec_name) {
> + spice_warning("spice: invalid encoder:codec value at %s", codecs);
> +
> + } else if (!get_name_index(video_encoder_names, encoder_name, &encoder_index)){
> + spice_warning("spice: unknown video encoder %s", encoder_name);
> +
> + } else if (!get_name_index(video_codec_names, codec_name, &codec_index)) {
> + spice_warning("spice: unknown video codec %s", codec_name);
> +
> + } else if (!video_encoder_procs[encoder_index]) {
> + spice_warning("spice: unsupported video encoder %s", encoder_name);
> +
> + } else if (reds->video_codecs->len == RED_MAX_VIDEO_CODECS) {
> + spice_warning("spice: cannot add more than %d video codec preferences", RED_MAX_VIDEO_CODECS);
> + c = NULL;
> +
> + } else {
> + RedVideoCodec new_codec;
> + new_codec.create = video_encoder_procs[encoder_index];
> + new_codec.type = video_codec_names[codec_index].id;
> + new_codec.cap = video_codec_caps[codec_index];
> + g_array_append_val(reds->video_codecs, new_codec);
> + }
> +
> + free(encoder_name);
> + free(codec_name);
> + codecs = c;
> }
> - g_array_append_val(reds->renderers, inf->id);
> - return TRUE;
> }
>
> +
> SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer *s, SpiceCoreInterface *core)
> {
> int ret;
> @@ -3498,6 +3601,9 @@ SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer *s, SpiceCoreInterface *cor
> if (s->renderers->len == 0) {
> reds_add_renderer(s, default_renderer);
> }
> + if (s->video_codecs->len == 0) {
> + reds_set_video_codecs(s, default_video_codecs);
> + }
> return ret;
> }
>
> @@ -3505,6 +3611,7 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *s)
> {
> spice_assert(reds == s);
> g_array_unref(s->renderers);
> + g_array_unref(s->video_codecs);
> reds_exit();
> }
>
> @@ -3820,6 +3927,19 @@ uint32_t reds_get_streaming_video(const RedsState *reds)
> return reds->streaming_video;
> }
>
> +SPICE_GNUC_VISIBLE int spice_server_set_video_codecs(SpiceServer *s, const char *video_codecs)
> +{
> + spice_assert(reds == s);
> + reds_set_video_codecs(s, video_codecs);
> + reds_on_vc_change(reds);
> + return 0;
> +}
Regarding the public API this can be tweaked later, but I'm not sure at
all this is going to be enough. I suspect when integrating this into
QEMU, libvirt will need to know whether gstreamer is supported, and
maybe the codecs which are available. If so, this would mean we would
get a nicer API than this string based one which coud be used instead.
I haven't done much research on this yet though, so maybe this is
enough, will need to check :)
Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
with the minor vp8 changes, and optionally the GArray ones (and maybe
the public API addition in a different commit ?)
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel