[FFmpeg-devel] [PATCH] metadata conversion API

[Michael Niedermayer]

On Tue, Mar 03, 2009 at 05:16:33PM -0800, Baptiste Coudurier wrote:
> On 3/3/2009 3:02 PM, Michael Niedermayer wrote:
> > On Tue, Mar 03, 2009 at 02:20:47PM -0800, Baptiste Coudurier wrote:
> >> On 3/3/2009 2:05 PM, Michael Niedermayer wrote:
> >>> On Tue, Mar 03, 2009 at 09:35:09PM +0000, Robert Swain wrote:
> >>>> 2009/3/3 Michael Niedermayer <michaelni at gmx.at>:
> >>>>> On Mon, Mar 02, 2009 at 06:25:30PM -0800, Baptiste Coudurier wrote:
> >>>>>> I however volunteer to add key/value as char* options to AVFormatContext
> >>>>>> and AVCodecContext and use them.
> >>>>> You said that already but there is no relation between that and such validity
> >>>>> checks, its not going to solve this problem.
> >>>> OK, so maybe this is off topic for this thread.
> >>>>
> >>>>> Things added to svn have to be justified and their advantages and
> >>>>> disadavatanges weighted against each other. Also once a technical discussion
> >>>>> happened its the decission of the maintainer(s). Or in exceptional
> >>>>> circumstances a vote (or consensus for thouse prefering the terminology)
> >>>>> overriding the maintainer.
> >>>>>
> >>>>> The arguments brought forth so far in favor of it, where that you want to
> >>>>> export ".mov/width" with a value that could be scaling or croping. And that
> >>>>> some advanced users may be able to make more sense out of it then we. And
> >>>>> that you prefer not to export the croping information in the existing
> >>>>> fields for this puprose.
> >>>>> And you want to export a field (whichs name you have not revealed yet)
> >>>>> that only has 2, 4 and 8 as valid values.
> >>>>> Have i missed some? I remember alot of flaming and trolling and some
> >>>>> unsuccessfull attempts at provocating me but little actual discussion
> >>>>> about it.
> >>>>>
> >>>>> The arguments against, being a roughly 10x increase in memory consumtion,
> >>>>> similar or worse increase in cpu consumtion for access and the loss of a
> >>>>> common set of parameters for codecs. Each could have their own flag to
> >>>>> enable b frames ...
> >>>> The CPU loss should be minimal and it's only a set up cost. How many
> >>>> cycles are really spent on option parsing?
> >>> we directly access fields from AVCodecContext currently just try
> >>> grep avctx libavcodec/*.c | grep -v av_log
> >>> there are over 5000 matches
> >>> i do not want to replace 5000 single instruction reads with 5000 function
> >>> calls doing a search through a table of strings.
> >>> Especially considering that i dont see what we would gain by this
> >> I don't see how this relates to using char */char * as options, you can
> >> set AVCodecContext fields if you want.
> > 
> > thats what we have currently
> > av_set_string() takes a name & value char * and sets the corresponding field
> > on AVCodecContext
> 
> This does not work with libx264 native options, does it ?

not currently, no


> 
> >> Although almost every codec has its own context anyway, point is to
> >> factorize when it's actually needed and not by principle.
> > 
> > So would you agree that the char * are only allowed to be used for things
> > that are used by just one muxer/demuxer or encoder/decoder ?
> 
> Yes, this rule would apply to any of them, even native ones.

yes it would apply to native ones, but for example this would disallow
having seperate options to enable trellis quant for x264 & native mpeg4.

Now i am not really convinced that it is a good idea ...
i guess the best i can do is limit the damage this will do, and trust
in your sanity not to misuse it too much.

Anyway, here are a few requirements on the system, i belive they should not
be controversal
* We need a transparent way to move the field into a struct because if for
  example your "mov/bits per sample" is added we might find out that
  avi too has a "avi/bits per sample" with similar semantics and by the
  "supreme rule" the system isnt allowed to be used for such things,
  they have to be moved to a struct and that must not require a major
  ver bump.
  The current way to access fields of structs is through AVOptions, it
  seems easiest thus if you would just add some offset/flag/field to AVClass
  to indicate that the structs of that kind have a char*/char* list
  that way any AVClass struct could automatically contain such name/value
  pairs and AVOption could easily access them transparently

* GUI tools have to be able to list the available options a user could set
  That includes name, default value, some description and max/min as well
  as type. Command line tools similarly have to be able to list this
  ffmpeg -h does it for us. The new char/char options must appear in
  this listing too and must contain at least as many details.
  Again the most compatible & least painfull way for user apps would be
  if the existing AVOption system would include them in its printout.

* All things both output as well as input have to be documented, no
  char/char option is allowed to lack documentation, this documentation
  should clearly say what we know about its meaning, which single demuxer/
  decoder/... uses it

* Seperation to metadata, while it probably makes sense to use AVMetadata
  and the functions that come with it, things like mov/bits per sample does
  not fall in the same category as author & title
  In that sense we also should apply great care when/if any of this is
  preserved on remuxing, it might make sense for some fields but others
  could be less good (timestamps, IDs that could identify the computer
  and succh ...)

but still i must say i have a bad feeling about the whole, demuxers are
not kernel modules, they arent seperare things, rather they all do very
very similar work and fields should be shared, this codec specific stuff
has more misuse potential than actual use.

A good rule of thumb might be:
If you/we can imagine any user (app) that might want to access 2 fields
as one then they should be merged and a real field in a real C struct.


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

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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-devel/attachments/20090304/691fab2d/attachment.pgp>