[Spice-devel] [spice v9] dcc: handle preferred video codec message
Victor Toso
victortoso at redhat.com
Mon Feb 27 13:21:25 UTC 2017
Hi,
On Mon, Feb 27, 2017 at 08:06:30AM -0500, Frediano Ziglio 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 | 132
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > server/dcc.h | 1 +
> > server/display-channel.c | 2 +
> > server/stream.c | 3 +-
> > 5 files changed, 141 insertions(+), 2 deletions(-)
> >
> > diff --git a/server/dcc-private.h b/server/dcc-private.h
> > index 64b32a7..bf69fbb 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 preferred 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 cf9431a..476f43d 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,126 @@ 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)
> > +{
> > + const RedVideoCodec *a = a_pointer;
> > + const RedVideoCodec *b = b_pointer;
> > +
> > + if (a->type != b->type) {
> > + guint i;
> > + 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)
> > +{
> > + guint i;
> > + GArray *video_codecs, *server_codecs;
> > + GString *msg;
> > +
> > + server_codecs = display_channel_get_video_codecs(DCC_TO_DC(dcc));
> > + spice_return_if_fail(server_codecs != NULL);
> > +
> > + /* Copy current host preference */
> > + video_codecs = g_array_sized_new(FALSE, FALSE, sizeof(RedVideoCodec),
> > server_codecs->len);
> > + g_array_append_vals(video_codecs, server_codecs->data,
> > server_codecs->len);
> > +
> > + /* 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:");
> > + 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_VIDEO_CODEC_TYPE */
> > + 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;
> > + gint has_codec[SPICE_VIDEO_CODEC_TYPE_ENUM_END] = { 0 };
> > +
> > + g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
> > +
> > + 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_debug("Client has sent unknow video-codec (value %d at
> > index %d). "
> > + "Ignoring as server can't handle it",
> > + video_codec, i);
> > + continue;
>
> Is this actually logically possible?
> I mean, there should be an exchange of capabilities between client and
> server and client should not attempt so send something the server
> cannot understand.
Well, even with the capabilities in place, we should guarantee that we
are storing only video codecs that we can handle, no? The client should
only send what server can encode but I don't see any harm on ignoring
unknown video codecs. I was dropping the message entirely before.
>
> > + }
> > +
> > + if (has_codec[video_codec] != 0)
> > + continue;
> > +
> > + has_codec[video_codec] = 1;
>
> This array made me think about having an hash of the index
> (in this case the hash function would be f(n) = n so basically
> a similar array).
> If you initialize the array to a maximum value you could have a
>
> static gint sort_video_codecs_by_client_preference(gconstpointer a_pointer,
> gconstpointer b_pointer,
> gpointer user_data)
> {
> const RedVideoCodec *a = a_pointer;
> const RedVideoCodec *b = b_pointer;
> const int *indexes = user_data;
>
> return indexes[b->type] - indexes[a->type];
Oh, indeed. This should work.
> }
>
> > + 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_for_encoding(DisplayChannelClient
> > *dcc)
> > +{
> > + if (dcc->priv->preferred_video_codecs != NULL) {
> > + return dcc->priv->preferred_video_codecs;
> > + }
> > + return display_channel_get_video_codecs(DCC_TO_DC(dcc));
> > +}
> > +
> > static int dcc_handle_gl_draw_done(DisplayChannelClient *dcc)
> > {
> > DisplayChannel *display = DCC_TO_DC(dcc);
> > @@ -1135,6 +1264,9 @@ int dcc_handle_message(RedChannelClient *rcc, uint16_t
> > type, uint32_t size, 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, type, size, msg);
> > }
> > diff --git a/server/dcc.h b/server/dcc.h
> > index ebdbb8d..37bd3e9 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_for_encoding(DisplayChannelClient
> > *dcc);
> >
> > G_END_DECLS
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 288969c..85df19d 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)
> > @@ -2057,6 +2058,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..f1ec560 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -687,13 +687,12 @@ static VideoEncoder*
> > dcc_create_video_encoder(DisplayChannelClient *dcc,
> > uint64_t starting_bit_rate,
> > VideoEncoderRateControlCbs
> > *cbs)
> > {
> > - DisplayChannel *display = DCC_TO_DC(dcc);
> > RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
> > int client_has_multi_codec = red_channel_client_test_remote_cap(rcc,
> > SPICE_DISPLAY_CAP_MULTI_CODEC);
> > int i;
> > GArray *video_codecs;
> >
> > - video_codecs = display_channel_get_video_codecs(display);
> > + video_codecs = dcc_get_preferred_video_codecs_for_encoding(dcc);
> > for (i = 0; i < video_codecs->len; i++) {
> > RedVideoCodec* video_codec = &g_array_index (video_codecs,
> > RedVideoCodec, i);
> >
>
> <OT>
> looking at all these flags looks like the filtering/ordering of these encoding
> is getting complicated. Basically we excluded codecs based on:
> - server settings;
> - client settings;
> - encoders support.
> The code is spread quite in different places.
> I have the sensation it's spread in too many places.
> </OT>
>
> Frediano
I think we can do some improvement when we start to introduce some hw
encoding. Or at least at the same time we get that priority patch in.
Thanks for the review,
toso
-------------- 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/20170227/aad37692/attachment-0001.sig>
More information about the Spice-devel
mailing list