Devolutions / Devolutions/IronRDP
Optional EGFX payload dump for offline triage of decode/encode failures
- Dominant language
- Rust
- Stars
- 3.2k
- Forks
- 275
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 189
Description
## Summary
Propose adding an opt-in payload-dump capability to `ironrdp-egfx` (and symmetrically to `ironrdp-server`) triggered by an `IRONRDP_EGFX_DUMP_DIR` env var. When the env var is unset, zero behavior change and a single `env::var_os()` cold-path check on the error path only.
## Why
EGFX decode failures against modern Windows servers are notoriously hard to diagnose without the failing bytes on disk. The PDU layer hands `bitmap_data` to the codec dispatch and a `Result<()>` comes back. If the codec rejects, the only artifact is a `tracing::warn!` line. Reproducing requires re-running the entire RDP session against the same server in the same state, which is often unavailable.
The pattern is widely used in similar projects:
- FreeRDP has codec-specific dumps via its WLOG appender plus `winpr/tools/rdp-decoder`.
- A downstream IronRDP consumer (Haven, github.com/GlassHaven/Haven) ships an `EGFX_DUMP_DIR` env var doing exactly this.
When triaging the RDPGFX_RECT16 off-by-one in #1238 (caught by Haven's dump-equipped decoder), having the failing bytes on disk would have shaved hours off reproduction.
## Proposed shape
`crates/ironrdp-egfx/src/client.rs`: at each codec dispatch error path (`decode_avc420`, `decode_clearcodec`, `decode_avc444`, `handle_uncompressed`, future `decode_progressive`), check `IRONRDP_EGFX_DUMP_DIR`. If set, write:
- `{millis}_{codec}_surface{id}.bin` containing the raw `bitmap_data` bytes
- `{millis}_{codec}_surface{id}.json` containing `{rect, codec, surface_id, error}` (hand-rolled, no `serde_json` dep added)
`crates/ironrdp-server/src/encoder/mod.rs`: parallel hook on encode error paths. Dumps the source pixel buffer and any partial output. The server-side feature is rarer in practice (we control the bytes we produce) but symmetric for completeness.
Naming: `IRONRDP_EGFX_DUMP_DIR` matches the existing `IRONRDP_LOG_PATH` / `IRONRDP_LOG` env conventions. Cost when unset: one `env::var_os()` call on the error path only, no impact on success paths or builds.
## Questions before I PR this
1. Is this kind of runtime payload-capture welcome in `ironrdp-egfx`, or would you prefer it live in a separate `ironrdp-egfx-debug` crate behind a feature flag?
2. Server-side encode-error dump: include in the same PR or follow-up? My read is that server-side encode errors are rare enough that the value is mostly symmetric / completeness; the high-value case is client-side decode errors.
3. Naming: `IRONRDP_EGFX_DUMP_DIR` ok, or prefer something different (e.g., `IRONRDP_DUMP_DIR` if you imagine extending to other codecs / channels)?
Happy to PR this once the shape is endorsed.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.