[FFmpeg-devel] [PATCH] asf - read/write metadata as UTF-16

[Michael Niedermayer]

On Mon, Feb 22, 2010 at 03:44:31PM +0100, Anton Khirnov wrote:
> On Sun, Feb 21, 2010 at 11:03:01PM +0100, Michael Niedermayer wrote:
> > On Sun, Feb 21, 2010 at 09:45:53PM +0100, Anton Khirnov wrote:
> > 
> > > + * a UCS-4 encoded Unicode character that is to be converted to UTF-16. If
> > > + * val is given as a function it is executed only once.
> > 
> > doesnt seems so
> > 
> fixed

> > 
> > PUT_UTF8 doesnt handle errors, and you do it quite incompletly
> > 
> so you want me to skip error checking or add it to PUT_UTF8?

id say validating the stuff should happen somewhere else, how did
invalid values reach that point to begin with?
also your error checking in utf16 is highly incomple, missing checks
for surrogate pairs at least


> > 
> > ok if you checked all uses of this function against the spec and
> > wmp
> > also make sure you bump the version string we store in asf files
> > 
> testing on wmp revealed this patch to be wrong due to asf being more
> braindead than i expected (it stores number of characters, not length in
> bytes here).  instead i've added a patch that removes put_str16
> completely as it's confusing and only used in one place anyway.

are you sure its unicode characters and not bytes/2
(needs surrogate pairs to seperate ...)


> > 
> > > @@ -272,7 +288,7 @@ static int asf_write_header1(AVFormatContext *s, int64_t file_size, int64_t data
> > >  {
> > >      ASFContext *asf = s->priv_data;
> > >      ByteIOContext *pb = s->pb;
> > > -    AVMetadataTag *title, *author, *copyright, *comment;
> > > +    AVMetadataTag *tags[5];
> > 
> > unrelated?
> > 
> > 
> > >      int header_size, n, extra_size, extra_size2, wav_extra_size, file_time;
> > >      int has_title;
> > >      int metadata_count;
> > > @@ -281,13 +297,14 @@ static int asf_write_header1(AVFormatContext *s, int64_t file_size, int64_t data
> > >      int bit_rate;
> > >      int64_t duration;
> > >  
> > > -    title     = av_metadata_get(s->metadata, "title"    , NULL, 0);
> > > -    author    = av_metadata_get(s->metadata, "author"   , NULL, 0);
> > > -    copyright = av_metadata_get(s->metadata, "copyright", NULL, 0);
> > > -    comment   = av_metadata_get(s->metadata, "comment"  , NULL, 0);
> > > +    tags[0] = av_metadata_get(s->metadata, "title"    , NULL, 0);
> > > +    tags[1] = av_metadata_get(s->metadata, "author"   , NULL, 0);
> > > +    tags[2] = av_metadata_get(s->metadata, "copyright", NULL, 0);
> > > +    tags[3] = av_metadata_get(s->metadata, "comment"  , NULL, 0);
> > > +    tags[4] = av_metadata_get(s->metadata, "rating"   , NULL, 0);
> > 
> > adding rating is a unrelated change
> > 
> split
> > 
> > why the strdup change?
> > 
> because 2*len(input) (maximum length of converted output) is much more
> than is needed in most cases.

what is it now? characters? if so 2*len is not enough not all characters
can be stored in 2 bytes.

[...]
> @@ -157,12 +160,12 @@ static void get_tag(AVFormatContext *s, const char *key, int type, int len)
>      if ((unsigned)len >= UINT_MAX)
>          return;
>  
> -    value = av_malloc(len+1);
> +    value = av_malloc(2*len+1);

integer overflow

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20100222/92c5da69/attachment.pgp>