[AKolenda]

ProgressiveDecoder kept its tile grid and the set of tiles sent in the current RDPGFX frame per (surface_id, codec_context_id). Windows moves a surface to a new codec context mid-session, deletes the old one with DeleteEncodingContext, and keeps refining the same tiles under the new id. Two things then went wrong without any error:

• A REGION under the new context that relies on a tile sent earlier in the same frame under the old context (MS-RDPEGFX 2.2.4.2.1.5 allows REGION rectangles to be covered by tiles from an earlier REGION in the frame) found no tile in its own set and painted nothing.
• A TILE_UPGRADE under the new context for a tile whose first pass arrived under the old context found a fresh tile with pass == 0 and was skipped.

On a Windows 11 host this left vertical 64-pixel columns of earlier animation frames on screen (swiping cards in a browser) until something repainted the area, such as scrolling.

Change

Tile state and frame tile membership are now kept per surface, shared by every codec context on it:

• ProgressiveDecoder stores one SurfaceTiles grid per surface and one frame tile set per surface.
• delete_context no longer releases tile state. MS-RDPEGFX lets a progressive tile context be associated with several codec contexts, and Windows deletes the previous context while the next one is still refining the same tiles. delete_surface and reset still release the state.
• decode_bitmap keeps its signature; the codec context id no longer selects any state.

This matches FreeRDP, which keeps one progressive tile grid per surface (libfreerdp/codec/progressive.c) and treats DeleteEncodingContext as a no-op (gdi_DeleteEncodingContext in libfreerdp/gdi/gfx.c).

Sub-band difference references were already keyed per surface and are unchanged. Keeping one grid per surface instead of one per codec context also stops the number of codec context ids from multiplying tile memory.

Validation

• New ironrdp-egfx test region_reuses_tile_sent_under_another_codec_context: a REGION under codec context 8 reuses a tile sent earlier in the same frame under context 7, and the output regions and pixels match the same-context case. It fails on master: the second rectangle is never painted.
• New ironrdp-graphics test upgrade_under_a_new_codec_context_refines_the_surface_tile: an UPGRADE under a new context, after the old context is deleted, refines the tile and produces the same coefficients and pixels as an uninterrupted same-context refinement. On master the upgrade is skipped.
• Existing tests that referred to per-context state now use the per-surface keys; progressive_context_is_deleted_with_encoding_context is renamed progressive_tiles_survive_encoding_context_deletion.
• cargo test -p ironrdp-graphics (246 passed), cargo test -p ironrdp-egfx (58 passed), cargo test -p ironrdp-testsuite-core --test integration_tests_core (1835 passed, 2 ignored), cargo xtask check fmt, cargo xtask check lints.
• Live test against the Windows 11 host that showed the artifacts, using a client build with the same decoder change: the stale columns no longer appear.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T06:41:00.539203Z	cd041ee	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "Codex (@codex) review" or "Codex (@codex) security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[Copilot]

🟢 Approval recommended

The reported failures have focused regression coverage; the remaining documentation mismatch is minor.

1 open finding

• Correct state management docs to describe per-surface tile state · New

What changed in this PR

This PR keeps Progressive tile state per surface so a new codec context can reuse and refine tiles without leaving stale graphics on screen.

Changes:

• Share tile and current-frame state across codec contexts on the same surface.
• Preserve tiles when an encoding context is deleted, while clearing them when the surface is deleted or the decoder resets.
• Add regression tests for cross-context REGION reuse and tile upgrades.

File	Description
crates/​ironrdp-graphics/​src/​progressive.rs	Moves tile ownership to surfaces and tests cross-context refinement.
crates/​ironrdp-egfx/​src/​client.rs	Tests cross-context REGION reuse through the graphics client.

🧠 Review effort: Lite

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd041eec4c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

[github-actions]

The PR collapses Progressive tile state from (surface_id, codec_context_id) keys to per-surface keys and makes delete_context a no-op. The core fix is well supported: MS-RDPEGFX 3.3.1.2 associates a progressive tile context with a surface and one or more codec contexts, FreeRDP keeps one grid per surface, the new egfx test exercises cross-context REGION tile reuse, and live Windows validation is reported. I independently confirmed in pr-head that delete_context is an empty no-op (progressive.rs:1530), decode_bitmap ignores _codec_context_id (progressive.rs:1331), and the egfx client still calls delete_context (client.rs:641). The residual defect is that the decoder no longer tracks codec-context identity at all, so the MS-RDPEGFX 3.3.5.3 MUST (remove the context on DELETE_ENCODING_CONTEXT) and 3.3.5.2 MUST (fresh progressive render for an unestablished contextId) are both unimplementable; I merged the two protocol candidates into one medium-severity finding since they share this root …

1. [protocol] Codec context lifecycle is untracked: DELETE_ENCODING_CONTEXT is a no-op and unestablished context IDs cannot start a fresh render — medium 🟠 — crates/ironrdp-graphics/src/progressive.rs
   After this PR the decoder keys all progressive state by surface and keeps no per-context record, so two MS-RDPEGFX MUSTs are unimplementable: 3.3.5.3 (remove the codec context identified by surfaceId/codecContextId when RDPGFX_DELETE_ENCODING_CONTEXT is decoded) and 3.3.5.2 (create a new context and begin a fresh progressive render when a WireToSurface2 carries a codecContextId that is not established). delete_context is an empty no-op (progressive.rs:1530, called from client.rs:641) and decode_bitmap ignores _codec_context_id (progressive.rs:1331), so a deleted or second context ID is decoded as a continuation of the previous bitmap, including upgrades against the other bitmap's coefficients. Per-surface tile sharing itself is sound per 3.3.1.2 and fixes real Windows artifacts; the residual gap only bites spec-legal server patterns Windows does not exercise, hence medium rather than high. A context-lifecycle map (for fresh-render reset, optionally also tile grid clearing) would restore conformance.
2. [skeptical + code-compressor] delete_context is an empty public no-op retained only for signature compatibility — low 🟡 — crates/ironrdp-graphics/src/progressive.rs
   The method body is {} and both parameters are unused; the only in-repo caller is crates/ironrdp-egfx/src/client.rs:641 plus tests. Keeping it avoids a breaking change to the published ironrdp-graphics crate, and the doc comment documents the no-op, but it leaves public API whose only observable behavior is 'nothing happens' and which contradicts its historical 'free the context' contract. Recommend a #[deprecated] note pointing at delete_surface, or removal of the method, the client.rs call, and the test invocations in the next breaking release.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

[github-actions]

[code-compressor] decode_bitmap keeps a _codec_context_id parameter that selects no state — low 🟡 — The parameter is underscore-prefixed and the doc bullet explains it 'does not select any state'; every caller must still plumb a value the decoder ignores, and the doc caveat exists purely to apologize for the dead argument. Removing it (updating the egfx caller and tests) matches the same reasoning that dropped the composite map keys, but it is a public API break, so retaining the signature is a defensible choice; treat this as optional cleanup for the next breaking release rather than a defect.

[AKolenda]

Leaving decode_bitmap as is so a bug fix does not change public API; dropping the unused codec_context_id parameter fits the next breaking release. Details in the PR comment below.

[AKolenda]

Thanks. Addressed in 105da2c:

• Module docs: the State management overview no longer says tile state is per codec context.
• delete_context doc: it now cites [MS-RDPEGFX] 3.3.1.2.

No code change for the codec-context lifecycle finding:

• Spec permits sharing and retention. [MS-RDPEGFX] 3.3.1.2 (v20260511) associates each progressive tile context with a surface tile and one or more codec contexts. Such a context "can be discarded once all of the codec contexts with which it is associated have been deleted". Discarding is optional, so keeping tile state with the surface is within the spec. FreeRDP does the same: one grid per surface, and gdi_DeleteEncodingContext is a no-op.
• A fresh render under a new context still decodes correctly. A server starting a new bitmap sends SIMPLE or FIRST tiles, which replace the tile's progressive refinement state: coefficients, signs, quantization and quality. Their difference variants also use the retained sub-band references, but those were already kept per surface before this PR. UPGRADE tiles and tile-free REGION reuse consume the retained tile state. Windows sends exactly those under a new codec context for tiles first sent under the previous one, which is the bug this PR fixes. Resetting or discarding tile state when a context changes or is deleted would bring the artifacts back. The same risk applies to association tracking that records an association only when the new context touches a tile and discards on the last delete.
• Remaining gap is validation only. The decoder does not track which codec context IDs are established. It therefore cannot reject an UPGRADE whose context ID was never established or was already deleted (3.3.5.2 says the context MUST be known for an UPGRADE). Such a stream refines the retained surface tile instead of failing. A context-ID map could enforce this without clearing surface tiles. That would be stricter validation for non-conforming servers, not something this fix needs, so I'd keep it out of this PR.

delete_context and the decode_bitmap signature stay as they are so a bug fix doesn't change public API. Deprecating or removing them can be decided for the next breaking release.

The overlap notice about #2085 is accurate. The reduce-extrapolate CONTEXT fallback and uses_reduce_extrapolate call are pre-existing; this PR only changes which map the fallback reads. Whichever PR lands second will need a small rebase.

[github-actions]

Re-scopes ProgressiveDecoder tile state and frame-tile tracking from (surface_id, codec_context_id) to surface_id, makes DELETE_ENCODING_CONTEXT a no-op for tile state, and adds regression tests for REGION reuse and UPGRADE refinement across a codec-context switch. Independent review confirms this matches MS-RDPEGFX 2.2.4.2.1.5 (REGION tiles may come from earlier REGIONs in the same frame, across codec contexts) and 3.3.1.2 (tile contexts shared by one or more codec contexts, discarding optional), mirrors FreeRDP, and fixes real rendering artifacts without unbounded state growth since delete_surface/reset still clear per-surface state. All four validated specialist candidates are published: one low-severity recorded deferral (lost per-context validation for non-conforming UPGRADEs) and three low-severity hygiene items (test fixture duplication, unused decode_bitmap parameter, no-op public delete_context). No correctness, protocol, or memory-safety issues found.

1. [skeptical] No-op delete_context lets UPGRADE payloads from unknown or deleted codec contexts mutate retained tiles — low 🟡 — crates/ironrdp-graphics/src/progressive.rs
   Previously tile state was isolated per (surface_id, codec_context_id), so an UPGRADE under a never-established or already-deleted context found a fresh pass==0 tile and was skipped. With tile state now shared per surface and delete_context a no-op, the same malformed payload refines the retained tile and publishes pixels, and the decoder cannot reject it because it never records which codec context IDs were established (MS-RDPEGFX 3.3.5.2 requires the context to be known for an UPGRADE). Impact is limited to pixel corruption from non-conforming servers on an already-untrusted decode path; no memory-safety or DoS concern. The PR conversation acknowledges the gap and reasonably defers a context-ID establishment map (enforceable without clearing surface tiles) to a follow-up; recorded so the deferral is a decision, not an unnoticed regression.
2. [code-compressor] delete_context is an empty public method kept as a shim — low 🟡 — crates/ironrdp-graphics/src/progressive.rs
   The body is now empty; its only in-repo caller is the crate-internal dispatch at ironrdp-egfx/src/client.rs:640-641, which invokes it right after logging the same PDU. Keeping a public no-op preserves API compatibility (a legitimate, documented tradeoff), but the indirection can be reduced now: delete the no-op call from the egfx client (private to that crate, trace logging already covers the PDU), mark the method #[deprecated] pointing at delete_surface/reset, and remove it at the next breaking release. This sheds a dead call path without changing any observable behavior.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

[github-actions]

[code-compressor] decode_bitmap keeps an unused codec_context_id parameter — low 🟡 — The parameter no longer selects any state; it is retained only to avoid breaking the public API in a bug-fix release, which the author documented. Optional compression: add a deprecation note ("codec context no longer selects decoder state") now and drop the parameter at the next breaking release, so the vestigial argument is scheduled for removal rather than silently persisting. No behavior change either way; callers are unaffected by a deprecation attribute.

[AKolenda]

Keeping this as is, as noted on the earlier thread: the ignored parameter is documented, and dropping it is a public API change better made in the next breaking release.

[AKolenda]

Addressed the re-review in 199015f:

• Test fixture duplication: fixed. progressive_tile_stream takes the REGION rectangle and both tests use it. The cross-context test still fails against the upstream master decoder.
• UPGRADE under an unknown or deleted codec context: agreed, this is a deliberate deferral. A context-ID establishment map can enforce it later without clearing surface tiles.
• delete_context / decode_bitmap: no change. delete_surface and reset are not semantic replacements for delete_context, so a deprecation pointing at them would mislead. Both signatures stay as is to keep public API unchanged in a bug fix.

@github-actions review-ready

[github-actions]

The PR re-keys ProgressiveDecoder tile state and frame tile membership from (surface_id, codec_context_id) to surface_id, making delete_context a no-op, so REGION reuse and TILE_UPGRADE across a mid-session codec context switch work. This matches MS-RDPEGFX 3.3.1.2 (progressive tile contexts are per-tile surface state shared by codec contexts, discarding optional) and FreeRDP's per-surface grid with a no-op DeleteEncodingContext. Cross-surface isolation, delete_surface, and reset are preserved, and two regression tests reproduce the fixed Windows artifacts. The change is correct; four low-severity residual items were verified independently: the no-op delete_context and ignored decode_bitmap codec_context_id parameter are retained API surface (documented deferral to the next breaking release, tracked so it is not forgotten), an UPGRADE under a never-established or deleted context ID now silently refines surface tiles instead of erroring (MS-RDPEGFX 3.3.5.2 MUST, deferred with a concret…

1. [skeptical] UPGRADE tiles under never-established or deleted context IDs silently refine surface tiles — low 🟡 — crates/ironrdp-graphics/src/progressive.rs
   With state keyed only by surface_id, an UPGRADE or tile-free REGION reuse carrying a codec context ID that was never established or was deleted now consumes the surface's retained tile grid instead of finding no tile and skipping, while MS-RDPEGFX 3.3.5.2 requires the context to be known for an UPGRADE. Conforming servers are unaffected; the author deferred enforcement to a follow-up context-ID establishment map that validates without clearing tiles. Track as a validation gap for a separate change.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

[AKolenda]

Re-review of 199015f: no code change.

• UPGRADE under unknown or deleted context IDs: the deferred validation gap, unchanged.
• Inert delete_context / decode_bitmap parameter: deferred to the next breaking release.
• Shadowed reduce-extrapolate fallback: kept. It is not equivalent when a resize fails after a CONTEXT block changed the flag; details are on that thread.

@github-actions review-ready