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

[Victor Toso]

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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel