[Spice-devel] [PATCH spice-server v2] Revert "gstreamer: Avoid memory copy if strides are different"
Christophe Fergeau
cfergeau at redhat.com
Wed Mar 8 16:23:59 UTC 2017
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 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 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170308/bc10eb50/attachment-0001.sig>
More information about the Spice-devel
mailing list