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