Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#8680 - License free mod player codec

Attached to Project: Rockbox
Opened by Rainer Sinsch (myth) - Monday, 03 March 2008, 21:51 GMT+2
Last edited by Thom Johansen (preglow) - Wednesday, 21 May 2008, 13:20 GMT+2
Task Type Patches
Category Codecs
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Here is the new license free mod player codec. This one is intended to supersede http://www.rockbox.org/tracker/task/5241.

+ File should be coding guideline conform now
+ Bugfixes in the finetuning calculation
   licensefree_mod.patch (41 KiB) 
 apps/metadata/metadata_parsers.h |    1 
 apps/metadata.c                  |    8 
 apps/SOURCES                     |    1 
 apps/filetypes.c                 |    1 
 apps/codecs/Makefile             |    1 
 apps/codecs/SOURCES              |    1 
 apps/codecs/mod.c                | 1086 +++++++++++++++++++++++++++++++++++++++
 firmware/export/id3.h            |    1 
 firmware/id3.c                   |    3 
 9 files changed, 1103 insertions(+)

This task depends upon

Closed by  Thom Johansen (preglow)
Wednesday, 21 May 2008, 13:20 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Committed. Please report existing issues in a new Flyspray entry.
Comment by Dave Chapman (linuxstb) - Monday, 03 March 2008, 23:24 GMT+2
A couple of quick comments:

1) As has been mentioned before (in  FS#5241 ), the code isn't following the Rockbox contribution guidelines - mainly the use of TAB characters and the 80-column line limit.

2) As mentioned in the comment at the top of firmware/export/id3.h, new codecs MUST be added at the end of the list, to avoid breaking all existing WPSes.

Comment by Rainer Sinsch (myth) - Monday, 03 March 2008, 23:26 GMT+2
+ Fixed a severy bug within slide up & down
+ Changed the loading routine so that it takes the mod file directly from the rockboxbuffer. This makes playing of large .mod files possible at the risk of hanging when more than 28MB mod files are in the playlist.
   licensefree_mod_v0.1.patch (41.9 KiB) 
 apps/metadata/metadata_parsers.h |    1 
 apps/metadata.c                  |    8 
 apps/SOURCES                     |    1 
 apps/filetypes.c                 |    1 
 apps/codecs/Makefile             |    1 
 apps/codecs/SOURCES              |    1 
 apps/codecs/mod.c                | 1111 +++++++++++++++++++++++++++++++++++++++
 firmware/export/id3.h            |    1 
 firmware/id3.c                   |    3 
 9 files changed, 1128 insertions(+)

Comment by Rainer Sinsch (myth) - Monday, 03 March 2008, 23:37 GMT+2
+ Fixed issue (2) from above. Issue (1) is still open (sorry for that, will fix it later).
   licensefree_mod_v0.11.patch (41.8 KiB) 
 apps/metadata/metadata_parsers.h |    1 
 apps/metadata.c                  |    8 
 apps/SOURCES                     |    1 
 apps/filetypes.c                 |    1 
 apps/codecs/Makefile             |    1 
 apps/codecs/SOURCES              |    1 
 apps/codecs/mod.c                | 1111 +++++++++++++++++++++++++++++++++++++++
 firmware/export/id3.h            |    1 
 firmware/id3.c                   |    3 
 9 files changed, 1128 insertions(+)

Comment by Dave Chapman (linuxstb) - Monday, 03 March 2008, 23:49 GMT+2
I've just tried to compile this patch, and it fails because apps/metadata/mod.c is missing in these patches
Comment by Marianne Arnold (pixelma) - Tuesday, 04 March 2008, 01:48 GMT+2
The patch seems to be missing the metadata/mod.c that is added to apps/SOURCES.
I noticed because it just stopped compiling in the metadata/[codec] part.

Oops, only saw the page refreshed now that I was adding the comment and noticed that linuxstb already said that...
Comment by Rainer Sinsch (myth) - Tuesday, 04 March 2008, 21:08 GMT+2
Sorry for all the inconvinience. Here is the correct patch. I also corrected a track-rewinding bug.
Comment by Rainer Sinsch (myth) - Tuesday, 04 March 2008, 21:09 GMT+2
and the attachment...
   licensefree_mod_v0.12.patch (44.8 KiB) 
 apps/metadata/mod.c              |   63 ++
 apps/metadata/metadata_parsers.h |    1 
 apps/metadata.c                  |    8 
 apps/SOURCES                     |    1 
 apps/filetypes.c                 |    1 
 apps/codecs/Makefile             |    1 
 apps/codecs/SOURCES              |    1 
 apps/codecs/mod.c                | 1137 +++++++++++++++++++++++++++++++++++++++
 firmware/export/id3.h            |    1 
 firmware/id3.c                   |    3 
 10 files changed, 1217 insertions(+)

Comment by Marvin Miller (Mouser X) - Wednesday, 05 March 2008, 07:13 GMT+2
I've successfully built Rockbox fro my Gigabeat with this patch, but when I try to do it for my Sansa, it keeps failing. I've done "make clean" and "make veryclean" and it still doesn't work. I get this: http://pastebin.ca/928677 Hopefully that can help figure out the problem. As for whether it runs on my Gigabeat or not, I haven't had a chance to find out yet. Thanks for this patch though. I've got some great MOD files, and I look forward to listening to them (especially if the chiptune stuff is improved. That's pretty much all I have).
Comment by Marvin Miller (Mouser X) - Wednesday, 05 March 2008, 08:07 GMT+2
Ah crud. I was thinking it when I typed it, but I missed it nonetheless. This was while compiling for my Sansa e250. Also, I have a Gigabeat F40, if that helps. Sorry I missed saying it the first time.
Comment by Marvin Miller (Mouser X) - Wednesday, 05 March 2008, 08:27 GMT+2
I haven't used the Fly Spray much. Sorry about the multiple posts. I just tried this on my Gigabeat, and this file http://shup.com/Shup/27198/80megamix.mod , though it sounds a little better (not much, if any) than  FS#5241 , it plays it a lot worse. That is, it doesn't progress through the file. I have to manually progress the file every few seconds, if I want it to get past a section of music. If someone is willing to do some debugging on this patch, I figured this file (80megamix) could be helpful. If it's a problem, this MOD is by goto80, and *should* be available on his website, but I didn't want to bother searching for it. And, it looks like I can attach files here. I had already uploaded the MOD, so I didn't bother attaching it. Besides, I figured you probably didn't want that file sitting on the Rockbox servers/bug tracker/this thing.

Thanks again, and I look forward to this being committed (eventually/soonish).
Comment by Adam Gashlin (AdamGashlin) - Wednesday, 05 March 2008, 09:00 GMT+2
You may want to remove the "static" keyword from synthrender so it will build for portalplayer targets. No idea if it works, but there it is.
Comment by Rainer Sinsch (myth) - Wednesday, 05 March 2008, 21:47 GMT+2
+ fixed the "static" bug
+ fixed arpeggio effect
+ fixed e6-effect (pattern loop) => 80megamix.mod doesn't hang anymore
+ lowered the internal calculation volume so that 80megamix.mod doesn't clip internally
   licensefree_mod_v0.13.patch (45.7 KiB) 
 apps/metadata/mod.c              |   63 ++
 apps/metadata/metadata_parsers.h |    1 
 apps/metadata.c                  |    8 
 apps/SOURCES                     |    1 
 apps/filetypes.c                 |    1 
 apps/codecs/Makefile             |    1 
 apps/codecs/SOURCES              |    1 
 apps/codecs/mod.c                | 1152 +++++++++++++++++++++++++++++++++++++++
 firmware/export/id3.h            |    1 
 firmware/id3.c                   |    3 
 10 files changed, 1232 insertions(+)

Comment by Rainer Sinsch (myth) - Friday, 14 March 2008, 19:10 GMT+2
Here is the "finalzed" version with full conformance to the rockbox coding guideline. I fixed all tab and 80-char-width problems. If there are no more issues I would say it is about time to add the codec to the official branch.
   licensefree_mod_v0.131.patch (52.6 KiB) 
 tools/rockboxdev.sh              |    1 
 apps/metadata/mod.c              |   63 +
 apps/metadata/metadata_parsers.h |    1 
 apps/metadata.c                  |    8 
 apps/SOURCES                     |    1 
 apps/filetypes.c                 |    1 
 apps/codecs/Makefile             |    1 
 apps/codecs/SOURCES              |    1 
 apps/codecs/mod.c                | 1327 +++++++++++++++++++++++++++++++++++++++
 firmware/export/id3.h            |    1 
 firmware/id3.c                   |    3 
 11 files changed, 1408 insertions(+)

Comment by Robert Menes (RMenes379) - Tuesday, 18 March 2008, 01:13 GMT+2
Tested this patch on my 30GB iPod video 5.5G. MODs play correctly, loop properly (at least the ones I tried ;)), and overall performance is very good (boost ratio only around 14-15% with minimal CPU boosting).

A bit shameful that it doesn't play S3M files, though, but excellent job getting the codec up!
Comment by Luis Fors (krispyfi) - Wednesday, 19 March 2008, 07:04 GMT+2
This is a good thing, but until it plays XM, S3M and IT files, I don't think it's a good idea to close  FS#2634 . (http://www.rockbox.org/tracker/task/2634)

Maybe the attached file will provide some inspiration... :)
   troupe_diamondmed.it (175 KiB)
Comment by Marvin Miller (Mouser X) - Friday, 21 March 2008, 19:15 GMT+2
Thus far, every MOD file I've played is looped infinitely. Is this intended behavior? If not, is there a way to fix it? I like my MODs (and ITs, S3Ms, etc., but I can wait for those), but it's a little annoying that I have to manually skip to the next file every time. Should I upload some more samples for you to check out? I can't right now, but if you want some MODs to work with, I have a lot for you to look at.
Comment by Rainer Sinsch (myth) - Friday, 21 March 2008, 20:02 GMT+2
It is true that the at the current development state all MODs are looped infinitely. Of course this can be easily changed (someone already added that to  FS#2634 ).
Comment by Marianne Arnold (pixelma) - Sunday, 20 April 2008, 00:55 GMT+2
I'm currently running the latest version of this patch (and used the now closed one with the license issue before). First let me say that I really like that it is now possible to play files bigger than the codec buffer. I only had 2 of those which now work well - good work! But I have a few more comments.

The second thing is the already mentioned infinite looping which I find a bit annoying unfortunately. It would really be nice to just set up a playlist and let it play, hope you'll find the time to "fix" it.

Then I have one .mod which causes "undefined instructions" at changing addresses when I skip to it (reproducibly) and when I start the playlist with it, I'll just get some sound garbage. It might feature some unusual effects and the old codec also didn't play it but it was at least nice enough to just skip it (the file plays fine on PC though, tried with DeliPlayer and Foobar (dumb codec plugin) directly from the "disk"). I'm not sure how to hand that file over to you: I'd rather like to send it off task if that's ok for you (e-mail address seems to be no problem).

Next point is the playing time and track time handling in the WPS (at least, haven't checked elsewhere). With this patch I'll get the current pattern shown as current playing time and total track time is always "2:00". The old patch was similar but at least showed final pattern number as total track time. I'd really like it to show real times here if that's possible in any way (I'd *imagine* it could since it has to buffer the complete track at once anyways?). At first glance it seems only a cosmetic thing but it also confuses "caption backlight" if that's set - of course this could also be fixed later...

Finally: a few of my mods don't sound right, it seems like some delay (or other timing) effects are not interpreted right. I'm not sure if the old codec dealt better with them, but thought I let you know. Give me a sign if you want examples, maybe it would be easiest if you find the time and ask me in IRC - I'm around quite often and most of the times not far from keyboard when joined at all).
Comment by Adam Polkosnik (apolkosnik) - Tuesday, 22 April 2008, 23:15 GMT+2
I've noticed problems playing sll6.mod. For some reason both implementations (license free and the other one) play high pitched samples. Also Klisje Paa Klisje.mod has tempo problems (since it's an older Noise Tracker module, it uses speeds like 20 or 90, while in ProTracker all speeds in the range of 20-FF are reserved for extended BPM tempos).
Comment by Adam Polkosnik (apolkosnik) - Saturday, 03 May 2008, 02:09 GMT+2
Little correction to my previous comment. License-free implementation actually plays the mods perfectly well (no problem with sll6.mod), excluding Klisje Paa Klisje.
Comment by Izzy D (Lakmir) - Tuesday, 06 May 2008, 22:03 GMT+2
I completely agree, krispyfi. This one should be expanded to support XM, S3M and IT files. This is the type of thing that makes Rockbox great. Not to mention that I would shed tears of joy the day I can play mods directly off of my 5G ipod.

And that's an excellent tune, btw. =)
Comment by Rainer Sinsch (myth) - Tuesday, 06 May 2008, 22:52 GMT+2
I would like to point out that this codec was never meant to support xm, s3m and it. During development the focus lied on a license free mod playback (amiga based protracker, noisetracket, etc.). The license issue was a major concern in  FS#5241 , so I made this one totally free for rockbox (I am willing to change the license to whatever is requested). Of course I am also seeing  FS#8806  now, which seems to be a reasonable MikMod Port for playing a wider range of tracked formats.

Maybe it is time someone decides which path to follow. Of course I am willing to correct the bugs on this codec and improve playback quality - but does it make sense where  FS#8806  is available now?

Comments about this are welcome.
Comment by Thom Johansen (preglow) - Monday, 12 May 2008, 17:56 GMT+2
I'm actually quite eager to commit this codec right now, unless anyone sees a reason not to. I know about the Mikmod codec as well, and while promising, I think it'll be quite some time before it's fit for SVN.
Comment by Robert Menes (RMenes379) - Monday, 12 May 2008, 18:04 GMT+2
I agree. The codec is very stable and seems ready for SVN.
Comment by Adam Gashlin (AdamGashlin) - Tuesday, 20 May 2008, 06:16 GMT+2
apps/metadata/mod.c won't build (anymore) with the #include "atoi.h" line, which it doesn't need anyway. I figure it's better to just mention this than to post the whole 50k patch again with one removed line.
Comment by Martti Roitto (maraz) - Wednesday, 21 May 2008, 08:27 GMT+2
I patched rev. 17535 with this a couple days ago (removing the atoi.h include) and was surprised by how well it works. Jumping on the SVN bandwagon and eagerly awaiting support for other module formats.

Loading...