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

[James Almer]

On 1/13/2025 7:35 PM, Andreas Rheinhardt wrote:
> 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).
That only applied to stts, because ctts had a realloc call for all 
entries before the loop.
If you think this approach is better, then i can remove that call.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250113/2397b3fd/attachment.sig>