fix(server): leave room for the surface-bits framing when tiling updates - #1941
Draft
Marynych Oleksandr (maryny4) wants to merge 1 commit into
Draft
Marynych Oleksandr (maryny4) wants to merge 1 commit into
Marynych Oleksandr (maryny4) wants to merge 1 commit into
Conversation
The encoder splits large bitmaps into strips so that each uncompressed
surface-bits update fits the MultifragmentUpdate reassembly buffer it
advertised, but it budgeted the pixels alone. `set_surface` wraps them in
a `TS_SURFCMD_SET_SURF_BITS` and a `TS_BITMAP_DATA_EX` (22 bytes), and the
client counts the whole reassembled update against the buffer. Whenever
`width * 4` divides the buffer size exactly, or leaves fewer than those 22
bytes over — 2048 or 4096 pixels wide with the 8 MiB default — a strip
fills the buffer with pixels alone, the framing pushes the update past the
limit and FreeRDP drops the connection ("Total size (8388630) exceeds
MultifragMaxRequestSize (8388608)").
Budget strips against `max_request_size` minus the framing, sized from the
surface-bits PDU itself rather than a hardcoded number. The tiler runs
ahead of codec selection, so the compressed codecs and the legacy bitmap
path get the same one-row-shorter strips at those widths; nothing else
changes for them, and widths that already left room keep their strip
height (3840 leaves 2 KiB).
The integration tests drive the encoder through its bench surface: a
2048-wide frame now leaves the buffer as a 1023-row strip plus the
remainder, each update at most the advertised size (the previous code
produced an 8388630-byte update); a 4096-wide frame splits into 511-row
strips; a frame that fits with its framing is sent whole; and a budget
below one row still splits by width. The unit tests next to the tiler pin
the strip heights and the 22-byte framing; they run with `cargo test
--lib`, since this crate's library tests are not part of `cargo test`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The encoder splits large bitmaps into strips so that each uncompressed surface-bits update fits the MultifragmentUpdate reassembly buffer it advertised, but it budgeted the pixels alone.
set_surfacewraps the pixels in aTS_SURFCMD_SET_SURF_BITSand aTS_BITMAP_DATA_EX(22 bytes), and the client counts the whole reassembled update against the buffer. Wheneverwidth * 4divides the buffer size exactly, or leaves fewer than those 22 bytes over — 2048 or 4096 pixels wide with the 8 MiB default — a strip fills the buffer with pixels alone, the framing pushes the update past the limit and FreeRDP drops the connection withTotal size (8388630) exceeds MultifragMaxRequestSize (8388608). Reported downstream as MuNeNiCK/hypr-rdp#94.The fix budgets strips against
max_request_sizeminus the framing, sized from the surface-bits PDU itself rather than a hardcoded number; the advertised capability is unchanged. The tiler runs ahead of codec selection, so the compressed codecs and the legacy bitmap path get the same one-row-shorter strips at those widths and nothing else changes for them; widths that already left room keep their strip height (3840 leaves 2 KiB).The integration tests in
ironrdp-testsuite-coredrive the encoder through its bench surface: a 2048-wide frame leaves as a 1023-row strip plus the remainder (the previous code produced an 8388630-byte update), a 4096-wide frame splits into 511-row strips, a frame that fits with its framing is sent whole, and a budget below one row still splits by width. The unit tests next to the tiler pin the strip heights and the 22-byte framing; they run withcargo test -p ironrdp-server --lib, since this crate's library tests are not part ofcargo test.Checked against a real client as well: hypr-rdp 0.1.6 in a VM (Hyprland 0.56.2, FreeRDP 3.31.1,
sdl-freerdp3 /size:2048x1080 /bpp:24, which keeps the graphics pipeline closed so the server sends bitmap updates). With the pinned IronRDP the first frame ends the session with the error above and the client decodes nothing; with this change the client decodes the first frame as a 1023-row strip and a 57-row strip (gdi_surface_bits … destTop 0 destBottom 1023anddestTop 1023 destBottom 1080) and the session stays up.