Repository: microsoft/terminal (language: cpp)

Issue:
Implement synchronized update control sequences
<!-- 
🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨

I ACKNOWLEDGE THE FOLLOWING BEFORE PROCEEDING:
1. If I delete this entire template and go my own path, the core team may close my issue without further explanation or engagement.
2. If I list multiple bugs/concerns in this one issue, the core team may close my issue without further explanation or engagement.
3. If I write an issue that has many duplicates, the core team may close my issue without further explanation or engagement (and without necessarily spending time to find the exact duplicate ID number).
4. If I leave the title incomplete when filing the issue, the core team may close my issue without further explanation or engagement.
5. If I file something completely blank in the body, the core team may close my issue without further explanation or engagement.

All good? Then proceed!
-->

# Description of the new feature/enhancement

https://gitlab.com/gnachman/iterm2/-/wikis/synchronized-updates-spec

The goal of synchronized updates is to avoid showing a half-drawn screen, such as while paging through a document in an editor.

A new control sequence is proposed that indicates the beginning and end of a update. No new content should be rendered by the terminal emulator until the update ends, at which point any changes made during the update should be applied atomically.

The purpose of this sequence is to provide a hint to the terminal emulator about how to draw atomically. If it turns out to be too difficult to do under some particular input, the hint can be safely ignored

<!-- 
A clear and concise description of what the problem is that the new feature would solve.
Describe why and how a user would use this new functionality (if applicable).
-->


```[tasklist]
### Tasks
```



Discussion / hints:
Yea okay, this seems well thought out. I like that there are contingincies included for "what happens when the client app disconnects before sending an \<end\>". Plus, I think we did add support for DCS sequences relatively recently, so this shouldn't be too bad. 

Part of the trick here is gonna be conpty, and the way we use that to render the _effects_ of output on the console buffer. I'd guess that this would have to be implemented in `conhost`, and then at the end of the synchronized update, conhost would render _once_ to the terminal. That makes sense to me. 
Note that this idea was discussed in terminal-wg a while back, and I think it was generally agreed that a DCS sequence was a bad choice for this. One of the benefits of this functionality is that it shouldn't require feature detection - if a terminal doesn't support it, then you just don't get the optimisation. However, that's not the case if you're using a DCS sequence, because there are a number of terminals that will output garbage if you send them an unrecognised DCS. So now an app can't really use this functionality unless they first determine whether it's supported somehow.

As is typical for terminal-wg, there was no official conclusion to the discussion, but the general consensus seemed to have been that a DECSET private mode sequence would be preferable, and the mode number that was settled on was 2026. I know at least one terminal has implemented synchronized updates via mode 2026, and at least one application is using that mode.

Also I know @gnachman has said "If I had it to do over again, I'd push for DECSET/DECRST", but he hasn't actually agreed to supporting the proposed mode in iTerm2 as far as I'm aware. If we're going to implement this, I'd much rather we used a DECSET mode than DCS, but I'd also like to have seen some definite commitment to that approach from people like iTerm2.
> However, that's not the case if you're using a DCS sequence, because there are a number of terminals that will output garbage if you send them an unrecognised DCS

Guh, I hate that. Maybe we come up with an agreement between us and iTerm2 to move forward onto a DECSET/DECRST implementation. We'd obviously have to still support the DCS version of the sequence, but maybe we can help push the ecosystem forward a bit here. Here's what I'm thinking:

* We implement support for the DCS version of this sequence.
* _At the same time_, as we add support for the DCS version, we add support for the DECSET/DECRST version. 
* **At the same time**, iTerm2 adds support for the DECSET/DECRST version. 

At this point, we'd update any existing documentation to suggest that app developers use the new version of the sequence, and the old one is deprecated. Terminal emulators that haven't been updated with support for the new sequence will silently ignore it. Apps that aren't updated to use the new sequence will still continue to work (and will _start_ working on the console/Terminal).

This would obviously require buyoff from @gnachman, but this seems like a reasonable path, and one that would work cross-platform. Thoughts?

##### notes: terminal-wg [pr](https://gitlab.freedesktop.org/terminal-wg/specifications/-/merge_requests/2)
I am fine with this. I believe Kitty has also implemented support for this control sequence, so check with @kovidgoyal too.
You can go ahead and use DECSET/DECRESET if it floats your boat. Since you are also implementing support for the DCS version, I dont really care. I will note that the only terminals that dont support DCS sequences are Apple's and the Linux kernel console. Using DECSET/DECRESET means that no future modifications can be made to the protocol to implement additional features. Which will mean in the long term that DECSET/DECRESET will be the deprecated ones, but hey, no harm in having them, just another redundant code path. Just be careful when choosing numbers for the codes to not conflict with existing uses, which was one of the motivations for choosing DCS in the first place. 
> Just be careful when choosing numbers for the codes to not conflict with existing uses,

I like to compare that feature a little bit to double buffering in graphics APIs, such as OpenGL (first released in 1992), wich is also there to avoid tearing effects. That standard had never changed. So i am pretty confident it won't need any feature extension here, too.


That's not a valid comparison. OpenGL's double bufferring buffers a single thing, a pixel buffer, and over a fast local link. In the terminal case you are bufferring terminal state beyond just what is drawn on the screen and over latency variable, lossy networks. 
> That's not a valid comparison. OpenGL's double bufferring buffers a single thing, a pixel buffer, and over a fast local link. In the terminal case you are bufferring terminal state beyond just what is drawn on the screen and over latency variable, lossy networks.

@kovidgoyal i do not want to fight over little differences when I explicitly did not say equal but like. That would be way to off-topic. Feel free to ignore my opinion. ;-)
@zadjii-msft good morning. I'd like to ask if there there had been actually any brainstorming progress on the msft side of the fence? I stilll do also think that DECSM/DECRM are the right choice, and I came back to see if here was any progress until then. :)
I actually hacked together a basic implementation of this a while back (just the mode 2026 approach) which I've been using in my local fork. We didn't have the ability to do DCS at the time, and I was also waiting to see what iTerm2 would do. But I suspect now that gnachman has changed his mind about supporting mode 2026 (it would have taken all of 3 lines of code if he was going to do it). So if it were up to me, I'd say we should just do the mode and ignore the DCS approach. Supporting both will push apps to use DCS, which is exactly what we want to avoid.

Even if we don't care about all the other terminals that will be negatively effected by the use of DCS (which I can assure you is more than two), the Windows console itself didn't support DCS until very recently. So there are still likely to be a significant number of users on an older version of Windows that'll break if sent a DCS sequence, and upgrading is not always an option (I know, because several of my computers are on older versions that aren't being offered upgrades).

If it turns out there are actually apps using the DCS sequence that can't be persuaded to add support for the mode, then we can always reconsider adding a DCS alias later (but preferably when the #6328 fix is more widely deployed).
> I'd say we should just do the mode and ignore the DCS approach. Supporting both will push apps to use DCS, which is exactly what we want to avoid.

I am all in, and absolutely agree with you on that decision. 

For reference, I've put together the spec as well with the (more interestingly maybe) list of supporting software for the "synchronized rendering" feature [here](https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036). @j4james I can't await adding you (your branch / PR) once ready to it, too. Looking forward to it. :)
I'm happy to do DECSET 2026. I've been busy, but will do it now.
> I'm happy to do DECSET 2026. I've been busy, but will do it now.

I am really happy to hear from you. Looking forward to it.
Done in commit f7efeb4bbaa5ba2796f94c3bab32e5c64ee9c743
Any updates or progress on this?

Fix patch:
diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp
index 90f6d5936f5..5824bcb2f2a 100644
--- a/src/renderer/base/renderer.cpp
+++ b/src/renderer/base/renderer.cpp
@@ -4,8 +4,6 @@
 #include "precomp.h"
 #include "renderer.hpp"
 
-#pragma hdrstop
-
 using namespace Microsoft::Console::Render;
 using namespace Microsoft::Console::Types;
 
@@ -90,6 +88,11 @@ IRenderData* Renderer::GetRenderData() const noexcept
             _pData->UnlockConsole();
         });
 
+        if (_isSynchronizingOutput)
+        {
+            _synchronizeWithOutput();
+        }
+
         // Last chance check if anything scrolled without an explicit invalidate notification since the last frame.
         _CheckViewportAndScroll();
 
@@ -183,6 +186,71 @@ void Renderer::NotifyPaintFrame() noexcept
     _thread.NotifyPaint();
 }
 
+// NOTE: You must be holding the console lock when calling this function.
+void Renderer::SynchronizedOutputBegin() noexcept
+{
+    // Kick the render thread into calling `_synchronizeWithOutput()`.
+    _isSynchronizingOutput = true;
+}
+
+// NOTE: You must be holding the console lock when calling this function.
+void Renderer::SynchronizedOutputEnd() noexcept
+{
+    // Unblock `_synchronizeWithOutput()` from the `WaitOnAddress` call.
+    _isSynchronizingOutput = false;
+    WakeByAddressSingle(&_isSynchronizingOutput);
+
+    // It's crucial to give the render thread at least a chance to gain the lock.
+    // Otherwise, a VT application could continuously spam DECSET 2026 (Synchronized Output) and
+    // essentially drop our renderer to 10 FPS, because `_isSynchronizingOutput` is always true.
+    //
+    // Obviously calling LockConsole/UnlockConsole here is an awful, ugly hack,
+    // since there's no guarantee that this is the same lock as the one the VT parser uses.
+    // But the alternative is Denial-Of-Service of the render thread.
+    //
+    // Note that this causes raw throughput of DECSET 2026 to be comparatively low, but that's fine.
+    // Apps that use DECSET 2026 don't produce that sequence continuously, but rather at a fixed rate.
+    _pData->UnlockConsole();
+    _pData->LockConsole();
+}
+
+void Renderer::_synchronizeWithOutput() noexcept
+{
+    constexpr DWORD timeout = 100;
+
+    UINT64 start = 0;
+    DWORD elapsed = 0;
+    bool wrong = false;
+
+    QueryUnbiasedInterruptTime(&start);
+
+    // Wait for `_isSynchronizingOutput` to be set to false or for a timeout to occur.
+    while (true)
+    {
+        // We can't call a blocking function while holding the console lock, so release it temporarily.
+        _pData->UnlockConsole();
+        const auto ok = WaitOnAddress(&_isSynchronizingOutput, &wrong, sizeof(_isSynchronizingOutput), timeout - elapsed);
+        _pData->LockConsole();
+
+        if (!ok || !_isSynchronizingOutput)
+        {
+            break;
+        }
+
+        UINT64 now;
+        QueryUnbiasedInterruptTime(&now);
+        elapsed = static_cast<DWORD>((now - start) / 10000);
+        if (elapsed >= timeout)
+        {
+            break;
+        }
+    }
+
+    // If a timeout occurred, `_isSynchronizingOutput` may still be true.
+    // Set it to false now to skip calling `_synchronizeWithOutput()` on the next frame.
+    _isSynchronizingOutput = false;
+}
+
 // Routine Description:
 // - Called when the system has requested we redraw a portion of the console.
 // Arguments:
diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp
index d5f6be56d18..c8c30457604 100644
--- a/src/renderer/base/renderer.hpp
+++ b/src/renderer/base/renderer.hpp
@@ -35,6 +35,8 @@ namespace Microsoft::Console::Render
         [[nodiscard]] HRESULT PaintFrame();
 
         void NotifyPaintFrame() noexcept;
+        void SynchronizedOutputBegin() noexcept;
+        void SynchronizedOutputEnd() noexcept;
         void TriggerSystemRedraw(const til::rect* const prcDirtyClient);
         void TriggerRedraw(const Microsoft::Console::Types::Viewport& region);
         void TriggerRedraw(const til::point* const pcoord);
@@ -91,6 +93,7 @@ namespace Microsoft::Console::Render
 
         [[nodiscard]] HRESULT _PaintFrame() noexcept;
         [[nodiscard]] HRESULT _PaintFrameForEngine(_In_ IRenderEngine* const pEngine) noexcept;
+        void _synchronizeWithOutput() noexcept;
         bool _CheckViewportAndScroll();
         [[nodiscard]] HRESULT _PaintBackground(_In_ IRenderEngine* const pEngine);
         void _PaintBufferOutput(_In_ IRenderEngine* const pEngine);
@@ -124,6 +127,7 @@ namespace Microsoft::Console::Render
         std::function<void()> _pfnBackgroundColorChanged;
         std::function<void()> _pfnFrameColorChanged;
         std::function<void()> _pfnRendererEnteredErrorState;
+        bool _isSynchronizingOutput = false;
         bool _forceUpdateViewport = false;
 
         til::point_span _lastSelectionPaintSpan{};
diff --git a/src/renderer/base/thread.cpp b/src/renderer/base/thread.cpp
index 467bef73585..39b7ec60783 100644
--- a/src/renderer/base/thread.cpp
+++ b/src/renderer/base/thread.cpp
@@ -34,7 +34,7 @@ DWORD WINAPI RenderThread::_ThreadProc()
 
         // Between waiting on _hEvent and calling PaintFrame() there should be a minimal delay,
         // so that a key press progresses to a drawing operation as quickly as possible.
-        // As such, we wait for the renderer to complete _before_ waiting on _hEvent.
+        // As such, we wait for the renderer to complete _before_ waiting on `_redraw`.
         renderer->WaitUntilCanRender();
 
         _redraw.wait();
@@ -78,6 +78,8 @@ void RenderThread::EnablePainting() noexcept
     }
 }
 
+// This function is meant to only be called by `Renderer`. You should use `TriggerTeardown()` instead,
+// even if you plan to call `EnablePainting()` later, because that ensures proper synchronization.
 void RenderThread::DisablePainting() noexcept
 {
     _enable.ResetEvent();
diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp
index f1adb1457c8..b99dd997dde 100644
--- a/src/terminal/adapter/DispatchTypes.hpp
+++ b/src/terminal/adapter/DispatchTypes.hpp
@@ -544,6 +544,7 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes
         ALTERNATE_SCROLL = DECPrivateMode(1007),
         ASB_AlternateScreenBuffer = DECPrivateMode(1049),
         XTERM_BracketedPasteMode = DECPrivateMode(2004),
+        SO_SynchronizedOutput = DECPrivateMode(2026),
         GCM_GraphemeClusterMode = DECPrivateMode(2027),
         W32IM_Win32InputMode = DECPrivateMode(9001),
     };
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index 9158b962ab3..c8f4c5eb85e 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -1914,6 +1914,19 @@ void AdaptDispatch::_ModeParamsHelper(const DispatchTypes::ModeParams param, con
     case DispatchTypes::ModeParams::XTERM_BracketedPasteMode:
         _api.SetSystemMode(ITerminalApi::Mode::BracketedPaste, enable);
         break;
+    case DispatchTypes::ModeParams::SO_SynchronizedOutput:
+        if (_renderer)
+        {
+            if (enable)
+            {
+                _renderer->SynchronizedOutputBegin();
+            }
+            else
+            {
+                _renderer->SynchronizedOutputEnd();
+            }
+        }
+        break;
     case DispatchTypes::ModeParams::GCM_GraphemeClusterMode:
         break;
     case DispatchTypes::ModeParams::W32IM_Win32InputMode:


Test patch: