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

[Christophe Fergeau]

On Thu, Apr 06, 2017 at 04:03:01PM +0200, Francois Gouget wrote:
> This makes it easier to reuse display_streams for other types of
> video streams should the need arise.
> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  src/channel-display.c | 110 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 62 insertions(+), 48 deletions(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 00b54406..6b0efaff 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -109,7 +109,7 @@ static display_surface *find_surface(SpiceDisplayChannelPrivate *c, guint32 surf
>  static void spice_display_channel_reset(SpiceChannel *channel, gboolean migrating);
>  static void spice_display_channel_reset_capabilities(SpiceChannel *channel);
>  static void destroy_canvas(display_surface *surface);
> -static void destroy_stream(SpiceChannel *channel, int id);
> +static void destroy_display_stream(display_stream *st, int id);
>  static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer data);
>  static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
>  
> @@ -1168,11 +1168,57 @@ static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
>  }
>  
>  /* coroutine context */
> +static display_stream *display_stream_create(SpiceChannel *channel, uint32_t surface_id, uint32_t flags, uint32_t codec_type)
> +{
> +    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> +    display_stream *st = g_new0(display_stream, 1);
> +
> +    st->flags = flags;
> +    st->surface = find_surface(c, surface_id);
> +    st->channel = channel;
> +    st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE, sizeof(drops_sequence_stats));
> +
> +    region_init(&st->region);
> +    display_update_stream_region(st);
> +
> +    switch (codec_type) {
> +#ifdef HAVE_BUILTIN_MJPEG
> +    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> +        st->video_decoder = create_mjpeg_decoder(codec_type, st);
> +        break;
> +#endif
> +    default:
> +#ifdef HAVE_GSTVIDEO
> +        st->video_decoder = create_gstreamer_decoder(codec_type, st);
> +#endif
> +        break;
> +    }
> +    if (st->video_decoder == NULL) {
> +        spice_printerr("could not create a video decoder for codec %u", codec_type);
> +        destroy_display_stream(st, 0);
> +        st = NULL;
> +    }
> +    return st;
> +}
> +
> +static void destroy_stream(SpiceChannel *channel, int id)
> +{
> +    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> +
> +    g_return_if_fail(c != NULL);
> +    g_return_if_fail(c->streams != NULL);
> +    g_return_if_fail(c->nstreams > id);
> +
> +    if (c->streams[id]) {
> +        destroy_display_stream(c->streams[id], id);
> +        c->streams[id] = NULL;
> +    }
> +}
> +
>  static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
>  {
>      SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
>      SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(in);
> -    display_stream *st;
>  
>      CHANNEL_DEBUG(channel, "%s: id %u", __FUNCTION__, op->id);
>  
> @@ -1188,36 +1234,15 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
>          memset(c->streams + n, 0, (c->nstreams - n) * sizeof(c->streams[0]));
>      }
>      g_return_if_fail(c->streams[op->id] == NULL);
> -    c->streams[op->id] = g_new0(display_stream, 1);
> -    st = c->streams[op->id];
> -
> -    st->flags = op->flags;
> -    st->dest = op->dest;
> -    st->clip = op->clip;
> -    st->surface = find_surface(c, op->surface_id);
> -    st->channel = channel;
> -    st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE, sizeof(drops_sequence_stats));
>  
> -    region_init(&st->region);
> -    display_update_stream_region(st);
> -
> -    switch (op->codec_type) {
> -#ifdef HAVE_BUILTIN_MJPEG
> -    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> -        st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
> -        break;
> -#endif
> -    default:
> -#ifdef HAVE_GSTVIDEO
> -        st->video_decoder = create_gstreamer_decoder(op->codec_type, st);
> -#else
> -        st->video_decoder = NULL;
> -#endif
> -    }
> -    if (st->video_decoder == NULL) {
> -        spice_printerr("could not create a video decoder for codec %u", op->codec_type);
> +    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:

diff --git a/src/channel-display.c b/src/channel-display.c
index 6b0efaff..ccaa7477 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1168,12 +1168,16 @@ static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
 }
 
 /* coroutine context */
-static display_stream *display_stream_create(SpiceChannel *channel, uint32_t surface_id, uint32_t flags, uint32_t codec_type)
+static display_stream *display_stream_create(SpiceChannel *channel, uint32_t surface_id,
+                                             uint32_t flags, uint32_t codec_type,
+                                             const SpiceRect *dest, const SpiceClip *clip)
 {
     SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
     display_stream *st = g_new0(display_stream, 1);
 
     st->flags = flags;
+    st->dest = *dest;
+    st->clip = *clip;
     st->surface = find_surface(c, surface_id);
     st->channel = channel;
     st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE, sizeof(drops_sequence_stats));
@@ -1235,14 +1239,13 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
     }
     g_return_if_fail(c->streams[op->id] == NULL);
 
-    c->streams[op->id] = display_stream_create(channel, op->surface_id, op->flags, op->codec_type);
+    c->streams[op->id] = display_stream_create(channel, op->surface_id,
+                                               op->flags, op->codec_type,
+                                               &op->dest, &op->clip);
     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;
     }
 }
 

Apart from this, looks good to me.

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/20170424/58feff11/attachment-0001.sig>