[Spice-devel,client,1/2] streaming: Send a special stream report to signal streaming errors

Submitted by Francois Gouget on Aug. 11, 2016, 11:04 a.m.

Details

Message ID f95b7c932d9b7143f46f8c130056fd3f7a814875.1470911003.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 Aug. 11, 2016, 11:04 a.m.
Servers that recognize this special report then stop streaming (sending 
regular screen updates instead) while older ones essentially ignore it.

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

This patchset is based on Victor Toso's idea [1] of using the stream 
reports to tell the server that, despite expectations, the client cannot 
handle a given stream.

You'll notice that this patch does not directly check for 
create_xxx_decoder() errors. Instead it relies on the previous patchset 
[2] deleting broken streams so that the following messages will run into 
an unknown stream.

Of course this could already happen in case of a malicious server 
sending garbage to the client. So this patchset is quite independent 
from the previous one.

I don't know what the consequences of receiving an unknown message would 
be for the server so I chose to hook into the 
display_handle_stream_activate_report() as that's where we get notified 
that the server supports the stream reports.

Maybe we should send such an error anywhere we receive a message with an 
unknown stream_id. There's really no reason for that to happen in those 
other places though, except if the server does not recognize the initial 
error stream report and continues streaming. In that case sending more 
error messages won't do any good. So sending an error in just this one 
place may make more sense.

We could also add an extra cap to identify servers that recognize this 
special type or stream report. But is it really worth it?

[1] https://lists.freedesktop.org/archives/spice-devel/2016-August/031101.html
[2] https://lists.freedesktop.org/archives/spice-devel/2016-August/031282.html


 src/channel-display.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display.c b/src/channel-display.c
index 709b3d2..3898f0a 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1530,10 +1530,28 @@  static void display_handle_stream_activate_report(SpiceChannel *channel, SpiceMs
 
     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 = op->stream_id < c->nstreams ? c->streams[op->stream_id] : NULL;
+    if (st == NULL) {
+        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", op->stream_id);
+        report.stream_id = op->stream_id;
+        report.unique_id = op->unique_id;
+        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;
+    }
 
     st->report_is_active = TRUE;
     st->report_id = op->unique_id;

Comments

Hi,

On Thu, Aug 11, 2016 at 01:04:09PM +0200, Francois Gouget wrote:
> Servers that recognize this special report then stop streaming (sending
> regular screen updates instead) while older ones essentially ignore it.
>
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>
> This patchset is based on Victor Toso's idea [1] of using the stream
> reports to tell the server that, despite expectations, the client cannot
> handle a given stream.
>
> You'll notice that this patch does not directly check for
> create_xxx_decoder() errors. Instead it relies on the previous patchset
> [2] deleting broken streams so that the following messages will run into
> an unknown stream.
>
> Of course this could already happen in case of a malicious server
> sending garbage to the client. So this patchset is quite independent
> from the previous one.
>
> I don't know what the consequences of receiving an unknown message would
> be for the server so I chose to hook into the
> display_handle_stream_activate_report() as that's where we get notified
> that the server supports the stream reports.

The server sends SPICE_MSG_DISPLAY_STREAM_ACTIVATE_REPORT on
dcc_create_stream() just after SPICE_MSG_DISPLAY_STREAM_CREATE (might)
have been sent so, in the client, we have the difference of this two
sequential messages to identify that the decoder failed as this patch
will send the failure on display_handle_stream_activate_report().

/*
SPICE_MSGC_DISPLAY_STREAM_REPORT is being used in the client on
display_update_stream_report() later on display_handle_stream_data().
*/

I think what we want is to inform the server as soon as the error
happened (if possible), likely after stream is destroyed [0].

[0] https://cgit.freedesktop.org/spice/spice-gtk/tree/src/channel-display.c#n1127

We might want to improve the display_update_stream_report() to handle
this kind of situation.

> Maybe we should send such an error anywhere we receive a message with an
> unknown stream_id. There's really no reason for that to happen in those
> other places though, except if the server does not recognize the initial
> error stream report and continues streaming.

I'm not 100% about the stream protocol but it might be the case that we
receive messages with a unknow/failed stream_id while decoder fails and
the error is sent and properly handled in the server.

> In that case sending more error messages won't do any good. So sending
> an error in just this one place may make more sense.

Sending one error per stream-id might be enough indeed.

> We could also add an extra cap to identify servers that recognize this
> special type or stream report. But is it really worth it?

Maybe! To be honest, first thing it seems interesting to avoid is the
server sending stream while the client is not able to decode; second is
a better logic in the server to change the encoder in case previous one
failed with the client (based on client's capabilities); No more stream
in case we don't have another encoder options.

So far, we only had mjpeg but with GStreamer encoder/decoder, I'm
expecting more options to be available quickly in the future... if this
kind of decoder failure does not fit SPICE_MSGC_DISPLAY_STREAM_REPORT,
we might want a new message and capability just for that.

Thanks for the patch and discussion!

Cheers,
  toso

>
> [1] https://lists.freedesktop.org/archives/spice-devel/2016-August/031101.html
> [2] https://lists.freedesktop.org/archives/spice-devel/2016-August/031282.html
>
>
>  src/channel-display.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 709b3d2..3898f0a 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1530,10 +1530,28 @@ static void display_handle_stream_activate_report(SpiceChannel *channel, SpiceMs
>
>      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 = op->stream_id < c->nstreams ? c->streams[op->stream_id] : NULL;
> +    if (st == NULL) {
> +        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", op->stream_id);
> +        report.stream_id = op->stream_id;
> +        report.unique_id = op->unique_id;
> +        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;
> +    }
>
>      st->report_is_active = TRUE;
>      st->report_id = op->unique_id;
> -- 
> 2.8.1
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, Aug 11, 2016 at 01:04:09PM +0200, Francois Gouget wrote:
> Servers that recognize this special report then stop streaming (sending 
> regular screen updates instead) while older ones essentially ignore it.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> This patchset is based on Victor Toso's idea [1] of using the stream 
> reports to tell the server that, despite expectations, the client cannot 
> handle a given stream.
> 
> You'll notice that this patch does not directly check for 
> create_xxx_decoder() errors. Instead it relies on the previous patchset 
> [2] deleting broken streams so that the following messages will run into 
> an unknown stream.
> 
> Of course this could already happen in case of a malicious server 
> sending garbage to the client. So this patchset is quite independent 
> from the previous one.
> 
> I don't know what the consequences of receiving an unknown message would 
> be for the server so I chose to hook into the 
> display_handle_stream_activate_report() as that's where we get notified 
> that the server supports the stream reports.
> 
> Maybe we should send such an error anywhere we receive a message with an 
> unknown stream_id. There's really no reason for that to happen in those 
> other places though, except if the server does not recognize the initial 
> error stream report and continues streaming. In that case sending more 
> error messages won't do any good. So sending an error in just this one 
> place may make more sense.
> 
> We could also add an extra cap to identify servers that recognize this 
> special type or stream report. But is it really worth it?

Part of this + some bits from
https://lists.freedesktop.org/archives/spice-devel/2016-August/031445.html
really belongs to the commit log. We want to have an explanation *why*
we want this in the commit log, very useful when looking back at the code 2
years later and wondering why this code was added.

Christophe
On Thu, Aug 11, 2016 at 01:04:09PM +0200, Francois Gouget wrote:
> Servers that recognize this special report then stop streaming (sending 
> regular screen updates instead) while older ones essentially ignore it.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> This patchset is based on Victor Toso's idea [1] of using the stream 
> reports to tell the server that, despite expectations, the client cannot 
> handle a given stream.
> 
> You'll notice that this patch does not directly check for 
> create_xxx_decoder() errors. Instead it relies on the previous patchset 
> [2] deleting broken streams so that the following messages will run into 
> an unknown stream.

I can't help but wonder why it's better this way? Is it so that you can
reuse the stream report code as mentioned below?

> 
> Of course this could already happen in case of a malicious server 
> sending garbage to the client. So this patchset is quite independent 
> from the previous one.
> 
> I don't know what the consequences of receiving an unknown message would 
> be for the server so I chose to hook into the 
> display_handle_stream_activate_report() as that's where we get notified 
> that the server supports the stream reports.

Have you considered using a dedicated client -> server message for that
rather than using magic values in a stream report message?

Christophe
On Thu, 1 Sep 2016, Christophe Fergeau wrote:
[...]
> > You'll notice that this patch does not directly check for 
> > create_xxx_decoder() errors. Instead it relies on the previous patchset 
> > [2] deleting broken streams so that the following messages will run into 
> > an unknown stream.
> 
> I can't help but wonder why it's better this way? Is it so that you can
> reuse the stream report code as mentioned below?

Yes, it's simpler this way: the client already has to be able to deal 
with invalid stream ids so this just reuses the existing mechanism.
Do you have a better proposal?


> > Of course this could already happen in case of a malicious server 
> > sending garbage to the client. So this patchset is quite independent 
> > from the previous one.
> > 
> > I don't know what the consequences of receiving an unknown message would 
> > be for the server so I chose to hook into the 
> > display_handle_stream_activate_report() as that's where we get notified 
> > that the server supports the stream reports.
> 
> Have you considered using a dedicated client -> server message for that
> rather than using magic values in a stream report message?

Yes but I don't see the benefit to adding one more message type and the 
unavoidable capability and checks that go with it.

Plus I think the stream report is a natural fit for this: it's meant to 
report dropped frames and if the client cannot handle the stream at all 
then all frames are in effect dropped. And this is exactly what the 
stream report we send claims: "the dropped frame count is maxed out"; 
all your frames are dropped. Setting "the received frame count to zero" 
even matches the case where the video decoder initialization fails since 
no frame has been received yet at that point.

Also the server already needs to validate data sent by the client so 
these values should not cause it any trouble.