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

[Michael Niedermayer]

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

thanks

[.,..]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130305/37c9232a/attachment.asc>