[Spice-devel] [RFC spice-server 1/3] stream-channel: Add preferred video codec capability
Kevin Pouget
kpouget at redhat.com
Thu Aug 29 07:56:40 UTC 2019
Hello,
On Wed, Aug 14, 2019 at 3:08 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > This patch enables the SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE
> > capability for the stream-channel.
> >
> > video_stream_parse_preferred_codecs: new function for parsing the
> > SPICE protocol message. This code used to be inside
> > dcc_handle_preferred_video_codec_type.
> >
> > struct StreamChannelClient::client_preferred_video_codecs: new field.
> >
> > Signed-off-by: Kevin Pouget <kpouget at redhat.com>
> > ---
> > server/dcc.c | 30 +-----------------------------
> > server/stream-channel.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > server/video-stream.c | 36 ++++++++++++++++++++++++++++++++++++
> > server/video-stream.h | 1 +
> > 4 files changed, 78 insertions(+), 29 deletions(-)
> >
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 21e8598e..538f1f51 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -1158,38 +1158,10 @@ static void on_display_video_codecs_update(GObject
> > *gobject, GParamSpec *pspec,
> > static int dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,
> > SpiceMsgcDisplayPreferredVideoCodecType
> > *msg)
> > {
> > - gint i, len;
> > - gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
> > - GArray *client;
> > -
> > g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
> >
> > - /* set default to a big and positive number */
> > - memset(indexes, 0x7f, sizeof(indexes));
> > -
> > - for (len = 0, 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 unknown video-codec (value %d at
> > index %d). "
> > - "Ignoring as server can't handle it",
> > - video_codec, i);
> > - continue;
> > - }
> > -
> > - if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> > - continue;
> > - }
> > -
> > - len++;
> > - indexes[video_codec] = len;
> > - }
> > - client = g_array_sized_new(FALSE, FALSE, sizeof(gint),
> > SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > - g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > -
> > g_clear_pointer(&dcc->priv->client_preferred_video_codecs,
> > g_array_unref);
> > - dcc->priv->client_preferred_video_codecs = client;
> > + dcc->priv->client_preferred_video_codecs =
> > video_stream_parse_preferred_codecs(msg);
> >
> > /* New client preference */
> > dcc_update_preferred_video_codecs(dcc);
> > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > index 7953018e..fa4804f1 100644
> > --- a/server/stream-channel.c
> > +++ b/server/stream-channel.c
> > @@ -22,6 +22,7 @@
> > #include <spice/stream-device.h>
> >
> > #include "red-channel-client.h"
> > +#include "red-client.h"
>
> Why red-client ?? Should not be video-stream.h ?
this include allows me to convert this "reds = red_client_get_server(client)"
> stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc, ...) {
> RedClient *client = red_channel_client_get_client(rcc);
> RedsState *reds = red_client_get_server(client);
> main_dispatcher_reset_stream_channel(reds_get_main_dispatcher(reds), channel_id);
> > #include "stream-channel.h"
> > #include "reds.h"
> > #include "common-graphics-channel.h"
> > @@ -52,6 +53,9 @@ struct StreamChannelClient {
> > /* current video stream id, <0 if not initialized or
> > * we are not sending a stream */
> > int stream_id;
> > + /* Array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements, with the client
> > + * preference order (index) as value */
> > + GArray *client_preferred_video_codecs;
>
> I think that currently a static array takes less space, but it's just my
> paranoia.
you mean to change the type to "SpiceVideoCodecType
__name__[SPICE_VIDEO_CODEC_TYPE_ENUM_END]"?
yes, could be, but I think what ever we change on this side, we should
change it on host-encoding side, for consistency.
I'm not sure it's worth the effort for saving a few bytes of glib wrapper?
> > };
> >
> > struct StreamChannelClientClass {
> > @@ -114,6 +118,8 @@ typedef struct StreamDataItem {
> > #define PRIMARY_SURFACE_ID 0
> >
> > static void stream_channel_client_on_disconnect(RedChannelClient *rcc);
> > +static bool
> > stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
> > +
> > SpiceMsgcDisplayPreferredVideoCodecType
> > *msg);
> >
>
> Minor: Not correctly indented
this was on purpose, as the "correct" alignment of 'msg' would go > 100chars
I put a new line between "<return type> <function name>"
> > RECORDER(stream_channel_data, 32, "Stream channel data packet");
> >
> > @@ -324,6 +330,10 @@ handle_message(RedChannelClient *rcc, uint16_t type,
> > uint32_t size, void *msg)
> > case SPICE_MSGC_DISPLAY_GL_DRAW_DONE:
> > /* client should not send this message */
> > return false;
> > + case SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE:
> > + return stream_channel_handle_preferred_video_codec_type(rcc,
> > + (SpiceMsgcDisplayPreferredVideoCodecType *)msg);
> > + return false;
>
> Second return after a return is not much useful, some code analysers
> could also complain about it.
yes, this is fully useless, I cancelled the second return
> > default:
> > return red_channel_client_handle_message(rcc, type, size, msg);
> > }
> > @@ -390,6 +400,20 @@ stream_channel_get_supported_codecs(StreamChannel
> > *channel, uint8_t *out_codecs)
> > return num;
> > }
> >
> > +static bool
> > +stream_channel_handle_preferred_video_codec_type(RedChannelClient *rcc,
> > +
> > SpiceMsgcDisplayPreferredVideoCodecType
> > *msg)
> > +{
> > + StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
> > +
> > + g_return_val_if_fail(msg->num_of_codecs > 0, TRUE);
>
> I know it's not a regression, but document for g_return_val_if_fail states
> "Verifies that the expression expr , usually representing a precondition, evaluates to TRUE"
> but this is not a precondition, this is checking a value from network.
I don't know, it actually looks like a precondition to me, even if
it's a RPC call.
but the type of num_of_codecs is uint8_t, so >=0.
I can replace this with
> if (msg->num_of_codecs == 0) {
> return true;
> }
> > +
> > + g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
> > + scc->client_preferred_video_codecs =
> > video_stream_parse_preferred_codecs(msg);
> > +
> > + return TRUE;
>
> I would use true/false with bool
sure
> > +}
> > +
> > static void
> > stream_channel_connect(RedChannel *red_channel, RedClient *red_client,
> > RedStream *stream,
> > int migration, RedChannelCapabilities *caps)
> > @@ -448,10 +472,25 @@ stream_channel_constructed(GObject *object)
> >
> > red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> > + red_channel_set_cap(red_channel,
> > SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE);
> >
> > reds_register_channel(reds, red_channel);
> > }
> >
> > +static void
> > +stream_channel_finalize(GObject *object)
> > +{
> > + StreamChannel *self = STREAM_CHANNEL(object);
> > + RedChannelClient *rcc;
> > +
> > + FOREACH_CLIENT(self, rcc) {
> > + StreamChannelClient *scc = STREAM_CHANNEL_CLIENT(rcc);
> > + g_clear_pointer(&scc->client_preferred_video_codecs, g_array_unref);
> > + }
>
> Each object should free itself, this should be in
> stream_channel_client_finalize, otherwise code will leak when client disconnects.
I see, I confused myself with the channel_class vs channel_client_class.
Fixed now
> > +
> > + G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
> > +}
> > +
> > static void
> > stream_channel_class_init(StreamChannelClass *klass)
> > {
> > @@ -459,6 +498,7 @@ stream_channel_class_init(StreamChannelClass *klass)
> > RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> >
> > object_class->constructed = stream_channel_constructed;
> > + object_class->finalize = stream_channel_finalize;
> >
> > channel_class->parser =
> > spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
> > channel_class->handle_message = handle_message;
> > diff --git a/server/video-stream.c b/server/video-stream.c
> > index 6aa859a0..a61c8ab2 100644
> > --- a/server/video-stream.c
> > +++ b/server/video-stream.c
> > @@ -20,6 +20,7 @@
> > #include "display-channel-private.h"
> > #include "main-channel-client.h"
> > #include "red-client.h"
> > +#include "stream-channel.h"
> >
>
> Why this?
removed
> > #define FPS_TEST_INTERVAL 1
> > #define FOREACH_STREAMS(display, item) \
> > @@ -468,6 +469,41 @@ static bool video_stream_add_frame(DisplayChannel
> > *display,
> > return FALSE;
> > }
> >
> > +/* Returns an array with SPICE_VIDEO_CODEC_TYPE_ENUM_END elements,
> > + * with the client preference order (index) as value */
> > +GArray
> > *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType
> > *msg)
> > +{
> > + gint i, len;
>
> gnot ga gbig gfun gof gall gthese gg gstrings
fixed, this is a code moved from elsewhere, but it's worth fixing the
types as glib is not involved with this variables
> GArray *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType *msg)
> {
> int i, len;
> int indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
> GArray *client;
>
> /* set default to a big and positive number */
> memset(indexes, 0x7f, sizeof(indexes));
>
> for (len = 0, i = 0; i < msg->num_of_codecs; i++) {
> SpiceVideoCodecType 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 unknown video-codec (value %d at index %d). "
> "Ignoring as server can't handle it",
> video_codec, i);
> continue;
> }
>
> if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> continue;
> }
>
> len++;
> indexes[video_codec] = len;
> }
> client = g_array_sized_new(FALSE, FALSE, sizeof(int), SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
>
> return client;
> }
---
but now, I'm not sure what to do with this Preferred Video Codec list.
This patch only sets the StreamChannelClient attribute:
> scc->client_preferred_video_codecs = video_stream_parse_preferred_codecs(msg);
so it should be discussed in with this email:
> [RFC spice-server 3/3] streaming: Restart guest video streams on video-codec changes
thanks,
Kevin
> > + gint indexes[SPICE_VIDEO_CODEC_TYPE_ENUM_END];
> > + GArray *client;
> > +
> > + /* set default to a big and positive number */
> > + memset(indexes, 0x7f, sizeof(indexes));
> > +
> > + for (len = 0, 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 unknown video-codec (value %d at
> > index %d). "
> > + "Ignoring as server can't handle it",
> > + video_codec, i);
> > + continue;
> > + }
> > +
> > + if (indexes[video_codec] < SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
> > + continue;
> > + }
> > +
> > + len++;
> > + indexes[video_codec] = len;
> > + }
> > + client = g_array_sized_new(FALSE, FALSE, sizeof(gint),
> > SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > + g_array_append_vals(client, indexes, SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> > +
> > + return client;
> > +}
> > +
> > /* TODO: document the difference between the 2 functions below */
> > void video_stream_trace_update(DisplayChannel *display, Drawable *drawable)
> > {
> > diff --git a/server/video-stream.h b/server/video-stream.h
> > index 46b076fd..73435515 100644
> > --- a/server/video-stream.h
> > +++ b/server/video-stream.h
> > @@ -147,6 +147,7 @@ void video_stream_detach_and_stop(DisplayChannel
> > *display);
> > void video_stream_trace_add_drawable(DisplayChannel *display, Drawable
> > *item);
> > void video_stream_detach_behind(DisplayChannel *display, QRegion *region,
> > Drawable *drawable);
> > +GArray
> > *video_stream_parse_preferred_codecs(SpiceMsgcDisplayPreferredVideoCodecType
> > *msg);
> >
> > void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent
> > *agent);
> > void video_stream_agent_stop(VideoStreamAgent *agent);
>
> Frediano
More information about the Spice-devel
mailing list