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

[Francois Gouget]

I attached the new patch to give an idea of what things look like now.


On Tue, 22 Sep 2015, Christophe Fergeau wrote:
[...]
> > +    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?

Fixed. I introduced this bug when moving the code to the zero_copy() 
function.


> > +    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?

Yes. I tweaked the comment to make that more explicit.


> > +        max_mem = *chunk + max_mem - 1;
> 
> max_mem = max_mem - skipped_chunk_count - 1; ?

No.
Let's say there are 7 chunks, that we skip the first two, and that we 
can put 4 GstMemory objects a GstBuffer.

chunk 0 -> zero_copy() skip
chunk 1 -> zero_copy() skip

Now chunk == 2, max_mem == 4 and what we want to know is when to exit 
the zero-copy loop:
      while (*len && *chunk < max_mem) {
And that's when *chunk == 2 + 4 -1

chunk 2 -> zero_copy() wrap it in a GstMemory
chunk 3 -> zero_copy() wrap it in a GstMemory
chunk 4 -> zero_copy() wrap it in a GstMemory

Now we return to push_raw_frame() when chunk is now 5 so it will pick up 
where we left off and copy the rest in a single GstMemory object.

chunk 5 -> push_raw_frame() copy to the last GstMemory
chunk 6 -> push_raw_frame() copy to the last GstMemory
chunk 7 -> push_raw_frame() copy to the last GstMemory


> > +    } 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.

It is a loop index all along. Also we need to compare it to max_mem so 
keeping it as an integer is clearer. I've renamed it to chunk_index 
though.


[...]
> > @@ -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.

It's used in two sibling blocks so this would requires duplicating that 
line and some #if check. Not really simpler.


[...]
> > +    GstMapInfo map = { .memory = NULL };
> 
> Could be GST_MAP_INFO_INIT

I didn't know about it. However it turns out it was buggy until 
recently. So I added a TODO.

https://bugzilla.gnome.org/show_bug.cgi?id=751881


[...]
> > +#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

The GstMemory object is allocated with the specific intent of mapping 
it. So if the map operation fails it's either because we're out of 
memory or because of a coding error. So I added a spice_assert() on the 
gst_memory_map() and code to check the allocation.


> > +        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'.

gst_buffer_new_allocate() allocates a new buffer which is not what we 
want. Also note that we cannot use gst_buffer_append() as that would 
cause a copy of all the memory objects of the first buffer, thus 
defeating the purpose of the zero_copy code..

I removed the b variables.


> >          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.

Yep. Added.


-- 
Francois Gouget <fgouget at codeweavers.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 09-copying.diff
Type: text/x-diff
Size: 9702 bytes
Desc: 09-copying.diff
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150929/c3e73899/attachment.diff>