Hi,
On Tue, Jan 31, 2017 at 07:16:04AM -0500, Frediano Ziglio 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.
> >
> > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
>
> 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
typo: s/not the space/note the space
> others less (but different) ?
At first I did not understand so I'll try to clarify slightly now that
I've talked with Frediano.
Current input that is supported by the API is like this:
[0] encoder1:codec1;encoder2:codec3;encoder2:codec2;
- We do have the priority set here, which is: codec1 > codec3 > codec2.
- We also have a way to disable values, for instance codec4 will never
be used because it was not set.
- We don't have a way to say that a few codecs might have the *same*
priority, which is addressed by Frediano's suggestions, such as:
[1] encoder1:codec1 encoder2:codec3;encoder2:codec2;
- The space means that both tuples have the same priority, meaning:
codec1 == codec3 > codec2
---
Just for reference, what I have proposed is an additional (numeric and
optional) field, and following the example above it would be:
[2] encoder1:codec1:2;encoder2:codec3:2;encoder2:codec2:1;
I guess both have pros/cons and some discussion is needed!
Thanks!
toso
>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel