[Spice-devel,02/13] server: Add a GStreamer 0.10 MJPEG video encoder and use it by default. (take 4)

Submitted by Francois Gouget on July 21, 2015, 6 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Francois Gouget July 21, 2015, 6 p.m.
The GStreamer video encoder supports both regular and sized streams.
It is otherwise quite basic and lacks any rate control: the bitrate is set at startup and will not react to changes in the network conditions. Still it should work fine on LANs.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---

Changes since take 3:
 - Failing to create the pipeline is probably a permanent issue so if 
   that happens encode_frame() now returns 
   VIDEO_ENCODER_FRAME_UNSUPPORTED so that Spice falls back to the 
   non-stream way of sending screen updates.

 - The video encoder will not get a zero bit rate so I removed handling 
   of that case. The bit rate floor has been raised to 128kbps. Going 
   below that does not seem very meaningful. Calculating the bit rate 
   ceiling has been moved to the get_bit_rate_cap() function.

 - Removed some redundant field initializations and added a comment.

 - Added a get_mbps() helper to simplify printing bandwidth values.

 - Some reformating and reordering to group related functions.

Changes since take 2:
 - I resolved the buffer timestamping issues. Appsrc is no longer a live 
   source (it had no reason to be) and it performs the timestamping. The 
   only remaining annoyance is that ffenc_mjpeg requires removing the 
   pipeline's default clock otherwise it gets stuck with the following 
   message:

   ERROR  ffmpeg :0:: Error, Invalid timestamp=0, last=0

   I consider this to be an ffenc_mjpeg bug since none of the other 
   encoders (VP8, H264 and even avenc_mpjpeg in GStreamer 1.0) has this 
   issue. So I'm keeping the clock removal as a workaround for 
   ffenc_mjpeg.

 - I went back to ffmpegcolorspace because it's the standard way to do
   color conversion in GStreamer 0.10 (present in gst-plugins-base), 
   while autovideoconvert is only present in gst-plugins-bad which I 
   believe we don't depend on.

 - I simplified the source frame copy code (push_raw_frame()). It's also 
   ready for adding zero-copy once GStreamer 1.0 support is added.

 - I improved the autoconf GStreamer 0.10 detection: it's now possible 
   to require GStreamer support, or just let it be included if present. 
   This also paves the way for GStreamer 1.0.

 - Changed set_appsrc_caps() and construct_pipeline() to return a 
   gboolean since they only return TRUE/FALSE.

 - Fixed some formatting issues, mostly braces placement.

Changes since take 1:
 - The width/height comments are now correct in the previous patch of
   the series.

 - Fixed the pipeline reconfiguration (for video size changes) so it 
   still works if we have to fall back to rebuilding the pipeline from 
   scratch.

 - Added a comment suggesting to avoid copying the compressed buffer as 
   a future improvement. I think this will require changing the way the 
   output buffer is allocated though. So I'm leaving that for after the 
   basic support committed.

 configure.ac               |  26 +++
 server/Makefile.am         |   8 +
 server/gstreamer_encoder.c | 451 +++++++++++++++++++++++++++++++++++++++++++++
 server/red_worker.c        |  11 +-
 server/video_encoder.h     |   6 +
 5 files changed, 500 insertions(+), 2 deletions(-)
 create mode 100644 server/gstreamer_encoder.c

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index 5b4caa4..ca91b5a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -80,6 +80,30 @@  AS_IF([test x"$enable_opengl" != "xno"], [
 SPICE_CHECK_SMARTCARD([SMARTCARD])
 AM_CONDITIONAL(SUPPORT_SMARTCARD, test "x$have_smartcard" = "xyes")
 
+AC_ARG_ENABLE(gstreamer,
+              AS_HELP_STRING([--enable-gstreamer=@<:@auto/yes/no@:>@],
+                             [Enable GStreamer 0.10 support]),
+              [],
+              [enable_gstreamer="auto"])
+
+if test "x$enable_gstreamer" != "xno"; then
+    PKG_CHECK_MODULES(GSTREAMER_0_10, [gstreamer-0.10, gstreamer-app-0.10],
+                      [enable_gstreamer="yes"
+                       have_gstreamer_0_10="yes"],
+                      [have_gstreamer_0_10="no"])
+    if test "x$have_gstreamer_0_10" = "xyes"; then
+        AC_SUBST(GSTREAMER_0_10_CFLAGS)
+        AC_SUBST(GSTREAMER_0_10_LIBS)
+        AS_VAR_APPEND([SPICE_REQUIRES], [" gstreamer-0.10 gstreamer-app-0.10"])
+        AC_DEFINE([HAVE_GSTREAMER_0_10], [1], [Define if supporting GStreamer 0.10])
+    elif test "x$enable_gstreamer" = "xyes"; then
+        AC_MSG_ERROR([GStreamer 0.10 support requested but not found. You may set GSTREAMER_0_10_CFLAGS and GSTREAMER_0_10_LIBS to avoid the need to call pkg-config.])
+    fi
+else
+    have_gstreamer_0_10="no"
+fi
+AM_CONDITIONAL(SUPPORT_GSTREAMER_0_10, test "x$have_gstreamer_0_10" = "xyes")
+
 AC_ARG_ENABLE([automated_tests],
               AS_HELP_STRING([--enable-automated-tests], [Enable automated tests using spicy-screenshot (part of spice--gtk)]),,
               [enable_automated_tests="no"])
@@ -311,6 +335,8 @@  echo "
 
         Smartcard:                ${have_smartcard}
 
+        GStreamer 0.10:           ${have_gstreamer_0_10}
+
         SASL support:             ${enable_sasl}
 
         Automated tests:          ${enable_automated_tests}
diff --git a/server/Makefile.am b/server/Makefile.am
index 36b6d45..4921bc3 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -11,6 +11,7 @@  AM_CPPFLAGS =					\
 	$(SASL_CFLAGS)				\
 	$(SLIRP_CFLAGS)				\
 	$(SMARTCARD_CFLAGS)			\
+	$(GSTREAMER_0_10_CFLAGS)		\
 	$(SSL_CFLAGS)				\
 	$(VISIBILITY_HIDDEN_CFLAGS)		\
 	$(WARN_CFLAGS)				\
@@ -41,6 +42,7 @@  libspice_server_la_LIBADD =						\
 	$(PIXMAN_LIBS)							\
 	$(SASL_LIBS)							\
 	$(SLIRP_LIBS)							\
+	$(GSTREAMER_0_10_LIBS)							\
 	$(SSL_LIBS)							\
 	$(Z_LIBS)							\
 	$(SPICE_NONPKGCONFIG_LIBS)					\
@@ -138,6 +140,12 @@  libspice_server_la_SOURCES +=	\
 	$(NULL)
 endif
 
+if SUPPORT_GSTREAMER_0_10
+libspice_server_la_SOURCES +=	\
+	gstreamer_encoder.c		\
+	$(NULL)
+endif
+
 EXTRA_DIST =					\
 	glz_encode_match_tmpl.c			\
 	glz_encode_tmpl.c			\
diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
new file mode 100644
index 0000000..8a21ad0
--- /dev/null
+++ b/server/gstreamer_encoder.c
@@ -0,0 +1,451 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2015 Jeremy White
+   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
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <<A HREF="http://www.gnu.org/licenses/">http://www.gnu.org/licenses/</A>>.
+*/
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <gst/gst.h>
+#include <gst/app/gstappsrc.h>
+#include <gst/app/gstappsink.h>
+
+#include "red_common.h"
+
+typedef struct GstEncoder GstEncoder;
+#define VIDEO_ENCODER_T GstEncoder
+#include "video_encoder.h"
+
+
+#define GSTE_DEFAULT_FPS 30
+
+
+typedef struct {
+    SpiceBitmapFmt  spice_format;
+    int             bpp;
+    int             depth;
+    int             endianness;
+    int             blue_mask;
+    int             green_mask;
+    int             red_mask;
+} SpiceFormatForGStreamer;
+
+struct GstEncoder {
+    VideoEncoder base;
+
+    /* Rate control callbacks */
+    VideoEncoderRateControlCbs cbs;
+    void *cbs_opaque;
+
+    /* Spice's initial bit rate estimation in bits per second. */
+    uint64_t starting_bit_rate;
+
+    /* ---------- Video characteristics ---------- */
+
+    int width;
+    int height;
+    SpiceFormatForGStreamer *format;
+    SpiceBitmapFmt spice_format;
+
+    /* ---------- GStreamer pipeline ---------- */
+
+    /* Pointers to the GStreamer pipeline elements. If pipeline is NULL the
+     * other pointers are invalid.
+     */
+    GstElement *pipeline;
+    GstCaps *src_caps;
+    GstAppSrc *appsrc;
+    GstElement *gstenc;
+    GstAppSink *appsink;
+
+    /* The frame counter for GStreamer buffers */
+    uint32_t frame;
+
+    /* The bit rate target for the outgoing network stream. (bits per second) */
+    uint64_t bit_rate;
+
+    /* The minimum bit rate */
+#   define GSTE_MIN_BITRATE (128 * 1024)
+};
+
+
+/* ---------- Miscellaneous GstEncoder helpers ---------- */
+
+static inline double get_mbps(uint64_t bit_rate)
+{
+    return (double)bit_rate / 1024 / 1024;
+}
+
+/* Returns the source frame rate which may change at any time so don't store
+ * the result.
+ */
+static uint32_t get_source_fps(GstEncoder *encoder)
+{
+    return encoder->cbs.get_source_fps ?
+        encoder->cbs.get_source_fps(encoder->cbs_opaque) : GSTE_DEFAULT_FPS;
+}
+
+static void reset_pipeline(GstEncoder *encoder)
+{
+    if (!encoder->pipeline) {
+        return;
+    }
+
+    gst_element_set_state(encoder->pipeline, GST_STATE_NULL);
+    gst_caps_unref(encoder->src_caps);
+    gst_object_unref(encoder->appsrc);
+    gst_object_unref(encoder->gstenc);
+    gst_object_unref(encoder->appsink);
+    gst_object_unref(encoder->pipeline);
+    encoder->pipeline = NULL;
+}
+
+/* The maximum bit rate we will use for the current video.
+ *
+ * This is based on a 10x compression ratio which should be more than enough
+ * for even MJPEG to provide good quality.
+ */
+static uint64_t get_bit_rate_cap(GstEncoder *encoder)
+{
+    uint32_t raw_frame_bits = encoder->width * encoder->height * encoder->format->bpp;
+    return raw_frame_bits * get_source_fps(encoder) / 10;
+}
+
+static void adjust_bit_rate(GstEncoder *encoder)
+{
+    if (encoder->bit_rate < GSTE_MIN_BITRATE) {
+        /* Don't let the bit rate go too low */
+        encoder->bit_rate = GSTE_MIN_BITRATE;
+    } else {
+        /* or too high */
+        encoder->bit_rate = MIN(encoder->bit_rate, get_bit_rate_cap(encoder));
+    }
+    spice_debug("adjust_bit_rate(%.3fMbps)", get_mbps(encoder->bit_rate));
+}
+
+
+/* ---------- GStreamer pipeline ---------- */
+
+/* A helper for gst_encoder_encode_frame(). */
+static SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
+{
+    int i;
+    static SpiceFormatForGStreamer format_map[] =  {
+        { SPICE_BITMAP_FMT_RGBA, 32, 24, 4321, 0xff000000, 0xff0000, 0xff00},
+        /* TODO: Check the other formats */
+        { SPICE_BITMAP_FMT_32BIT, 32, 24, 4321, 0xff000000, 0xff0000, 0xff00},
+        { SPICE_BITMAP_FMT_24BIT, 24, 24, 4321, 0xff0000, 0xff00, 0xff},
+        { SPICE_BITMAP_FMT_16BIT, 16, 16, 4321, 0x001f, 0x03E0, 0x7C00},
+    };
+
+    for (i = 0; i < sizeof(format_map) / sizeof(format_map[0]); i++) {
+        if (format_map[i].spice_format == format) {
+            return &format_map[i];
+        }
+    }
+
+    return NULL;
+}
+
+static gboolean set_appsrc_caps(GstEncoder *encoder)
+{
+    GstCaps *new_caps = gst_caps_new_simple("video/x-raw-rgb",
+        "bpp", G_TYPE_INT, encoder->format->bpp,
+        "depth", G_TYPE_INT, encoder->format->depth,
+        "width", G_TYPE_INT, encoder->width,
+        "height", G_TYPE_INT, encoder->height,
+        "endianness", G_TYPE_INT, encoder->format->endianness,
+        "red_mask", G_TYPE_INT, encoder->format->red_mask,
+        "green_mask", G_TYPE_INT, encoder->format->green_mask,
+        "blue_mask", G_TYPE_INT, encoder->format->blue_mask,
+        "framerate", GST_TYPE_FRACTION, get_source_fps(encoder), 1,
+        NULL);
+    if (!new_caps) {
+        spice_warning("GStreamer error: could not create the source caps");
+        reset_pipeline(encoder);
+        return FALSE;
+    }
+    g_object_set(G_OBJECT(encoder->appsrc), "caps", new_caps, NULL);
+    if (encoder->src_caps) {
+        gst_caps_unref(encoder->src_caps);
+    }
+    encoder->src_caps = new_caps;
+    return TRUE;
+}
+
+/* A helper for gst_encoder_encode_frame(). */
+static gboolean construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
+{
+    GError *err = NULL;
+    const gchar* desc = "appsrc name=src format=2 do-timestamp=true ! ffmpegcolorspace ! ffenc_mjpeg name=encoder ! appsink name=sink";
+    spice_debug("GStreamer pipeline: %s", desc);
+    encoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
+    if (!encoder->pipeline) {
+        spice_warning("GStreamer error: %s", err->message);
+        g_clear_error(&err);
+        return FALSE;
+    }
+    encoder->appsrc = GST_APP_SRC(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "src"));
+    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 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 */
+    spice_debug("removing the pipeline clock");
+    gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
+    spice_debug("setting state to PLAYING");
+    if (gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
+        spice_warning("GStreamer error: unable to set the pipeline to the playing state");
+        reset_pipeline(encoder);
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
+/* A helper for gst_encoder_encode_frame(). */
+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_debug("GStreamer error: the pipeline reconfiguration failed, rebuilding it instead");
+        reset_pipeline(encoder); /* we can rebuild it... */
+    }
+}
+
+/* A helper for gst_encoder_encode_frame(). */
+static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
+                          const SpiceRect *src, int top_down)
+{
+    const uint32_t stream_height = src->bottom - src->top;
+    const uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8;
+    uint32_t len = stream_stride * stream_height;
+    GstBuffer *buffer = gst_buffer_new_and_alloc(len);
+    uint8_t *dst = GST_BUFFER_DATA(buffer);
+
+    /* Note that we should not reorder the lines, even if top_down is false.
+     * It just changes the number of lines to skip at the start of the bitmap.
+     */
+    const uint32_t skip_lines = top_down ? src->top : bitmap->y - (src->bottom - 0);
+    uint32_t chunk_offset = bitmap->stride * skip_lines;
+    SpiceChunks *chunks = bitmap->data;
+    uint32_t chunk = 0;
+
+    if (stream_stride == bitmap->stride) {
+        /* We can copy the bitmap chunk by chunk */
+        while (len && chunk < chunks->num_chunks) {
+            /* Make sure that the chunk is not padded */
+            if (chunks->chunk[chunk].len % bitmap->stride != 0) {
+                gst_buffer_unref(buffer);
+                return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+            }
+            if (chunk_offset >= chunks->chunk[chunk].len) {
+                chunk_offset -= chunks->chunk[chunk].len;
+                chunk++;
+                continue;
+            }
+
+            uint8_t *src = chunks->chunk[chunk].data + chunk_offset;
+            uint32_t thislen = MIN(chunks->chunk[chunk].len - chunk_offset, len);
+            memcpy(dst, src, thislen);
+            dst += thislen;
+            len -= thislen;
+            chunk_offset = 0;
+            chunk++;
+        }
+        spice_assert(len == 0);
+
+    } else {
+        /* We have to do a line-by-line copy because for each we have to leave
+         * out pixels on the left or right.
+         */
+        chunk_offset += src->left * encoder->format->bpp / 8;
+        for (int l = 0; l < stream_height; l++) {
+            /* We may have to move forward by more than one chunk the first
+             * time around.
+             */
+            while (chunk_offset >= chunks->chunk[chunk].len) {
+                /* Make sure that the chunk is not padded */
+                if (chunks->chunk[chunk].len % bitmap->stride != 0) {
+                    gst_buffer_unref(buffer);
+                    return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+                }
+                chunk_offset -= chunks->chunk[chunk].len;
+                chunk++;
+            }
+
+            /* Copy the line */
+            uint8_t *src = chunks->chunk[chunk].data + chunk_offset;
+            memcpy(dst, src, stream_stride);
+            dst += stream_stride;
+            chunk_offset += bitmap->stride;
+        }
+        spice_assert(dst - GST_BUFFER_DATA(buffer) == len);
+    }
+
+    gst_buffer_set_caps(buffer, encoder->src_caps);
+    GST_BUFFER_OFFSET(buffer) = encoder->frame++;
+
+    GstFlowReturn ret = gst_app_src_push_buffer(encoder->appsrc, buffer);
+    if (ret != GST_FLOW_OK) {
+        spice_debug("GStreamer error: unable to push source buffer (%d)", ret);
+        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+    }
+
+    return VIDEO_ENCODER_FRAME_ENCODE_DONE;
+}
+
+/* A helper for gst_encoder_encode_frame(). */
+static int pull_compressed_buffer(GstEncoder *encoder,
+                                  uint8_t **outbuf, size_t *outbuf_size,
+                                  int *data_size)
+{
+    GstBuffer *buffer = gst_app_sink_pull_buffer(encoder->appsink);
+    if (buffer) {
+        int len = GST_BUFFER_SIZE(buffer);
+        spice_assert(outbuf && outbuf_size);
+        if (!*outbuf || *outbuf_size < len) {
+            *outbuf = spice_realloc(*outbuf, len);
+            *outbuf_size = len;
+        }
+        /* TODO Try to avoid this copy by changing the GstBuffer handling */
+        memcpy(*outbuf, GST_BUFFER_DATA(buffer), len);
+        gst_buffer_unref(buffer);
+        *data_size = len;
+        return VIDEO_ENCODER_FRAME_ENCODE_DONE;
+    }
+    return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+}
+
+
+/* ---------- VideoEncoder's public API ---------- */
+
+static void gst_encoder_destroy(GstEncoder *encoder)
+{
+    reset_pipeline(encoder);
+    free(encoder);
+}
+
+static int gst_encoder_encode_frame(GstEncoder *encoder,
+                                    const SpiceBitmap *bitmap,
+                                    int width, int height,
+                                    const SpiceRect *src, int top_down,
+                                    uint32_t frame_mm_time,
+                                    uint8_t **outbuf, size_t *outbuf_size,
+                                    int *data_size)
+{
+    if (width != encoder->width || height != encoder->height ||
+        encoder->spice_format != bitmap->format) {
+        spice_debug("video format change: width %d -> %d, height %d -> %d, format %d -> %d", encoder->width, width, encoder->height, height, encoder->spice_format, bitmap->format);
+        encoder->format = map_format(bitmap->format);
+        if (!encoder->format) {
+            spice_debug("unable to map format type %d", bitmap->format);
+            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+        }
+        encoder->spice_format = bitmap->format;
+        encoder->width = width;
+        encoder->height = height;
+        if (encoder->pipeline) {
+            reconfigure_pipeline(encoder);
+        }
+    }
+    if (!encoder->pipeline && !construct_pipeline(encoder, bitmap)) {
+        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+    }
+
+    int rc = push_raw_frame(encoder, bitmap, src, top_down);
+    if (rc == VIDEO_ENCODER_FRAME_ENCODE_DONE) {
+        rc = pull_compressed_buffer(encoder, outbuf, outbuf_size, data_size);
+    }
+    return rc;
+}
+
+static void gst_encoder_client_stream_report(GstEncoder *encoder,
+                                             uint32_t num_frames,
+                                             uint32_t num_drops,
+                                             uint32_t start_frame_mm_time,
+                                             uint32_t end_frame_mm_time,
+                                             int32_t end_frame_delay,
+                                             uint32_t audio_delay)
+{
+    spice_debug("client report: #frames %u, #drops %d, duration %u video-delay %d audio-delay %u",
+                num_frames, num_drops,
+                end_frame_mm_time - start_frame_mm_time,
+                end_frame_delay, audio_delay);
+}
+
+static void gst_encoder_notify_server_frame_drop(GstEncoder *encoder)
+{
+    spice_debug("server report: getting frame drops...");
+}
+
+static uint64_t gst_encoder_get_bit_rate(GstEncoder *encoder)
+{
+    return encoder->bit_rate;
+}
+
+static void gst_encoder_get_stats(GstEncoder *encoder, VideoEncoderStats *stats)
+{
+    uint64_t raw_bit_rate = encoder->width * encoder->height * encoder->format->bpp * get_source_fps(encoder);
+
+    spice_assert(encoder != NULL && stats != NULL);
+    stats->starting_bit_rate = encoder->starting_bit_rate;
+    stats->cur_bit_rate = encoder->bit_rate;
+
+    /* Use the compression level as a proxy for the quality */
+    stats->avg_quality = 100.0 - raw_bit_rate / stats->cur_bit_rate;
+    if (stats->avg_quality < 0) {
+        stats->avg_quality = 0;
+    }
+}
+
+GstEncoder *create_gstreamer_encoder(uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
+{
+    GstEncoder *encoder;
+
+    spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
+
+    gst_init(NULL, NULL);
+
+    encoder = spice_new0(GstEncoder, 1);
+    encoder->base.destroy = &gst_encoder_destroy;
+    encoder->base.encode_frame = &gst_encoder_encode_frame;
+    encoder->base.client_stream_report = &gst_encoder_client_stream_report;
+    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;
+
+    if (cbs) {
+        encoder->cbs = *cbs;
+    }
+    encoder->cbs_opaque = cbs_opaque;
+    encoder->bit_rate = encoder->starting_bit_rate = starting_bit_rate;
+
+    /* All the other fields are initialized to zero by spice_new0(). */
+
+    return encoder;
+}
diff --git a/server/red_worker.c b/server/red_worker.c
index 2c3f09b..7e53cf2 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -3083,6 +3083,7 @@  static void red_stream_update_client_playback_latency(void *opaque, uint32_t del
 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));
@@ -3097,6 +3098,12 @@  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;
@@ -3106,9 +3113,9 @@  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_mjpeg_encoder(initial_bit_rate, &video_cbs, agent);
+        agent->video_encoder = create_video_encoder(initial_bit_rate, &video_cbs, agent);
     } else {
-        agent->video_encoder = create_mjpeg_encoder(0, NULL, NULL);
+        agent->video_encoder = create_video_encoder(0, NULL, NULL);
     }
     red_channel_client_pipe_add(&dcc->common.base, &agent->create_item);
 
diff --git a/server/video_encoder.h b/server/video_encoder.h
index 7d96351..64af9ae 100644
--- a/server/video_encoder.h
+++ b/server/video_encoder.h
@@ -158,8 +158,14 @@  typedef struct VideoEncoderRateControlCbs {
  * @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);
+
 VIDEO_ENCODER_T* create_mjpeg_encoder(uint64_t starting_bit_rate,
                                       VideoEncoderRateControlCbs *cbs,
                                       void *cbs_opaque);
 
+VIDEO_ENCODER_T* create_gstreamer_encoder(uint64_t starting_bit_rate,
+                                          VideoEncoderRateControlCbs *cbs,
+                                          void *cbs_opaque);
+
 #endif

Comments

Hello,

few comments/suggestions below

On Tue, Jul 21, 2015 at 08:00:47PM +0200, Francois Gouget wrote:
> The GStreamer video encoder supports both regular and sized streams.
> It is otherwise quite basic and lacks any rate control: the bitrate is set at startup and will not react to changes in the network conditions. Still it should work fine on LANs.
>
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>
> Changes since take 3:
>  - Failing to create the pipeline is probably a permanent issue so if
>    that happens encode_frame() now returns
>    VIDEO_ENCODER_FRAME_UNSUPPORTED so that Spice falls back to the
>    non-stream way of sending screen updates.
>
>  - The video encoder will not get a zero bit rate so I removed handling
>    of that case. The bit rate floor has been raised to 128kbps. Going
>    below that does not seem very meaningful. Calculating the bit rate
>    ceiling has been moved to the get_bit_rate_cap() function.
>
>  - Removed some redundant field initializations and added a comment.
>
>  - Added a get_mbps() helper to simplify printing bandwidth values.
>
>  - Some reformating and reordering to group related functions.
>
> Changes since take 2:
>  - I resolved the buffer timestamping issues. Appsrc is no longer a live
>    source (it had no reason to be) and it performs the timestamping. The
>    only remaining annoyance is that ffenc_mjpeg requires removing the
>    pipeline's default clock otherwise it gets stuck with the following
>    message:
>
>    ERROR  ffmpeg :0:: Error, Invalid timestamp=0, last=0
>
>    I consider this to be an ffenc_mjpeg bug since none of the other
>    encoders (VP8, H264 and even avenc_mpjpeg in GStreamer 1.0) has this
>    issue. So I'm keeping the clock removal as a workaround for
>    ffenc_mjpeg.
>
>  - I went back to ffmpegcolorspace because it's the standard way to do
>    color conversion in GStreamer 0.10 (present in gst-plugins-base),
>    while autovideoconvert is only present in gst-plugins-bad which I
>    believe we don't depend on.
>
>  - I simplified the source frame copy code (push_raw_frame()). It's also
>    ready for adding zero-copy once GStreamer 1.0 support is added.
>
>  - I improved the autoconf GStreamer 0.10 detection: it's now possible
>    to require GStreamer support, or just let it be included if present.
>    This also paves the way for GStreamer 1.0.
>
>  - Changed set_appsrc_caps() and construct_pipeline() to return a
>    gboolean since they only return TRUE/FALSE.
>
>  - Fixed some formatting issues, mostly braces placement.
>
> Changes since take 1:
>  - The width/height comments are now correct in the previous patch of
>    the series.
>
>  - Fixed the pipeline reconfiguration (for video size changes) so it
>    still works if we have to fall back to rebuilding the pipeline from
>    scratch.
>
>  - Added a comment suggesting to avoid copying the compressed buffer as
>    a future improvement. I think this will require changing the way the
>    output buffer is allocated though. So I'm leaving that for after the
>    basic support committed.
>
>  configure.ac               |  26 +++
>  server/Makefile.am         |   8 +
>  server/gstreamer_encoder.c | 451 +++++++++++++++++++++++++++++++++++++++++++++
>  server/red_worker.c        |  11 +-
>  server/video_encoder.h     |   6 +
>  5 files changed, 500 insertions(+), 2 deletions(-)
>  create mode 100644 server/gstreamer_encoder.c
>
> diff --git a/configure.ac b/configure.ac
> index 5b4caa4..ca91b5a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -80,6 +80,30 @@ AS_IF([test x"$enable_opengl" != "xno"], [
>  SPICE_CHECK_SMARTCARD([SMARTCARD])
>  AM_CONDITIONAL(SUPPORT_SMARTCARD, test "x$have_smartcard" = "xyes")
>
> +AC_ARG_ENABLE(gstreamer,
> +              AS_HELP_STRING([--enable-gstreamer=@<:@auto/yes/no@:>@],
> +                             [Enable GStreamer 0.10 support]),
> +              [],
> +              [enable_gstreamer="auto"])
> +
> +if test "x$enable_gstreamer" != "xno"; then
> +    PKG_CHECK_MODULES(GSTREAMER_0_10, [gstreamer-0.10, gstreamer-app-0.10],
> +                      [enable_gstreamer="yes"
> +                       have_gstreamer_0_10="yes"],
> +                      [have_gstreamer_0_10="no"])
> +    if test "x$have_gstreamer_0_10" = "xyes"; then
> +        AC_SUBST(GSTREAMER_0_10_CFLAGS)
> +        AC_SUBST(GSTREAMER_0_10_LIBS)
> +        AS_VAR_APPEND([SPICE_REQUIRES], [" gstreamer-0.10 gstreamer-app-0.10"])
> +        AC_DEFINE([HAVE_GSTREAMER_0_10], [1], [Define if supporting GStreamer 0.10])
> +    elif test "x$enable_gstreamer" = "xyes"; then
> +        AC_MSG_ERROR([GStreamer 0.10 support requested but not found. You may set GSTREAMER_0_10_CFLAGS and GSTREAMER_0_10_LIBS to avoid the need to call pkg-config.])
> +    fi
> +else
> +    have_gstreamer_0_10="no"
> +fi
> +AM_CONDITIONAL(SUPPORT_GSTREAMER_0_10, test "x$have_gstreamer_0_10" = "xyes")
> +
>  AC_ARG_ENABLE([automated_tests],
>                AS_HELP_STRING([--enable-automated-tests], [Enable automated tests using spicy-screenshot (part of spice--gtk)]),,
>                [enable_automated_tests="no"])
> @@ -311,6 +335,8 @@ echo "
>  
>          Smartcard:                ${have_smartcard}
>  
> +        GStreamer 0.10:           ${have_gstreamer_0_10}
> +
>          SASL support:             ${enable_sasl}
>  
>          Automated tests:          ${enable_automated_tests}
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 36b6d45..4921bc3 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -11,6 +11,7 @@ AM_CPPFLAGS =					\
>  	$(SASL_CFLAGS)				\
>  	$(SLIRP_CFLAGS)				\
>  	$(SMARTCARD_CFLAGS)			\
> +	$(GSTREAMER_0_10_CFLAGS)		\
>  	$(SSL_CFLAGS)				\
>  	$(VISIBILITY_HIDDEN_CFLAGS)		\
>  	$(WARN_CFLAGS)				\
> @@ -41,6 +42,7 @@ libspice_server_la_LIBADD =						\
>  	$(PIXMAN_LIBS)							\
>  	$(SASL_LIBS)							\
>  	$(SLIRP_LIBS)							\
> +	$(GSTREAMER_0_10_LIBS)							\
>  	$(SSL_LIBS)							\
>  	$(Z_LIBS)							\
>  	$(SPICE_NONPKGCONFIG_LIBS)					\
> @@ -138,6 +140,12 @@ libspice_server_la_SOURCES +=	\
>  	$(NULL)
>  endif
>  
> +if SUPPORT_GSTREAMER_0_10
> +libspice_server_la_SOURCES +=	\
> +	gstreamer_encoder.c		\
> +	$(NULL)
> +endif
> +
>  EXTRA_DIST =					\
>  	glz_encode_match_tmpl.c			\
>  	glz_encode_tmpl.c			\
> diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
> new file mode 100644
> index 0000000..8a21ad0
> --- /dev/null
> +++ b/server/gstreamer_encoder.c
> @@ -0,0 +1,451 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2015 Jeremy White
> +   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
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <<A HREF="http://www.gnu.org/licenses/">http://www.gnu.org/licenses/</A>>.
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <gst/gst.h>
> +#include <gst/app/gstappsrc.h>
> +#include <gst/app/gstappsink.h>
> +
> +#include "red_common.h"
> +
> +typedef struct GstEncoder GstEncoder;
> +#define VIDEO_ENCODER_T GstEncoder
> +#include "video_encoder.h"
> +
> +
> +#define GSTE_DEFAULT_FPS 30
> +
> +
> +typedef struct {
> +    SpiceBitmapFmt  spice_format;
> +    int             bpp;
> +    int             depth;
> +    int             endianness;
> +    int             blue_mask;
> +    int             green_mask;
> +    int             red_mask;
> +} SpiceFormatForGStreamer;
> +
> +struct GstEncoder {
> +    VideoEncoder base;
> +
> +    /* Rate control callbacks */
> +    VideoEncoderRateControlCbs cbs;
> +    void *cbs_opaque;
> +
> +    /* Spice's initial bit rate estimation in bits per second. */
> +    uint64_t starting_bit_rate;
> +
> +    /* ---------- Video characteristics ---------- */
> +
> +    int width;
> +    int height;
> +    SpiceFormatForGStreamer *format;
> +    SpiceBitmapFmt spice_format;
> +
> +    /* ---------- GStreamer pipeline ---------- */
> +
> +    /* Pointers to the GStreamer pipeline elements. If pipeline is NULL the
> +     * other pointers are invalid.
> +     */
> +    GstElement *pipeline;
> +    GstCaps *src_caps;
> +    GstAppSrc *appsrc;
> +    GstElement *gstenc;
> +    GstAppSink *appsink;
> +
> +    /* The frame counter for GStreamer buffers */
> +    uint32_t frame;
> +
> +    /* The bit rate target for the outgoing network stream. (bits per second) */
> +    uint64_t bit_rate;
> +
> +    /* The minimum bit rate */
> +#   define GSTE_MIN_BITRATE (128 * 1024)
> +};
> +
> +
> +/* ---------- Miscellaneous GstEncoder helpers ---------- */
> +
> +static inline double get_mbps(uint64_t bit_rate)
> +{
> +    return (double)bit_rate / 1024 / 1024;
> +}
> +
> +/* Returns the source frame rate which may change at any time so don't store
> + * the result.
> + */
> +static uint32_t get_source_fps(GstEncoder *encoder)
> +{
> +    return encoder->cbs.get_source_fps ?
> +        encoder->cbs.get_source_fps(encoder->cbs_opaque) : GSTE_DEFAULT_FPS;
> +}
> +
> +static void reset_pipeline(GstEncoder *encoder)
> +{
> +    if (!encoder->pipeline) {
> +        return;
> +    }
> +
> +    gst_element_set_state(encoder->pipeline, GST_STATE_NULL);
> +    gst_caps_unref(encoder->src_caps);
> +    gst_object_unref(encoder->appsrc);
> +    gst_object_unref(encoder->gstenc);
> +    gst_object_unref(encoder->appsink);
> +    gst_object_unref(encoder->pipeline);
> +    encoder->pipeline = NULL;
> +}
> +
> +/* The maximum bit rate we will use for the current video.
> + *
> + * This is based on a 10x compression ratio which should be more than enough
> + * for even MJPEG to provide good quality.
> + */
> +static uint64_t get_bit_rate_cap(GstEncoder *encoder)
> +{
> +    uint32_t raw_frame_bits = encoder->width * encoder->height * encoder->format->bpp;
> +    return raw_frame_bits * get_source_fps(encoder) / 10;
> +}
> +
> +static void adjust_bit_rate(GstEncoder *encoder)
> +{
> +    if (encoder->bit_rate < GSTE_MIN_BITRATE) {
> +        /* Don't let the bit rate go too low */
> +        encoder->bit_rate = GSTE_MIN_BITRATE;
> +    } else {
> +        /* or too high */
> +        encoder->bit_rate = MIN(encoder->bit_rate, get_bit_rate_cap(encoder));
> +    }
> +    spice_debug("adjust_bit_rate(%.3fMbps)", get_mbps(encoder->bit_rate));
> +}
> +
> +
> +/* ---------- GStreamer pipeline ---------- */
> +
> +/* A helper for gst_encoder_encode_frame(). */
> +static SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
> +{
> +    int i;
> +    static SpiceFormatForGStreamer format_map[] =  {
> +        { SPICE_BITMAP_FMT_RGBA, 32, 24, 4321, 0xff000000, 0xff0000, 0xff00},
> +        /* TODO: Check the other formats */
> +        { SPICE_BITMAP_FMT_32BIT, 32, 24, 4321, 0xff000000, 0xff0000, 0xff00},
> +        { SPICE_BITMAP_FMT_24BIT, 24, 24, 4321, 0xff0000, 0xff00, 0xff},
> +        { SPICE_BITMAP_FMT_16BIT, 16, 16, 4321, 0x001f, 0x03E0, 0x7C00},
> +    };
> +
> +    for (i = 0; i < sizeof(format_map) / sizeof(format_map[0]); i++) {
> +        if (format_map[i].spice_format == format) {
> +            return &format_map[i];
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static gboolean set_appsrc_caps(GstEncoder *encoder)
> +{
> +    GstCaps *new_caps = gst_caps_new_simple("video/x-raw-rgb",
> +        "bpp", G_TYPE_INT, encoder->format->bpp,
> +        "depth", G_TYPE_INT, encoder->format->depth,
> +        "width", G_TYPE_INT, encoder->width,
> +        "height", G_TYPE_INT, encoder->height,
> +        "endianness", G_TYPE_INT, encoder->format->endianness,
> +        "red_mask", G_TYPE_INT, encoder->format->red_mask,
> +        "green_mask", G_TYPE_INT, encoder->format->green_mask,
> +        "blue_mask", G_TYPE_INT, encoder->format->blue_mask,
> +        "framerate", GST_TYPE_FRACTION, get_source_fps(encoder), 1,
> +        NULL);
> +    if (!new_caps) {
> +        spice_warning("GStreamer error: could not create the source caps");
> +        reset_pipeline(encoder);
> +        return FALSE;

This function is called twice and one of them do reset_pipeline in case
of failure. I would remove reset_pipeline here to be dealt by who called
it.

(I'm not sure if it is possible to gst_caps_new_simple return NULL)

> +    }
> +    g_object_set(G_OBJECT(encoder->appsrc), "caps", new_caps, NULL);
> +    if (encoder->src_caps) {
> +        gst_caps_unref(encoder->src_caps);
> +    }
> +    encoder->src_caps = new_caps;
> +    return TRUE;
> +}
> +
> +/* A helper for gst_encoder_encode_frame(). */
> +static gboolean construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitmap)
> +{
> +    GError *err = NULL;
> +    const gchar* desc = "appsrc name=src format=2 do-timestamp=true ! ffmpegcolorspace ! ffenc_mjpeg name=encoder ! appsink name=sink";

Either keep * with the type or with variable.

> +    spice_debug("GStreamer pipeline: %s", desc);
> +    encoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
> +    if (!encoder->pipeline) {

I guess we want to check the err != NULL as well as
gst_parse_launch_full may return non NULL pipeline even if error is set.
/me believes that if the pipeline had an error to be created we should
not try to play it.

> +        spice_warning("GStreamer error: %s", err->message);
> +        g_clear_error(&err);
> +        return FALSE;
> +    }
> +    encoder->appsrc = GST_APP_SRC(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "src"));
> +    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 source caps */
> +    encoder->src_caps = NULL;
> +    if (!set_appsrc_caps(encoder)) {

reset_pipeline here in case you remove it from set_appsrc_caps

> +        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 */
> +    spice_debug("removing the pipeline clock");
> +    gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);

This is related to ffenc_mjpeg described in the mail, right?
I think it would be good to open a bug against 0.10 version and add a
FIXME here to track the issue.

https://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer

> +    spice_debug("setting state to PLAYING");
> +    if (gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
> +        spice_warning("GStreamer error: unable to set the pipeline to the playing state");
> +        reset_pipeline(encoder);
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> +/* A helper for gst_encoder_encode_frame(). */
> +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_debug("GStreamer error: the pipeline reconfiguration failed, rebuilding it instead");
> +        reset_pipeline(encoder); /* we can rebuild it... */
> +    }
> +}
> +
> +/* A helper for gst_encoder_encode_frame(). */
> +static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
> +                          const SpiceRect *src, int top_down)
> +{
> +    const uint32_t stream_height = src->bottom - src->top;
> +    const uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8;
> +    uint32_t len = stream_stride * stream_height;
> +    GstBuffer *buffer = gst_buffer_new_and_alloc(len);
> +    uint8_t *dst = GST_BUFFER_DATA(buffer);
> +
> +    /* Note that we should not reorder the lines, even if top_down is false.
> +     * It just changes the number of lines to skip at the start of the bitmap.
> +     */
> +    const uint32_t skip_lines = top_down ? src->top : bitmap->y - (src->bottom - 0);
> +    uint32_t chunk_offset = bitmap->stride * skip_lines;
> +    SpiceChunks *chunks = bitmap->data;
> +    uint32_t chunk = 0;
> +
> +    if (stream_stride == bitmap->stride) {
> +        /* We can copy the bitmap chunk by chunk */

I think we definitely need more help functions here to set the GstBuffer.
Later patches introduce gst 1.0 and the zero-copy and this becomes a bit
hard to track.

> +        while (len && chunk < chunks->num_chunks) {
> +            /* Make sure that the chunk is not padded */
> +            if (chunks->chunk[chunk].len % bitmap->stride != 0) {
> +                gst_buffer_unref(buffer);
> +                return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +            }
> +            if (chunk_offset >= chunks->chunk[chunk].len) {
> +                chunk_offset -= chunks->chunk[chunk].len;
> +                chunk++;
> +                continue;
> +            }
> +
> +            uint8_t *src = chunks->chunk[chunk].data + chunk_offset;
> +            uint32_t thislen = MIN(chunks->chunk[chunk].len - chunk_offset, len);
> +            memcpy(dst, src, thislen);
> +            dst += thislen;
> +            len -= thislen;
> +            chunk_offset = 0;
> +            chunk++;
> +        }
> +        spice_assert(len == 0);
> +
> +    } else {
> +        /* We have to do a line-by-line copy because for each we have to leave
> +         * out pixels on the left or right.
> +         */
> +        chunk_offset += src->left * encoder->format->bpp / 8;
> +        for (int l = 0; l < stream_height; l++) {
> +            /* We may have to move forward by more than one chunk the first
> +             * time around.
> +             */
> +            while (chunk_offset >= chunks->chunk[chunk].len) {
> +                /* Make sure that the chunk is not padded */
> +                if (chunks->chunk[chunk].len % bitmap->stride != 0) {
> +                    gst_buffer_unref(buffer);
> +                    return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +                }
> +                chunk_offset -= chunks->chunk[chunk].len;
> +                chunk++;
> +            }
> +
> +            /* Copy the line */
> +            uint8_t *src = chunks->chunk[chunk].data + chunk_offset;
> +            memcpy(dst, src, stream_stride);
> +            dst += stream_stride;
> +            chunk_offset += bitmap->stride;
> +        }
> +        spice_assert(dst - GST_BUFFER_DATA(buffer) == len);
> +    }
> +
> +    gst_buffer_set_caps(buffer, encoder->src_caps);
> +    GST_BUFFER_OFFSET(buffer) = encoder->frame++;
> +
> +    GstFlowReturn ret = gst_app_src_push_buffer(encoder->appsrc, buffer);
> +    if (ret != GST_FLOW_OK) {
> +        spice_debug("GStreamer error: unable to push source buffer (%d)", ret);
> +        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +    }
> +
> +    return VIDEO_ENCODER_FRAME_ENCODE_DONE;
> +}
> +
> +/* A helper for gst_encoder_encode_frame(). */
> +static int pull_compressed_buffer(GstEncoder *encoder,
> +                                  uint8_t **outbuf, size_t *outbuf_size,
> +                                  int *data_size)
> +{

This will be totally changed when video encoder manage the compressed
buffer.

> +    GstBuffer *buffer = gst_app_sink_pull_buffer(encoder->appsink);
> +    if (buffer) {
> +        int len = GST_BUFFER_SIZE(buffer);
> +        spice_assert(outbuf && outbuf_size);
> +        if (!*outbuf || *outbuf_size < len) {
> +            *outbuf = spice_realloc(*outbuf, len);
> +            *outbuf_size = len;
> +        }
> +        /* TODO Try to avoid this copy by changing the GstBuffer handling */
> +        memcpy(*outbuf, GST_BUFFER_DATA(buffer), len);
> +        gst_buffer_unref(buffer);
> +        *data_size = len;
> +        return VIDEO_ENCODER_FRAME_ENCODE_DONE;
> +    }
> +    return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +}
> +
> +
> +/* ---------- VideoEncoder's public API ---------- */
> +
> +static void gst_encoder_destroy(GstEncoder *encoder)
> +{
> +    reset_pipeline(encoder);
> +    free(encoder);
> +}
> +
> +static int gst_encoder_encode_frame(GstEncoder *encoder,
> +                                    const SpiceBitmap *bitmap,
> +                                    int width, int height,
> +                                    const SpiceRect *src, int top_down,
> +                                    uint32_t frame_mm_time,
> +                                    uint8_t **outbuf, size_t *outbuf_size,
> +                                    int *data_size)
> +{
> +    if (width != encoder->width || height != encoder->height ||
> +        encoder->spice_format != bitmap->format) {
> +        spice_debug("video format change: width %d -> %d, height %d -> %d, format %d -> %d", encoder->width, width, encoder->height, height, encoder->spice_format, bitmap->format);

Please, break this 180 chars line.

> +        encoder->format = map_format(bitmap->format);
> +        if (!encoder->format) {
> +            spice_debug("unable to map format type %d", bitmap->format);
> +            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +        }
> +        encoder->spice_format = bitmap->format;
> +        encoder->width = width;
> +        encoder->height = height;
> +        if (encoder->pipeline) {
> +            reconfigure_pipeline(encoder);
> +        }
> +    }
> +    if (!encoder->pipeline && !construct_pipeline(encoder, bitmap)) {

If we reach this we may need a spice_warning, no?

> +        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +    }
> +
> +    int rc = push_raw_frame(encoder, bitmap, src, top_down);
> +    if (rc == VIDEO_ENCODER_FRAME_ENCODE_DONE) {
> +        rc = pull_compressed_buffer(encoder, outbuf, outbuf_size, data_size);
> +    }
> +    return rc;
> +}
> +
> +static void gst_encoder_client_stream_report(GstEncoder *encoder,
> +                                             uint32_t num_frames,
> +                                             uint32_t num_drops,
> +                                             uint32_t start_frame_mm_time,
> +                                             uint32_t end_frame_mm_time,
> +                                             int32_t end_frame_delay,
> +                                             uint32_t audio_delay)
> +{
> +    spice_debug("client report: #frames %u, #drops %d, duration %u video-delay %d audio-delay %u",
> +                num_frames, num_drops,
> +                end_frame_mm_time - start_frame_mm_time,
> +                end_frame_delay, audio_delay);
> +}
> +
> +static void gst_encoder_notify_server_frame_drop(GstEncoder *encoder)
> +{
> +    spice_debug("server report: getting frame drops...");

This will be improved in following patches

> +}
> +
> +static uint64_t gst_encoder_get_bit_rate(GstEncoder *encoder)
> +{
> +    return encoder->bit_rate;
> +}
> +
> +static void gst_encoder_get_stats(GstEncoder *encoder, VideoEncoderStats *stats)
> +{
> +    uint64_t raw_bit_rate = encoder->width * encoder->height * encoder->format->bpp * get_source_fps(encoder);
> +
> +    spice_assert(encoder != NULL && stats != NULL);
> +    stats->starting_bit_rate = encoder->starting_bit_rate;
> +    stats->cur_bit_rate = encoder->bit_rate;
> +
> +    /* Use the compression level as a proxy for the quality */
> +    stats->avg_quality = 100.0 - raw_bit_rate / stats->cur_bit_rate;
> +    if (stats->avg_quality < 0) {
> +        stats->avg_quality = 0;
> +    }
> +}
> +
> +GstEncoder *create_gstreamer_encoder(uint64_t starting_bit_rate, VideoEncoderRateControlCbs *cbs, void *cbs_opaque)
> +{
> +    GstEncoder *encoder;
> +
> +    spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
> +
> +    gst_init(NULL, NULL);
> +
> +    encoder = spice_new0(GstEncoder, 1);
> +    encoder->base.destroy = &gst_encoder_destroy;
> +    encoder->base.encode_frame = &gst_encoder_encode_frame;
> +    encoder->base.client_stream_report = &gst_encoder_client_stream_report;
> +    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;
> +
> +    if (cbs) {
> +        encoder->cbs = *cbs;
> +    }
> +    encoder->cbs_opaque = cbs_opaque;
> +    encoder->bit_rate = encoder->starting_bit_rate = starting_bit_rate;
> +
> +    /* All the other fields are initialized to zero by spice_new0(). */
> +
> +    return encoder;
> +}
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 2c3f09b..7e53cf2 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -3083,6 +3083,7 @@ static void red_stream_update_client_playback_latency(void *opaque, uint32_t del
>  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));
> @@ -3097,6 +3098,12 @@ 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;
> @@ -3106,9 +3113,9 @@ 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_mjpeg_encoder(initial_bit_rate, &video_cbs, agent);
> +        agent->video_encoder = create_video_encoder(initial_bit_rate, &video_cbs, agent);
>      } else {
> -        agent->video_encoder = create_mjpeg_encoder(0, NULL, NULL);
> +        agent->video_encoder = create_video_encoder(0, NULL, NULL);
>      }
>      red_channel_client_pipe_add(&dcc->common.base, &agent->create_item);
>  
> diff --git a/server/video_encoder.h b/server/video_encoder.h
> index 7d96351..64af9ae 100644
> --- a/server/video_encoder.h
> +++ b/server/video_encoder.h
> @@ -158,8 +158,14 @@ typedef struct VideoEncoderRateControlCbs {
>   * @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);
> +
>  VIDEO_ENCODER_T* create_mjpeg_encoder(uint64_t starting_bit_rate,
>                                        VideoEncoderRateControlCbs *cbs,
>                                        void *cbs_opaque);
>  
> +VIDEO_ENCODER_T* create_gstreamer_encoder(uint64_t starting_bit_rate,
> +                                          VideoEncoderRateControlCbs *cbs,
> +                                          void *cbs_opaque);
> +
>  #endif
> -- 
> 2.1.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel