Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'
ffmpeg | branch: master | Michael Niedermayer <michaelni@gmx.at> | Sun May 1 00:21:56 2011 +0200| [ffb5a0d533498102c31aa131bc91a4cce868b0a8] | committer: Michael Niedermayer Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45' * commit '85770f2a2651497861ed938efcd0df3696ff5e45': AVOptions: make default_val a union, as proposed in AVOption2. Move ff_dynarray_add to lavu and make it public. lavf: remove duplicate assignment in avformat_alloc_context. lavf: use designated initializers for AVClasses. options: simplify av_find_opt by using av_next_option. Merged-by: Michael Niedermayer <michaelni@gmx.at>
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c3...
On date Sunday 2011-05-01 00:27:06 +0200, Michael Niedermayer wrote:
ffmpeg | branch: master | Michael Niedermayer <michaelni@gmx.at> | Sun May 1 00:21:56 2011 +0200| [ffb5a0d533498102c31aa131bc91a4cce868b0a8] | committer: Michael Niedermayer
Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'
* commit '85770f2a2651497861ed938efcd0df3696ff5e45': AVOptions: make default_val a union, as proposed in AVOption2. Move ff_dynarray_add to lavu and make it public. lavf: remove duplicate assignment in avformat_alloc_context. lavf: use designated initializers for AVClasses. options: simplify av_find_opt by using av_next_option.
Merged-by: Michael Niedermayer <michaelni@gmx.at>
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c3...
Why did you remove .i64 and .q from the default_val union? For the rational it is more safe/logical not assume sizeof(double) >= sizeof(int64_t), for the second it may be convenient to express a default by using a rational rather than its double representation, for documentation/convenience purposes. Also I suppose we agreed that public API changes should be posted for review before being pushed.
On date Sunday 2011-05-01 01:01:30 +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 00:27:06 +0200, Michael Niedermayer wrote:
ffmpeg | branch: master | Michael Niedermayer <michaelni@gmx.at> | Sun May 1 00:21:56 2011 +0200| [ffb5a0d533498102c31aa131bc91a4cce868b0a8] | committer: Michael Niedermayer
Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'
* commit '85770f2a2651497861ed938efcd0df3696ff5e45': AVOptions: make default_val a union, as proposed in AVOption2. Move ff_dynarray_add to lavu and make it public. lavf: remove duplicate assignment in avformat_alloc_context. lavf: use designated initializers for AVClasses. options: simplify av_find_opt by using av_next_option.
Merged-by: Michael Niedermayer <michaelni@gmx.at>
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c3...
Why did you remove .i64 and .q from the default_val union?
For the rational it is more safe/logical not assume sizeof(double) >= sizeof(int64_t), for the second it may be convenient to express a default by using a rational rather than its double representation, for documentation/convenience purposes.
String default value setting doesn't work, that's because av_set_string3() doesn't accept NULL values for val, so for example: av_set_string3(obj, name, NULL, 1, NULL); will always return AVERROR(EINVAL). Here there is a problem since you can't represent a NULL value string with a string safely, you could set "" but that's not the very same thing. -- The best book on programming for the layman is "Alice in Wonderland"; but that's because it's the best book on anything for the layman.
On Sun, May 01, 2011 at 02:04:58AM +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 01:01:30 +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 00:27:06 +0200, Michael Niedermayer wrote:
ffmpeg | branch: master | Michael Niedermayer <michaelni@gmx.at> | Sun May 1 00:21:56 2011 +0200| [ffb5a0d533498102c31aa131bc91a4cce868b0a8] | committer: Michael Niedermayer
Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'
* commit '85770f2a2651497861ed938efcd0df3696ff5e45': AVOptions: make default_val a union, as proposed in AVOption2. Move ff_dynarray_add to lavu and make it public. lavf: remove duplicate assignment in avformat_alloc_context. lavf: use designated initializers for AVClasses. options: simplify av_find_opt by using av_next_option.
Merged-by: Michael Niedermayer <michaelni@gmx.at>
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c3...
Why did you remove .i64 and .q from the default_val union?
For the rational it is more safe/logical not assume sizeof(double) >= sizeof(int64_t), for the second it may be convenient to express a default by using a rational rather than its double representation, for documentation/convenience purposes.
String default value setting doesn't work, that's because av_set_string3() doesn't accept NULL values for val, so for example:
av_set_string3(obj, name, NULL, 1, NULL);
will always return AVERROR(EINVAL).
Here there is a problem since you can't represent a NULL value string with a string safely, you could set "" but that's not the very same thing.
But you could define a valid string that FFmpeg treats always as NULL. e.g. avcodec/string.h extern const char * av_null_string; avutil/string.c const char *av_null_string = "(null)"; av_set_string(..) if (str == av_null_string) behave this way -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
On date Sunday 2011-05-01 10:31:12 +1000, Peter Ross wrote:
On Sun, May 01, 2011 at 02:04:58AM +0200, Stefano Sabatini wrote: [...]
String default value setting doesn't work, that's because av_set_string3() doesn't accept NULL values for val, so for example:
av_set_string3(obj, name, NULL, 1, NULL);
will always return AVERROR(EINVAL).
Here there is a problem since you can't represent a NULL value string with a string safely, you could set "" but that's not the very same thing.
But you could define a valid string that FFmpeg treats always as NULL.
e.g.
avcodec/string.h extern const char * av_null_string;
avutil/string.c const char *av_null_string = "(null)";
av_set_string(..)
if (str == av_null_string)
behave this way
That's possible, an alternative could be to make av_set_string() accept the NULL vale for string options. But this is only part of the problem, when you set options for the first time you have random values in the context, so you need a function for initing the context, that is av_opt_set_defaults2() is not anymore sufficient, you need at least to pass a flag to it for telling that it has to init the context (and ignore the already present string fubar values).
On Sun, May 01, 2011 at 12:01:35PM +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 10:31:12 +1000, Peter Ross wrote:
On Sun, May 01, 2011 at 02:04:58AM +0200, Stefano Sabatini wrote: [...]
String default value setting doesn't work, that's because av_set_string3() doesn't accept NULL values for val, so for example:
av_set_string3(obj, name, NULL, 1, NULL);
will always return AVERROR(EINVAL).
Here there is a problem since you can't represent a NULL value string with a string safely, you could set "" but that's not the very same thing.
But you could define a valid string that FFmpeg treats always as NULL.
e.g.
avcodec/string.h extern const char * av_null_string;
avutil/string.c const char *av_null_string = "(null)";
av_set_string(..)
if (str == av_null_string)
behave this way
That's possible, an alternative could be to make av_set_string() accept the NULL vale for string options.
already done locally
But this is only part of the problem, when you set options for the first time you have random values in the context, so you need a function for initing the context, that is av_opt_set_defaults2() is not anymore sufficient, you need at least to pass a flag to it for telling that it has to init the context (and ignore the already present string fubar values).
contexts should be allocated with av_mallocz() or if they are on the stack they should be memset(0). [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The misfortune of the wise is better than the prosperity of the fool. -- Epicurus
On Sun, May 01, 2011 at 01:01:30AM +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 00:27:06 +0200, Michael Niedermayer wrote:
ffmpeg | branch: master | Michael Niedermayer <michaelni@gmx.at> | Sun May 1 00:21:56 2011 +0200| [ffb5a0d533498102c31aa131bc91a4cce868b0a8] | committer: Michael Niedermayer
Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'
* commit '85770f2a2651497861ed938efcd0df3696ff5e45': AVOptions: make default_val a union, as proposed in AVOption2. Move ff_dynarray_add to lavu and make it public. lavf: remove duplicate assignment in avformat_alloc_context. lavf: use designated initializers for AVClasses. options: simplify av_find_opt by using av_next_option.
Merged-by: Michael Niedermayer <michaelni@gmx.at>
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c3...
Why did you remove .i64 and .q from the default_val union?
short awnser: because it didnt work, it broke fate. more elaborately, i think its a bad idea to have 3 different ways to store a real-scalar value. It leads to problems/confusion and complex code. The problem is quite real (fate failing) and fixing that would lead to complex code with IMHO little solid advantage Its also more error prone as its easy to set the wrong one in the table and theres nothing that would point at that mistake.
For the rational it is more safe/logical not assume sizeof(double) >= sizeof(int64_t), for the second it may be convenient to express a default by using a rational rather than its double representation, for documentation/convenience purposes.
for documentation/convenience one can write dbl=5.0/3.0 in the table
Also I suppose we agreed that public API changes should be posted for review before being pushed.
yes, but this did not seem controversal to me. Adding a string value in addition to double is something that was discussed and something everyone seems to have agreed to be a good idea. But let me ask differently, what should i change on the code? if nothing then IMHO skiping the patch sending wasnt that wrong and if theres something, please tell me, ill fix it. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
On date Sunday 2011-05-01 22:08:29 +0200, Michael Niedermayer wrote:
On Sun, May 01, 2011 at 01:01:30AM +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 00:27:06 +0200, Michael Niedermayer wrote:
ffmpeg | branch: master | Michael Niedermayer <michaelni@gmx.at> | Sun May 1 00:21:56 2011 +0200| [ffb5a0d533498102c31aa131bc91a4cce868b0a8] | committer: Michael Niedermayer
Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'
* commit '85770f2a2651497861ed938efcd0df3696ff5e45': AVOptions: make default_val a union, as proposed in AVOption2. Move ff_dynarray_add to lavu and make it public. lavf: remove duplicate assignment in avformat_alloc_context. lavf: use designated initializers for AVClasses. options: simplify av_find_opt by using av_next_option.
Merged-by: Michael Niedermayer <michaelni@gmx.at>
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c3...
Why did you remove .i64 and .q from the default_val union?
short awnser: because it didnt work, it broke fate.
more elaborately, i think its a bad idea to have 3 different ways to store a real-scalar value. It leads to problems/confusion and complex code. The problem is quite real (fate failing) and fixing that would lead to complex code with IMHO little solid advantage Its also more error prone as its easy to set the wrong one in the table and theres nothing that would point at that mistake.
Can you say where the fate problem exactly was?
For the rational it is more safe/logical not assume sizeof(double) >= sizeof(int64_t), for the second it may be convenient to express a default by using a rational rather than its double representation, for documentation/convenience purposes.
for documentation/convenience one can write dbl=5.0/3.0 in the table
yes, although there is an approximation error but maybe this can be safely ignored
Also I suppose we agreed that public API changes should be posted for review before being pushed.
yes, but this did not seem controversal to me.
But there was an RFC and someone else was working on that (Anton), in this case letting the original contributor do the work (in case he wants) is IMO a good norm, in doubt better to choose the safest option and publish the patch anyway (eventually telling: i'll push soon if I see no objections...).
Adding a string value in addition to double is something that was discussed and something everyone seems to have agreed to be a good idea.
But let me ask differently, what should i change on the code? if nothing then IMHO skiping the patch sending wasnt that wrong and if theres something, please tell me, ill fix it.
I have a bad feeling about the int64/double issue, it could affect portability (but I need to check better the code), also I'd like to fix default string values setting. WIP patches attached (just as a reference, the second one can be possibly simplified, and not tested), and I'm not sure I like the av_null_string approach. -- no brainer: A decision which, viewed through the retrospectoscope, is "obvious" to those who failed to make it originally.
On Mon, May 02, 2011 at 12:19:31AM +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 22:08:29 +0200, Michael Niedermayer wrote:
On Sun, May 01, 2011 at 01:01:30AM +0200, Stefano Sabatini wrote:
On date Sunday 2011-05-01 00:27:06 +0200, Michael Niedermayer wrote:
ffmpeg | branch: master | Michael Niedermayer <michaelni@gmx.at> | Sun May 1 00:21:56 2011 +0200| [ffb5a0d533498102c31aa131bc91a4cce868b0a8] | committer: Michael Niedermayer
Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'
* commit '85770f2a2651497861ed938efcd0df3696ff5e45': AVOptions: make default_val a union, as proposed in AVOption2. Move ff_dynarray_add to lavu and make it public. lavf: remove duplicate assignment in avformat_alloc_context. lavf: use designated initializers for AVClasses. options: simplify av_find_opt by using av_next_option.
Merged-by: Michael Niedermayer <michaelni@gmx.at>
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c3...
Why did you remove .i64 and .q from the default_val union?
short awnser: because it didnt work, it broke fate.
more elaborately, i think its a bad idea to have 3 different ways to store a real-scalar value. It leads to problems/confusion and complex code. The problem is quite real (fate failing) and fixing that would lead to complex code with IMHO little solid advantage Its also more error prone as its easy to set the wrong one in the table and theres nothing that would point at that mistake.
Can you say where the fate problem exactly was?
I dont know exactly, i saw uses of i think dbl where it was not certain the type actually was double. also the tables might have contained wrong types. It seemed best to throw the other types out instead of debuging this further
For the rational it is more safe/logical not assume sizeof(double) >= sizeof(int64_t), for the second it may be convenient to express a default by using a rational rather than its double representation, for documentation/convenience purposes.
for documentation/convenience one can write dbl=5.0/3.0 in the table
yes, although there is an approximation error but maybe this can be safely ignored
yes
Also I suppose we agreed that public API changes should be posted for review before being pushed.
yes, but this did not seem controversal to me.
But there was an RFC and someone else was working on that (Anton), in this case letting the original contributor do the work (in case he wants) is IMO a good norm, in doubt better to choose the safest option and publish the patch anyway (eventually telling: i'll push soon if I see no objections...).
I dont remember a RFC, But as you mention it now i found it on libav-devel, well if people want me to read something please post it to ffmpeg-devel or CC me directly. I did know anton was planing to work on it the day before from IRC and i wanted to implement it before him because he said it needs deep changes in avoptions and i knew that i could implement it without deep changes. It was unfortunate that i couldnt finish it before anton started. And thus some work was duplicated.
Adding a string value in addition to double is something that was discussed and something everyone seems to have agreed to be a good idea.
But let me ask differently, what should i change on the code? if nothing then IMHO skiping the patch sending wasnt that wrong and if theres something, please tell me, ill fix it.
I have a bad feeling about the int64/double issue, it could affect portability (but I need to check better the code), also I'd like to fix default string values setting.
WIP patches attached (just as a reference, the second one can be possibly simplified, and not tested), and I'm not sure I like the av_null_string approach.
I think simply storing NULL when NULL is meant is simpler [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
participants (4)
-
git＠videolan.org -
Michael Niedermayer -
Peter Ross -
Stefano Sabatini