Hi,
On Mon, Feb 20, 2017 at 12:16:33PM +0100, Christophe de Dinechin wrote:
>
> > On 9 Feb 2017, at 09:21, Victor Toso <li...@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 <victort...@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).You are saying that I should be more careful when a->type == b->type. Indeed, in that case, we might be changing the order of which encoder will be used (nowadays only one codec has multiple encoders, spice:mjpeg and gstreamer:mjpeg). This function was reworked to remove the priority part (you can see that I indeed was careful enough to have a->priority == b->priority before, but not here [0]) so, good catch :) [0] https://lists.freedesktop.org/archives/spice-devel/2017-January/034849.html I'll improve it. > > > + > > + 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. I had some issues with it in the past, don't recall what was it so let's try again and see if all is good. > > > + 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). I'll play with that too but I don't expect us to be supporting more then 10 video codecs in the next few years. (/me saves this email to see how wrong I was) > > > + > > + /* 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:”)); I only use localized strings on errors to users. Never thought about it in the info domain. Still, I'll lower this to a debug message as per Frediano suggestion. > > > + 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? DisplayChannelClient does. There are two possibilities around changing the order of video-codecs for encoding: 1-) Client sending the SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE message, which is handled by DisplayChannelClient; 2-) Host (admin) calls spice_server_set_video_codecs() which end up updating all DisplayChannels video-codecs in the function above. In this case, DisplayChannelClient needs to know about it in order to sort again the GArray based on both client-side and host-side order of preference. Here it is still interesting to have this notification as the host might disable/enable video-codec, but the host-side preference code is ongoing discussion [1]. I plan to implement Frediano's suggestions this week. [1] https://lists.freedesktop.org/archives/spice-devel/2017-February/035396.html (Yes, the discussion end-up happening in a different thread then the RFC [2] one!) [2] https://lists.freedesktop.org/archives/spice-devel/2017-January/034848.html Thanks for the review, toso > > > } > > > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > _______________________________________________ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
