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

[Michael Niedermayer]

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.


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

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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-cvslog/attachments/20080619/638c7230/attachment.pgp>