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@xxxxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel