[Spice-devel] [spice-gtk v6 2/9] channel-display: implement preferred video codec msgc
Victor Toso
victortoso at redhat.com
Fri Feb 3 15:05:12 UTC 2017
Hi,
On Fri, Feb 03, 2017 at 03:57:21PM +0100, Pavel Grunt wrote:
> On Tue, 2017-01-31 at 12:08 +0100, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> >
> > * SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE
> >
> > This message was introduced in protocol 0.12.13 to establish client
> > side preference on video codec to be used in streams.
> >
> > At this moment, we only introduce a new API [0] to select *the*
> > preferred video codec for client; In a later time, it should be
> > possible to use this message upon connection in order to to give
> > higher priority for video codecs with better performance and with
> > hardware decoding capabilities.
> >
> > [0] spice_display_change_preferred_video_codec_type()
> >
> > Note that host preference for encoding is expected to be considered
> > first then the client's preference.
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> > configure.ac | 2 +-
> > doc/reference/spice-gtk-sections.txt | 1 +
> > src/channel-display.c | 52
> > ++++++++++++++++++++++++++++++++++++
> > src/channel-display.h | 1 +
> > src/map-file | 1 +
> > src/spice-glib-sym-file | 1 +
> > 6 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 4fd0bd7..ee083e4 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -69,7 +69,7 @@ AC_CHECK_LIBM
> > AC_SUBST(LIBM)
> >
> > AC_CONFIG_SUBDIRS([spice-common])
> > -PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.12])
> > +PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.13])
> >
> > COMMON_CFLAGS='-I${top_builddir}/spice-common/
> > -I${top_srcdir}/spice-common/ ${SPICE_PROTOCOL_CFLAGS}'
> > AC_SUBST(COMMON_CFLAGS)
> > diff --git a/doc/reference/spice-gtk-sections.txt
> > b/doc/reference/spice-gtk-sections.txt
> > index f2235e8..6f49df3 100644
> > --- a/doc/reference/spice-gtk-sections.txt
> > +++ b/doc/reference/spice-gtk-sections.txt
> > @@ -168,6 +168,7 @@ spice_display_get_gl_scanout
> > spice_display_gl_draw_done
> > spice_display_get_primary
> > spice_display_change_preferred_compression
> > +spice_display_change_preferred_video_codec_type
> > spice_gl_scanout_free
> > <SUBSECTION Standard>
> > SPICE_DISPLAY_CHANNEL
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index ca56cd1..d4e63e6 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -524,6 +524,58 @@ void
> > spice_display_change_preferred_compression(SpiceChannel *channel,
> > gint comp
> > spice_msg_out_send_internal(out);
> > }
> >
> > +static void
> > spice_display_send_client_preferred_video_codecs(SpiceChannel
> > *channel,
> > + const
> > GArray *codecs)
> > +{
> > + gint i;
> it should be unsigned (GArray.len is guint)
>
> > + SpiceMsgOut *out;
> > + SpiceMsgcDisplayPreferredVideoCodecType *msg;
> > +
> > + msg = g_malloc0(sizeof(SpiceMsgcDisplayPreferredVideoCodecType)
> > +
> > + (sizeof(SpiceVideoCodecType) * codecs->len));
> > + msg->num_of_codecs = codecs->len;
> > + for (i = 0; i < codecs->len; i++) {
> > + msg->codecs[i] = g_array_index(codecs, gint, i);
> > + }
> > +
> > + out = spice_msg_out_new(channel,
> > SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE);
> > + out->marshallers->msgc_display_preferred_video_codec_type(out-
> > >marshaller, msg);
> > + spice_msg_out_send_internal(out);
> > + g_free(msg);
> > +}
> > +
> > +/**
> > + * spice_display_change_preferred_video_codec:
> > + * @channel: a #SpiceDisplayChannel
> > + * @compression: a #SpiceVideoCodecType
> wrong name ^
>
> > + *
> > + * Tells the spice server to change the preferred video codec type
> > for
> > + * streaming in @channel. Application can set only one preferred
> > video codec.
> not exactly true - it is per channel (ie it should be possible to use
> different codecs at the same time with a Windows vm)
You are right, I'll change that part to "Application can set only one
preferred video codec per display channel"
>
> > + *
> > + * Since: 0.34
> > + */
> > +void spice_display_change_preferred_video_codec_type(SpiceChannel
> > *channel, gint codec_type)
> > +{
> > + GArray *codecs;
> > +
> > + g_return_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel));
> > + g_return_if_fail(codec_type >= SPICE_VIDEO_CODEC_TYPE_MJPEG);
> > + g_return_if_fail(codec_type < SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> imo would be better to have a single condition (to indicate "not in a
> range")
>
> > +
> > + if (!spice_channel_test_capability(channel,
> > SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE)) {
> > + CHANNEL_DEBUG(channel, "does not have capability to change
> > the preferred video codec type");
> > + return;
> > + }
> > +
> > + /* TODO: Only one value at this time while we don't try to
> > detect
> > + * capabilities for hw decoding */
>
> sounds more like fixme than todo. Actually I don't understand the
> suggestion.
spice-gtk should try to detect video codecs that it can do hw decoding
and store this information to send it to the server.
I'll update the comment and change it to FIXME.
>
> > + CHANNEL_DEBUG(channel, "changing preferred video codec type to
> > %d", codec_type);
> > + codecs = g_array_new(FALSE, FALSE, sizeof(gint));
> > + g_array_append_val(codecs, codec_type);
> > + spice_display_send_client_preferred_video_codecs(channel,
> > codecs);
> > + g_array_unref(codecs);
> > +}
> > +
> > /**
> > * spice_display_get_gl_scanout:
> > * @channel: a #SpiceDisplayChannel
> > diff --git a/src/channel-display.h b/src/channel-display.h
> > index ad82a16..fccf228 100644
> > --- a/src/channel-display.h
> > +++ b/src/channel-display.h
> > @@ -149,6 +149,7 @@
> > gboolean spice_display_get_primary(SpiceChannel *channel,
> > guint32 surface
> > SpiceDisplayPrimary
> > *primary);
> >
> > void spice_display_change_preferred_compression(SpiceChannel
> > *channel, gint compression);
> > +void spice_display_change_preferred_video_codec_type(SpiceChannel
> > *channel, gint codec_type);
> >
> > GType spice_gl_scanout_get_type (void) G_GNUC_CONST;
> > void spice_gl_scanout_free (SpiceGlScanout
> > *scanout);
> > diff --git a/src/map-file b/src/map-file
> > index 3d92153..31cafc2 100644
> > --- a/src/map-file
> > +++ b/src/map-file
> > @@ -21,6 +21,7 @@ spice_channel_type_to_string;
> > spice_client_error_quark;
> > spice_cursor_channel_get_type;
> > spice_display_change_preferred_compression;
> > +spice_display_change_preferred_video_codec_type;
> > spice_display_channel_get_type;
> > spice_display_get_gl_scanout;
> > spice_display_get_grab_keys;
> > diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file
> > index 473c5ca..d73f799 100644
> > --- a/src/spice-glib-sym-file
> > +++ b/src/spice-glib-sym-file
> > @@ -19,6 +19,7 @@ spice_channel_type_to_string
> > spice_client_error_quark
> > spice_cursor_channel_get_type
> > spice_display_change_preferred_compression
> > +spice_display_change_preferred_video_codec_type
> > spice_display_channel_get_type
> > spice_display_get_gl_scanout
> > spice_display_get_primary
>
> Pavel
Thanks for the review, I'll address them.
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/20170203/3dd5125f/attachment.sig>
More information about the Spice-devel
mailing list