[Spice-devel] [RFC spice-server 1/3] stream-channel: Add preferred video codec capability
Frediano Ziglio
fziglio at redhat.com
Wed Aug 14 13:08:51 UTC 2019
>
> 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 ?
> #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.
> };
>
> 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
> 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.
> 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.
> +
> + 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
> +}
> +
> 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.
> +
> + 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?
> #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
> + 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