docs(rdpsnd): document RdpsndServerHandler::start wFormatNo contract#1343
Conversation
The value returned by `start` is stamped directly onto every Wave/Wave2 PDU as `wFormatNo`. A compliant client resolves each wave's format as `ClientFormats[wFormatNo]` against the list *it* sent in the Client Audio Formats PDU (e.g. FreeRDP's `rdpsnd_recv_wave2_pdu` rejects `wFormatNo >= NumberOfClientFormats` and silently drops all audio). Returning an index into the handler's own `get_formats()` list instead only works when the chosen format happens to occupy the same position in both lists. With several offered formats and a client that accepts a subset, the two lists differ in length and order, so a `get_formats()` index is wrong or out of range and audio dies with no error — a subtle trap the previous (undocumented) signature gave no hint about. Document that the returned index addresses `client_format.formats`, and add brief docs to the trait and its other methods while here. Docs only; no behavior change.
|
This is ready for review whenever someone has a moment. It's docs-only — adds rustdoc to |
There was a problem hiding this comment.
Pull request overview
This PR adds Rustdoc documentation to RdpsndServerHandler, focusing on the contract for start()’s Option<u16> return value so implementers correctly compute wFormatNo for Wave/Wave2 PDUs.
Changes:
- Documented the
RdpsndServerHandlertrait’s role in RDPSND server operation. - Added detailed rustdoc to
get_formats()and especiallystart(), clarifying that the returned index must addressclient_format.formats(the client-echoed list), not the server’s offered format list. - Added a brief doc comment to
stop()describing when it is invoked.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
This is useful documentation, as it’s typical when you had to dive into a rabbit hole because of a an obscure bug. Thank you for contributing it.
I would be more than happy to have a misuse-resistant API instead of the error-prone we have currently (even if it’s now well documented). Generally speaking, I think this would be a superior API on many levels: hard to misuse, self-explanatory types and signature, the crate can also perform additional sanity checks -- e.g.: is the selected format is not part of the common set? On another note, it also makes sense to me if the function only gets to see the audio formats in common with the client, and we can let the crate handle everything else (if there is nothing in common, we don’t even need to call the method in the first place). I guess it depends how far we want to go in the abstraction. I’m unsure whether it can be genuinely useful for the consumers to see the full client format set unconditionally. |
|
Opened #1356 to track the misuse-resistant |
…ndler
The old `start(&ClientAudioFormatPdu) -> Option<u16>` made every handler
compute `wFormatNo` itself, as an index into the *client's* format list.
Getting it wrong (e.g. returning a server-list index) yields
`wFormatNo >= NumberOfClientFormats`, which a compliant client rejects,
silently dropping all audio — and each handler re-implemented the same
server-vs-client intersection.
Move that work into the crate and split selection from lifecycle:
fn choose_format<'a>(&mut self, common: &'a [NegotiatedFormat]) -> Option<&'a NegotiatedFormat>;
fn start(&mut self, format: &NegotiatedFormat);
The crate computes `common` (formats from `get_formats()` the client also
advertised, in the server's preference order, each tagged with its
client-list `wFormatNo`), calls `choose_format`, then `start` with the
chosen format. `NegotiatedFormat` has a private `wformat_no` and no public
constructor, and `choose_format` returns a borrow of an element of `common`,
so a handler cannot pick a format the client did not accept nor emit an
out-of-range `wFormatNo`. `choose_format` is not called when nothing is in
common. Separating `choose_format` (pure selection) from `start` (encoder
init / producer startup) keeps the two concerns distinct.
Resolves the footgun documented in Devolutions#1343. Closes Devolutions#1356.
BREAKING CHANGE: `RdpsndServerHandler::start` is replaced by `choose_format`
(selection) plus `start(&NegotiatedFormat)` (lifecycle). Implementors now
return a `&NegotiatedFormat` from `choose_format` instead of computing a
`wFormatNo`.
- Add `NegotiatedFormat` (private `wformat_no`, `format()` accessor) and
`negotiate_formats` (intersection + client-index mapping), with unit tests
for client-index mapping, the PCM-only/AAC-first regression, empty overlap,
and WAVEFORMATEX-identity equality.
- Match formats on WAVEFORMATEX identity (tag/channels/rate/bits), ignoring
derived and codec-`data` fields a client may not echo verbatim.
- Update the example server to the new two-method shape.
What
RdpsndServerHandlerand its methods are currently undocumented. This PR adds rustdoc to the trait, with the load-bearing detail onstart's return value.Why
The
Option<u16>returned bystartis stamped directly onto everyWave/Wave2PDU aswFormatNo(RdpsndServer::wave). The non-obvious part — with no doc to warn you — is which list that index addresses:ClientFormats[wFormatNo]against the list it sent in the Client Audio Formats PDU, and rejects anywFormatNo >= NumberOfClientFormats. FreeRDP does exactly this inrdpsnd_recv_wave2_pdu(andrdpsnd_recv_wave_info_pdu) — out-of-range is dropped, so all audio silently disappears.get_formats()in both length and order. Returning aget_formats()index only works when the chosen format happens to sit at the same position in both lists.This bites the moment a handler offers more than one format and a client accepts only some of them: indexing
get_formats()sends the wrong (or out-of-range)wFormatNo, and the failure mode is silent — no error, just no sound. We hit this in a downstream server when adding a second (AAC) format alongside PCM: mstsc kept working (it lists the chosen format at the same index in both), but FreeRDP/Thincast clients that accepted only PCM got an out-of-range index and went silent. Picking the format and then returning its position withinclient_format.formatsfixes it.The doc makes the contract explicit so the next implementer doesn't have to rediscover it from a wire capture.
Notes
cargo check -p ironrdp-rdpsndandRUSTDOCFLAGS="-D warnings" cargo doc -p ironrdp-rdpsnd --no-depsboth pass; no reformatting fromcargo fmt.startreturn the chosenAudioFormat(or itsclient_format.formatsindex) and letting the crate compute/validatewFormatNo. That's an API/semver change, so I kept this PR to docs only.