[FFmpeg-devel] Extending AVOption system

[Michael Niedermayer]

On Thu, Jun 12, 2008 at 12:12:10AM +0200, Stefano Sabatini wrote:
> On date Monday 2008-06-09 19:23:49 +0200, Michael Niedermayer encoded:
> > On Mon, Jun 09, 2008 at 06:16:25PM +0200, Stefano Sabatini wrote:
> > > On date Monday 2008-06-09 17:06:55 +0200, Michael Niedermayer encoded:
> > > > On Mon, Jun 09, 2008 at 04:45:05PM +0200, Stefano Sabatini wrote:
> > > > > On date Sunday 2008-06-08 21:17:51 +0200, Michael Niedermayer encoded:
> > > > > > On Sun, Jun 08, 2008 at 07:53:07PM +0200, Stefano Sabatini wrote:
> > > > > > > Michael, eventual applications which work with non sorted arrays
> > > > > > > won't work anymore with bsearch(), so I have to provide all the required
> > > > > > > ifdeffery to keep backward compatibility, right?
> > > > > > 
> > > > > > hmm, who is using AVOptions arrays outside lav* ?
> > > > > 
> > > > > Who can say? Anyway since we're indeed breaking backward compatibility
> > > > > a major bump seems anyway the right thing to do (again: I can also
> > > > > live without such a major bump if you prefer like this, just making my
> > > > > point clear).
> > > > > 
> > > > > Rethinking at it the major bump would be required in libavutil, since
> > > > > we're requesting at some point these two things:
> > > > > 1) AVClass.option array has to be sorted
> > > > > 2) AVClass.option_count has to be correctly specified
> > > > > 
> > > > > If this is not true then the behaviour of the function which deal with
> > > > > AVClass is undefined.
> > > > 
> > > > Just store the length in the first entry of the AVOption array. No change
> > > > to AVClass no version bumps. And the length is where the corresponding array
> > > > is.
> > > > {" len", NULL, <array size>, FF_OPT_TYPE_CONST,
> > > 
> > > And still so we're requesting that:
> > > 1) AVClass.option array has to be sorted
> > > 2) AVClass.option_count has to contain a "len" option
> 
> mmh, it was:
> 2) AVClass.option has to contain a "len" option
> 
> > > With this option we save a minor bump for the introduction of
> > > option_count, not the major bump, also we're complicating the
> > > interface.
> > 
> > 1. AVClass.option[0].name == " len" -> array is sorted and option[0].offset
> >    contains the length
> > 2. AVClass.option[0].name != " len" -> array is not sorted and is NULL
> >    terminated
> > 
> > 2 is under #ifdef version < blah
> 
> Sorry to bother again, I'm not really happy with this solution too...
> 
> This still breaks compatibility in a very subtle way, suppose that a
> funky user from Mars already uses AVOption and has an AVClass with the
> first item set to
> 
> { " len", 42, ...}

This is ridiculous, command line options dont start with spaces
even less so ones using AVOption.


> 
> when 42 is not equal to sizeof(options)/sizeof(AVOptions) -1 and
> furthermore the array isn't sorted, yes you may wonder why someone
> should do such thing but you know people from mars do very weird
> things...
> 
> Even if possibly there aren't users affected by this problem, we're
> still complicating the API, what's even worst we can't define the size of
> the array in the options array itself like this:
> 
> static AVOption options[]={
>  {" len", NULL, sizeof(options)/sizoef(AVOption),
>  ...
> }
> 
> since it issues an incomplete type error.

{" len", NULL, 123,

will not


> 
> My solution is to set in the item the offset to -1 meaning
> "unspecified", then perform a sort of lazy initialization when
> performing the first av_opt_find():
[...]
> which requires also to eliminate the constness of both:

rejected


> 
>     const struct AVOption *option;
> 
> in log.h:AVClass and of:
> 
> static const AVOption options[]={
> 
> in the various utils.c.
> 

> For this I continue to prefer the solution contemplating a major bump.

We will not bump the major version so people can set
bt=b in the preset files

we could put the whole new code under #if version >= X though

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080612/957c28a6/attachment.pgp>