[Spice-devel] [client] streaming: Don't crash if the stream creation fails

[Victor Toso]

Hi,

On Mon, Jul 25, 2016 at 07:11:05PM +0200, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>
> 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 at redhat.com>

Cheers,
  toso