Add Windows SDR to HDR Tonemapping Fix mod - #5016
Conversation
|
Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review. To get started, comment See the pull request review process for the full details. |
|
/ai-review |
|
/ai-review |
|
@millerpb |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The approach (reversible in-memory patching of the DXBC constants, with checksum fixup) is sound and there's precedent for this kind of thing in the catalog, and there's no existing mod that overlaps. A few things need attention before merge. 1. The README is missing the
## ⚠ Important usage note ⚠
In order to use this mod, you must allow Windhawk to inject into the **dwm.exe**
system process. To do so, add it to the process inclusion list in the advanced
settings. If you do not do this, it will silently fail to inject.
2.
-// @architecture amd64
+// @architecture x86-643. The hardcoded checksum allowlist makes the mod break silently on every
You already have a much more version-stable identifier: the sRGB constants themselves. Consider dropping the allowlist and instead, for every DXBC blob found, running the constant search on a copy and patching only if all four splats are present: // PatchShaderBuf: count per-constant matches instead of returning a single bool.
static int PatchShaderBuf(BYTE* buf, DWORD size, float gamma) {
int constantsFound = 0;
for (int ci = 0; ci < 4; ci++) {
...
if (foundThisConstant) constantsFound++;
}
return constantsFound; // caller requires == 4
}Requiring all four makes false positives essentially impossible (a blob containing Related, and important if you make this change: 4. The memory-region scan can silently skip the shaders — walk the PE sections instead.
if (mbi.State == MEM_COMMIT &&
mbi.Protect == PAGE_READONLY &&
regionSize > 4096)An exact Since you already parse the PE headers to get auto* sec = IMAGE_FIRST_SECTION(nt);
for (WORD i = 0; i < nt->FileHeader.NumberOfSections; i++, sec++) {
if (!(sec->Characteristics & IMAGE_SCN_MEM_READ) ||
(sec->Characteristics & IMAGE_SCN_MEM_EXECUTE)) {
continue;
}
totalPatched += ScanRegionForShaders(modBase + sec->VirtualAddress,
sec->Misc.VirtualSize, gamma);
}Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
I've addressed the suggestions in the ai review within reason. However, the constant-only detection matched 37 unrelated shaders on Windows 11 (instead of the known 4), so checksums were kept as the primary selector with HasDXBCSubBlob kept as an improvement to the container detection logic. |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The previous round's items are all addressed — the 1. Verify The whole reason the mod recomputes the checksum is that D3D rejects a DXBC blob whose hash doesn't match. So writing a wrong checksum is worse than not patching at all: the shader stops being creatable, and in DWM that means a compositing failure rather than "the mod did nothing". For the four leaf shaders the allowlist makes this fairly safe — a checksum match means the bytes are exactly what you tested against. But the container blob at lines 474-476 and 567-569 is not allowlisted: it's whatever enclosing blob the heuristic picked, its contents change with every A pre-flight self-check costs nothing and turns that into a clean skip: // Before touching anything: confirm our checksum routine reproduces this blob's
// own checksum. If it doesn't, we can't safely rewrite it.
static bool ChecksumRoundTrips(const DXBCHeader* hdr) {
DWORD verify[4];
CalcDXBCChecksum((BYTE*)hdr, hdr->size, verify);
return memcmp(verify, hdr->checksum, 16) == 0;
}Apply it to a container before accepting it as 2. The container relationship is positional, and only one nesting level is handled.
Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ready-for-reviewer |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. Solid, self-contained submission — no hooks, fully reversible in memory, correct use of 1. The container round-trip guard doesn't skip the container
if (!ChecksumRoundTrips(hdr))
{
Wh_Log(L" Container at %p: checksum did not round-trip, skipping", (void *)hdr);
i += hdr->size - 1; // loop will add 1 more, landing just after the blob
continue;
}2. The leaf's checksum is never round-trip-verified before it's overwritten Containers get if (matchIdx < 0)
continue;
// The expected hash is kKnownChecksums[matchIdx], so this validates our
// checksum implementation against this exact blob before we overwrite it.
if (!ChecksumRoundTrips(hdr))
{
Wh_Log(L" Shader #%d at %p: checksum did not round-trip, skipping", matchIdx, (void *)hdr);
continue;
}3. A partial patch is reported as success
static unsigned g_matchedMask; // bit i = kKnownChecksums[i] was patched
...
g_matchedMask |= 1u << matchIdx; // where numPatched++ happens
...
// in Wh_ModInit, after the section loop:
if (g_matchedMask != 0xF)
{
Wh_Log(L"Only mask 0x%X of 4 target shaders patched — reverting", g_matchedMask);
RestoreAllPatches();
return FALSE;
}4. The checksum allowlist pins the mod to a single
5.
6. README: no recovery path if DWM won't come up The mod rewrites bytecode the desktop compositor has to accept. If that ever goes wrong on a build you haven't tested, the user is looking at a broken desktop with no obvious way back — and unlike an Explorer mod, they can't just kill the process. A couple of lines under "Known limitations" describing how to recover (safe mode, disabling the mod / Windhawk from there) would be worth having. Optional improvements
Minor polish — none of this affects users today, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself.
Next steps:
See the review process for details. |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. Nice idea, and a genuinely better home for it than the upstream PoC: no 1. The shader identification is pinned to one
Two things worth doing:
2. Lines 405–410 say:
…but the code only logs the counts (line 627–628) and patches every occurrence anyway. If that comment is right, then on a build where a target shader also contains an sRGB encode path, the mod silently corrupts it — and in DWM that means a compositor failure, not a cosmetic glitch. Either enforce the check or drop the claim. The fail-safe machinery already exists, so enforcing is cheap: for (int ci = 0; ci < 4; ci++) {
if (counts[ci] != 1) {
Wh_Log(L" Shader #%d: constant %d matched %d time(s), expected 1 — skipping",
matchIdx, ci, counts[ci]);
continue; // -> partial mask -> Wh_ModInit reverts everything
}
}If in fact you've verified that a count > 1 is normal and harmless on the shaders you target, then the comment is the thing to fix. 3. The "HDR may be off" diagnostic is wrong and will send users down the wrong path. Line 756: Wh_Log(L"No target shaders found — HDR may be off, or dwmcore.dll checksums changed after a Windows Update");The shader blobs live in 4. Rewriting the entire blob into live read-only image memory is a wider window than it needs to be.
Have // PatchShaderBuf fills `offsets` with the byte offset of each 12-byte splat it replaced.
for (size_t off : offsets) {
PatchRecord rec;
rec.addr = (BYTE*)hdr + off;
rec.original.assign(rec.addr, rec.addr + 12);
if (!WriteMemorySafe(rec.addr, patched.data() + off, 12)) { /* roll back, continue */ }
g_patches.push_back(std::move(rec));
}
// checksum lastThis also makes the multi-page Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
Made more adjustments per ai review. Here are some explanations for the ones I did not pursue. #4b — #4c — "out-of-bounds leaf" rollback leaves #4e — |
|
This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time. Comment |
|
/ai-review |
|
This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time. Comment |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The patching machinery is in good shape now — the What's left is about what happens when the allowlist doesn't match, which is the mod's normal state on every build except the one you tested. 1. When the checksums don't match, neither the user nor you get anything to act on. The README says "please file an issue", and the log says the checksums changed — but there is nothing in that report to work with. A read-only diagnostic pass, run only when // Read-only: log every leaf blob that carries all four sRGB splats exactly
// once, so a user on an unrecognized build can paste the checksums into an
// issue.
static void LogSrgbCandidates(BYTE* region, size_t regionSize) {
for (size_t i = 0; i + kDXBCHeaderSize <= regionSize; i += 4) {
auto* hdr = (DXBCHeader*)(region + i);
if (memcmp(hdr->magic, "DXBC", 4) != 0 || hdr->reserved != 1 ||
hdr->size < (DWORD)kDXBCHeaderSize || i + hdr->size > regionSize ||
HasDXBCSubBlob(hdr)) {
continue;
}
std::vector<BYTE> tmp(hdr->size);
memcpy(tmp.data(), hdr, hdr->size);
int counts[4] = {};
if (PatchShaderBuf(tmp.data(), hdr->size, 2.2f, counts) != 4 ||
counts[0] != 1 || counts[1] != 1 || counts[2] != 1 ||
counts[3] != 1) {
continue;
}
const BYTE* ck = (const BYTE*)hdr->checksum;
Wh_Log(L"sRGB candidate at +0x%zx, size %lu, checksum "
L"%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x",
i, hdr->size, ck[0], ck[1], ck[2], ck[3], ck[4], ck[5], ck[6],
ck[7], ck[8], ck[9], ck[10], ck[11], ck[12], ck[13], ck[14],
ck[15]);
}
}This also settles the question behind the fallback you deferred. Your measurement of 37 over-matches was for constant-only detection; the selector 2. The README has no recovery path if DWM doesn't come up. This was raised in an earlier round and is still missing. The guards make it unlikely — an allowlisted checksum means byte-identical content, and ## If something goes wrong
If DWM fails to start or the desktop is unusable after enabling this mod, boot
Windows into Safe Mode (Windhawk does not inject there) or start Windhawk with
`windhawk.exe -safe-mode`, then disable or uninstall the mod.Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself.
Next steps:
See the review process for details. |
|
Okay, I've addressed the remaining points, optional improvements, and verified the fourth component of the splat concern: I disassembled all four target blobs directly from the installed dwmcore.dll using D3DDisassemble. In the SM4 (lib_4_0) sections, the only paths that run on any HDR-capable hardware, every sRGB constant is a 4-component immediate of the form l(x, x, x, 0.000000), and every instruction that references them uses .xyz only. The .w slot is always 0, is never read, and is never touched by our patch. Confirmed non-issue. |
|
Made some more changes to target more eotf values in dwm. This generally fixes some of the odd flicker in scRGB apps. Only seems occur on occasion now on window resize, then back to expected scRGB on interaction. |
|
/ai-review |
Windows SDR to HDR Tonemapping Fix
Adds a new mod (
dwm-eotf-gamma.wh.cpp) that patches the DXBC shader bytecode indwmcore.dllat DWM startup to replace the sRGB EOTF with a configurable pure power-law gamma for SDR-to-HDR tone mapping.What it does
When Windows HDR is enabled, DWM converts SDR content to HDR scRGB using the sRGB EOTF (an ~2.2 power curve with a linear toe segment near black). This mod patches the four floating-point constants that define that transfer function — replacing them with a simple power-law curve — giving users direct control over the SDR-to-HDR tone mapping without permanently modifying any files on disk.
Based on dwm_eotf by ledoge (GPL-3.0).
Key details
dwm.exe(x86_64)dwmcore.dll's read-only memory at startup; recalculates shader checksums so D3D accepts the patched bytecodeChangelog
If this pull request updates an existing mod, describe the changes below:
Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by:
Based on https://github.com/ledoge/dwm_eotf
Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.