[Spice-devel] [client v4 1/2] streaming: Send a special stream report to signal streaming errors
Victor Toso
lists at victortoso.com
Mon Aug 22 12:59:09 UTC 2016
Hi,
On Fri, Aug 19, 2016 at 12:48:46PM +0200, Francois Gouget wrote:
> Servers that recognize this special report then stop streaming (sending
> regular screen updates instead) while older ones essentially ignore it.
If the idea is accepted, it would be great to extend the commit log
explaining how we are using the DISPLAY_STREAM_REPORT to stop the
streaming on server side.
>
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>
> Note that although we check for the right server cap, we may send the
> stream report before we have received the STREAM_ACTIVATE_REPORT
> message. Does anyone know what's the point of that message anyway?
I did not know the reason for STREAM_ACTIVATE_REPORT, for my
understanding the main reason is to generate a report-id [0] which is
totally different from stream-id. AFAICT, I don't think this report-id
is necessary; STREAM_REPORT based on stream-id should be enough but you
know much more about this code then I do :)
[0] https://cgit.freedesktop.org/spice/spice/tree/server/stream.c#n793
>
>
> Changes from v3:
> * Really fix the caps check (forgot to commit that bit before v3).
>
> 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 */
> + spice_printerr("notify the server that stream %u does not exist", id);
> + report.stream_id = id;
> + report.unique_id = 0;
Based on my comment above, this ignores the report-id (unique-id) and is
wrong based on my understanding of the current state of this protocol.
Your 2/2 patch includes the check just before [1] which should make this
work but not sure if it is the correct approach in case we need the
report-id for some reason.
[1] https://cgit.freedesktop.org/spice/spice/tree/server/dcc.c#n933
Again, not sure if we need the report-id but just taking note that it
exists.
> + 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;
Apart from that, this patch can be split in two. The creating of this
get_stream_by_id (1) function and the stream_report logic (2);
(1) could be applied even if (2) is not accepted, IMHO.
Cheers,
toso
> --
> 2.8.1
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list