[FFmpeg-devel] [PATCH 0/7] ffmpeg: add a grow_array() helper function

[Aurelien Jacobs]

On Wed, Aug 18, 2010 at 10:46:14PM +0200, Michael Niedermayer wrote:
> On Tue, Aug 17, 2010 at 12:09:52AM +0200, Aurelien Jacobs wrote:
> > On Mon, Aug 16, 2010 at 11:44:26AM -0400, Ronald S. Bultje wrote:
> > > Hi,
> > > 
> > > On Mon, Aug 16, 2010 at 2:00 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > On Fri, Aug 13, 2010 at 08:28:12PM +0200, Aurelien Jacobs wrote:
> > > >> Ooops... I slightly messed up this patch serie.
> > > >> I forgot the first patch which adds a grow_array() function which is
> > > >> then used by all the following patches. Here it is.
> > > >> The rest of the serie changes each *[MAX_STREAMS] arrays from ffmpeg.c
> > > >> into a dynamically allocated array.
> > > >>
> > > >> Aurel
> > > >>
> > > >>
> > > >> diff --git a/ffmpeg.c b/ffmpeg.c
> > > >> index aec1f79..28ce27f 100644
> > > >> --- a/ffmpeg.c
> > > >> +++ b/ffmpeg.c
> > > >> @@ -646,6 +646,23 @@ static int ffmpeg_exit(int ret)
> > > >> ? ? ?return ret;
> > > >> ?}
> > > >>
> > > >> +static void *grow_array(void *array, int elem_size, int *size, int new_size)
> > > >> +{
> > > >> + ? ?if (*size < new_size) {
> > > >> + ? ? ? ?uint8_t *tmp = av_realloc(array, new_size*elem_size);
> > > >
> > > > integer overflow
> > 
> > Added a check.
> > 
> > > > also see ff_dynarray_add() and add notes that refer to it and back so changed
> > > > done to one can be checked if they would also be usefull to the other
> > 
> > Note added.
> > 
> > > Isn't this the same as av_fast_realloc()?
> > 
> > Not exactly. It is different from both ff_dynarray_add() and
> > av_fast_realloc(). This one indeed extand the size of an allocation, but
> > it also memset(0) the newly allocated part (to allow sparse array with
> > no uninitialized elements) and it keep track of the number of elements
> > in the array, instead of the size of the buffer.
> > 
> > Aurel
> >  ffmpeg.c             |   22 ++++++++++++++++++++++
> >  libavformat/cutils.c |    1 +
> >  2 files changed, 23 insertions(+)
> > f233c6a7f4fba2259ca9fa0fa8d9431965737e40  grow_array.diff
> > commit a392dcf1f80cec191262051cbb315f24549e724f
> > Author: Aurelien Jacobs <aurel at gnuage.org>
> > Date:   Fri Aug 13 16:58:19 2010 +0200
> > 
> >     ffmpeg: add a grow_array() helper function
> > 
> > diff --git a/ffmpeg.c b/ffmpeg.c
> > index aec1f79..da74dce 100644
> > --- a/ffmpeg.c
> > +++ b/ffmpeg.c
> > @@ -646,6 +646,28 @@ static int ffmpeg_exit(int ret)
> >      return ret;
> >  }
> >  
> > +/* similar to ff_dynarray_add() and av_fast_realloc() */
> > +static void *grow_array(void *array, int elem_size, int *size, int new_size)
> > +{
> > +    if (new_size >= INT_MAX / elem_size) {
> > +        fprintf(stderr, "Array too big.\n");
> > +        ffmpeg_exit(1);
> > +    }
> > +    if (*size < new_size) {
> > +        uint8_t *tmp = av_realloc(array, new_size*elem_size);
> > +        if (!tmp) {
> > +            fprintf(stderr, "Could not alloc buffer.\n");
> > +            ffmpeg_exit(1);
> > +        }
> > +        memset(tmp + *size*elem_size, 0, (new_size-*size) * elem_size);
> > +        *size = new_size;
> > +        return tmp;
> > +    }
> > +    return array;
> > +}
> 
> > +#define GROW_ARRAY(array, size)  \
> > +    array = grow_array(array, sizeof(*array), &nb_##array, size)
> 
> iam not a fan of such macros, they hurt readability alot for the little
> gain of a few keypresses.

I'm generally not fond of such kind of macro either, but in this case,
it avoid repeating "array" 4 times for each graw_array() call. With the
typical length of the "array" name, without macro we get ugly overly
long lines for each grow_array() call.
So I really don't care about the number of keypresses. I only cared
about keeping line length reasonable...

But anyway, I really have no strong feeling about this, so I will inline
this macro. Do you want me to resend this whole patchset for review ?

Aurel