| Message ID | CAPUDrwfiqZXgf=v7cRy6_H35tO=yU8h2iJO3zXQFfUxbozpsqA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 25, 2017 at 11:59:51AM -0700, Dale Curtis wrote: > On Fri, Aug 25, 2017 at 5:43 AM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > > > This patch breaks: > > http://samples.ffmpeg.org/mov/mp4/discont-frag.mp4 > > > > > Hmm, indeed it does. The reason is that we read the sample count from the > stsz box and then read the trun box. I don't think this block of code has > ever been correct in that case: > > http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/mov.c;hb=HEAD# > l4287 > > It shifts all the ctts entries incorrectly and even did so prior to my > patch. I've uploaded a new version of my fix which simply deletes this > block of code. It passes all the fate test cases and those we have in > Chrome. Let me know if fails any of your private test cases. > > - dale > mov.c | 25 +++---------------------- > 1 file changed, 3 insertions(+), 22 deletions(-) > 526e37d02ef1cc4ab1eed7d4f330ecc2b4bb5e8e sample_count_fix_v3.patch > From 049f885ee972b0efb6dcbf456025e56dd627b8b9 Mon Sep 17 00:00:00 2001 > From: Dale Curtis <dalecurtis@chromium.org> > Date: Mon, 31 Jul 2017 13:44:22 -0700 > Subject: [PATCH] [mov] Bail when invalid sample data is present. > > ctts data in ffmpeg relies on the index entries array to be 1:1 > with samples... yet sc->sample_count can be read directly from > the 'stsz' box and index entries are only generated if a chunk > count has been read from 'stco' box. > > Ensure that if sc->sample_count > 0, sc->chunk_count is too as > a basic sanity check. Additionally we need to check that after > the index is built we have the right number of entries, so we > also check in mov_read_trun() that sc->sample_count == > st->nb_index_entries. > --- > libavformat/mov.c | 25 +++---------------------- > 1 file changed, 3 insertions(+), 22 deletions(-) This changes the printed duration start time and bitrate for MAV_0034.3G2 see https://trac.ffmpeg.org/ticket/2757 [...]
On Fri, Aug 25, 2017 at 6:09 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > > This changes the printed duration start time and bitrate for > MAV_0034.3G2 > see > https://trac.ffmpeg.org/ticket/2757 Thanks, I've restored the ability to query these types of files. That file also reveals the potential for a case that was unhandled prior to any of my patches. File which mixed boxes with and without ctts entries would pick up either the wrong ctts entry or uninitialized data. Zero initializing the the ctts data array solves this problem. - dale
Ping, without the patch in the previous message you can get crashes when encountering these types of files. - dale On Mon, Aug 28, 2017 at 2:45 PM, Dale Curtis <dalecurtis@chromium.org> wrote: > On Fri, Aug 25, 2017 at 6:09 PM, Michael Niedermayer < > michael@niedermayer.cc> wrote: >> >> This changes the printed duration start time and bitrate for >> MAV_0034.3G2 >> see >> https://trac.ffmpeg.org/ticket/2757 > > > Thanks, I've restored the ability to query these types of files. That file > also reveals the potential for a case that was unhandled prior to any of my > patches. File which mixed boxes with and without ctts entries would pick up > either the wrong ctts entry or uninitialized data. Zero initializing the > the ctts data array solves this problem. > > - dale >
On Thu, Aug 31, 2017 at 12:23:04PM -0700, Dale Curtis wrote: > Ping, without the patch in the previous message you can get crashes when > encountering these types of files. will apply thanks [...]
From 049f885ee972b0efb6dcbf456025e56dd627b8b9 Mon Sep 17 00:00:00 2001 From: Dale Curtis <dalecurtis@chromium.org> Date: Mon, 31 Jul 2017 13:44:22 -0700 Subject: [PATCH] [mov] Bail when invalid sample data is present. ctts data in ffmpeg relies on the index entries array to be 1:1 with samples... yet sc->sample_count can be read directly from the 'stsz' box and index entries are only generated if a chunk count has been read from 'stco' box. Ensure that if sc->sample_count > 0, sc->chunk_count is too as a basic sanity check. Additionally we need to check that after the index is built we have the right number of entries, so we also check in mov_read_trun() that sc->sample_count == st->nb_index_entries. --- libavformat/mov.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/libavformat/mov.c b/libavformat/mov.c index 876f48d912..f8bcba4cd1 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3755,8 +3755,9 @@ static int mov_read_trak(MOVContext *c, AVIOContext *pb, MOVAtom atom) c->trak_index = -1; /* sanity checks */ - if (sc->chunk_count && (!sc->stts_count || !sc->stsc_count || - (!sc->sample_size && !sc->sample_count))) { + if ((sc->chunk_count && (!sc->stts_count || !sc->stsc_count || + (!sc->sample_size && !sc->sample_count))) || + (!sc->chunk_count && sc->sample_count)) { av_log(c->fc, AV_LOG_ERROR, "stream %d, missing mandatory atoms, broken header\n", st->index); return 0; @@ -4284,26 +4285,6 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) entries = avio_rb32(pb); av_log(c->fc, AV_LOG_TRACE, "flags 0x%x entries %u\n", flags, entries); - /* Always assume the presence of composition time offsets. - * Without this assumption, for instance, we cannot deal with a track in fragmented movies that meet the following. - * 1) in the initial movie, there are no samples. - * 2) in the first movie fragment, there is only one sample without composition time offset. - * 3) in the subsequent movie fragments, there are samples with composition time offset. */ - if (!sc->ctts_count && sc->sample_count) - { - /* Complement ctts table if moov atom doesn't have ctts atom. */ - ctts_data = av_fast_realloc(NULL, &sc->ctts_allocated_size, sizeof(*sc->ctts_data) * sc->sample_count); - if (!ctts_data) - return AVERROR(ENOMEM); - /* Don't use a count greater than 1 here since it will leave a gap in - * the ctts index which the code below relies on being sequential. */ - sc->ctts_data = ctts_data; - for (i = 0; i < sc->sample_count; i++) { - sc->ctts_data[sc->ctts_count].count = 1; - sc->ctts_data[sc->ctts_count].duration = 0; - sc->ctts_count++; - } - } if ((uint64_t)entries+sc->ctts_count >= UINT_MAX/sizeof(*sc->ctts_data)) return AVERROR_INVALIDDATA; if (flags & MOV_TRUN_DATA_OFFSET) data_offset = avio_rb32(pb); -- 2.14.1.342.g6490525c54-goog