[alandarev]

Summary

Bound bitmap-upload resources during bulk loading instead of allocating a transfer buffer and sampler for every image. On the Linux/NVIDIA reproduction in #620, upload-related resource retention exhausted a 1024-FD limit before startup completed.

• Add three persistent, fenced RGBA8 staging slots: 4 MiB each at the unchanged 1024×1024 bitmap limit. Reuse waits for completion; mapping never cycles or grows the pool. Uploads remain immediate whole-image submissions and do not depend on presentation.
• Share the four existing nearest/linear × repeat/clamp bitmap sampler configurations at renderer/device scope. Bitmap unload releases its texture but only unregisters the borrowed sampler.
• Preserve upload-stage errors, check submission results, and commit bitmap registration/accounting only after success. Fatal texture errors now exit unsuccessfully while optional continuation remains available.
• Split existing loader methods into a test-linkable implementation without changing the public bitmap API. Add decoded JPEG/TGA, failure/accounting/refcount/replacement, shutdown and fatal-dispatch tests. Guard truncated OZT headers/pixel payloads and initialize the bitmap-cache sentinel.
• Add an opt-in claimed-window, non-presenting GPU stress/readback test and document deliberate future higher-resolution builds. No SDL/vendor, asset, FD-limit or CI changes are included.

Related issues

Closes #620.

Verification

• Linux Release and Debug: 256/256 tests passed, including the real Vulkan GPU regression with validation layers observed enabled.
• Native Windows MSVC x64 Release: build passed; 259 tests passed, 1 opt-in GPU test skipped. Windows preflight.
• Pinned clang-format 21.1.8 and Cppcheck contribution checks passed locally. Fork quality CI passed.
• Server-ready startup measurements during development: three starts at actual soft/hard FD limits of 1024 reached connected login, with a sampled peak of 84 total FDs; three 8192-limit starts peaked at 82–84. The issue baseline peaked at 1406 with the higher limit and failed both 1024-limit attempts. Representative Debug starts also passed. These runtime measurements preceded the final test extraction/formatting/truncated-input guard; final-source build/test results are listed above.
• Manually opened and visually checked the client during development; it looked correct. This is not exhaustive world/editor or load/unload verification.

The Windows preflight commit 2b941fe8 contains this exact contribution (45cb60fb) plus only a fork-specific workflow. Quality CI ran at 8799e5d7, adding only a fork/temporary-branch-specific comparison-base correction because the fork's main branch is older than upstream. Both CI scaffolding changes are excluded from this PR; application and test sources match.

GPU regression

With a claimed window and no swapchain acquisition/presentation, each device lifetime performs 2400 changing uploads with three staging buffers, four samplers, three preallocated real destination textures and a reusable readback buffer. Intermediate checkpoints verify all three destination sizes, including padded rows and maximum-size images; the test repeats after device recreation. Only destination allocation is substituted through the existing test seam to isolate staging pressure; a separate default-allocation smoke test covers ordinary texture ownership.

To opt in when running CTest on a GPU-capable host, set MU_RUN_GPU_UPLOAD_TESTS=1; additionally set MU_GPU_UPLOAD_VALIDATION=1 for validation-mode coverage. Without opt-in, the GPU case explicitly skips.

Limits

No native D3D12/Metal runtime or TSan result is claimed. Decoder tests use real decoders/cache/accounting but stub external renderer/registry effects; fatal-dispatch tests stub popup and OS cleanup. GPU readbacks are periodic, not every upload. The original baseline lacked startup timings, so historical performance parity is not established. The sampler-allocation-limit failure itself was not reproduced; the measured regression is FD exhaustion.

Written with AI assistance and tested by hand.

Checklist

• I have read and followed docs/CODING_RULES.md.
• I have built the project locally.
• Changes are focused on the bitmap upload resource-lifetime fix and its verification.

[Mosch0512]

pls fix the merge conflicts <3

[alandarev]

Rebased onto current main (98816a6). Two adaptations to what landed meanwhile:

• the OZT header and truncation check now uses Render::Textures::Ozt::ReadHeader / CopyPixels
  (OztFile) instead of this PR's own check, in the loader file this PR introduces
  (GlobalBitmapLoad.cpp), together with the editor's load-world bookkeeping;
• TextureFailure dispatches through Core::Platform::ErrorDialog (UTF-8 title and message),
  with the same exit code and the same continue/fatal behaviour.

Build + ctest 471/471, clang-format and Cppcheck clean, GPU upload test with the Vulkan
validation layer passes. The Enhanced look PR (#716) builds on this branch as its first commit.

[alandarev]

Rebased onto current main (112cece, after #610). Adaptations to what landed meanwhile:

• the OZT header and truncation check now uses Render::Textures::Ozt::ReadHeader / CopyPixels
  (OztFile) instead of this PR's own check, in the loader file this PR introduces
  (GlobalBitmapLoad.cpp), together with the editor's load-world bookkeeping;
• feat: rmlui UI system #610's LoadImageExclusive, the padToPowerOfTwo parameter (RmlUi loads its images unpadded)
  and CreateBitmapStorage moved into the same loader file; the 1024x1024 decode limits still
  refuse larger images in both modes, so the 4 MiB staging slots hold;
• TextureFailure dispatches through Core::Platform::ErrorDialog (UTF-8 title and message),
  with the same exit code and the same continue/fatal behaviour.

main does not build on Linux (#730); with #719's fixes under it, build + ctest 621/621,
clang-format and Cppcheck clean, and the GPU upload test with the Vulkan validation layer passes.
On Windows (editor off) it builds and passes its tests in the fork's CI as the first commit of
#716's branch (https://github.com/alandarev/mumain-sven/actions/runs/38795377425). The Enhanced look PR (#716) builds on this branch as its first
commit.

[Mosch0512]

Verdict: changes requested (item 1 below).

Bounding the staging buffers and sharing the four samplers is the right fix for #620. Before this PR, every bitmap also kept its own sampler, which on large loads can run into Vulkan's maxSamplerAllocationCount (as low as 4000 on some drivers). Moving registration and accounting behind a successful upload also fixes the leaked m_dwUsedTextureMemory on failure. What needs changing is the failure model. One GPU error that leaves no staging slot in doubt now disables bitmap uploads for the rest of the session.

Required

1. A single failed texture creation, map, acquire or begin-copy faults the whole uploader until device shutdown. The next checked LoadBitmap (the default; about 700 of the 738 calls) then exits the client (inline, BitmapGpuResources.cpp line 150).

Non-blocking

• Diagnostics: with no initialized uploader, the error reads "upload: wrong thread" (line 106). After a fault, each rejection overwrites the root-cause error (line 113).
• std::exit replaces ExitProcess on Windows, so static destructors now run on the fatal path (TextureFailure.cpp line 28).
• A failed replacement still destroys the image it replaces. This predates the PR, but the new test locks it in (GlobalBitmapLoad.cpp line 221).
• Tests: the GPU regression prints the FD peaks but never checks them (test_bitmap_gpu_upload_integration.cpp line 307). bitmap_loader_tests redefines symbols that MuClient also defines (tests/render/CMakeLists.txt line 258).
• Efficiency follow-ups: one slot, submit and fence per image (line 133); the JPEG RGBA temporary (GlobalBitmapLoad.cpp line 451); 12 MiB of staging stays resident (line 49).
• Cleanups: an unreachable capacity check (line 127), duplicated pitch math (line 124), and CODING_RULES rules 1 and 3 in Upload and the sampler table (line 103, BitmapGpuResources.h line 111).

I reviewed this by reading and tracing the code. I didn't build or run it.

[Mosch0512]

Required: Fail(..., true) sets m_faulted. After that, every Upload returns "upload: uninitialized or faulted" until Shutdown, and production code never reads IsFaulted(). Every GPU stage sets it: texture creation here, map (157), acquire (172), begin-copy (179), submit (202) and the fence wait (95).

The comment at line 197 explains why a slot must not be reused after a failed submit. That reasoning only covers submit and the fence wait. In the other four cases, Retire has just waited for this slot, so it is idle and nothing reads it:

• texture creation and map fail before anything is recorded;
• acquire fails before a command buffer exists;
• begin-copy fails and the command buffer is cancelled.

The resulting failure mode is much broader than before. Before this PR, a failed SDL_CreateGPUTexture (for example running out of device memory on a 1024×1024 texture) failed only that bitmap. Now:

1. One unchecked load fails, for example an RmlUi or editor image through LoadImageExclusive, or one of the LoadBitmap(..., false) calls.
2. The next checked LoadBitmap also fails. That covers almost all of them, since bCheck defaults to true. PopUpErrorCheckMsgBox(..., true) then shows a dialog the player cannot continue past, and the client exits.
3. In a FOR_WORK build, where the player can continue, every map change force-unloads the IDs it replaces (GlobalBitmapLoad.cpp:221) before decoding. The new map then renders without textures.

Suggested fix: return a plain failure without faulting for texture creation, map, acquire and begin-copy. Only a failed submit or fence wait needs to take its slot out of service, and that could be done per slot: mark that slot as unusable and skip it, and fault only when no slot is left. The test "bitmap owner faults on GPU errors" would then expect !IsFaulted() for stages 0-3.

[Mosch0512]

Non-blocking: m_thread is a default std::thread::id until Initialize and again after Shutdown, and it never equals a running thread's id. Before init, after shutdown, or after a failed init, every upload therefore reports "upload: wrong thread: upload must run on the owner thread". The real cause is an uninitialized uploader, which the !m_device branch below would report. With m_device null that branch can't be reached. Checking !m_device first would give the right message. Reading m_device is no more racy than reading m_thread, given the external serialization documented in the header.

[Mosch0512]

Non-blocking: after a fault, every rejected upload calls Fail("upload: uninitialized or faulted", false). That overwrites m_stage and m_error, so the original SDL error is gone after the next load. The renderer's Shutdown log ("bitmap uploader shutdown failed at {}: {}") and every following g_ErrorReport line show the generic text. The root cause appears only in the first failure's line, among hundreds of follow-on failures. Returning {nullptr, nullptr, "upload: faulted", m_error} without calling Fail would keep it.

[Mosch0512]

Non-blocking: on Windows this path used to call ExitProcess(0), which skips C++ static destructors and atexit handlers. std::exit runs them. They now run from whatever call stack hit the texture error, after DestroyWindow/DestroySound have torn down part of the client while the renderer, the SDL GPU device and the window are still alive. Destruction order across files isn't defined. That covers the AnimationTaskPool singleton (joins its workers), g_MuHelper, the renderer's registry maps and s_bitmapGpuResources.

I haven't hit a crash from this. If the only goal was a failing exit code, ExitProcess(EXIT_FAILURE) keeps the old Windows behavior; on Linux and macOS the shim in WinApiShims.h already calls exit().

[Mosch0512]

Non-blocking (predates the PR): the replacement path force-unloads the current bitmap before the new file is even opened. If the new one fails to decode or upload, the ID is left empty. Any holders of the old image (it had Ref == 2 in the test) then draw the GetTexture error sentinel or the white fallback. The new test "bitmap replacement keeps existing ID and refcount semantics on rejected upload" asserts exactly this (GetNumberOfTexture() == 0), so it now locks the behavior in.

This PR already moved registration and accounting behind a successful upload. Decoding and uploading into a fresh BITMAP_t first, then unloading the old one and inserting the new one, would make a failed replacement keep the original. Otherwise, a test name that says the original is dropped would be clearer.

[Mosch0512]

Non-blocking: the three slots, 12 MiB of host-visible upload memory, stay allocated from renderer Init until device shutdown, although nearly all bitmap uploads happen on loading screens. The docs section already mentions the size. Was releasing them after load phases, or creating them on the first upload, considered? Either keeps the FD bound without the permanent footprint.

[Mosch0512]

Non-blocking: stagingBytes > kSlotCapacity can't be true. Width and height are bounded just above, and kSlotCapacity is derived from those same limits with static_asserts in the header. The dead check shares its message with the real pixels.size() check, which suggests a runtime overflow is possible. It could be dropped, or turned into an assert.

[Mosch0512]

Non-blocking: the 256-byte pitch rounding appears in kMaximumPitch, here, in the integration test, and as FrameReadbackRowAlignment in MuRendererSDLGpu.cpp. CheckDownload hard-codes + 255) / 256 * 256 (line 172 of the test), so changing kRowAlignment would leave the readback using the old pitch. A public static constexpr std::uint64_t PaddedPitch(int width) on BitmapGpuResources could serve kMaximumPitch, Upload and the test.

[Mosch0512]

Non-blocking: Upload is about 110 lines (103-213). It validates the request, retires the slot, creates the texture, copies rows, records the pass, submits and picks the sampler. CODING_RULES rule 1: "A function does one thing" and "Aim for functions that fit on one screen (~40 lines)". releaseTexture cleanup is repeated on four paths in between. Extracting the validation, the staging copy and the record/submit step would keep the ownership rules easy to check. It would also make the fault-model change above easier to apply.

[Mosch0512]

Non-blocking: the filter × wrap table is encoded with unnamed numbers in three places. The 4 here; i / 2 == 0 and i % 2 == 0, plus the stage-name string, in Initialize; and (LINEAR ? 2u : 0u) + (CLAMP ? 1u : 0u) in Upload. CODING_RULES rule 3: "if it's not self-evident, give it a name." A named kSamplerCount and one SamplerIndex(filter, wrap) helper used by both sides would keep them from drifting apart.

[nitoygo]

This PR is merged unchanged into the integration PR #764, together with #758 and #716, with the conflicts resolved there. When #764 merges with a merge commit, GitHub marks this PR as merged too.

Please don't push to this branch until #764 merges. A new commit here would leave this PR open, and it would have to be merged into #764 again.

What #764 adds on top of this PR:

• Upscaled textures. The loader in GlobalBitmapLoad.cpp now also carries feat(render): load upscaled textures marked in the file header #718's and fix(render): follow-ups to upscaled textures: canvas pixels and size limit #758's changes: the marker scale, original-size textures, canvases, released pixels and the texture memory count.
• Large images. fix(render): follow-ups to upscaled textures: canvas pixels and size limit #758 allows marked images up to 4096 px. The three 4 MiB slots stay as they are, and a level larger than one slot is uploaded in bands of rows, one submission each. docs/build/rendering-parity.md is updated.
• Failure handling (@Mosch0512's requested change), in commit 5d24eb1:
  • Texture creation, map, acquire and begin-copy failures fail only that upload.
  • A failed submit or fence wait retires only its own slot. The uploader stops once no slot is left.
  • A stopped uploader keeps the original error.
  • An uninitialized uploader reports that, instead of "wrong thread".

The opt-in GPU test passes on Direct3D 12 and Vulkan, including a new 2048×1030 banded upload.