[Spice-devel,v5,09/20] server: Avoid copying the input frame in the GStreamer encoder.

Submitted by Francois Gouget on Aug. 27, 2015, 7:01 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Francois Gouget Aug. 27, 2015, 7:01 p.m.
To do so we wrap the source bitmap chunks in GstMemory objects that become part of the GStreamer buffer, and rely on the buffer's lifetime being short enough.
Note that we can only avoid copies for the first 1 Mpixels or so. That's because Spice splits larger frames into more chunks than we can fit GstMemory fragments in a GStreamer buffer. So if there are more pixels we will avoid copies for the first 3840 KB and copy the rest.

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

Changes since take 4:
 - Moved the zero-copy code to a zero_copy() helper function.


 server/gstreamer_encoder.c | 131 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 125 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
index f20d0f8..be18101 100644
--- a/server/gstreamer_encoder.c
+++ b/server/gstreamer_encoder.c
@@ -35,6 +35,10 @@  typedef struct GstEncoder GstEncoder;
 
 #define GSTE_DEFAULT_FPS 30
 
+#ifndef HAVE_GSTREAMER_0_10
+# define DO_ZERO_COPY
+#endif
+
 
 typedef struct {
     SpiceBitmapFmt spice_format;
@@ -87,6 +91,11 @@  struct GstEncoder {
     /* The default video buffer */
     GstVideoBuffer *default_buffer;
 
+#ifdef DO_ZERO_COPY
+    /* Set to TRUE until GStreamer no longer needs the raw bitmap buffer. */
+    gboolean needs_bitmap;
+#endif
+
     /* The frame counter for GStreamer buffers */
     uint32_t frame;
 
@@ -398,6 +407,76 @@  static inline int line_copy(GstEncoder *encoder, const SpiceBitmap *bitmap,
      return TRUE;
 }
 
+#ifdef DO_ZERO_COPY
+/* A helper for zero_copy() */
+static void unref_bitmap(gpointer mem)
+{
+    GstEncoder *encoder = (GstEncoder*)mem;
+    encoder->needs_bitmap = FALSE;
+}
+
+/* A helper for push_raw_frame(). */
+static inline int zero_copy(GstEncoder *encoder, const SpiceBitmap *bitmap,
+                            GstBuffer *buffer, uint32_t *chunk,
+                            uint32_t *chunk_offset, uint32_t *len)
+{
+    /* We cannot control the lifetime of the bitmap but we can wrap it in
+     * the buffer anyway because:
+     * - Before returning from gst_encoder_encode_frame() we wait for the
+     *   pipeline to have converted this frame into a compressed buffer.
+     *   So it has to have gone through the frame at least once.
+     * - For all encoders but MJPEG, the first element of the pipeline will
+     *   convert the bitmap to another image format which entails copying
+     *   it. So by the time the encoder starts its work, this buffer will
+     *   not be needed anymore.
+     * - The MJPEG encoder does not perform inter-frame compression and thus
+     *   does not need to keep hold of this buffer once it has processed it.
+     * encoder->needs_bitmap lets us verify that these conditions still hold
+     * true through an assert.
+     */
+    SpiceChunks *chunks = bitmap->data;
+    while (*chunk_offset >= chunks->chunk[*chunk].len) {
+        /* Make sure that the chunk is not padded */
+        if (chunks->chunk[*chunk].len % bitmap->stride != 0) {
+        }
+        *chunk_offset -= chunks->chunk[*chunk].len;
+        (*chunk)++;
+    }
+
+    int max_mem = gst_buffer_get_max_memory();
+    if (chunks->num_chunks - *chunk > max_mem) {
+        /* There are more chunks than we can fit memory objects in a
+         * buffer. This will cause the buffer to merge memory objects to
+         * fit the extra chunks, which means doing wasteful memory copies.
+         * So use the zero-copy approach for the first max_mem-1 chunks,
+         * and directly copy the remainder to the last memory object.
+         */
+        max_mem = *chunk + max_mem - 1;
+    } else {
+        max_mem = chunks->num_chunks;
+    }
+
+    while (*len && *chunk < max_mem) {
+        /* Make sure that the chunk is not padded */
+        if (chunks->chunk[*chunk].len % bitmap->stride != 0) {
+            return FALSE;
+        }
+        uint32_t thislen = MIN(chunks->chunk[*chunk].len - *chunk_offset, *len);
+        GstMemory *mem = gst_memory_new_wrapped(GST_MEMORY_FLAG_READONLY,
+                                                chunks->chunk[*chunk].data,
+                                                chunks->chunk[*chunk].len,
+                                                *chunk_offset, thislen,
+                                                encoder, &unref_bitmap);
+        gst_buffer_append_memory(buffer, mem);
+        *len -= thislen;
+        *chunk_offset = 0;
+        (*chunk)++;
+    }
+    encoder->needs_bitmap = TRUE;
+    return TRUE;
+}
+#endif
+
 /* A helper for gst_encoder_encode_frame(). */
 static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
                           const SpiceRect *src, int top_down)
@@ -405,15 +484,14 @@  static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
     const uint32_t height = src->bottom - src->top;
     const uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8;
     uint32_t len = stream_stride * height;
-    GstBuffer *buffer = gst_buffer_new_and_alloc(len);
 #ifdef HAVE_GSTREAMER_0_10
+    GstBuffer *buffer = gst_buffer_new_and_alloc(len);
     uint8_t *b = GST_BUFFER_DATA(buffer);
+    uint8_t *dst = b;
 #else
-    GstMapInfo map;
-    gst_buffer_map(buffer, &map, GST_MAP_WRITE);
-    uint8_t *b = map.data;
+    GstBuffer *buffer = gst_buffer_new();
+    GstMapInfo map = { .memory = NULL };
 #endif
-    uint8_t *dst = b;
 
     /* 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.
@@ -425,8 +503,19 @@  static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
         /* We have to do a line-by-line copy because for each we have to leave
          * out pixels on the left or right.
          */
+#ifndef HAVE_GSTREAMER_0_10
+        GstMemory *mem = gst_allocator_alloc(NULL, len, NULL);
+        gst_memory_map(mem, &map, GST_MAP_WRITE);
+        uint8_t *b = map.data;
+        uint8_t *dst = b;
+#endif
+
         chunk_offset += src->left * encoder->format->bpp / 8;
         if (!line_copy(encoder, bitmap, chunk_offset, stream_stride, height, dst)) {
+#ifndef HAVE_GSTREAMER_0_10
+            gst_memory_unmap(map.memory, &map);
+            gst_memory_unref(map.memory);
+#endif
             gst_buffer_unref(buffer);
             return VIDEO_ENCODER_FRAME_UNSUPPORTED;
         }
@@ -435,9 +524,32 @@  static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
         SpiceChunks *chunks = bitmap->data;
         uint32_t chunk = 0;
         /* We can copy the bitmap chunk by chunk */
+#ifdef DO_ZERO_COPY
+        if (!zero_copy(encoder, bitmap, buffer, &chunk, &chunk_offset, &len)) {
+            gst_buffer_unref(buffer);
+            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+        }
+        /* Now we must avoid any write to the GstBuffer object as it would
+         * cause a copy of the read-only memory objects we just added.
+         * Fortunately we can append extra writable memory objects instead.
+         */
+#endif
+
+#ifndef HAVE_GSTREAMER_0_10
+        if (len) {
+            GstMemory *mem = gst_allocator_alloc(NULL, len, NULL);
+            gst_memory_map(mem, &map, GST_MAP_WRITE);
+        }
+        uint8_t *dst = map.data;
+#endif
+
         while (len && chunk < chunks->num_chunks) {
             /* Make sure that the chunk is not padded */
             if (chunks->chunk[chunk].len % bitmap->stride != 0) {
+#ifndef HAVE_GSTREAMER_0_10
+                gst_memory_unmap(map.memory, &map);
+                gst_memory_unref(map.memory);
+#endif
                 gst_buffer_unref(buffer);
                 return VIDEO_ENCODER_FRAME_UNSUPPORTED;
             }
@@ -460,7 +572,10 @@  static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
 #ifdef HAVE_GSTREAMER_0_10
     gst_buffer_set_caps(buffer, encoder->src_caps);
 #else
-    gst_buffer_unmap(buffer, &map);
+    if (map.memory) {
+        gst_memory_unmap(map.memory, &map);
+        gst_buffer_append_memory(buffer, map.memory);
+    }
 #endif
     GST_BUFFER_OFFSET(buffer) = encoder->frame++;
 
@@ -558,6 +673,10 @@  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, buffer);
+#ifdef DO_ZERO_COPY
+        /* GStreamer should have released the source frame buffer by now */
+        spice_assert(!encoder->needs_bitmap);
+#endif
     }
     return rc;
 }

Comments

On Thu, Aug 27, 2015 at 09:01:10PM +0200, Francois Gouget wrote:
> To do so we wrap the source bitmap chunks in GstMemory objects that become part of the GStreamer buffer, and rely on the buffer's lifetime being short enough.
> Note that we can only avoid copies for the first 1 Mpixels or so. That's because Spice splits larger frames into more chunks than we can fit GstMemory fragments in a GStreamer buffer. So if there are more pixels we will avoid copies for the first 3840 KB and copy the rest.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> Changes since take 4:
>  - Moved the zero-copy code to a zero_copy() helper function.
> 
> 
>  server/gstreamer_encoder.c | 131 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 125 insertions(+), 6 deletions(-)
> 
> diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
> index f20d0f8..be18101 100644
> --- a/server/gstreamer_encoder.c
> +++ b/server/gstreamer_encoder.c
> @@ -35,6 +35,10 @@ typedef struct GstEncoder GstEncoder;
>  
>  #define GSTE_DEFAULT_FPS 30
>  
> +#ifndef HAVE_GSTREAMER_0_10
> +# define DO_ZERO_COPY
> +#endif
> +
>  
>  typedef struct {
>      SpiceBitmapFmt spice_format;
> @@ -87,6 +91,11 @@ struct GstEncoder {
>      /* The default video buffer */
>      GstVideoBuffer *default_buffer;
>  
> +#ifdef DO_ZERO_COPY
> +    /* Set to TRUE until GStreamer no longer needs the raw bitmap buffer. */
> +    gboolean needs_bitmap;
> +#endif
> +
>      /* The frame counter for GStreamer buffers */
>      uint32_t frame;
>  
> @@ -398,6 +407,76 @@ static inline int line_copy(GstEncoder *encoder, const SpiceBitmap *bitmap,
>       return TRUE;
>  }
>  
> +#ifdef DO_ZERO_COPY
> +/* A helper for zero_copy() */
> +static void unref_bitmap(gpointer mem)
> +{
> +    GstEncoder *encoder = (GstEncoder*)mem;
> +    encoder->needs_bitmap = FALSE;
> +}
> +
> +/* A helper for push_raw_frame(). */
> +static inline int zero_copy(GstEncoder *encoder, const SpiceBitmap *bitmap,
> +                            GstBuffer *buffer, uint32_t *chunk,
> +                            uint32_t *chunk_offset, uint32_t *len)
> +{
> +    /* We cannot control the lifetime of the bitmap but we can wrap it in
> +     * the buffer anyway because:
> +     * - Before returning from gst_encoder_encode_frame() we wait for the
> +     *   pipeline to have converted this frame into a compressed buffer.
> +     *   So it has to have gone through the frame at least once.
> +     * - For all encoders but MJPEG, the first element of the pipeline will
> +     *   convert the bitmap to another image format which entails copying
> +     *   it. So by the time the encoder starts its work, this buffer will
> +     *   not be needed anymore.
> +     * - The MJPEG encoder does not perform inter-frame compression and thus
> +     *   does not need to keep hold of this buffer once it has processed it.
> +     * encoder->needs_bitmap lets us verify that these conditions still hold
> +     * true through an assert.
> +     */
> +    SpiceChunks *chunks = bitmap->data;
> +    while (*chunk_offset >= chunks->chunk[*chunk].len) {
> +        /* Make sure that the chunk is not padded */
> +        if (chunks->chunk[*chunk].len % bitmap->stride != 0) {
> +        }

Empty block?

> +        *chunk_offset -= chunks->chunk[*chunk].len;
> +        (*chunk)++;
> +    }

Imo 'chunk' is not a very good name, as the from the naming, I'd expect
it to be a SpiceChunk, not a guint32. Up until here, it really is a
'skipped_chunk_count'

> +
> +    int max_mem = gst_buffer_get_max_memory();
> +    if (chunks->num_chunks - *chunk > max_mem) {
> +        /* There are more chunks than we can fit memory objects in a
> +         * buffer. This will cause the buffer to merge memory objects to
> +         * fit the extra chunks, which means doing wasteful memory copies.
> +         * So use the zero-copy approach for the first max_mem-1 chunks,
> +         * and directly copy the remainder to the last memory object.
> +         */

The copy of the remainder is done in push_raw_frame if I followed the
code correctly?

> +        max_mem = *chunk + max_mem - 1;

max_mem = max_mem - skipped_chunk_count - 1; ?

> +    } else {
> +        max_mem = chunks->num_chunks;
> +    }
> +
> +    while (*len && *chunk < max_mem) {

and here 'chunk' becomes a loop index initialized at
skipped_chunk_count, maybe call it 'i' ? or make it a SpiceChunk *
initialized to chunks->chunk[skipped_chunk_count], this would make the
loop body a bit easier to read.

> +        /* Make sure that the chunk is not padded */
> +        if (chunks->chunk[*chunk].len % bitmap->stride != 0) {
> +            return FALSE;
> +        }
> +        uint32_t thislen = MIN(chunks->chunk[*chunk].len - *chunk_offset, *len);
> +        GstMemory *mem = gst_memory_new_wrapped(GST_MEMORY_FLAG_READONLY,
> +                                                chunks->chunk[*chunk].data,
> +                                                chunks->chunk[*chunk].len,
> +                                                *chunk_offset, thislen,
> +                                                encoder, &unref_bitmap);
> +        gst_buffer_append_memory(buffer, mem);
> +        *len -= thislen;
> +        *chunk_offset = 0;
> +        (*chunk)++;
> +    }
> +    encoder->needs_bitmap = TRUE;
> +    return TRUE;
> +}
> +#endif
> +
>  /* A helper for gst_encoder_encode_frame(). */
>  static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
>                            const SpiceRect *src, int top_down)
> @@ -405,15 +484,14 @@ static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
>      const uint32_t height = src->bottom - src->top;
>      const uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8;
>      uint32_t len = stream_stride * height;
> -    GstBuffer *buffer = gst_buffer_new_and_alloc(len);
>  #ifdef HAVE_GSTREAMER_0_10
> +    GstBuffer *buffer = gst_buffer_new_and_alloc(len);
>      uint8_t *b = GST_BUFFER_DATA(buffer);
> +    uint8_t *dst = b;

Seems like the declaration of uint8_t *dst = GST_BUFFER_DATA(buffer);
could also be moved close to the 'gst_allocator_alloc()' calls in an
inner block.

>  #else
> -    GstMapInfo map;
> -    gst_buffer_map(buffer, &map, GST_MAP_WRITE);
> -    uint8_t *b = map.data;
> +    GstBuffer *buffer = gst_buffer_new();
> +    GstMapInfo map = { .memory = NULL };

Could be GST_MAP_INFO_INIT

>  #endif
> -    uint8_t *dst = b;
>  
>      /* 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.
> @@ -425,8 +503,19 @@ static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
>          /* We have to do a line-by-line copy because for each we have to leave
>           * out pixels on the left or right.
>           */
> +#ifndef HAVE_GSTREAMER_0_10
> +        GstMemory *mem = gst_allocator_alloc(NULL, len, NULL);
> +        gst_memory_map(mem, &map, GST_MAP_WRITE);

Apparently it's possible for gst_memory_map() to fail, would be nicer to
check its return value

> +        uint8_t *b = map.data;
> +        uint8_t *dst = b;
> +#endif

Could gst_buffer_new_allocate() be used instead? I don't think the
temporary 'b' variable is used at all apart for being assigned to 'dst'.

> +
>          chunk_offset += src->left * encoder->format->bpp / 8;
>          if (!line_copy(encoder, bitmap, chunk_offset, stream_stride, height, dst)) {
> +#ifndef HAVE_GSTREAMER_0_10
> +            gst_memory_unmap(map.memory, &map);
> +            gst_memory_unref(map.memory);
> +#endif

Looks like some gst_buffer_unmap() should have been there before this
commit.

>              gst_buffer_unref(buffer);
>              return VIDEO_ENCODER_FRAME_UNSUPPORTED;
>          }
> @@ -435,9 +524,32 @@ static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
>          SpiceChunks *chunks = bitmap->data;
>          uint32_t chunk = 0;
>          /* We can copy the bitmap chunk by chunk */
> +#ifdef DO_ZERO_COPY
> +        if (!zero_copy(encoder, bitmap, buffer, &chunk, &chunk_offset, &len)) {
> +            gst_buffer_unref(buffer);
> +            return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +        }
> +        /* Now we must avoid any write to the GstBuffer object as it would
> +         * cause a copy of the read-only memory objects we just added.
> +         * Fortunately we can append extra writable memory objects instead.
> +         */
> +#endif
> +
> +#ifndef HAVE_GSTREAMER_0_10
> +        if (len) {
> +            GstMemory *mem = gst_allocator_alloc(NULL, len, NULL);
> +            gst_memory_map(mem, &map, GST_MAP_WRITE);
> +        }
> +        uint8_t *dst = map.data;
> +#endif
> +
>          while (len && chunk < chunks->num_chunks) {
>              /* Make sure that the chunk is not padded */
>              if (chunks->chunk[chunk].len % bitmap->stride != 0) {
> +#ifndef HAVE_GSTREAMER_0_10
> +                gst_memory_unmap(map.memory, &map);
> +                gst_memory_unref(map.memory);
> +#endif
>                  gst_buffer_unref(buffer);
>                  return VIDEO_ENCODER_FRAME_UNSUPPORTED;
>              }
> @@ -460,7 +572,10 @@ static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
>  #ifdef HAVE_GSTREAMER_0_10
>      gst_buffer_set_caps(buffer, encoder->src_caps);
>  #else
> -    gst_buffer_unmap(buffer, &map);
> +    if (map.memory) {
> +        gst_memory_unmap(map.memory, &map);
> +        gst_buffer_append_memory(buffer, map.memory);
> +    }
>  #endif
>      GST_BUFFER_OFFSET(buffer) = encoder->frame++;
>  
> @@ -558,6 +673,10 @@ 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, buffer);
> +#ifdef DO_ZERO_COPY
> +        /* GStreamer should have released the source frame buffer by now */
> +        spice_assert(!encoder->needs_bitmap);
> +#endif
>      }
>      return rc;
>  }
> -- 
> 2.5.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel