On Wed, 2017-06-14 at 16:40 +0100, Frediano Ziglio wrote:
> On new client we must restart the stream so new clients
"On new client" -> "When a new client is connected"
> can receive correct data without having to wait next full
"wait" -> "wait for the"
> screen (which on idle screen could take ages).
> On disconnection we should tell the guest to stop streaming
> not wasting resources to stream not needed data.
>
> Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> ---
> server/stream-channel.c | 78
> +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/server/stream-channel.c b/server/stream-channel.c
> index ae44f59..97128cb 100644
> --- a/server/stream-channel.c
> +++ b/server/stream-channel.c
> @@ -111,8 +111,32 @@ stream_channel_client_init(StreamChannelClient
> *client)
> }
>
> static void
> +ask_new_stream(StreamChannel *channel, StreamMsgStartStop *start)
> +{
> + if (channel->start_cb) {
> + channel->start_cb(channel->start_opaque, start, channel);
> + }
> +}
I think this could use a better name. Maybe start_stop_stream()? or
even request_new_stream()?
> +
> +static void
> stream_channel_client_on_disconnect(RedChannelClient *rcc)
> {
> + RedChannel *red_channel = red_channel_client_get_channel(rcc);
> +
> + // if there are still some client connected keep streaming
> + // TODO, maybe would be worth sending new codecs if they are
> better
> + if (red_channel_is_connected(red_channel)) {
> + return;
> + }
> +
> + StreamChannel *channel = STREAM_CHANNEL(red_channel);
> + channel->stream_id = -1;
> + channel->width = 0;
> + channel->height = 0;
> +
> + // send stream stop to device
> + StreamMsgStartStop start = { 0, };
Perhaps this variable would be better named "stop"
> + ask_new_stream(channel, &start);
> }
>
> static StreamChannelClient*
> @@ -247,6 +271,52 @@ stream_channel_new(RedsState *server)
> NULL);
> }
>
> +// find common codecs supported by all clients
> +static StreamMsgStartStop*
> +stream_channel_get_supported_codecs(StreamChannel *channel)
> +{
> + GListIter iter;
> + RedChannelClient *rcc;
> + int codec;
> +
> + static const uint16_t codec2cap[] = {
> + 0, // invalid
> + SPICE_DISPLAY_CAP_CODEC_MJPEG,
> + SPICE_DISPLAY_CAP_CODEC_VP8,
> + SPICE_DISPLAY_CAP_CODEC_H264,
> + SPICE_DISPLAY_CAP_CODEC_VP9,
> + };
> +
> + bool supported[SPICE_N_ELEMENTS(codec2cap)];
> +
> + for (codec = 0; codec < SPICE_N_ELEMENTS(codec2cap); ++codec) {
> + supported[codec] = true;
> + }
> +
> + FOREACH_CLIENT(channel, iter, rcc) {
> + for (codec = 1; codec < SPICE_N_ELEMENTS(codec2cap);
> ++codec) {
> + // if do not support codec delete from list
> + if (!red_channel_client_test_remote_cap(rcc,
> codec2cap[codec])) {
> + supported[codec] = false;
> + }
> + }
> + }
> +
> + // surely mjpeg is supported
> + supported[SPICE_VIDEO_CODEC_TYPE_MJPEG] = true;
> +
> + StreamMsgStartStop *start = spice_malloc0(sizeof(*start) +
> sizeof(start->codecs[0]) * 256);
> + int num = 0;
> + for (codec = 1; codec < SPICE_N_ELEMENTS(codec2cap); ++codec) {
> + if (supported[codec]) {
> + start->codecs[num++] = codec;
> + }
> + }
> + start->num_codecs = num;
> +
> + return start;
> +}
I find this function name a little bit odd as well. It's really
allocating and returning a new start/stop message which is pre-filled
with the supported codecs. But the function name name doesn't mention
the message at all, only the codecs. I suppose it's fine as is, but I
think it might be more straightforward as something like:
stream_msg_start_stop_new_for_channel(StreamChannel*);
It's up to you.
> +
> static void
> stream_channel_connect(RedChannel *red_channel, RedClient
> *red_client, RedsStream *stream,
> int migration, RedChannelCapabilities *caps)
> @@ -259,6 +329,14 @@ stream_channel_connect(RedChannel *red_channel,
> RedClient *red_client, RedsStrea
> client = stream_channel_client_new(channel, red_client, stream,
> migration, caps);
> spice_return_if_fail(client != NULL);
>
> + // ask new stream
"ask" -> "request" or "ask for"
> + StreamMsgStartStop* start =
> stream_channel_get_supported_codecs(channel);
> + // send in any case, even if list is not changed
> + // notify device about changes
> + ask_new_stream(channel, start);
> + free(start);
> +
> +
> // TODO set capabilities like SPICE_DISPLAY_CAP_MONITORS_CONFIG
> // see guest_set_client_capabilities
> RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
