MIDI (from MUS) working for me

[chippo]

Gentleheads,

I've got MIDI (from MUS) sound working in legacy-2. I'm using a wad called Doom.wad (commercial DOOM I, Ultimate) which is 12408292 bytes long. Strange thing is, I don't recognize the music, and I thought that I'd played my way through all doom levels a hundred years ago. Anyway, it's definitely music, it sounds ok and is in a major key.

I used this desciption of a MIDI header:
http://www.sonicspot.com/guide/midifiles.html
and found that the start of the first track was 6 bytes too early.

Here's the current 'svn diff' of my code against the repository:Index: interface/sdl/i_sound.cpp
================================================== =================
--- interface/sdl/i_sound.cpp (revision 420)
+++ interface/sdl/i_sound.cpp (working copy)
@@ -23,8 +23,12 @@
#include <math.h>
#include <unistd.h>
#include <stdlib.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <iostream>

-
#if defined(FREEBSD) || defined(__APPLE_CC__) || defined(__MACOS__)
# include <SDL.h>
# include <SDL_mixer.h>
@@ -585,6 +589,26 @@
CONS_Printf("Cannot convert MUS to MIDI: error %d.\n", err);
return 0;
}
+ int fd;
+ if ((fd=open("qmus2mid.mid", O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR))
+ == -1)
+ {
+ cerr << "Couldn't open qmus2mid.mid for writing\n";
+ perror("open");
+ exit(1);
+ }
+ if (write(fd, mus2mid_buffer, midlength) != (int)midlength)
+ {
+ cerr << "Couldn't write " << midlength << " bytes to qmus2mid.mid\n";
+ perror("write");
+ exit(1);
+ }
+ if (close(fd) != 0)
+ {
+ cerr << "Couldn't close qmus2mid.mid\n";
+ perror("close");
+ exit(1);
+ }

music.rwop = SDL_RWFromConstMem(mus2mid_buffer, midlength);
}
Index: audio/qmus2mid.cpp
================================================== =================
--- audio/qmus2mid.cpp (revision 420)
+++ audio/qmus2mid.cpp (working copy)
@@ -70,12 +70,14 @@
{
Sint16 *p = reinterpret_cast<Sint16*>(*f);
*p++ = ((x>>8) & 0xff) | (x<<8);
+ *f += 2;
}

inline void fwritelong(Uint32 x, byte **f)
{
Sint32 *p = reinterpret_cast<Sint32*>(*f);
*p++ = ((x>>24) & 0xff) | ((x>>8) & 0xff00) | ((x<<8) & 0xff0000) | (x<<24);
+ *f += 4;
}


Index: Makefile
================================================== =================
--- Makefile (revision 420)
+++ Makefile (working copy)
@@ -48,7 +48,7 @@
platform = -DLINUX
interface = -DSDL #-DNO_MIXER
# linker
- LIBS = -lSDLmain -lSDL -lSDL_mixer -lpng -lz -L. -ltnl -ltomcrypt
+ LIBS = -L/usr/local/lib -lSDLmain -lSDL -lSDL_mixer -lpng -lz -L. -ltnl -ltomcrypt
# LIBS = -lSDLmain -lSDL -lpng -lz -L. -ltnl32 -ltomcrypt32
OPENGLLIBS = -lGL -lGLU
# CF += -m32
@@ -104,7 +104,7 @@
# NO_MIXER : do not include SDL_mixer in the build
# USEASM : use assembler routines where possible

-export CF += -Wall $(platform) $(interface) $(linkage)
+export CF += -I/usr/local/include -Wall $(platform) $(interface) $(linkage)

INCLUDES = -Iinclude
CFLAGS = $(CF) $(INCLUDES)

Gdb is my friend; viva gdb viva! But one of the troubles of using a debugger too much, is that you can loose sight of the bigger (code) picture. Now I must look carefully at all these source files, that I saw just a few lines of each, while leaping around the code in GDB.

I will commit my changes in both SVN and CVS (in legacy-1) once:
- I understand all the code I'm modifying.
- Have worked out how the error crept in. (BTW, 'svn blame' won't tell you whose to blame for this error; I've checked:))
- Have checked that there are no parallel functions that need a similar fix.

Cheers,
chippo

PS: Anyone know a keyboard sequence (which works in legacy-2) which will release the mouse (obviously only applicable in windowed mode)? And please don't flame me if it's well documented somewhere; just tell me to RTFM.

[chippo]

- Have worked out how the error crept in. (BTW, 'svn blame' won't tell you whose to blame for this error; I've checked:))

I've worked out where the error crept in. See here:
http://doomlegacy.svn.sourceforge.net/viewvc/doomlegacy/legacy/trunk/audio/qmus2mid.cpp?view=diff&r1=306&r2=341&diff_format=h

This was a change to make 64-bit cpus work. But I can't work out why the code (as stands in SVN) doesn't work right. But then, I've never heard of the directive 'reinterpret_cast', before either. Guess I'll have to swot up on C++ before I can figure it out.

Note also, that in legacy-1 this file has not been made 64-bit clean:
http://doomlegacy.cvs.sourceforge.net/doomlegacy/doomlegacy/qmus2mid.c?revision=1.6&view=markup
So the reason that MIDI isn't working in legacy-1 is a different reason to in legacy-2. But I'll get there, eventually.

[DooMAD]

Not sure if this is any use to you, just a random snippet from my IRC logs:


[00:01] <GhostlyDeath> There is a simple fix to being able to play MIDIs
[00:01] <GhostlyDeath> It's one or two lines of code
[00:11] <GhostlyDeath> In the C version anyway
[00:12] <GhostlyDeath> In I_RegisterSong(), add "pMidiFileData = data;"
where it checks to see if it's a MIDI file
[00:12] <GhostlyDeath> I mean : "iMus2MidSize = len;"

[Pate]

Reinterpret_cast means approximately "I am 100% sure that this pointer to [sometype] is in reality a pointer to [the type I want]. In case something breaks, it is my own fault". So effectively it is the same as regular C casts.

Also, you seem to have customised your makefile. When you commit, make sure that the repository's Makefile is not modified.

So the reason that MIDI isn't working in legacy-1 is a different reason to in legacy-2. But I'll get there, eventually.

There's really no need to figure this one out for Legacy 1. We're not going to release any more of those.

[chippo]

Also, you seem to have customised your makefile. When you commit, make sure that the repository's Makefile is not modified.

And I'm writing out a debug file somewhere else. No, of course I won't commit debug fluff.

I almost always manually specify all the files I want to commit on the command line (rather than a recursive commit), in addition to reading through the diff and satisfying myself that those changes are the ones mentioned in the log.

For this case, a work in progress, I sloppily didn't look carefully and did it recursively. But I was also remembering that smitemeister is looking at the same issue as me and thought that if he's online and working on legacy, there's much better stuff for him to be doing that tracking a bug I've just found. Though, sadly, I still can't work out why the original code doesn't do the right thing....

There's really no need to figure this one out for Legacy 1. We're not going to release any more of those.

I hope that not policy, and hope that nobody cares if someone else (like me) does the work. Legacy-1 is still better than legacy-2 in several respects (more on that later), and until we can offer our users the super legacy-2, I think that it would be cool to give 'em a legacy-1 with music working. I hope to do one more legacy-1 release with music (under SDL) working.

Cheers,
chippenbronx

[Aliotroph?]

Heh, is there even a policy on Legacy releases? People can commit stuff if they want and Rellik maintains the page so if you convince him to put up a new binary it happens. hehehe

[Ajapted]

This was a change to make 64-bit cpus work. But I can't work out why the code (as stands in SVN) doesn't work right.
Maybe because the fwriteshort() and fwritelong() functions don't adjust the 'f' pointer? Seems to me that the two calls to fwriteshort() that occur together are writing the same piece of memory instead of consecutive pieces of memory.

[chippo]

Maybe because the fwriteshort() and fwritelong() functions don't adjust the 'f' pointer? Not f. The place that f points to. Because my fix was: *f += 2;
and it works - the midi file now looks correct (and it plays too, of course). However, what is curious to me is the following. Here is the code before my fix:inline void fwriteshort(Uint16 x, byte **f)
{
Sint16 *p = reinterpret_cast<Sint16*>(*f);
*p++ = ((x>>8) & 0xff) | (x<<8);
}
Isn't (p == (*f))? And isn't the binding of the lvalue in that last line: (*(p++))? And if so, then surely the p++ in the last line is exactly the same as *f += 2 ('cos p is a pointer to a 2 byte thingy)? But obviously not... But what was being incremented by that ++ then, and why didn't it hurt?

Seems to me that the two calls to fwriteshort() that occur together are writing the same piece of memory instead of consecutive pieces of memory.

Yup. The 'MTrk' was being placed at postition 8 in the file (the first byte being at position 0) instead of position 14.

[smite-meister]

I've worked out where the error crept in.

Oops, my bad. *guilty* Thanks for finding that one out.
qmus2mid had originally a really convoluted way of doing the endianness correction.
When I did the 64-bit-cleaning for Legacy 2, it seems I messed that one up.
I simplified the endianness stuff and committed the fix (http://doomlegacy.svn.sourceforge.net/viewvc/doomlegacy?diff_format=h&view=rev&revision=425) to the SVN.

I wanted to see if qmus2mid now works, and added this to i_sound.cpp:

ville@azathoth:~/legacy/trunk$ svn diff interface/sdl/i_sound.cpp
Index: interface/sdl/i_sound.cpp
================================================== =================
--- interface/sdl/i_sound.cpp (revision 422)
+++ interface/sdl/i_sound.cpp (working copy)
@@ -586,6 +586,10 @@
return 0;
}

+ FILE *xxx = fopen("qmus2mid.mid", "wb");
+ fwrite(mus2mid_buffer, midlength, 1, xxx);
+ fclose(xxx);
+
music.rwop = SDL_RWFromConstMem(mus2mid_buffer, midlength);
}
else

Before the fix, the MIDI file produced was corrupted as expected, and after the fix it plays flawlessly using the commandline timidity player, so I guess we can assume qmus2mid now works (again!).

Edit: Also, with SDL_mixer 1.2.7, the in-game MIDI music works fine. It was not a 64-bit issue after all...

BTW, Legacy-1 was never made 64-bit clean.

[smite-meister]

I think that it would be cool to give 'em a legacy-1 with music working. I hope to do one more legacy-1 release with music (under SDL) working.

Yeah, I agree, although Legacy 2 should be the main dev effort.

[chippo]

so I guess we can assume qmus2mid now works (again!).

But. The in-game midi music still won't work, SDL_mixer still gives me the error "Module format not recognized", which really means that Timidity cannot recognize the data it is given.

But just to be sure, would you post the output of:
hexdump -C < qmus2mid.mid | head

TIA,
chiplet

[chippo]

My system is an AMD64. I wonder if this is a 64-bit issue in the Timidity version included with SDL_mixer?

But since the standalone timidity++ does seem to be 64-bit clean ('cos you can play the file), it shouldn't be too difficult to back port the relevant pieces from timidity++ to SDL_mixer....

Oops, probably not that easy, and clearly why it ain't 64-bit clean. From http://www.libsdl.org/cgi/viewvc.cgi/trunk/SDL_mixer/timidity/?sortby=date#dirlist it looks like the last time SDL_mixer synced with standalone TiMidity, was in 1995 with version v0.2i.

BTW, Legacy-1 was never made 64-bit clean.

But that's why you bought an amd64. :-)

[GhostlyDeath]

Not sure if this is any use to you, just a random snippet from my IRC logs:

In my Legacy port named ReMooD:

else if (!memcmp(data,"MThd",4))
{ // support mid file in WAD !!! (no conversion needed)
pMidiFileData = data;
iMus2MidSize = len; // would this work?
}

[chippo]

In my Legacy port named ReMooD:

else if (!memcmp(data,"MThd",4))
{ // support mid file in WAD !!! (no conversion needed)
pMidiFileData = data;
iMus2MidSize = len; // would this work?
}

The code you're refering to is only applicable in the os2 and win32 builds. And only in legacy-1. We're talking about the SDL build and legacy-2.

I admit that SDL (evilly) doesn't appear in the subject (mea culpa), but it is the legacy-2 forum.

But thanks for the effort,
chippig

[g6672D]

Come to that, are there any Doom ports that have full 64-bit support? At the moment it seems, 64-bit operating systems are hampered by compatibility problems which somewhat negates the value of doing so.

[Ajapted]

However, what is curious to me is the following. Here is the code before my fix:inline void fwriteshort(Uint16 x, byte **f)
{
Sint16 *p = reinterpret_cast<Sint16*>(*f);
*p++ = ((x>>8) & 0xff) | (x<<8);
}
Isn't (p == (*f))? And isn't the binding of the lvalue in that last line: (*(p++))? And if so, then surely the p++ in the last line is exactly the same as *f += 2 ('cos p is a pointer to a 2 byte thingy)? But obviously not... But what was being incremented by that ++ then, and why didn't it hurt?
The 'p' variable is simply a local variable, not a binding, so the ++ actually does nothing here.

[chippo]

The 'p' variable is simply a local variable, not a binding, so the ++ actually does nothing here.

Right, it's just on the stack... duh!

So, what the original coder probably intended was:inline void fwriteshort(Uint16 x, byte **f)
{
Sint16** p = reinterpret_cast<Sint16**>(f);
*(*p)++ = ((x>>8) & 0xff) | (x<<8);
}

BTW, (regarding the reinterpret_cast) how would the following C++ compile differently from the above?inline void fwriteshort(Uint16 x, byte **f)
{
Sint16** p = (Sint16**)f;
*((*p)++) = ((x>>8) & 0xff) | (x<<8);
}

Just looking at this code, though ..., surely the cast to signed is dangerous. If the MSbit is set, aren't some compilers on some architectures (depending on 1' or 2's complement issues) allowed to carry the sign bit for the right shift? Another advantage of using unsigned, is that you won't need to worry about the &ing against 0xff.

If this code hadn't been replaced in SVN yesterday, I would have advised an update to something like:inline void fwriteshort(Uint16 x, byte **f)
{
Uint16** p = (Uint16**)f;
*((*p)++) = (x>>8) | (x<<8);
}

Is seems that I'm not alone in worrying about the signed issue when manipulating bytes. In his patch of yesterday to qmus2mid.cpp (http://doomlegacy.svn.sourceforge.net/viewvc/doomlegacy/legacy/trunk/audio/qmus2mid.cpp?diff_format=h&r1=425&r2=424&pathrev=425), smite-meister indeed changed the signed arithmetic to unsigned. However, while reviewing the changes to the other committed file m_swap.h (http://doomlegacy.svn.sourceforge.net/viewvc/doomlegacy/legacy/trunk/include/m_swap.h?diff_format=h&r1=425&r2=424&pathrev=425), I see that it still (shamefully) is using signed logic. :-( Smite-meister's commit didn't cause this, though.

[Ajapted]

So, what the original coder probably intended was:inline void fwriteshort(Uint16 x, byte **f)
{
Sint16** p = reinterpret_cast<Sint16**>(f);
*(*p)++ = ((x>>8) & 0xff) | (x<<8);
}
That looks truly horrendous, even if it works ;)

I don't remember the difference between reinterpret_cast and C-style casting, I think they do exactly the same thing (I always use the C-style casts).

Just looking at this code, though ..., surely the cast to signed is dangerous. If the MSbit is set, aren't some compilers on some architectures (depending on 1' or 2's complement issues) allowed to carry the sign bit for the right shift?
Yes, I don't remember if compilers must carry the sign, but some definitely do. Unsigned is definitely better for doing these byte swap functions.

[smite-meister]

BTW, (regarding the reinterpret_cast) how would the following C++ compile differently from the above?inline void fwriteshort(Uint16 x, byte **f)
{
Sint16** p = (Sint16**)f;
*((*p)++) = ((x>>8) & 0xff) | (x<<8);
}

They should compile to identical code. The parentheses around (*p)++ are superfluous, though. reinterpret_cast and the traditional C cast are AFAIK entirely equivalent, but I prefer reinterpret_cast since the meaning is explicit and text editors can use syntax highlighting on it.

http://cplusplus.com/doc/tutorial/typecasting.html gives a good exposition of the new C++ cast operators.


Just looking at this code, though ..., surely the cast to signed is dangerous.

No, the shift is performed on x, which is unsigned. Only the result is assigned to a signed variable of the same size, which preserves the binary pattern. The signedness of p is not significant here. Of course unsigned would cause much less confusion.


However, while reviewing the changes to the other committed file m_swap.h (http://doomlegacy.svn.sourceforge.net/viewvc/doomlegacy/legacy/trunk/include/m_swap.h?diff_format=h&r1=425&r2=424&pathrev=425), I see that it still (shamefully) is using signed logic. :-(
It's not, the shifts are again unsigned. The final cast to Sint16 or Sint32 is in fact useless and should probably be removed. It would only make a difference if the result is assigned to a non-integer variable or an integer variable of greater size.

[smite-meister]

Yes, I don't remember if compilers must carry the sign, but some definitely do.
This simply must be defined in the C standard, you can't leave stuff like this for the compiler-writers to decide. AFAIR >> carries the sign if and only if the target is signed.

[chippo]

This simply must be defined in the C standard, you can't leave stuff like this for the compiler-writers to decide.

This might be spec'ed in a recent ANSI/ISO document, but I can assure you that a hundred years ago, when I was a varsity, there were compilers (C) that behaved differently (from each other).

But maybe it's not spec'ed now, for efficiency reasons. They could have reasoned:
- Of the many cases (big-endian vs little-endian x mixed-endian) x (1's-complement vs 2's-complement), for some of them, it would cost an extra instruction or two to do The Right Thing (tm) with the sign bit. (Ideally signed-variable>>13 should be done with a single (bit) shift machine instruction.)
- It's stoopid to right shift signed numbers, so don't do it.
Therefore, we're not going to spec what happens when you right shift signed entities; the compiler-writer can choose some efficient way.

While googling to try find the answer, I come across +-1990 info, like this article: Notes on Writing Portable Programs in C (http://www.psgd.org/paul/docs/cport/cport.htm) when on page 11 (http://www.psgd.org/paul/docs/cport/cport11.htm) they say:Shift operators:
When shifting signed ints right, the vacated bits might be filled with zeroes or with copies of the sign bit. unsigned ints will be filled with zeroes.

But since we all agree that it's _clearer_ to use unsigneds for byte manipulation I guess there's no problem for future commits.

[Ajapted]

This simply must be defined in the C standard, you can't leave stuff like this for the compiler-writers to decide. AFAIR >> carries the sign if and only if the target is signed.
The draft ANSI C standard at the back of the K&R book "the C programming language" states:
The value of E1>>E2 is E1 right-shifted E2 bit positions. The right shift is equivalent to division by 2^E2 if E1 unsigned or if it has a non-negative value; otherwise the result is implementation-defined.
According to that, what happens with signed negative values depends on the implementation.

Of course this may have changed with the later standards like C99, and C++ may have standardized on a certain behaviour.