[Spice-devel] [PATCH v5 07/20] server: Let the video encoder manage the compressed buffer and avoid copying it.

Francois Gouget fgouget at codeweavers.com
Thu Aug 27 12:01:02 PDT 2015


This way the video encoder is not forced to use malloc()/free().
This also allows more flexibility in how the video encoder manages the buffer which allows for a zero-copy implementation in both video encoders.
The current implementations also ensure that there is no reallocation of the VideoBuffer structure.

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

Changes since take 4:
 - Some formatting fixes.

 server/gstreamer_encoder.c |  81 ++++++++++++++++++++++++++--------
 server/mjpeg_encoder.c     | 105 +++++++++++++++++++++++++++++++--------------
 server/red_worker.c        |  31 ++++++-------
 server/video_encoder.h     |  45 ++++++++++++++++---
 4 files changed, 186 insertions(+), 76 deletions(-)

diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
index 2318f40..425be55 100644
--- a/server/gstreamer_encoder.c
+++ b/server/gstreamer_encoder.c
@@ -26,6 +26,8 @@
 
 #include "red_common.h"
 
+typedef struct GstVideoBuffer GstVideoBuffer;
+#define VIDEO_BUFFER_T GstVideoBuffer
 typedef struct GstEncoder GstEncoder;
 #define VIDEO_ENCODER_T GstEncoder
 #include "video_encoder.h"
@@ -44,6 +46,12 @@ typedef struct {
     uint32_t red_mask;
 } SpiceFormatForGStreamer;
 
+struct GstVideoBuffer {
+    VideoBuffer base;
+    GstBuffer *gst_buffer;
+    gboolean persistent;
+};
+
 struct GstEncoder {
     VideoEncoder base;
 
@@ -72,6 +80,9 @@ struct GstEncoder {
     GstElement *gstenc;
     GstAppSink *appsink;
 
+    /* The default video buffer */
+    GstVideoBuffer *default_buffer;
+
     /* The frame counter for GStreamer buffers */
     uint32_t frame;
 
@@ -86,6 +97,35 @@ struct GstEncoder {
 };
 
 
+/* ---------- The GstVideoBuffer implementation ---------- */
+
+static inline GstVideoBuffer* gst_video_buffer_ref(GstVideoBuffer *buffer)
+{
+    buffer->base.ref_count++;
+    return buffer;
+}
+
+static void gst_video_buffer_unref(GstVideoBuffer *buffer)
+{
+    if (--buffer->base.ref_count == 0) {
+        gst_buffer_unref(buffer->gst_buffer);
+        if (!buffer->persistent) {
+            free(buffer);
+        }
+    }
+}
+
+static GstVideoBuffer* create_gst_video_buffer(gboolean persistent)
+{
+    GstVideoBuffer *buffer = spice_new0(GstVideoBuffer, 1);
+    buffer->base.ref = &gst_video_buffer_ref;
+    buffer->base.unref = &gst_video_buffer_unref;
+    buffer->persistent = persistent;
+    buffer->base.ref_count = persistent ? 0 : 1;
+    return buffer;
+}
+
+
 /* ---------- Miscellaneous GstEncoder helpers ---------- */
 
 static inline double get_mbps(uint64_t bit_rate)
@@ -386,24 +426,24 @@ static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
 }
 
 /* A helper for gst_encoder_encode_frame(). */
-static int pull_compressed_buffer(GstEncoder *encoder,
-                                  uint8_t **outbuf, size_t *outbuf_size,
-                                  int *data_size)
+static int pull_compressed_buffer(GstEncoder *encoder, GstVideoBuffer **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;
+    GstVideoBuffer *video_buffer;
+    if (encoder->default_buffer->base.ref_count == 0) {
+        /* The default buffer is unused so we can reuse it. */
+        video_buffer = encoder->default_buffer;
+        video_buffer->base.ref(video_buffer);
+    } else {
+        video_buffer = create_gst_video_buffer(FALSE);
+    }
+    video_buffer->gst_buffer = gst_app_sink_pull_buffer(encoder->appsink);
+    if (video_buffer->gst_buffer) {
+        video_buffer->base.data = GST_BUFFER_DATA(video_buffer->gst_buffer);
+        video_buffer->base.size = GST_BUFFER_SIZE(video_buffer->gst_buffer);
+        *buffer = video_buffer;
         return VIDEO_ENCODER_FRAME_ENCODE_DONE;
     }
+    video_buffer->base.unref(video_buffer);
     return VIDEO_ENCODER_FRAME_UNSUPPORTED;
 }
 
@@ -412,6 +452,11 @@ static int pull_compressed_buffer(GstEncoder *encoder,
 
 static void gst_encoder_destroy(GstEncoder *encoder)
 {
+    if (encoder->default_buffer->base.ref_count == 0) {
+        free(encoder->default_buffer);
+    } else {
+        encoder->default_buffer->persistent = FALSE;
+    }
     reset_pipeline(encoder);
     free(encoder);
 }
@@ -421,8 +466,7 @@ static int gst_encoder_encode_frame(GstEncoder *encoder,
                                     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)
+                                    GstVideoBuffer **buffer)
 {
     if (width != encoder->width || height != encoder->height ||
         encoder->spice_format != bitmap->format) {
@@ -447,7 +491,7 @@ static int gst_encoder_encode_frame(GstEncoder *encoder,
 
     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);
+        rc = pull_compressed_buffer(encoder, buffer);
     }
     return rc;
 }
@@ -515,6 +559,7 @@ GstEncoder *create_gstreamer_encoder(SpiceVideoCodecType codec_type,
     encoder->base.get_bit_rate = &gst_encoder_get_bit_rate;
     encoder->base.get_stats = &gst_encoder_get_stats;
     encoder->base.codec_type = codec_type;
+    encoder->default_buffer = create_gst_video_buffer(TRUE);
 
     if (cbs) {
         encoder->cbs = *cbs;
diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 9f1b392..0c982b7 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -21,6 +21,8 @@
 
 #include "red_common.h"
 
+typedef struct MJpegVideoBuffer MJpegVideoBuffer;
+#define VIDEO_BUFFER_T MJpegVideoBuffer
 typedef struct MJpegEncoder MJpegEncoder;
 #define VIDEO_ENCODER_T MJpegEncoder
 #include "video_encoder.h"
@@ -73,6 +75,9 @@ static const int mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
  */
 #define MJPEG_WARMUP_TIME 3000LL // 3 sec
 
+/* The compressed buffer initial size. */
+#define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
+
 enum {
     MJPEG_QUALITY_EVAL_TYPE_SET,
     MJPEG_QUALITY_EVAL_TYPE_UPGRADE,
@@ -157,12 +162,19 @@ typedef struct MJpegEncoderRateControl {
     uint64_t warmup_start_time;
 } MJpegEncoderRateControl;
 
+struct MJpegVideoBuffer {
+    VideoBuffer base;
+    size_t maxsize;
+};
+
 struct MJpegEncoder {
     VideoEncoder base;
     uint8_t *row;
     uint32_t row_size;
     int first_frame;
 
+    MJpegVideoBuffer* default_buffer;
+
     struct jpeg_compress_struct cinfo;
     struct jpeg_error_mgr jerr;
 
@@ -184,6 +196,31 @@ static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
                                                 uint64_t byte_rate,
                                                 uint32_t latency);
 
+static inline MJpegVideoBuffer* mjpeg_buffer_ref(MJpegVideoBuffer *buffer)
+{
+    buffer->base.ref_count++;
+    return buffer;
+}
+
+static void mjpeg_buffer_unref(MJpegVideoBuffer *buffer)
+{
+    if (--buffer->base.ref_count == 0) {
+        free(buffer->base.data);
+        free(buffer);
+    }
+}
+
+static MJpegVideoBuffer* create_mjpeg_video_buffer(void)
+{
+    MJpegVideoBuffer *buffer = spice_new0(MJpegVideoBuffer, 1);
+    buffer->base.ref = &mjpeg_buffer_ref;
+    buffer->base.unref = &mjpeg_buffer_unref;
+    buffer->maxsize = MJPEG_INITIAL_BUFFER_SIZE;
+    buffer->base.data = malloc(buffer->maxsize);
+    buffer->base.ref_count = 1;
+    return buffer;
+}
+
 static inline int rate_control_is_active(MJpegEncoder* encoder)
 {
     return encoder->cbs.get_roundtrip_ms != NULL;
@@ -192,6 +229,7 @@ static inline int rate_control_is_active(MJpegEncoder* encoder)
 static void mjpeg_encoder_destroy(MJpegEncoder *encoder)
 {
     jpeg_destroy_compress(&encoder->cinfo);
+    encoder->default_buffer->base.unref(encoder->default_buffer);
     free(encoder->row);
     free(encoder);
 }
@@ -285,24 +323,22 @@ static void term_mem_destination(j_compress_ptr cinfo)
 
 /*
  * Prepare for output to a memory buffer.
- * The caller may supply an own initial buffer with appropriate size.
- * Otherwise, or when the actual data output exceeds the given size,
- * the library adapts the buffer size as necessary.
- * The standard library functions malloc/free are used for allocating
- * larger memory, so the buffer is available to the application after
- * finishing compression, and then the application is responsible for
- * freeing the requested memory.
+ * The caller must supply its own initial buffer and size.
+ * When the actual data output exceeds the given size, the library
+ * will adapt the buffer size as necessary using the malloc()/free()
+ * functions. The buffer is available to the application after the
+ * compression and the application is then responsible for freeing it.
  */
-
 static void
 spice_jpeg_mem_dest(j_compress_ptr cinfo,
                     unsigned char ** outbuffer, size_t * outsize)
 {
   mem_destination_mgr *dest;
-#define OUTPUT_BUF_SIZE  4096	/* choose an efficiently fwrite'able size */
 
-  if (outbuffer == NULL || outsize == NULL)	/* sanity check */
+  if (outbuffer == NULL || *outbuffer == NULL ||
+      outsize == NULL || *outsize == 0) { /* sanity check */
     ERREXIT(cinfo, JERR_BUFFER_SIZE);
+  }
 
   /* The destination object is made permanent so that multiple JPEG images
    * can be written to the same buffer without re-executing jpeg_mem_dest.
@@ -317,13 +353,6 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
   dest->pub.term_destination = term_mem_destination;
   dest->outbuffer = outbuffer;
   dest->outsize = outsize;
-  if (*outbuffer == NULL || *outsize == 0) {
-    /* Allocate initial buffer */
-    *outbuffer = malloc(OUTPUT_BUF_SIZE);
-    if (*outbuffer == NULL)
-      ERREXIT1(cinfo, JERR_OUT_OF_MEMORY, 10);
-    *outsize = OUTPUT_BUF_SIZE;
-  }
 
   dest->pub.next_output_byte = dest->buffer = *outbuffer;
   dest->pub.free_in_buffer = dest->bufsize = *outsize;
@@ -700,7 +729,7 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, uint64_t now)
 static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
                                      SpiceBitmapFmt format,
                                      int width, int height,
-                                     uint8_t **dest, size_t *dest_len,
+                                     MJpegVideoBuffer *buffer,
                                      uint32_t frame_mm_time)
 {
     uint32_t quality;
@@ -784,7 +813,7 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
         }
     }
 
-    spice_jpeg_mem_dest(&encoder->cinfo, dest, dest_len);
+    spice_jpeg_mem_dest(&encoder->cinfo, &buffer->base.data, &buffer->maxsize);
 
     encoder->cinfo.image_width      = width;
     encoder->cinfo.image_height     = height;
@@ -927,25 +956,34 @@ static int mjpeg_encoder_encode_frame(MJpegEncoder *encoder,
                                       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)
+                                      MJpegVideoBuffer **buffer)
 {
-    int ret;
-
-    ret = mjpeg_encoder_start_frame(encoder, bitmap->format,
-                                    width, height, outbuf, outbuf_size,
-                                    frame_mm_time);
-    if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
-        return ret;
+    MJpegVideoBuffer *video_buffer;
+    if (encoder->default_buffer->base.ref_count == 1) {
+        /* Only the MJpegEncoder is referencing the default buffer
+         * so we can reuse it.
+         */
+        video_buffer = encoder->default_buffer;
+        video_buffer->base.ref(video_buffer);
+    } else {
+        video_buffer = create_mjpeg_video_buffer();
     }
 
-    if (!encode_mjpeg_frame(encoder, src, bitmap, top_down)) {
-        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+    int ret = mjpeg_encoder_start_frame(encoder, bitmap->format, width, height,
+                                        video_buffer, frame_mm_time);
+    if (ret == VIDEO_ENCODER_FRAME_ENCODE_DONE) {
+        if (encode_mjpeg_frame(encoder, src, bitmap, top_down)) {
+            video_buffer->base.size = mjpeg_encoder_end_frame(encoder);
+            *buffer = video_buffer;
+        } else {
+            ret = VIDEO_ENCODER_FRAME_UNSUPPORTED;
+        }
     }
 
-    *data_size = mjpeg_encoder_end_frame(encoder);
-
-    return VIDEO_ENCODER_FRAME_ENCODE_DONE;
+    if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
+        video_buffer->base.unref(video_buffer);
+    }
+    return ret;
 }
 
 
@@ -1355,6 +1393,7 @@ MJpegEncoder *create_mjpeg_encoder(SpiceVideoCodecType codec_type,
     encoder->base.get_stats = &mjpeg_encoder_get_stats;
     encoder->base.codec_type = codec_type;
     encoder->first_frame = TRUE;
+    encoder->default_buffer = create_mjpeg_video_buffer();
     encoder->rate_control.byte_rate = starting_bit_rate / 8;
     encoder->starting_bit_rate = starting_bit_rate;
 
diff --git a/server/red_worker.c b/server/red_worker.c
index 6587047..fa24412 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -701,9 +701,6 @@ struct DisplayChannelClient {
     uint32_t palette_cache_items;
 
     struct {
-        uint32_t stream_outbuf_size;
-        uint8_t *stream_outbuf; // caution stream buffer is also used as compress bufs!!!
-
         RedCompressBuf *used_compress_bufs;
 
         FreeList free_list;
@@ -8529,6 +8526,12 @@ static inline void display_begin_send_message(RedChannelClient *rcc)
     red_channel_client_begin_send_message(rcc);
 }
 
+static void red_release_video_encoder_buffer(uint8_t *data, void *opaque)
+{
+    VideoBuffer *buffer = (VideoBuffer*)opaque;
+    buffer->unref(buffer);
+}
+
 static inline int red_marshall_stream_data(RedChannelClient *rcc,
                   SpiceMarshaller *base_marshaller, Drawable *drawable)
 {
@@ -8538,7 +8541,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
     SpiceImage *image;
     RedWorker *worker = dcc->common.worker;
     uint32_t frame_mm_time;
-    int n;
+    VideoBuffer *outbuf;
     int width, height;
     int ret;
 
@@ -8571,7 +8574,6 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
 
     StreamAgent *agent = &dcc->stream_agents[get_stream_id(worker, stream)];
     uint64_t time_now = red_now();
-    size_t outbuf_size;
 
     if (!dcc->use_video_encoder_rate_control) {
         if (time_now - agent->last_send_time < (1000 * 1000 * 1000) / agent->fps) {
@@ -8588,12 +8590,11 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
                         drawable->red_drawable->mm_time :
                         reds_get_mm_time();
 
-    outbuf_size = dcc->send_data.stream_outbuf_size;
     ret = agent->video_encoder->encode_frame(agent->video_encoder,
             &image->u.bitmap, width, height,
             &drawable->red_drawable->u.copy.src_area,
             stream->top_down, frame_mm_time,
-            &dcc->send_data.stream_outbuf, &outbuf_size, &n);
+            &outbuf);
 
     switch (ret) {
     case VIDEO_ENCODER_FRAME_DROP:
@@ -8610,7 +8611,6 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
         spice_error("bad return value (%d) from VideoEncoder::encode_frame", ret);
         return FALSE;
     }
-    dcc->send_data.stream_outbuf_size = outbuf_size;
 
     if (!drawable->sized_stream) {
         SpiceMsgDisplayStreamData stream_data;
@@ -8619,7 +8619,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
 
         stream_data.base.id = get_stream_id(worker, stream);
         stream_data.base.multi_media_time = frame_mm_time;
-        stream_data.data_size = n;
+        stream_data.data_size = outbuf->size;
 
         spice_marshall_msg_display_stream_data(base_marshaller, &stream_data);
     } else {
@@ -8629,7 +8629,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
 
         stream_data.base.id = get_stream_id(worker, stream);
         stream_data.base.multi_media_time = frame_mm_time;
-        stream_data.data_size = n;
+        stream_data.data_size = outbuf->size;
         stream_data.width = width;
         stream_data.height = height;
         stream_data.dest = drawable->red_drawable->bbox;
@@ -8638,12 +8638,12 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
         rect_debug(&stream_data.dest);
         spice_marshall_msg_display_stream_data_sized(base_marshaller, &stream_data);
     }
-    spice_marshaller_add_ref(base_marshaller,
-                             dcc->send_data.stream_outbuf, n);
+    spice_marshaller_add_ref_full(base_marshaller, outbuf->data, outbuf->size,
+                                  &red_release_video_encoder_buffer, outbuf);
     agent->last_send_time = time_now;
 #ifdef STREAM_STATS
     agent->stats.num_frames_sent++;
-    agent->stats.size_sent += n;
+    agent->stats.size_sent += outbuf->size;
     agent->stats.end = frame_mm_time;
 #endif
 
@@ -9405,7 +9405,6 @@ static void display_channel_client_on_disconnect(RedChannelClient *rcc)
     red_release_pixmap_cache(dcc);
     red_release_glz(dcc);
     red_reset_palette_cache(dcc);
-    free(dcc->send_data.stream_outbuf);
     red_display_reset_compress_buf(dcc);
     free(dcc->send_data.free_list.res);
     red_display_destroy_streams_agents(dcc);
@@ -10776,7 +10775,6 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
 {
     DisplayChannel *display_channel;
     DisplayChannelClient *dcc;
-    size_t stream_buf_size;
 
     if (!worker->display_channel) {
         spice_warning("Display channel was not created");
@@ -10792,9 +10790,6 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
         return;
     }
     spice_info("New display (client %p) dcc %p stream %p", client, dcc, stream);
-    stream_buf_size = 32*1024;
-    dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
-    dcc->send_data.stream_outbuf_size = stream_buf_size;
     red_display_init_glz_data(dcc);
 
     dcc->send_data.free_list.res =
diff --git a/server/video_encoder.h b/server/video_encoder.h
index ac96589..fb378c3 100644
--- a/server/video_encoder.h
+++ b/server/video_encoder.h
@@ -23,6 +23,38 @@
 
 #include "red_common.h"
 
+
+typedef struct VideoBuffer VideoBuffer;
+#ifndef VIDEO_BUFFER_T
+# define VIDEO_BUFFER_T VideoBuffer
+#endif
+
+/* A structure containing the data for a compressed frame. See encode_frame(). */
+struct VideoBuffer {
+    /* A pointer to the compressed frame data. */
+    uint8_t *data;
+
+    /* The size of the compressed frame in bytes. */
+    uint32_t size;
+
+    /* The buffer's reference count. */
+    uint32_t ref_count;
+
+    /* Increments the video buffer's reference count and returns the new count.
+     *
+     * @buffer:   The video buffer.
+     * @return:   The new reference count.
+     */
+    VIDEO_BUFFER_T* (*ref)(VIDEO_BUFFER_T *buffer);
+
+    /* Decrements the video buffer's reference count and deallocates it as
+     * appropriate.
+     *
+     * @buffer:   The video buffer.
+     */
+    void (*unref)(VIDEO_BUFFER_T *buffer);
+};
+
 enum {
     VIDEO_ENCODER_FRAME_UNSUPPORTED = -1,
     VIDEO_ENCODER_FRAME_DROP,
@@ -52,11 +84,10 @@ struct VideoEncoder {
      * @height:    The height of the video area. This always matches src.
      * @src:       A rectangle specifying the area occupied by the video.
      * @top_down:  If true the first video line is specified by src.top.
-     * @outbuf:    The buffer for the compressed frame. This must either be
-     *             NULL or point to a buffer allocated by malloc since it may be
-     *             reallocated, if its size is too small.
-     * @outbuf_size: The size of the outbuf buffer.
-     * @data_size: The size of the compressed frame.
+     * @buffer:    A pointer to a VideoBuffer structure containing the
+     *             compressed frame if successful. This buffer should be
+     *             unref()-ed as soon as no longer needed, optimally before the
+     *             next call to encode_frame().
      * @return:
      *     VIDEO_ENCODER_FRAME_ENCODE_DONE if successful.
      *     VIDEO_ENCODER_FRAME_UNSUPPORTED if the frame cannot be encoded.
@@ -65,8 +96,8 @@ struct VideoEncoder {
      */
     int (*encode_frame)(VIDEO_ENCODER_T *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);
+                        int top_down, uint32_t frame_mm_time,
+                        VIDEO_BUFFER_T** buffer);
 
     /*
      * Bit rate control methods.
-- 
2.5.0



More information about the Spice-devel mailing list