[FFmpeg-cvslog] r13685 - trunk/ffmpeg.c

[Baptiste Coudurier]

Hi,

Michael Niedermayer wrote:
> On Thu, Jun 19, 2008 at 12:56:45PM +0100, Robert Swain wrote:
>> 2008/6/17 Michael Niedermayer <michaelni at gmx.at>:
>>> On Tue, Jun 17, 2008 at 04:00:47AM +0200, Michael Niedermayer wrote:
>>>> On Tue, Jun 17, 2008 at 02:28:03AM +0100, Robert Swain wrote:
>>>>> 2008/6/8 Robert Swain <robert.swain at gmail.com>:
>>>>>> 2008/6/7 Baptiste Coudurier <baptiste.coudurier at smartjog.com>:
>>>>>>> Any volunteer to purge -target and puts svcd/vcd/dvd.ffpreset,
>>>>>>> and add ipod/iphone.ffpreset ?
>>>>> Removing -target may be more difficult as it doesn't just deal with
>>>>> codec options...
>>>>>
>>>>>>> I think ipod is definitely wanted to produce h264 files playable :>
>>>>>> I intend to work on this as well as PSP, MPEG profiles/levels and
>>>>>> probably some libx264 quality preset stuff too. How do we want to
>>>>>> manage the presets? Dump them in a presets subdir of FFmpeg trunk or
>>>>>> something? Then how will we install them? Wouldn't it be better to
>>>>>> have them installed to /usr/share or something but read files from
>>>>>> ~/.ffmpeg preferentially?
>>>>> I've encountered a problem somewhere but I'm not quite sure where. At
>>>>> the moment I'm suspecting opt_default but that's as far as I've got.
>>>>> If I have a line in a preset file that reads (without the quotes):
>>>>> 'rc_eq=blurCplx^(1-qComp)' opt_preset parses it from the file
>>>>> correctly and passes it to opt_default correctly but when I check the
>>>>> value fo avctx->rc_eq in libx264.c it is '\003' and nothing more. Any
>>>>> idea why that might happen? Just putting it out there in case you spot
>>>>> the issue quicker than I do. :) *Continues the hunt...*
>>>> try av_strdup() the argument, sorry this is my fault ...
>>>> ill fix it tomorrow unless you are quicker
>>> Iam not sure which is the best solution ...
>>> we could av_strdup() in av_set_string() (instead of the memcpy)
>>> or
>>> add a av_set_string_dup() that does the av_strdup() when the field is a
>>> char *
>>> or
>>> check before the av_set_string() call if the field is a char * and do a
>>> av_strdup() when the source is static.
>>>
>>> Also theres the issue with freeing them again. If we do plan to free
>>> them again (is actually not really needed for ffmpeg.c) then we must
>>> ensure that we dont mix pointers to the command line with malloced()
>>> memory ...
>> Pros and cons of each? The first approach sounds quite simple and
>> minimal in terms of code alteration.
> 
> The first breaks the API & ABI and we loose the ability to set char* to
> a pointer, as its always set to a copy, i do not know if any of these is an
> issue in practice. But AVOptions originally where supposed to be just some
> lightwight access layer to access fields of structures, enumerate them, ...
> 
> It was reimar who added malloc() for FF_OPT_TYPE_BINARY and broke the
> lightweightness of AVOptions with that.
> Id really like to drop FF_OPT_TYPE_BINARY and let codecs parse a hex
> string rather it seems much cleaner and simpler ...
> 
> Maybe we should just add a av_set_string2(..., int flags) 
> where flags indicate if strdup and free should be done on *char
> That could then at least treat FF_OPT_TYPE_BINARY without malloc/free
> flags as error.
> 

I'd like to drop malloc/free problem for sure, but I'd like to keep a
simple and easy way to set decryption keys.

Maybe an helper in opt.c doing conversion would help, and a way to set
key len has to be found too.

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