-
Notifications
You must be signed in to change notification settings - Fork 8.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip DECPS/MIDI output on Ctrl+C/Break #14214
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,6 @@ | |
#include "MidiAudio.hpp" | ||
#include "../terminal/parser/stateMachine.hpp" | ||
|
||
#include <dsound.h> | ||
|
||
#pragma comment(lib, "dxguid.lib") | ||
|
||
using Microsoft::WRL::ComPtr; | ||
using namespace std::chrono_literals; | ||
|
||
|
@@ -17,12 +13,13 @@ using namespace std::chrono_literals; | |
constexpr auto WAVE_SIZE = 16u; | ||
constexpr auto WAVE_DATA = std::array<byte, WAVE_SIZE>{ 128, 159, 191, 223, 255, 223, 191, 159, 128, 96, 64, 32, 0, 32, 64, 96 }; | ||
|
||
MidiAudio::MidiAudio(HWND windowHandle) | ||
void MidiAudio::_initialize(HWND windowHandle) noexcept | ||
{ | ||
_hwnd = windowHandle; | ||
_directSoundModule.reset(LoadLibraryExW(L"dsound.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32)); | ||
if (_directSoundModule) | ||
{ | ||
if (auto createFunction = GetProcAddressByFunctionDeclaration(_directSoundModule.get(), DirectSoundCreate8)) | ||
if (const auto createFunction = GetProcAddressByFunctionDeclaration(_directSoundModule.get(), DirectSoundCreate8)) | ||
{ | ||
if (SUCCEEDED(createFunction(nullptr, &_directSound, nullptr))) | ||
{ | ||
|
@@ -35,55 +32,29 @@ MidiAudio::MidiAudio(HWND windowHandle) | |
} | ||
} | ||
|
||
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. | ||
const auto lock = std::unique_lock{ _inUseMutex }; | ||
} | ||
catch (...) | ||
{ | ||
// If the lock fails, we'll just have to live with the consequences. | ||
} | ||
} | ||
|
||
void MidiAudio::Initialize() | ||
void MidiAudio::BeginSkip() noexcept | ||
{ | ||
_shutdownFuture = _shutdownPromise.get_future(); | ||
_skip.SetEvent(); | ||
} | ||
|
||
void MidiAudio::Shutdown() | ||
void MidiAudio::EndSkip() noexcept | ||
{ | ||
// 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(); | ||
_skip.ResetEvent(); | ||
} | ||
|
||
void MidiAudio::Lock() | ||
void MidiAudio::PlayNote(HWND windowHandle, const int noteNumber, const int velocity, const std::chrono::milliseconds duration) noexcept | ||
try | ||
{ | ||
_inUseMutex.lock(); | ||
} | ||
if (_skip.is_signaled()) | ||
{ | ||
return; | ||
} | ||
|
||
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) | ||
if (_hwnd != windowHandle) | ||
{ | ||
throw Microsoft::Console::VirtualTerminal::StateMachine::ShutdownException{}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we aren't using this exception anymore, we should get rid of the definition in |
||
_initialize(windowHandle); | ||
Comment on lines
+53
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll explain reason for this change below in ControlCore. |
||
} | ||
} | ||
|
||
void MidiAudio::PlayNote(const int noteNumber, const int velocity, const std::chrono::microseconds duration) noexcept | ||
try | ||
{ | ||
const auto& buffer = _buffers.at(_activeBufferIndex); | ||
if (velocity && buffer) | ||
{ | ||
|
@@ -106,10 +77,10 @@ try | |
buffer->SetCurrentPosition((_lastBufferPosition + 12) % WAVE_SIZE); | ||
} | ||
|
||
// 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); | ||
// By waiting on the skip event with a maximum duration of the note, we'll | ||
// either be paused for the appropriate amount of time, or we'll break out early | ||
// because BeginSkip() was called. This happens for Ctrl+C or during shutdown. | ||
_skip.wait(::base::saturated_cast<DWORD>(duration.count())); | ||
|
||
if (velocity && buffer) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,10 @@ | |
#include "pch.h" | ||
#include "ControlCore.h" | ||
|
||
// MidiAudio | ||
#include <mmeapi.h> | ||
#include <dsound.h> | ||
|
||
#include <DefaultSettings.h> | ||
#include <unicode.hpp> | ||
#include <Utf16Parser.hpp> | ||
|
@@ -241,8 +245,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
{ | ||
_renderer->TriggerTeardown(); | ||
} | ||
|
||
_shutdownMidiAudio(); | ||
} | ||
|
||
bool ControlCore::Initialize(const double actualWidth, | ||
|
@@ -401,9 +403,33 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
const WORD scanCode, | ||
const ::Microsoft::Terminal::Core::ControlKeyStates modifiers) | ||
{ | ||
if (ch == L'\x3') // Ctrl+C or Ctrl+Break | ||
{ | ||
_handleControlC(); | ||
} | ||
Comment on lines
+406
to
+409
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure where to put this... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... You want this behavior to be a part of the terminal handling it right? Like, if a user binds an action to ctrl+c, the action should be executed and this new behavior shouldn't happen right? If that's the case, I suggest taking a look at |
||
|
||
return _terminal->SendCharEvent(ch, scanCode, modifiers); | ||
} | ||
|
||
void ControlCore::_handleControlC() | ||
{ | ||
if (!_midiAudioSkipTimer) | ||
{ | ||
_midiAudioSkipTimer = _dispatcher.CreateTimer(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the best way to create timers in Windows Terminal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, yeah; we can audit dispatcher use later. Is it acceptable to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation isn't particularly great
However wading through the underlying code for this, the timer is canceled before it gets restarted. |
||
_midiAudioSkipTimer.Interval(std::chrono::seconds(1)); | ||
_midiAudioSkipTimer.IsRepeating(false); | ||
_midiAudioSkipTimer.Tick([weakSelf = get_weak()](auto&&, auto&&) { | ||
if (const auto self = weakSelf.get()) | ||
{ | ||
self->_midiAudio.EndSkip(); | ||
} | ||
}); | ||
} | ||
|
||
_midiAudio.BeginSkip(); | ||
_midiAudioSkipTimer.Start(); | ||
} | ||
|
||
bool ControlCore::_shouldTryUpdateSelection(const WORD vkey) | ||
{ | ||
// GH#6423 - don't update selection if the key that was pressed was a | ||
|
@@ -1393,57 +1419,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
// - duration - How long the note should be sustained (in microseconds). | ||
void ControlCore::_terminalPlayMidiNote(const int noteNumber, const int velocity, const std::chrono::microseconds duration) | ||
{ | ||
// We create the audio instance on demand, and lock it for the duration | ||
// of the note output so it can't be destroyed while in use. | ||
auto& midiAudio = _getMidiAudio(); | ||
midiAudio.Lock(); | ||
|
||
// We then unlock the terminal, so the UI doesn't hang while we're busy. | ||
// Unlock the terminal, so the UI doesn't hang while we're busy. | ||
auto& terminalLock = _terminal->GetReadWriteLock(); | ||
terminalLock.unlock(); | ||
|
||
// This call will block for the duration, unless shutdown early. | ||
midiAudio.PlayNote(noteNumber, velocity, duration); | ||
_midiAudio.PlayNote(reinterpret_cast<HWND>(_owningHwnd), noteNumber, velocity, std::chrono::duration_cast<std::chrono::milliseconds>(duration)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The device will be reinitialized if the HWND changes. But we don't expect this to happen too often right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, we don't! |
||
|
||
// Once complete, we reacquire the terminal lock and unlock the audio. | ||
// If the terminal has shutdown in the meantime, the Unlock call | ||
// will throw an exception, forcing the thread to exit ASAP. | ||
terminalLock.lock(); | ||
midiAudio.Unlock(); | ||
} | ||
|
||
// Method Description: | ||
// - Returns the MIDI audio instance, created on demand. | ||
// Arguments: | ||
// - <none> | ||
// Return Value: | ||
// - a reference to the MidiAudio instance. | ||
MidiAudio& ControlCore::_getMidiAudio() | ||
{ | ||
if (!_midiAudio) | ||
{ | ||
const auto windowHandle = reinterpret_cast<HWND>(_owningHwnd); | ||
_midiAudio = std::make_unique<MidiAudio>(windowHandle); | ||
_midiAudio->Initialize(); | ||
} | ||
return *_midiAudio; | ||
} | ||
|
||
// Method Description: | ||
// - Shuts down the MIDI audio system if previously instantiated. | ||
// Arguments: | ||
// - <none> | ||
// Return Value: | ||
// - <none> | ||
void ControlCore::_shutdownMidiAudio() | ||
{ | ||
if (_midiAudio) | ||
{ | ||
// We lock the terminal here to make sure the shutdown promise is | ||
// set before the audio is unlocked in the thread that is playing. | ||
auto lock = _terminal->LockForWriting(); | ||
_midiAudio->Shutdown(); | ||
} | ||
} | ||
|
||
bool ControlCore::HasSelection() const | ||
|
@@ -1528,6 +1511,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
{ | ||
_closing = true; | ||
|
||
// Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song. | ||
_midiAudio.BeginSkip(); | ||
Comment on lines
+1514
to
+1515
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what required larger changes to But both options are bad IMO: We had a long history of deadlock in Windows Terminal due to the main thread acquiring the console lock. And technically we don't need one, since all we want to do is call Another downside is that I had to sprinkle the DirectSound headers into a bunch of .cpp files so that the destructor compiles without "undefined type" errors. I tried declaring a custom There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is OK for now. We can reevalute the header situation in the future. :) |
||
|
||
// Stop accepting new output and state changes before we disconnect everything. | ||
_connection.TerminalOutput(_connectionOutputEventToken); | ||
_connectionStateChangedRevoker.revoke(); | ||
|
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.
This is the new trick.