Skip to content

Commit 6bb8014

Browse files
bkaradzic-microsoftbkaradzicbghgaryCopilot
authored
Reworked threading model. (#1652)
[Updated by Copilot on behalf of @bghgary] ## Context Replaces the old per-thread encoder model (`GetEncoderForThread`/`EndEncoders` + `UpdateToken` wrapping `SafeTimespanGuarantor`) with a **single frame encoder** owned by `DeviceImpl`, synchronized by a `FrameCompletionScope` RAII gate. `SafeTimespanGuarantor`, `UpdateToken`, and the `Update` classes are removed. `DeviceUpdate` remains as a no-op shim — removing it is an API break and a separate PR (#1653). ## Model `StartRenderingCurrentFrame` acquires the encoder (`bgfx::begin`) and opens the gate; `FinishRenderingCurrentFrame` waits for all `FrameCompletionScope`s to release, closes the gate, ends the encoder, and submits the frame. All consumers (NativeEngine, Canvas, NativeXr) read the shared encoder via `GetActiveEncoder()`. `SubmitCommands` and `ReadTexture` each hold a stack-scoped `FrameCompletionScope`, so the encoder can't be ended mid-work; when invoked outside a frame (e.g. an XHR callback) the scope blocks until the next frame opens. ``` Main Thread JS Thread bgfx render thread StartRenderingCurrentFrame() m_frameEncoder = bgfx::begin(true) m_frameBlocked = false; CV notify tick frameStartDispatcher -----> RAF callbacks submitCommands() AcquireFrameCompletionScope (immediate; frame open) GetEncoder() -> m_frameEncoder; draw... scope released; pendingScopes-- -> CV notify FinishRenderingCurrentFrame() wait scopes == 0; m_frameBlocked = true tick beforeRenderDispatcher bgfx::end(m_frameEncoder) Frame() -> bgfx::frame() ------------------------------> GPU submit & render tick afterRenderDispatcher ``` ## Synchronization rules 1. Single encoder, acquired before the gate opens (mutex gives memory ordering) and ended only after all scopes release. 2. `SubmitCommands`/`ReadTexture` always hold a scope — the encoder can't be ended mid-command, regardless of microtask draining or reentrancy. 3. Apps must bracket any main-thread wait for JS work with `Start`/`Finish`, or `SubmitCommands` blocks forever waiting for the gate (HeadlessScreenshotApp, StyleTransferApp, UnitTests do this). 4. `Canvas::Flush` acquires a scope when called outside a frame, then discards residual encoder state before nanovg rendering (the shared encoder carries NativeEngine state). ## Notes - `Tests.ExternalTexture.Msaa.cpp` opens the next frame before waiting on `startup()` — a workaround for the legacy async `AddToContextAsync` pattern, obsoleted once #1646's sync `CreateForJavaScript` lands. - `Tests.ExternalTexture.RestoreAfterDeviceLoss` (added on master after this branch) uses the same async pattern; the same frame-pump workaround is applied here. Obsoleted once #1646's sync `CreateForJavaScript` lands. - bgfx: added `m_encoderBeginLock` in `bgfx::frame()` to prevent a deadlock with `bgfx::destroy()`. - Follow-up: removing the `DeviceUpdate` no-op shim is an API-breaking change handled in #1653. --------- Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ff9d4db commit 6bb8014

25 files changed

Lines changed: 461 additions & 462 deletions

File tree

‎Apps/HeadlessScreenshotApp/Win32/App.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,17 @@ int main()
159159
deviceUpdate.Finish();
160160
device.FinishRenderingCurrentFrame();
161161

162+
// Reopen the gate so JS can continue running (startup may issue bgfx commands).
163+
device.StartRenderingCurrentFrame();
164+
deviceUpdate.Start();
165+
162166
// Wait for `startup` to finish.
163167
startup.get_future().wait();
164168

169+
// Close the frame opened above.
170+
deviceUpdate.Finish();
171+
device.FinishRenderingCurrentFrame();
172+
165173
struct Asset
166174
{
167175
const char* Name;

‎Apps/PrecompiledShaderTest/Source/App.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,17 @@ int RunApp(
166166
deviceUpdate.Finish();
167167
device.FinishRenderingCurrentFrame();
168168

169+
// Reopen the gate so JS can continue running (startup may issue bgfx commands).
170+
device.StartRenderingCurrentFrame();
171+
deviceUpdate.Start();
172+
169173
// Wait for `startup` to finish.
170174
startup.get_future().wait();
171175

176+
// Close the frame opened above.
177+
deviceUpdate.Finish();
178+
device.FinishRenderingCurrentFrame();
179+
172180
// Start a new frame for rendering the scene.
173181
device.StartRenderingCurrentFrame();
174182
deviceUpdate.Start();

‎Apps/StyleTransferApp/Win32/App.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,9 +362,17 @@ int APIENTRY wWinMain(_In_ HINSTANCE hInstance,
362362
g_update->Finish();
363363
g_device->FinishRenderingCurrentFrame();
364364

365+
// Reopen the gate so JS can continue running (startup may issue bgfx commands).
366+
g_device->StartRenderingCurrentFrame();
367+
g_update->Start();
368+
365369
// Wait for `startup` to finish.
366370
startup.get_future().wait();
367371

372+
// Close the frame opened above.
373+
g_update->Finish();
374+
g_device->FinishRenderingCurrentFrame();
375+
368376
// --------------------------- Rendering loop -------------------------
369377

370378
HACCEL hAccelTable = LoadAccelerators(hInstance, MAKEINTRESOURCE(IDC_PLAYGROUNDWIN32));

‎Apps/UnitTests/Source/Tests.ExternalTexture.DeviceLoss.cpp‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,12 +141,16 @@ TEST(ExternalTexture, RestoreAfterDeviceLoss)
141141
addToContextCalled.get_future().wait();
142142
update.Finish();
143143
device.FinishRenderingCurrentFrame();
144-
startupDone.get_future().get();
145144

146-
// --- Phase 1: render red into texture 1, readback ---
145+
// Open the next frame BEFORE waiting on startup(): under the reworked single-frame-encoder
146+
// model the AddToContextAsync .then() runs startup() on the JS thread, whose SubmitCommands
147+
// blocks until a frame is in progress. The same frame is reused for Phase 1's renderFrame.
147148
device.StartRenderingCurrentFrame();
148149
update.Start();
149150

151+
startupDone.get_future().get();
152+
153+
// --- Phase 1: render red into texture 1, readback (reuses the open frame) ---
150154
std::promise<void> render1Done;
151155
loader.Dispatch([&render1Done](Napi::Env env) {
152156
auto jsPromise = env.Global().Get("renderFrame").As<Napi::Function>().Call({}).As<Napi::Promise>();
@@ -221,12 +225,15 @@ TEST(ExternalTexture, RestoreAfterDeviceLoss)
221225
addToContext2Called.get_future().wait();
222226
update.Finish();
223227
device.FinishRenderingCurrentFrame();
224-
restoreDone.get_future().get();
225228

226-
// Render blue into restored RTT.
229+
// Open the next frame before waiting on restoreTexture(): SubmitCommands needs an open frame
230+
// under the reworked model. The same frame is reused for the blue render below.
227231
device.StartRenderingCurrentFrame();
228232
update.Start();
229233

234+
restoreDone.get_future().get();
235+
236+
// Render blue into restored RTT (reuses the open frame).
230237
std::promise<void> render2Done;
231238
loader.Dispatch([&render2Done](Napi::Env env) {
232239
auto jsPromise = env.Global().Get("renderFrame").As<Napi::Function>().Call({}).As<Napi::Promise>();

‎Apps/UnitTests/Source/Tests.ExternalTexture.Msaa.cpp‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,17 @@ namespace
8989
addToContextCalled.get_future().wait();
9090
update.Finish();
9191
device.FinishRenderingCurrentFrame();
92-
startupDone.get_future().get();
9392

94-
// New frame: drive a single renderFrame() on the JS side.
93+
// Open the next frame BEFORE waiting for startup() to complete. The AddToContextAsync .then()
94+
// callback runs on the JS thread and calls startup(), which constructs NativeEngine + Scene + RTT,
95+
// each step submitting bgfx commands. In the threading model from #1652, SubmitCommands
96+
// synchronously acquires a FrameCompletionScope and blocks until a frame is in progress, so a
97+
// frame must be open while startup() runs. The same frame is reused for renderFrame().
9598
device.StartRenderingCurrentFrame();
9699
update.Start();
97100

101+
startupDone.get_future().get();
102+
98103
std::promise<void> renderDone;
99104
loader.Dispatch([&renderDone](Napi::Env env) {
100105
auto jsPromise = env.Global().Get("renderFrame").As<Napi::Function>().Call({}).As<Napi::Promise>();

‎Apps/UnitTests/Source/Tests.JavaScript.cpp‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,13 @@ TEST(JavaScript, All)
4040

4141
Babylon::Graphics::Device device{g_deviceConfig};
4242

43+
// Start rendering a frame to unblock the JavaScript from queuing graphics
44+
// commands. The frame is held open through script load and the test pump
45+
// (which only ticks bgfx via Finish; Start) so the JS thread can submit
46+
// at any time without racing the gate. A final Finish closes it after
47+
// runtime teardown.
48+
device.StartRenderingCurrentFrame();
49+
4350
std::optional<Babylon::Polyfills::Canvas> nativeCanvas;
4451

4552
Babylon::AppRuntime::Options options{};
@@ -87,9 +94,22 @@ TEST(JavaScript, All)
8794
loader.LoadScript("app:///Assets/babylonjs.materials.js");
8895
loader.LoadScript("app:///Assets/tests.javaScript.all.js");
8996

90-
device.StartRenderingCurrentFrame();
91-
device.FinishRenderingCurrentFrame();
97+
// Pump frames while JS tests run — tests use RAF internally and
98+
// SubmitCommands requires an active frame. The frame was opened
99+
// immediately after device creation; the loop just ticks bgfx
100+
// (Finish; Start) once per iteration so commands can advance.
101+
auto exitCodeFuture = exitCodePromise.get_future();
102+
while (exitCodeFuture.wait_for(std::chrono::milliseconds(16)) != std::future_status::ready)
103+
{
104+
device.FinishRenderingCurrentFrame();
105+
device.StartRenderingCurrentFrame();
106+
}
92107

93-
auto exitCode{exitCodePromise.get_future().get()};
108+
auto exitCode = exitCodeFuture.get();
94109
EXPECT_EQ(exitCode, 0);
110+
111+
// Runtime destructor joins the JS thread; must happen before Finish.
112+
nativeCanvas.reset();
113+
114+
device.FinishRenderingCurrentFrame();
95115
}

‎Core/Graphics/CMakeLists.txt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ set(SOURCES
66
"InternalInclude/Babylon/Graphics/continuation_scheduler.h"
77
"InternalInclude/Babylon/Graphics/FrameBuffer.h"
88
"InternalInclude/Babylon/Graphics/DeviceContext.h"
9-
"InternalInclude/Babylon/Graphics/SafeTimespanGuarantor.h"
109
"InternalInclude/Babylon/Graphics/Texture.h"
1110
"Source/BgfxCallback.cpp"
1211
"Source/FrameBuffer.cpp"
@@ -16,7 +15,6 @@ set(SOURCES
1615
"Source/DeviceImpl.h"
1716
"Source/DeviceImpl_${BABYLON_NATIVE_PLATFORM}.${BABYLON_NATIVE_PLATFORM_IMPL_EXT}"
1817
"Source/DeviceImpl_${GRAPHICS_API}.${BABYLON_NATIVE_PLATFORM_IMPL_EXT}"
19-
"Source/SafeTimespanGuarantor.cpp"
2018
"Source/Texture.cpp")
2119

2220
add_library(Graphics ${SOURCES})

‎Core/Graphics/Include/Shared/Babylon/Graphics/Device.h‎

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -58,45 +58,23 @@ namespace Babylon::Graphics
5858
DepthStencilFormat BackBufferDepthStencilFormat{DepthStencilFormat::Depth24Stencil8};
5959
};
6060

61-
class Device;
61+
class DeviceImpl;
6262

63+
// Deprecated: DeviceUpdate is a no-op compatibility shim. Frame synchronization
64+
// is now handled by FrameCompletionScope inside StartRenderingCurrentFrame/
65+
// FinishRenderingCurrentFrame. This class will be removed in a future PR.
6366
class DeviceUpdate
6467
{
6568
public:
66-
void Start()
67-
{
68-
m_start();
69-
}
69+
void Start() {}
70+
void Finish() {}
7071

7172
void RequestFinish(std::function<void()> onFinishCallback)
7273
{
73-
m_requestFinish(std::move(onFinishCallback));
74-
}
75-
76-
void Finish()
77-
{
78-
std::promise<void> promise{};
79-
auto future = promise.get_future();
80-
RequestFinish([&promise] { promise.set_value(); });
81-
future.wait();
82-
}
83-
84-
private:
85-
friend class Device;
86-
87-
template<typename StartCallableT, typename RequestEndCallableT>
88-
DeviceUpdate(StartCallableT&& start, RequestEndCallableT&& requestEnd)
89-
: m_start{std::forward<StartCallableT>(start)}
90-
, m_requestFinish{std::forward<RequestEndCallableT>(requestEnd)}
91-
{
74+
onFinishCallback();
9275
}
93-
94-
std::function<void()> m_start{};
95-
std::function<void(std::function<void()>)> m_requestFinish{};
9676
};
9777

98-
class DeviceImpl;
99-
10078
class Device
10179
{
10280
public:
@@ -136,7 +114,7 @@ namespace Babylon::Graphics
136114
void EnableRendering();
137115
void DisableRendering();
138116

139-
DeviceUpdate GetUpdate(const char* updateName);
117+
DeviceUpdate GetUpdate(const char* /*updateName*/) { return {}; }
140118

141119
void StartRenderingCurrentFrame();
142120
void FinishRenderingCurrentFrame();

‎Core/Graphics/InternalInclude/Babylon/Graphics/DeviceContext.h‎

Lines changed: 49 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
#include "BgfxCallback.h"
44
#include <bx/allocator.h>
55
#include "continuation_scheduler.h"
6-
#include "SafeTimespanGuarantor.h"
6+
7+
#include <arcana/threading/task.h>
78

89
#include <napi/env.h>
910

@@ -15,7 +16,6 @@
1516

1617
namespace Babylon::Graphics
1718
{
18-
class Update;
1919
class DeviceContext;
2020
class DeviceImpl;
2121

@@ -29,53 +29,41 @@ namespace Babylon::Graphics
2929
bgfx::TextureFormat::Enum Format{};
3030
};
3131

32-
class UpdateToken final
32+
// FrameCompletionScope is an RAII guard that prevents the render thread from
33+
// closing a bgfx frame while JS-thread work is still in flight. While any
34+
// scope is alive, FinishRenderingCurrentFrame() blocks before bgfx::frame()
35+
// — so all encoder commands recorded while a scope was held land in the
36+
// same bgfx frame, never split across two.
37+
//
38+
// Acquisition blocks if the gate is closed (m_frameBlocked == true), so a
39+
// JS thread that picks up work between frames waits for the next Start
40+
// before proceeding.
41+
//
42+
// Two scoping patterns are used:
43+
// 1. JS-frame scoped: capture the scope into an m_runtime.Dispatch lambda
44+
// so it survives until the JS-thread queue services the next
45+
// continuation. Use this when subsequent work in the same JS task may
46+
// also touch bgfx (chained submitCommands, RAF callbacks, scene
47+
// cleanup, etc.). NativeEngine::SubmitCommands and
48+
// ScheduleRequestAnimationFrameCallbacks both do this.
49+
// 2. Block scoped: hold the scope on the stack across a single self-
50+
// contained bgfx phase. Used for one-shot operations like
51+
// Canvas::Flush() called outside an active frame, and
52+
// ReadTextureAsync.
53+
class FrameCompletionScope final
3354
{
3455
public:
35-
UpdateToken(const UpdateToken& other) = delete;
36-
UpdateToken& operator=(const UpdateToken& other) = delete;
37-
38-
UpdateToken(UpdateToken&&) noexcept = default;
39-
40-
// The move assignment of `SafeTimespanGuarantor::SafetyGuarantee` is marked as delete.
41-
// See https://github.com/Microsoft/GSL/issues/705.
42-
//UpdateToken& operator=(UpdateToken&& other) = delete;
56+
FrameCompletionScope(const FrameCompletionScope&) = delete;
57+
FrameCompletionScope& operator=(const FrameCompletionScope&) = delete;
58+
FrameCompletionScope& operator=(FrameCompletionScope&&) = delete;
4359

44-
bgfx::Encoder* GetEncoder();
45-
46-
private:
47-
friend class Update;
48-
49-
UpdateToken(DeviceContext&, SafeTimespanGuarantor&);
50-
51-
DeviceContext& m_context;
52-
SafeTimespanGuarantor::SafetyGuarantee m_guarantee;
53-
};
54-
55-
class Update
56-
{
57-
public:
58-
continuation_scheduler<>& Scheduler()
59-
{
60-
return m_safeTimespanGuarantor.OpenScheduler();
61-
}
62-
63-
UpdateToken GetUpdateToken()
64-
{
65-
return {m_context, m_safeTimespanGuarantor};
66-
}
60+
FrameCompletionScope(FrameCompletionScope&&) noexcept;
61+
~FrameCompletionScope();
6762

6863
private:
6964
friend class DeviceContext;
70-
71-
Update(SafeTimespanGuarantor& safeTimespanGuarantor, DeviceContext& context)
72-
: m_safeTimespanGuarantor{safeTimespanGuarantor}
73-
, m_context{context}
74-
{
75-
}
76-
77-
SafeTimespanGuarantor& m_safeTimespanGuarantor;
78-
DeviceContext& m_context;
65+
FrameCompletionScope(DeviceImpl&);
66+
DeviceImpl* m_impl;
7967
};
8068

8169
class DeviceContext
@@ -93,7 +81,23 @@ namespace Babylon::Graphics
9381
continuation_scheduler<>& BeforeRenderScheduler();
9482
continuation_scheduler<>& AfterRenderScheduler();
9583

96-
Update GetUpdate(const char* updateName);
84+
// Scheduler that fires when StartRenderingCurrentFrame ticks the frame start dispatcher.
85+
// Use this to schedule work (e.g., requestAnimationFrame callbacks) that should run each frame.
86+
continuation_scheduler<>& FrameStartScheduler();
87+
88+
// Acquire a scope that prevents FinishRenderingCurrentFrame from
89+
// completing until the scope is destroyed. JS-thread callers that
90+
// need coverage across the whole current JS task should capture the
91+
// scope into an m_runtime.Dispatch lambda; callers needing only a
92+
// single phase can hold it stack-scoped. See the class comment on
93+
// FrameCompletionScope above for the two patterns.
94+
FrameCompletionScope AcquireFrameCompletionScope();
95+
96+
// Active encoder for the current frame. Managed by DeviceImpl in
97+
// StartRenderingCurrentFrame/FinishRenderingCurrentFrame.
98+
// Used by NativeEngine, Canvas, and NativeXr.
99+
void SetActiveEncoder(bgfx::Encoder* encoder);
100+
bgfx::Encoder* GetActiveEncoder();
97101

98102
void RequestScreenShot(std::function<void(std::vector<uint8_t>)> callback);
99103
void RequestCaptureNextFrame();
@@ -114,7 +118,7 @@ namespace Babylon::Graphics
114118
using CaptureCallbackTicketT = arcana::ticketed_collection<std::function<void(const BgfxCallback::CaptureData&)>>::ticket;
115119
CaptureCallbackTicketT AddCaptureCallback(std::function<void(const BgfxCallback::CaptureData&)> callback);
116120

117-
bgfx::ViewId AcquireNewViewId(bgfx::Encoder&);
121+
bgfx::ViewId AcquireNewViewId();
118122
bgfx::ViewId PeekNextViewId() const;
119123

120124
// TODO: find a different way to get the texture info for frame capture
@@ -124,8 +128,6 @@ namespace Babylon::Graphics
124128
static bx::AllocatorI& GetDefaultAllocator() { return m_allocator; }
125129

126130
private:
127-
friend UpdateToken;
128-
129131
DeviceImpl& m_graphicsImpl;
130132

131133
std::unordered_map<uint16_t, TextureInfo> m_textureHandleToInfo{};

‎Core/Graphics/InternalInclude/Babylon/Graphics/FrameBuffer.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ namespace Babylon::Graphics
3333
uint16_t Height() const;
3434
bool DefaultBackBuffer() const;
3535

36-
void Bind(bgfx::Encoder& encoder);
37-
void Unbind(bgfx::Encoder& encoder);
36+
void Bind();
37+
void Unbind();
3838

3939
void Clear(bgfx::Encoder& encoder, uint16_t flags, uint32_t rgba, float depth, uint8_t stencil);
40-
void SetViewPort(bgfx::Encoder& encoder, float x, float y, float width, float height);
41-
void SetScissor(bgfx::Encoder& encoder, float x, float y, float width, float height);
40+
void SetViewPort(float x, float y, float width, float height);
41+
void SetScissor(float x, float y, float width, float height);
4242
void Submit(bgfx::Encoder& encoder, bgfx::ProgramHandle programHandle, uint8_t flags);
4343
void SetStencil(bgfx::Encoder& encoder, uint32_t stencilState);
4444
void Blit(bgfx::Encoder& encoder, bgfx::TextureHandle dst, uint16_t dstX, uint16_t dstY, bgfx::TextureHandle src, uint16_t srcX = 0, uint16_t srcY = 0, uint16_t width = UINT16_MAX, uint16_t height = UINT16_MAX);
@@ -48,7 +48,7 @@ namespace Babylon::Graphics
4848

4949
private:
5050
Rect GetBgfxScissor(float x, float y, float width, float height) const;
51-
void SetBgfxViewPortAndScissor(bgfx::Encoder& encoder, const Rect& viewPort, const Rect& scissor);
51+
void SetBgfxViewPortAndScissor(const Rect& viewPort, const Rect& scissor);
5252

5353
DeviceContext& m_deviceContext;
5454
const uintptr_t m_deviceID{};

0 commit comments

Comments
 (0)