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

[Alexander Kojevnikov]

On Tue, Mar 5, 2013 at 4:15 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Mar 04, 2013 at 10:22:24PM -0800, Alexander Kojevnikov wrote:
>> 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
>
>> 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.
>
> applied

Thank you Michael!

Because this commit fixes a regression, could you also apply it to the
affected release branches:

% git branch -r --contains 6776a8f
  origin/HEAD -> origin/master
  origin/master
  origin/release/0.11
  origin/release/1.0
  origin/release/1.1