Hi, On Thu, Jan 19, 2017 at 12:14:06PM +0200, Uri Lublin wrote: > On 01/06/2017 10:19 AM, Victor Toso wrote: > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > 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. > > Hi Victor, > > Note that the behavior is different. > For example, this implementation accepts strings like > "@#$@34:@%$^$%^" > ":" > "ddf:" > ":fdf" > > as valid while previous one rejected them. The input is accepted but it isn't valid as we check each value to static array with the valid values. I changed the behavior slightly here. All examples above will fail to change the video preference although the error message might differ. > > Also for "a:b:c" -- previous implementation > returned "a" and "b", this one returns "a" and "b:c" Again, the input wouldn't be valid or accepted. The reason for that is to easily extend with the priority per video-codec patch which was sent as RFC [0], the interesting change would be: - pref = g_strsplit(preferences[i], ":", 2); + pref = g_strsplit(preferences[i], ":", 3); [0] https://lists.freedesktop.org/archives/spice-devel/2017-January/034849.html I don't see any necessary change for this cases, unless I'm not seeing something important as most of this are wrong inputs that in both implementations are wrong. Maybe improving the commit log? Thanks for reviewing, toso > > Regards, > Uri. > > > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > --- > > server/reds.c | 62 +++++++++++++++++++++++++++-------------------------------- > > 1 file changed, 28 insertions(+), 34 deletions(-) > > > > diff --git a/server/reds.c b/server/reds.c > > index e061e4d..26a8b51 100644 > > --- a/server/reds.c > > +++ b/server/reds.c > > @@ -3512,34 +3512,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); > > > > @@ -3548,13 +3529,28 @@ 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)) { > > @@ -3570,11 +3566,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); > > >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel