On Tue, Mar 07, 2017 at 03:52:50PM +0000, Frediano Ziglio wrote: > This reverts commit c3d237075b994fe67edddd58f2b3164cb579e6f4. > > When you call gst_buffer_add_video_meta_full GStreamer assumes > that buffer is contiguous. This results usually in some pixel > shifts in the video. The pixel shifts you can see are artifacts > due to how the guest sends the frames. > > Assuming you allocate the 2 chunks with 2 malloc: > > p1 = malloc(size); > ... > p2 = malloc(size); > > Usually the memory allocator tend to allocate linearly if there are > space adding a prefix to describe next block leading to a memory > arrangement as: > > +-------------------+ > | p1 prefix | > +-------------------+ > | p1 buffer | > +-------------------+ > | p2 prefix | > +-------------------+ > | p2 buffer | > +-------------------+ > > now if you take p1 pointer and you assume it points to a 2 * size > buffer you will get p2 prefix in the middle, this prefix is the > pixel shifts. The bit which was missing to me is that if you add p1 and p2 to a GstBuffer as 2 separate chunks, and then try to use zero copy with such a buffer, then GStreamer will not be able to cope with these 2 separate chunks (unless the chunk is the size of a plane/frame), and will just assume the first chunk is big enough to hold a whole frame, leading to the situation you describe. Maybe you can add something about this to the commit log? Christophe > > Problems happens specifically in gst_video_frame_map_id. > This bug is reported in https://bugzilla.gnome.org/show_bug.cgi?id=779524. > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx> > --- > server/gstreamer-encoder.c | 26 ++++++-------------------- > 1 file changed, 6 insertions(+), 20 deletions(-) > > Changes since v1: > - add more comment on pixel shifts reason. > > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c > index 991eb51..df54cad 100644 > --- a/server/gstreamer-encoder.c > +++ b/server/gstreamer-encoder.c > @@ -1236,6 +1236,8 @@ static void clear_zero_copy_queue(SpiceGstEncoder *encoder, gboolean unref_queue > /* Nothing to do */ > } > > +#endif > + > /* A helper for push_raw_frame() */ > static inline int line_copy(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap, > uint32_t chunk_offset, uint32_t stream_stride, > @@ -1266,8 +1268,6 @@ static inline int line_copy(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap, > return TRUE; > } > > -#endif > - > /* A helper for push_raw_frame() */ > static inline int chunk_copy(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap, > uint32_t chunk_index, uint32_t chunk_offset, > @@ -1350,8 +1350,8 @@ static int push_raw_frame(SpiceGstEncoder *encoder, > gpointer bitmap_opaque) > { > uint32_t height = src->bottom - src->top; > - uint32_t len; > - uint32_t chunk_index = 0; > + uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8; > + uint32_t len = stream_stride * height; > GstBuffer *buffer = gst_buffer_new(); > /* TODO Use GST_MAP_INFO_INIT once GStreamer 1.4.5 is no longer relevant */ > GstMapInfo map = { .memory = NULL }; > @@ -1362,10 +1362,6 @@ static int push_raw_frame(SpiceGstEncoder *encoder, > uint32_t skip_lines = top_down ? src->top : bitmap->y - (src->bottom - 0); > uint32_t chunk_offset = bitmap->stride * skip_lines; > > -#ifndef DO_ZERO_COPY > - uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8; > - > - len = stream_stride * height; > if (stream_stride != bitmap->stride) { > /* We have to do a line-by-line copy because for each we have to > * leave out pixels on the left or right. > @@ -1381,19 +1377,9 @@ static int push_raw_frame(SpiceGstEncoder *encoder, > return VIDEO_ENCODER_FRAME_UNSUPPORTED; > } > } else { > -#else > - { > - /* using GStreamer 1.0, we can avoid cropping the image by simply passing > - * the appropriate offset and stride as metadata */ > - gsize offset[] = { src->left * encoder->format->bpp / 8 }; > - gint stride[] = { bitmap->stride }; > - gst_buffer_add_video_meta_full(buffer, GST_VIDEO_FRAME_FLAG_NONE, > - encoder->format->gst_format, bitmap->x, bitmap->y, > - 1, offset, stride); > - > - len = bitmap->stride * height; > - > /* We can copy the bitmap chunk by chunk */ > + uint32_t chunk_index = 0; > +#ifdef DO_ZERO_COPY > if (!zero_copy(encoder, bitmap, bitmap_opaque, buffer, &chunk_index, > &chunk_offset, &len)) { > gst_buffer_unref(buffer); > -- > 2.9.3 > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel