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 <[email protected]> > --- > > 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
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
pgpgldexXTkJu.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/spice-devel
