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

[Michael Niedermayer]

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 ?

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/4007bdaa/attachment.asc>