[FFmpeg-devel] [PATCH 3/3] Export metadata in the generic format.

[Michael Niedermayer]

On Wed, Oct 13, 2010 at 07:27:48AM +0200, Anton Khirnov wrote:
> On Tue, Oct 12, 2010 at 11:54:12PM +0200, Aurelien Jacobs wrote:
> > On Tue, Oct 12, 2010 at 09:37:32PM +0200, Anton Khirnov wrote:
> > > On Tue, Oct 12, 2010 at 09:12:31PM +0200, Michael Niedermayer wrote:
> > > > On Tue, Oct 12, 2010 at 07:35:17PM +0200, Anton Khirnov wrote:
> > > > > On Mon, Oct 11, 2010 at 10:33:21PM +0200, Michael Niedermayer wrote:
> > > > > > On Tue, Oct 05, 2010 at 07:57:06PM +0200, Anton Khirnov wrote:
> > > > > > 
> > > > > > thats a strange place for it one would expect it at the end of the header
> > > > > > reading code
> > > > > > 
> > > > > > 
> > > > > [...]
> > > > > > 
> > > > > > same as asf odd place, there could be multipl info tags
> > > > > > and the same applies to other changes too
> > > > > > 
> > > > > Better now?
> > > > > 
> > > > > I've also removed the conv tables from (de)muxers completely instead of
> > > > > just ifdefing them, since NULL is also a perfeclty valid conv table.
> > > > [...]
> > > > > diff --git a/libavformat/metadata.c b/libavformat/metadata.c
> > > > > index 8fc1771..713304b 100644
> > > > > --- a/libavformat/metadata.c
> > > > > +++ b/libavformat/metadata.c
> > > > > @@ -140,8 +140,8 @@ void metadata_conv(AVMetadata **pm, const AVMetadataConv *d_conv,
> > > > >      *pm = dst;
> > > > >  }
> > > > >  
> > > > > -void av_metadata_conv(AVFormatContext *ctx, const AVMetadataConv *d_conv,
> > > > > -                                            const AVMetadataConv *s_conv)
> > > > > +void metadata_conv_ctx(AVFormatContext *ctx, const AVMetadataConv *d_conv,
> > > > 
> > > > ff_ prefix
> > > done
> > > 
> > > i guess metadata_conv() should get a ff_ prefix too, i'll send a patch
> > > for this later
> > 
> > Sure ! Patch welcome.
> > 
> > > [...]
> > > @@ -147,7 +146,9 @@ typedef struct {
> > >  }AVMetadataTag;
> > >  
> > >  typedef struct AVMetadata AVMetadata;
> > > +#if FF_API_OLD_METADATA
> > >  typedef struct AVMetadataConv AVMetadataConv;
> > > +#endif
> > 
> > AFAICT this typedef is still used with the new API (but don't need to be
> > public anymore). Did you tried to compile with:
> >   --extra-cflags=-DFF_API_OLD_METADATA=0
> fixed
> > 
> > > [...]
> > > @@ -152,3 +152,9 @@ void av_metadata_conv(AVFormatContext *ctx, const AVMetadataConv *d_conv,
> > >      for (i=0; i<ctx->nb_programs; i++)
> > >          metadata_conv(&ctx->programs[i]->metadata, d_conv, s_conv);
> > >  }
> > > +
> > > +void av_metadata_conv(AVFormatContext *ctx, const AVMetadataConv *d_conv,
> > > +                                            const AVMetadataConv *s_conv)
> > > +{
> > > +    return;
> > > +}
> > 
> > Should be inside #if FF_API_OLD_METADATA.
> > 
> fixed
> > Overall, I have a slightly bad feeling about exporting normalized
> > metadata tag instead of the native one, but I don't have any objective
> > reason for this, just a feeling. So if everyone is happy with this, I
> > won't object.
> > 
> Well it seems to be the simplest solution. And in all the time the new
> metadata API existed i didn't see any program use the native tags.

i suggest we move forward and deal with problems if they arrise.
Nothing will be helped by stoping due to vague bad feeling. Its not specific
enough to be able to do anything about.

and patch should be ok

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101013/aadd6e5a/attachment.pgp>