[FFmpeg-devel] [PATCH] add ME_T/ESA to avcodec.h

[Baptiste Coudurier]

Hi,

Michael Niedermayer wrote:
> On Fri, Jun 06, 2008 at 01:49:04AM +0100, Robert Swain wrote:
>> 2008/6/5 Michael Niedermayer <michaelni at gmx.at>:
>>> On Thu, Jun 05, 2008 at 10:27:46AM -0700, Baptiste Coudurier wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Thu, Jun 05, 2008 at 09:42:06AM +0100, Robert Swain wrote:
>>>>>> 2008/6/5 Michael Niedermayer <michaelni at gmx.at>:
>>>>>>> On Wed, Jun 04, 2008 at 11:55:23PM +0100, Robert Swain wrote:
>>>>>>>> On 4 Jun 2008, at 21:56, Baptiste Coudurier wrote:
>>>>>>>>> Michael Niedermayer wrote:
>>>>>>>>>> Me too, ive a half finished per codec defaults change locally ...
>>>>>>>>> Awesome !
>>>>>>>> I made some patches for this that didn't quite work that used the
>>>>>>>> 'wrong' approach. Baptiste said they should use AVOption instead but I
>>>>>>>> was unaware of this API at the time and by the time I'd finished
>>>>>>>> writing it only to be told it would need rewriting, I didn't have the
>>>>>>>> motivation to fix it up. :)
>>>>>>>>
>>>>>>>> If you want to look at what I did, they are patches 0001-0004* here:
>>>>>>>>
>>>>>>>> http://www.swains.plus.com/superdump/ffmpeg/patches/
>>>>>>>>
>>>>>>>> I would appreciate if you made what you've done available too as I may
>>>>>>>> have some criticisms! :)
>>>>>>> Code below,
>>>>>>> just put -vpre anime-hq on the command line and have a file with the path
>>>>>>> ~/.ffmpeg/mpeg4-anime-hq.ffpreset
>>>>>>> with all your AVOptoion key=value stuff in it
>>>>>>> similar for other codecs.
>>>>>>>
>>>>>>> minor known bug, -vpre must be after -vcodec
>>>>>>>
>>>>>>> Iam planning to commit the code if there are no objections ...
>>>>>> This is presets code, not per codec defaults, but it's good. :)
>>>>> Well, defaults are one preset IMHO
>>>>> It would be easy just to do the equivalent of '-vpre default' when no
>>>>> other -vpre is specified.
>>>>> I suspect that would be much simpler than your code, also defaults would
>>>>> be in a user editable file and not bloat the library itself ...
>>>>>
>>>> I had the idea that some codecs would need to override default
>>>> AVOptions, more specifically min and max ranges, like libx264 or dnxhd
>>>> for qmin/qmax.
>>>>
>>>> For example DNxHD supports qmax up to 1024, and FFmpeg cannot use this
>>>> possibility atm (needed to encode /dev/random for example), should I add
>>>> bump qmax max value to 1024 ?
>>> yes
>>>
>>>
>>>> We might add a mechanism where codecs could override ranges and default
>>>> value.
>>> yes ...
>>>
>>> but ranges depend on more than just the codec, they also depend on other
>>> values, not every combination is always allowed.
>>>
>>> Simply adding a AVCodec.check_values() could check all that ...
>>> the presets suggested by me could handle defaults ...
>> We (M?ns, Baptiste and myself) seem to agree that hardcoding default
>> values as well as the separate issue of extending to handle hardcoded
>> ranges and whatever else people think might be useful is preferable to
>> user-editable defaults. I think hardcoding is better because it avoids
>> any potential issues with users having altered the default values and
>> blaming FFmpeg as a consequence. 
> 
> Quite vague argument, i now could try to guess what you mean exactly and
> reply but i think it would be better if you would elaborate on
> "any potential issues"
> 
> Also a user can always edit the source, and i wouldnt bet that distros
> and people compiling windows binaries for the masses wont do it ...
> Its a lot easier to ask a user to post the default-x264.ffpreset file
> or check that it matches svn in these cases

Considering problems we have requesting samples/gdb/commandline, I don't
think that asking yet another file is a good solution.

> Besides if defaults would be in a editable file, it would become part of
> bugreports.html to provide that file if it differs from svn and of faq
> to check that file first, we could even print its content with a sufficient
> verbose level and ask peopl to use that in bugreports

Same point as before.

> mplayer also supports user editable configuration files and alot of
> people use it and are happy with it.

Yes, and this is a good thing.

> I really think you guys are going in the completely wrong direction
> here
> adding code duplication to _avoid_ supporting a usefull feature.> 

Well, I don't think Robert really explained correctly what I meant.
There are 2 separate issues:

1) replacing -target dvd/svcd/whatever (adding ipod), which VERY WELL
fits in presets in homedir

2) adding a way to override AVOptions range for qmax etc.. which needs
to be hardcoded because user has no business saying that qmax max value
for mpeg-2 is 7821637. Mainly because bumping default max value to 1024
would require every codec supporting qmax to check if value is valid.

That's what I think.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA