[Spice-devel] [PATCH spice-server] reds: Add ability to query the video-codecs currently enabled

[Frediano Ziglio]

> 
> 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 at redhat.com>
> ---
>  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.

> 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;

??

>  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

> +    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 " */"

> +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.

Frediano