[Spice-devel] [spice v11 08/27] server: Let the video encoder manage the compressed buffer

Francois Gouget fgouget at codeweavers.com
Fri Mar 25 12:38:02 UTC 2016


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.

Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
---
 server/dcc-send.c          | 25 +++++++-------
 server/dcc.c               |  5 ---
 server/dcc.h               |  3 --
 server/gstreamer-encoder.c | 59 +++++++++++++++++++++-----------
 server/mjpeg-encoder.c     | 85 +++++++++++++++++++++++++++++-----------------
 server/video-encoder.h     | 26 ++++++++++----
 6 files changed, 126 insertions(+), 77 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 91d6352..3be443e 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -1649,6 +1649,12 @@ static void red_lossy_marshall_qxl_draw_text(RedChannelClient *rcc,
     }
 }
 
+static void red_release_video_encoder_buffer(uint8_t *data, void *opaque)
+{
+    VideoBuffer *buffer = (VideoBuffer*)opaque;
+    buffer->free(buffer);
+}
+
 static int red_marshall_stream_data(RedChannelClient *rcc,
                                     SpiceMarshaller *base_marshaller, Drawable *drawable)
 {
@@ -1657,7 +1663,6 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
     Stream *stream = drawable->stream;
     SpiceImage *image;
     uint32_t frame_mm_time;
-    int n;
     int width, height;
     int ret;
 
@@ -1689,7 +1694,6 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
 
     StreamAgent *agent = &dcc->stream_agents[get_stream_id(display, stream)];
     uint64_t time_now = spice_get_monotonic_time_ns();
-    size_t outbuf_size;
 
     if (!dcc->use_video_encoder_rate_control) {
         if (time_now - agent->last_send_time < (1000 * 1000 * 1000) / agent->fps) {
@@ -1701,19 +1705,17 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
         }
     }
 
+    VideoBuffer *outbuf;
     /* workaround for vga streams */
     frame_mm_time =  drawable->red_drawable->mm_time ?
                         drawable->red_drawable->mm_time :
                         reds_get_mm_time();
-    outbuf_size = dcc->send_data.stream_outbuf_size;
     ret = !agent->video_encoder ? VIDEO_ENCODER_FRAME_UNSUPPORTED :
           agent->video_encoder->encode_frame(agent->video_encoder,
                                              frame_mm_time,
                                              &image->u.bitmap, width, height,
                                              &drawable->red_drawable->u.copy.src_area,
-                                             stream->top_down,
-                                             &dcc->send_data.stream_outbuf,
-                                             &outbuf_size, &n);
+                                             stream->top_down, &outbuf);
     switch (ret) {
     case VIDEO_ENCODER_FRAME_DROP:
         spice_assert(dcc->use_video_encoder_rate_control);
@@ -1729,7 +1731,6 @@ static 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;
@@ -1738,7 +1739,7 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
 
         stream_data.base.id = get_stream_id(display, 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 {
@@ -1748,7 +1749,7 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
 
         stream_data.base.id = get_stream_id(display, 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;
@@ -1757,12 +1758,12 @@ static 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
 
diff --git a/server/dcc.c b/server/dcc.c
index 99b2540..d94d960 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -381,10 +381,6 @@ DisplayChannelClient *dcc_new(DisplayChannel *display,
     // TODO: tune quality according to bandwidth
     dcc->jpeg_quality = 85;
 
-    size_t stream_buf_size;
-    stream_buf_size = 32*1024;
-    dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
-    dcc->send_data.stream_outbuf_size = stream_buf_size;
     dcc->send_data.free_list.res =
         spice_malloc(sizeof(SpiceResourceList) +
                      DISPLAY_FREE_LIST_DEFAULT_SIZE * sizeof(SpiceResourceID));
@@ -492,7 +488,6 @@ void dcc_stop(DisplayChannelClient *dcc)
     dcc->pixmap_cache = NULL;
     dcc_release_glz(dcc);
     dcc_palette_cache_reset(dcc);
-    free(dcc->send_data.stream_outbuf);
     free(dcc->send_data.free_list.res);
     dcc_destroy_stream_agents(dcc);
     dcc_encoders_free(dcc);
diff --git a/server/dcc.h b/server/dcc.h
index 436d0be..502b0eb 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -88,9 +88,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!!!
-
         FreeList free_list;
         uint64_t pixmap_cache_items[MAX_DRAWABLE_PIXMAP_CACHE_ITEMS];
         int num_pixmap_cache_items;
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 09e3678..fc04f78 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -37,6 +37,12 @@ typedef struct {
     uint32_t bpp;
 } SpiceFormatForGStreamer;
 
+typedef struct SpiceGstVideoBuffer {
+    VideoBuffer base;
+    GstBuffer *gst_buffer;
+    GstMapInfo map;
+} SpiceGstVideoBuffer;
+
 typedef struct SpiceGstEncoder {
     VideoEncoder base;
 
@@ -80,6 +86,25 @@ typedef struct SpiceGstEncoder {
 } SpiceGstEncoder;
 
 
+/* ---------- The SpiceGstVideoBuffer implementation ---------- */
+
+static void spice_gst_video_buffer_free(VideoBuffer *video_buffer)
+{
+    SpiceGstVideoBuffer *buffer = (SpiceGstVideoBuffer*)video_buffer;
+    if (buffer->gst_buffer) {
+        gst_buffer_unref(buffer->gst_buffer);
+    }
+    free(buffer);
+}
+
+static SpiceGstVideoBuffer* create_gst_video_buffer(void)
+{
+    SpiceGstVideoBuffer *buffer = spice_new0(SpiceGstVideoBuffer, 1);
+    buffer->base.free = spice_gst_video_buffer_free;
+    return buffer;
+}
+
+
 /* ---------- Miscellaneous SpiceGstEncoder helpers ---------- */
 
 static inline double get_mbps(uint64_t bit_rate)
@@ -461,29 +486,22 @@ static int push_raw_frame(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap,
 
 /* A helper for spice_gst_encoder_encode_frame() */
 static int pull_compressed_buffer(SpiceGstEncoder *encoder,
-                                  uint8_t **outbuf, size_t *outbuf_size,
-                                  int *data_size)
+                                  VideoBuffer **outbuf)
 {
-    spice_return_val_if_fail(outbuf && outbuf_size, VIDEO_ENCODER_FRAME_UNSUPPORTED);
-
     GstSample *sample = gst_app_sink_pull_sample(encoder->appsink);
     if (sample) {
-        GstMapInfo map;
-        GstBuffer *buffer = gst_sample_get_buffer(sample);
-        if (buffer && gst_buffer_map(buffer, &map, GST_MAP_READ)) {
-            int size = gst_buffer_get_size(buffer);
-            if (!*outbuf || *outbuf_size < size) {
-                free(*outbuf);
-                *outbuf = spice_malloc(size);
-                *outbuf_size = size;
-            }
-            /* TODO Try to avoid this copy by changing the GstBuffer handling */
-            memcpy(*outbuf, map.data, size);
-            *data_size = size;
-            gst_buffer_unmap(buffer, &map);
+        SpiceGstVideoBuffer *buffer = create_gst_video_buffer();
+        buffer->gst_buffer = gst_sample_get_buffer(sample);
+        if (buffer->gst_buffer &&
+            gst_buffer_map(buffer->gst_buffer, &buffer->map, GST_MAP_READ)) {
+            buffer->base.data = buffer->map.data;
+            buffer->base.size = gst_buffer_get_size(buffer->gst_buffer);
+            *outbuf = (VideoBuffer*)buffer;
+            gst_buffer_ref(buffer->gst_buffer);
             gst_sample_unref(sample);
             return VIDEO_ENCODER_FRAME_ENCODE_DONE;
         }
+        buffer->base.free((VideoBuffer*)buffer);
         gst_sample_unref(sample);
     }
     spice_debug("failed to pull the compressed buffer");
@@ -505,10 +523,11 @@ static int spice_gst_encoder_encode_frame(VideoEncoder *video_encoder,
                                           const SpiceBitmap *bitmap,
                                           int width, int height,
                                           const SpiceRect *src, int top_down,
-                                          uint8_t **outbuf, size_t *outbuf_size,
-                                          int *data_size)
+                                          VideoBuffer **outbuf)
 {
     SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
+    g_return_val_if_fail(outbuf != NULL, VIDEO_ENCODER_FRAME_UNSUPPORTED);
+    *outbuf = NULL;
 
     if (width != encoder->width || height != encoder->height ||
         encoder->spice_format != bitmap->format) {
@@ -534,7 +553,7 @@ static int spice_gst_encoder_encode_frame(VideoEncoder *video_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, outbuf);
         if (rc != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
             /* The input buffer will be stuck in the pipeline, preventing
              * later ones from being processed. So reset the pipeline.
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index becd12e..debe50a 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -70,6 +70,9 @@ static const int mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
  */
 #define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
 
+/* The compressed buffer initial size. */
+#define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
+
 enum {
     MJPEG_QUALITY_EVAL_TYPE_SET,
     MJPEG_QUALITY_EVAL_TYPE_UPGRADE,
@@ -154,6 +157,11 @@ typedef struct MJpegEncoderRateControl {
     uint64_t warmup_start_time;
 } MJpegEncoderRateControl;
 
+typedef struct MJpegVideoBuffer {
+    VideoBuffer base;
+    size_t maxsize;
+} MJpegVideoBuffer;
+
 typedef struct MJpegEncoder {
     VideoEncoder base;
     uint8_t *row;
@@ -180,6 +188,26 @@ static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
                                                 uint64_t byte_rate,
                                                 uint32_t latency);
 
+static void mjpeg_video_buffer_free(VideoBuffer *video_buffer)
+{
+    MJpegVideoBuffer *buffer = (MJpegVideoBuffer*)video_buffer;
+    free(buffer->base.data);
+    free(buffer);
+}
+
+static MJpegVideoBuffer* create_mjpeg_video_buffer(void)
+{
+    MJpegVideoBuffer *buffer = spice_new0(MJpegVideoBuffer, 1);
+    buffer->base.free = mjpeg_video_buffer_free;
+    buffer->maxsize = MJPEG_INITIAL_BUFFER_SIZE;
+    buffer->base.data = malloc(buffer->maxsize);
+    if (!buffer->base.data) {
+        free(buffer);
+        buffer = NULL;
+    }
+    return buffer;
+}
+
 static inline int rate_control_is_active(MJpegEncoder* encoder)
 {
     return encoder->cbs.get_roundtrip_ms != NULL;
@@ -283,24 +311,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.
@@ -315,13 +341,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;
@@ -707,7 +726,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;
@@ -789,7 +808,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;
@@ -930,26 +949,30 @@ static int mjpeg_encoder_encode_frame(VideoEncoder *video_encoder,
                                       const SpiceBitmap *bitmap,
                                       int width, int height,
                                       const SpiceRect *src, int top_down,
-                                      uint8_t **outbuf, size_t *outbuf_size,
-                                      int *data_size)
+                                      VideoBuffer **outbuf)
 {
     MJpegEncoder *encoder = (MJpegEncoder*)video_encoder;
+    MJpegVideoBuffer *buffer = create_mjpeg_video_buffer();
+    if (!buffer) {
+        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+    }
 
     int 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;
+                                        buffer, frame_mm_time);
+    if (ret == VIDEO_ENCODER_FRAME_ENCODE_DONE) {
+        if (encode_frame(encoder, src, bitmap, top_down)) {
+            buffer->base.size = mjpeg_encoder_end_frame(encoder);
+            *outbuf = (VideoBuffer*)buffer;
+        } else {
+            ret = VIDEO_ENCODER_FRAME_UNSUPPORTED;
+        }
     }
 
-    if (!encode_frame(encoder, src, bitmap, top_down)) {
-        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+    if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
+        buffer->base.free((VideoBuffer*)buffer);
     }
-
-    *data_size = mjpeg_encoder_end_frame(encoder);
-
-    return VIDEO_ENCODER_FRAME_ENCODE_DONE;
+    return ret;
 }
 
 
diff --git a/server/video-encoder.h b/server/video-encoder.h
index 8f3807b..f94fd69 100644
--- a/server/video-encoder.h
+++ b/server/video-encoder.h
@@ -21,6 +21,22 @@
 #ifndef _H_VIDEO_ENCODER
 #define _H_VIDEO_ENCODER
 
+/* A structure containing the data for a compressed frame. See encode_frame(). */
+typedef struct VideoBuffer VideoBuffer;
+struct VideoBuffer {
+    /* A pointer to the compressed frame data. */
+    uint8_t *data;
+
+    /* The size of the compressed frame in bytes. */
+    uint32_t size;
+
+    /* Releases the video buffer resources and deallocates it.
+     *
+     * @buffer:   The video buffer.
+     */
+    void (*free)(VideoBuffer *buffer);
+};
+
 enum {
     VIDEO_ENCODER_FRAME_UNSUPPORTED = -1,
     VIDEO_ENCODER_FRAME_DROP,
@@ -45,11 +61,9 @@ struct VideoEncoder {
      * @bitmap:        The Spice screen.
      * @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.
+     * @outbuf:        A pointer to a VideoBuffer structure containing the
+     *                 compressed frame if successful. Call the buffer's
+     *                 free() method as soon as it is no longer needed.
      * @return:
      *     VIDEO_ENCODER_FRAME_ENCODE_DONE if successful.
      *     VIDEO_ENCODER_FRAME_UNSUPPORTED if the frame cannot be encoded.
@@ -59,7 +73,7 @@ struct VideoEncoder {
     int (*encode_frame)(VideoEncoder *encoder, uint32_t frame_mm_time,
                         const SpiceBitmap *bitmap, int width, int height,
                         const SpiceRect *src, int top_down,
-                        uint8_t **outbuf, size_t *outbuf_size, int *data_size);
+                        VideoBuffer** outbuf);
 
     /*
      * Bit rate control methods.
-- 
2.7.0



More information about the Spice-devel mailing list