[Spice-devel] [PATCH spice-server v3] Revert "gstreamer: Avoid memory copy if strides are different"

[Frediano Ziglio]

This reverts commit c3d237075b994fe67edddd58f2b3164cb579e6f4.

When you call gst_buffer_add_video_meta_full GStreamer assumes that
buffer is contiguous. Specifically GStreamer assumes that the first
chunk is big enough to hold a whole frame. 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.

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 at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
---
 server/gstreamer-encoder.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

Changes since v2:
- improve comment.

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