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@xxxxxxxxxxxxxxx> > --- > > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel