#8064 closed patch
Pitch bend range implementation
| Reported by: | SF/jamieson630 | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Port: Win32 |
| Version: | Keywords: | ||
| Cc: | Game: | Day of the Tentacle |
Description
This patch corrects the pitch bend effect when SCUMMVM is using Windows MMSYSTEM (the - ewindows option).
Pitch bend information was being manipulated before being stored, in order to accomodate value ranges for Adlib output. Consequently, it became almost impossible to reconstruct the appropriate values for MIDI messages -- and in fact, the values being sent out were way out of whack.
The original pitch bend values (which are MIDI- compliant) are now what is being stored. Transformations needed for Adlib are now being performed within Adlib functions.
I tested this patch under -ewindows and -eadlib on Day of the Tentacle, primarily using Green Tentacle's band music and the opening logo sounds. I also checked out more subtle pitch bend effects used during the second cutscene (after Bernard finds the secret lab, through the vortex sequence).
If anybody is using other music systems, please test this! There is a highly convoluted relationship between pitch bends, detunes and transpositions that changes from one driver to the next, so I'm not completely sure that I didn't corrupt the pitch bend values needed by other drivers.
The following revisions are patched: imuse.cpp - Revision 1.12 mididrv.cpp - Revision 1.5
(Note: There are a couple miscellaneous tab-indent corrections in this patch as well, just to get things in line with cosmetic guidelines.)
Ticket imported from: #614005. Ticket imported from: patches/169.
Attachments (8)
Change History (32)
by , 22 years ago
| Attachment: | pitchbend.diff added |
|---|
comment:1 by , 22 years ago
Would help if I actually attach the patch. ;) Thanks to Trinity78 for pointing this out.
comment:2 by , 22 years ago
Seems to work OK here on Mac OS X - that is i can't notice any negative effects in the DOTT "rock music" made by green tentacle. Then again I am not sure whether I notice any effects :-) Maybe pitchbend is not implemented here anyway.
comment:3 by , 22 years ago
| Owner: | set to |
|---|
comment:5 by , 22 years ago
Fingolfin, are you staying that you don't hear ANY pitch bending on Mac OS X? That makes the guitar solo sound really unnatural, more like a keyboard than a wailing guitar.
Would it help if I try to sample the -eadlib and -ewindows output off my box to demonstrate the target sound for that riff?
comment:6 by , 22 years ago
Ender, indeed I did - but accidentally :-( I was not being careful enough, shame on me.
jamieson, yeah that would be helpful.
comment:7 by , 22 years ago
I am attaching three MP3 files to illustrate the pitch bend problem. These are all variations on the stereo music "Green T and the Sushi Platter".
sushi_adlib.mp3 --------------- This is the -eadlib rendering. Since the iMuse code seems to be geared around Adlib output, the pitch bend has always worked fine. In the patch, I moved Adlib-specific transformations to the IMuse_Adlib class, so the output remains unchanged.
sushi_windows.mp3 ----------------- This is the -ewindows rendering with this patch enabled.
sushi_windows.bad.mp3 --------------------- This is the -ewindows rendering without this patch. Notice how all of the pitch bends are virtually non-existent. This is because 1) the pitch bend range for the sound card was not initialized to +/- 12 semitones and is still in its default state of +/- 2 semitones; and 2) the pitch bend values were getting factored down internally for Adlib, which is not an appropriate transformation for MPU-401 output. Consequently, the final pitch bend values would only change the pitch by a few cents.
by , 22 years ago
| Attachment: | sushi_windows.mp3 added |
|---|
-ewindows, with patch (44KHz 16-bit M, 24 Kbps)
by , 22 years ago
| Attachment: | sushi_windows.bad.mp3 added |
|---|
-ewindows, without patch (44KHz 16-bit M, 24 Kbps)
comment:8 by , 22 years ago
OK so now the QuickTime output sounds aweful. This is because QTMA actually expects the pitchbend data as an 8.8 fixed point, and allows you to specify arbitrary pitchbend this way. So I changed the code to scale the incoming data to +/- 12 semitones, but the result still sounds pretty bad. I am wondering if there is really everything correct? I tried to scale the range to +/- 6 and while that doesn't sound completly correct either, it's much better. Hrm.
comment:9 by , 22 years ago
Sorry for that weird wrapping, Internet Explore 5 here is buggy :-(
Anyway I forgot to mention that when I change the pitch bend range for the CoreAudio backend (the other OS X midi backend) by sending it exactly the midi commands you use for the windows backend, it results in the same "wrong" pitch bend sounds. I think that internally the QT driver ultimatly uses CoreAudio anyway, which might explain this shared odditiy, but I am not sure.
comment:10 by , 22 years ago
Fingolfin, are you talking about the pitch bends in Sam & Max or DOTT?
For DOTT, the MIDI tracks apparently assume +/- 12 semitones, although the MIDI files don't contain explicit SysEx or control change messages to that effect. (Exception: the starting logo music did include SysEx to do explicit pitch bend sensitivity changes.)
For Sam & Max, I've already noticed a problem with pitch bending. I suspect that the Sam & Max MIDI files finally started using the Standardized MIDI control change messages for changin pitch bend sensitivity. Right now ScummVM has no functionality to interpret those. (In fact, this patch here doesn't even deal with SysEx messages to change pitch bend sensitivity -- it just assumes it will be +/- 12 semitones for everything.)
The above gaps in functionality are something I can address once I'm off work next week. In the meantime, Fingolfin, can you verify for me that DOTT at least sounds okay on OS X, and if not, can you upload an MP3 recording of the Green T output so I can hear how the pitch bends sound on that version? Thanks.
comment:11 by , 22 years ago
Actually all my tests were made with Green Ts rock music!
Sorry I have no clue how I could capture the audio output here to a file... I'll look into it maybe I'll find a way.
comment:12 by , 22 years ago
I have to apply this tiny patch, or the carnival music at the end of Monkey Island 2 sounds terrible.
It simply makes the same pitchbend change to mc_key_on() that the original patch does to part_changed().
comment:14 by , 22 years ago
Still working on some issues with QT and CoreAudio -- this is slow going since I don't have access to a Mac locally. The QT problems seem to be more than just the *range* of pitch bending, but still related to pitch bend issues in general, so I'd like to see it addressed in this patch.
Any feedback yet on ALSA pitch bend behaviors?
by , 22 years ago
| Attachment: | imuse.pitchbend2.diff added |
|---|
Patch against iMuse.cpp 1.54 (October 12, 2002)
comment:15 by , 22 years ago
I'm including imuse.pitchbend2.diff, which provides more thorough support for pitch bend ranges. Sam & Max use on- the-fly changes to the pitch bend range in various musical passages. Without tracking the current pitch bend range, and sending out appropriate signals, several of the musical passages (including the opening cutscene) sound out-of-tune or otherwise screwy.
I kind of hesitated to upgrade the part_changed parameter to a 16-bit value, but I couldn't really see any other option. Pitch bend range needs to be tracked just like any of those other parameters -- and there weren't any more bits to play with.
(Note: this patch does not address any QT issues I mentioned earlier. Still mulling over those. Fingolfin and I are about ready to chalk it up to QTMA pitch bend implementation bugs.... So, Ender, if you want to close this, we can always just open up something new if we hit on a solution.)
comment:17 by , 22 years ago
I tried the DOTT intro and Green T music with the ALSA driver, and it didn't sound correct. It wasn't bending the notes far enough. Unfortunately I don't know how to capture the sound to a file.
I tried humming the note at the end of the "boing" sound at the beginning of the intro and comparing it to my piano, and I think it came out as an E flat. Does that help at all?
comment:18 by , 22 years ago
Hmm... Now the pitch bend for MI2 and DOTT is broken. The problem seems to have been introduced when the default _pitchbend_factor was changed from 2 to 12.
Which games is it that needs 12 by default?
comment:19 by , 22 years ago
Okay, the +/- 12 semitone pitch bend range is not a valid assumption for DOTT after all, but apparently the channels were initially set up with no range at all. I have gone back to +/- 2 semitones unless the MIDI track calls for something else, but the new architecture based on part_changed seems to still play the DOTT music (and relevant MI2 music, e.g. Largo's theme) correctly under Adlib and Windows.
I have committed changes that should greatly improve pitch bend behavior under QTMA, especially in Sam & Max. However, being without a Mac I have no idea what the results will be. Fingolfin?
(BTW, looking at the QTMA code, I would think that the control change messages used to set pitch bend range for Windows MMSYSTEM would cause an "Unknown MIDI effect" error under QTMA. This may need to be reclassified as a warning to avoid unwarranted program termination.)
comment:20 by , 22 years ago
| Summary: | Pitch bend for MMSYSTEM → Pitch bend range implementation |
|---|
comment:21 by , 22 years ago
Ack -- I broke the Mac compile with the latest commit. That's what comes of doing Mac changes on Windows at 4 in the morning. ;)
I am attaching a patch with corrections, but I won't directly commit this time; Fingolfin, can you review these for syntactical correctness and then let me know how it affects QT output?
comment:22 by , 22 years ago
| Status: | new → closed |
|---|
comment:23 by , 22 years ago
Closing this, pitch bend works mostly fine, any remaining problem can be resolved outside of this tracker item.
comment:24 by , 6 years ago
| Component: | → Port: Win32 |
|---|---|
| Game: | → Day of the Tentacle |
Patch against a September 24 CVS snapshot