Add support for the DECPS (Play Sound) escape sequence

[j4james]

Summary of the Pull Request

The DECPS (Play Sound) escape sequence provides applications with a
way to play a basic sequence of musical notes. This emulates
functionality that was originally supported on the DEC VT520 and VT525
hardware terminals.

PR Checklist

• Closes Add support for the DECPS escape sequence #8687
• CLA signed.
• Tests added/passed
• Documentation updated.
• Schema updated.
• I've discussed this with core contributors already. Issue number
  where discussion took place: Add support for the DECPS escape sequence #8687

Detailed Description of the Pull Request / Additional comments

When a DECPS control is executed, any further output is blocked until
all the notes have finished playing. So to prevent the UI from hanging
during this period, we have to temporarily release the console/terminal
lock, and then reacquire it before returning.

The problem we then have is how to deal with the terminal being closed
during that unlocked interval. The way I've dealt with that is with a
promise that is set to indicate a shutdown. This immediately aborts any
sound that is in progress, but also signals the thread that it needs to
exit as soon as possible.

The thread exit is achieved by throwing a custom exception which is
recognised by the state machine and rethrown instead of being logged.
This gets it all the way up to the root of the write operation, so it
won't attempt to process anything further output that might still be
buffered.

Validation Steps Performed

Thanks to the testing done by @jerch on a real VT525 terminal, we have a
good idea of how this sequence is supposed to work, and I'm fairly
confident that our implementation is reasonably compatible.

The only significant difference I'm aware of is that we support multiple
notes in a sequence. That was a feature that was documented in the
VT520/VT525 manual, but didn't appear to be supported on the actual
device.

[lhecker]

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 PlayNote?

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.

[j4james]

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):

• ControlCore::_terminalPlayMidiNote
• Terminal::PlayMidiNote
• AdaptDispatch::PlaySounds
• OutputStateMachineEngine::ActionCsiDispatch
• StateMachine::_ActionCsiDispatch
• StateMachine::ProcessCharacter
• StateMachine::ProcessString
• ControlCore::_connectionOutputHandler

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.

[lhecker]

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.
I thought that this would be equivalent to using an exception. After all, we don't have a ShutdownException for other sequences either. 🤔

[lhecker]

Ah I see... The problem is this bit right?

but also signals the thread that it needs to exit as soon as possible.

I'll have to reread the PR again, because I haven't entirely understood yet why this is necessary. The "unlocked interval" is ControlCore::_terminalPlayMidiNote right? But that one only plays a single note, so the early return as suggested above should completely very quickly as well. I suppose I misunderstood something.

The reason I'm vary of using an exception here is because a lot of our code isn't exception safe (including ControlCore::_terminalPlayMidiNote and MidiAudio itself) and so it scares me a bit personally.

[j4james]

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.

[lhecker]

I'm not sure I was able to express my suggestion well, so just to be sure:
What I'm suggesting would be (as far as I can see) entirely equivalent to your current approach. After Shutdown has been called, all future calls to PlayNote return early and the remaining buffer finishes playing immediately. So it shouldn't block the UI more than it does now.

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 CATCH_LOG along this call chain in a few years, or maybe a COM call in-between, would anyone remember to test this particular VT sequence's behavior during shutdown?
The boolean flag has the benefit that the "behavior" is contained entirely within this class within a single file. This makes it "robust" in my view.

[j4james]

What I'm suggesting would be (as far as I can see) entirely equivalent to your current approach. After Shutdown has been called, all future calls to PlayNote return early and the remaining buffer finishes playing immediately.

But that's not the way it currently works. When Shutdown is called, the exception it triggers in the Unlock is like a direct jump to ControlCore::_connectionOutputHandler. Once that returns there is zero further processing - there are no future calls to PlayNote or anything else. If there were, that would almost certainly kill the app, because everything would already have been deleted.

Shutdown is called from the ControlCore destructor, which also goes on to destroy the MidiAudio class. The only thing keeping it alive at that point is the lock in the destructor, and it'll be able to acquire that as soon as the the Unlock method throws. From then on there will be no more MidiAudio, no more ControlCore, no more Terminal, and no more TermControl.

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).

[DHowett]

This makes sense for TermControl. Who in conhost is going to handle this exception? Or are we going to bubble it up and out as a failure? I know we have a lot of failure states for exceptions coming out of lower tiers in conhost already...

I see that MIDI is shut down during ConsoleCloseProcessState; what happens when there are multiple applications connected to one console session and one of them exits (perhaps while a note is playing?) What if it isn't the one playing the note? Are we "deathly serial" here and will only process the exit after we shut down MIDI?

Will another process in the same session stand a chance of Initialize-ing MIDI again afterwards?

[j4james]

In conhost it's caught in the WriteConsoleWImplHelper and returns as an E_ABORT (the ShutdownException derives from wil::ResultException).

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 MidiAudio class that gets shutdown when a tab is closed is just a thing for managing the note blocking. That can be recreated as often as you like - it's just a field in theControlCore class.

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.

[DHowett]

Ah, I'm thinking about one conhost session (not in Terminal) with multiple attached processes. I misremembered the purpose of CloseConsoleProcessState, and I thought that it would fire per attached process exit. You can safely ignore that question now that I know CCPS happens per console exit.

ACK on it being caught in WriteConsoleWImplHelper. 😄

[jerch]

@j4james Oh grats for getting it all wrapped up in a working format. 👍

Offtopic: Regarding #13091 (comment) plz hang on, hope to get down to the testing on the weekend.

Edit: In case you wondered - I want to address DECPS again later on with a more general sound addon also capable for PCM transports. But for this I first have to get the base64-sixel thingy straight (still needs arm simd and general lib cleanup).

[j4james]

Offtopic: Regarding #13091 (comment) plz hang on, hope to get down to the testing on the weekend.

That will be great, thank you! I've actually also got a game I wanted to share with you at some point, which I'm hoping will be playable on your VT525, and which makes use of the DECPS sequence for sound effects. You can see a demo video of it here: #12631 (comment)

It works fairly well on both the VT100 and VT240 (tested with MAME), but you obviously need a VT525 for the full color support and sound effects. I still need to tweak the sounds to play one note at a time, though, since I was originally assuming multi-note support in DECPS.

[jerch]

... You can see a demo video of it here: ... It works fairly well on both the VT100 ...

Wait wot? How do you draw those pacman symbols on a VT100? 🤯

[j4james]

Wait wot? How do you draw those pacman symbols on a VT100? 🤯

Yeah, the VT100 can't do the graphics - it falls back to a basic ASCII-art rendition with some of the line-drawing symbols from the special graphics character set. I just meant that it was playable in terms of frame rate and responsiveness, so I figured a real hardware terminal should be able to handle it just as well.

[carlos-zamora]

Thanks! This all seems pretty straightforward to me. The main question I have is if there's any way to keep the noexcepts, but it seems like the exception is the way to go.

[carlos-zamora]

I'm guessing we can remove the Fuzzing config here

[DHowett]

This is a dependency of host. It must be built for fuzzing because host must be built for fuzzing.

[j4james]

OK. I wasn't sure what exactly was needed here - I just accepted what Visual Studio generated automatically. I'll remove the fuzzing if that's not necessary.

Edit: Only saw @DHowett's message after I replied. Fuzzing gets to live.

[carlos-zamora]

@lhecker or @zadjii-msft:
Would there be any value to wrapping all of these in a method try-catch to preserve the noexcept-ness?

[j4james]

I don't think so. If an exception is getting this far, it's likely because we want it to fall through to the next level, otherwise it would typically have been handled in the _SafeExecute method.

[carlos-zamora]

Does this work with AuditMode too? Just making sure before it gets committed.

[DHowett]

FWIW, if the build below is succeeding with the AuditMode|***.Build.0 lines (which mean that audit build is enabled), this is working in audit mode.

[j4james]

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.

[carlos-zamora]

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 :)

[carlos-zamora]

@msftbot merge this in 15 minutes

[ghost]

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I won't merge this pull request until after the UTC date Wed, 01 Jun 2022 17:52:55 GMT, which is in 15 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[DHowett]

Thanks for doing this! Since it adds a new lib and all, I'm gonna give it a post-merge review. Sorry for not being ahead of the ball @j4james!

Quick notes before I dive in: I'm worried about adding a new library dependency to conhost -- we're measured in the Windows Perf. Gates (WPG) on image loads, commit, disk I/O on startup, etc. and I am worried that loading dead parts of winmm might cause us some trouble there.

Should we consider delay binding the MIDI bits until they're first used? Maybe it won't help...

Note to self: I'll have to add a sources and dirs and learn what we need to do to add a new library to the conhost build. We haven't done that in literal years! 😄

[DHowett]

What happens if anything else happens while the note was playing? Are we resizing, reflowing, allowing the user to lock the console with a pending selection or open the property sheet? If so, how do each of those things impact what happens next?

[j4james]

That's a good question. In conhost it should be fine I think. When we release the lock, it's similar to what would happen if we'd just reached the end of the output buffer at that point. And then when we reacquire the lock, that's similar to receiving a new chunk of content that needs to be processed.

It gets trickier with a conpty connection though. Conpty doesn't know that it's blocked on the other side, so when you resize, I think it still sends through frame updates to redraw the page. But because those are blocked, they build up and produce a bit of a mess when they finally get through.

You can test with a sequence like this, which play 16 seconds worth of notes in a single call:

printf "\e[4;32;1;2;3;4;5;6;7;8;9;10;11;12;13;14;15;16,~"

I suspect that part of the problem is bash wanting to redraw the prompt when we resize, which we don't always handle very well at the best of times. But it just looks a little messed up - it's not like it's going to kill anything (I hope).

Also worth mentioning that the passthrough mode doesn't have this problem as far as I can see.

[DHowett]

This makes sense for TermControl. Who in conhost is going to handle this exception? Or are we going to bubble it up and out as a failure? I know we have a lot of failure states for exceptions coming out of lower tiers in conhost already...

I see that MIDI is shut down during ConsoleCloseProcessState; what happens when there are multiple applications connected to one console session and one of them exits (perhaps while a note is playing?) What if it isn't the one playing the note? Are we "deathly serial" here and will only process the exit after we shut down MIDI?

Will another process in the same session stand a chance of Initialize-ing MIDI again afterwards?

[DHowett]

This is awesome, as usual. Thank you 😄

[j4james]

Should we consider delay binding the MIDI bits until they're first used?

That seems reasonable. I know there's already a bit of a delay the first time you play something, so I don't think it would matter too much if this made it slightly worse. Is this just a linker flag we need to throw in somewhere?

[PetterS]

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?

[j4james]

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 Lock method manually before the destructor is reached - that's just a bit messier.

[KalleOlaviNiemitalo]

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.

[j4james]

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.

[PetterS]

Alright, thanks. Perhaps it is OK as long as the mutex is also unlocked here.

[ghost]

🎉Windows Terminal Preview v1.15.186 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download