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

Alexander Kojevnikov alexander at kojevnikov.com
Tue Mar 5 07:22:24 CET 2013


On Mon, Mar 4, 2013 at 3:00 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Mar 04, 2013 at 10:11:52AM -0800, Alexander Kojevnikov wrote:
>> 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.
>
>> 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.
>
> This patch worsense the duration and bitrate estimation for some vbr
> files like:
> http://usr.bandzone.cz/test/mp3/duration-problem.mp3
> It changes from
> Duration: 00:02:27.41, start: 0.000000, bitrate: 192 kb/s
> to
> Duration: 00:11:47.56, start: 0.000000, bitrate: 40 kb/s
>
> i guess this is because after the patch the first mp3 frame is used
> whiel previously a later was used.
> I suspect the first mp3 frame is in general a poor estimation for the
> whole file (often more silence at the begin)
>
> maybe the average of the first few could be used or a random later or
> something ?

I did the mean bit rate calculation, makes a lot more sense than
picking a random frame's bit rate when a Xing tag is not present.

For the given sample it results in:
  Duration: 00:02:47.48, start: 0.000000, bitrate: 168 kb/s
-------------- next part --------------
From 26a6f08dd98c148b7a78a9eee7bb79158b1a4a7e 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,
otherwise calculate the mean bit rate from parsed frames. 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 | 10 +++++++---
 libavformat/mp3dec.c          |  6 ++++--
 tests/ref/lavf-fate/mp3       |  4 ++--
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
index 5f97d71..bb4d75c 100644
--- a/libavcodec/mpegaudio_parser.c
+++ b/libavcodec/mpegaudio_parser.c
@@ -30,6 +30,7 @@ typedef struct MpegAudioParseContext {
     int frame_size;
     uint32_t header;
     int header_count;
+    int no_bitrate;
 } MpegAudioParseContext;
 
 #define MPA_HEADER_SIZE 4
@@ -74,15 +75,18 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
                     if((state&SAME_HEADER_MASK) != (s->header&SAME_HEADER_MASK) && s->header)
                         s->header_count= -3;
                     s->header= state;
-                    s->header_count++;
                     s->frame_size = ret-4;
 
-                    if (s->header_count > 1) {
+                    if (s->header_count > 0) {
                         avctx->sample_rate= sr;
                         avctx->channels   = channels;
                         s1->duration      = frame_size;
-                        avctx->bit_rate   = bit_rate;
+                        if (s->no_bitrate || !avctx->bit_rate) {
+                            s->no_bitrate = 1;
+                            avctx->bit_rate += (bit_rate - avctx->bit_rate) / s->header_count;
+                        }
                     }
+                    s->header_count++;
                     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..361314b 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
+7fcf80c2059b5c058a6cdd2e2f798b6c *./tests/data/lavf-fate/lavf.mp3
+96366 ./tests/data/lavf-fate/lavf.mp3
 ./tests/data/lavf-fate/lavf.mp3 CRC=0x6c9850fe
-- 
1.8.1.3


More information about the ffmpeg-devel mailing list