[FFmpeg-cvslog] Merge commit '85770f2a2651497861ed938efcd0df3696ff5e45'

[Stefano Sabatini]

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 at 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 at gmx.at>
> > > 
> > > > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ffb5a0d533498102c31aa131bc91a4cce868b0a8
> > > ---
> > 
> > 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avstring-add-definition-for-av_null_string.patch
Type: text/x-diff
Size: 1336 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-cvslog/attachments/20110502/82c585b5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-opt-add-logic-for-setting-NULL-and-default-string-va.patch
Type: text/x-diff
Size: 4654 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-cvslog/attachments/20110502/82c585b5/attachment-0001.bin>