Hey,
On Tue, Mar 01, 2016 at 04:51:14PM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> ---
> server/dcc.c | 5 ++++-
> server/dcc.h | 2 +-
> server/stream.c | 41 +++++++++++++++++++++++++++++------------
> 3 files changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/server/dcc.c b/server/dcc.c
> index d4a6c7c..df650c9 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -408,7 +408,10 @@ static void dcc_create_all_streams(DisplayChannelClient *dcc)
>
> while ((item = ring_next(ring, item))) {
> Stream *stream = SPICE_CONTAINEROF(item, Stream, link);
> - dcc_create_stream(dcc, stream);
> + if (!dcc_create_stream(dcc, stream)) {
> + stream_stop(DCC_TO_DC(dcc), stream);
> + return;
> + }
> }
> }
>
> diff --git a/server/dcc.h b/server/dcc.h
> index 436d0be..c2ef871 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -184,7 +184,7 @@ void dcc_destroy_surface (DisplayCha
> uint32_t surface_id);
> void dcc_stream_agent_clip (DisplayChannelClient* dcc,
> StreamAgent *agent);
> -void dcc_create_stream (DisplayChannelClient *dcc,
> +gboolean dcc_create_stream (DisplayChannelClient *dcc,
> Stream *stream);
> void dcc_create_surface (DisplayChannelClient *dcc,
> int surface_id);
> diff --git a/server/stream.c b/server/stream.c
> index 9fae3ef..c2898ca 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -440,7 +440,12 @@ static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
> display->streams_size_total += stream->width * stream->height;
> display->stream_count++;
> FOREACH_DCC(display, dcc_ring_item, next, dcc) {
> - dcc_create_stream(dcc, stream);
> + if (!dcc_create_stream(dcc, stream)) {
> + drawable->stream = NULL;
> + stream->current = NULL;
> + stream_stop(display, stream);
> + return;
> + }
> }
I tried to test these error paths, and things mostly behave as expected
when they are triggered, that's good :)
> spice_debug("stream %d %dx%d (%d, %d) (%d, %d) %u fps",
> (int)(stream - display->streams_buf), stream->width,
> @@ -698,24 +703,31 @@ 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)
> +gboolean dcc_create_stream(DisplayChannelClient *dcc, Stream *stream)
> {
> StreamAgent *agent = &dcc->stream_agents[get_stream_id(DCC_TO_DC(dcc), stream)];
>
> - spice_return_if_fail(region_is_empty(&agent->vis_region));
> + spice_return_val_if_fail(region_is_empty(&agent->vis_region), FALSE);
>
> stream->refs++;
> if (stream->current) {
> @@ -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 :)
Looks good otherwise.
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel