[Spice-devel] [PATCH v5 4/5] gstreamer: Avoid memory copy if strides are different

[Christophe Fergeau]

On Tue, Jan 10, 2017 at 02:20:40PM +0000, Frediano Ziglio wrote:
> If bitmap stride and stream stride are different copy was used.
> Using GStreamer 1.0 you can avoid the copy setting correctly
> image offset and stride.

Oh that's nice!

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/gstreamer-encoder.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index ba81377..46ae7f7 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -1338,8 +1338,8 @@ static int push_raw_frame(SpiceGstEncoder *encoder,
>                            gpointer bitmap_opaque)
>  {
>      uint32_t height = src->bottom - src->top;
> -    uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8;
> -    uint32_t len = stream_stride * height;
> +    uint32_t len;
> +    uint32_t chunk_index = 0;
>      GstBuffer *buffer = gst_buffer_new();
>      /* TODO Use GST_MAP_INFO_INIT once GStreamer 1.4.5 is no longer relevant */
>      GstMapInfo map = { .memory = NULL };
> @@ -1350,6 +1350,10 @@ 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;
>  
> +#ifdef HAVE_GSTREAMER_0_10

I'd use #ifndef DO_ZERO_COPY here

> +    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.
> @@ -1365,9 +1369,18 @@ static int push_raw_frame(SpiceGstEncoder *encoder,
>              return VIDEO_ENCODER_FRAME_UNSUPPORTED;
>          }
>      } else {
> +#else
> +    if (TRUE) {

a single '{' instead of 'if (TRUE) {' should do the trick here

> +        /* using GStreamer 1.0 we can avoid to cropping the image if the
> +         * stride is different simply passing appropriate offset and stride */
> +        gsize offset[GST_VIDEO_MAX_PLANES] = {src->left * encoder->format->bpp / 8, 0, 0, 0};
> +        gint stride[GST_VIDEO_MAX_PLANES] = {bitmap->stride, 0, 0, 0};
> +        gst_buffer_add_video_meta_full(buffer, GST_VIDEO_FRAME_FLAG_NONE,
> +            encoder->format->gst_format, bitmap->x, bitmap->y, 1, offset, stride);

I'd use
 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);

Can you send a v2 with these changes, and cc: Francois, his review would
be very useful here in my opinion.

Christophe
-------------- 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/20170120/0431037e/attachment.sig>