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

[Michael Niedermayer]

On Tue, Mar 05, 2013 at 08:11:07AM -0800, Alexander Kojevnikov wrote:
> 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

locally done

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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20130307/84a25bbc/attachment.asc>