[Spice-devel,client,v5,3/4] streaming: Stop streaming if GStreamer silently drops every frame

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

Details

Message ID 8bee8ec4bedfed404f61f26f5053e44d8fd78a33.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 | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 5fb0b77..5ebfc53 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -446,6 +446,13 @@  static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
         return TRUE;
     }
 
+    /* Verify that the pipeline is decoding the frames as expected */
+    g_mutex_lock(&decoder->queues_mutex);
+    if (g_queue_get_length(decoder->decoding_queue) > 25) {
+        spice_warning("GStreamer is not decoding the frames!");
+        free_pipeline(decoder);
+    }
+    g_mutex_unlock(&decoder->queues_mutex);
     if (decoder->pipeline == NULL) {
         /* An error occurred, causing the GStreamer pipeline to be freed */
         spice_warning("An error occurred, stopping the video stream");

Comments

Missing commit log.

On Mon, Oct 31, 2016 at 09:25:41PM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  src/channel-display-gst.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 5fb0b77..5ebfc53 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -446,6 +446,13 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>          return TRUE;
>      }
>  
> +    /* Verify that the pipeline is decoding the frames as expected */
> +    g_mutex_lock(&decoder->queues_mutex);
> +    if (g_queue_get_length(decoder->decoding_queue) > 25) {

Why 25 ?

Looks ok otherwise.

Christophe
On Tue, 15 Nov 2016, Christophe Fergeau wrote:

> 
> Missing commit log.
> 
> On Mon, Oct 31, 2016 at 09:25:41PM +0100, Francois Gouget wrote:
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> >  src/channel-display-gst.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 5fb0b77..5ebfc53 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -446,6 +446,13 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> >          return TRUE;
> >      }
> >  
> > +    /* Verify that the pipeline is decoding the frames as expected */
> > +    g_mutex_lock(&decoder->queues_mutex);
> > +    if (g_queue_get_length(decoder->decoding_queue) > 25) {
> 
> Why 25 ?

It's arbitrary.

Frames may accumulate in the queue if the GStreamer pipeline is slow in 
decoding them so setting this value too low may cause false positives. 
So it should be large enough but not so large that the client takes ages 
to realize something is wrong.

The server is currently sending a maximum of 30 fps (but the client does 
not know that) so this is, at a minimum, a bit under 1 second worth of 
frames queued up for decoding. That should really not happen.

Do you want that value in a macro? What name? How would you describe it?