On Thu, Jul 11, 2019 at 08:52:44AM +0200, Kevin Pouget wrote:
> Hello,
>
> On Thu, Jul 11, 2019 at 8:16 AM Victor Toso <victortoso@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Thu, Jul 04, 2019 at 12:29:21PM +0200, Kevin Pouget wrote:
> > > spice_display_channel_change_preferred_video_codec_types: new function
> > > for sending an array of video codecs instead of only one.
> > >
> > > Signed-off-by: Kevin Pouget <kpouget@xxxxxxxxxx>
> >
> > Looks fine,
> >
> > > ---
> > > doc/reference/spice-gtk-sections.txt | 1 +
> > > src/channel-display.c | 76 +++++++++++++++++++++++-----
> > > src/channel-display.h | 2 +
> > > src/map-file | 1 +
> > > src/spice-glib-sym-file | 1 +
> > > 5 files changed, 68 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/doc/reference/spice-gtk-sections.txt b/doc/reference/spice-gtk-sections.txt
> > > index a0336aa..33f4038 100644
> > > --- a/doc/reference/spice-gtk-sections.txt
> > > +++ b/doc/reference/spice-gtk-sections.txt
> > > @@ -185,6 +185,7 @@ spice_display_change_preferred_compression
> > > spice_display_channel_change_preferred_compression
> > > spice_display_change_preferred_video_codec_type
> > > spice_display_channel_change_preferred_video_codec_type
> > > +spice_display_channel_change_preferred_video_codec_types
> >
> > Sounds like we could deprecate the previous one but IMHO,
>
> yes, the previous one should be deprecated, no good reason to keep both.
> Should I update the patch ?
My point was that we might keep it and deprecate both later on
when we define a better protocol message but I don't really mind.
Yep, you can do it in this patch as it introduces the
replacement.
> > eventually this preferred-video-codec protocol message might be
> > replaced by something with more relevant info of decoding
> > capabilities for better decision on encoding side... video-codec
> > type alone doesn't say much but for the time being, it was what
> > we could get...
>
> indeed, we'll see with the rest of the smart streaming work what
> relevant information the client can tell
Yeah! Not trivial to get this info reliably but it would be
smart.
> > > spice_gl_scanout_free
> > > <SUBSECTION Standard>
> > > SPICE_DISPLAY_CHANNEL
> > > diff --git a/src/channel-display.c b/src/channel-display.c
> > > index 9a83c53..4558a56 100644
> > > --- a/src/channel-display.c
> > > +++ b/src/channel-display.c
> > > @@ -20,6 +20,7 @@
> > > #ifdef HAVE_SYS_TYPES_H
> > > #include <sys/types.h>
> > > #endif
> > > +#include <glib/gi18n-lib.h>
> > >
> > > #include "spice-client.h"
> > > #include "spice-common.h"
> > > @@ -610,18 +611,15 @@ void spice_display_channel_change_preferred_compression(SpiceChannel *channel, g
> > > }
> > >
> > > static void spice_display_send_client_preferred_video_codecs(SpiceChannel *channel,
> > > - const GArray *codecs)
> > > + const gint *codecs, gsize ncodecs)
> > > {
> > > - guint i;
> > > 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);
> > > - }
> > > + (sizeof(SpiceVideoCodecType) * ncodecs));
> > > + msg->num_of_codecs = ncodecs;
> > > + memcpy(msg->codecs, codecs, sizeof(*codecs) * ncodecs);
> > >
> > > out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE);
> > > out->marshallers->msgc_display_preferred_video_codec_type(out->marshaller, msg);
> > > @@ -659,8 +657,6 @@ void spice_display_change_preferred_video_codec_type(SpiceChannel *channel, gint
> > > */
> > > void spice_display_channel_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 &&
> > > codec_type < SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > > @@ -675,10 +671,64 @@ void spice_display_channel_change_preferred_video_codec_type(SpiceChannel *chann
> > > * This array can be rearranged to have @codec_type in the front (which is
> > > * the preferred for the client side) */
> > > CHANNEL_DEBUG(channel, "changing preferred video codec type to %s", gst_opts[codec_type].name);
> > > - 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_send_client_preferred_video_codecs(channel, &codec_type, 1);
> > > +}
> > > +
> > > +/**
> > > + * spice_display_channel_change_preferred_video_codecs_types:
> > > + * @channel: a #SpiceDisplayChannel
> > > + * @codecs: an array of @ncodecs #SpiceVideoCodecType types
> > > + * @ncodecs: the number of codec types in the @codecs array
> > > + * @err: #GError describing the reason why the change failed
> > > + *
> > > + * Tells the spice server the ordered preferred video codec types to
> > > + * use for streaming in @channel.
> > > + *
> > > + * Returns: %TRUE if the preferred codec list was successfully changed, and %FALSE
> > > + * otherwise.
> > > + *
> > > + * Since: 0.38
> > > + */
> > > +gboolean spice_display_channel_change_preferred_video_codec_types(SpiceChannel *channel,
> > > + const gint *codecs, gsize ncodecs,
> > > + GError **err)
> > > +{
> > > + gsize i;
> > > + GString *msg;
> > > +
> > > + g_return_val_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel), FALSE);
> >
> > Is it expected to work with ncodecs = 0? (Didn't follow previous
> > discussion)
>
> I will add this guard:
>
> > g_return_val_if_fail(ncodecs != 0, FALSE);
>
> as the server side doesnt't want to receive 0 codecs:
Ok
> > in dcc.c:dcc_handle_preferred_video_codec_type() :
> >> g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
>
> > Didn't test yet but looks fine.
>
> thanks,
>
> Kevin
>
> > > +
> > > + 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");
> > > + g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > > + _("Channel does not have capability to change the preferred video codec type"));
> > > +
> > > + return FALSE;
> > > + }
> > > +
> > > + msg = g_string_new("changing preferred video codec type to: ");
> > > + for (i = 0; i < ncodecs; i++) {
> > > + gint codec_type = codecs[i];
> > > +
> > > + if (codec_type < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
> > > + codec_type >= SPICE_VIDEO_CODEC_TYPE_ENUM_END)
> > > + {
Open braces should be in the above line
With this and the guard above,
Acked-by: Victor Toso <victortoso@xxxxxxxxxx>
> > > + g_string_free(msg, TRUE);
> > > + g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > > + _("Invalid codec-type found (%d) ... "), codec_type);
> > > +
> > > + return FALSE;
> > > + }
> > > +
> > > + g_string_append_printf(msg, "%s ", gst_opts[codec_type].name);
> > > +
> > > + }
> > > + CHANNEL_DEBUG(channel, "%s", msg->str);
> > > + g_string_free(msg, TRUE);
> > > +
> > > + spice_display_send_client_preferred_video_codecs(channel, codecs, ncodecs);
> > > +
> > > + return TRUE;
> > > }
> > >
> > > /**
> > > diff --git a/src/channel-display.h b/src/channel-display.h
> > > index 5b48d2f..cf18538 100644
> > > --- a/src/channel-display.h
> > > +++ b/src/channel-display.h
> > > @@ -150,6 +150,8 @@ gboolean spice_display_channel_get_primary(SpiceChannel *channel, guint32
> > >
> > > void spice_display_channel_change_preferred_compression(SpiceChannel *channel, gint compression);
> > > void spice_display_channel_change_preferred_video_codec_type(SpiceChannel *channel, gint codec_type);
> > > +gboolean spice_display_channel_change_preferred_video_codec_types(SpiceChannel *channel, const gint *codecs,
> > > + gsize ncodecs, GError **err);
> > >
> > > 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 500683c..3cb9873 100644
> > > --- a/src/map-file
> > > +++ b/src/map-file
> > > @@ -25,6 +25,7 @@ spice_display_change_preferred_compression;
> > > spice_display_change_preferred_video_codec_type;
> > > spice_display_channel_change_preferred_compression;
> > > spice_display_channel_change_preferred_video_codec_type;
> > > +spice_display_channel_change_preferred_video_codec_types;
> > > spice_display_channel_get_gl_scanout;
> > > spice_display_channel_get_primary;
> > > spice_display_channel_get_type;
> > > diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file
> > > index 2df1cc0..c3b2561 100644
> > > --- a/src/spice-glib-sym-file
> > > +++ b/src/spice-glib-sym-file
> > > @@ -23,6 +23,7 @@ spice_display_change_preferred_compression
> > > spice_display_change_preferred_video_codec_type
> > > spice_display_channel_change_preferred_compression
> > > spice_display_channel_change_preferred_video_codec_type
> > > +spice_display_channel_change_preferred_video_codec_types
> > > spice_display_channel_get_gl_scanout
> > > spice_display_channel_get_primary
> > > spice_display_channel_get_type
> > > --
> > > 2.21.0
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel