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@xxxxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
pgpW1ZqZkMisz.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel