cmus high CPU usage issue (2)

[zxc]

Hi there,

I think I've found a bug in the cmus player.

Several days ago I found this player and started importing my mp3s to the
library but I noticed that at some point the program stops counting the
seconds below (total duration) and my CPU usage goes to 50 percent and stays
there for as long as the program is running. And when this happens when I
try to quit the player it asks me if I am sure that I want to do that
because it is still importing files. I say 'y' and the program hangs. After
that I have to call `killall cmus` to kill the process. After some digging I
found the problematic mp3 file. I think that the file is somehow broken but
it plays well with Audacious and several other players.

I saw that there is 300kb message body limit that's why I uploaded the
failing mp3 file here
http://www.4shared.com/file/iEY_IbRp/failing-mp3.html

Steps to reproduce:
1) unzip the mp3 file
2) start cmus and try to add the unzipped mp3 file to the library.
3) pay attention to the CPU usage (with top for example)
Also you can try to play the file. I had problems with that too.

I think that an expected behaviour would be to skip this file during import
if it cannot be read.

I use cmus v2.4.0 on a 64bit (amd64) Debian distro, kernel 2.6.32-5.

If you need more information please contact me.

Regards,
Vladimir

[Johannes Weißl]

Hi Vladimir,

Post by zxc
I think I've found a bug in the cmus player.

Correct, thanks very much for reporting! The fix will make it to 2.4.1
:-). The problem was that id3.c contains a incorrect file reading
function. I removed it and used the (correct) one from file.c (no idea
why there was this duplication anyway!).

Post by zxc
I saw that there is 300kb message body limit that's why I uploaded the
failing mp3 file here
http://www.4shared.com/file/iEY_IbRp/failing-mp3.html

Strange, the limit seems to have no effect, I got your first mail!

Greetings,
Johannes

[Johannes Weißl]

Reported-by: zxc <***@gmail.com>
---
file.c | 4 ++--
id3.c | 26 +++++---------------------
2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/file.c b/file.c
index aa101fe..1120d39 100644
--- a/file.c
+++ b/file.c
@@ -30,10 +30,10 @@
ssize_t read_all(int fd, void *buf, size_t count)
{
char *buffer = buf;
- int pos = 0;
+ ssize_t pos = 0;

do {
- int rc;
+ ssize_t rc;

rc = read(fd, buffer + pos, count - pos);
if (rc == -1) {
diff --git a/id3.c b/id3.c
index a48e0e3..fe3af81 100644
--- a/id3.c
+++ b/id3.c
@@ -23,6 +23,7 @@
#include "options.h"
#include "debug.h"
#include "utils.h"
+#include "file.h"

#include <unistd.h>
#include <stdint.h>
@@ -485,23 +486,6 @@ static int v2_4_0_frame_header_parse(struct v2_frame_header *header, const char
return 1;
}

-static int read_all(int fd, char *buf, size_t size)
-{
- size_t pos = 0;
-
- while (pos < size) {
- int rc = read(fd, buf + pos, size - pos);
-
- if (rc == -1) {
- if (errno == EINTR || errno == EAGAIN)
- continue;
- return -1;
- }
- pos += rc;
- }
- return 0;
-}
-
static char *parse_genre(const char *str)
{
int parenthesis = 0;
@@ -1017,7 +1001,7 @@ static int v2_read(struct id3tag *id3, int fd, const struct v2_header *header)
buf_size = header->size;
buf = xnew(char, buf_size);
rc = read_all(fd, buf, buf_size);
- if (rc) {
+ if (rc == -1) {
free(buf);
return rc;
}
@@ -1132,7 +1116,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
char buf[138];

rc = read_all(fd, buf, 10);
- if (rc)
+ if (rc == -1)
goto rc_error;
if (v2_header_parse(&header, buf)) {
rc = v2_read(id3, fd, &header);
@@ -1146,7 +1130,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
if (off == -1)
goto error;
rc = read_all(fd, buf, 138);
- if (rc)
+ if (rc == -1)
goto rc_error;

if (is_v1(buf + 10)) {
@@ -1180,7 +1164,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
if (off == -1)
goto error;
rc = read_all(fd, id3->v1, 128);
- if (rc)
+ if (rc == -1)
goto rc_error;
id3->has_v1 = is_v1(id3->v1);
}

[Gregory Petrosyan]

Hi Johannes,

These are the patches in -maint now (please do notice my enormous contribution
:-)

Gregory Petrosyan (1):
Bump copyright in --version output

Johannes Weißl (13):
Doc: add help for :shell
ffmpeg: move up "config/ffmpeg.h" include
fix two memleaks
fix cache refresh bug
fix cache refresh bug [2]
configure: fix FLAC include path
configure: fix ffmpeg header detection
fix TCP/IP networking protocol
fix segfault when hitting win-activate on empty tree
display error if seeking failed
fix segfault when using tqueue/lqueue
fix lqueue command
fix infinite loop when adding certain mp3 files

Am I right that nothing is missing here?

Gregory

[Johannes Weißl]

Hi Gregory,

Post by Gregory Petrosyan
These are the patches in -maint now (please do notice my enormous
contribution :-)

:-). Reviewing patches can not be overestimated!

Post by Gregory Petrosyan
Bump copyright in --version output
Doc: add help for :shell
ffmpeg: move up "config/ffmpeg.h" include
fix two memleaks
fix cache refresh bug
fix cache refresh bug [2]
configure: fix FLAC include path
configure: fix ffmpeg header detection
fix TCP/IP networking protocol
fix segfault when hitting win-activate on empty tree
display error if seeking failed
fix segfault when using tqueue/lqueue
fix lqueue command
fix infinite loop when adding certain mp3 files
Am I right that nothing is missing here?

Besides the (second) patch I just submitted, I don't think anything is
missing!

For the second patch:
Very strange, I tried cmus, gstreamer, mutagen, libid3, taglib and
eyeD3, and none of them could read the tags of "failing-mp3.mp3". But
there are tags at the end of the file (you can see them in the hexdump).
So I thought for sure that the id3v2 footer is corrupt, but it is not!
In cmus a casting bug prevents reading, I guess I have to check the
other applications/libraries :-). So now cmus seems to be the only
program that can display the tags in this particular file! Not bad!


Greetings,
Johannes

[Gregory Petrosyan]

Post by Johannes Weißl

          Bump copyright in --version output
          Doc: add help for :shell
          ffmpeg: move up "config/ffmpeg.h" include
          fix two memleaks
          fix cache refresh bug
          fix cache refresh bug [2]
          configure: fix FLAC include path
          configure: fix ffmpeg header detection
          fix TCP/IP networking protocol
          fix segfault when hitting win-activate on empty tree
          display error if seeking failed
          fix segfault when using tqueue/lqueue
          fix lqueue command
          fix infinite loop when adding certain mp3 files
Am I right that nothing is missing here?

Besides the (second) patch I just submitted, I don't think anything is
missing!

Cool! Will try to release 2.4.1 tomorrow, it has quite a few important fixes!

                Gregory

[gt]

Post by Johannes Weißl
Very strange, I tried cmus, gstreamer, mutagen, libid3, taglib and
eyeD3, and none of them could read the tags of "failing-mp3.mp3". But
there are tags at the end of the file (you can see them in the hexdump).
So I thought for sure that the id3v2 footer is corrupt, but it is not!
In cmus a casting bug prevents reading, I guess I have to check the
other applications/libraries :-). So now cmus seems to be the only
program that can display the tags in this particular file! Not bad!

Yes even mplayer or vlc couldn't display the tags, though they played
the file fine.

Only mediainfo could display the tags. But that is expected of it, since
it's a full tag displaying application.

[zxc]

Forwarding to cmus-***@lists.sourceforge.net.
I sent the mail only to gt <***@gmail.com> by mistake.

---------- Forwarded message ----------
From: zxc <***@gmail.com>
Date: Thu, May 26, 2011 at 12:11 PM
Subject: Re: 2.4.1
To: gt <***@gmail.com>


Great guys. I wonder if you could look at something else too ; )
Yesterday I started tagging some of my mp3s and some of the albums had 2
CDs.
I was using EasyTag to tag the mp3s and when the album contained two CDs the
mp3s were tagged as "Cd 1" and "Cd 2", not "1" and "2" for the CD number
field (int the ID3 tag).
After that I removed the cache file in ~/.cmus and restarted the program and
I updated my library. I noticed that in view 1 (Artist/Album - Track) the
mp3s were not ordered by album number.
They were ordered like that:
* track number 1 from CD1
* track number 1 from CD2
* track number 2 from CD1
* track number 2 from CD2
... and so on...

I modified the mp3s to hold "1" and "2" instead of "Cd 1" and "Cd 2" for the
CD number field (int the ID3 tag) and updated my library again. Now
everything is OK.
The ordering was:
* track number 1 from CD1
* track number 2 from CD1
* .....
* track number 1 from CD2
* track number 2 from CD2
...

But I have lots of mp3s and it will be difficult to check all of them for
this kind of incorrect ID3 information.
I don't know if the ID3 tag can hold String for the CD number field by
design but if it can then could we make the cmus player order the tracks by
comparing Strings instead of Numbers.
That way "Cd 1" will be before "Cd 2" and the tracks will still be ordered
correctly. We could ignore linguistic sorting because this field is supposed
to be something simple. I could try to apply a fix by myself but
unfortunately I do not have much experience in C and I am not familiar with
the project's code base so for now I am only watching what you guys are
doing. I really like the project.

/Vladimir

[Johannes Weißl]

Hi Vladimir,

Post by zxc
Great guys. I wonder if you could look at something else too ; )

No problem :-).

Post by zxc
I don't know if the ID3 tag can hold String for the CD number field by
design but if it can then could we make the cmus player order the tracks by
comparing Strings instead of Numbers.

Yes, seems ID3 can! In Ogg Vorbis also of course, since there are no
typed tag fields there! I think the best would be to (better) parse the
discnumber field (i.e. skip any non-numbers at the beginning of the
string!). So e.g.:

"CD 1" -> 1
"CD 02" -> 2
"3" -> 3

(note: "CD 1" < "CD 02" would not work for string comparison!)
What do you think?


Question for Gregory / others: Where should this happen?
1. comments_get_int()?
2. new comments_get_int_fuzzy() to be used for discnumber?
3. ?

Post by zxc
I could try to apply a fix by myself but unfortunately I do not have
much experience in C and I am not familiar with the project's code
base so for now I am only watching what you guys are doing. I really
like the project.

No coding needed, bug reports and suggestions / ideas are very valuable!
Great you like the project :-).


Greetings,
Johannes

[zxc]

Post by Johannes Weißl
(note: "CD 1" < "CD 02" would not work for string comparison!)
What do you think?

Oh, Yes you are right ; )

Post by Johannes Weißl
"CD 1" -> 1
"CD 02" -> 2
"3" -> 3

Yes, I think this will do the job.

[Johannes Weißl]

DISCNUMBER="CD 1/2" -> 1

Suggested-by: zxc <***@gmail.com>
---
comment.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/comment.c b/comment.c
index 83cd79a..896e73e 100644
--- a/comment.c
+++ b/comment.c
@@ -93,6 +93,8 @@ int comments_get_int(const struct keyval *comments, const char *key)
val = keyvals_get_val(comments, key);
if (val == NULL)
return -1;
+ while (*val && !(*val >= '0' && *val <= '9'))
+ val++;
if (str_to_int(val, &ival) == -1)
return -1;
return ival;

[Johannes Weißl]

Post by gt

Post by Johannes Weißl
Very strange, I tried cmus, gstreamer, mutagen, libid3, taglib and
eyeD3, and none of them could read the tags of "failing-mp3.mp3". But
there are tags at the end of the file (you can see them in the hexdump).
So I thought for sure that the id3v2 footer is corrupt, but it is not!
In cmus a casting bug prevents reading, I guess I have to check the
other applications/libraries :-). So now cmus seems to be the only
program that can display the tags in this particular file! Not bad!

Yes even mplayer or vlc couldn't display the tags, though they played
the file fine.
Only mediainfo could display the tags. But that is expected of it, since
it's a full tag displaying application.

Hmm, I have to take a look at this, never used it! Thanks!

I had a second look at the specification and on taglib's source code,
and it seems the mp3 does violate the standard. While the footer is
correct, there is no header present (which is required). But it is no
harm that cmus is a little more freely than the standard...


Johannes

[Johannes Weißl]

Interesting bug! With gcc on my system (amd64), this is false
(which causes a wrong seek in id3.c):

int foo(uint32_t x) {
return ((off_t) (-x)) == ((off_t) (-((off_t) x)));
}

It might be different for other compilers (e.g. tcc is fine for me) or
on 32-bit platforms.
---
id3.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/id3.c b/id3.c
index fe3af81..315163c 100644
--- a/id3.c
+++ b/id3.c
@@ -1140,7 +1140,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
}
if (v2_footer_parse(&header, buf)) {
/* footer at end of file - 128 */
- off = lseek(fd, -(header.size + 138), SEEK_END);
+ off = lseek(fd, -((off_t) header.size + 138), SEEK_END);
if (off == -1)
goto error;
rc = v2_read(id3, fd, &header);
@@ -1149,7 +1149,7 @@ int id3_read_tags(struct id3tag *id3, int fd, unsigned int flags)
}
} else if (v2_footer_parse(&header, buf + 128)) {
/* footer at end of file */
- off = lseek(fd, -(header.size + 10), SEEK_END);
+ off = lseek(fd, -((off_t) header.size + 10), SEEK_END);
if (off == -1)
goto error;
rc = v2_read(id3, fd, &header);

[Gregory Petrosyan]

Post by Johannes Weißl
Interesting bug! With gcc on my system (amd64), this is false
int foo(uint32_t x) {
   return ((off_t) (-x)) == ((off_t) (-((off_t) x)));
}
It might be different for other compilers (e.g. tcc is fine for me) or
on 32-bit platforms.

Hm, yes, on 32bit linux it is always true, and on 64-bit OS X it is
false for non-zero x, for both clang and gcc.

It has a simple explanation, I think:

-x for e.g. 0xff is 0xffffff00, and on 64 bit system off_t(-x) will be
0x00000000ffffff00.
-off_t(x) is, on the other hand, 0xffffffffffffff00.

                Gregory

[Johannes Weißl]

Post by Gregory Petrosyan

Post by Johannes Weißl
Interesting bug! With gcc on my system (amd64), this is false
int foo(uint32_t x) {
   return ((off_t) (-x)) == ((off_t) (-((off_t) x)));
}
It might be different for other compilers (e.g. tcc is fine for me) or
on 32-bit platforms.

Hm, yes, on 32bit linux it is always true, and on 64-bit OS X it is
false for non-zero x, for both clang and gcc.
-x for e.g. 0xff is 0xffffff00, and on 64 bit system off_t(-x) will be
0x00000000ffffff00.
-off_t(x) is, on the other hand, 0xffffffffffffff00.

Hmm, yes, I thought of something like this, but couldn't come up with an
explanation that fast :-)! But that would indicate a bug in tcc, right?
I have to read the C standard, maybe it is undefined behaviour?


Johannes

[Gregory Petrosyan]

Post by Johannes Weißl

Post by Gregory Petrosyan
-x for e.g. 0xff is 0xffffff00, and on 64 bit system off_t(-x) will be
0x00000000ffffff00.
-off_t(x) is, on the other hand, 0xffffffffffffff00.

Hmm, yes, I thought of something like this, but couldn't come up with an
explanation that fast :-)! But that would indicate a bug in tcc, right?
I have to read the C standard, maybe it is undefined behaviour?

My guess is that implementations are free to make it any size (maybe
at least as big as int) as long as it is signed.

                Gregory

[Gregory Petrosyan]

Post by Johannes Weißl
Hi Vladimir,

Post by zxc
I think I've found a bug in the cmus player.

Correct, thanks very much for reporting! The fix will make it to 2.4.1
:-). The problem was that id3.c contains a incorrect file reading
function. I removed it and used the (correct) one from file.c (no idea
why there was this duplication anyway!).

Wow, that was a quick fix! Thanks a lot, already applied!

Post by Johannes Weißl

Post by zxc
I saw that there is 300kb message body limit that's why I uploaded the
failing mp3 file here
http://www.4shared.com/file/iEY_IbRp/failing-mp3.html

Strange, the limit seems to have no effect, I got your first mail!

Well, list admins (AFAIK me and Timo :-) are notified of such cases, and can
act properly!

Gregory