Add support for the DECPS (Play Sound) escape sequence #13208
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -410,6 +410,10 @@ Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "InteractivityOneCore", "src | |
| EndProject | ||
| Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "RendererWddmCon", "src\renderer\wddmcon\lib\wddmcon.vcxproj", "{75C6F576-18E9-4566-978A-F0A301CAC090}" | ||
| EndProject | ||
| Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Audio", "Audio", "{40BD8415-DD93-4200-8D82-498DDDC08CC8}" | ||
| EndProject | ||
| Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "MidiAudio", "src\audio\midi\lib\midi.vcxproj", "{3C67784E-1453-49C2-9660-483E2CC7F7AD}" | ||
| EndProject | ||
| Global | ||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
| AuditMode|Any CPU = AuditMode|Any CPU | ||
|
|
@@ -3449,6 +3453,46 @@ Global | |
| {75C6F576-18E9-4566-978A-F0A301CAC090}.Release|x64.Build.0 = Release|x64 | ||
| {75C6F576-18E9-4566-978A-F0A301CAC090}.Release|x86.ActiveCfg = Release|Win32 | ||
| {75C6F576-18E9-4566-978A-F0A301CAC090}.Release|x86.Build.0 = Release|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|Any CPU.ActiveCfg = AuditMode|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|ARM.ActiveCfg = AuditMode|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|ARM64.Build.0 = AuditMode|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|DotNet_x64Test.ActiveCfg = AuditMode|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|DotNet_x86Test.ActiveCfg = AuditMode|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|x64.ActiveCfg = AuditMode|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|x64.Build.0 = AuditMode|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|x86.ActiveCfg = AuditMode|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.AuditMode|x86.Build.0 = AuditMode|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|Any CPU.ActiveCfg = Debug|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|ARM.ActiveCfg = Debug|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|ARM64.ActiveCfg = Debug|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|ARM64.Build.0 = Debug|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|DotNet_x64Test.ActiveCfg = Debug|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|DotNet_x86Test.ActiveCfg = Debug|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|x64.ActiveCfg = Debug|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|x64.Build.0 = Debug|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|x86.ActiveCfg = Debug|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Debug|x86.Build.0 = Debug|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|Any CPU.ActiveCfg = Fuzzing|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|ARM.ActiveCfg = Fuzzing|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|ARM64.ActiveCfg = Fuzzing|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|ARM64.Build.0 = Fuzzing|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|DotNet_x64Test.ActiveCfg = Fuzzing|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|DotNet_x86Test.ActiveCfg = Fuzzing|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|x64.ActiveCfg = Fuzzing|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|x64.Build.0 = Fuzzing|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|x86.ActiveCfg = Fuzzing|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Fuzzing|x86.Build.0 = Fuzzing|Win32 | ||
|
Comment on lines
+3476
to
+3485
There was a problem hiding this comment. I'm guessing we can remove the Fuzzing config here There was a problem hiding this comment. This is a dependency of host. It must be built for fuzzing because host must be built for fuzzing. There was a problem hiding this comment. OK. I wasn't sure what exactly was needed here - I just accepted what Visual Studio generated automatically. Edit: Only saw @DHowett's message after I replied. Fuzzing gets to live. |
||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|Any CPU.ActiveCfg = Release|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|ARM.ActiveCfg = Release|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|ARM64.ActiveCfg = Release|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|ARM64.Build.0 = Release|ARM64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|DotNet_x64Test.ActiveCfg = Release|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|DotNet_x86Test.ActiveCfg = Release|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|x64.ActiveCfg = Release|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|x64.Build.0 = Release|x64 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|x86.ActiveCfg = Release|Win32 | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD}.Release|x86.Build.0 = Release|Win32 | ||
| EndGlobalSection | ||
| GlobalSection(SolutionProperties) = preSolution | ||
| HideSolutionNode = FALSE | ||
|
|
@@ -3552,6 +3596,8 @@ Global | |
| {8222900C-8B6C-452A-91AC-BE95DB04B95F} = {05500DEF-2294-41E3-AF9A-24E580B82836} | ||
| {06EC74CB-9A12-428C-B551-8537EC964726} = {E8F24881-5E37-4362-B191-A3BA0ED7F4EB} | ||
| {75C6F576-18E9-4566-978A-F0A301CAC090} = {05500DEF-2294-41E3-AF9A-24E580B82836} | ||
| {40BD8415-DD93-4200-8D82-498DDDC08CC8} = {89CDCC5C-9F53-4054-97A4-639D99F169CD} | ||
| {3C67784E-1453-49C2-9660-483E2CC7F7AD} = {40BD8415-DD93-4200-8D82-498DDDC08CC8} | ||
| EndGlobalSection | ||
| GlobalSection(ExtensibilityGlobals) = postSolution | ||
| SolutionGuid = {3140B1B7-C8EE-43D1-A772-D82A7061A271} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| #include "precomp.h" | ||
| #include "MidiAudio.hpp" | ||
| #include "../terminal/parser/stateMachine.hpp" | ||
|
|
||
| namespace | ||
| { | ||
| class MidiOut | ||
| { | ||
| public: | ||
| static constexpr auto NOTE_OFF = 0x80; | ||
| static constexpr auto NOTE_ON = 0x90; | ||
| static constexpr auto PROGRAM_CHANGE = 0xC0; | ||
|
|
||
| // We're using a square wave as an approximation of the sound that the | ||
| // original VT525 terminals might have produced. This is probably not | ||
| // quite right, but it works reasonably well. | ||
| static constexpr auto SQUARE_WAVE_SYNTH = 80; | ||
|
|
||
| MidiOut() noexcept | ||
| { | ||
| midiOutOpen(&handle, MIDI_MAPPER, NULL, NULL, CALLBACK_NULL); | ||
| OutputMessage(PROGRAM_CHANGE, SQUARE_WAVE_SYNTH); | ||
| } | ||
| ~MidiOut() noexcept | ||
| { | ||
| midiOutClose(handle); | ||
| } | ||
| void OutputMessage(const int b1, const int b2, const int b3 = 0, const int b4 = 0) noexcept | ||
| { | ||
| midiOutShortMsg(handle, MAKELONG(MAKEWORD(b1, b2), MAKEWORD(b3, b4))); | ||
| } | ||
|
|
||
| MidiOut(const MidiOut&) = delete; | ||
| MidiOut(MidiOut&&) = delete; | ||
| MidiOut& operator=(const MidiOut&) = delete; | ||
| MidiOut& operator=(MidiOut&&) = delete; | ||
|
|
||
| private: | ||
| HMIDIOUT handle = nullptr; | ||
| }; | ||
| } | ||
|
|
||
| using namespace std::chrono_literals; | ||
|
|
||
| MidiAudio::~MidiAudio() noexcept | ||
| { | ||
| try | ||
| { | ||
| #pragma warning(suppress : 26447) | ||
| // We acquire the lock here so the class isn't destroyed while in use. | ||
| // If this throws, we'll catch it, so the C26447 warning is bogus. | ||
| _inUseMutex.lock(); | ||
|
There was a problem hiding this comment. Forgive me for jumping in like this, but isn't using an object while it is being destructed undefined behavior anyway? So this needs to be prevented in some other way? There was a problem hiding this comment. I wouldn't have thought so. Obviously the destructor itself is going to be using the object while it is being destructed, so I don't see why it should be any different for other methods. It's just not something that can be relied on in general, because a destructor may be busy cleaning up resources that the other methods depend on. But that's not the case here. I may be wrong though. I'm not an expert on the standard. Do you have a reference to somewhere that documents this as undefined behavior? If necessary we could always call the There was a problem hiding this comment. https://docs.microsoft.com/en-us/cpp/standard-library/mutex-class-stl?view=msvc-170#dtormutex_destructor and https://en.cppreference.com/w/cpp/thread/mutex/%7Emutex say that the behavior is undefined if an std::mutex is destroyed while owned. It looks like that will happen here, as MidiAudio::~MidiAudio locks _inUseMutex and leaves it locked and the members are then destroyed. There was a problem hiding this comment. Oh sorry, I misunderstood what @PetterS was saying. If the issue is just that the mutex shouldn't be destroyed while locked, we can just unlock it here as well. We're only acquiring the lock to make sure the playing thread isn't still holding it - we don't need to retain the lock. There was a problem hiding this comment. Alright, thanks. Perhaps it is OK as long as the mutex is also unlocked here. |
||
| } | ||
| catch (...) | ||
| { | ||
| // If the lock fails, we'll just have to live with the consequences. | ||
| } | ||
| } | ||
|
|
||
| void MidiAudio::Initialize() | ||
| { | ||
| _shutdownFuture = _shutdownPromise.get_future(); | ||
| } | ||
|
|
||
| void MidiAudio::Shutdown() | ||
| { | ||
| // Once the shutdown promise is set, any note that is playing will stop | ||
| // immediately, and the Unlock call will exit the thread ASAP. | ||
| _shutdownPromise.set_value(); | ||
| } | ||
|
|
||
| void MidiAudio::Lock() | ||
| { | ||
| _inUseMutex.lock(); | ||
| } | ||
lhecker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| void MidiAudio::Unlock() | ||
| { | ||
| // We need to check the shutdown status before releasing the mutex, | ||
| // because after that the class could be destroyed. | ||
| const auto shutdownStatus = _shutdownFuture.wait_for(0s); | ||
| _inUseMutex.unlock(); | ||
| // If the wait didn't timeout, that means the shutdown promise was set, | ||
| // so we need to exit the thread ASAP by throwing an exception. | ||
| if (shutdownStatus != std::future_status::timeout) | ||
| { | ||
| throw Microsoft::Console::VirtualTerminal::StateMachine::ShutdownException{}; | ||
|
There was a problem hiding this comment. Instead of using an exception to abort processing the next notes, would it make sense to use a boolean member instead and just return early in all further calls to That way we wouldn't need to make any accommodations for this new exception type and the control flow would be contained entirely within this class. There was a problem hiding this comment. The problem with using a boolean is we then have to propagate that state up through the entire call stack, including all the following (and then some):
But half of those calls already use a boolean return value to signal conpty passthrough, so we'd need to come up with some other mechanism for differentiating between those states. In comparison the exception solution seemed a whole lot easier. There was a problem hiding this comment. Oh, I thought it'd be fine to just do this: void MidiAudio::PlayNote(const int noteNumber, const int velocity, const std::chrono::microseconds duration)
{
if (_shutdown)
{
return;
}
// ...
}It would still process the VT sequence, but effectively stop doing anything with it. There was a problem hiding this comment. Ah I see... The problem is this bit right?
I'll have to reread the PR again, because I haven't entirely understood yet why this is necessary. The "unlocked interval" is The reason I'm vary of using an exception here is because a lot of our code isn't exception safe (including There was a problem hiding this comment. A note can last up to about 8 seconds I think. But even if it's only say half a second, you could have a whole buffer of notes still pending that could last for minutes. If we just handled this the way we do other sequences, the UI would hang for that entire duration, because we usually only release the terminal lock once the buffer is empty. So that's why I've got the terminal temporarily unlocked while the sounds is in progress. But if the app/tab is closed during that period, we can't just carry on processing the rest of the buffer, because the user wouldn't want the remaining 10 minutes of audio to be output after they've closed. Also at that point, half of the framework would have been destroyed anyway, so any attempt to continue processing output would likely end up in something crashing. There was a problem hiding this comment. I'm not sure I was able to express my suggestion well, so just to be sure: In my opinion the benefits from an exception-less approach in this particular case are an improved robustness against breaking this feature accidentally in the future. For instance if someone were to add a There was a problem hiding this comment.
But that's not the way it currently works. When
That's why I say we need to exit the thread ASAP. I mean like immediately that instant. There's no time for anything else. I know this feels a bit precarious, but I can't see a cleaner way to handle it. What you're proposing would just crash (at least as I understand it). There was a problem hiding this comment. This makes sense for TermControl. Who in I see that MIDI is shut down during Will another process in the same session stand a chance of There was a problem hiding this comment. In conhost it's caught in the I'm not sure I understand what you mean by multiple processes in the same session. The MIDI handle itself is a shared resource that stays open for as long as the terminal process is running. The I've tested opening and closing multiple tabs while they're all playing sounds at the same time, so I know that works. I don't know if you mean something else though. There was a problem hiding this comment. Ah, I'm thinking about one conhost session (not in Terminal) with multiple attached processes. I misremembered the purpose of ACK on it being caught in |
||
| } | ||
| } | ||
|
|
||
| void MidiAudio::PlayNote(const int noteNumber, const int velocity, const std::chrono::microseconds duration) | ||
| { | ||
| // The MidiOut is a local static because we can only have one instance, | ||
| // and we only want to construct it when it's actually needed. | ||
| static MidiOut midiOut; | ||
|
|
||
| if (velocity) | ||
| { | ||
| midiOut.OutputMessage(MidiOut::NOTE_ON, noteNumber, velocity); | ||
| } | ||
|
|
||
| // By waiting on the shutdown future with the duration of the note, we'll | ||
| // either be paused for the appropriate amount of time, or we'll break out | ||
| // of the wait early if we've been shutdown. | ||
| _shutdownFuture.wait_for(duration); | ||
|
|
||
| if (velocity) | ||
| { | ||
| midiOut.OutputMessage(MidiOut::NOTE_OFF, noteNumber, velocity); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /*++ | ||
| Copyright (c) Microsoft Corporation | ||
| Licensed under the MIT license. | ||
|
|
||
| Module Name: | ||
| - MidiAudio.hpp | ||
|
|
||
| Abstract: | ||
| This modules provide basic MIDI support with blocking sound output. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <future> | ||
| #include <mutex> | ||
|
|
||
| class MidiAudio | ||
| { | ||
| public: | ||
| MidiAudio() = default; | ||
| MidiAudio(const MidiAudio&) = delete; | ||
| MidiAudio(MidiAudio&&) = delete; | ||
| MidiAudio& operator=(const MidiAudio&) = delete; | ||
| MidiAudio& operator=(MidiAudio&&) = delete; | ||
| ~MidiAudio() noexcept; | ||
| void Initialize(); | ||
| void Shutdown(); | ||
| void Lock(); | ||
| void Unlock(); | ||
| void PlayNote(const int noteNumber, const int velocity, const std::chrono::microseconds duration); | ||
|
|
||
| private: | ||
| std::promise<void> _shutdownPromise; | ||
| std::future<void> _shutdownFuture; | ||
| std::mutex _inUseMutex; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <ProjectGuid>{3c67784e-1453-49c2-9660-483e2cc7f7ad}</ProjectGuid> | ||
| <Keyword>Win32Proj</Keyword> | ||
| <RootNamespace>midi</RootNamespace> | ||
| <ProjectName>MidiAudio</ProjectName> | ||
| <TargetName>MidiAudio</TargetName> | ||
| <ConfigurationType>StaticLibrary</ConfigurationType> | ||
| </PropertyGroup> | ||
| <Import Project="$(SolutionDir)src\common.build.pre.props" /> | ||
| <Import Project="$(SolutionDir)src\common.nugetversions.props" /> | ||
| <ItemGroup> | ||
| <ClCompile Include="..\MidiAudio.cpp" /> | ||
| <ClCompile Include="..\precomp.cpp"> | ||
| <PrecompiledHeader>Create</PrecompiledHeader> | ||
| </ClCompile> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <ClInclude Include="..\MidiAudio.hpp" /> | ||
| <ClInclude Include="..\precomp.h" /> | ||
| </ItemGroup> | ||
| <!-- Careful reordering these. Some default props (contained in these files) are order sensitive. --> | ||
| <Import Project="$(SolutionDir)src\common.build.post.props" /> | ||
| <Import Project="$(SolutionDir)src\common.nugetversions.targets" /> | ||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| #include "precomp.h" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| /*++ | ||
| Copyright (c) Microsoft Corporation | ||
| Licensed under the MIT license. | ||
|
|
||
| Module Name: | ||
| - precomp.h | ||
|
|
||
| Abstract: | ||
| - Contains external headers to include in the precompile phase of console build process. | ||
| - Avoid including internal project headers. Instead include them only in the classes that need them (helps with test project building). | ||
| --*/ | ||
|
|
||
| #pragma once | ||
|
|
||
| // clang-format off | ||
|
|
||
| // This includes support libraries from the CRT, STL, WIL, and GSL | ||
| #include "LibraryIncludes.h" | ||
|
|
||
| #ifndef WIN32_LEAN_AND_MEAN | ||
| #define WIN32_LEAN_AND_MEAN // Exclude rarely-used stuff from Windows headers | ||
| #define NOMCX | ||
| #define NOHELP | ||
| #define NOCOMM | ||
| #endif | ||
|
|
||
| // Windows Header Files: | ||
| #include <windows.h> | ||
|
|
||
| // clang-format on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with AuditMode too? Just making sure before it gets committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, if the build below is succeeding with the
AuditMode|***.Build.0lines (which mean that audit build is enabled), this is working in audit mode.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely compile it with the AuditMode configuration selected if that's what you mean. And I know it's actually doing something because it picked up a bunch of issues I needed to fix. I don't know if we actually need all of those options though - again I just accepted whatever Visual Studio created for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, just making sure we're actually listening to whatever AuditMode catches, because VS adds it in and we don't check it sometimes. Sounds like everything is good here then :)