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

[Francois Gouget]

On Fri, 4 Mar 2016, Christophe Fergeau wrote:
[...]
> > @@ -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.

I'm not too keen on introducing a dependency on RedDrawable. I like that 
encode_frame() only takes parameters that correspond to what it really 
needs so that it could be used to encode things that are not 
RedDrawables. That said that flexibility may be overkill.

Another point is that encode_frame really only supports QXL_DRAW_COPY 
RedDrawables, and only those with the right kind of rop, etc. So passing 
a RedDrawable seems too generic. Maybe the bitmap and src_area should be 
replaced by an SrcCopy object (and width/height removed altogether of 
course), while the bitmap_opaque parameter and bitmap_{ref,unref}() 
functions would be kept around for the memory management aspects.

Certainly there will still be some cleanups to do after this patch 
series. This RedDrawable business could be one, width/height would 
definitely be another, and I think getting rid of 
use_video_encoder_rate_control would be good too.


-- 
Francois Gouget <fgouget at codeweavers.com>