[FFmpeg-devel] [PATCH] opt: implement av_opt_set_from_string().

[Nicolas George]

Le quintidi 25 fructidor, an CCXX, Stefano Sabatini a écrit :
> Flat syntax works this way:
> val1:val2
> 
> If you want to set b, you *must* set a, so :42 to me means:
> key1=val1:key2=val2
> 
> so in this case:
> :42
> =>
> a=:b=42
> 
> Should be also possibly simpler to implement.

That is what the patch currently implements, and it is clearly the simplest.
I just asked because some other pieces of software may work otherwise. For
example, for mplayer, -vf expand=::::1 will set to 1 the fifth parameter
(osd) and leave the first four to their default value (they are integers, an
empty string would be a syntax error).

But if you are ok with it I am too.

> > +#define WHITESPACES " \n\t"
> \r\t\f?

I copypasted the one used in av_get_token. We may want to unify that at some
point but that is unrelated. For now, I will leave it that way because it
would be strange to ignore \r around the option name but not the option
value.

> Nit: MUST be terminated

Changed.

> Nit: key_pos for enhanced readability

Idem.

> this pos++ increment is a bit confusing (and possibly unnecessary)

It seems more logical to me, and it makes the test just below simpler:

> > +    if (pos == key_size)

> > +                av_log(ctx, AV_LOG_ERROR, "Option '%s' not found.\n", key);
> Nit: strip trailing "."

Fixed here, not in the first instance where I copypasted it from
(unrelated).

> > +        test_ctx.string = av_strdup("default");
> Note: this should not be necessary anymore, since we support default
> string values since a long time.

We may want to support situations where a value was set by accessing the
fields directly.

> Could you show the output? (I don't remember if the output of the test
> is checked by FATE).

See at the end.

> > + * @param ctx          the AVClass object to set options
> nit: to set option on? (on which/where to set options)

Done.

> I don't know if this is the right place where to document the key
> limitations, possibly somewhere in opt.h/AVOption.name is a better
> place (rather than in this patch).

I added this at the end of the doxy so that it does not get lost if we
forget to add it at the proper place:

 * Options names must use only the following characters: a-z A-Z 0-9 - . / _
 * Separators must use characters distinct from from options and from each
 * other.

> Looks nice otherwise, thanks so far for your work on this.

No problem. New patch incoming.

Regards,

-- 
  Nicolas George


Output of the test part (with *stars* around the part at log level error,
the rest is debug):

Testing av_opt_set_from_string()
[test @ 0x7fffa1ea7210] Setting options string ''

[test @ 0x7fffa1ea7210] Setting options string '5'
[test @ 0x7fffa1ea7210] Setting 'num' to value '5'

[test @ 0x7fffa1ea7210] Setting options string '5:hello'
[test @ 0x7fffa1ea7210] Setting 'num' to value '5'
[test @ 0x7fffa1ea7210] Setting 'string' to value 'hello'

[test @ 0x7fffa1ea7210] Setting options string '5:hello:size=pal'
[test @ 0x7fffa1ea7210] Setting 'num' to value '5'
[test @ 0x7fffa1ea7210] Setting 'string' to value 'hello'
[test @ 0x7fffa1ea7210] Setting 'size' to value 'pal'

[test @ 0x7fffa1ea7210] Setting options string '5:size=pal:hello'
[test @ 0x7fffa1ea7210] Setting 'num' to value '5'
[test @ 0x7fffa1ea7210] Setting 'size' to value 'pal'
[test @ 0x7fffa1ea7210] *No option name near 'hello'*
[test @ 0x7fffa1ea7210] *Error setting options string: '5:size=pal:hello'*

[test @ 0x7fffa1ea7210] Setting options string ':'
[test @ 0x7fffa1ea7210] Setting 'num' to value ''
[test @ 0x7fffa1ea7210] [Eval @ 0x7fffa1ea6e50] *Undefined constant or missing '(' in ''*
[test @ 0x7fffa1ea7210] *Unable to parse option value ""*
[test @ 0x7fffa1ea7210] *Error setting options string: ':'*

[test @ 0x7fffa1ea7210] Setting options string '='
[test @ 0x7fffa1ea7210] Setting '' to value ''
[test @ 0x7fffa1ea7210] Option '' not found
[test @ 0x7fffa1ea7210] Error setting options string: '='

[test @ 0x7fffa1ea7210] Setting options string ' 5 : hello : size = pal '
[test @ 0x7fffa1ea7210] Setting 'num' to value '5'
[test @ 0x7fffa1ea7210] Setting 'string' to value 'hello'
[test @ 0x7fffa1ea7210] Setting 'size' to value 'pal'

[test @ 0x7fffa1ea7210] Setting options string 'a_very_long_option_name_that_will_need_to_be_ellipsized_around_here=42'
[test @ 0x7fffa1ea7210] Setting 'a_very_long_option_name_that_will_need_to_be_ellipsized_around_h...' to value '42'
[test @ 0x7fffa1ea7210] *Option 'a_very_long_option_name_that_will_need_to_be_ellipsized_around_h...' not found*
[test @ 0x7fffa1ea7210] *Error setting options string: 'a_very_long_option_name_that_will_need_to_be_ellipsized_around_here=42'*
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120916/ebaa4e48/attachment.asc>