Repository navigation
feat(rdpsnd): harden Windows client playback path - #1648
Conversation
GitHub Actions
Inactive
github-actions
Bot
added
needs-review
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Hardens RDPSND negotiation, playback, volume handling, and audio-mode configuration.
Changes:
- Preserves negotiated client format indices and supports pre-v8 waves.
- Expands CPAL PCM support and applies per-channel volume.
- Aligns audio-mode configuration, tests, and ActiveX documentation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
crates/ironrdp-testsuite-extra/tests/client_config.rs |
Tests .rdp audio modes. |
crates/ironrdp-testsuite-core/tests/rdpsnd/server.rs |
Updates negotiation tests. |
crates/ironrdp-testsuite-core/tests/rdpsnd/client.rs |
Tests formats and wave handling. |
crates/ironrdp-rdpsnd/src/server.rs |
Uses shared format matching. |
crates/ironrdp-rdpsnd/src/client.rs |
Preserves format order and handles waves. |
crates/ironrdp-rdpsnd-native/src/cpal.rs |
Expands PCM playback and volume support. |
crates/ironrdp-rdpsnd-native/examples/cpal.rs |
Supplies volume state to the example. |
crates/ironrdp-client/src/rdp.rs |
Tests RDPSND backend selection. |
crates/ironrdp-client/src/config.rs |
Synchronizes audio mode and channel state. |
crates/ironrdp-activex/README.md |
Documents audio behavior and manual testing. |
GitHub Actions
Inactive
github-actions
Bot
added
risk/medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The client-side wFormatNo fix is the strongest part of this PR: indexing the client reply list instead of the server list is correct, and replacing the HashSet intersection with an order-preserving Vec makes the index reproducible. The trait break (wave takes &AudioFormat) and DecodeStream::new gaining a volume parameter are justified by that fix. Volume application, capability flags, and the tolerant CryptKey/WaveEncrypt handling are sound. One blocking issue: the new pre-v8 Wave arm decodes WaveInfo and Wave from one buffer, but the repo's own framing shows they arrive as two channel payloads, so real pre-v8 audio is dropped with no Wave Confirm; the new test round-trips IronRDP's combined encoder and cannot catch it. Also bundled: a server-side negotiation strictness change for non-PCM formats with no non-PCM test, an untested apply_volume, and README/test text that entrenches the mode-1/mode-2 conflation.
Protocol analysis: partially_accepted — Confirmed and elevated to blocking: the pre-v8 SNDC_WAVE arm assumes one contiguous payload, contradicted by the repo's own decode (SndWavePdu's 4-byte bPad occupies the RDPSND header slot; data_len = BodySize - 12 counts bytes absent from that payload). Also confirmed: the mislabelled packing comment; the mode-1/mode-2 conflation (REMOTE_CONSOLE_AUDIO exists in ironrdp-pdu, never set); the non-PCM strictness change in negotiate_formats. Rejected or downgraded: the Wave Confirm timestamp delta and linear volume approximation are pre-existing or acknowledged in-source with endpoints preserved; the wTimeStamp widening is inert since no backend reads ts; the ALIVE-with-zero-formats and WaitingForTraining teardown concerns are untouched by this diff.
GitHub Actions
Inactive
github-actions
Bot
added
needs-review
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
removed
the
risk/medium
Keep client format order for wFormatNo, play pre-v8 Wave PDUs, and apply volume on a broader CPAL PCM offer so ActiveX mode 0 can redirect remote audio reliably.
Drop redundant pdu:: prefixes and alias the recorded-wave buffer type so workspace clippy -D warnings passes in CI.
Track interleaved sample phase so odd-length blocks and multi-channel layouts do not swap left/right lanes. Clarify that volume scaling is a simple amplitude map, not a logarithmic MS-RDPEA fidelity model.
Keep stable rustfmt happy for the multi-arg volume helper.
Collapse multi-line lock chains and fix assert indentation so stable rustfmt matches CI.
MS-RDPEA WaveInfo is two PDUs (WaveInfo then bare Wave). Play CPAL streams after open, filter advertised PCM by device support, document the PlayOnServer vs REMOTE_CONSOLE_AUDIO gap, and cover volume scaling in testsuite-core.
CI runs cargo +stable fmt --all; normalize indentation after the playback review fixes so the formatting check passes.
Document that the client and server implement WaveInfo plus bare Wave in addition to Wave2, and retrigger pull_request CI on the tip.
Move CPAL volume helper tests into ironrdp-testsuite-extra so core does not depend on extra-tier native crates. Correct WaveInfo/Wave MS-RDPEA section citations, reject WaveInfo audio lengths that overflow BodySize, confirm short bare Wave payloads, and avoid cloning AudioFormat when playing waves.
971686e to
38896b4
Compare
2d9a9bf
into
master
Add the client MS-RDPEAI capture path on top of hardened RDPSND playback: connector CFG + static channel wiring, CPAL PCM capture backend, ironrdp-client --audio-capture, and ActiveX AudioCaptureRedirectionMode. PCM capture only accepts encode formats that match the Open capture stream, rejects non-16-bit capture (Data PDU size contract), and gates the capture backend behind ironrdp-rdpsnd-native/capture. Depends on #1648 (playback).
Keep client format order for wFormatNo, play pre-v8 Wave PDUs, and apply volume on a broader CPAL PCM offer so ActiveX mode 0 can redirect remote audio reliably.
Also fix clippy noise in the RDPSND client suite and keep interleaved volume L/R phase stable across wave blocks. Volume scaling is a simple amplitude map, not a logarithmic MS-RDPEA model.