[Spice-devel] [spice-server v6 7/9] reds: drop sscanf() in favour of g_strsplit()

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> As there is a need to iterate over every encoder:codec pair and we do
> a check for every encoder and every codec, g_strsplit() is less
> complex and easier to follow.
> 
> This change makes much easier to extend the input which is currently a
> list of tuples encoder:video-codec.
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>

I had a though this morning about the syntax.

We are moving from a set or "encoder:codec" separated with ";" to
a set of "encoder:codec:priority". All this to allow same priority in
the list. Why not having a "light" separator, something like

"gstreamer:vp8 gstreamer:h264;gstreamer:h264;spice:mjpeg"

(not the space). So the first 2 will have same "priority" while
the others less (but different) ?

> ---
>  server/reds.c                      | 63
>  ++++++++++++++++++--------------------
>  server/tests/test-codecs-parsing.c |  8 ++---
>  2 files changed, 33 insertions(+), 38 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index fb58bd0..d20edee 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3527,34 +3527,15 @@ static const int video_codec_caps[] = {
>  };
>  
>  
> -/* Expected string:  encoder:codec;encoder:codec */
> -static const char* parse_video_codecs(const char *codecs, char **encoder,
> -                                      char **codec)
> -{
> -    if (!codecs) {
> -        return NULL;
> -    }
> -    while (*codecs == ';') {
> -        codecs++;
> -    }
> -    if (!*codecs) {
> -        return NULL;
> -    }
> -    int n;
> -    *encoder = *codec = NULL;
> -    if (sscanf(codecs, "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n", encoder, codec,
> &n) != 2) {
> -        while (*codecs != '\0' && *codecs != ';') {
> -            codecs++;
> -        }
> -        return codecs;
> -    }
> -    return codecs + n;
> -}
> -
> +/*
> + * @codecs should be described in a semi-colon separated list of
> encoder:codec.
> + * e.g: "gstreamer:vp8;spice:mjpeg"
> + */
>  static void reds_set_video_codecs_from_string(RedsState *reds, const char
>  *codecs)
>  {
> -    char *encoder_name, *codec_name;
>      GArray *video_codecs;
> +    gchar **preferences;
> +    gint i;
>  
>      g_return_if_fail(codecs != NULL);
>  
> @@ -3563,13 +3544,29 @@ static void
> reds_set_video_codecs_from_string(RedsState *reds, const char *codec
>      }
>  
>      video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
> -    const char *c = codecs;
> -    while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
> +    preferences = g_strsplit(codecs, ";", -1);
> +    for (i = 0; preferences[i] != NULL; i++) {
> +        const gchar *encoder_name, *codec_name;
>          uint32_t encoder_index, codec_index;
> -        if (!encoder_name || !codec_name) {
> -            spice_warning("spice: invalid encoder:codec value at %s",
> codecs);
> +        gchar **pref;
> +
> +        /* Don't bother with empty strings that would be an extra ';'
> somewhere */
> +        if (g_str_equal(preferences[i], "")) {
> +            continue;
> +        }
> +
> +        pref = g_strsplit(preferences[i], ":", 2);
>  
> -        } else if (!get_name_index(video_encoder_names, encoder_name,
> &encoder_index)){
> +        if (pref[0] == NULL || pref[1] == NULL) {
> +            spice_warning("spice: invalid encoder:codec value at '%s'",
> preferences[i]);
> +            g_strfreev(pref);
> +            continue;
> +        }
> +
> +        encoder_name = pref[0];
> +        codec_name = pref[1];
> +
> +        if (!get_name_index(video_encoder_names, encoder_name,
> &encoder_index)){
>              spice_warning("spice: unknown video encoder %s", encoder_name);
>  
>          } else if (!get_name_index(video_codec_names, codec_name,
>          &codec_index)) {
> @@ -3585,11 +3582,9 @@ static void
> reds_set_video_codecs_from_string(RedsState *reds, const char *codec
>              new_codec.cap = video_codec_caps[codec_index];
>              g_array_append_val(video_codecs, new_codec);
>          }
> -
> -        free(encoder_name);
> -        free(codec_name);
> -        codecs = c;
> +        g_strfreev(pref);
>      }
> +    g_strfreev(preferences);
>  
>      if (video_codecs->len == 0) {
>          spice_warning("Failed to set video codecs, input string: '%s'",
>          codecs);
> diff --git a/server/tests/test-codecs-parsing.c
> b/server/tests/test-codecs-parsing.c
> index 5af2e5d..16fab24 100644
> --- a/server/tests/test-codecs-parsing.c
> +++ b/server/tests/test-codecs-parsing.c
> @@ -84,12 +84,12 @@ static void codecs_bad(void)
>          },{
>              ";:;",
>              G_LOG_LEVEL_WARNING,
> -            "*spice: invalid encoder:codec value*",
> +            "*spice: unknown video encoder*",
>              TRUE,
>          },{
>              "::::",
>              G_LOG_LEVEL_WARNING,
> -            "*spice: invalid encoder:codec value*",
> +            "*spice: unknown video encoder*",
>              TRUE,
>          },{
>              "missingcolon",
> @@ -99,12 +99,12 @@ static void codecs_bad(void)
>          },{
>              ":missing_encoder",
>              G_LOG_LEVEL_WARNING,
> -            "*spice: invalid encoder:codec value*",
> +            "*spice: unknown video encoder*",
>              TRUE,
>          },{
>              "missing_value:;",
>              G_LOG_LEVEL_WARNING,
> -            "*spice: invalid encoder:codec value*",
> +            "*spice: unknown video encoder*",
>              TRUE,
>          },{
>              "unknown_encoder:mjpeg",
> --
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>