[FFmpeg-devel] [PATCH] mp3dec: Fix VBR bit rate parsing

[Alexander Kojevnikov]

On Thu, Feb 28, 2013 at 2:04 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Wed, Feb 27, 2013 at 10:07:52PM -0800, Alexander Kojevnikov wrote:
>> On Wed, Feb 27, 2013 at 3:56 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Tue, Feb 26, 2013 at 10:28:42PM -0800, Alexander Kojevnikov wrote:
>> >> Commit 6776a8f[0] introduced a regression in calculation[1] of the bit
>> >> rate of VBR streams. Instead of keeping the bit rate from the Xiph
>> >> tag, it now overrides it during parsing with the bit rate from one of
>> >> the frames.
>> >>
>> >> Attached patch should fix it. It relies on the assumption[2] that the
>> >> Xiph/Info tag has "Xiph" id string for VBR streams and "Info" for CBR.
>> >
>> > xiph ?
>>
>> My apologies, it's "Xing", not "Xiph".
>>
>> > and it seems the patch breaks "make fate"
>> > --- ./tests/ref/lavf-fate/mp3   2013-02-26 02:17:04.526545833 +0100
>> > +++ tests/data/fate/lavf-fate-mp3       2013-02-27 12:50:36.909166861 +0100
>> > @@ -1,3 +1,3 @@
>> > -40a4e41ae74ec8dacdf02402831a6a58 *./tests/data/lavf-fate/lavf.mp3
>> > -97230 ./tests/data/lavf-fate/lavf.mp3
>> > +98ed29febe5ddfe85eef0d3460701141 *./tests/data/lavf-fate/lavf.mp3
>> > +95970 ./tests/data/lavf-fate/lavf.mp3
>> >  ./tests/data/lavf-fate/lavf.mp3 CRC=0x6c9850fe
>>
>> I checked both generated files, the difference is only in the first
>> frame containing the Xing tag, the length and content of which is
>> driven by the previously deducted bit rate. The first frame of the
>> underlying VBR mp3 stream has a bit rate of 32 kbps, while the last
>> looked up frame has a bit rate 320 kbps. The muxer selects the size of
>> the first frame (to contain the Xing tag) depending on the deducted
>> bit rate, both sizes are equally correct. I updated the test file to
>> reflect this.
>
> This patch still breaks some of my mp3 files
> try testcase.mp3 from the lame repository as example (but it affects
> other files too)
> http://lame.cvs.sourceforge.net/viewvc/lame/lame/testcase.mp3?view=log
>
> before the patch it seems the initial skiping of samples works:
> [mp3 @ 0x28d21a0] pad 576 920
> [mp3 @ 0x28d21a0] demuxer injecting skip 1105
> [mp3 @ 0x28d2a00] skip 1105 samples due to side data
> [mp3 @ 0x28d2a00] skip 1105/1152 samples
> afterwards:
> nothing
>
> if you decode it to .wav the size also changes

Good catch, attached patch should fix this.
-------------- next part --------------
From 1a4df01cf3bab5c3662fc011df5adb3be0dee4b3 Mon Sep 17 00:00:00 2001
From: Alexander Kojevnikov <alexander at kojevnikov.com>
Date: Tue, 26 Feb 2013 21:47:11 -0800
Subject: [PATCH] mp3dec: Fix VBR bit rate parsing

When parsing the Xing/Info tag, don't set the bit rate if it's an Info tag.

When parsing the stream, don't override the bit rate if it's already set.
This way, the bit rate will be set correctly both for CBR and VBR streams.

Signed-off-by: Alexander Kojevnikov <alexander at kojevnikov.com>
---
 libavcodec/mpegaudio_parser.c | 3 ++-
 libavformat/mp3dec.c          | 6 ++++--
 tests/ref/lavf-fate/mp3       | 4 ++--
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
index 5f97d71..8869eb0 100644
--- a/libavcodec/mpegaudio_parser.c
+++ b/libavcodec/mpegaudio_parser.c
@@ -81,7 +81,8 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
                         avctx->sample_rate= sr;
                         avctx->channels   = channels;
                         s1->duration      = frame_size;
-                        avctx->bit_rate   = bit_rate;
+                        if (!avctx->bit_rate)
+                            avctx->bit_rate = bit_rate;
                     }
                     break;
                 }
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index c6d6987..57e4ba3 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -120,6 +120,7 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base)
     const int64_t xing_offtbl[2][2] = {{32, 17}, {17,9}};
     MPADecodeHeader c;
     int vbrtag_size = 0;
+    int is_cbr;
 
     v = avio_rb32(s->pb);
     if(ff_mpa_check_header(v) < 0)
@@ -135,7 +136,8 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base)
     /* Check for Xing / Info tag */
     avio_skip(s->pb, xing_offtbl[c.lsf == 1][c.nb_channels == 1]);
     v = avio_rb32(s->pb);
-    if(v == MKBETAG('X', 'i', 'n', 'g') || v == MKBETAG('I', 'n', 'f', 'o')) {
+    is_cbr = v == MKBETAG('I', 'n', 'f', 'o');
+    if (v == MKBETAG('X', 'i', 'n', 'g') || is_cbr) {
         v = avio_rb32(s->pb);
         if(v & XING_FLAG_FRAMES)
             frames = avio_rb32(s->pb);
@@ -180,7 +182,7 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base)
     if(frames)
         st->duration = av_rescale_q(frames, (AVRational){spf, c.sample_rate},
                                     st->time_base);
-    if(size && frames)
+    if (size && frames && !is_cbr)
         st->codec->bit_rate = av_rescale(size, 8 * c.sample_rate, frames * (int64_t)spf);
 
     return 0;
diff --git a/tests/ref/lavf-fate/mp3 b/tests/ref/lavf-fate/mp3
index 91e2b48..c0e055f 100644
--- a/tests/ref/lavf-fate/mp3
+++ b/tests/ref/lavf-fate/mp3
@@ -1,3 +1,3 @@
-40a4e41ae74ec8dacdf02402831a6a58 *./tests/data/lavf-fate/lavf.mp3
-97230 ./tests/data/lavf-fate/lavf.mp3
+98ed29febe5ddfe85eef0d3460701141 *./tests/data/lavf-fate/lavf.mp3
+95970 ./tests/data/lavf-fate/lavf.mp3
 ./tests/data/lavf-fate/lavf.mp3 CRC=0x6c9850fe
-- 
1.8.1.3