[FFmpeg-devel] [PATCH 2/2] avformat/mov: simplify allocating stts/ctts arrays

[Andreas Rheinhardt]

James Almer:
> No point using av_fast_realloc() in a loop when we want to allocate all
> entries to begin with, and any duplicate stts/ctts will just replace the
> old arrays.
> Furthermore, these are temporary arrays that will be merged into tts_data
> when building the index.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  libavformat/isom.h |  2 --
>  libavformat/mov.c  | 40 ++++++----------------------------------
>  2 files changed, 6 insertions(+), 36 deletions(-)
> 
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index ccdead7192..16981dc918 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -183,12 +183,10 @@ typedef struct MOVStreamContext {
>      unsigned int tts_allocated_size;
>      MOVTimeToSample *tts_data;
>      unsigned int stts_count;
> -    unsigned int stts_allocated_size;
>      MOVStts *stts_data;
>      unsigned int sdtp_count;
>      uint8_t *sdtp_data;
>      unsigned int ctts_count;
> -    unsigned int ctts_allocated_size;
>      MOVCtts *ctts_data;
>      unsigned int stsc_count;
>      MOVStsc *stsc_data;
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 138120488a..f310cb8d49 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -3498,22 +3498,14 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          av_log(c->fc, AV_LOG_WARNING, "Duplicated STTS atom\n");
>      av_freep(&sc->stts_data);
>      sc->stts_count = 0;
> -    if (entries >= INT_MAX / sizeof(*sc->stts_data))
> +
> +    sc->stts_data = av_malloc_array(entries, sizeof(*sc->stts_data));
> +    if (!sc->stts_data)
>          return AVERROR(ENOMEM);
>  
>      for (i = 0; i < entries && !pb->eof_reached; i++) {
>          unsigned int sample_duration;
>          unsigned int sample_count;
> -        unsigned int min_entries = FFMIN(FFMAX(i + 1, 1024 * 1024), entries);
> -        MOVStts *stts_data = av_fast_realloc(sc->stts_data, &sc->stts_allocated_size,
> -                                             min_entries * sizeof(*sc->stts_data));
> -        if (!stts_data) {
> -            av_freep(&sc->stts_data);
> -            sc->stts_count = 0;
> -            return AVERROR(ENOMEM);
> -        }
> -        sc->stts_count = min_entries;
> -        sc->stts_data = stts_data;
>  
>          sample_count    = avio_rb32(pb);
>          sample_duration = avio_rb32(pb);
> @@ -3656,20 +3648,12 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  
>      if (!entries)
>          return 0;
> -    if (entries >= UINT_MAX / sizeof(*sc->ctts_data))
> -        return AVERROR_INVALIDDATA;
>      av_freep(&sc->ctts_data);
> -    sc->ctts_data = av_fast_realloc(NULL, &sc->ctts_allocated_size, entries * sizeof(*sc->ctts_data));
> +    sc->ctts_data = av_malloc_array(entries, sizeof(*sc->ctts_data));
>      if (!sc->ctts_data)
>          return AVERROR(ENOMEM);
>  
>      for (i = 0; i < entries && !pb->eof_reached; i++) {
> -        MOVCtts *ctts_data;
> -        const size_t min_size_needed = (ctts_count + 1) * sizeof(MOVCtts);
> -        const size_t requested_size =
> -            min_size_needed > sc->ctts_allocated_size ?
> -            FFMAX(min_size_needed, 2 * sc->ctts_allocated_size) :
> -            min_size_needed;
>          int count    = avio_rb32(pb);
>          int duration = avio_rb32(pb);
>  
> @@ -3680,18 +3664,8 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>              continue;
>          }
>  
> -        if (ctts_count >= UINT_MAX / sizeof(MOVCtts) - 1)
> -            return AVERROR(ENOMEM);
> -
> -        ctts_data = av_fast_realloc(sc->ctts_data, &sc->ctts_allocated_size, requested_size);
> -
> -        if (!ctts_data)
> -            return AVERROR(ENOMEM);
> -
> -        sc->ctts_data = ctts_data;
> -
> -        ctts_data[ctts_count].count = count;
> -        ctts_data[ctts_count].offset = duration;
> +        sc->ctts_data[ctts_count].count = count;
> +        sc->ctts_data[ctts_count].offset = duration;
>          ctts_count++;
>  
>          av_log(c->fc, AV_LOG_TRACE, "count=%d, duration=%d\n",
> @@ -4585,7 +4559,6 @@ static int mov_merge_tts_data(MOVContext *mov, AVStream *st, int flags)
>      } else
>          sc->ctts_count = 0;
>      av_freep(&sc->ctts_data);
> -    sc->ctts_allocated_size = 0;
>  
>      idx = 0;
>      if (stts) {
> @@ -4610,7 +4583,6 @@ static int mov_merge_tts_data(MOVContext *mov, AVStream *st, int flags)
>      } else
>          sc->stts_count = 0;
>      av_freep(&sc->stts_data);
> -    sc->stts_allocated_size = 0;
>  
>      return 0;
>  }

I always thought that this pattern is used because it avoids having to
allocate a potentially huge buffer just because a counter says that many
elements are present even when the actual file doesn't contain that many
entries because it is very small (this might cause timeout issues in the
fuzzer).

- Andreas