[Spice-devel,client,v5,1/4] streaming: Report invalid streams to the server

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

Details

Message ID 35638bed417cadc8bfbe357d2ce0a5235cc66d23.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.
The error is sent using the existing client stream report message where
the dropped frame count is maxed out while the received frame count is
zero. Servers that recognize it can then switch to sending regular
screen updates for that area so the client is not stuck with a frozen
area on the screen.
This can be useful in case the client is unable to decode the stream for
some reason like a bug in the GStreamer plugins, the decoder not liking
odd video dimensions, etc.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---

Changed the commit log. Hope it's as desired.


 src/channel-display.c | 68 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display.c b/src/channel-display.c
index 709b3d2..6cbbdd3 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1077,6 +1077,38 @@  static void display_update_stream_region(display_stream *st)
     }
 }
 
+static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
+{
+    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+
+    if (c != NULL && c->streams != NULL && id < c->nstreams &&
+        c->streams[id] != NULL) {
+        return c->streams[id];
+    }
+
+    if (spice_channel_test_capability(channel, SPICE_DISPLAY_CAP_STREAM_REPORT)) {
+        SpiceMsgcDisplayStreamReport report;
+        SpiceMsgOut *msg;
+
+        /* Send a special stream report to indicate there is no such stream */
+        spice_printerr("notify the server that stream %u does not exist", id);
+        report.stream_id = id;
+        report.unique_id = 0;
+        report.start_frame_mm_time = 0;
+        report.end_frame_mm_time = 0;
+        report.num_frames = 0;
+        report.num_drops = UINT_MAX;
+        report.last_frame_delay = 0;
+        report.audio_delay = 0;
+
+        msg = spice_msg_out_new(SPICE_CHANNEL(channel), SPICE_MSGC_DISPLAY_STREAM_REPORT);
+        msg->marshallers->msgc_display_stream_report(msg->marshaller, &report);
+        spice_msg_out_send(msg);
+    }
+
+    return NULL;
+}
+
 /* coroutine context */
 static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
 {
@@ -1127,6 +1159,8 @@  static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
     if (st->video_decoder == NULL) {
         spice_printerr("could not create a video decoder for codec %u", op->codec_type);
         destroy_stream(channel, op->id);
+        /* Trigger the error reporting code */
+        get_stream_by_id(channel, op->id);
     }
 }
 
@@ -1224,17 +1258,10 @@  void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg,
 static void display_update_stream_report(SpiceDisplayChannel *channel, uint32_t stream_id,
                                          uint32_t frame_time, int32_t latency)
 {
-    SpiceDisplayChannelPrivate *c = channel->priv;
-    display_stream *st;
+    display_stream *st = get_stream_by_id(SPICE_CHANNEL(channel), stream_id);
     guint64 now;
 
-    g_return_if_fail(c != NULL);
-    g_return_if_fail(c->streams != NULL);
-    g_return_if_fail(c->nstreams > stream_id);
-
-    st = channel->priv->streams[stream_id];
     g_return_if_fail(st != NULL);
-
     if (!st->report_is_active) {
         return;
     }
@@ -1347,15 +1374,10 @@  static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
 {
     SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
     SpiceStreamDataHeader *op = spice_msg_in_parsed(in);
-    display_stream *st;
+    display_stream *st = get_stream_by_id(channel, op->id);
     guint32 mmtime;
     int32_t latency;
 
-    g_return_if_fail(c != NULL);
-    g_return_if_fail(c->streams != NULL);
-    g_return_if_fail(c->nstreams > op->id);
-
-    st =  c->streams[op->id];
     g_return_if_fail(st != NULL);
     mmtime = stream_get_time(st);
 
@@ -1415,17 +1437,10 @@  static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
 /* coroutine context */
 static void display_handle_stream_clip(SpiceChannel *channel, SpiceMsgIn *in)
 {
-    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
     SpiceMsgDisplayStreamClip *op = spice_msg_in_parsed(in);
-    display_stream *st;
+    display_stream *st = get_stream_by_id(channel, op->id);
 
-    g_return_if_fail(c != NULL);
-    g_return_if_fail(c->streams != NULL);
-    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);
     }
@@ -1524,17 +1539,10 @@  static void display_handle_stream_destroy_all(SpiceChannel *channel, SpiceMsgIn
 /* coroutine context */
 static void display_handle_stream_activate_report(SpiceChannel *channel, SpiceMsgIn *in)
 {
-    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
     SpiceMsgDisplayStreamActivateReport *op = spice_msg_in_parsed(in);
-    display_stream *st;
+    display_stream *st = get_stream_by_id(channel, op->stream_id);
 
-    g_return_if_fail(c != NULL);
-    g_return_if_fail(c->streams != NULL);
-    g_return_if_fail(c->nstreams > op->stream_id);
-
-    st = c->streams[op->stream_id];
     g_return_if_fail(st != NULL);
-
     st->report_is_active = TRUE;
     st->report_id = op->unique_id;
     st->report_max_window = op->max_window_size;

Comments

On Mon, Oct 31, 2016 at 09:25:33PM +0100, Francois Gouget wrote:
> The error is sent using the existing client stream report message where
> the dropped frame count is maxed out while the received frame count is
> zero. Servers that recognize it can then switch to sending regular
> screen updates for that area so the client is not stuck with a frozen
> area on the screen.
> This can be useful in case the client is unable to decode the stream for
> some reason like a bug in the GStreamer plugins, the decoder not liking
> odd video dimensions, etc.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> Changed the commit log. Hope it's as desired.
> 
> 
>  src/channel-display.c | 68 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 709b3d2..6cbbdd3 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1077,6 +1077,38 @@ static void display_update_stream_region(display_stream *st)
>      }
>  }
>  
> +static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
> +{
> +    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> +
> +    if (c != NULL && c->streams != NULL && id < c->nstreams &&
> +        c->streams[id] != NULL) {
> +        return c->streams[id];
> +    }
> +
> +    if (spice_channel_test_capability(channel, SPICE_DISPLAY_CAP_STREAM_REPORT)) {
> +        SpiceMsgcDisplayStreamReport report;
> +        SpiceMsgOut *msg;
> +
> +        /* Send a special stream report to indicate there is no such stream */

Can you add to the comment which values are special? (what you already
said in the commit log). This would be better in a helper function in my
opinion, see my comment on one of the next commits.

Christophe
On Tue, Nov 15, 2016 at 05:33:07PM +0100, Christophe Fergeau wrote:
> On Mon, Oct 31, 2016 at 09:25:33PM +0100, Francois Gouget wrote:
> > The error is sent using the existing client stream report message where
> > the dropped frame count is maxed out while the received frame count is
> > zero. Servers that recognize it can then switch to sending regular
> > screen updates for that area so the client is not stuck with a frozen
> > area on the screen.
> > This can be useful in case the client is unable to decode the stream for
> > some reason like a bug in the GStreamer plugins, the decoder not liking
> > odd video dimensions, etc.
> > 
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> > 
> > Changed the commit log. Hope it's as desired.
> > 
> > 
> >  src/channel-display.c | 68 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 38 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 709b3d2..6cbbdd3 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1077,6 +1077,38 @@ static void display_update_stream_region(display_stream *st)
> >      }
> >  }
> >  
> > +static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
> > +{
> > +    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> > +
> > +    if (c != NULL && c->streams != NULL && id < c->nstreams &&
> > +        c->streams[id] != NULL) {
> > +        return c->streams[id];
> > +    }
> > +
> > +    if (spice_channel_test_capability(channel, SPICE_DISPLAY_CAP_STREAM_REPORT)) {
> > +        SpiceMsgcDisplayStreamReport report;
> > +        SpiceMsgOut *msg;
> > +
> > +        /* Send a special stream report to indicate there is no such stream */
> 
> Can you add to the comment which values are special? (what you already
> said in the commit log). This would be better in a helper function in my
> opinion, see my comment on one of the next commits.

One way of doing that maybe to memset() the whole struct to 0, and only
set the fields we need, rather than setting everything to 0.

Christophe