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

[Christophe Fergeau]

Hey,

On Tue, Mar 01, 2016 at 04:51:14PM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160303/7d1d7f1e/attachment.sig>