Repository navigation
fix(engine): render text without ClearType colour fringes on Windows - #7663
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed at aa409f13. The code change is correct: every render, snapshot, capture, validate, layout, studio and audio-fx path launches Chrome through buildChromeArgs, so a single --disable-lcd-text entry covers all of them. The Windows before/after numbers and the green Linux regression shards support it. I found no blocker in the code.
Requesting changes for one reason: the PR body is missing the required fails without the fix: line. A fix PR has to name the test that fails without the fix on a line of its own. The test plan currently says "Fails without the fix (3 failed, 121 passed)" but never names the test. Please add something like:
fails without the fix: packages/engine/src/services/browserManager.test.ts › buildChromeArgs browser GPU mode › always disables LCD text (beginframe|screenshot|drawelement)
I'll approve once the body has that line. No code change is needed.
Non-blocking: packages/producer/src/parity-harness.ts:332 and packages/player/tests/perf/runner.ts:94 still build their own arg lists with --font-render-hinting=none and no --disable-lcd-text. Linux CI is unaffected. If the parity harness is ever run on Windows, though, it would compare against fringed text. You could add the flag there too, either in this PR or in a follow-up.
There was a problem hiding this comment.
Re-reviewed at aa409f13. The head is unchanged, and the body now names the test that fails without the fix: browserManager.test.ts › buildChromeArgs browser GPU mode › always disables LCD text. That test checks for --disable-lcd-text in the args, so it can't pass without the new line. My earlier review found no problems in the code. Approving. The parity harness and perf runner note from before is still optional.
What
Always launch Chrome with
--disable-lcd-text, so text is rasterised with greyscale antialiasing instead of LCD subpixel antialiasing.Why
On Windows with ClearType on, glyph edges in renders, snapshots and captures get a red/blue colour split, including text on an opaque page background in an MP4. Subpixel antialiasing is tuned for one physical display, so in a video frame it is just a colour error baked into the pixels.
--disable-lcd-textremoves it with no glyph movement, only colour. Linux output is unchanged (see below).Related work
Refs #7661. This takes the issue's first proposal as the default for every caller. It does not add a way to pass arbitrary Chrome flags.
How
One line in
buildChromeArgs(packages/engine/src/services/browserManager.ts), next to--font-render-hinting=none. Render, snapshot, capture, validate, layout and the studio thumbnails all build their Chrome args through it, so every path gets the flag.Test plan
fails without the fix: packages/engine/src/services/browserManager.test.ts › buildChromeArgs browser GPU mode › always disables LCD text (beginframe|screenshot|drawelement)
Unit test:
buildChromeArgscontains--disable-lcd-textin every capture mode, and on win32. Fails without the fix (3 failed, 121 passed), passes with it (124 passed).Windows, measured on a GitHub
windows-latestrunner with ClearType turned on (SPI_GETFONTSMOOTHING1,SPI_GETFONTSMOOTHINGTYPE2). Fixture: 22 px Consolas,#f0f0f0on#202020, 1280x720. Same build, only this line removed for "before". Split = sum of |R−B| over sum of |G−background| on glyph pixels (0 = neutral grey):Run: https://github.com/heygen-com/hyperframes/actions/runs/43492623591 (artifact
lcd-outhas the before/after frames and 6x crops).Linux, no regression: the PR's
regressionshards are green. The full producer suite (80 fixtures, the CI Docker test image on linux/amd64) passed 74 with this change. The 6 that failed are all inshard-schedule.json's "never run in CI" list, and fail the same way without this change: the same failed frame counts and the same PSNR to two decimals forfast-capture-gsap,heygen-promo-preview-assetsandpage-side-shader-compositor-render-compat; 99 failed frames at PSNR about 12.3 either way forwebgl-video-texture-render-compat(it varies by 0.05 between runs); and the same 45 s setup timeout ford3-adapterandmaplibre-adapter. No baseline moved.