[PATCH] -fs parameter human friendly
Hi all, addresses issue 551. It exports the av_strtod() function to eval.h and uses it in ffmpeg.c (minor bump missing for easying the maintainance of the patch). Regards. -- FFmpeg = Funny and Fiendish Meaningless Patchable Embarassing Guide
On Mon, Aug 11, 2008 at 11:00:17AM +0200, Stefano Sabatini wrote:
Hi all,
addresses issue 551. It exports the av_strtod() function to eval.h and uses it in ffmpeg.c (minor bump missing for easying the maintainance of the patch). [...] +static int opt_limit_filesize(const char* opt, const char* arg) +{ + char *tail; + limit_filesize = av_strtod(arg, &tail); + if(*tail) { + fprintf(stderr, "Invalid value for %s: %s\n", opt, arg); + av_exit(1); + } + return 0; +} + static const OptionDef options[] = { /* main options */ { "L", OPT_EXIT, {(void*)show_license}, "show license" }, @@ -3761,7 +3773,7 @@ { "map", HAS_ARG | OPT_EXPERT, {(void*)opt_map}, "set input stream mapping", "file:stream[:syncfile:syncstream]" }, { "map_meta_data", HAS_ARG | OPT_EXPERT, {(void*)opt_map_meta_data}, "set meta data information of outfile from infile", "outfile:infile" }, { "t", OPT_FUNC2 | HAS_ARG, {(void*)opt_recording_time}, "record or transcode \"duration\" seconds of audio/video", "duration" }, - { "fs", HAS_ARG | OPT_INT64, {(void*)&limit_filesize}, "set the limit file size in bytes", "limit_size" }, // + { "fs", OPT_FUNC2 | HAS_ARG, {(void*)opt_limit_filesize}, "set the limit file size", "limit_size" }, //
this is not a reasonable solution fix OPT_INT64 instead [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No great genius has ever existed without some touch of madness. -- Aristotle
On date Monday 2008-08-11 21:23:24 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:00:17AM +0200, Stefano Sabatini wrote:
Hi all,
addresses issue 551. It exports the av_strtod() function to eval.h and uses it in ffmpeg.c (minor bump missing for easying the maintainance of the patch). [...] +static int opt_limit_filesize(const char* opt, const char* arg) +{ + char *tail; + limit_filesize = av_strtod(arg, &tail); + if(*tail) { + fprintf(stderr, "Invalid value for %s: %s\n", opt, arg); + av_exit(1); + } + return 0; +} + static const OptionDef options[] = { /* main options */ { "L", OPT_EXIT, {(void*)show_license}, "show license" }, @@ -3761,7 +3773,7 @@ { "map", HAS_ARG | OPT_EXPERT, {(void*)opt_map}, "set input stream mapping", "file:stream[:syncfile:syncstream]" }, { "map_meta_data", HAS_ARG | OPT_EXPERT, {(void*)opt_map_meta_data}, "set meta data information of outfile from infile", "outfile:infile" }, { "t", OPT_FUNC2 | HAS_ARG, {(void*)opt_recording_time}, "record or transcode \"duration\" seconds of audio/video", "duration" }, - { "fs", HAS_ARG | OPT_INT64, {(void*)&limit_filesize}, "set the limit file size in bytes", "limit_size" }, // + { "fs", OPT_FUNC2 | HAS_ARG, {(void*)opt_limit_filesize}, "set the limit file size", "limit_size" }, //
this is not a reasonable solution
fix OPT_INT64 instead
Yep, bright idea. Regression test passed. Regards. -- FFmpeg = Fostering and Frightening Meaningful Proud Ecstatic Goblin
On date Monday 2008-08-11 23:43:53 +0200, Stefano Sabatini encoded:
On date Monday 2008-08-11 21:23:24 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:00:17AM +0200, Stefano Sabatini wrote:
Hi all,
addresses issue 551. It exports the av_strtod() function to eval.h and uses it in ffmpeg.c (minor bump missing for easying the maintainance of the patch). [...] +static int opt_limit_filesize(const char* opt, const char* arg) +{ + char *tail; + limit_filesize = av_strtod(arg, &tail); + if(*tail) { + fprintf(stderr, "Invalid value for %s: %s\n", opt, arg); + av_exit(1); + } + return 0; +} + static const OptionDef options[] = { /* main options */ { "L", OPT_EXIT, {(void*)show_license}, "show license" }, @@ -3761,7 +3773,7 @@ { "map", HAS_ARG | OPT_EXPERT, {(void*)opt_map}, "set input stream mapping", "file:stream[:syncfile:syncstream]" }, { "map_meta_data", HAS_ARG | OPT_EXPERT, {(void*)opt_map_meta_data}, "set meta data information of outfile from infile", "outfile:infile" }, { "t", OPT_FUNC2 | HAS_ARG, {(void*)opt_recording_time}, "record or transcode \"duration\" seconds of audio/video", "duration" }, - { "fs", HAS_ARG | OPT_INT64, {(void*)&limit_filesize}, "set the limit file size in bytes", "limit_size" }, // + { "fs", OPT_FUNC2 | HAS_ARG, {(void*)opt_limit_filesize}, "set the limit file size", "limit_size" }, //
this is not a reasonable solution
fix OPT_INT64 instead
Yep, bright idea.
Regression test passed.
Regards. -- FFmpeg = Fostering and Frightening Meaningful Proud Ecstatic Goblin
Index: libavcodec/eval.h =================================================================== --- libavcodec/eval.h (revision 14690) +++ libavcodec/eval.h (working copy) @@ -84,4 +84,18 @@ double ff_parse_eval(AVEvalExpr * e, double *const_value, void *opaque); void ff_eval_free(AVEvalExpr * e);
+/** + * Parses the value in \p name and returns its value as a double. + * + * @param name a string representing a number, may contain the the + * 'k', 'M', 'G', 'ki', 'Mi', 'Gi' and 'B' postfixes. This allows + * using f.e. kB, MiB, G and B as a postfix. Values are expressed in + * bits, unless the 'B' postfix is used, in this case the values are + * assumed to be in bytes. + * @param tail if non-NULL put at this address the pointer to the + * character just after the part of the string successfully parsed + * @return the value of the parsed string as a numer of bits + */ +double av_strtod(const char *name, char **tail); + #endif /* FFMPEG_EVAL_H */ Index: libavcodec/eval.c =================================================================== --- libavcodec/eval.c (revision 14690) +++ libavcodec/eval.c (working copy) @@ -82,11 +82,7 @@ ['Y'-'E']= 24, };
-/** strtod() function extended with 'k', 'M', 'G', 'ki', 'Mi', 'Gi' and 'B' - * postfixes. This allows using f.e. kB, MiB, G and B as a postfix. This - * function assumes that the unit of numbers is bits not bytes. - */ -static double av_strtod(const char *name, char **tail) { +double av_strtod(const char *name, char **tail) { double d; char *next; d = strtod(name, &next);
Index: cmdutils.c =================================================================== --- cmdutils.c (revision 14695) +++ cmdutils.c (working copy) @@ -29,6 +29,7 @@ #include "libavfilter/avfilter.h" #include "libavdevice/avdevice.h" #include "libavutil/avstring.h" +#include "libavcodec/eval.h" #include "cmdutils.h" #include "version.h" #ifdef CONFIG_NETWORK @@ -42,7 +43,7 @@ { char *tail; const char *error; - double d = strtod(numstr, &tail); + double d = av_strtod(numstr, &tail); if (*tail) error= "Expected number for %s but found: %s\n"; else if (d < min || d > max)
Ping? -- FFmpeg = Fiendish Fostering Majestic Picky Ecstatic Governor
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote:
On date Monday 2008-08-11 21:23:24 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:00:17AM +0200, Stefano Sabatini wrote:
Hi all,
addresses issue 551. It exports the av_strtod() function to eval.h and uses it in ffmpeg.c (minor bump missing for easying the maintainance of the patch). [...] +static int opt_limit_filesize(const char* opt, const char* arg) +{ + char *tail; + limit_filesize = av_strtod(arg, &tail); + if(*tail) { + fprintf(stderr, "Invalid value for %s: %s\n", opt, arg); + av_exit(1); + } + return 0; +} + static const OptionDef options[] = { /* main options */ { "L", OPT_EXIT, {(void*)show_license}, "show license" }, @@ -3761,7 +3773,7 @@ { "map", HAS_ARG | OPT_EXPERT, {(void*)opt_map}, "set input stream mapping", "file:stream[:syncfile:syncstream]" }, { "map_meta_data", HAS_ARG | OPT_EXPERT, {(void*)opt_map_meta_data}, "set meta data information of outfile from infile", "outfile:infile" }, { "t", OPT_FUNC2 | HAS_ARG, {(void*)opt_recording_time}, "record or transcode \"duration\" seconds of audio/video", "duration" }, - { "fs", HAS_ARG | OPT_INT64, {(void*)&limit_filesize}, "set the limit file size in bytes", "limit_size" }, // + { "fs", OPT_FUNC2 | HAS_ARG, {(void*)opt_limit_filesize}, "set the limit file size", "limit_size" }, //
this is not a reasonable solution
fix OPT_INT64 instead
Yep, bright idea.
Regression test passed.
Regards. -- FFmpeg = Fostering and Frightening Meaningful Proud Ecstatic Goblin
Index: libavcodec/eval.h =================================================================== --- libavcodec/eval.h (revision 14690) +++ libavcodec/eval.h (working copy) @@ -84,4 +84,18 @@ double ff_parse_eval(AVEvalExpr * e, double *const_value, void *opaque); void ff_eval_free(AVEvalExpr * e);
+/** + * Parses the value in \p name and returns its value as a double. + * + * @param name a string representing a number, may contain the the
the the?
+ * 'k', 'M', 'G', 'ki', 'Mi', 'Gi' and 'B' postfixes. This allows + * using f.e. kB, MiB, G and B as a postfix.
<list> postfixes. This allows using f.e <list> postfix. what is this supposed to mean?
+ * Values are expressed in + * bits, unless the 'B' postfix is used, in this case the values are + * assumed to be in bytes.
what?
+ * @param tail if non-NULL put at this address the pointer to the + * character just after the part of the string successfully parsed
parse error
+ * @return the value of the parsed string as a numer of bits
numer!? bits?
+ */ +double av_strtod(const char *name, char **tail); + #endif /* FFMPEG_EVAL_H */ Index: libavcodec/eval.c =================================================================== --- libavcodec/eval.c (revision 14690) +++ libavcodec/eval.c (working copy) @@ -82,11 +82,7 @@ ['Y'-'E']= 24, };
-/** strtod() function extended with 'k', 'M', 'G', 'ki', 'Mi', 'Gi' and 'B' - * postfixes. This allows using f.e. kB, MiB, G and B as a postfix. This - * function assumes that the unit of numbers is bits not bytes. - */ -static double av_strtod(const char *name, char **tail) {
why didnt you just copy this? :(((((((((((((((((
+double av_strtod(const char *name, char **tail) { double d; char *next; d = strtod(name, &next);
Index: cmdutils.c =================================================================== --- cmdutils.c (revision 14695) +++ cmdutils.c (working copy) @@ -29,6 +29,7 @@ #include "libavfilter/avfilter.h" #include "libavdevice/avdevice.h" #include "libavutil/avstring.h" +#include "libavcodec/eval.h" #include "cmdutils.h" #include "version.h" #ifdef CONFIG_NETWORK @@ -42,7 +43,7 @@ { char *tail; const char *error; - double d = strtod(numstr, &tail); + double d = av_strtod(numstr, &tail); if (*tail) error= "Expected number for %s but found: %s\n"; else if (d < min || d > max)
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod(). Actually i think this would be a great idea [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User questions about the command line tools should be sent to the ffmpeg-user ML. And questions about how to use libav* should be sent to the libav-user ML.
On date Wednesday 2008-08-20 01:43:17 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote: [...]
Michael:
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod().
I think this could be a very good idea, anyway maybe is still a good idea to export av_strtod(), skip review of the following patch otherwise.
Index: libavcodec/eval.h =================================================================== --- libavcodec/eval.h (revision 14690) +++ libavcodec/eval.h (working copy) @@ -84,4 +84,18 @@ double ff_parse_eval(AVEvalExpr * e, double *const_value, void *opaque); void ff_eval_free(AVEvalExpr * e);
+/** + * Parses the value in \p name and returns its value as a double. + * + * @param name a string representing a number, may contain the the
the the?
Fixed.
+ * 'k', 'M', 'G', 'ki', 'Mi', 'Gi' and 'B' postfixes. This allows + * using f.e. kB, MiB, G and B as a postfix.
<list> postfixes. This allows using f.e <list> postfix. what is this supposed to mean?
Reworded, now it should be more clear.
+ * Values are expressed in + * bits, unless the 'B' postfix is used, in this case the values are + * assumed to be in bytes.
what?
As above.
+ * @param tail if non-NULL put at this address the pointer to the + * character just after the part of the string successfully parsed
parse error
Check the new explanation, I took it from a comment in the implementation, I don't think parse error is very expressive.
+ * @return the value of the parsed string as a numer of bits
numer!? bits?
Fixed the typo, as for the return value, yes it is expressed in bits.
+ */ +double av_strtod(const char *name, char **tail); + #endif /* FFMPEG_EVAL_H */ Index: libavcodec/eval.c =================================================================== --- libavcodec/eval.c (revision 14690) +++ libavcodec/eval.c (working copy) @@ -82,11 +82,7 @@ ['Y'-'E']= 24, };
-/** strtod() function extended with 'k', 'M', 'G', 'ki', 'Mi', 'Gi' and 'B' - * postfixes. This allows using f.e. kB, MiB, G and B as a postfix. This - * function assumes that the unit of numbers is bits not bytes. - */ -static double av_strtod(const char *name, char **tail) {
why didnt you just copy this? :(((((((((((((((((
+double av_strtod(const char *name, char **tail) { double d; char *next; d = strtod(name, &next);
Index: cmdutils.c =================================================================== --- cmdutils.c (revision 14695) +++ cmdutils.c (working copy) @@ -29,6 +29,7 @@ #include "libavfilter/avfilter.h" #include "libavdevice/avdevice.h" #include "libavutil/avstring.h" +#include "libavcodec/eval.h" #include "cmdutils.h" #include "version.h" #ifdef CONFIG_NETWORK @@ -42,7 +43,7 @@ { char *tail; const char *error; - double d = strtod(numstr, &tail); + double d = av_strtod(numstr, &tail); if (*tail) error= "Expected number for %s but found: %s\n"; else if (d < min || d > max)
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod(). Actually i think this would be a great idea
I'll explore that possibility tomorrow. Good night. -- FFmpeg = Friendly and Funny Meaningless Portable Egregious Guru
On date Wednesday 2008-08-20 03:05:24 +0200, Stefano Sabatini encoded:
On date Wednesday 2008-08-20 01:43:17 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote: [...]
Michael:
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod().
I think this could be a very good idea, anyway maybe is still a good idea to export av_strtod(), skip review of the following patch otherwise. [...]
And finally I discovered that eval.h is not a public header, when we can still use it in cmdutils.c with: #include "libavcodec/eval.h" I think ff* tools shouldn't use internal functions as they are the implementation reference for libav*. On the other hand I find eval features quite useful per-se, so I think maybe it's arrived the time to export it, I volunteer to do all the required work if Michael and other maintainers agree. The plan could sound as follows: 1) move eval.[ch] to libavutil 2) s/ff_/av_/ in some function names 3) export eval.h interface and bump minor libavutil version Regards. -- FFmpeg = Foolish and Faboulous Meaningless Powered Encoding/decoding Genius
On Wed, Aug 20, 2008 at 06:49:01PM +0200, Stefano Sabatini wrote:
On date Wednesday 2008-08-20 03:05:24 +0200, Stefano Sabatini encoded:
On date Wednesday 2008-08-20 01:43:17 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote: [...]
Michael:
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod().
I think this could be a very good idea, anyway maybe is still a good idea to export av_strtod(), skip review of the following patch otherwise. [...]
And finally I discovered that eval.h is not a public header, when we can still use it in cmdutils.c with: #include "libavcodec/eval.h"
I think ff* tools shouldn't use internal functions as they are the implementation reference for libav*.
On the other hand I find eval features quite useful per-se, so I think maybe it's arrived the time to export it, I volunteer to do all the required work if Michael and other maintainers agree.
i agree in principle but it might take longer than normal to review because theres quite a bit i should review and should work on ...
The plan could sound as follows: 1) move eval.[ch] to libavutil 2) s/ff_/av_/ in some function names 3) export eval.h interface and bump minor libavutil version
yes, but i have to review eval API/ABI and think about it before it can be exported, i wont do this soon ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many things microsoft did are stupid, but not doing something just because microsoft did it is even more stupid. If everything ms did were stupid they would be bankrupt already.
On date Wednesday 2008-08-20 23:17:30 +0200, Michael Niedermayer encoded:
On Wed, Aug 20, 2008 at 06:49:01PM +0200, Stefano Sabatini wrote:
On date Wednesday 2008-08-20 03:05:24 +0200, Stefano Sabatini encoded:
On date Wednesday 2008-08-20 01:43:17 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote: [...]
Michael:
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod().
I think this could be a very good idea, anyway maybe is still a good idea to export av_strtod(), skip review of the following patch otherwise. [...]
And finally I discovered that eval.h is not a public header, when we can still use it in cmdutils.c with: #include "libavcodec/eval.h"
I think ff* tools shouldn't use internal functions as they are the implementation reference for libav*.
On the other hand I find eval features quite useful per-se, so I think maybe it's arrived the time to export it, I volunteer to do all the required work if Michael and other maintainers agree.
i agree in principle but it might take longer than normal to review because theres quite a bit i should review and should work on ...
The plan could sound as follows: 1) move eval.[ch] to libavutil 2) s/ff_/av_/ in some function names 3) export eval.h interface and bump minor libavutil version
yes, but i have to review eval API/ABI and think about it before it can be exported, i wont do this soon ... [...]
I see, I'll stay tuned meaningwhile... Regards. -- FFmpeg = Frightening and Frightening Mythic Patchable Exuberant Guru
On date Thursday 2008-08-21 00:04:54 +0200, Stefano Sabatini encoded:
On date Wednesday 2008-08-20 23:17:30 +0200, Michael Niedermayer encoded:
On Wed, Aug 20, 2008 at 06:49:01PM +0200, Stefano Sabatini wrote:
On date Wednesday 2008-08-20 03:05:24 +0200, Stefano Sabatini encoded:
On date Wednesday 2008-08-20 01:43:17 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote: [...]
Michael:
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod().
I think this could be a very good idea, anyway maybe is still a good idea to export av_strtod(), skip review of the following patch otherwise. [...]
And finally I discovered that eval.h is not a public header, when we can still use it in cmdutils.c with: #include "libavcodec/eval.h"
I think ff* tools shouldn't use internal functions as they are the implementation reference for libav*.
On the other hand I find eval features quite useful per-se, so I think maybe it's arrived the time to export it, I volunteer to do all the required work if Michael and other maintainers agree.
i agree in principle but it might take longer than normal to review because theres quite a bit i should review and should work on ...
The plan could sound as follows: 1) move eval.[ch] to libavutil 2) s/ff_/av_/ in some function names 3) export eval.h interface and bump minor libavutil version
yes, but i have to review eval API/ABI and think about it before it can be exported, i wont do this soon ... [...]
I see, I'll stay tuned meaningwhile...
I stayed tuned all the time, and now here it is an updated patch :-)). Regards. -- FFmpeg = Fascinating and Fast Mastering Puritan Elastic Gadget
On Sat, Jun 05, 2010 at 11:27:00PM +0200, Stefano Sabatini wrote:
On date Thursday 2008-08-21 00:04:54 +0200, Stefano Sabatini encoded:
On date Wednesday 2008-08-20 23:17:30 +0200, Michael Niedermayer encoded:
On Wed, Aug 20, 2008 at 06:49:01PM +0200, Stefano Sabatini wrote:
On date Wednesday 2008-08-20 03:05:24 +0200, Stefano Sabatini encoded:
On date Wednesday 2008-08-20 01:43:17 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote: [...]
Michael:
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod().
I think this could be a very good idea, anyway maybe is still a good idea to export av_strtod(), skip review of the following patch otherwise. [...]
And finally I discovered that eval.h is not a public header, when we can still use it in cmdutils.c with: #include "libavcodec/eval.h"
I think ff* tools shouldn't use internal functions as they are the implementation reference for libav*.
On the other hand I find eval features quite useful per-se, so I think maybe it's arrived the time to export it, I volunteer to do all the required work if Michael and other maintainers agree.
i agree in principle but it might take longer than normal to review because theres quite a bit i should review and should work on ...
The plan could sound as follows: 1) move eval.[ch] to libavutil 2) s/ff_/av_/ in some function names 3) export eval.h interface and bump minor libavutil version
yes, but i have to review eval API/ABI and think about it before it can be exported, i wont do this soon ... [...]
I see, I'll stay tuned meaningwhile...
I stayed tuned all the time, and now here it is an updated patch :-)).
Regards. -- FFmpeg = Fascinating and Fast Mastering Puritan Elastic Gadget
cmdutils.c | 8 ++++++-- doc/fftools-common-opts.texi | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-) 3396c5dc56bd6ce97edb26326316474ab72d431c 0001-Make-all-the-numeric-options-accepts-in-input-an-exp.patch
From 883c306338ce2f1b37ca04db4ec55f41345989fd Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-lala at poste.it> Date: Sat, 5 Jun 2010 23:22:38 +0200 Subject: [PATCH] Make all the numeric options accepts in input an expression which is evaluated through the eval API.
Fix roundup issue #551.
ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
On date Monday 2010-06-07 03:43:29 +0200, Michael Niedermayer encoded:
On Sat, Jun 05, 2010 at 11:27:00PM +0200, Stefano Sabatini wrote: [...]
cmdutils.c | 8 ++++++-- doc/fftools-common-opts.texi | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-) 3396c5dc56bd6ce97edb26326316474ab72d431c 0001-Make-all-the-numeric-options-accepts-in-input-an-exp.patch
From 883c306338ce2f1b37ca04db4ec55f41345989fd Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-lala at poste.it> Date: Sat, 5 Jun 2010 23:22:38 +0200 Subject: [PATCH] Make all the numeric options accepts in input an expression which is evaluated through the eval API.
Fix roundup issue #551.
ok
Now I see this introduces an usability regression, for example using -fs 4gi rather than -fs 4Gi the value obtained is 4, and the final "gi" is simply discarded. This because currently the eval API doesn't allow to set the tail to the parsed expression (which maybe should be addressed?). Anyway my fix is to simply use av_strtod(). Regards. -- FFmpeg = Free and Fascinating Most Power Extroverse God
On Wed, Jun 09, 2010 at 12:15:57AM +0200, Stefano Sabatini wrote:
On date Monday 2010-06-07 03:43:29 +0200, Michael Niedermayer encoded:
On Sat, Jun 05, 2010 at 11:27:00PM +0200, Stefano Sabatini wrote: [...]
cmdutils.c | 8 ++++++-- doc/fftools-common-opts.texi | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-) 3396c5dc56bd6ce97edb26326316474ab72d431c 0001-Make-all-the-numeric-options-accepts-in-input-an-exp.patch
From 883c306338ce2f1b37ca04db4ec55f41345989fd Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-lala at poste.it> Date: Sat, 5 Jun 2010 23:22:38 +0200 Subject: [PATCH] Make all the numeric options accepts in input an expression which is evaluated through the eval API.
Fix roundup issue #551.
ok
Now I see this introduces an usability regression, for example using -fs 4gi rather than -fs 4Gi the value obtained is 4, and the final "gi" is simply discarded. This because currently the eval API doesn't allow to set the tail to the parsed expression
(which maybe should be addressed?).
It should fail with an error message not return 4
Anyway my fix is to simply use av_strtod().
fine as well [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
... defining _GNU_SOURCE... For the love of all that is holy, and some that is not, don't do that. -- Luca & Mans
On date Wednesday 2010-06-09 03:55:51 +0200, Michael Niedermayer encoded:
On Wed, Jun 09, 2010 at 12:15:57AM +0200, Stefano Sabatini wrote:
On date Monday 2010-06-07 03:43:29 +0200, Michael Niedermayer encoded:
On Sat, Jun 05, 2010 at 11:27:00PM +0200, Stefano Sabatini wrote: [...]
cmdutils.c | 8 ++++++-- doc/fftools-common-opts.texi | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-) 3396c5dc56bd6ce97edb26326316474ab72d431c 0001-Make-all-the-numeric-options-accepts-in-input-an-exp.patch
From 883c306338ce2f1b37ca04db4ec55f41345989fd Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-lala at poste.it> Date: Sat, 5 Jun 2010 23:22:38 +0200 Subject: [PATCH] Make all the numeric options accepts in input an expression which is evaluated through the eval API.
Fix roundup issue #551.
ok
Now I see this introduces an usability regression, for example using -fs 4gi rather than -fs 4Gi the value obtained is 4, and the final "gi" is simply discarded. This because currently the eval API doesn't allow to set the tail to the parsed expression
(which maybe should be addressed?).
It should fail with an error message not return 4
Anyway my fix is to simply use av_strtod().
fine as well
Applied. -- FFmpeg = Fancy and Fanciful Meaningful Programmable Elastic Geisha
Michael Niedermayer wrote:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote:
+ * Values are expressed in + * bits, unless the 'B' postfix is used, in this case the values are + * assumed to be in bytes.
what?
I would read this as meaning "if a value does not have the 'B' suffix, it is assumed to be expressed in bits; if it does have that suffix, it is assumed to be expressed in bytes". The current phrasing seems relatively clear to me (even in my current mildly bleary state of mind), although I would say "in which case" instead of "in this case".
+ * @param tail if non-NULL put at this address the pointer to the + * character just after the part of the string successfully parsed
parse error
I interpret this as meaning something like "if the 'tail' parameter is not NULL, then after the function returns, dereferencing that pointer will give a pointer to the first character 'name' parameter which was not parsed into the return value". I'm not sure offhand of how to rephrase it for better clarity without getting inappropriately verbose, particularly since it reads fairly comprehensibly to me as it stands.
+ * @return the value of the parsed string as a numer of bits
numer!? bits?
"number", obviously - and I interpret this as meaning that the return value will always be expressed in bits, no matter what units the input string used for its number. -- The Wanderer Warning: Simply because I argue an issue does not mean I agree with any side of it. Secrecy is the beginning of tyranny.
On Tue, Aug 19, 2008 at 10:26:59PM -0400, The Wanderer wrote:
Michael Niedermayer wrote:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote:
+ * Values are expressed in + * bits, unless the 'B' postfix is used, in this case the values are + * assumed to be in bytes.
what?
I would read this as meaning "if a value does not have the 'B' suffix, it is assumed to be expressed in bits; if it does have that suffix, it is assumed to be expressed in bytes". The current phrasing seems relatively clear to me (even in my current mildly bleary state of mind), although I would say "in which case" instead of "in this case".
its not a issue of grammer, its a issue of (av_)strtod parsing a string and returning a double. Simply try to awnser if "3.141593" is in bits or bytes according to this description. It simply makes no sense, the number is not in bits nor in bytes. Nor does either of them have any meaning here [...]
+ * @return the value of the parsed string as a numer of bits
numer!? bits?
"number", obviously - and I interpret this as meaning that the return value will always be expressed in bits, no matter what units the input string used for its number.
double pi= av_strtod("3.1415927", NULL); is in units of bits? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
Michael Niedermayer wrote:
On Tue, Aug 19, 2008 at 10:26:59PM -0400, The Wanderer wrote:
Michael Niedermayer wrote:
what?
I would read this as meaning "if a value does not have the 'B' suffix, it is assumed to be expressed in bits; if it does have that suffix, it is assumed to be expressed in bytes". The current phrasing seems relatively clear to me (even in my current mildly bleary state of mind), although I would say "in which case" instead of "in this case".
its not a issue of grammer, its a issue of (av_)strtod parsing a string and returning a double. Simply try to awnser if "3.141593" is in bits or bytes according to this description. It simply makes no sense, the number is not in bits nor in bytes. Nor does either of them have any meaning here
Point. I didn't actually consider that aspect, or read the code closely (just the comments). If I had, I would have been inclined to assume that any value received which had a non-integer portion would be an indication of a bad input string. Under this scenario, the use of av_strtod would be simply because of the absence of any other known existing-and-approved way of translating a (portion of a) string to a numeric value.
numer!? bits?
"number", obviously - and I interpret this as meaning that the return value will always be expressed in bits, no matter what units the input string used for its number.
double pi= av_strtod("3.1415927", NULL); is in units of bits?
Either that or an error condition. Or at least you could interpret and define things that way - it could equally easily be exactly the kind of problem you're apparently considering it, in which case there's no point in my having spoken up in the first place. -- The Wanderer Warning: Simply because I argue an issue does not mean I agree with any side of it. Secrecy is the beginning of tyranny.
On date Wednesday 2008-08-20 19:15:25 -0400, The Wanderer encoded:
Michael Niedermayer wrote:
On Tue, Aug 19, 2008 at 10:26:59PM -0400, The Wanderer wrote:
Michael Niedermayer wrote:
what?
I would read this as meaning "if a value does not have the 'B' suffix, it is assumed to be expressed in bits; if it does have that suffix, it is assumed to be expressed in bytes". The current phrasing seems relatively clear to me (even in my current mildly bleary state of mind), although I would say "in which case" instead of "in this case".
its not a issue of grammer, its a issue of (av_)strtod parsing a string and returning a double. Simply try to awnser if "3.141593" is in bits or bytes according to this description. It simply makes no sense, the number is not in bits nor in bytes. Nor does either of them have any meaning here
Point. I didn't actually consider that aspect, or read the code closely (just the comments). If I had, I would have been inclined to assume that any value received which had a non-integer portion would be an indication of a bad input string. Under this scenario, the use of av_strtod would be simply because of the absence of any other known existing-and-approved way of translating a (portion of a) string to a numeric value.
numer!? bits?
"number", obviously - and I interpret this as meaning that the return value will always be expressed in bits, no matter what units the input string used for its number.
double pi= av_strtod("3.1415927", NULL); is in units of bits?
Either that or an error condition.
Or at least you could interpret and define things that way - it could equally easily be exactly the kind of problem you're apparently considering it, in which case there's no point in my having spoken up in the first place.
Well we can also treat B as a multiplier like the other ones (M, Mi, G, Gi, etc) with the only exception which it can be appended to the other ones (MB, MiB, GB, etc). So it makes sense to define for example: 3.1415927B = 3.1415927 * 8 although I practically can't see any example where this could make sense. Anyway I consider this patch frozen until when eval will be exported, which is a precondition for this patch. Regards. -- FFmpeg = Foolish and Forgiving MultiPurpose Evanescent Gospel
On date Wednesday 2008-08-20 01:43:17 +0200, Michael Niedermayer encoded:
On Mon, Aug 11, 2008 at 11:43:53PM +0200, Stefano Sabatini wrote: [...]
Index: cmdutils.c =================================================================== --- cmdutils.c (revision 14695) +++ cmdutils.c (working copy) @@ -29,6 +29,7 @@ #include "libavfilter/avfilter.h" #include "libavdevice/avdevice.h" #include "libavutil/avstring.h" +#include "libavcodec/eval.h" #include "cmdutils.h" #include "version.h" #ifdef CONFIG_NETWORK @@ -42,7 +43,7 @@ { char *tail; const char *error; - double d = strtod(numstr, &tail); + double d = av_strtod(numstr, &tail); if (*tail) error= "Expected number for %s but found: %s\n"; else if (d < min || d > max)
i wonder if it would make sense to just use the whole expression evaluator instead of av_strtod(). Actually i think this would be a great idea
Check it, I'm not very happy about the duplication of const_values and const_names which are taken from eval.c for consistency, suggestions are welcome. BTW what about to rename the function "parse_expr_or_die"? Regards. -- FFmpeg = Fantastic and Faboulous Magic Portable Evanescent Game
participants (3)
-
inverseparadox＠comcast.net -
michaelni＠gmx.at -
stefano.sabatini-lala＠poste.it