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

[Alexander Kojevnikov]

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.
-------------- next part --------------
From fea3f63d23d59d92a9e0b8380ef1386e30633937 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 stream size (and as a
result, 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          | 7 +++++--
 tests/ref/lavf-fate/mp3       | 4 ++--
 3 files changed, 9 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..a6132e4 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_xing;
 
     v = avio_rb32(s->pb);
     if(ff_mpa_check_header(v) < 0)
@@ -135,11 +136,13 @@ 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_xing = v == MKBETAG('X', 'i', 'n', 'g');
+    if (is_xing || v == MKBETAG('I', 'n', 'f', 'o')) {
         v = avio_rb32(s->pb);
         if(v & XING_FLAG_FRAMES)
             frames = avio_rb32(s->pb);
-        if(v & XING_FLAG_SIZE)
+        /* Don't set size/bitrate for CBR streams */
+        if (v & XING_FLAG_SIZE && is_xing)
             size = avio_rb32(s->pb);
         if (v & XING_FLAG_TOC && frames)
             read_xing_toc(s, size, av_rescale_q(frames, (AVRational){spf, c.sample_rate},
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