[Spice-devel] [spice v8] dcc: handle preferred video codec message
Christophe de Dinechin
christophe at dinechin.org
Mon Feb 20 11:16:33 UTC 2017
> On 9 Feb 2017, at 09:21, Victor Toso <lists at victortoso.com> wrote:
>
> [0] SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE
>
> This message provides a list of video codecs based on client's order
> of preference.
>
> We duplicate the video codecs array from reds.c and sort it using the
> order of codecs as reference.
>
> This message will not change an ongoing streaming but it could change
> newly created streams depending the rank value of each video codec
> that can be set by spice_server_set_video_codecs()
>
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
> server/dcc-private.h | 5 ++
> server/dcc.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++
> server/dcc.h | 1 +
> server/display-channel.c | 2 +
> server/stream.c | 5 +-
> 5 files changed, 138 insertions(+), 1 deletion(-)
>
> diff --git a/server/dcc-private.h b/server/dcc-private.h
> index 64b32a7..dd54c70 100644
> --- a/server/dcc-private.h
> +++ b/server/dcc-private.h
> @@ -51,6 +51,11 @@ struct DisplayChannelClientPrivate
> int num_pixmap_cache_items;
> } send_data;
>
> + /* Host prefererred video-codec order sorted with client preferred */
> + GArray *preferred_video_codecs;
> + /* Last client preferred video-codec order */
> + GArray *client_preferred_video_codecs;
> +
> uint8_t surface_client_created[NUM_SURFACES];
> QRegion surface_client_lossy_region[NUM_SURFACES];
>
> diff --git a/server/dcc.c b/server/dcc.c
> index afe37b1..9271ea5 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -39,6 +39,8 @@ enum
> PROP_ZLIB_GLZ_STATE
> };
>
> +static void on_display_video_codecs_update(GObject *gobject, GParamSpec *pspec, gpointer user_data);
> +
> static void
> display_channel_client_get_property(GObject *object,
> guint property_id,
> @@ -99,12 +101,19 @@ display_channel_client_constructed(GObject *object)
> dcc_init_stream_agents(self);
>
> image_encoders_init(&self->priv->encoders, &DCC_TO_DC(self)->priv->encoder_shared_data);
> +
> + g_signal_connect(DCC_TO_DC(self), "notify::video-codecs",
> + G_CALLBACK(on_display_video_codecs_update), self);
> }
>
> static void
> display_channel_client_finalize(GObject *object)
> {
> DisplayChannelClient *self = DISPLAY_CHANNEL_CLIENT(object);
> +
> + g_signal_handlers_disconnect_by_func(DCC_TO_DC(self), on_display_video_codecs_update, self);
> + g_clear_pointer(&self->priv->preferred_video_codecs, g_array_unref);
> + g_clear_pointer(&self->priv->client_preferred_video_codecs, g_array_unref);
> g_free(self->priv);
>
> G_OBJECT_CLASS(display_channel_client_parent_class)->finalize(object);
> @@ -1105,6 +1114,120 @@ static int dcc_handle_preferred_compression(DisplayChannelClient *dcc,
> return TRUE;
> }
>
> +
> +/* TODO: Client preference should only be considered when host has video-codecs
> + * with the same priority value. At the moment, the video-codec GArray will be
> + * sorted following only the client's preference (@user_data)
> + *
> + * example:
> + * host encoding preference: gstreamer:mjpeg;gstreamer:vp8;gstreamer:h264
> + * client decoding preference: h264, vp9, mjpeg
> + * result: gstreamer:h264;gstreamer:mjpeg;gstreamer:vp8
> + */
> +static gint sort_video_codecs_by_client_preference(gconstpointer a_pointer,
> + gconstpointer b_pointer,
> + gpointer user_data)
> +{
> + guint i;
> + const RedVideoCodec *a = a_pointer;
> + const RedVideoCodec *b = b_pointer;
> + GArray *client_pref = user_data;
> +
> + for (i = 0; i < client_pref->len; i++) {
> + if (a->type == g_array_index(client_pref, gint, i))
> + return -1;
> +
> + if (b->type == g_array_index(client_pref, gint, i))
> + return 1;
> + }
The order as you wrote it is not good, because it is possible to have both a < b and b < a, e.g. if client_pref[0] matches both a and b, then we will return -1 if we pass (a,b) and also -1 if we pass (b, a). That is likely to break the sorting algorithm in some subtle cases. My recommendation is something like:
+ int client = g_array_index(client_pref, gint, i);
+ if (a->type == client && b->type != client))
+ return -1;
+
+ if (b->type == client && a->type != client)
+ return 1;
this will ensure that you get an “equal” and not “less than” result if both a and b match.
Alternatively, add a comment explaining why the situation may never arise (which I don’t think is true).
> +
> + return 0;
> +}
> +
> +static void dcc_update_preferred_video_codecs(DisplayChannelClient *dcc)
> +{
> + guint i;
> + RedsState *reds;
> + GArray *video_codecs, *server_codecs;
> + GString *msg;
> +
> + reds = red_channel_get_server(red_channel_client_get_channel(&dcc->parent));
> + server_codecs = reds_get_video_codecs(reds);
> + spice_return_if_fail(server_codecs != NULL);
> +
> + /* Copy current host preference */
> + video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
Use g_array_sized_new, you know the size is server_codecs->len.
> + for (i = 0; i < server_codecs->len; i++) {
> + RedVideoCodec codec = g_array_index(server_codecs, RedVideoCodec, i);
> + g_array_append_val(video_codecs, codec);
> + }
I did not see any guarantee in the documentation for GArray that the elements would be contiguous (there is such a guarantee in C++ for std::vector). However, based on the implementation of g_array_index (https://sourcecodebrowser.com/glib2.0/2.12.4/garray_8h.html#a8d6a673ea4f70676523ff28b2e7d3a7f) I assume it is safe to consider that to be true. So you could replace the loop with:
RealVIdeoCodec *server_codecs_data = &g_array_index(server_codecs, RealVideoCodec, 0);
g_array_append_vals(video_codecs, server_codecs_data, server_codecs->len);
This will be faster (one function call, one expand, one memcpy instead of n).
> +
> + /* Sort the copy of current host preference based on client's preference */
> + g_array_sort_with_data(video_codecs, sort_video_codecs_by_client_preference,
> + dcc->priv->client_preferred_video_codecs);
> + g_clear_pointer(&dcc->priv->preferred_video_codecs, g_array_unref);
> + dcc->priv->preferred_video_codecs = video_codecs;
> +
> + msg = g_string_new("Preferred video-codecs:”);
Should this be localized? Or do we avoid localizing info strings?
msg = g_string_new(_(“Preferred video-codecs:”));
> + for (i = 0; i < video_codecs->len; i++) {
> + RedVideoCodec codec = g_array_index(video_codecs, RedVideoCodec, i);
> + g_string_append_printf(msg, " %d", codec.type);
> + }
> + spice_info("%s", msg->str);
> + g_string_free(msg, TRUE);
> +}
> +
> +static void on_display_video_codecs_update(GObject *gobject,
> + GParamSpec *pspec, gpointer user_data)
> +{
> + DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(user_data);
> +
> + /* Only worry about video-codecs update if client has sent
> + * SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION */
> + if (dcc->priv->client_preferred_video_codecs == NULL)
> + return;
> +
> + /* New host preference */
> + dcc_update_preferred_video_codecs(dcc);
> +}
> +
> +static int dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,
> + SpiceMsgcDisplayPreferredVideoCodecType *msg)
> +{
> + gint i;
> + GArray *client_codecs;
> +
> + g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
> + g_return_val_if_fail(msg->num_of_codecs < SPICE_VIDEO_CODEC_TYPE_ENUM_END, TRUE);
> +
> + client_codecs = g_array_new(FALSE, FALSE, sizeof(gint));
Recommend using g_array_sized_new
> + for (i = 0; i < msg->num_of_codecs; i++) {
> + gint video_codec = msg->codecs[i];
> +
> + if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
> + video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> + spice_warning("Client has sent invalid video-codec (value %d at index %d). "
> + "Ignoring preferred video-codec message entirely",
> + video_codec, i);
> + g_array_unref(client_codecs);
> + return TRUE;
> + }
> +
> + g_array_append_val(client_codecs, video_codec);
> + }
If you want to use g_array_append_vals here, you need to move the test loop separately. In that case, it should be done before the allocation, so that in case of error, you did not even allocate and don’t need to free.
> + g_clear_pointer(&dcc->priv->client_preferred_video_codecs, g_array_unref);
> + dcc->priv->client_preferred_video_codecs = client_codecs;
> +
> + /* New client preference */
> + dcc_update_preferred_video_codecs(dcc);
> + return TRUE;
> +}
> +
> +GArray *dcc_get_preferred_video_codecs(DisplayChannelClient *dcc)
> +{
> + return dcc->priv->preferred_video_codecs;
> +}
> +
> static int dcc_handle_gl_draw_done(DisplayChannelClient *dcc)
> {
> DisplayChannel *display = DCC_TO_DC(dcc);
> @@ -1135,6 +1258,9 @@ int dcc_handle_message(RedChannelClient *rcc, uint32_t size, uint16_t type, void
> (SpiceMsgcDisplayPreferredCompression *)msg);
> case SPICE_MSGC_DISPLAY_GL_DRAW_DONE:
> return dcc_handle_gl_draw_done(dcc);
> + case SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE:
> + return dcc_handle_preferred_video_codec_type(dcc,
> + (SpiceMsgcDisplayPreferredVideoCodecType *)msg);
> default:
> return red_channel_client_handle_message(rcc, size, type, msg);
> }
> diff --git a/server/dcc.h b/server/dcc.h
> index 6fb468d..8e7b863 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -204,6 +204,7 @@ uint64_t dcc_get_max_stream_bit_rate(DisplayChannelClient *dcc);
> void dcc_set_max_stream_bit_rate(DisplayChannelClient *dcc, uint64_t rate);
> int dcc_config_socket(RedChannelClient *rcc);
> gboolean dcc_is_low_bandwidth(DisplayChannelClient *dcc);
> +GArray *dcc_get_preferred_video_codecs(DisplayChannelClient *dcc);
>
> G_END_DECLS
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 7d3c6e4..de33ef7 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -214,6 +214,7 @@ void display_channel_set_video_codecs(DisplayChannel *display, GArray *video_cod
>
> g_clear_pointer(&display->priv->video_codecs, g_array_unref);
> display->priv->video_codecs = g_array_ref(video_codecs);
> + g_object_notify(G_OBJECT(display), "video-codecs”);
For my education, who consumes this notification, and why could we do without it before?
> }
>
> GArray *display_channel_get_video_codecs(DisplayChannel *display)
> @@ -2061,6 +2062,7 @@ display_channel_constructed(GObject *object)
>
> red_channel_set_cap(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_COMPRESSION);
> + red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE);
> red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> }
>
> diff --git a/server/stream.c b/server/stream.c
> index e2cd66e..faa33d6 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -693,7 +693,10 @@ static VideoEncoder* dcc_create_video_encoder(DisplayChannelClient *dcc,
> int i;
> GArray *video_codecs;
>
> - video_codecs = display_channel_get_video_codecs(display);
> + video_codecs = dcc_get_preferred_video_codecs(dcc);
> + if (video_codecs == NULL)
> + video_codecs = display_channel_get_video_codecs(display);
> +
> for (i = 0; i < video_codecs->len; i++) {
> RedVideoCodec* video_codec = &g_array_index (video_codecs, RedVideoCodec, i);
>
> --
> 2.9.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list