On Thu, Jul 4, 2019 at 2:30 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>
> >
> > spice_server_get_video_codecs: new public function to query the video
> > encoders/codecs currently enabled in the spice server. It returns a
> > string that can be fed to the setter counter
> > (spice_server_set_video_codecs). The string returned by this function
> > should be released with spice_server_free_video_codecs.
> >
> > spice_server_free_video_codecs: new public function to free the the
> > video codec list returned by spice_server_get_video_codecs.
> >
> > Signed-off-by: Kevin Pouget <kpouget@xxxxxxxxxx>
> > ---
> > server/dcc.c | 5 +++-
> > server/reds.c | 52 ++++++++++++++++++++++++++++++++++++++++
> > server/reds.h | 2 ++
> > server/spice-server.h | 12 ++++++++++
> > server/spice-server.syms | 6 +++++
> > 5 files changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 71d09b77..808e4248 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -1140,7 +1140,10 @@ static void
> > dcc_update_preferred_video_codecs(DisplayChannelClient *dcc)
> > 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);
> > + char *codec_name = reds_get_video_codec_fullname(&codec);
> > +
> > + g_string_append_printf(msg, "%s ", codec_name);
> > + g_free(codec_name);
> > }
> > spice_debug("%s", msg->str);
> > g_string_free(msg, TRUE);
>
> Looks like this code is doing really similar stuff to the new public function.
> Maybe the code for format from the array of RedVideoCodec to string may be
> the same?
>
> I assume the function would be in display-channel.c as dealing display
> stuff although from current declaration I would see it in a video-encoder.c
> file.
yes indeed, it's exactly the same code, I merged them into a new
function video_codecs_to_string
that I put in video-stream.c (there's no video-encoder.c, and
display-channel.c doesn't deal with RedVideoCodec)
> > diff --git a/server/reds.c b/server/reds.c
> > index 671e0a86..a51d576e 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3708,6 +3708,18 @@ static gboolean get_name_index(const EnumNames
> > names[], const char *name, uint32
> > return FALSE;
> > }
> >
> > +/* returns NULL if index is invalid. */
> > +static const char *get_index_name(const EnumNames names[], uint32_t index)
> > +{
> > + int i;
> > +
> > + for (i = 0; names[i].name != NULL && names[i].id != index; i++) {
> > + continue;
> > + }
> > +
> > + return names[i].name;
> > +}
> > +
>
> Maybe it's me more familiar with pointers, what about
>
> while (names->name != NULL && names->id != index) {
> ++names;
> }
> return names->name;
>
> ??
yes, that's indeed a bit more lightweight
I'm not really fan of the pre-increment notation though (slightly less
natural to read, and no actual benefit),
I changed it if you don't mind
> > static const EnumNames renderer_names[] = {
> > {RED_RENDERER_SW, "sw"},
> > {RED_RENDERER_INVALID, NULL},
> > @@ -3755,6 +3767,24 @@ static const int video_codec_caps[] = {
> > SPICE_DISPLAY_CAP_CODEC_VP9,
> > };
> >
> > +char *reds_get_video_codec_fullname(RedVideoCodec *codec)
> > +{
> > + int i;
> > + const char *encoder_name = NULL;
> > + const char *codec_name = get_index_name(video_codec_names, codec->type);
> > +
> > + spice_assert(codec_name);
> > +
> > + for (i = 0; i < G_N_ELEMENTS(video_encoder_procs); i++) {
> > + if (video_encoder_procs[i] == codec->create) {
> > + encoder_name = get_index_name(video_encoder_names, i);
> > + break;
> > + }
> > + }
> > + spice_assert(encoder_name);
> > +
> > + return g_strdup_printf("%s:%s", encoder_name, codec_name);
> > +}
> >
> > /* Parses the given codec string and returns newly-allocated strings
> > describing
> > * the next encoder and codec in the list. These strings must be freed by
> > the
> > @@ -4196,6 +4226,28 @@ SPICE_GNUC_VISIBLE int
> > spice_server_set_video_codecs(SpiceServer *reds, const ch
> > return 0;
> > }
> >
> > +SPICE_GNUC_VISIBLE const char *spice_server_get_video_codecs(SpiceServer
> > *reds)
> > +{
> > + GString *msg;
> > + int i;
> > + GArray *video_codecs = reds_get_video_codecs(reds);
> > +
> > + msg = g_string_new("");
> > + for (i = 0; i < video_codecs->len; i++) {
> > + RedVideoCodec codec = g_array_index(video_codecs, RedVideoCodec, i);
> > + char *codec_name = reds_get_video_codec_fullname(&codec);
> > +
> > + g_string_append_printf(msg, "%s;", codec_name);
> > + g_free(codec_name);
> > + }
> > +
> > + return g_string_free(msg, FALSE);
> > +}
> > +
> > +SPICE_GNUC_VISIBLE void spice_server_free_video_codecs(SpiceServer *reds,
> > const char *video_codecs) {
>
> Minor, style: bracket on next line
fixed
> > + g_free((char *) video_codecs);
> > +}
> > +
> > GArray* reds_get_video_codecs(const RedsState *reds)
> > {
> > return reds->config->video_codecs;
> > diff --git a/server/reds.h b/server/reds.h
> > index 8c5ee84d..e3355f81 100644
> > --- a/server/reds.h
> > +++ b/server/reds.h
> > @@ -27,6 +27,7 @@
> > #include "char-device.h"
> > #include "spice.h"
> > #include "red-channel.h"
> > +#include "video-encoder.h"
> > #include "main-dispatcher.h"
> > #include "migration-protocol.h"
> >
> > @@ -54,6 +55,7 @@ void reds_send_device_display_info(RedsState *reds);
> > void reds_handle_agent_mouse_event(RedsState *reds, const VDAgentMouseState
> > *mouse_state); // used by inputs_channel
> >
> > GArray* reds_get_renderers(RedsState *reds);
> > +char *reds_get_video_codec_fullname(RedVideoCodec *codec);
> >
> > enum {
> > RED_RENDERER_INVALID,
> > diff --git a/server/spice-server.h b/server/spice-server.h
> > index 5f572f4f..481d5194 100644
> > --- a/server/spice-server.h
> > +++ b/server/spice-server.h
> > @@ -136,6 +136,18 @@ enum {
> > };
> >
> > int spice_server_set_video_codecs(SpiceServer *s, const char* video_codecs);
> > +/* Returns a newly allocated string describing video encoders/codecs
> > + * currently allowed in @reds Spice server. The string returned by
> > + * this function must be released with spice_server_free_video_codecs.
> > + *
> > + * @reds: the Spice server to query
> > + * @return the string describing the video encoders/codecs currently enabled
> > + */
> > +const char *spice_server_get_video_codecs(SpiceServer *s);
> > +/* Releases the memory of the video-codec string returned by
> > + * spice_server_get_video_codecs.
> > + * */
>
> Minor style: just " */"
fixed
> > +void spice_server_free_video_codecs(SpiceServer *reds, const char
> > *video_codecs);
> > int spice_server_set_playback_compression(SpiceServer *s, int enable);
> > int spice_server_set_agent_mouse(SpiceServer *s, int enable);
> > int spice_server_set_agent_copypaste(SpiceServer *s, int enable);
> > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > index ac4d9b14..e33526f3 100644
> > --- a/server/spice-server.syms
> > +++ b/server/spice-server.syms
> > @@ -178,3 +178,9 @@ SPICE_SERVER_0.14.2 {
> > global:
> > spice_qxl_set_device_info;
> > } SPICE_SERVER_0.13.2;
> > +
> > +SPICE_SERVER_0.14.3 {
> > +global:
> > + spice_server_get_video_codecs;
> > + spice_server_free_video_codecs;
> > +} SPICE_SERVER_0.14.2;
>
> Very OT: looks like the current code is quite not clear on how these stuff
> are split, having some code/data on reds.c, some other in dcc/display-channel
> and some also in video-encoding.h. Not a regression and it's probably caused
> by evolution of reds.c which is quite a mess IMHO. But nothing to stop this
> series/patch surely.
yes indeed, these functions are spread in several files, but at the
same time they're
related to various components (redsState, red-worker, dcc, display
channel, encoders),
it's hard to know where they would better fit ... !
I put the new version of the patch below
thanks,
Kevin
---
Author: Kevin Pouget <kpouget@xxxxxxxxxx>
Date: Tue Jun 18 15:25:29 2019 +0200
reds: Add ability to query the video-codecs currently enabled
spice_server_get_video_codecs: new public function to query the video
encoders/codecs currently enabled in the spice server. It returns a
string that can be fed to the setter counter
(spice_server_set_video_codecs). The string returned by this function
should be released with spice_server_free_video_codecs.
spice_server_free_video_codecs: new public function to free the the
video codec list returned by spice_server_get_video_codecs.
video_codecs_to_string: new private function to transform an array of
RedVideoCodec into a string.
Signed-off-by: Kevin Pouget <kpouget@xxxxxxxxxx>
diff --git a/server/dcc.c b/server/dcc.c
index bd393e04..b364a8e1 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1120,9 +1120,8 @@ static gint
sort_video_codecs_by_client_preference(gconstpointer a_pointer,
static void dcc_update_preferred_video_codecs(DisplayChannelClient *dcc)
{
- guint i;
GArray *video_codecs, *server_codecs;
- GString *msg;
+ char *codecs_str;
server_codecs = display_channel_get_video_codecs(DCC_TO_DC(dcc));
spice_return_if_fail(server_codecs != NULL);
@@ -1137,13 +1136,9 @@ static void
dcc_update_preferred_video_codecs(DisplayChannelClient *dcc)
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_debug("%s", msg->str);
- g_string_free(msg, TRUE);
+ codecs_str = video_codecs_to_string(video_codecs, " ");
+ spice_debug("Preferred video-codecs: %s", codecs_str);
+ g_free(codecs_str);
}
static void on_display_video_codecs_update(GObject *gobject,
GParamSpec *pspec, gpointer user_data)
diff --git a/server/reds.c b/server/reds.c
index aabfe919..7783ab0b 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3708,6 +3708,16 @@ static gboolean get_name_index(const EnumNames
names[], const char *name, uint32
return FALSE;
}
+/* returns NULL if index is invalid. */
+static const char *get_index_name(const EnumNames names[], uint32_t index)
+{
+ while (names->name != NULL && names->id != index) {
+ names++;
+ }
+
+ return names->name;
+}
+
static const EnumNames renderer_names[] = {
{RED_RENDERER_SW, "sw"},
{RED_RENDERER_INVALID, NULL},
@@ -3755,6 +3765,24 @@ static const int video_codec_caps[] = {
SPICE_DISPLAY_CAP_CODEC_VP9,
};
+char *reds_get_video_codec_fullname(RedVideoCodec *codec)
+{
+ int i;
+ const char *encoder_name = NULL;
+ const char *codec_name = get_index_name(video_codec_names, codec->type);
+
+ spice_assert(codec_name);
+
+ for (i = 0; i < G_N_ELEMENTS(video_encoder_procs); i++) {
+ if (video_encoder_procs[i] == codec->create) {
+ encoder_name = get_index_name(video_encoder_names, i);
+ break;
+ }
+ }
+ spice_assert(encoder_name);
+
+ return g_strdup_printf("%s:%s", encoder_name, codec_name);
+}
/* Parses the given codec string and returns newly-allocated strings describing
* the next encoder and codec in the list. These strings must be freed by the
@@ -4223,6 +4251,16 @@ SPICE_GNUC_VISIBLE int
spice_server_set_video_codecs(SpiceServer *reds, const ch
return 0;
}
+SPICE_GNUC_VISIBLE const char *spice_server_get_video_codecs(SpiceServer *reds)
+{
+ return video_codecs_to_string(reds_get_video_codecs(reds), ";");
+}
+
+SPICE_GNUC_VISIBLE void spice_server_free_video_codecs(SpiceServer
*reds, const char *video_codecs)
+{
+ g_free((char *) video_codecs);
+}
+
GArray* reds_get_video_codecs(const RedsState *reds)
{
return reds->config->video_codecs;
diff --git a/server/reds.h b/server/reds.h
index 8c5ee84d..e3355f81 100644
--- a/server/reds.h
+++ b/server/reds.h
@@ -27,6 +27,7 @@
#include "char-device.h"
#include "spice.h"
#include "red-channel.h"
+#include "video-encoder.h"
#include "main-dispatcher.h"
#include "migration-protocol.h"
@@ -54,6 +55,7 @@ void reds_send_device_display_info(RedsState *reds);
void reds_handle_agent_mouse_event(RedsState *reds, const
VDAgentMouseState *mouse_state); // used by inputs_channel
GArray* reds_get_renderers(RedsState *reds);
+char *reds_get_video_codec_fullname(RedVideoCodec *codec);
enum {
RED_RENDERER_INVALID,
diff --git a/server/spice-server.h b/server/spice-server.h
index 5f572f4f..24df8e65 100644
--- a/server/spice-server.h
+++ b/server/spice-server.h
@@ -136,6 +136,18 @@ enum {
};
int spice_server_set_video_codecs(SpiceServer *s, const char* video_codecs);
+/* Returns a newly allocated string describing video encoders/codecs
+ * currently allowed in @reds Spice server. The string returned by
+ * this function must be released with spice_server_free_video_codecs.
+ *
+ * @reds: the Spice server to query
+ * @return the string describing the video encoders/codecs currently enabled
+ */
+const char *spice_server_get_video_codecs(SpiceServer *s);
+/* Releases the memory of the video-codec string returned by
+ * spice_server_get_video_codecs.
+ */
+void spice_server_free_video_codecs(SpiceServer *reds, const char
*video_codecs);
int spice_server_set_playback_compression(SpiceServer *s, int enable);
int spice_server_set_agent_mouse(SpiceServer *s, int enable);
int spice_server_set_agent_copypaste(SpiceServer *s, int enable);
diff --git a/server/spice-server.syms b/server/spice-server.syms
index ac4d9b14..e33526f3 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -178,3 +178,9 @@ SPICE_SERVER_0.14.2 {
global:
spice_qxl_set_device_info;
} SPICE_SERVER_0.13.2;
+
+SPICE_SERVER_0.14.3 {
+global:
+ spice_server_get_video_codecs;
+ spice_server_free_video_codecs;
+} SPICE_SERVER_0.14.2;
diff --git a/server/video-encoder.h b/server/video-encoder.h
index 71f4131a..90d21312 100644
--- a/server/video-encoder.h
+++ b/server/video-encoder.h
@@ -213,4 +213,6 @@ typedef struct RedVideoCodec {
uint32_t cap;
} RedVideoCodec;
+char *video_codecs_to_string(GArray *video_codecs, const char *sep);
+
#endif /* VIDEO_ENCODER_H_ */
diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8..a39262d5 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -970,3 +970,25 @@ void
video_stream_trace_add_drawable(DisplayChannel *display,
trace->height = src_area->bottom - src_area->top;
trace->dest_area = item->red_drawable->bbox;
}
+
+/*
+ * video_codecs: an array of RedVideoCodec
+ * sep: a string for separating the list elements
+ *
+ * returns a string of "enc:codec<sep>"* that must be released
+ * with g_free.
+ */
+char *video_codecs_to_string(GArray *video_codecs, const char *sep) {
+ int i;
+ GString *msg = g_string_new("");
+
+ for (i = 0; i < video_codecs->len; i++) {
+ RedVideoCodec codec = g_array_index(video_codecs, RedVideoCodec, i);
+ char *codec_name = reds_get_video_codec_fullname(&codec);
+
+ g_string_append_printf(msg, "%s%s", codec_name, sep);
+ g_free(codec_name);
+ }
+
+ return g_string_free(msg, FALSE);
+}
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel