We might need another dependency for MIDI support #13252
Comments
zadjii-msft
added
Area-VT
ghost
added
the
Needs-Triage
|
Btw, if you want to revert PR #13208 until we can figure out a solution for this, that's fine by me. It's not exactly an essential feature. And if in turns out that we can't use the MIDI API, I could try and come up with another way to produce the sound. I just chose MIDI because it seemed easiest. |
zadjii-msft
removed
the
Needs-Triage
|
Plan of record right now is to stash this behind velocity, so it only ships in Preview (and critically, not in the OS-side build). That way we can get a little longer window on figuring out how we want to resolve this. |
|
Hi, I just tried to compile Terminal from 4e20a86 and installing it from a loose package as detailed in https://github.com/microsoft/terminal/blob/main/doc/building.md. I can confirm that installing the loose package fails for me with (pardon my French!) I'm currently building from |
|
Curious. That package should be available on your system, @phil-blain, somewhere in |
|
It is indeed in |
* conhost requires an additional dependency in Windows, which might cause us trouble in WPG * Terminal requires an additional *package* dependency, which *will* cause us trouble in WPG (since GmDls is about 3MB) I chose to scope the feature checks to MidiOut directly, as I wanted to keep the delay behavior in MidiAudio::PlayNote. This is negotiable. References #13252
|
With #13258 merged, I'm moving this out to 1.16 |
DHowett
removed
the
Severity-Blocking
|
@DHowett FYI, I've been playing around with an alternate implementation that uses DirectSound to generate the audio in place of Midi. Would it be worthwhile submitting a PR with that change, or do you have other plans for resolving this? |
|
Huh, that seems like it might be a good way out of this! We wouldn't need to ship another package dependency... I don't have any concrete plans that will resolve the |
OK, that's a deciding factor for me. I'll try and put together a PR this weekend. |
## Summary of the Pull Request The original `DECPS` implementation made use of the Windows MIDI APIs to generate the sound, but that required a 3MB package dependency for the GS wavetable DLS. This PR reimplements the `MidiAudio` class using `DirectSound`, so we can avoid that dependency. ## References The original `DECPS` implementation was added in PR #13208, but was hidden behind a velocity flag in #13258. ## PR Checklist * [x] Closes #13252 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #13252 ## Detailed Description of the Pull Request / Additional comments The way it works is by creating a sound buffer with a single triangle wave that is played in a loop. We generate different notes simply by adjusting the frequency at which that buffer is played. When we need a note to end, we just set the volume to its minimum value rather than stopping the buffer. If we don't do that, the repeated starting and stopping tends to produce a lot of static in the output. We also use two buffers, which we alternate between notes, as another way to reduce that static. One other thing worth mentioning is the handling of the buffer position. At the end of each note we save the current position, and then use an offset from that position when starting the following note. This helps produce a clearer separation between tones when repeating sequences of the same note. In an ideal world, we should really have something like an attack-decay- sustain-release envelope for each note, but the above hack seems to work reasonably well, and keeps the implementation simple. ## Validation Steps Performed I've manually tested both conhost and Terminal with the sample tunes listed in issue #8687, as well as a couple of games that I have which make use of `DECPS` sound effects.
ghost
added
Resolution-Fix-Committed
## Summary of the Pull Request The original `DECPS` implementation made use of the Windows MIDI APIs to generate the sound, but that required a 3MB package dependency for the GS wavetable DLS. This PR reimplements the `MidiAudio` class using `DirectSound`, so we can avoid that dependency. ## References The original `DECPS` implementation was added in PR #13208, but was hidden behind a velocity flag in #13258. ## PR Checklist * [x] Closes #13252 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #13252 ## Detailed Description of the Pull Request / Additional comments The way it works is by creating a sound buffer with a single triangle wave that is played in a loop. We generate different notes simply by adjusting the frequency at which that buffer is played. When we need a note to end, we just set the volume to its minimum value rather than stopping the buffer. If we don't do that, the repeated starting and stopping tends to produce a lot of static in the output. We also use two buffers, which we alternate between notes, as another way to reduce that static. One other thing worth mentioning is the handling of the buffer position. At the end of each note we save the current position, and then use an offset from that position when starting the following note. This helps produce a clearer separation between tones when repeating sequences of the same note. In an ideal world, we should really have something like an attack-decay- sustain-release envelope for each note, but the above hack seems to work reasonably well, and keeps the implementation simple. ## Validation Steps Performed I've manually tested both conhost and Terminal with the sample tunes listed in issue #8687, as well as a couple of games that I have which make use of `DECPS` sound effects. (cherry picked from commit bc79867) Service-Card-Id: 84270205 Service-Version: 1.15
|
🎉This issue was addressed in #13471, which has now been successfully released as Handy links: |
~Promoted from a Teams thread. Tracking here so we don't lose it. ~
Notes from @DHowett:
The text was updated successfully, but these errors were encountered: