| Message ID | 20180827195928.25540-1-jstebbins@jetheaddev.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Aug 27, 2018 at 12:59:28PM -0700, John Stebbins wrote: > This type of audio is defined by the QT spec, but can be found in > non-QT branded files in the wild. > > Fixes ticket #7376 > --- > libavformat/mov.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) So IIUC this is supporting a case that is slightly violating the spec ? Iam asking as i was wondering if it makes sense to check a few more things. For example the size could be checked if it actually is large enough to contain a version=1 header either way i think this patch should be ok [...] thx
2018-08-31 1:43 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc>: > On Mon, Aug 27, 2018 at 12:59:28PM -0700, John Stebbins wrote: >> This type of audio is defined by the QT spec, but can be found in >> non-QT branded files in the wild. >> >> Fixes ticket #7376 >> --- >> libavformat/mov.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) > > So IIUC this is supporting a case that is slightly violating the spec ? > Iam asking as i was wondering if it makes sense to check a few more > things. For example the size could be checked if it actually is > large enough to contain a version=1 header > > either way i think this patch should be ok Maybe Baptiste finds testclip_wrong_version_1_audio_stsd.mp4 or has a comment. Carl Eugen
On 08/30/2018 04:43 PM, Michael Niedermayer wrote: > On Mon, Aug 27, 2018 at 12:59:28PM -0700, John Stebbins wrote: >> This type of audio is defined by the QT spec, but can be found in >> non-QT branded files in the wild. >> >> Fixes ticket #7376 >> --- >> libavformat/mov.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) > So IIUC this is supporting a case that is slightly violating the spec ? > Iam asking as i was wondering if it makes sense to check a few more > things. For example the size could be checked if it actually is > large enough to contain a version=1 header > > either way i think this patch should be ok > > Yes, your understanding is correct. I don't think the size can be used here because it includes the sizes of any other atoms that are contained in the stsd entry. It might be possible to sanity check some of the version qt version 1 (or 2) values read and rewind the stream if they don't make sense (i.e. extreme values). Or the following data could first be read as if it were *not* qt version 1 and checked if it made sense (i.e. looked like the start of an atom), then rewind. Unfortunately, the audio actually plays correctly if you just drop the qt version 1 data on the floor. So it's not necessary for playback which means someone could create yet another spec violating sample that has 24 bit pcm but muxed using isom version 1 instead. If anything like this exists in the wild, it would be necessary to perform heuristic sanity checking of values I think.
diff --git a/libavformat/mov.c b/libavformat/mov.c index 8915e3b9e0..3d4f6bcb21 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -2100,6 +2100,18 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, } } +static int mov_pcm_gt16(enum AVCodecID codec_id) +{ + return codec_id == AV_CODEC_ID_PCM_S24LE || + codec_id == AV_CODEC_ID_PCM_S32LE || + codec_id == AV_CODEC_ID_PCM_F32LE || + codec_id == AV_CODEC_ID_PCM_F64LE || + codec_id == AV_CODEC_ID_PCM_S24BE || + codec_id == AV_CODEC_ID_PCM_S32BE || + codec_id == AV_CODEC_ID_PCM_F32BE || + codec_id == AV_CODEC_ID_PCM_F64BE; +} + static void mov_parse_stsd_audio(MOVContext *c, AVIOContext *pb, AVStream *st, MOVStreamContext *sc) { @@ -2120,8 +2132,10 @@ static void mov_parse_stsd_audio(MOVContext *c, AVIOContext *pb, st->codecpar->sample_rate = ((avio_rb32(pb) >> 16)); // Read QT version 1 fields. In version 0 these do not exist. + // PCM with bitdepth > 16 is only defined by QT version 1. av_log(c->fc, AV_LOG_TRACE, "version =%d, isom =%d\n", version, c->isom); if (!c->isom || + mov_pcm_gt16(st->codecpar->codec_id) || (compatible_brands && strstr(compatible_brands->value, "qt "))) { if (version == 1) {