[Spice-devel] [spice v10 05/27] server: Check the client video codec capabilities

[Francois Gouget]

On Thu, 3 Mar 2016, Christophe Fergeau wrote:
[...]
> > -void dcc_create_stream(DisplayChannelClient *dcc, Stream *stream)
> > +gboolean dcc_create_stream(DisplayChannelClient *dcc, Stream *stream)
> > @@ -739,9 +751,13 @@ void dcc_create_stream(DisplayChannelClient *dcc, Stream *stream)
> >          video_cbs.update_client_playback_delay = update_client_playback_delay;
> >  
> >          initial_bit_rate = get_initial_bit_rate(dcc, stream);
> > -        agent->video_encoder = dcc_create_video_encoder(initial_bit_rate, &video_cbs);
> > +        agent->video_encoder = dcc_create_video_encoder(dcc, initial_bit_rate, &video_cbs);
> >      } else {
> > -        agent->video_encoder = dcc_create_video_encoder(0, NULL);
> > +        agent->video_encoder = dcc_create_video_encoder(dcc, 0, NULL);
> > +    }
> > +    if (agent->video_encoder == NULL) {
> > +        stream->refs--;
> > +        return FALSE;
> >      }
> >      red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), &agent->create_item);
> 
> Note that here we haven't sent a "create-stream" message yet when we
> return early because agent->video_encoder is NULL. Then in the caller
> you call stream_stop(display, stream); as part of the cleanup, which
> sends a "stop-stream" message. This triggers a runtime warning in the
> client. Would be nicer to avoid this :)

I have retested and fixed the issue you mentioned. However I then 
extended the tests to the case where a new client connects, causing the 
old one to be disconnected and this did not work.

Looking into it I came to the conclusion that the approach is wrong. 
That is: multiple clients can connect simultaneously, each with 
their own capabilities. That's why the video encoder is on the 
per-client agent, and not on the stream (Thus causing frames to be 
encoded multiple times in such a case but I'm not sure thatś 
avoidable with the current architecture. I'm not even sure it's any 
better if the client want jpeg compression for regular transfers, 
maybe someone knows).

So it's not because no video encoder is suitable for a given client that 
the whole stream should be scrapped. Instead we should proceed with a 
NULL video_encoder and return VIDEO_ENCODER_FRAME_UNSUPPORTED in that 
case in red_marshall_stream_data().

That approach seems to work fine and I don't think it really has any 
drawback compared to the previous one.

Unfortunately I still have the problem with the audio playback getting 
stuck when disconnecting and reconnecting to Xspice which limits the 
tests I can do :-(


(What caused the disconnection-connection case to break is 
that a stream got created after the first client disconnected. Since 
there was no client the stream creation succeeded and a frame got added. 
But then the second client connected, dcc_stream_create() failed, but it 
already had a frame so that kept the stream alive and it crashed when 
trying to call the NULL video_encoder)



Here's the revised patch for reference:

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 8cf5a6a..4372f34 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -1706,7 +1706,8 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
                         drawable->red_drawable->mm_time :
                         reds_get_mm_time();
     outbuf_size = dcc->send_data.stream_outbuf_size;
-    ret = agent->video_encoder->encode_frame(agent->video_encoder,
+    ret = !agent->video_encoder ? VIDEO_ENCODER_FRAME_UNSUPPORTED :
+          agent->video_encoder->encode_frame(agent->video_encoder,
                                              frame_mm_time,
                                              &image->u.bitmap, width, height,
                                              &drawable->red_drawable->u.copy.src_area,
diff --git a/server/stream.c b/server/stream.c
index b053c5c..be095a6 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -698,17 +698,24 @@ static void update_client_playback_delay(void *opaque, uint32_t delay_ms)
 }
 
 /* A helper for dcc_create_stream(). */
-static VideoEncoder* dcc_create_video_encoder(uint64_t starting_bit_rate,
+static VideoEncoder* dcc_create_video_encoder(DisplayChannelClient *dcc,
+                                              uint64_t starting_bit_rate,
                                               VideoEncoderRateControlCbs *cbs)
 {
+    RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
+    int client_has_multi_codec = red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_MULTI_CODEC);
+    if (!client_has_multi_codec || red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_CODEC_MJPEG)) {
 #ifdef HAVE_GSTREAMER_1_0
-    VideoEncoder* video_encoder = gstreamer_encoder_new(starting_bit_rate, cbs);
-    if (video_encoder) {
-        return video_encoder;
-    }
+        VideoEncoder* video_encoder = gstreamer_encoder_new(starting_bit_rate, cbs);
+        if (video_encoder) {
+            return video_encoder;
+        }
 #endif
-    /* Use the builtin MJPEG video encoder as a fallback */
-    return mjpeg_encoder_new(starting_bit_rate, cbs);
+        /* Use the builtin MJPEG video encoder as a fallback */
+        return mjpeg_encoder_new(starting_bit_rate, cbs);
+    }
+
+    return NULL;
 }
 
 void dcc_create_stream(DisplayChannelClient *dcc, Stream *stream)
@@ -739,9 +746,9 @@ void dcc_create_stream(DisplayChannelClient *dcc, Stream *stream)
         video_cbs.update_client_playback_delay = update_client_playback_delay;
 
         initial_bit_rate = get_initial_bit_rate(dcc, stream);
-        agent->video_encoder = dcc_create_video_encoder(initial_bit_rate, &video_cbs);
+        agent->video_encoder = dcc_create_video_encoder(dcc, initial_bit_rate, &video_cbs);
     } else {
-        agent->video_encoder = dcc_create_video_encoder(0, NULL);
+        agent->video_encoder = dcc_create_video_encoder(dcc, 0, NULL);
     }
     red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), &agent->create_item);
 


-- 
Francois Gouget <fgouget at codeweavers.com>