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

Submitted by Francois Gouget on May 2, 2017, 11:05 a.m.

Details

Message ID alpine.DEB.2.20.1705021203430.5974@amboise
State New
Headers show
Series "Abstract video streaming from the network details" ( rev: 4 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget May 2, 2017, 11:05 a.m.
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.


I have a use case here where I need to create a stream before I have 
received the rect and where I don't use clipping at all which is why I 
naturally made these optional. But that code is not public (yet anyway) 
and one could argue that the rect should be known at stream creation 
time. In any case it would be trivial to initialize local variables to 
dummy values and pass them to display_stream_create() so any variant 
would be easy for me to adjust to.

(And I apologize for the delay, my Internet connection broke down last 
week and then I was away for the extended week-end)

Patch hide | download patch | download mbox

commit 6a3bd17fa881c3d95c3c3e923161137e147660b6
Author: Francois Gouget <fgouget@codeweavers.com>
Date:   Fri May 20 19:32:42 2016 +0200

    streaming: Separate the network code from the display_stream management
    
    This makes it easier to reuse display_streams for other types of
    video streams should the need arise.
    
    Signed-off-by: Francois Gouget <fgouget@codeweavers.com>

diff --git a/src/channel-display.c b/src/channel-display.c
index 00b54406..c129e3ba 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,59 @@  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, const SpiceRect *dest)
+{
+    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+    display_stream *st = g_new0(display_stream, 1);
+
+    st->flags = flags;
+    st->dest = *dest;
+    /* st->clip defaults to SPICE_CLIP_TYPE_NONE */
+    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 +1236,14 @@  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, &op->dest);
+    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]->clip = op->clip;
     }
 }
 
@@ -1503,24 +1529,14 @@  static void display_handle_stream_clip(SpiceChannel *channel, SpiceMsgIn *in)
     display_update_stream_region(st);
 }
 
-static void destroy_stream(SpiceChannel *channel, int id)
+static void destroy_display_stream(display_stream *st, int id)
 {
-    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
-    display_stream *st;
     int i;
 
-    g_return_if_fail(c != NULL);
-    g_return_if_fail(c->streams != NULL);
-    g_return_if_fail(c->nstreams > id);
-
-    st = c->streams[id];
-    if (!st)
-        return;
-
     if (st->num_input_frames > 0) {
         guint64 drops_duration_total = 0;
         guint32 num_out_frames = st->num_input_frames - st->arrive_late_count - st->num_drops_on_playback;
-        CHANNEL_DEBUG(channel, "%s: id=%d #in-frames=%u out/in=%.2f "
+        CHANNEL_DEBUG(st->channel, "%s: id=%d #in-frames=%u out/in=%.2f "
             "#drops-on-receive=%u avg-late-time(ms)=%.2f "
             "#drops-on-playback=%u", __FUNCTION__,
             id,
@@ -1530,20 +1546,20 @@  static void destroy_stream(SpiceChannel *channel, int id)
             st->arrive_late_count ? st->arrive_late_time / ((double)st->arrive_late_count): 0,
             st->num_drops_on_playback);
         if (st->num_drops_seqs) {
-            CHANNEL_DEBUG(channel, "%s: #drops-sequences=%u ==>", __FUNCTION__, st->num_drops_seqs);
+            CHANNEL_DEBUG(st->channel, "%s: #drops-sequences=%u ==>", __FUNCTION__, st->num_drops_seqs);
         }
         for (i = 0; i < st->num_drops_seqs; i++) {
             drops_sequence_stats *stats = &g_array_index(st->drops_seqs_stats_arr,
                                                          drops_sequence_stats,
                                                          i);
             drops_duration_total += stats->duration;
-            CHANNEL_DEBUG(channel, "%s: \t len=%u start-ms=%u duration-ms=%u", __FUNCTION__,
-                                   stats->len,
-                                   stats->start_mm_time - st->first_frame_mm_time,
-                                   stats->duration);
+            CHANNEL_DEBUG(st->channel, "%s: \t len=%u start-ms=%u duration-ms=%u", __FUNCTION__,
+                          stats->len,
+                          stats->start_mm_time - st->first_frame_mm_time,
+                          stats->duration);
         }
         if (st->num_drops_seqs) {
-            CHANNEL_DEBUG(channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, drops_duration_total);
+            CHANNEL_DEBUG(st->channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, drops_duration_total);
         }
     }
 
@@ -1554,7 +1570,6 @@  static void destroy_stream(SpiceChannel *channel, int id)
     }
 
     g_free(st);
-    c->streams[id] = NULL;
 }
 
 static void clear_streams(SpiceChannel *channel)

Comments

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
On Tue, 9 May 2017, Christophe Fergeau wrote:
[...]
> > 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().

It is in the sense that if you were to remove the code setting it on the 
caller side, the stream code would not behave erratically due to invalid 
or uninitialized data.


> 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 :)

Sure. That totally works for me!