[Spice-devel] GStreamer's zero-copy code is broken

[Francois Gouget]

On Fri, 24 Feb 2017, Francois Gouget wrote:

> On Fri, 24 Feb 2017, Christophe de Dinechin wrote:
> [...]
> > Looking at the patch, I wonder why we pass an offset and a stride from 
> > different sources (offset is from the source, but stride is from the 
> > bitmap)? Shouldn’t we use the stream stride?
> 
> I think that's correct:
>  * bitmap->{x,y} are the width and height of the source image.
> 
>  * bitmap->stride is the size in bytes of each line. In other words 
>    that's how much you must add to the pointer to get the pixel of the 
>    next line.
> 
>  * src->{left,right,top,bottom} identifies which subset of the source 
>    image should be encoded and sent to the client. So the offset 
>    corresponds to the src->left.
> 
> However I don't see any way to tell gst_buffer_add_video_meta_full() 
> that you only want to take 400 pixels out of every 640 pixel line, or 
> only 200 lines out of the 480 lines.
> 
> For instance if we have
>   bitmap->x = 640
>   bitmap->y = 480
>   bitmap->stride = 2560
>   src->left = src->top = 0
>   src->right = 200
>   src->bottom = 100
> 
> Then we will call:
> 
> gst_buffer_add_video_meta_full(buffer, GST_VIDEO_FRAME_FLAG_NONE,
>     encoder->format->gst_format, 
>     640 /* bitmap->x */, 480 /* bitmap->y */,
>     1,
>     { 0 } /* offset */,
>     { 2560 } /* stride */);
> 
> How will GStreamer know that it should encode a 200x100 video since we 
> did not tell it that:
>     src->right - src->left = 200
>     src>bottom - src->top = 100

Actually the GStreamer pipeline 'knows' the size of the area to encode 
through the source caps which are then (if I remember correctly) 
attached to the buffer.

Also src->top is taken into account through the chunk_offset value which 
is the offset in bytes into the bitmap data computed by zero_copy().


-- 
Francois Gouget <fgouget at codeweavers.com>