Hi, On Mon, Jul 25, 2016 at 07:11:05PM +0200, Francois Gouget wrote: > Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx> > --- > > As far as I can tell there is no way to tell the server that the stream > creation failed. So the client will still receive the video as a stream > and won't be able to display it. But at least it will not crash. Right. I think we should consider improving the protocol to handle that or maybe using the SPICE_MSGC_DISPLAY_STREAM_REPORT to handle failures on client side? Don't know.. https://cgit.freedesktop.org/spice/spice-protocol/commit/?id=a04cc68ba16c5ad8cf1f35b61c1464 Minor comments bellow. > > src/channel-display.c | 49 +++++++++++++++++++++++++++---------------------- > 1 file changed, 27 insertions(+), 22 deletions(-) > > diff --git a/src/channel-display.c b/src/channel-display.c > index cf9c583..c9d2710 100644 > --- a/src/channel-display.c > +++ b/src/channel-display.c > @@ -109,6 +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 display_session_mm_time_reset_cb(SpiceSession *session, gpointer data); > static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout); > > @@ -1114,18 +1115,18 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in) > #ifdef HAVE_BUILTIN_MJPEG > case SPICE_VIDEO_CODEC_TYPE_MJPEG: > st->video_decoder = create_mjpeg_decoder(op->codec_type, st); > - break; > + return; > #endif > default: > #ifdef HAVE_GSTVIDEO > st->video_decoder = create_gstreamer_decoder(op->codec_type, st); > + return; > #else > - st->video_decoder = NULL; > + break; > #endif > } > - if (st->video_decoder == NULL) { > - spice_printerr("could not create a video decoder for codec %u", op->codec_type); > - } > + spice_printerr("could not create a video decoder for codec %u", op->codec_type); > + destroy_stream(channel, op->id); > } > > static const SpiceRect *stream_get_dest(display_stream *st, SpiceMsgIn *frame_msg) > @@ -1346,6 +1347,7 @@ static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in) > g_return_if_fail(c->nstreams > op->id); > > st = c->streams[op->id]; > + g_return_if_fail(st != NULL); > mmtime = stream_get_time(st); > > if (spice_msg_in_type(in) == SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) { > @@ -1413,6 +1415,7 @@ static void display_handle_stream_clip(SpiceChannel *channel, SpiceMsgIn *in) > g_return_if_fail(c->nstreams > op->id); > > st = c->streams[op->id]; > + g_return_if_fail(st != NULL); > > if (st->msg_clip) { > spice_msg_in_unref(st->msg_clip); > @@ -1439,20 +1442,21 @@ static void destroy_stream(SpiceChannel *channel, int id) > if (!st) > return; > > - 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 " > - "#drops-on-receive=%u avg-late-time(ms)=%.2f " > - "#drops-on-playback=%u", __FUNCTION__, > - id, > - st->num_input_frames, > - num_out_frames / (double)st->num_input_frames, > - st->arrive_late_count, > - 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); > - } > - for (i = 0; i < st->num_drops_seqs; i++) { > + if (st->num_input_frames) { Although the code is a bit mixed, from non booleans/pointers we usually prefer explicit checks like (st->num_input_frams > 0) Could you also move num_out_frames and drops_duration_total to this block? > + 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 " > + "#drops-on-receive=%u avg-late-time(ms)=%.2f " > + "#drops-on-playback=%u", __FUNCTION__, > + id, > + st->num_input_frames, > + num_out_frames / (double)st->num_input_frames, > + st->arrive_late_count, > + 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); > + } > + 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); > @@ -1461,9 +1465,10 @@ static void destroy_stream(SpiceChannel *channel, int id) > 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); > + } > + if (st->num_drops_seqs) { > + CHANNEL_DEBUG(channel, "%s: drops-total-duration=%"G_GUINT64_FORMAT" ==>", __FUNCTION__, drops_duration_total); > + } > } > > g_array_free(st->drops_seqs_stats_arr, TRUE); display_update_stream_report() is the only place that access priv->streams[id] but does log a critical if NULL. That should be covered by display_handle_stream_data() check but you might want to add the check just in case the code changes in the future? Does not seem necessary but I thought it would be better to highlight it. Anyway, I could do the changes that you agree on before pushing this. Let me know what you think about *_DISPLAY_STREAM_REPORT Acked-by: Victor Toso <victortoso@xxxxxxxxxx> Cheers, toso _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel