From: Chris Moeller <kode54@gmail.com> --- libavformat/id3v2.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 46b9394..c38c610 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -400,6 +400,47 @@ error: } /** + * Parse a comment tag. + */ +static void read_comm(AVFormatContext *s, AVIOContext *pb, int taglen, + AVDictionary **metadata) +{ + const char *key = "comment"; + uint8_t *dst; + int encoding, dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_VAL; + int language; + + if (taglen < 4) + return; + + encoding = avio_r8(pb); + taglen--; + + language = avio_rl24(pb); + taglen -= 3; + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + return; + } + + if (dst && dst[0]) { + key = (const char *) dst; + dict_flags |= AV_DICT_DONT_STRDUP_KEY; + } + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + if (dict_flags & AV_DICT_DONT_STRDUP_KEY) + av_freep((void*)&key); + return; + } + + if (dst) + av_dict_set(metadata, key, (const char *) dst, dict_flags); +} + +/** * Parse GEOB tag into a ID3v2ExtraMetaGEOB struct. */ static void read_geobtag(AVFormatContext *s, AVIOContext *pb, int taglen, @@ -905,6 +946,9 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, av_log(s, AV_LOG_WARNING, "Skipping %s ID3v2 frame %s.\n", type, tag); avio_skip(pb, tlen); + /* check for comment frame */ + } else if (!memcmp(tag, "COMM", 4)) { + read_comm(s, pbx, tlen, metadata); /* check for text tag or supported special meta tag */ } else if (tag[0] == 'T' || !memcmp(tag, "USLT", 4) || -- 2.7.4 (Apple Git-66)
From: Chris Moeller <kode54@gmail.com> --- libavformat/mp3dec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..3055e2c 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,53 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } } +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; + + if (sscanf(de->value, "%x %x %x %llx %x %llx", &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6) { + *duration = 0; + return; + } + + mp3->start_pad = start_pad; + mp3->end_pad = end_pad; + if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset; + avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET); + for (i = 0; i < 8; i++) { + v = avio_rb32(s->pb); + if (ff_mpa_check_header(v) < 0) + return; + if (avpriv_mpegaudio_decode_header(c, v) != 0) + break; + *size += c->frame_size; + avio_skip(s->pb, c->frame_size - 4); + } +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +350,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; + unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; + uint64_t duration = 0; ffio_init_checksum(s->pb, ff_crcA001_update, 0); @@ -327,16 +376,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base); if (!mp3->frames && !mp3->header_filesize) + vbrtag_size = 0; + + mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + + if (!mp3->frames && !size && !duration) return -1; /* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET); - if (mp3->frames) + if (duration) + st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); + else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + if (size) { + if (duration) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); + else if (mp3->frames) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + } return 0; } -- 2.7.4 (Apple Git-66)
On Fri, Jul 22, 2016 at 05:19:14PM -0700, kode54@gmail.com wrote:
From: Chris Moeller <kode54@gmail.com>
--- libavformat/mp3dec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..3055e2c 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,53 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } }
+static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; +
+ if (sscanf(de->value, "%x %x %x %llx %x %llx", &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6) {
%llx mismatches the type [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Avoid a single point of failure, be that a person or equipment.
Le sextidi 6 thermidor, an CCXXIV, Michael Niedermayer a écrit :
+ uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset;
+ if (sscanf(de->value, "%x %x %x %llx %x %llx", &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6) {
%llx mismatches the type
The other ones too, although it is less visible since uint32_t is usually just unsigned int. Regards, -- Nicolas George
On 7/23/2016 8:25 AM, Michael Niedermayer wrote:
On Fri, Jul 22, 2016 at 05:19:14PM -0700, kode54@gmail.com wrote:
From: Chris Moeller <kode54@gmail.com>
--- libavformat/mp3dec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..3055e2c 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,53 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } }
+static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; +
+ if (sscanf(de->value, "%x %x %x %llx %x %llx", &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6) {
%llx mismatches the type
Also please use the SCN* macros, which work like the PRI* macros from printf.
From: Chris Moeller <kode54@gmail.com> --- libavformat/id3v2.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 46b9394..c38c610 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -400,6 +400,47 @@ error: } /** + * Parse a comment tag. + */ +static void read_comm(AVFormatContext *s, AVIOContext *pb, int taglen, + AVDictionary **metadata) +{ + const char *key = "comment"; + uint8_t *dst; + int encoding, dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_VAL; + int language; + + if (taglen < 4) + return; + + encoding = avio_r8(pb); + taglen--; + + language = avio_rl24(pb); + taglen -= 3; + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + return; + } + + if (dst && dst[0]) { + key = (const char *) dst; + dict_flags |= AV_DICT_DONT_STRDUP_KEY; + } + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + if (dict_flags & AV_DICT_DONT_STRDUP_KEY) + av_freep((void*)&key); + return; + } + + if (dst) + av_dict_set(metadata, key, (const char *) dst, dict_flags); +} + +/** * Parse GEOB tag into a ID3v2ExtraMetaGEOB struct. */ static void read_geobtag(AVFormatContext *s, AVIOContext *pb, int taglen, @@ -905,6 +946,9 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, av_log(s, AV_LOG_WARNING, "Skipping %s ID3v2 frame %s.\n", type, tag); avio_skip(pb, tlen); + /* check for comment frame */ + } else if (!memcmp(tag, "COMM", 4)) { + read_comm(s, pbx, tlen, metadata); /* check for text tag or supported special meta tag */ } else if (tag[0] == 'T' || !memcmp(tag, "USLT", 4) || -- 2.7.4 (Apple Git-66)
Sorry for the noise, I made some poor assumptions. First, I wasn’t really paying attention when it mentioned the whitespace errors on apply, and didn’t correct them the first time. Second, I didn’t realize that it would ignore my —subject directive when sending the revised first patch. Now I learn that there is an —annotate switch. More knowledge for the future. The second patch depends on the first, since the relevant metadata is in a COMM frame.
On Sat, Jul 23, 2016, at 01:59 AM, kode54@gmail.com wrote:
[...]
/** + * Parse a comment tag. + */ +static void read_comm(AVFormatContext *s, AVIOContext *pb, int taglen, + AVDictionary **metadata) This should probably be `read_comment`, it's not too long of a function name, and `read_comm` sounds a little ambiguous.
+{ + const char *key = "comment"; + uint8_t *dst; + int encoding, dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_VAL; + int language; + + if (taglen < 4) + return; + + encoding = avio_r8(pb); + taglen--; + + language = avio_rl24(pb); + taglen -= 3; Unless you intend to do checking of `encoding`/`language`, and proceed conditionally, this should probably combine both `taglen` statements. E.g.
encoding = avio_r8(pb); language = avio_rl24(pb); tagline -= 4;
[...]
As far I can tell, that's it for this patch (someone else might have some more comments). -- Josh
On Fri, Jul 22, 2016 at 05:59:09PM -0700, kode54@gmail.com wrote:
From: Chris Moeller <kode54@gmail.com>
--- libavformat/id3v2.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 46b9394..c38c610 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -400,6 +400,47 @@ error: }
/** + * Parse a comment tag. + */ +static void read_comm(AVFormatContext *s, AVIOContext *pb, int taglen, + AVDictionary **metadata) +{ + const char *key = "comment"; + uint8_t *dst; + int encoding, dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_VAL; + int language; + + if (taglen < 4) + return; + + encoding = avio_r8(pb); + taglen--; + + language = avio_rl24(pb); + taglen -= 3; + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + return; + } + + if (dst && dst[0]) { + key = (const char *) dst; + dict_flags |= AV_DICT_DONT_STRDUP_KEY; + } + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + if (dict_flags & AV_DICT_DONT_STRDUP_KEY) + av_freep((void*)&key); + return; + } + + if (dst) + av_dict_set(metadata, key, (const char *) dst, dict_flags); +} + +/** * Parse GEOB tag into a ID3v2ExtraMetaGEOB struct. */ static void read_geobtag(AVFormatContext *s, AVIOContext *pb, int taglen,
> @@ -905,6 +946,9 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
av_log(s, AV_LOG_WARNING, "Skipping %s ID3v2 frame %s.\n", type, tag); avio_skip(pb, tlen); + /* check for comment frame */ + } else if (!memcmp(tag, "COMM", 4)) { + read_comm(s, pbx, tlen, metadata);
pbx can be uninitialized here [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
From: Chris Moeller <kode54@gmail.com> Let's try and get this right, one last time. No uninitialized variables, no problems with unsynchronized or compressed frames, etc. --- libavformat/id3v2.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 46b9394..380a982 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -400,6 +400,45 @@ error: } /** + * Parse a comment tag. + */ +static void read_comment(AVFormatContext *s, AVIOContext *pb, int taglen, + AVDictionary **metadata) +{ + const char *key = "comment"; + uint8_t *dst; + int encoding, dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_VAL; + int language; + + if (taglen < 4) + return; + + encoding = avio_r8(pb); + language = avio_rl24(pb); + taglen -= 4; + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + return; + } + + if (dst && dst[0]) { + key = (const char *) dst; + dict_flags |= AV_DICT_DONT_STRDUP_KEY; + } + + if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { + av_log(s, AV_LOG_ERROR, "Error reading comment frame, skipped\n"); + if (dict_flags & AV_DICT_DONT_STRDUP_KEY) + av_freep((void*)&key); + return; + } + + if (dst) + av_dict_set(metadata, key, (const char *) dst, dict_flags); +} + +/** * Parse GEOB tag into a ID3v2ExtraMetaGEOB struct. */ static void read_geobtag(AVFormatContext *s, AVIOContext *pb, int taglen, @@ -908,6 +947,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, /* check for text tag or supported special meta tag */ } else if (tag[0] == 'T' || !memcmp(tag, "USLT", 4) || + !memcmp(tag, "COMM", 4) || (extra_meta && (extra_func = get_extra_meta_func(tag, isv34)))) { pbx = pb; @@ -975,6 +1015,8 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, read_ttag(s, pbx, tlen, metadata, tag); else if (!memcmp(tag, "USLT", 4)) read_uslt(s, pbx, tlen, metadata); + else if (!memcmp(tag, "COMM", 4)) + read_comment(s, pbx, tlen, metadata); else /* parse special meta tag */ extra_func->read(s, pbx, tlen, tag, extra_meta, isv34); -- 2.7.4 (Apple Git-66)
From: Chris Moeller <kode54@gmail.com> --- libavformat/mp3dec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..3055e2c 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,53 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } } +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; + + if (sscanf(de->value, "%x %x %x %llx %x %llx", &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6) { + *duration = 0; + return; + } + + mp3->start_pad = start_pad; + mp3->end_pad = end_pad; + if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset; + avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET); + for (i = 0; i < 8; i++) { + v = avio_rb32(s->pb); + if (ff_mpa_check_header(v) < 0) + return; + if (avpriv_mpegaudio_decode_header(c, v) != 0) + break; + *size += c->frame_size; + avio_skip(s->pb, c->frame_size - 4); + } +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +350,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; + unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; + uint64_t duration = 0; ffio_init_checksum(s->pb, ff_crcA001_update, 0); @@ -327,16 +376,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base); if (!mp3->frames && !mp3->header_filesize) + vbrtag_size = 0; + + mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + + if (!mp3->frames && !size && !duration) return -1; /* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET); - if (mp3->frames) + if (duration) + st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); + else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + if (size) { + if (duration) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); + else if (mp3->frames) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + } return 0; } -- 2.7.4 (Apple Git-66)
On Fri, Aug 05, 2016 at 03:54:25PM -0700, kode54@gmail.com wrote:
From: Chris Moeller <kode54@gmail.com>
--- libavformat/mp3dec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..3055e2c 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,53 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } }
+static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; +
+ if (sscanf(de->value, "%x %x %x %llx %x %llx", &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6) { + *duration = 0; + return; + }
libavformat/mp3dec.c: In function ‘mp3_parse_itunes_tag’: libavformat/mp3dec.c:318:5: warning: format ‘%llx’ expects argument of type ‘long long unsigned int *’, but argument 6 has type ‘uint64_t *’ [-Wformat] libavformat/mp3dec.c:318:5: warning: format ‘%llx’ expects argument of type ‘long long unsigned int *’, but argument 8 has type ‘uint64_t *’ [-Wformat] check for duration < 0 missing
+ + mp3->start_pad = start_pad; + mp3->end_pad = end_pad;
assigning unsigend to signed with no range checks could result in overflow, though even if it doesnt overflow the value should be checked to be within a sane range
+ if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; +
+ *size = (unsigned int) last_eight_frames_offset;
value could be truncated, missing range check
+ avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET);
missing seek faiure check also please provide a testcase/sample for this (a fate test would be even better) thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB You can kill me, but you cannot change the truth.
This hopefully fixes all of the shortcomings that were still present in the patch I last submitted. Unfortunately, I don't track the mailing list as closely as I should, so I missed the responses to my last round of patches. The first one made it in, though. There are reasonable limits applied to check the validity of the input values now, giving approximately 32 frames of lead and 64 frames of tail padding, with a rudimentary check on total file size before attempting the total file size check, and error checking on the seek operation. Christopher Snowhill (1): avformat: parse iTunes gapless information libavformat/mp3dec.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) -- 2.9.3 (Apple Git-75)
--- libavformat/mp3dec.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..47f4028 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,59 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } } +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; + + if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" %"PRIx64, &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6 || + duration < 0 || + start_pad > (576 * 2 * 32) || + end_pad > (576 * 2 * 64) || + last_eight_frames_offset >= (avio_size(s->pb) - base - vbrtag_size)) { + *duration = 0; + return; + } + + mp3->start_pad = (signed int) start_pad; + mp3->end_pad = (signed int) end_pad; + if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset; + if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET) < 0) + return; + + for (i = 0; i < 8; i++) { + v = avio_rb32(s->pb); + if (ff_mpa_check_header(v) < 0) + return; + if (avpriv_mpegaudio_decode_header(c, v) != 0) + break; + *size += c->frame_size; + avio_skip(s->pb, c->frame_size - 4); + } +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +356,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; + unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; + uint64_t duration = 0; ffio_init_checksum(s->pb, ff_crcA001_update, 0); @@ -327,16 +382,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base); if (!mp3->frames && !mp3->header_filesize) + vbrtag_size = 0; + + mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + + if (!mp3->frames && !size && !duration) return -1; /* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET); - if (mp3->frames) + if (duration) + st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); + else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + if (size) { + if (duration) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); + else if (mp3->frames) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + } return 0; } -- 2.9.3 (Apple Git-75)
On Thu, Dec 08, 2016 at 01:12:07PM -0800, Christopher Snowhill wrote:
--- libavformat/mp3dec.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-)
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..47f4028 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,59 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } }
+static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int i; +
+ if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return;
inconsistent indention
+ + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; + + if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" %"PRIx64, &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6 ||
+ duration < 0 ||
duration is a pointer
+ start_pad > (576 * 2 * 32) || + end_pad > (576 * 2 * 64) ||
+ last_eight_frames_offset >= (avio_size(s->pb) - base - vbrtag_size)) {
avio_size() could fail i think
+ *duration = 0; + return; + } +
+ mp3->start_pad = (signed int) start_pad; + mp3->end_pad = (signed int) end_pad;
useless casts
+ if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset;
+ if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET) < 0) + return;
tabs are not allowed in ffmpeg git [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange
Let's try this again. I can't believe I missed all those things you mentioned. It's almost as if I didn't actually proofread my patch before sending it.
Signed-off-by: Christopher Snowhill <kode54@gmail.com> --- libavformat/mp3dec.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 099ca57..b895e37 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,66 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } } +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int64_t temp_duration, file_size; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; + + file_size = avio_size(s->pb); + if (file_size < 0) + file_size = 0; + + if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" %"PRIx64, &zero, &start_pad, &end_pad, &temp_duration, &zero, &last_eight_frames_offset) < 6 || + temp_duration < 0 || + start_pad > (576 * 2 * 32) || + end_pad > (576 * 2 * 64) || + (file_size && (last_eight_frames_offset >= (file_size - base - vbrtag_size)))) { + *duration = 0; + return; + } + + *duration = temp_duration; + + mp3->start_pad = start_pad; + mp3->end_pad = end_pad; + if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset; + if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET) < 0) + return; + + for (i = 0; i < 8; i++) { + v = avio_rb32(s->pb); + if (ff_mpa_check_header(v) < 0) + return; + if (avpriv_mpegaudio_decode_header(c, v) != 0) + break; + *size += c->frame_size; + avio_skip(s->pb, c->frame_size - 4); + } +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +363,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; + unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; + uint64_t duration = 0; ffio_init_checksum(s->pb, ff_crcA001_update, 0); @@ -327,16 +389,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base); if (!mp3->frames && !mp3->header_filesize) + vbrtag_size = 0; + + mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + + if (!mp3->frames && !size && !duration) return -1; /* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET); - if (mp3->frames) + if (duration) + st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); + else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + if (size) { + if (duration) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); + else if (mp3->frames) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + } return 0; } -- 2.10.1.windows.1
On Thu, 26 Jan 2017 16:47:12 -0800 Christopher Snowhill <kode54@gmail.com> wrote:
Signed-off-by: Christopher Snowhill <kode54@gmail.com> --- libavformat/mp3dec.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-)
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 099ca57..b895e37 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,66 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } }
+static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int64_t temp_duration, file_size; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) + return; + + file_size = avio_size(s->pb); + if (file_size < 0) + file_size = 0; + + if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" %"PRIx64, &zero, &start_pad, &end_pad, &temp_duration, &zero, &last_eight_frames_offset) < 6 || + temp_duration < 0 || + start_pad > (576 * 2 * 32) || + end_pad > (576 * 2 * 64) || + (file_size && (last_eight_frames_offset >= (file_size - base - vbrtag_size)))) { + *duration = 0; + return; + } + + *duration = temp_duration; + + mp3->start_pad = start_pad; + mp3->end_pad = end_pad; + if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset; + if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET) < 0) + return; + + for (i = 0; i < 8; i++) { + v = avio_rb32(s->pb); + if (ff_mpa_check_header(v) < 0) + return; + if (avpriv_mpegaudio_decode_header(c, v) != 0) + break; + *size += c->frame_size; + avio_skip(s->pb, c->frame_size - 4); + } +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +363,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; + unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; + uint64_t duration = 0;
ffio_init_checksum(s->pb, ff_crcA001_update, 0);
@@ -327,16 +389,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base);
if (!mp3->frames && !mp3->header_filesize) + vbrtag_size = 0; + + mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + + if (!mp3->frames && !size && !duration) return -1;
/* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET);
- if (mp3->frames) + if (duration) + st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); + else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + if (size) { + if (duration) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); + else if (mp3->frames) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + }
return 0; }
If we add this, can we add a fate sample as well? The test should be similar to the other gapless tests.
It needs this patch first, but I have some example MP3 files that decode gaplessly with these patches applied. Encoded using the latest iTunes as of this writing (12.5.5.5) on macOS Sierra. Attaching the MP3 files, as well as source FLAC files, after this patch goes through.
From: Chris Moeller <kode54@gmail.com> --- libavformat/id3v2.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 9969d7a..f7fa3ef 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -823,6 +823,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, const ID3v2EMFunc *extra_func = NULL; unsigned char *uncompressed_buffer = NULL; av_unused int uncompressed_buffer_size = 0; + const char *comm_frame; av_log(s, AV_LOG_DEBUG, "id3v2 ver:%d flags:%02X len:%d\n", version, flags, len); @@ -834,12 +835,14 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, } isv34 = 0; taghdrlen = 6; + comm_frame = "COM"; break; case 3: case 4: isv34 = 1; taghdrlen = 10; + comm_frame = "COMM"; break; default: @@ -950,7 +953,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, /* check for text tag or supported special meta tag */ } else if (tag[0] == 'T' || !memcmp(tag, "USLT", 4) || - !memcmp(tag, "COMM", 4) || + !strcmp(tag, comm_frame) || (extra_meta && (extra_func = get_extra_meta_func(tag, isv34)))) { pbx = pb; @@ -1018,7 +1021,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, read_ttag(s, pbx, tlen, metadata, tag); else if (!memcmp(tag, "USLT", 4)) read_uslt(s, pbx, tlen, metadata); - else if (!memcmp(tag, "COMM", 4)) + else if (!strcmp(tag, comm_frame)) read_comment(s, pbx, tlen, metadata); else /* parse special meta tag */ -- 2.10.1 (Apple Git-78)
On Fri, Jan 27, 2017 at 01:20:31PM -0800, Christopher Snowhill wrote:
From: Chris Moeller <kode54@gmail.com>
--- libavformat/id3v2.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued
Combined with the ID3v2.2 COM frame fix I just submitted, these MP3 files will decode to a continuous signal. On 1/27/2017 4:10:54 AM, wm4 <nfxjfg@googlemail.com> wrote: On Thu, 26 Jan 2017 16:47:12 -0800 Christopher Snowhill wrote:
Signed-off-by: Christopher Snowhill --- libavformat/mp3dec.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-)
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 099ca57..b895e37 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,66 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } }
+static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int64_t temp_duration, file_size; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length (10 * 8 + 2 * 16 + 11)) + return; + + file_size = avio_size(s->pb); + if (file_size + file_size = 0; + + if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" %"PRIx64, &zero, &start_pad, &end_pad, &temp_duration, &zero, &last_eight_frames_offset) + temp_duration + start_pad > (576 * 2 * 32) || + end_pad > (576 * 2 * 64) || + (file_size && (last_eight_frames_offset >= (file_size - base - vbrtag_size)))) { + *duration = 0; + return; + } + + *duration = temp_duration; + + mp3->start_pad = start_pad; + mp3->end_pad = end_pad; + if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset; + if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET) + return; + + for (i = 0; i + v = avio_rb32(s->pb); + if (ff_mpa_check_header(v) + return; + if (avpriv_mpegaudio_decode_header(c, v) != 0) + break; + *size += c->frame_size; + avio_skip(s->pb, c->frame_size - 4); + } +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +363,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; + unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; + uint64_t duration = 0;
ffio_init_checksum(s->pb, ff_crcA001_update, 0);
@@ -327,16 +389,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base);
if (!mp3->frames && !mp3->header_filesize) + vbrtag_size = 0; + + mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + + if (!mp3->frames && !size && !duration) return -1;
/* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET);
- if (mp3->frames) + if (duration) + st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); + else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + if (size) { + if (duration) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); + else if (mp3->frames) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + }
return 0; }
If we add this, can we add a fate sample as well? The test should be similar to the other gapless tests. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Disregard those two, they were mistaken transcodes. These two are supposed to be gapless, however. Same signal, just not transcoded and downsampled first. On 1/27/2017 1:26:24 PM, Christopher Snowhill <kode54@gmail.com> wrote: Combined with the ID3v2.2 COM frame fix I just submitted, these MP3 files will decode to a continuous signal. On 1/27/2017 4:10:54 AM, wm4 <nfxjfg@googlemail.com> wrote: On Thu, 26 Jan 2017 16:47:12 -0800 Christopher Snowhill wrote:
Signed-off-by: Christopher Snowhill --- libavformat/mp3dec.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-)
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 099ca57..b895e37 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,66 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } }
+static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ + uint32_t v; + AVDictionaryEntry *de; + MP3DecContext *mp3 = s->priv_data; + size_t length; + uint32_t zero, start_pad, end_pad; + uint64_t last_eight_frames_offset; + int64_t temp_duration, file_size; + int i; + + if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) + return; + + length = strlen(de->value); + + /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ + if (length (10 * 8 + 2 * 16 + 11)) + return; + + file_size = avio_size(s->pb); + if (file_size + file_size = 0; + + if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" %"PRIx64, &zero, &start_pad, &end_pad, &temp_duration, &zero, &last_eight_frames_offset) + temp_duration + start_pad > (576 * 2 * 32) || + end_pad > (576 * 2 * 64) || + (file_size && (last_eight_frames_offset >= (file_size - base - vbrtag_size)))) { + *duration = 0; + return; + } + + *duration = temp_duration; + + mp3->start_pad = start_pad; + mp3->end_pad = end_pad; + if (end_pad >= 528 + 1) + mp3->end_pad = end_pad - (528 + 1); + st->start_skip_samples = mp3->start_pad + 528 + 1; + av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); + if (!s->pb->seekable) + return; + + *size = (unsigned int) last_eight_frames_offset; + if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET) + return; + + for (i = 0; i + v = avio_rb32(s->pb); + if (ff_mpa_check_header(v) + return; + if (avpriv_mpegaudio_decode_header(c, v) != 0) + break; + *size += c->frame_size; + avio_skip(s->pb, c->frame_size - 4); + } +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +363,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; + unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; + uint64_t duration = 0;
ffio_init_checksum(s->pb, ff_crcA001_update, 0);
@@ -327,16 +389,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base);
if (!mp3->frames && !mp3->header_filesize) + vbrtag_size = 0; + + mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + + if (!mp3->frames && !size && !duration) return -1;
/* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET);
- if (mp3->frames) + if (duration) + st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); + else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + if (size) { + if (duration) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); + else if (mp3->frames) + st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + }
return 0; }
If we add this, can we add a fate sample as well? The test should be similar to the other gapless tests. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Sat, Jan 28, 2017 at 04:09:20PM -0800, Christopher Snowhill wrote:
Disregard those two, they were mistaken transcodes. These two are supposed to be gapless, however. Same signal, just not transcoded and downsampled first.
where should they be uploaded to ? also is there a corresponding patch with fate test to test ? thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
I've finally assembled a fate test for this patch. The test file goes here: <fate-suite>/gapless/gapless-itunes.mp3 I have uploaded a copy here: https://f.losno.co/gapless-itunes.mp3
--- tests/fate/gapless.mak | 3 ++- tests/ref/fate/gapless-mp3-itunes | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 tests/ref/fate/gapless-mp3-itunes diff --git a/tests/fate/gapless.mak b/tests/fate/gapless.mak index 0253b9ec61..46b3b98c13 100644 --- a/tests/fate/gapless.mak +++ b/tests/fate/gapless.mak @@ -1,5 +1,6 @@ -FATE_GAPLESS-$(CONFIG_MP3_DEMUXER) += fate-gapless-mp3 +FATE_GAPLESS-$(CONFIG_MP3_DEMUXER) += fate-gapless-mp3 fate-gapless-mp3-itunes fate-gapless-mp3: CMD = gapless $(TARGET_SAMPLES)/gapless/gapless.mp3 +fate-gapless-mp3-itunes: CMD = gapless $(TARGET_SAMPLES)/gapless/gapless-itunes.mp3 FATE_GAPLESS-$(CONFIG_MP3_DEMUXER) += fate-audiomatch-square-mp3 fate-audiomatch-square-mp3: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/square3.mp3 $(TARGET_SAMPLES)/audiomatch/square3.wav diff --git a/tests/ref/fate/gapless-mp3-itunes b/tests/ref/fate/gapless-mp3-itunes new file mode 100644 index 0000000000..0337ca3e38 --- /dev/null +++ b/tests/ref/fate/gapless-mp3-itunes @@ -0,0 +1,5 @@ +45c78bcbfc9efddb3389ab518095728d *tests/data/fate/gapless-mp3-itunes.out-1 +596b51d8634e184d3a67b1cd072f7d73 +45c78bcbfc9efddb3389ab518095728d *tests/data/fate/gapless-mp3-itunes.out-2 +596b51d8634e184d3a67b1cd072f7d73 +c8cd71407ac5bbfadf5ccf7b5a64430c *tests/data/fate/gapless-mp3-itunes.out-3 -- 2.13.5 (Apple Git-94)
On Sun, Oct 01, 2017 at 08:13:11PM -0700, Christopher Snowhill wrote:
I've finally assembled a fate test for this patch.
The test file goes here:
<fate-suite>/gapless/gapless-itunes.mp3
I have uploaded a copy here:
uploaded to fate samples thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
On Fri, Aug 05, 2016 at 03:54:24PM -0700, kode54@gmail.com wrote:
From: Chris Moeller <kode54@gmail.com>
Let's try and get this right, one last time. No uninitialized variables, no problems with unsynchronized or compressed frames, etc.
--- libavformat/id3v2.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
applied thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued
participants (8)
-
Christopher Snowhill -
James Almer -
Josh de Kock -
kode54＠gmail.com
-
Michael Niedermayer -
Michael Niedermayer -
Nicolas George -
wm4