[FFmpeg-devel] [RFC] Fixing libx264.c

[Robert Swain]

2009/5/22 Michael Niedermayer <michaelni at gmx.at>:
> On Fri, May 22, 2009 at 09:55:59AM +0000, Loren Merritt wrote:
>> On Fri, 22 May 2009, Michael Niedermayer wrote:
>>> On Thu, May 21, 2009 at 06:57:41PM -0700, Jason Garrett-Glaser wrote:
>>>
>>>> 3. ?ffmpeg sets directpred to "temporal".
>>>> Why it's broken currently: directpred temporal breaks horrifically
>>>> when b-pyramid is applied, so it's easy to mix the two improperly
>>>> (spatial is a better default anyways, and is the one x264 uses).
>>>> Solution: set the default to spatial in options.c (which it should be).
>>>
>>> its a little annying semantically as mpeg4 asp has no spatial and thus
>>> strictly mpeg4 should fail with the default.
>>> maybe none or auto would be better defaults.
>>>
>>> though i have no real objection to change it to spatial if you want, its
>>> just a little dirty for codecs that dont support spatial
>>
>> Is that an official ffmpeg policy: codecs should refuse to encode if you
>> enable any option they don't support?
>
> no, its not official policy
>
> its just that a user asking explicitly for some feature that a encoder doesnt
> support, should receive an error message. That being something occasionally
> requested ...

Then one needs to know whether the option has been set by the user or
internally as a default value. I'm definitely pro command line
validation.

>> Every time anyone adds a new option
>> for one codec, they have to modify all the other codecs to check it too?
>
> no, this certainly was not my intent.
> Rather AVCodecs could have a list of supported options and leave the checking
> to some common code ...

This sounds good and very similar to Baptiste's AVOption 'list' per
codec (IIRC) proposal that would have been a possible per codec
default solution.

>> Should intra-only codecs refuse to encode with the default gop_size of 12?
>> Should codecs that don't have quantizers refuse to encode if qmin/qmax are
>> set to anything at all?
>>
>> Surely the only sane thing to do is to ignore all options that are
>> irrelevant to the format being encoded. mpeg4 doesn't have any choice of
>> direct modes, so the direct mode option shouldn't matter.
>>
>> With the common mpegvideo codepath we do have to check that options used by
>> some mpeglike codecs but not the one in use aren't enabled. But once we're
>> checking that, is it better to error out or to disable the option?
>
> There are a varity of constraining points here
> A The user should receive an error if she requests nonsensical options like
> ?an explicit gop size or b frames with intra only codecs

This would be nice.

> B Variables should not be set to semantically incorrect values, its a receipe
> ?for bugs and maintaince issues
> C Variables that are used (mpegvideo common code) must not be set to incorrect
> ?values
> D code should of course be simple & readable & small & fast ...
>
> A & B are not really achived currently, C & D are
>
> and again, i dont mind the global default for directpred to be set to spatial
> i just think its not a pretty solution and might byte us in the future (when
> there is comon code using directpred for example)

This whole thread is mostly an extension of C by interpreting
'incorrect' subjectively and demanding that default values produce
both compatible output and have some acceptable speed/quality trade
off. If this is considered, C is not met.

Regards,
Rob