[Spice-devel] [client v2 5/5] streaming: Separate the network code from the display_stream management

[Christophe Fergeau]

Hey,

On Tue, May 02, 2017 at 01:05:45PM +0200, Francois Gouget wrote:
> On Mon, 24 Apr 2017, Christophe Fergeau wrote:
> [...]
> > > +    c->streams[op->id] = display_stream_create(channel, op->surface_id, op->flags, op->codec_type);
> > > +    if (c->streams[op->id] == NULL) {
> > > +        spice_printerr("could not create the %u video stream", op->id);
> > >          destroy_stream(channel, op->id);
> > >          report_invalid_stream(channel, op->id);
> > > +    } else {
> > > +        c->streams[op->id]->dest = op->dest;
> > > +        c->streams[op->id]->clip = op->clip;
> > 
> > Any reason why you initialize everything in display_stream_create()
> > except these SpiceRect/SpiceClip instances? I'd just squash this in:
> 
> There's no big reason.
>  * Concerning dest, setting it is required for the stream to work so 
>    it could make sense to set it up in display_stream_create().
>  * But one does not need to set an explicit clip region to use the 
>    stream and as is it defaults to SPICE_CLIP_TYPE_NONE which I think 
>    makes sense. So I'd leave setting it up to the caller so it is 
>    optional.
> 
> So I'd propose another variant where display_stream_create() takes a 
> rect parameter but leaves clip to its default. See the attached patch. 
> Feel free to pick any of the three variants.

In the current code, clip is not really optional as it's unconditionally
set after calling display_stream_create(). I'll push with my variant,
and we can revise how this all works and make this optional when/if the
code you alude to gets public :)

Thanks,

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/20170509/e8b7cedc/attachment.sig>