[Spice-devel] [spice v10 06/27] server: Let the administrator pick the video encoder and codec

Christophe Fergeau cfergeau at redhat.com
Thu Mar 3 14:30:45 UTC 2016


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 at codeweavers.com>
> ---
> 
> 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 at redhat.com>
with the minor vp8 changes, and optionally the GArray ones (and maybe
the public API addition in a different commit ?)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160303/1c526e70/attachment-0001.sig>


More information about the Spice-devel mailing list