[FFmpeg-devel] [PATCH/RFC] Per-codec option system

[Michael Niedermayer]

On Thu, Oct 01, 2009 at 11:30:41AM +0200, Robert Swain wrote:
> 2009/9/30 Michael Niedermayer <michaelni at gmx.at>:
> > On Wed, Sep 30, 2009 at 10:02:02AM +0200, Robert Swain wrote:
> >> 2009/9/30 Jason Garrett-Glaser <darkshikari at gmail.com>:
> >> >> * parameters that are semantically identical between a pair of codecs,
> >> >> ?must use the same parameter name, they also must use a compatible
> >> >> ?parameter value system. That makes some sense because, well, its nice if
> >> >> ?users dont have to releaern a new system for each codec in ffmpeg.
> >> >
> >> > IMO this should be handled internally to codecs, not by literally
> >> > sharing the parameters. ?In other words, it should be a policy: codecs
> >> > should use the same name for parameters when it makes sense. ?It
> >> > should be enforced by rules, not code.
> >>
> >> I agree that it makes sense for semantically identical parameters to
> >> use the same name, but I definitely agree with Jason that it does not
> >> need to be the same declared variable. Items with the same semantics
> >> can have different types. Sometimes items may share a (sane) name but
> >> have different semantics and so should have different descriptions of
> >> the variables.
> >
> > i think some example of this could help the discussion? do you maybe have
> > one where we would want different types?
> 
> Aside from what others have mentioned:
> 
> '-me_range          <int>   E.V.. limit motion vectors range (1023 for
> DivX player)'
> 
> This variable has different meaning for x264:
> 
> '--merange <integer>     Maximum motion vector search range [16]'
> 
> The quality option was another of which I was thinking. 

./ffmpeg -some_parameter x
should have the same semantic meaning no matter which codec unless its clear
from the "some_parameter" that it is specific to a codec like
-codec_internal_quantization_parameter (yeah its too long, i exagerated
   it for clarity in that example)
its perfectly fine if we have a -x264_merange or whichever else name but
-me_range for mpeg1 encoding should mean the same thing that it does for
h264 encoding. I hope you agree that having options change their semantic
meaning based on other options like -vcodec without that being clear from
their name is not an improvment.


> And then
> things which are flags which get extended to have multiple levels like
> trellis and so on.

my grep has only one match for trell in avcodec.h and that is an int
maybe you think of the old API but that was changed for all codecs.


[...]
> >> their types may not be sufficient for reuse in new codecs and they
> >
> > types can be changed ...
> 
> Indeed. But, for example, when adding new codecs which have
> multiple-level (e.g. 0 - off, 1 - low, 2 - medium, 3 - high ...)
> variables versus existing parameters that are flags, changing the type
> of the variable means API breakage unless one keeps both methods in
> place and translates one to the other.

yes


> If the variables are defined
> within the codec scope, it could be added without having to alter the
> variable type for other codecs and so no breakage would occur.

well, yes but you now need to specify the on/off[low/high] using 2 different
ways depending on the codec. As a user just using 1 codecs its of course no
issue but an application using ffmpeg or some human using 2 codecs would
need to use the correct parameter depending on the codec. I do think this
could become very hard if the number of such parmeters where large.
One can remember flags +TRELL (ffmpeg native) and -trellis 5 (x264) but
if there are 100 such options remembering which to set for which encoder
is likely not too funny


> 
> >> would then either need a new variant option or need to be moved out of
> >> AVCC, that their ranges may differ per codec and so on. I _much_
> >> prefer codecs doing their own option parsing of strings.
> >
> > i already asked jason, so let me ask you too, do you have some arguments
> > in favor of strings being parsed by duplicated parsing code in codecs
> > and inevitably incompatible option value systems?
> 
> The code need not be duplicated. The string parsing could be coded
> such that it can be shared between all codecs.
> 

> Passing a string simplifies the API. Rather than the external caller
> requiring lots of lines of assignment to individual API variables, one
> can create a single string and pass the pointer to it. This seems to
> work very neatly for x264, it seems to be very flexible and so it
> seems like a good idea to me.

i use(d) that system in my postprocessing code, it didnt work out nicely
after i skiped over pretending that it did. The first issue was that this
practically never integrates cleanly into a user application, user apps have
GUI interfaces that have buttons and sliders not a text field for a string,
command line tools tend to have their own parameter parsing code that
already seperates each parameter so keeping several parameters as a single
string looks like a hack.

But above all, the AVOptions system can handle your single string as well
you only need to add a function into libavcodec/opt.c/h that breaks a string
up by a given seperator and call av_set_string3() for each pair.


> 
> What do you mean by inevitably incompatible? Variables with the same
> meaning but lots of different names? That shouldn't happen if we're
> anything like as strict as we have been for variables under the
> current regime.

thats what ive meant approximately, yes


> 
> >> >> * as a consequence of above it also must be possible to transparently move
> >> >> ?a name/value parameter into AVCodecContext, this can be done by extending
> >> >> ?AVOptions.
> >>
> >> Why is this necessary?
> >
> > it follows from the other requirements that you seem not to understand
> 
> * so what did you mean?
> 
[...]
> >> >> * If there exists an established parameter name/value for something like
> >> >> ?a paremeter in lame or x264 then these should in addition to the standard
> >> >> ?system in ffmpeg be supported.
> >> >
> >> > So you want to have two redundant systems for passing options to an
> >> > encoder, each with different naming schemes? ?This is a usability
> >> > disaster.
> >>
> >> I hope that a sane new string parsing method (or similar) will
> >> supersede the current system which is difficult to learn (CLI options
> >> are order dependent which is unusual and confusing) and has a bad
> >> policy with regard to what it does automatically and what the default
> >> behaviour is. The default behaviour should be to produce good quality
> >> output for all codecs, to negotiate mismatches between input and
> >> output (frame rate, sample rate, resolution, pixel format, sample
> >> format, channels, etc.) unless no sane solution can be found or unless
> >> the user specifies otherwise. It should of course inform the user of
> >> what it is doing in case it is not desired, and it should try not to
> >> do anything if reasonably possible (e.g. input is 44.1kHz, output
> >> needs 11025Hz/22050Hz, downsample to 22050Hz; output needs 44.1Khz do
> >> nothing).
> >
> > You throw a huge number of totally unrelated things you dont like into
> > the same pot, but let me say i fear there is no "heal all ill" new NIH
> > syndrom rewritten parser that will be better. Bugs have to be fixed
> > not once every 6 month a rewrite be done that ends up with new bugs
> > that people refuse to work on.
> 
> Of course. Is it really once every six months that a different rewrite
> is proposed though? Or is it rather that there is distaste for the
> current method, no significant change has been enacted and so once
> every six months the same problem rears its head for another round of
> discussion?
> 
> You don't need to answer that, I completely agree that there's no
> point reinventing the wheel repeatedly when no one is bothered to work
> on improving just one idea to a usable state.

as a simple suggestion, why dont you write a function that takes a
single string, breaks it appart and calls av_set_string3() for each?
If you think this is usefull of course ...


[...]
> > The defaults have historically been "everything disabled" for encoders,
> > I cant see how that is related to have fields in a struct or strings
> > parsed by codecs.
> 
> It isn't, but I would request a policy change if this is considered policy.

iam not against it at all but that should be discussed in a seperate thread,
also we do need a volunteer to actually implement it otherwise we would end
up with quite abit of inconsistent behavior if the old code would not be
updated ...


> 
> > automatically selecting good fps, sample rate, ... is difficult and wont
> > happen through a parameter parser rewrite. I suggested many times methods
> > to improve these but no single person gave a damn, so i assume you want to
> > work on this now or is it just bickering?
> 
> Do you have any links to or pointers to find your suggestions? I would
> like to see them as I expect your suggestions are good. I apologise if
> I did not see them or pick up on them in the past.

IIRC it was just a few words here and there when it was on topic, probably
in replies to bugs ...

the basic problem with fps can be summarized in that the choosing of the
framerate/timebase for the muxer is seperated in 2 steps
av_find_stream_info() that knows the timestamps of the input but has no
knowledge of what timebases or framerates the output can handle or which
would be painfull (in terms of overhead like for avi) for the output
and the second part, that is the user app that has the output from
av_find_stream_info() but no longer sees the actual timestamps

maybe the problem is clearer with an example
when muxing to avi, each timestamp that is skiped requires an empty
chunk of 8 bytes thus we prefer a 1/30 timebase over a 1/120
but mixed fps content like 24fps + 30fps can be more accurately be
represented by a 1/120 timebase, so in containers where that doesnt
cost us anything (like mov) we certainly would prefer it but in
avi 1/30 is likely better, av_find_stream_info() cant decide and the
user app afterwards also cant each lacks half of the information needed

then for encoding into mpeg1 only some fps values are allowed, but
av_find_stream_info() does not know if mpeg1 encoding will follow or
h264 or anything else so it cannot consider that, and the user app
after av_find_stream_info() just sees a guessed timebase value ...


[...]
> > also id like to mention that we do have a supported_samplerates array in
> > AVCodec, i assume you want to add code to use that?
> 
> I want some sample rate conversion code that works for any number of
> channels and that has SIMD for cases that can be optimised. 

> Why isn't
> there single channel resampling that can be iterated for N channels?
> Or is there but it's just not hooked up in ffmpeg.c?

I thought that our resampling code could handle N channls


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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20091001/f226b87c/attachment.pgp>