Bug 130185 - Incorrect parsing of MPEG frame length
Summary: Incorrect parsing of MPEG frame length
Status: RESOLVED FIXED
Alias: None
Product: taglib
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Scott Wheeler
URL:
Keywords:
: 127056 130625 143988 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-07-03 12:30 UTC by Martyn Honeyford
Modified: 2008-02-24 22:00 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
return correct (twice as long) play time for single channel mp3 files (1.58 KB, patch)
2006-09-14 22:39 UTC, Martin Aumueller
Details
Correctly read channel mode from mpeg frame header. (616 bytes, patch)
2006-12-26 23:25 UTC, David Dent
Details
fixs song length detection of MP3 VBR files (952.10 KB, patch)
2007-02-04 22:49 UTC, Tobias Rafreider
Details
Improved version of taglib-1.4 hack (952.44 KB, patch)
2007-02-06 04:00 UTC, Tobias Rafreider
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martyn Honeyford 2006-07-03 12:30:03 UTC
Version:           1.4.1-beta1 (using KDE 3.5.3, Gentoo)
Compiler:          gcc version 3.4.6 (Gentoo 3.4.6-r1, ssp-3.4.5-1.0, pie-8.7.9)
OS:                Linux (i686) release 2.6.17-gentoo-r1

I have downloaded several podcasts using Amarok and they appear to be truncated in an odd way.
An example is Season 3 Episode 18 of LUG radio, from;
http://lugradio.org/episodes.rss

If you look at the show notes, it should be 89:47 long.
When downloaded, it "appears" to be 45:39 long.
If I transfer the file to my ipod, it stops after 45:39.
Similarly if I play the file with mplayer, it stops after 45:39.
In amarok itself, it states the length as 45:39, but it continutes to play the rest of the file (and the counter no longer increases).
Similarly if I play the file in xmms, it plays the rest of the file!
So it appears that the whole show has been downloaded, but there is something wrong with the file.  
I will download the file directly to see if it player correctly in mplayer (I am on a slow internet connection at the moment).
This also happened with the latest version of the 1upyours podcast (1upyours.1up.com) - note it was at a different length and only the last couple of minutes were missing.
Comment 1 Mark Kretschmann 2006-07-03 12:53:25 UTC
Which format was it, MP3?
Comment 2 Martyn Honeyford 2006-07-03 13:00:24 UTC
Sorry, yes mp3.  I can probably send u the file if it would help.
Comment 3 Mark Kretschmann 2006-07-03 15:54:31 UTC
Ok, this probably means they're using VBR (variable bit rate), without proper XING header. In this case the player can only guess the correct length.

Use the OGG version instead, or tell them to fix their MP3's.
Comment 4 Martyn Honeyford 2006-07-04 15:00:48 UTC
Thanks for the quick response.
I have done a bit more testing and it would appear that the 2 podcasts I have tried so far (LUGRadio and 1upyours) both use VBR mp3 files without XING headers as you suggested.

As a further test, I downloaded the latest 1upyours podcast (http://zdmedia.vo.llnwd.net/o1/Podcasts/063006.mp3) to my hard drive as a plain mp3 and then used Amarok to xfer it to my ipod, just as a regular mp3 not a podcast.
As expected it was truncated when I played it.
I then booted into Windows and used iTunes to transfer the exact same file to my ipod and it played the whole thing fine with no truncation.
I figured that iTunes must have fixed up the file, so I rebooted back into Linux, and used Amarok to get the file off the ipod again and diffed it with the original file and they were identical!

So it would seem that the ipod is perfectly capable of playing the file correctly just that Amarok is incorrectly setting the length of the file (I assume that this is set in the database somewhere).

I take the point that the files in question are at fault, but I think that this is something that Amarok should really try to work around as I strongly expect that there are going to be a lot of podcasts like this (as I said, both of the ones I have tried so far have been like it - one of which is a linux podcast!) and I would guess that most podcast creators aren't going to be particularly interested in fixing them if they work on iTunes.

Incidently, I tried the latest mp3fix.rb script on the LUG show which made more of the show play, but it was still truncated.  
Comment 5 Mark Kretschmann 2006-07-04 15:15:37 UTC
In the case of LUGradio it's a bit silly though, wouldn't you agree? :) (then again the whole show is silly, so it fits)
Comment 6 Martyn Honeyford 2006-07-04 23:42:52 UTC
Hehe yeah - they do provide a CBR "compatability" version, but its not in the rss feed so doesn't help much if you want to use it as a "true" podcast.
I'll drop them an email to see if they will make an RSS feed for the CBR version, but as I said, I expect its going to be a very common problem.
Comment 7 Martyn Honeyford 2006-07-05 07:39:16 UTC
I don't know if it will help, but I tried a couple of mp3 fixing/checking programs with the latest 1upshow and I came across mp3_check (http://sourceforge.net/projects/mp3check/) which was able to determine the correct length.  It is written in C, GPLed and only about 2k of code in total, so I expect it should be reasonably easy to integrate into amarok.
I had a quick peek through the amarok source, but looked a bit too in-depth for me to work out in 5-10 mins.
Comment 8 Martin Aumueller 2006-08-05 03:33:17 UTC
*** Bug 127056 has been marked as a duplicate of this bug. ***
Comment 9 Tobias Niwi 2006-08-30 13:06:34 UTC
I have this problem with nearly all podcasts I am listening to. E.g. all postcasts by the popular German radio stations Deutschlandradio and Deutschlandfunk are affected. It might be possible that even all German radio stations under public law (öffentlich-rechtlich) are affected. This is a show stopper. This makes Amarok useless as a podcast-client at least in Gemany. And while this is true quite many of these podcasts recommend Amarok as client for Linux. As a result this even means bad advertisement for Linux.
Comment 10 Snarkout 2006-09-10 21:31:38 UTC
I have the same problem - I'll see a few seconds to a few minutes out of an hour long show.  Amarok will generally play the entire podcast once downloaded, though, but continues to play with the progress bar pegged to the right hand side of the slider.  My iPod does not - it honors the time amarok reports.

This is also affecting the progress bar for the download as well in my case.  I can download the same 'cast repeatedly (after deleting) and the progress for the DL hangs at the same spot each time.  Looking at network activity confirms that the download is still taking place though.  The download exits normally, with no errors.  

As others have mentioned, other players (mplayer, for example) display the correct time.  So far I have had no time-related problems if I download these 'casts via another means and move them to my amarok playlist manually.

Using:
Arch Linux "current" fully updated.
Amarok 1.4.2 - this has been an issue prior to this release though.
xine engine
Comment 11 Mark Kretschmann 2006-09-14 17:38:32 UTC
Invalid, as detailed above (VBR without XING).
Comment 12 Tobias Niwi 2006-09-14 19:04:39 UTC
If tis means that the problem will not be solved by Amarok it means that Amarok is usless for receiving podcasts by most German radio stations. I hope they will fix their MP3-files. Until then I will stopp using Amarokm for podcasts. It just doesn't make sense to miss regulary the last part of every radio show.
Comment 13 Martin Aumueller 2006-09-14 22:39:48 UTC
Created attachment 17773 [details]
return correct (twice as long) play time for single channel mp3 files
Comment 14 Martin Aumueller 2006-09-14 22:43:52 UTC
It seems that all the affected mp3 files are single channel/mono files without xing header.

Simply doubling the returned length in the single channel case made the returned play length agree with the values returned by the mp3check tool mentioned above. The attached patch does this. Scott, if you are ok with this, I could commit this change.
Comment 15 Martin Aumueller 2006-09-23 18:30:27 UTC
Please forget comments #13/#14: the attached patch just reproduces a bug present in mp3_check 1.83.
Comment 16 Valent Turkovic 2006-12-18 10:16:57 UTC
Can you please try to fix this, because AFAIK lugradio is the most listener linux podcast, so it affects lots of linux users.

All other stand-alone players (ie. not podcast clients) like xmms, audacious... and others play all lugradio (mono vbr) files with correct lenght?

How can they see the real lenght even without XING headers? Can't you make amarok do the same?

Thank you very much in advance.
Comment 17 Valent Turkovic 2006-12-20 10:33:49 UTC
Please check the comment from lame developes at:
https://sourceforge.net/tracker/?func=detail&atid=100290&aid=1618205&group_id=290
Comment 18 Jeff Mitchell 2006-12-23 15:55:43 UTC
After some discussion with developers, we've decided to mark this invalid.  The Xing header exists for exactly this purpose and is supported by TagLib.  It's about ten years old and there's no reason any modern encoder doesn't put it on.

If we did a workaround to calculate the length from all the frames, it could slow down collection scanning by a good deal at best and several orders of magnitude at worst, depending on how many broken VBR MP3s a person has and what filesystems they're stored on.

The best thing for you to do is swamp LugRadio with complaints and get them to use a good encoder.  They're a Linux podcast...why aren't they using LAME, which besides having excellent sound quality, does support adding Xing headers?  (All my music is encoded this way with --vbr-new, and I have verified that the Xing headers do exist and the correct length is calculated.)

As a side fix, take a look at markey's mp3fixer script at

http://www.kde-apps.org/content/show.php?content=31539

This will fix broken frames and add Xing headers to your VBR files if they don't exist already.  Be warned that you should back up any important MP3s you run this on first, as in very rare cases it can cause some damage.
Comment 19 Valent Turkovic 2006-12-24 16:17:59 UTC
Please read the post at:
http://forums.lugradio.org/viewtopic.php?t=3034

as you can see they do use lame encoder, and I tested it myself. I decoded their mp3 file to wav and then encoded it with lame with various options for encoding like:
--preset voice
-V 8 --vbr-new
-V 9 --vbr-new 

each mp3 file that lame created came didn't show correct time lenght in amarok. I'm not exprienced as you so can you explain me why? Does lame use XING headers? Does it put wrong headers? What is going on here?

All other players I tested like audacious, xmms, beep... and others show correct lenght, I guess that they do the calculation, and they don't take XING headers for granted.
Comment 20 Robert Hegemann 2006-12-25 03:25:51 UTC
taglib/mpeg/mpegproperties.cpp:
the calculation of "timePerFrame" is wrong in case of MPEG-2(.5) Layer3:

Each granule in Layer3 represents 576 PCM samples
MPEG-1 layer3 frames consist of two granules, equals 1152 PCM samples
MPEG-2(.5) layer3 frames consist of one granule only, equals 576 PCM samples

current taglib code assumes 1152 pcm samples in all cases, which is obviously wrong.

Ciao Robert (LAME developer)
Comment 21 Alexandre Oliveira 2006-12-25 04:00:40 UTC
Reopening
Comment 22 Alexandre Oliveira 2006-12-25 04:01:43 UTC
Reassinging to taglib, due to information on comment #20, thanks Robert Hegemann.
Comment 23 Valent Turkovic 2006-12-25 22:21:46 UTC
Yipeee :)))
I'm so happy this bug is going to be fixed!

Thank you Robert! 

To all of you happy Christmas and New Year!

Thank you all for your great effort in making gnu/linux a great OS that it is... and it's great apps.

Valent. 
Comment 24 David Dent 2006-12-26 20:52:06 UTC
In addition to the bug in #20.

taglib/mpeg/mpegheader.cpp

in method

void MPEG::Header::parse()

  // The channel mode is encoded as a 2 bit value at the end of the 3nd byte,
  // i.e. xxxxxx11

  d->channelMode = ChannelMode(uchar(data[2]) & 0x3);

It appears this is not correctly setting the channel mode to mono with mono files.

This causes a problem because the xing header offset differers for mono files.
taglib/mpeg/mpegproperties.cpp
in method
void int xingHeaderOffset MPEG::XingHeader::xingHeaderOffset(firstHeader.version(), firstHeader.channelMode());

xingHeaderOffset is set incorrectly for at least mpegv1 single channel files. It is set to 0x24 instead of 0x15.
As the xingHeaderOffset is incorrect the xing header is not loaded and the file is assumed to be cbr and the track length is calculated incorrectly.
Comment 25 David Dent 2006-12-26 23:25:37 UTC
Created attachment 19038 [details]
Correctly read channel mode from mpeg frame header.

As documented here
http://www.codeproject.com/audio/MPEGAudioInfo.asp#MPEGAudioFrameHeader

Channel mode is bit 24 and 25, the first two bits of the forth byte not the
last two bits of the third. Now single channel files are identified and the
correct xing offset used. This allows track lengths to be calculated correctly.
Comment 26 Robert Hegemann 2006-12-26 23:55:23 UTC
commenting #24:

in case of MPEG-2(.5) the offset is even different

MPEG-1     4+17(1ch) 4+32(2ch)
MPEG-2(.5) 4+ 9(1ch) 4+17(2ch)

4 bytes SYNC word plus 9, 17 or 32 bytes used for sideinfo

A Xing header is some valid MP3 frame which decodes to silence, the Xing-data follows after the sideinfo within the so called "ancillary data"

Caution: in case CRC protection is used, Xing header data does not compensate this and will start at the positions as noted above (2 bytes too early, but that's legacy and we have to accept it)!
Comment 27 Ljubomir 2006-12-27 10:37:06 UTC
*** This bug has been confirmed by popular vote. ***
Comment 28 David Dent 2006-12-30 06:46:09 UTC
Sorry the bug in #24 was fixed in svn in march 06
http://websvn.kde.org/trunk/kdesupport/taglib/mpeg/mpegheader.cpp?rev=515068&r1=288617&r2=515068
However the comment is out of sync with the code.
Comment 29 Valent Turkovic 2007-01-23 12:10:29 UTC
So can you explain to me what is being done to fix this bug? Does new version of taglib needs to be released with patch so that it fixes this problem?
Comment 30 Tobias Rafreider 2007-02-04 22:45:44 UTC
During the last weeks I put many efforts into hacking taglib.
I am happy to announce a taglib-1.4 hack release for that issue.

All MP3 VBR files should now have a correct song length, thus making it possible to use Amarok as an Ipod synchronizing tool.

Currently Amarok complies with taglib 1.4. My hack is binary compatible to version 1.4. Amarok does not need to be recompiled.

What has been done?
- The class MPEG::Header was not correctly implemented. Invalid frames were treated as correct although they were not valid. This is fixed now

-  The methods for detecting frameheaders in class MPEG::File have 
been fixed. Especially the "nextFrameOffset" method has never been working well. 

- MPEG::Properties::read() can now handle Mp3 VBR files better. 
  
Comment 18 refers to the XING Header. Not all VBR files have a XING Header. I use streamripper to record webradio. Most of these files are VBR without such a header because you do not know the song length in advance.

Performance is one thing but functionality another. If you are not able to synchronize VBR files with an Ipod (e.g. Amarok) users get disappointed. 
I implemented a trade-off in my hack to maintain performance. Files without XING Headers can be CBR or VBR. The avg. BitRate of 100 frames in the middle of a song are used to detect the song length now. Tests on my machine have shown that Amarok has still acceptable performance!

I really do not understand why core developers have ignored this issue. Comment 18 is a bad excuse. All other taglib independent players like XMMS, XINE, RealPlayer can handle this. Again Performance is one thing but functionality another!!!

You can find my hack on http://www.rafreider.de/taglib-1.4-vbr-hack.tar.gz

I hope that my hack can flow in trunk.
Comment 31 Tobias Rafreider 2007-02-04 22:49:29 UTC
Created attachment 19556 [details]
fixs song length detection of MP3 VBR files
Comment 32 Tobias Niwi 2007-02-05 11:29:59 UTC
I tested the hack by Tobias Rafreider this morning. I discovered two problems. I don't know if they are related to this bug from a technical point of view but from a user perspective they do.

I use the following podcast-feed with Amarok: http://www.dradio.de/rss/podcast/sendungen/politischesbuch/
When playing one of the radio-show from this feed (http://ondemand-mp3.dradio.de/podcast/2007/02/02/dkultur_200702021749.mp3), several problems occur:

1. It still ends 1 second too early in Amarok (7:35 min instead of 7:36 min). Audacity plays the same file correctly. While 1 missing second for music is only a minor problem, for talk radio it can be a show stopper. Final statements often are said in the last seconds. Also it is irritating when Amarok stops in the middle of a sentence. The listener never knows, how much he or she is missing. 

2. Taglib gives wrong information about file length (1:50 min. instead of 7:36 min). I get this wrong Information in Amarok as well as in Konqueror.

3. When downloading the file with Amarok the length slider already stops at 1:50 min. However, the file is played up to 7:35 min.

This problem is not only related to the reported file but occurs also with many or even all other podcasts from http://www.dradio.de.
Comment 33 Mark Kretschmann 2007-02-05 13:30:46 UTC
On Monday 05 February 2007 11:30, Tobias Niwi wrote:
> 1. It still ends 1 second too early in Amarok (7:35 min instead of 7:36
> min).


xinelib always cuts the last second off. This has nothing to do with taglib.
Comment 34 Tobias Rafreider 2007-02-06 04:00:17 UTC
Created attachment 19564 [details]
Improved version of taglib-1.4 hack

MP3 files encoded as MPEG2 are now better supported
Comment 35 Tobias Rafreider 2007-02-06 04:04:06 UTC
Comment on attachment 19564 [details]
Improved version of taglib-1.4 hack

MP3 files encoded as MPEG2 are now better supported. 

The podcast stream should now work fine!
Comment 36 Valent Turkovic 2007-02-06 12:43:43 UTC
I have Fedora Core 6 and the problem is still present with latest updates.
I have taglib.i386 1.4-5.fc6

Is there an newer version awailable so that fedora team can make new rpms for taglib ?

Thank you.
Comment 37 Valent Turkovic 2007-02-06 12:44:22 UTC
I have Fedora Core 6 and the problem is still present with latest updates.
I have taglib.i386 1.4-5.fc6

Is there an newer version awailable so that fedora team can make new rpms for taglib ?

Thank you.
Comment 38 Valent Turkovic 2007-04-04 23:50:04 UTC
Can you please tell me what is going on regarding this issue? As I understand it - it is fixed but via a hack that is not in upstream main source code.

Please make this bug fix applied in latest source code and PLEASE make new version available of taglib as soon as possible.

We need this right now because Fedora 7, SUSE 10.3 and new Ubuntu are right around the corner and I would like all new linux distros to have WORKING podcast client - ie. Amarok.

So please, please, pretty please make this available.

I'm really very grateful for all of your hard work that you put in this project!
Comment 39 Scott Wheeler 2007-04-05 08:46:09 UTC
*** Bug 130625 has been marked as a duplicate of this bug. ***
Comment 40 Valent Turkovic 2007-04-05 09:28:26 UTC
regarding comment #35 by Tobias Rafreider  

Hmm, I see that the attachement is a file called "attachement.cgi" What do I do with it? I was expecting a patch file which I would apply to the source if taglig. I don't know that to do with this .cgi file.
Comment 41 Mark Kretschmann 2007-04-05 11:28:56 UTC
Rename it to foo.diff.
Comment 42 Valent Turkovic 2007-04-05 12:10:18 UTC
On 5 Apr 2007 09:28:58 -0000, Mark Kretschmann <markey@web.de> wrote:
[bugs.kde.org quoted mail]
$ cp attachement.cgi taglib.patch
$ tar xvzf taglib-1.4.tar.gz
$ cp taglib.patch taglib-1.4/
$ cd taglib-1.4
patch -p0 < taglib.patch

then a bunch of gibberish

The taglib.patch is a binary file, that sounds fishy to me.

How do I apply this patch?
Comment 43 richlv 2007-04-05 12:17:53 UTC
`file taglib.patch`

it probably is gzipped or bzipped
Comment 44 Valent Turkovic 2007-04-05 18:03:55 UTC
> `file taglib.patch`
>
> it probably is gzipped or bzipped
>


You were right, so stupid of me not to remembeu using file command :(
Comment 45 Mark Kretschmann 2007-04-09 10:21:27 UTC
*** Bug 143988 has been marked as a duplicate of this bug. ***
Comment 46 Valent Turkovic 2007-06-15 19:22:42 UTC
Will you please, please, please release a new and patched version of taglib - there is an really big issue with taglib and linux podcasts - if you produce linux podcast they all have wrong playback length and issues on most podcast players on linux and also in amarok and also on ipods! I can listen only to half of the show and now the whole podcast show because of this bug.

I heard that fix is in place and only thing I need is to wait for new version of taglib to be pushed out... and it's been months and still this hasn't happened and all distros still distribute the buggy taglib version. Please, please, please make it available so that all mayor linux distros can include it and so that producing and playback of audio podcasts (and other audio materials) on linux works ok, as it does on other platforms.

Thank you for your hard work in these two years and for providing opensource community this great tool called taglib!
Comment 47 Lukáš Lalinský 2007-11-22 19:55:55 UTC
SVN commit 740177 by lalinsky:

Use correct frame sizes when calculating length of MPEG 2 or 2.5 streams.

CCBUG:130185


 M  +10 -2     taglib/mpeg/mpegproperties.cpp  
 M  +1 -0      tests/CMakeLists.txt  
 M  +1 -0      tests/Makefile.am  
 AM            tests/data/mpeg2.mp3  
 A             tests/test_mpeg.cpp   [License: UNKNOWN]


WebSVN link: http://websvn.kde.org/?view=rev&revision=740177
Comment 48 Valent Turkovic 2007-12-29 21:00:27 UTC
Nobody noticed that taglib 1.5 got released and this bug is fixed, I tested it with only one MP3 file I had problems earlier so please test more but it seams fixed.

Valent.
Comment 49 Valent Turkovic 2007-12-29 21:12:23 UTC
Nope it is not fixed :( I used a fixed rate MP3 and now then I tested it with variable rate mono file I still see this bug :(
Comment 50 Valent Turkovic 2007-12-29 21:19:35 UTC
why aren't there been patches applied to fix this issue even the SVN code has this bug AFAIK. Why? 

Is has passed more than a year since this bug was discovered, and it exists probably since begining and it still isnt' fixed. Why?
Comment 51 Scott Wheeler 2008-01-31 00:38:40 UTC
This is fixed in SVN.