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

[Frediano Ziglio]

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

Before I forgot this.

Looks like GStreamer when you call gst_buffer_add_video_meta_full
assume that buffer is contiguous. The 8 pixel shift (more or less)
you can see are artifacts due to how the guest send the frames but
basically are bytes inside 2 chunks of data.

Problems happens specifically in gst_video_frame_map_id.

I would revert the patch for the time being adding this call later
in case we have only one chunks (passing metadata is also required
to pass texture directly to VAAPI).

Frediano