diff mbox

[FFmpeg-devel,PATCHv3,2/2] avformat: parse iTunes gapless information

Message ID 1470437665-78199-2-git-send-email-kode54@gmail.com
State Changes Requested
Headers show

Commit Message

kode54@gmail.com Aug. 5, 2016, 10:54 p.m. UTC
From: Chris Moeller <kode54@gmail.com>

---
 libavformat/mp3dec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer Aug. 7, 2016, 10:06 a.m. UTC | #1
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

[...]
kode54@gmail.com Dec. 8, 2016, 9:12 p.m. UTC | #2
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(-)
diff mbox

Patch

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;
 }