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