[Spice-devel] [spice v10 11/27] server: Avoid copying the input frame in the GStreamer encoder

[Christophe Fergeau]

Hey,

On Tue, Mar 01, 2016 at 04:53:36PM +0100, Francois Gouget wrote:
> Note that we can only avoid copies for the first 1 Mpixels or so.
> That's because Spice splits larger frames into more chunks than we can
> fit GstMemory fragments in a GStreamer buffer. So if there are more
> pixels we will avoid copies for the first 3840 KB and copy the rest.
> Furthermore, while in practice the GStreamer encoder will only modify
> the RedDrawable refcount during the encode_frame(), in theory the
> refcount could be decremented from the GStreamer thread after
> encode_frame() returns.
> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  server/dcc-send.c          |  13 +++--
>  server/gstreamer-encoder.c | 137 ++++++++++++++++++++++++++++++++++++++++-----
>  server/mjpeg-encoder.c     |   5 +-
>  server/stream.c            |  18 +++++-
>  server/video-encoder.h     |  23 ++++++--
>  5 files changed, 168 insertions(+), 28 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 68ea085..a24e8ef 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -1656,12 +1656,12 @@ static void red_release_video_encoder_buffer(uint8_t *data, void *opaque)
>  }
>  
>  static int red_marshall_stream_data(RedChannelClient *rcc,
> -                                    SpiceMarshaller *base_marshaller, Drawable *drawable)
> +                                    SpiceMarshaller *base_marshaller,
> +                                    Drawable *drawable)
>  {
>      DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
>      DisplayChannel *display = DCC_TO_DC(dcc);
>      Stream *stream = drawable->stream;
> -    SpiceImage *image;
>      uint32_t frame_mm_time;
>      int width, height;
>      int ret;
> @@ -1670,10 +1670,10 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>          spice_assert(drawable->sized_stream);
>          stream = drawable->sized_stream;
>      }
> -    spice_assert(drawable->red_drawable->type == QXL_DRAW_COPY);
> -
> -    image = drawable->red_drawable->u.copy.src_bitmap;
> +    RedDrawable *red_drawable = drawable->red_drawable;
> +    spice_assert(red_drawable->type == QXL_DRAW_COPY);
>  
> +    SpiceImage *image = red_drawable->u.copy.src_bitmap;
>      if (image->descriptor.type != SPICE_IMAGE_TYPE_BITMAP) {
>          return FALSE;
>      }
> @@ -1714,7 +1714,8 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>                                               frame_mm_time,
>                                               &image->u.bitmap, width, height,
>                                               &drawable->red_drawable->u.copy.src_area,
> -                                             stream->top_down, &outbuf);
> +                                             stream->top_down, red_drawable,
> +                                             &outbuf);

For what it's worth, having both drawable->red_drawable->u.copy.src_area
and red_drawable (which you added in this patch and which value is
drawable->red_drawable) in the parameter list is not really consistent
(especially when there is already image->u.bitmap which is also derived
from drawable->red_drawable even if it's not explicit).
I think we can do without it.

While looking at this series, I was wondering whether we should just
byte the bullet and pass a RedDrawable* to encode_frame() directly
rather than passing image->u.bitmap,
drawable->red_drawable->u.copy.src_area, and now the RedDrawable +
ref/unref methods for that. RedDrawable is just the representation
server-side of a QXL drawable and the resources it uses, so I don't
think it's too bad from a layering point of view.
Then the gstreamer encoder could just call red_drawable_ref/_unref where
appropriate.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160304/3fd08a1c/attachment.sig>