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

[Robert Swain]

2008/6/7 Michael Niedermayer <michaelni at gmx.at>:
> 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"

It felt vague when I wrote it and I was trying to formulate something
more convincing and couldn't. So, I guess that's my response. :)

> 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
>
> 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
>
> mplayer also supports user editable configuration files and alot of
> people use it and are happy with it.
> I really think you guys are going in the completely wrong direction
> here
> adding code duplication to _avoid_ supporting a usefull feature.

I don't really mind either way. Having per codec defaults and presets
is the main thing and your code will cater for both but automatically
loading the 'default' preset if available would definitely be
appreciated.

I challenge you to find the bug in my code that causes that weird
error regarding freeing a non-aligned pointer. My debugging skills are
not good enough but I tried valgrind and gdb on it and couldn't get
anything more than that av_freep was called on something and that's
where the error occurred. I couldn't find out either where this
av_freep call was located in the code or what pointer it had been
called upon. I have a hunch it's AVCodec related as that's what I
messed with in the code but I don't really know. :/

Thanks,
Rob