[Spice-devel,client,v5,2/4] streaming: Stop streaming if frames cannot be decoded

Submitted by Francois Gouget on Oct. 31, 2016, 8:25 p.m.

Details

Message ID b08931aa2b90f735dfda6aa5ee86a725675ff61f.1477944750.git.fgouget@free.fr
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget Oct. 31, 2016, 8:25 p.m.
Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 src/channel-display-gst.c   | 41 ++++++++++++++++++++++++++++++++++++-----
 src/channel-display-mjpeg.c |  8 +++++---
 src/channel-display-priv.h  |  4 +++-
 src/channel-display.c       |  7 ++++++-
 4 files changed, 50 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index f52299f..5fb0b77 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -280,6 +280,28 @@  static void free_pipeline(SpiceGstDecoder *decoder)
     decoder->pipeline = NULL;
 }
 
+static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer video_decoder)
+{
+    SpiceGstDecoder *decoder = video_decoder;
+
+    if (GST_MESSAGE_TYPE(msg) == GST_MESSAGE_ERROR) {
+        GError *err = NULL;
+        gchar *debug_info = NULL;
+        gst_message_parse_error(msg, &err, &debug_info);
+        spice_warning("GStreamer error from element %s: %s",
+                      GST_OBJECT_NAME(msg->src), err->message);
+        if (debug_info) {
+            SPICE_DEBUG("debug information: %s", debug_info);
+            g_free(debug_info);
+        }
+        g_clear_error(&err);
+
+        /* We won't be able to process any more frame anyway */
+        free_pipeline(decoder);
+    }
+    return TRUE;
+}
+
 static gboolean create_pipeline(SpiceGstDecoder *decoder)
 {
     gchar *desc;
@@ -324,6 +346,8 @@  static gboolean create_pipeline(SpiceGstDecoder *decoder)
 
     appsink_cbs.new_sample = new_sample;
     gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder, NULL);
+    gst_bus_add_watch(gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline)),
+                      handle_pipeline_message, decoder);
 
     decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder->pipeline));
 
@@ -390,9 +414,9 @@  static void release_buffer_data(gpointer data)
     spice_msg_in_unref(frame_msg);
 }
 
-static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
-                                          SpiceMsgIn *frame_msg,
-                                          int32_t latency)
+static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
+                                              SpiceMsgIn *frame_msg,
+                                              int32_t latency)
 {
     SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
 
@@ -400,7 +424,7 @@  static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
     uint32_t size = spice_msg_in_frame_data(frame_msg, &data);
     if (size == 0) {
         SPICE_DEBUG("got an empty frame buffer!");
-        return;
+        return TRUE;
     }
 
     SpiceStreamDataHeader *frame_op = spice_msg_in_parsed(frame_msg);
@@ -419,7 +443,13 @@  static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
          * saves CPU so do it.
          */
         SPICE_DEBUG("dropping a late MJPEG frame");
-        return;
+        return TRUE;
+    }
+
+    if (decoder->pipeline == NULL) {
+        /* An error occurred, causing the GStreamer pipeline to be freed */
+        spice_warning("An error occurred, stopping the video stream");
+        return FALSE;
     }
 
     /* ref() the frame_msg for the buffer */
@@ -440,6 +470,7 @@  static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
         SPICE_DEBUG("GStreamer error: unable to push frame of size %u", size);
         stream_dropped_frame_on_playback(decoder->base.stream);
     }
+    return TRUE;
 }
 
 static gboolean gstvideo_init(void)
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 4976d53..67746c3 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -235,8 +235,9 @@  static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
 
 /* ---------- VideoDecoder's public API ---------- */
 
-static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
-                                      SpiceMsgIn *frame_msg, int32_t latency)
+static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
+                                          SpiceMsgIn *frame_msg,
+                                          int32_t latency)
 {
     MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
     SpiceMsgIn *last_msg;
@@ -262,12 +263,13 @@  static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
      * So drop late frames as early as possible to save on processing time.
      */
     if (latency < 0) {
-        return;
+        return TRUE;
     }
 
     spice_msg_in_ref(frame_msg);
     g_queue_push_tail(decoder->msgq, frame_msg);
     mjpeg_decoder_schedule(decoder);
+    return TRUE;
 }
 
 static void mjpeg_decoder_reschedule(VideoDecoder *video_decoder)
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index aa57f95..b9c08a3 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -50,8 +50,10 @@  struct VideoDecoder {
      * @frame_msg: The Spice message containing the compressed frame.
      * @latency:   How long in milliseconds until the frame should be
      *             displayed. Negative values mean the frame is late.
+     * @return:    False if the decoder can no longer decode frames,
+     *             True otherwise.
      */
-    void (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
+    gboolean (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
 
     /* The format of the encoded video. */
     int codec_type;
diff --git a/src/channel-display.c b/src/channel-display.c
index 6cbbdd3..3d88cb0 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1423,7 +1423,12 @@  static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
      * decoding and best decide if/when to drop them when they are late,
      * taking into account the impact on later frames.
      */
-    st->video_decoder->queue_frame(st->video_decoder, in,  latency);
+    if (!st->video_decoder->queue_frame(st->video_decoder, in, latency)) {
+        destroy_stream(channel, op->id);
+        /* Trigger the error reporting code */
+        get_stream_by_id(channel, op->id);
+        return;
+    }
     if (c->enable_adaptive_streaming) {
         display_update_stream_report(SPICE_DISPLAY_CHANNEL(channel), op->id,
                                      op->multi_media_time, latency);

Comments

Missing commit log.

On Mon, Oct 31, 2016 at 09:25:37PM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  src/channel-display-gst.c   | 41 ++++++++++++++++++++++++++++++++++++-----
>  src/channel-display-mjpeg.c |  8 +++++---
>  src/channel-display-priv.h  |  4 +++-
>  src/channel-display.c       |  7 ++++++-
>  4 files changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index f52299f..5fb0b77 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -280,6 +280,28 @@ static void free_pipeline(SpiceGstDecoder *decoder)
>      decoder->pipeline = NULL;
>  }
>  
> +static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer video_decoder)
> +{
> +    SpiceGstDecoder *decoder = video_decoder;
> +
> +    if (GST_MESSAGE_TYPE(msg) == GST_MESSAGE_ERROR) {
> +        GError *err = NULL;
> +        gchar *debug_info = NULL;
> +        gst_message_parse_error(msg, &err, &debug_info);
> +        spice_warning("GStreamer error from element %s: %s",
> +                      GST_OBJECT_NAME(msg->src), err->message);
> +        if (debug_info) {
> +            SPICE_DEBUG("debug information: %s", debug_info);
> +            g_free(debug_info);
> +        }
> +        g_clear_error(&err);
> +
> +        /* We won't be able to process any more frame anyway */
> +        free_pipeline(decoder);
> +    }
> +    return TRUE;
> +}
> +
>  static gboolean create_pipeline(SpiceGstDecoder *decoder)
>  {
>      gchar *desc;
> @@ -324,6 +346,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>  
>      appsink_cbs.new_sample = new_sample;
>      gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder, NULL);
> +    gst_bus_add_watch(gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline)),
> +                      handle_pipeline_message, decoder);
>  
>      decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder->pipeline));
>  
> @@ -390,9 +414,9 @@ static void release_buffer_data(gpointer data)
>      spice_msg_in_unref(frame_msg);
>  }
>  
> -static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> -                                          SpiceMsgIn *frame_msg,
> -                                          int32_t latency)
> +static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> +                                              SpiceMsgIn *frame_msg,
> +                                              int32_t latency)
>  {
>      SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
>  
> @@ -400,7 +424,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>      uint32_t size = spice_msg_in_frame_data(frame_msg, &data);
>      if (size == 0) {
>          SPICE_DEBUG("got an empty frame buffer!");
> -        return;
> +        return TRUE;
>      }
>  
>      SpiceStreamDataHeader *frame_op = spice_msg_in_parsed(frame_msg);
> @@ -419,7 +443,13 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>           * saves CPU so do it.
>           */
>          SPICE_DEBUG("dropping a late MJPEG frame");
> -        return;
> +        return TRUE;
> +    }
> +
> +    if (decoder->pipeline == NULL) {
> +        /* An error occurred, causing the GStreamer pipeline to be freed */
> +        spice_warning("An error occurred, stopping the video stream");
> +        return FALSE;
>      }
>  
>      /* ref() the frame_msg for the buffer */
> @@ -440,6 +470,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>          SPICE_DEBUG("GStreamer error: unable to push frame of size %u", size);
>          stream_dropped_frame_on_playback(decoder->base.stream);
>      }
> +    return TRUE;
>  }
>  
>  static gboolean gstvideo_init(void)
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 4976d53..67746c3 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -235,8 +235,9 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
>  
>  /* ---------- VideoDecoder's public API ---------- */
>  
> -static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> -                                      SpiceMsgIn *frame_msg, int32_t latency)
> +static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> +                                          SpiceMsgIn *frame_msg,
> +                                          int32_t latency)
>  {
>      MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
>      SpiceMsgIn *last_msg;
> @@ -262,12 +263,13 @@ static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
>       * So drop late frames as early as possible to save on processing time.
>       */
>      if (latency < 0) {
> -        return;
> +        return TRUE;
>      }
>  
>      spice_msg_in_ref(frame_msg);
>      g_queue_push_tail(decoder->msgq, frame_msg);
>      mjpeg_decoder_schedule(decoder);
> +    return TRUE;
>  }
>  
>  static void mjpeg_decoder_reschedule(VideoDecoder *video_decoder)
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index aa57f95..b9c08a3 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -50,8 +50,10 @@ struct VideoDecoder {
>       * @frame_msg: The Spice message containing the compressed frame.
>       * @latency:   How long in milliseconds until the frame should be
>       *             displayed. Negative values mean the frame is late.
> +     * @return:    False if the decoder can no longer decode frames,
> +     *             True otherwise.
>       */
> -    void (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
> +    gboolean (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
>  
>      /* The format of the encoded video. */
>      int codec_type;
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 6cbbdd3..3d88cb0 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1423,7 +1423,12 @@ static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
>       * decoding and best decide if/when to drop them when they are late,
>       * taking into account the impact on later frames.
>       */
> -    st->video_decoder->queue_frame(st->video_decoder, in,  latency);
> +    if (!st->video_decoder->queue_frame(st->video_decoder, in, latency)) {
> +        destroy_stream(channel, op->id);
> +        /* Trigger the error reporting code */
> +        get_stream_by_id(channel, op->id);

I'd prefer if this called a helper with a more descriptive name, which
would make the comment not useful.

Seems fine otherwise.

Christophe
Forgot to ask if you hit these issues in practice, if so in which
scenario, and what happened before this patch series.

Christophe

On Tue, Nov 15, 2016 at 05:40:50PM +0100, Christophe Fergeau wrote:
> Missing commit log.
> 
> On Mon, Oct 31, 2016 at 09:25:37PM +0100, Francois Gouget wrote:
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> >  src/channel-display-gst.c   | 41 ++++++++++++++++++++++++++++++++++++-----
> >  src/channel-display-mjpeg.c |  8 +++++---
> >  src/channel-display-priv.h  |  4 +++-
> >  src/channel-display.c       |  7 ++++++-
> >  4 files changed, 50 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index f52299f..5fb0b77 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -280,6 +280,28 @@ static void free_pipeline(SpiceGstDecoder *decoder)
> >      decoder->pipeline = NULL;
> >  }
> >  
> > +static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer video_decoder)
> > +{
> > +    SpiceGstDecoder *decoder = video_decoder;
> > +
> > +    if (GST_MESSAGE_TYPE(msg) == GST_MESSAGE_ERROR) {
> > +        GError *err = NULL;
> > +        gchar *debug_info = NULL;
> > +        gst_message_parse_error(msg, &err, &debug_info);
> > +        spice_warning("GStreamer error from element %s: %s",
> > +                      GST_OBJECT_NAME(msg->src), err->message);
> > +        if (debug_info) {
> > +            SPICE_DEBUG("debug information: %s", debug_info);
> > +            g_free(debug_info);
> > +        }
> > +        g_clear_error(&err);
> > +
> > +        /* We won't be able to process any more frame anyway */
> > +        free_pipeline(decoder);
> > +    }
> > +    return TRUE;
> > +}
> > +
> >  static gboolean create_pipeline(SpiceGstDecoder *decoder)
> >  {
> >      gchar *desc;
> > @@ -324,6 +346,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
> >  
> >      appsink_cbs.new_sample = new_sample;
> >      gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder, NULL);
> > +    gst_bus_add_watch(gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline)),
> > +                      handle_pipeline_message, decoder);
> >  
> >      decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder->pipeline));
> >  
> > @@ -390,9 +414,9 @@ static void release_buffer_data(gpointer data)
> >      spice_msg_in_unref(frame_msg);
> >  }
> >  
> > -static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > -                                          SpiceMsgIn *frame_msg,
> > -                                          int32_t latency)
> > +static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > +                                              SpiceMsgIn *frame_msg,
> > +                                              int32_t latency)
> >  {
> >      SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
> >  
> > @@ -400,7 +424,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> >      uint32_t size = spice_msg_in_frame_data(frame_msg, &data);
> >      if (size == 0) {
> >          SPICE_DEBUG("got an empty frame buffer!");
> > -        return;
> > +        return TRUE;
> >      }
> >  
> >      SpiceStreamDataHeader *frame_op = spice_msg_in_parsed(frame_msg);
> > @@ -419,7 +443,13 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> >           * saves CPU so do it.
> >           */
> >          SPICE_DEBUG("dropping a late MJPEG frame");
> > -        return;
> > +        return TRUE;
> > +    }
> > +
> > +    if (decoder->pipeline == NULL) {
> > +        /* An error occurred, causing the GStreamer pipeline to be freed */
> > +        spice_warning("An error occurred, stopping the video stream");
> > +        return FALSE;
> >      }
> >  
> >      /* ref() the frame_msg for the buffer */
> > @@ -440,6 +470,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> >          SPICE_DEBUG("GStreamer error: unable to push frame of size %u", size);
> >          stream_dropped_frame_on_playback(decoder->base.stream);
> >      }
> > +    return TRUE;
> >  }
> >  
> >  static gboolean gstvideo_init(void)
> > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > index 4976d53..67746c3 100644
> > --- a/src/channel-display-mjpeg.c
> > +++ b/src/channel-display-mjpeg.c
> > @@ -235,8 +235,9 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
> >  
> >  /* ---------- VideoDecoder's public API ---------- */
> >  
> > -static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> > -                                      SpiceMsgIn *frame_msg, int32_t latency)
> > +static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> > +                                          SpiceMsgIn *frame_msg,
> > +                                          int32_t latency)
> >  {
> >      MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
> >      SpiceMsgIn *last_msg;
> > @@ -262,12 +263,13 @@ static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> >       * So drop late frames as early as possible to save on processing time.
> >       */
> >      if (latency < 0) {
> > -        return;
> > +        return TRUE;
> >      }
> >  
> >      spice_msg_in_ref(frame_msg);
> >      g_queue_push_tail(decoder->msgq, frame_msg);
> >      mjpeg_decoder_schedule(decoder);
> > +    return TRUE;
> >  }
> >  
> >  static void mjpeg_decoder_reschedule(VideoDecoder *video_decoder)
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index aa57f95..b9c08a3 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -50,8 +50,10 @@ struct VideoDecoder {
> >       * @frame_msg: The Spice message containing the compressed frame.
> >       * @latency:   How long in milliseconds until the frame should be
> >       *             displayed. Negative values mean the frame is late.
> > +     * @return:    False if the decoder can no longer decode frames,
> > +     *             True otherwise.
> >       */
> > -    void (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
> > +    gboolean (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
> >  
> >      /* The format of the encoded video. */
> >      int codec_type;
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 6cbbdd3..3d88cb0 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1423,7 +1423,12 @@ static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
> >       * decoding and best decide if/when to drop them when they are late,
> >       * taking into account the impact on later frames.
> >       */
> > -    st->video_decoder->queue_frame(st->video_decoder, in,  latency);
> > +    if (!st->video_decoder->queue_frame(st->video_decoder, in, latency)) {
> > +        destroy_stream(channel, op->id);
> > +        /* Trigger the error reporting code */
> > +        get_stream_by_id(channel, op->id);
> 
> I'd prefer if this called a helper with a more descriptive name, which
> would make the comment not useful.
> 
> Seems fine otherwise.
> 
> Christophe



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, 15 Nov 2016, Christophe Fergeau wrote:

> Forgot to ask if you hit these issues in practice, if so in which
> scenario, and what happened before this patch series.

I guess you're asking about all of them:

* Stop streaming if frames cannot be decoded
  This happens on the server when I tell x264enc to encode 17 pixel 
  high frames. It also happened with one of the encoders (maybe a 0.10 
  one) when given any frame with an odd width or height. If encoders 
  fail to deal with some frame sizes then I can only assume that some 
  decoders may fail too. It would totally not surprise me if some 
  hardware decoders were picky that way.
  

* Stop streaming if GStreamer silently drops every frame
  This happened before the previous patch added code to detect GStreamer 
  errors. Now that we detect these errors I don't think this happens 
  anymore.

* Report invalid streams to the server
  This can happen when the client receives a message that the server 
  sent before it received the client's stream_report 'closing' a stream.
  Also the client has to validate (and was validating) the server data 
  anyway so this is just about reporting errors rather than silently 
  ignoring them.

As to what happened before this patch series: the area where the video 
should have been displayed was just never refreshed.