[Spice-devel] [spice v7 1/3] dcc: handle preferred video codec message
Victor Toso
lists at victortoso.com
Thu Feb 9 07:25:13 UTC 2017
Hi,
On Tue, Feb 07, 2017 at 11:41:05AM +0100, Pavel Grunt wrote:
> On Mon, 2017-02-06 at 12:06 +0100, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> >
> > [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 | 121
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > server/dcc.h | 1 +
> > server/display-channel.c | 2 +
> > server/stream.c | 5 +-
> > 5 files changed, 133 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..62c6b45 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,115 @@ static int
> > dcc_handle_preferred_compression(DisplayChannelClient *dcc,
> > return TRUE;
> > }
> >
> > +
> > +/* TODO: Client preference should only considered when host has
> > video-codecs
> > + * with the same priority value. At the moment, server's video-
> > codec order
> > + * is sorted following client's preference. */
> > +static gint sort_video_codecs_by_client_preference(gconstpointer
> > a_pointer,
> > + gconstpointer
> > b_pointer,
> > + gpointer
> > user_data)
> > +{
> > + gint i;
> unsigned ;)
Fixed
>
> > + 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;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void dcc_update_preferred_video_codecs(DisplayChannelClient
> > *dcc)
> > +{
> > + gint 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 and then sort it based on
> > client preference */
> > + video_codecs = g_array_new(FALSE, FALSE,
> > sizeof(RedVideoCodec));
> > + for (i = 0; i < server_codecs->len; i++) {
> > + RedVideoCodec codec = g_array_index(server_codecs,
> > RedVideoCodec, i);
> > + g_array_append_val(video_codecs, codec);
> > + }
> > + /* Server codecs is sorted by host's preference using priority
> are
Fixed
> > system. We
> > + * will keep host preference order but sort by client's
> > preference when it
> > + * is possible */
>
> well the comment is confusing for me - we keep host options, we are
> not keeping the host order of encoders
Sorry, the comment is outdated. We don't sort the host's preference
using priority system. We use them in the given order from
spice_server_set_video_codecs()
I'll change the previous comment "Copy current host preference and then
sort it based on client preference" to "Copy current host preference" -
Although it could seem very obvious now I think this comment is
important overall while reading the code - (very likely we will have
three different arrays of video-codecs: client's preference, host's
preference and the sorted array which will be used to choose the
video-codec.
I'll change this second comment to: "Sort the copy of current host
preference based on client's preference."
There is a TODO in sort_video_codecs_by_client_preference() in regards
of how the sorting works now and how it should work in the future. I'll
try to improve it slightly.
I'll be sending patch shortly. Thanks for reviewing.
toso
>
> > + 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:");
> > + 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;
> > +
> > + spice_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
> > + spice_return_val_if_fail(msg->num_of_codecs <
> > SPICE_VIDEO_CODEC_TYPE_ENUM_END, TRUE);
>
> I am afraid that spice_return may abort depending on the configuration
> flags of spice-common.
>
> In this case it is not really desired (Client can have newer protocol
> than the Server) so please use g_return_... Or don't use the "end"
> condition - it is checked below anyway
>
> > +
> > + client_codecs = g_array_new(FALSE, FALSE, sizeof(gint));
> > + 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);
> > + }
> > + 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 +1253,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");
> > }
> >
> > 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);
> >
>
>
> Pavel
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- 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/20170209/106d02df/attachment-0001.sig>
More information about the Spice-devel
mailing list