DUMB 2.0 and 0.9.3 support #823

SimonN · 2017-09-13T05:19:34Z

Specifying -lm on Arch Linux is necessary for DUMB support: Otherwise, cmake concludes that DUMB is not available. I used the DUMB 1.0-1 binary package from the Arch repo. This Arch package points to kode54's fork of DUMB with version 1.0 as suggested by the DUMB homepage, not to version 0.9.3 that A5 officially supports.

I didn't test whether this is required on other systems, or whether it breaks the build on other systems.

Still no successful playback: I've merely fixed A5's build for me. I can load a module with Allegro 5, but then my application will immediately crash within DUMB. Crashlog follows. I haven't debugged this crash yet! The fault could be in DUMB, in the DUMB Arch package, in A5, in my code, in a faulty module, ...

I'll understand if you won't merge this PR before we have seen successful playback with DUMB 1.0 and A5. According to @SiegeLord, DUMB 1.0 made breaking changes. Still, I'm opening this PR to share my findings, allowing others to shed more light on this.

#0  0x0000000000000000 in ?? ()
#1  0x00007ffff3dd1962 in ?? () from /usr/lib/libdumb.so
#2  0x00007ffff3dd2d15 in dumb_read_it_quick () from /usr/lib/libdumb.so
#3  0x00007ffff3dd0996 in dumb_read_it () from /usr/lib/libdumb.so
#4  0x00007ffff78d5d74 in mod_stream_init (f=0x555556284600, buffer_count=3,
    samples=2048, loader=0x7ffff3dd0990 <dumb_read_it>)
    at /home/simon/c/notown/allegro5/addons/acodec/modaudio.c:213
#5  0x00007ffff78d625c in _al_load_it_audio_stream (filename=<optimized out>,
    buffer_count=3, samples=2048)
    at /home/simon/c/notown/allegro5/addons/acodec/modaudio.c:391

For reference, here's an excerpt from cmake's error log when DUMB's libraries don't specify -lm:

Run Build Command:"/usr/bin/make" "cmTC_5d751/fast"
/usr/bin/make -f CMakeFiles/cmTC_5d751.dir/build.make CMakeFiles/cmTC_5d751.dir/build
make[1]: Entering directory '/home/simon/c/notown/allegro5/simon/CMakeFiles/CMakeTmp'
Building C object CMakeFiles/cmTC_5d751.dir/src.c.o
/usr/bin/cc   -msse -W -Wall -Wpointer-arith -Wmissing-declarations -Wstrict-prototypes -Wmissing-prototypes -DDUMB_COMPILES   -o CMakeFiles/cmTC_5d751.dir/src.c.o   -c /home/simon/c/notown/allegro5/simon/CMakeFiles/CMakeTmp/src.c
Linking C executable cmTC_5d751
/usr/bin/cmake -E cmake_link_script CMakeFiles/cmTC_5d751.dir/link.txt --verbose=1
/usr/bin/cc  -msse -W -Wall -Wpointer-arith -Wmissing-declarations -Wstrict-prototypes -Wmissing-prototypes -DDUMB_COMPILES    -rdynamic CMakeFiles/cmTC_5d751.dir/src.c.o  -o cmTC_5d751 /usr/lib/libdumb.so 
/usr/lib/libdumb.so: undefined reference to `__pow_finite'
/usr/lib/libdumb.so: undefined reference to `__log_finite'
/usr/lib/libdumb.so: undefined reference to `__exp_finite'
/usr/lib/libdumb.so: undefined reference to `sin'
/usr/lib/libdumb.so: undefined reference to `__log2_finite'
/usr/lib/libdumb.so: undefined reference to `cos'
collect2: error: ld returned 1 exit status
make[1]: *** [CMakeFiles/cmTC_5d751.dir/build.make:99: cmTC_5d751] Error 1
make[1]: Leaving directory '/home/simon/c/notown/allegro5/simon/CMakeFiles/CMakeTmp'
make: *** [Makefile:126: cmTC_5d751/fast] Error 2

Source file was:

            #include <dumb.h>
            int main(void)
            {
                dumb_register_stdfiles();
                return 0;
            }

SimonN · 2017-09-16T09:11:58Z

More light on the crash inside DUMB 1.0 A5 runtime! I've built DUMB 1.0 with debugging flags, let A5 use that debugging build, and get a much more informative stacktrace, along with:

 /home/simon/c/notown/dumb/src/core/dumbfile.c:36:
register_dumbfile_system: Assertion `dfs->seek' failed.

All XM files work. All IT, MOD, and S3M files crash.

Explanation: siege.c is SiegeLord's example A5 program from issue #670 that loads a music file given on the command line, then immediately plays it.

#0  0x00007ffff73318a0 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff7332f09 in abort () from /usr/lib/libc.so.6
#2  0x00007ffff732a0dc in __assert_fail_base () from /usr/lib/libc.so.6
#3  0x00007ffff732a153 in __assert_fail () from /usr/lib/libc.so.6
#4  0x00007ffff43afc93 in register_dumbfile_system (dfs=0x7ffff78be2c0 <dfs>)
    at /home/simon/c/notown/dumb/src/core/dumbfile.c:36
#5  0x00007ffff76ba0b7 in init_libdumb ()
    at /home/simon/c/notown/allegro5/addons/acodec/modaudio.c:346
#6  0x00007ffff76ba14f in init_libdumb ()
    at /home/simon/c/notown/allegro5/addons/acodec/modaudio.c:451
#7  _al_load_mod_audio_stream_f (f=f@entry=0x5555557c51e0, 
    buffer_count=buffer_count@entry=4, samples=samples@entry=2048)
    at /home/simon/c/notown/allegro5/addons/acodec/modaudio.c:452
#8  0x00007ffff76ba19c in _al_load_mod_audio_stream (filename=<optimized out>, 
    buffer_count=4, samples=2048)
    at /home/simon/c/notown/allegro5/addons/acodec/modaudio.c:368
#9  0x0000555555554acb in main (argc=2, argv=0x7fffffffe798) at siege.c:15

I suppose:

The interface, or at least the calling conventions, between DUMB 0.9.3 and DUMB 1.0 differ.
A5 calls the 1.0 interface in such a way that the DUMB assert fails.
Either A5 or DUMB has an issue, provided A5 would like to offer support for DUMB 1.0.

Now it's time to dig deep into A5 source and the DUMB API...

SimonN · 2017-09-17T01:02:58Z

I've fixed the API mismatches to the best of my knowledge. I've submitted new documentation about DUMBFILE_SYSTEM to DUMB, this got merged today.

Ive changed A5's modaudio.c to call DUMB 1.0, but I'm not yet submitting my modaudio.c to this A5 pull request: A5 returns null streams even on XM files that it played with DUMB 1.0 before I started hacking. I want to more work on this first. (If you want to see my changes, they're in my public github fork of allegro5, but I rebase public branches heavily until I finally make PRs from them.)

I'll continue to investigate. For any questions, I'll sit in irc.freenode.net #allegro or irc.quakenet.org #lix, or reply here.

SimonN · 2017-09-17T08:37:32Z

I've got it working! XM, IT, MOD, S3M all play well with the DUMB git HEAD.

DUMB supports even more tracked formats in 1.0, but Allegro doesn't register loaders for them yet. We can look into that soon.

My code relies on dumb.h reporting the correct version. Since the DUMB commit tagged 1.0 reports 0.9.3 internally, I've proposed upstream to release another tagged 1.y.z > 1.0.0: kode54/dumb#48

beoran · 2017-09-17T08:44:58Z

As an aside, I am glad this is coming together! Thank you very much, Simon, for your relentless efforts.

…

On 17 Sep 2017 10:37, "Simon Na." ***@***.***> wrote: I've got it working! XM, IT, MOD, S3M all play well with the DUMB git HEAD. DUMB supports even more tracked formats in 1.0, but Allegro doesn't register loaders for them yet. We can look into that soon. My code relies on dumb.h reporting the correct version. Since the DUMB commit tagged 1.0 reports 0.9.3 internally, I've proposed upstream to release another tagged 1.y.z > 1.0.0: kode54/dumb#48 <kode54/dumb#48> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#823 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEWeazhmdM_FSV3mxlghReLJjLLvUNmks5sjNpMgaJpZM4PVl9N> .

SimonN · 2017-09-17T08:48:41Z

You're welcome!

Feels so good when research and work pay off. I haven't contributed to either library before and dug through internals in both.

SimonN · 2017-09-19T00:52:04Z

News from DUMB upstream: We're working on stabilizing the API for DUMB 2.0. kode54, the maintainer, hasn't planned any breaking changes, only bugfixes. The DUMB header needs small clean-up in the public API, and I'm helping upstream getting all the ducks in a row.

This means A5 should wait until DUMB 2.0 before merging this PR. I estimate we'll have DUMB 2.0 in 2 weeks. Then, I'll push another commit to this PR's target branch to make A5 check for DUMB 2.0 instead of 1.0.

DUMB 2.0 has a catch-all loader to guess file format from an open file. Looks like I can simplify the A5 code and allow all the 10+ tracker formats in one go.

SiegeLord · 2017-09-19T04:24:33Z

addons/acodec/acodec.c

@@ -38,6 +38,48 @@ bool al_init_acodec_addon(void)
 #endif

 #ifdef ALLEGRO_CFG_ACODEC_MODAUDIO
+#include <dumb.h>


Probably shouldn't include a header inside the body of a function.

Good catch, thanks. Fixed in new commit below.

SimonN · 2017-09-21T09:04:29Z

I expect the DUMB 2.0 API to stabilize in a week. I have pushed a commit to this PR (A5-DUMB-2.0-interop) that uses the most likely DUMB 2.0 API, and still should build with DUMB 0.9.3.

Once DUMB 2.0 is tagged upstream, I'll adapt to the then-stable API, and rebase this PR onto A5 master, squashing commits as appropriate.

I'd love to make it onto the A5 October release before the ship sails!

In modaudio.c, Allegro's MOD_FILE has a field double length; to which we've assigned from l / 65546.0 before where l is long, but in 2.0, we might assign from l / 65536.0 where l is an integer 64+ bits wide. Should I change the field's declaration? Would that have any effect on A5 outside of modaudio.c? (The largest integer result could be stored, but maybe not all the intermediade fractional values.)

Upstream planning for this 64-bit type

beoran · 2017-09-21T20:20:31Z

I am not the one to decide but I consider this feature important enough to delay the next Allegro 5 release for. The DUMB dependency was getting stale, upgrading to 2.0 seems a great idea.

…

On 21 Sep 2017 11:04 a.m., "Simon Naarmann" ***@***.***> wrote: I expect the DUMB 2.0 API to stabilize within a week. I have pushed a commit to this PR (A5-DUMB-2.0-interop) that uses the most likely DUMB 2.0 API, and still should build with DUMB 0.9.3. Once DUMB 2.0 is tagged upstream, I'll rebase this PR onto A5 master and squash commits as appropriate. I'd love to make it onto the A5 October release before the ship sails! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#823 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEWeQNIvONau1RRy6-1RagSJC1yfVTMks5skiaegaJpZM4PVl9N> .

SiegeLord · 2017-09-25T00:48:10Z

It's fine to change MOD_FILE's declaration to whatever you need it to be, it's purely an internal implementation detail with no user-visible API or ABI effects.

SimonN · 2017-09-25T06:23:00Z

Hmm, even if it's purely internal, I think I'll keep MOD_FILE as-is, even if it might fail subtly for gigantic module files of several hundred GB.

We have gold-plated the DUMB API for 64-bit file offsets (module files > 2 GB), but that's only because we want the API future-proof. Maybe that was unnecessary already.

Upstream, I have an open PR with small changes to the 2.0 API release candidate. Maybe everybody will already agree with that API, maybe there'll be 1 (unlikely: 2 or more) PR after that one. That means: Early 2017-10 tag of DUMB 2.0 is still likely.

SimonN · 2017-09-26T04:56:51Z

DUMB 2.0.0 is tagged upstream! @SiegeLord will look over this PR one more time tomorrow, afterwards I'll rebase and squash.

I've run a quick module-playing test with this PR (including yesterday's commit) against tagged 2.0.0, nothing broke here.

SiegeLord · 2017-09-27T02:55:18Z

addons/acodec/acodec.c

+   ret &= al_register_audio_stream_loader_f(".mod", _al_load_dumb_audio_stream_f);
+   ret &= al_register_audio_stream_loader(".mtm", _al_load_dumb_audio_stream);
+   ret &= al_register_audio_stream_loader_f(".mtm", _al_load_dumb_audio_stream_f);
+   ret &= al_register_audio_stream_loader(".nst", _al_load_dumb_audio_stream);


Didn't you just say it wasn't listed?

I think he meant it wasn't listed in the DUMB documentation.

Earlier upstream discussion: kode54/dumb#53 (comment) -- based on that, I decided to add .nst in the code above.

I'm considering to remove .nst above because, after googling around, it seems really hard to find NST modules with that extension. Not even modarchive lists NST as a format to search.

SiegeLord · 2017-09-27T03:29:53Z

I tried this with dumb2, and found one issue. It appears that the stream length is not reported correctly (e.g. I used ex_stream_seek, and it did not display the length). This seems to work fine with dumb1 (with this code). Maybe it's a dumb2 issue though.

SimonN · 2017-09-27T04:44:22Z

stream length is not reported correctly (e.g. I used ex_stream_seek, and it did not display the length). This seems to work fine with dumb1 (with this code).

Excellent catch. Can reproduce exactly as you told me: With my latest A5 PR and ex_stream_seek hybridsong220.xm, I can seek in the earlier dumb cbbaa56733f7 from 2 weeks ago, but not in dumb 2.0.0. Will examine!

SimonN · 2017-09-27T06:22:52Z

Fixed both issues (.nst registration and failure to seek). Rebased onto master, but not yet squashed.

I fixed the seek failure with dumb_read_any (DUMB runs eagerly through the file once on load and remembers positions for faster seeking) instead of dumb_read_any_quick (don't run through on file load).

Strangely, dumb_read_any_quick worked 20 commits ago. I'm confused and will investigate, maybe our non-perfect DUMB API 2.0 64-bit conversion is at fault. But even if I find the real cause, using dumb_read_any_quick would be a minor performance gain -- very nice to have, but not as important as offering seeking and length for now.
See next post!

SimonN · 2017-09-27T06:51:37Z

It's all fixed and good!

I've bisected the DUMB codebase, and now I feel dumb: In this A5 PR, I #if the DUMB header version. For >= 2.0, I used dumb_read_any_quick; for smaller DUMB, I used the old dumb readers. That's why my PR behaves so differently with DUMB 2.0 and 1.0.

I should always use dumb_read_any to allow seeking, never dumb_read_any_quick. It's already fixed in the above a947cb2.

The 64-bit DUMB API has nothing to do with this, sorry for tying @SiegeLord into an hour of rambling!

SiegeLord · 2017-09-28T05:11:31Z

Ok, tested this again and it seemed to work fine. Feel free to squash it when convenient.

beoran · 2017-09-28T06:00:34Z

@SiegeLord On the subject of squashing, I see that now Github has a convenient looking "squash and merge" button for Allegro developers with sufficient privileges to the repo. Do you think it is OK to actually use this button once all reviews and tests are OK? It saves the contributor the effort of having to do a manual squash.

SimonN · 2017-09-28T06:10:18Z

Squashed into 2 commits: Fixing whitespace, then a clean diff of the feature.

@beoran: As contributor, I like the manual squash and take my time to get it perfect, because it defines the patch's final presentation. I care about it.

But maybe others are fine however their patch turn out? Github's squash-and-merge might certainly be handy when somebody wouldn't want to squash their commits themselves. :-)

We support two versions of DUMB to play tracked music: * 0.9.3 from Sourceforge <http://dumb.sourceforge.net> * 2.0.0 by kode54 on github <https://github.com/kode54/dumb> The DUMB homepage on Sourceforge endorses kode54's fork and links to it. The Allegro source tests the DUMB version and supports either API. Supported tracker formats in DUMB 2.0 that Allegro 5 can now play: * IT (Impulse Tracker) * XM (Fasttracker II) * MOD (Ultimate SoundTracker, ProTracker) * STM (Scream Tracker) * S3M (Scream Tracker 3) * 669 (Composer 669) * AMF (Asylum Music Format) * AMF (Digital Sound and Music Interface Advanced Music Format) * DSM (Digital Sound Interface Kit module format) * MTM (MultiTracker) * OKT (Oktalyzer) * PSM (Protracker Studio, both the older PSM16 and the newer PSM) * PTM (PolyTracker) * RIFF AM/AMFF (Galaxy Music System internal format) The CMake build script uses -lm on all platforms for the compile test. Without -lm, cmake finds the include paths and libraries, can compile the test program, but couldn't link it on Arch Linux.

SiegeLord · 2017-09-28T15:13:33Z

@beoran, it doesn't do the rebase, as far as I can tell. I really don't like having merges in the history. We very rarely ask people to squash and rebase (I always rebase myself before merging), Simon is just being super nice :D.

elias-pschernig · 2017-09-28T18:14:12Z

I hate repositories who have 1000ds of useless merge commits - so I'm really glad we avoid them in Allegro (except for actual branch merges, which are rare now that we switched to a single development branch).

beoran · 2017-09-28T22:42:05Z

I see, I can agree with that. I will refrain from using the button, then.

…

On 28 Sep 2017 20:14, "elias-pschernig" ***@***.***> wrote: I hate repositories who have 1000ds of useless merge commits - so I'm really glad we avoid them in Allegro (except for actual branch merges, which are rare now that we switched to a single development branch). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#823 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEWeY-rAGzpHOiE6xJsVy_XtQjBQTYpks5sm-H0gaJpZM4PVl9N> .

SiegeLord · 2017-09-29T02:01:46Z

Alright, merged! Thanks a lot Simon, you're the best!

SimonN · 2017-09-29T05:45:58Z

You're welcome! Thanks to SiegeLord for the frequent reviews, and to everyone for feedback and encouragement.

SimonN mentioned this pull request Sep 13, 2017

Unable to load/play tracker files (.it/.xm/etc) #670

Closed

SimonN force-pushed the recognize-dumb-on-arch branch from d713245 to 34ba82f Compare September 17, 2017 08:37

SimonN changed the title ~~let cmake find DUMB 1.0 on Arch (don't merge yet? I got crashes)~~ DUMB 1.0 and 0.9.3 support (maybe wait for upstream tag) Sep 17, 2017

SimonN mentioned this pull request Sep 17, 2017

Release 1.0.1 because tag 1.0 reports 0.9.3 in dumb.h kode54/dumb#48

Closed

SimonN changed the title ~~DUMB 1.0 and 0.9.3 support (maybe wait for upstream tag)~~ DUMB 2.0 and 0.9.3 support (wait for upstream's 2.0, probably early of 2017-10) Sep 19, 2017

SimonN changed the title ~~DUMB 2.0 and 0.9.3 support (wait for upstream's 2.0, probably early of 2017-10)~~ DUMB 2.0 and 0.9.3 support (wait for upstream's 2.0, probably early 2017-10) Sep 19, 2017

SiegeLord reviewed Sep 19, 2017

View reviewed changes

SimonN mentioned this pull request Sep 21, 2017

Large file support kode54/dumb#51

Closed

SimonN force-pushed the recognize-dumb-on-arch branch from 545b41c to c1b2600 Compare September 21, 2017 08:57

SimonN changed the title ~~DUMB 2.0 and 0.9.3 support (wait for upstream's 2.0, probably early 2017-10)~~ DUMB 2.0 and 0.9.3 support (2.0 tagged! Final review, will then squash) Sep 26, 2017

SiegeLord reviewed Sep 27, 2017

View reviewed changes

SimonN force-pushed the recognize-dumb-on-arch branch from f8cc830 to a947cb2 Compare September 27, 2017 06:20

remove trailing whitespace in modaudio.c

c97f009

SimonN force-pushed the recognize-dumb-on-arch branch from a947cb2 to b08f5f4 Compare September 28, 2017 06:07

SimonN changed the title ~~DUMB 2.0 and 0.9.3 support (2.0 tagged! Final review, will then squash)~~ DUMB 2.0 and 0.9.3 support Sep 28, 2017

SimonN force-pushed the recognize-dumb-on-arch branch from b08f5f4 to 5e2ac76 Compare September 28, 2017 06:22

SiegeLord merged commit 5e2ac76 into liballeg:master Sep 29, 2017

SimonN deleted the recognize-dumb-on-arch branch September 29, 2017 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DUMB 2.0 and 0.9.3 support #823

DUMB 2.0 and 0.9.3 support #823

SimonN commented Sep 13, 2017

SimonN commented Sep 16, 2017 •

edited

Loading

SimonN commented Sep 17, 2017

SimonN commented Sep 17, 2017

beoran commented Sep 17, 2017 via email

SimonN commented Sep 17, 2017

SimonN commented Sep 19, 2017

SiegeLord Sep 19, 2017

SimonN Sep 19, 2017 •

edited

Loading

SimonN commented Sep 21, 2017 •

edited

Loading

beoran commented Sep 21, 2017 via email

SiegeLord commented Sep 25, 2017

SimonN commented Sep 25, 2017 •

edited

Loading

SimonN commented Sep 26, 2017 •

edited

Loading

SiegeLord Sep 27, 2017

fatcerberus Sep 27, 2017

SimonN Sep 27, 2017 •

edited

Loading

SiegeLord commented Sep 27, 2017

SimonN commented Sep 27, 2017

SimonN commented Sep 27, 2017 •

edited

Loading

SimonN commented Sep 27, 2017 •

edited

Loading

SiegeLord commented Sep 28, 2017

beoran commented Sep 28, 2017 •

edited

Loading

SimonN commented Sep 28, 2017 •

edited

Loading

SiegeLord commented Sep 28, 2017

elias-pschernig commented Sep 28, 2017

beoran commented Sep 28, 2017 via email

SiegeLord commented Sep 29, 2017

SimonN commented Sep 29, 2017

DUMB 2.0 and 0.9.3 support #823

DUMB 2.0 and 0.9.3 support #823

Conversation

SimonN commented Sep 13, 2017

SimonN commented Sep 16, 2017 • edited Loading

SimonN commented Sep 17, 2017

SimonN commented Sep 17, 2017

beoran commented Sep 17, 2017 via email

SimonN commented Sep 17, 2017

SimonN commented Sep 19, 2017

SiegeLord Sep 19, 2017

Choose a reason for hiding this comment

SimonN Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

SimonN commented Sep 21, 2017 • edited Loading

beoran commented Sep 21, 2017 via email

SiegeLord commented Sep 25, 2017

SimonN commented Sep 25, 2017 • edited Loading

SimonN commented Sep 26, 2017 • edited Loading

SiegeLord Sep 27, 2017

Choose a reason for hiding this comment

fatcerberus Sep 27, 2017

Choose a reason for hiding this comment

SimonN Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

SiegeLord commented Sep 27, 2017

SimonN commented Sep 27, 2017

SimonN commented Sep 27, 2017 • edited Loading

SimonN commented Sep 27, 2017 • edited Loading

SiegeLord commented Sep 28, 2017

beoran commented Sep 28, 2017 • edited Loading

SimonN commented Sep 28, 2017 • edited Loading

SiegeLord commented Sep 28, 2017

elias-pschernig commented Sep 28, 2017

beoran commented Sep 28, 2017 via email

SiegeLord commented Sep 29, 2017

SimonN commented Sep 29, 2017

SimonN commented Sep 16, 2017 •

edited

Loading

SimonN Sep 19, 2017 •

edited

Loading

SimonN commented Sep 21, 2017 •

edited

Loading

SimonN commented Sep 25, 2017 •

edited

Loading

SimonN commented Sep 26, 2017 •

edited

Loading

SimonN Sep 27, 2017 •

edited

Loading

SimonN commented Sep 27, 2017 •

edited

Loading

SimonN commented Sep 27, 2017 •

edited

Loading

beoran commented Sep 28, 2017 •

edited

Loading

SimonN commented Sep 28, 2017 •

edited

Loading