[FFmpeg-cvslog] r16966 - trunk/libavformat/aiff.c

[Aurelien Jacobs]

Michael Niedermayer wrote:

> On Mon, Feb 02, 2009 at 04:32:30PM -0800, Baptiste Coudurier wrote:
> > Aurelien Jacobs wrote:
> > > Baptiste Coudurier wrote:
> > > 
> > >> Aurelien Jacobs wrote:
> > >>> Baptiste Coudurier wrote:
> > >>>
> > >>>> aurel wrote:
> > >>>>> Author: aurel
> > >>>>> Date: Tue Feb  3 00:43:12 2009
> > >>>>> New Revision: 16966
> > >>>>>
> > >>>>> Log:
> > >>>>> use new metadata API in aiff demuxer
> > >>>>>
> > >>>>> Modified:
> > >>>>>    trunk/libavformat/aiff.c
> > >>>>>
> > >>>>> Modified: trunk/libavformat/aiff.c
> > >>>>> ==============================================================================
> > >>>>> --- trunk/libavformat/aiff.c	Tue Feb  3 00:37:03 2009	(r16965)
> > >>>>> +++ trunk/libavformat/aiff.c	Tue Feb  3 00:43:12 2009	(r16966)
> > >>>>> @@ -79,9 +79,10 @@ static int get_tag(ByteIOContext *pb, ui
> > >>>>>  }
> > >>>>>  
> > >>>>>  /* Metadata string read */
> > >>>>> -static void get_meta(ByteIOContext *pb, char * str, int strsize, int size)
> > >>>>> +static void get_meta(AVFormatContext *s, const char *key, int size)
> > >>>>>  {
> > >>>>> -    int res = get_buffer(pb, (uint8_t*)str, FFMIN(strsize-1, size));
> > >>>>> +    uint8_t str[1024];
> > >>>>> +    int res = get_buffer(s->pb, str, FFMIN(sizeof(str)-1, size));
> > >>>>>      if (res < 0)
> > >>>>>          return;
> > >>>> Does the new API have the 1024 limitation ? I hope not.
> > >>> Hopefully not !
> > >>>
> > >>>> Therefore everything must be read and it's up the compat layer to
> > >>>> truncate it to 1024.
> > >>> Everything ? size is a 32 byte value read directly from the bitstream,
> > >>> so I would rather not try to av_malloc(size).
> > >>> So IMO some sanity check must be applied. I though 1024 would be plenty
> > >>> enough. How much do you think the limitation should be ?
> > >> You have a "comment" field, what if quicktime reads it all ?
> > > 
> > > If quicktime wants to read a full 1GB comment field, that is it's own
> > > problem. I don't think FFmpeg should do this...
> > 
> > Well you can get users complaining...
> > Though I also think it's not reasonable.
> > 
> > >> What if I want to write my lyrics in there ? :>
> > > 
> > > Then you would probably need a bit more than 1024 bytes...
> > > If you think it's useful, I will av_malloc() the buffer,
> > > but I still need to set some limit. Would a 1MB limit
> > > be reasonable enough ?
> > 
> > To be honest, I don't really know, I guess 1MB is ok, I'd like to get
> > others opinion also.
> 
> we may need some user settable limit, a file with huge metadata might be too
> much for some embeded system but users on a desktop might still want it
> all

Indeed, a 1MB metadata malloc would be insane on most embeded systems.
A user settable limit sounds like the best solution.
Until then, I think we should stick to the hard 1024 byte limit (there is
no regression here anyway).
Just as a reference, attached patch uses a 1MB malloced buffer.

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: md_aiff_size.diff
Type: text/x-patch
Size: 833 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20090203/0fcfc9a4/attachment.bin>