Skip to content
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.

Sign up for GitHub

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

AtlasEngine: Fix various bugs found in testing #13906

Merged
3 commits merged into from
Sep 2, 2022
Merged

AtlasEngine: Fix various bugs found in testing #13906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98c80e2
AtlasEngine: Fix various issues found in testing
lhecker Sep 1, 2022
de8ab54
Merge branch 'main' into dev/lhecker/atlas-engine-fixup
lhecker Sep 2, 2022
700b229
Address feedback
lhecker Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 7 additions & 22 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

// Tell the DX Engine to notify us when the swap chain changes.
// We do this after we initially set the swapchain so as to avoid unnecessary callbacks (and locking problems)
_renderEngine->SetCallback(std::bind(&ControlCore::_renderEngineSwapChainChanged, this));
_renderEngine->SetCallback([this](auto handle) { _renderEngineSwapChainChanged(handle); });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason std::bind wouldn't work and I didn't want to figure it out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES


_renderEngine->SetRetroTerminalEffect(_settings->RetroTerminalEffect());
_renderEngine->SetPixelShaderPath(_settings->PixelShaderPath());
Expand Down Expand Up @@ -596,24 +596,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void ControlCore::ToggleShaderEffects()
{
const auto path = _settings->PixelShaderPath();
auto lock = _terminal->LockForWriting();
// Originally, this action could be used to enable the retro effects
// even when they're set to `false` in the settings. If the user didn't
// specify a custom pixel shader, manually enable the legacy retro
// effect first. This will ensure that a toggle off->on will still work,
// even if they currently have retro effect off.
if (_settings->PixelShaderPath().empty() && !_renderEngine->GetRetroTerminalEffect())
if (path.empty())
{
// SetRetroTerminalEffect to true will enable the effect. In this
// case, the shader effect will already be disabled (because neither
// a pixel shader nor the retro effects were originally requested).
// So we _don't_ want to toggle it again below, because that would
// toggle it back off.
_renderEngine->SetRetroTerminalEffect(true);
_renderEngine->SetRetroTerminalEffect(!_renderEngine->GetRetroTerminalEffect());
}
else
{
_renderEngine->ToggleShaderEffects();
_renderEngine->SetPixelShaderPath(_renderEngine->GetPixelShaderPath().empty() ? std::wstring_view{ path } : std::wstring_view{});
Comment on lines +599 to +612
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. "Toggle terminal visual effects" action not working

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh thank you, this will make #9898 so easy

}
// Always redraw after toggling effects. This way even if the control
// does not have focus it will update immediately.
Expand Down Expand Up @@ -1558,25 +1554,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

uint64_t ControlCore::SwapChainHandle() const
{
// This is called by:
// * TermControl::RenderEngineSwapChainChanged, who is only registered
// after Core::Initialize() is called.
// * TermControl::_InitializeTerminal, after the call to Initialize, for
// _AttachDxgiSwapChainToXaml.
// In both cases, we'll have a _renderEngine by then.
return reinterpret_cast<uint64_t>(_renderEngine->GetSwapChainHandle());
}

void ControlCore::_rendererWarning(const HRESULT hr)
{
_RendererWarningHandlers(*this, winrt::make<RendererWarningArgs>(hr));
}

void ControlCore::_renderEngineSwapChainChanged()
void ControlCore::_renderEngineSwapChainChanged(const HANDLE handle)
{
_SwapChainChangedHandlers(*this, nullptr);
_SwapChainChangedHandlers(*this, winrt::box_value<uint64_t>(reinterpret_cast<uint64_t>(handle)));
}

void ControlCore::_rendererBackgroundColorChanged()
Expand Down
3 changes: 1 addition & 2 deletions src/cascadia/TerminalControl/ControlCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void SizeChanged(const double width, const double height);
void ScaleChanged(const double scale);
uint64_t SwapChainHandle() const;

void AdjustFontSize(int fontSizeDelta);
void ResetFontSize();
Expand Down Expand Up @@ -315,7 +314,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

#pragma region RendererCallbacks
void _rendererWarning(const HRESULT hr);
void _renderEngineSwapChainChanged();
void _renderEngineSwapChainChanged(const HANDLE handle);
void _rendererBackgroundColorChanged();
void _rendererTabColorChanged();
#pragma endregion
Expand Down
2 changes: 0 additions & 2 deletions src/cascadia/TerminalControl/ControlCore.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ namespace Microsoft.Terminal.Control
IControlAppearance UnfocusedAppearance { get; };
Boolean HasUnfocusedAppearance();

UInt64 SwapChainHandle { get; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @zadjii-msft - this will impact xproc work


Windows.Foundation.Size FontSize { get; };
String FontFaceName { get; };
UInt16 FontWeight { get; };
Expand Down
29 changes: 10 additions & 19 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,19 +731,24 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return _core.ConnectionState();
}

winrt::fire_and_forget TermControl::RenderEngineSwapChainChanged(IInspectable /*sender*/, IInspectable /*args*/)
winrt::fire_and_forget TermControl::RenderEngineSwapChainChanged(IInspectable /*sender*/, IInspectable args)
{
// This event is only registered during terminal initialization,
// so we don't need to check _initializedTerminal.
// We also don't lock for things that come back from the renderer.
auto weakThis{ get_weak() };
const auto weakThis{ get_weak() };

// Create a copy of the swap chain HANDLE in args, since we don't own that parameter.
// By the time we return from the co_await below, it might be deleted already.
winrt::handle handle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

winrt has a handle wrapper type? didn't know!

const auto processHandle = GetCurrentProcess();
const auto sourceHandle = reinterpret_cast<HANDLE>(winrt::unbox_value<uint64_t>(args));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if the core is out of proc, how will this work? It can raise the event with it's own handle value, sure, but then the TermControl is gonna have to come back around and ask the Core to dupe the handle to the window process anyways.

(see https://github.com/microsoft/terminal/pull/12938/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R837)

I think I can merge these together, just gonna ignore args when the control is attached to a content process.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Well it's up to us to define whether HANDLE ownership is transferred for that event or not right? If you define it so that the HANDLE ownership is transferred than you can DuplicateHandle on the content-process side of things.

THROW_IF_WIN32_BOOL_FALSE(DuplicateHandle(processHandle, sourceHandle, processHandle, handle.put(), 0, FALSE, DUPLICATE_SAME_ACCESS));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no issue doing this in-proc i presume? nice


co_await wil::resume_foreground(Dispatcher());

if (auto control{ weakThis.get() })
{
const auto chainHandle = reinterpret_cast<HANDLE>(control->_core.SwapChainHandle());
_AttachDxgiSwapChainToXaml(chainHandle);
_AttachDxgiSwapChainToXaml(handle.get());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Race condition in GetSwapChainHandle() due to it being called outside
    of the console lock and with single-threaded Direct2D enabled

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it okay that we let the handle de-scope later and get destroyed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we just duplicated it... don't we need to keep the duplicate copy "alive" (if we're out of proc, for example?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean yeah we can do that, but it'd be pretty foolish if ISwapChainPanelNative2 didn't duplicate the handle internally. That'd be just not be very robust...
...
.....
Ok I'm concerned now hahaha.

}
}

Expand Down Expand Up @@ -830,21 +835,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
_interactivity.Initialize();

_AttachDxgiSwapChainToXaml(reinterpret_cast<HANDLE>(_core.SwapChainHandle()));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Race condition in GetSwapChainHandle() due to it being called outside
    of the console lock and with single-threaded Direct2D enabled

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change doesn't impact launching terminal with a bunch of splits at the same time, right? wt sp;sp;sp;sp;sp

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(things that may cause device contention)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wtd "sp;sp;sp;sp;sp" worked fine in a debug build. Since I tested this with an added Sleep(3000) I can't conceive any situation myself were this would fail to work. 🤔


// Tell the DX Engine to notify us when the swap chain changes. We do
// this after we initially set the swapchain so as to avoid unnecessary
// callbacks (and locking problems)
_core.SwapChainChanged({ get_weak(), &TermControl::RenderEngineSwapChainChanged });

// !! LOAD BEARING !!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't love the removal of LOAD BEARING warning comments...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They cause problems though... EnablePainting() is not a thread synchronization primitive. Creating the resources on the main thread here and then calling EnablePainting() doesn't ensure that the render thread can actually access the most up to date representation of those classes.

// Make sure you enable painting _AFTER_ calling _AttachDxgiSwapChainToXaml
//
// If you EnablePainting first, then you almost certainly won't have any
// problems when running in Debug. However, in Release, you'll run into
// issues where the Renderer starts trying to paint before we've
// actually attached the swapchain to anything, and the DxEngine is not
// prepared to handle that.
_core.EnablePainting();

auto bufferHeight = _core.BufferHeight();
Expand Down
22 changes: 6 additions & 16 deletions src/renderer/atlas/AtlasEngine.api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,11 @@ HRESULT AtlasEngine::Enable() noexcept
return S_OK;
}

[[nodiscard]] std::wstring_view AtlasEngine::GetPixelShaderPath() noexcept
{
return _api.customPixelShaderPath;
}

[[nodiscard]] bool AtlasEngine::GetRetroTerminalEffect() const noexcept
{
return _api.useRetroTerminalEffect;
Expand All @@ -301,17 +306,6 @@ HRESULT AtlasEngine::Enable() noexcept
return static_cast<float>(_api.dpi) / static_cast<float>(USER_DEFAULT_SCREEN_DPI);
}

[[nodiscard]] HANDLE AtlasEngine::GetSwapChainHandle()
{
if (WI_IsFlagSet(_api.invalidations, ApiInvalidations::Device))
{
_createResources();
WI_ClearFlag(_api.invalidations, ApiInvalidations::Device);
}

return _api.swapChainHandle.get();
}

[[nodiscard]] Microsoft::Console::Types::Viewport AtlasEngine::GetViewportInCharacters(const Types::Viewport& viewInPixels) const noexcept
{
assert(_api.fontMetrics.cellSize.x != 0);
Expand All @@ -337,7 +331,7 @@ void AtlasEngine::SetAntialiasingMode(const D2D1_TEXT_ANTIALIAS_MODE antialiasin
}
}

void AtlasEngine::SetCallback(std::function<void()> pfn) noexcept
void AtlasEngine::SetCallback(std::function<void(HANDLE)> pfn) noexcept
{
_api.swapChainChangedCallback = std::move(pfn);
}
Expand Down Expand Up @@ -428,10 +422,6 @@ void AtlasEngine::SetWarningCallback(std::function<void(HRESULT)> pfn) noexcept
return S_OK;
}

void AtlasEngine::ToggleShaderEffects() noexcept
{
}

[[nodiscard]] HRESULT AtlasEngine::UpdateFont(const FontInfoDesired& fontInfoDesired, FontInfo& fontInfo, const std::unordered_map<std::wstring_view, uint32_t>& features, const std::unordered_map<std::wstring_view, float>& axes) noexcept
{
static constexpr std::array fallbackFaceNames{ static_cast<const wchar_t*>(nullptr), L"Consolas", L"Lucida Console", L"Courier New" };
Expand Down
44 changes: 29 additions & 15 deletions src/renderer/atlas/AtlasEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,9 @@ void AtlasEngine::_createResources()
D3D_FEATURE_LEVEL_11_0,
D3D_FEATURE_LEVEL_10_1,
D3D_FEATURE_LEVEL_10_0,
D3D_FEATURE_LEVEL_9_3,
D3D_FEATURE_LEVEL_9_2,
D3D_FEATURE_LEVEL_9_1,
Comment on lines +585 to +587
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Added support for DirectX 9 hardware

};

auto hr = E_UNEXPECTED;
Expand Down Expand Up @@ -624,16 +627,6 @@ void AtlasEngine::_createResources()
_r.deviceContext = deviceContext.query<ID3D11DeviceContext1>();
}

{
wil::com_ptr<IDXGIAdapter1> dxgiAdapter;
THROW_IF_FAILED(_r.device.query<IDXGIObject>()->GetParent(__uuidof(dxgiAdapter), dxgiAdapter.put_void()));
THROW_IF_FAILED(dxgiAdapter->GetParent(__uuidof(_r.dxgiFactory), _r.dxgiFactory.put_void()));

DXGI_ADAPTER_DESC1 desc;
THROW_IF_FAILED(dxgiAdapter->GetDesc1(&desc));
_r.d2dMode = debugForceD2DMode || WI_IsAnyFlagSet(desc.Flags, DXGI_ADAPTER_FLAG_REMOTE | DXGI_ADAPTER_FLAG_SOFTWARE);
}

#ifndef NDEBUG
// D3D debug messages
if (deviceFlags & D3D11_CREATE_DEVICE_DEBUG)
Expand All @@ -646,6 +639,18 @@ void AtlasEngine::_createResources()
}
#endif // NDEBUG

const auto featureLevel = _r.device->GetFeatureLevel();

{
wil::com_ptr<IDXGIAdapter1> dxgiAdapter;
THROW_IF_FAILED(_r.device.query<IDXGIObject>()->GetParent(__uuidof(dxgiAdapter), dxgiAdapter.put_void()));
THROW_IF_FAILED(dxgiAdapter->GetParent(__uuidof(_r.dxgiFactory), _r.dxgiFactory.put_void()));

DXGI_ADAPTER_DESC1 desc;
THROW_IF_FAILED(dxgiAdapter->GetDesc1(&desc));
_r.d2dMode = debugForceD2DMode || featureLevel < D3D_FEATURE_LEVEL_10_0 || WI_IsAnyFlagSet(desc.Flags, DXGI_ADAPTER_FLAG_REMOTE | DXGI_ADAPTER_FLAG_SOFTWARE);
}

if (!_r.d2dMode)
{
// Our constant buffer will never get resized
Expand All @@ -663,7 +668,7 @@ void AtlasEngine::_createResources()
if (!_api.customPixelShaderPath.empty())
{
const char* target = nullptr;
switch (_r.device->GetFeatureLevel())
switch (featureLevel)
{
case D3D_FEATURE_LEVEL_10_0:
target = "ps_4_0";
Expand Down Expand Up @@ -791,10 +796,19 @@ void AtlasEngine::_createSwapChain()
desc.Format = DXGI_FORMAT_B8G8R8A8_UNORM;
desc.SampleDesc.Count = 1;
desc.BufferUsage = DXGI_USAGE_RENDER_TARGET_OUTPUT;
desc.BufferCount = 2;
// Sometimes up to 2 buffers are locked, for instance during screen capture or when moving the window.
// 3 buffers seems to guarantee a stable framerate at display frequency at all times.
desc.BufferCount = 3;
Comment on lines +819 to +821
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 2 swap chain buffers are less performant than 3

desc.Scaling = DXGI_SCALING_NONE;
desc.SwapEffect = _sr.isWindows10OrGreater && !_r.d2dMode ? DXGI_SWAP_EFFECT_FLIP_DISCARD : DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL;
// If our background is opaque we can enable "independent" flips by setting DXGI_SWAP_EFFECT_FLIP_DISCARD and DXGI_ALPHA_MODE_IGNORE.
// DXGI_SWAP_EFFECT_FLIP_DISCARD is a mode that was created at a time were display drivers
// lacked support for Multiplane Overlays (MPO) and were copying buffers was expensive.
// This allowed DWM to quickly draw overlays (like gamebars) on top of rendered content.
// With faster GPU memory in general and with support for MPO in particular this isn't
// really an advantage anymore. Instead DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL allows for a
// more "intelligent" composition and display updates to occur like Panel Self Refresh
// (PSR) which requires dirty rectangles (Present1 API) to work correctly.
desc.SwapEffect = DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL;
Comment on lines +823 to +830
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Flip-Discard and Present() is less energy efficient than
    Flip-Sequential and Present1()

// If our background is opaque we can enable "independent" flips by setting DXGI_ALPHA_MODE_IGNORE.
// As our swap chain won't have to compose with DWM anymore it reduces the display latency dramatically.
desc.AlphaMode = _api.backgroundOpaqueMixin ? DXGI_ALPHA_MODE_IGNORE : DXGI_ALPHA_MODE_PREMULTIPLIED;
desc.Flags = debugGeneralPerformance ? 0 : DXGI_SWAP_CHAIN_FLAG_FRAME_LATENCY_WAITABLE_OBJECT;
Expand Down Expand Up @@ -837,7 +851,7 @@ void AtlasEngine::_createSwapChain()
{
try
{
_api.swapChainChangedCallback();
_api.swapChainChangedCallback(_api.swapChainHandle.get());
}
CATCH_LOG();
}
Expand Down
12 changes: 5 additions & 7 deletions src/renderer/atlas/AtlasEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

#pragma once

#include <d2d1.h>
#include <d2d1_1.h>
#include <d3d11_1.h>
#include <dwrite_3.h>

Expand Down Expand Up @@ -60,14 +60,14 @@ namespace Microsoft::Console::Render

// DxRenderer - getter
HRESULT Enable() noexcept override;
[[nodiscard]] std::wstring_view GetPixelShaderPath() noexcept override;
[[nodiscard]] bool GetRetroTerminalEffect() const noexcept override;
[[nodiscard]] float GetScaling() const noexcept override;
[[nodiscard]] HANDLE GetSwapChainHandle() override;
[[nodiscard]] Types::Viewport GetViewportInCharacters(const Types::Viewport& viewInPixels) const noexcept override;
[[nodiscard]] Types::Viewport GetViewportInPixels(const Types::Viewport& viewInCharacters) const noexcept override;
// DxRenderer - setter
void SetAntialiasingMode(D2D1_TEXT_ANTIALIAS_MODE antialiasingMode) noexcept override;
void SetCallback(std::function<void()> pfn) noexcept override;
void SetCallback(std::function<void(HANDLE)> pfn) noexcept override;
void EnableTransparentBackground(const bool isTransparent) noexcept override;
void SetForceFullRepaintRendering(bool enable) noexcept override;
[[nodiscard]] HRESULT SetHwnd(HWND hwnd) noexcept override;
Expand All @@ -77,7 +77,6 @@ namespace Microsoft::Console::Render
void SetSoftwareRendering(bool enable) noexcept override;
void SetWarningCallback(std::function<void(HRESULT)> pfn) noexcept override;
[[nodiscard]] HRESULT SetWindowSize(til::size pixels) noexcept override;
void ToggleShaderEffects() noexcept override;
[[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& pfiFontInfoDesired, FontInfo& fiFontInfo, const std::unordered_map<std::wstring_view, uint32_t>& features, const std::unordered_map<std::wstring_view, float>& axes) noexcept override;
void UpdateHyperlinkHoveredId(uint16_t hoveredId) noexcept override;

Expand Down Expand Up @@ -1008,7 +1007,7 @@ namespace Microsoft::Console::Render
// D2D resources
wil::com_ptr<ID3D11Texture2D> atlasBuffer;
wil::com_ptr<ID3D11ShaderResourceView> atlasView;
wil::com_ptr<ID2D1RenderTarget> d2dRenderTarget;
wil::com_ptr<ID2D1DeviceContext> d2dRenderTarget;
wil::com_ptr<ID2D1SolidColorBrush> brush;
wil::com_ptr<IDWriteTextFormat> textFormats[2][2];
Buffer<DWRITE_FONT_AXIS_VALUE> textFormatAxes[2][2];
Expand Down Expand Up @@ -1038,7 +1037,6 @@ namespace Microsoft::Console::Render
CachedCursorOptions cursorOptions;
RenderInvalidations invalidations = RenderInvalidations::None;

til::rect previousDirtyRectInPx;
til::rect dirtyRect;
i16 scrollOffset = 0;
bool d2dMode = false;
Expand Down Expand Up @@ -1095,7 +1093,7 @@ namespace Microsoft::Console::Render
i16 scrollOffset = 0;

std::function<void(HRESULT)> warningCallback;
std::function<void()> swapChainChangedCallback;
std::function<void(HANDLE)> swapChainChangedCallback;
wil::unique_handle swapChainHandle;
HWND hwnd = nullptr;
u16 dpi = USER_DEFAULT_SCREEN_DPI; // changes are flagged as ApiInvalidations::Font|Size
Expand Down
Loading